I am the assigned Gen-ART reviewer for this draft. For background on Gen-ART, please see the FAQ at < http://wiki.tools.ietf.org/area/gen/trac/wiki/GenArtfaq>. Please resolve these comments along with any other Last Call comments you may receive. Document: draft-ietf-savi-dhcp-12.txt Reviewer: Elwyn Davies Review Date: 22 March 2012 IETF LC End Date: 20 March 2012 IESG Telechat date: (if known) - Summary: Not ready. I regret to say that this document is perhaps the least well-structured draft that I have reviewed over the last 7 years. As documented below it appears to have a sizeable number of (probably) minor issues together with a myriad of editorial nits, I haven't tried to document the language nits as this would be far too burdensome. As regards how to proceed, I would consider discussing the attributes (i.e. the link/anchor type classification first). Then get concrete about the relevant control messages and their directions (client->server, server->client) Then the state machine which applies only to SAVI-Validation class doing the usual trick of giving an outline before going into gory detail (currently s7.). Error transitions need more attention. All in all, the plethora of forward references to things that haven't been introduced properly needs to be fixed. Nits/editorial comments: General: The draft *desperately* needs a general editorial pass from a English mother tongue editor. There are many instances of missing articles, wrong multiplicity, erroneous words etc that distract from easy reading, e.g., s1 para 1: OLD: This document describes the procedure for creating binding between DHCP address and binding anchor on SAVI device [I-D.ietf-savi- framework]. The removal and restoration of the bindings are also specified in this document. NEW This document describes the procedure for creating a binding between an address allocated to a network attachment point by DHCP and a suitable binding anchor on a SAVI device [I-D.ietf-savi- framework]. The removal and restoration of such bindings are also specified in this document. I do not have time or energy to list all the problems here. Some of them appear as issues below. Issues: Abstract: The abstract should not contain references. s1. The first paragraph of s1 is very difficult to parse. Checking up in the SAVI framework, the term 'binding anchor' (which should really have its definition copied here) is supposed to be a link layer property of the host's network attachment that is connected to the IP address. A possible rewording in shown above, but in any case the term 'DHCP address' in this context is unclear - it could mean the DHCP server/relay address instead of what I take it is meant (as indicated much better in the abstract) , i.e., the address allocated by the DHCP server to the network attachment point. A pointer to the list of possible types of binding anchor in s3.2 of the framework might help a bit. s1, para 2/s15.2: The reference for IP Source Guard is incomplete (it doesn't specify the draft name). s1, paras 4 and 5: Reversing the order of these paragraphs would make better sense (general -> particular rather than vice versa). s4, para1: s/At least one DHCP server must be deployed in the network, and DHCP relay may be used to.../One or more DHCP servers mediate the allocation and distribution of IP addresses to hosts requesting them using the DHCP protocol. DHCP relays may be needed to../ s4, para 1: What is a SAVI device? The picture doesn't help. s4, para 3: SUGGESTED is not an RFC2119 term. s/SUGGESTED/RECOMMENDED/ perhaps. Figure 1: The figure is poorly drawn with inconsistent use of shapes (router A and router B differ in representation). My previous understanding was that the protection perimeter would effectively enclose at least Client A assuming that is to be 'protected' from spoofing. I presume there are supposed to be connections between (e.g. SAVI Device A and Router B) but the character selected is inappropriate). A key to the shapes/devices would be helpful. s5: 'Two main data structures': Three are described in s5. Which one is NOT a main data structure? s5: The discussion of the BST and the FT as control plane and data plane representations strikes me as irrelevant to the definition of the mechanism. This is purely an implementation decision. The BST is created. If the implementation chooses to clone certan columns of the table from the control processor to the forwarding processor (which is what seems to be implicit in the discussion), so be it, but it need not concern this document. There seems to be some implication (see the comment on s5.2 below) that moving the data from BST to FT is not a simple matter of copying, but since nothing is said about this other than the oblique reference in s5.2, I guess we assume that the FT is just a copy of the relevant columns in the BST for which the state is BOUND. But of course we haven't defined what goes in the state column yet. Doh! s5.1, para 1: Expansion of TID acronym comes after first usage. s5.1: The values in the State column are not properly defined until s7.2. s5.2: The last sentence claims that the FT table should be updated by 'some other means' in an IPv6 environment 'as explained in s4'. I can't see any such explanation. s5.3: Why should this draft seek to constrain the implementation of the proposal by mandating that a SAVI device 'MUST NOT set up' a binding table that was not previously required? This seems to me to be an implementation decision if it turns out that this is the most convenient way to implement the proposal. s5.3: Forward reference to event EVE_DCP_REPLY_NULL. At this point in the document the reader has no idea what this acronym is/stands for.. and it isn't explained here either. s6: A sentence explaining what the purpose of the attributes is supposed to be would be useful. s6, para 2: "Attribute of each binding anchor is configurable.": How? And when? Is this expected to be a manual or management protocol process? s6: Setting out the mutual exclusions as a table would help. Currently only one end of the relationship is specified (e.g., SAVI-SAVI is also mutually exclusive with SAVI-BindRecovery but this is specified only in the second one (s6.5) and not in the first (s6.4)). Messy! This table could also explicitly show what attributes *are* compatible. Its rather a matter of guesswork at the moment. s6.x: the phrase 'server(or server/relay type) DHCP message is not properly defined. Presumably it means messages expected to be originated by a DHCP server or relay. Maybe a list of acceptable messages would be helpful. s6.1: 'To filter bogus DHCP server message by default': How does the device know that the server message is bogus? Does this include DHCP messages coming from a link with the SAVI-DHCP-Trust attribute that are headed out onto this unconfigured link? Or is this only talking about incoming DHCP server messages? Possibly this is implied by the last sentence. s6.3: So for example, if a malicious node can get a spoofed DHCP server or other control message into Router A (since it is a router I guess we must assume there are other connections to it than just the DHCP server) headed towards SAVI Device B, it will be passed on/acted on? Does this matter? Or is this a limitation? Contrast this with the statement at the end of S6.1 regarding the link from SAVI Device A to Router B. s6.4: '...from which the data traffic is not to be checked.': What about control messages? s6.5: It would be sensible to explain here why the device needs to do BindRecovery rather than directing us to s8.1. s7: Please say it is a state machine. s7.1: I don't think that you can 'attach a node to a binding anchor'. The anchor is a property of the link. the node is attached to a link that is associated with the binding anchor. s7.3.2: The data value 'binding entry count' associated with each binding anchor would be best described as part of the state machine rather than just in the security section where it looks a bit like an afterthought. s7.3.2: Do we know that all possible/relevant control messages are covered here? I note that the constraints in s9.2 mentioned in para 1 only cover a small subset of these messages. It is written as if s9.2 applies to all the messages. Also s9.2 covers other control messages than are mentioned in s7.3.2. In particular are their control messages form RFC 3736 that need to be explicitly mentioned (so as to be ignored or cause an error?) s7.3.2/s7.5: What happens if a control message arrives but does not generate a valid event? Does this have any effect on the state machine? Is it an error transition (especially if the binding is in the INIT_BIND state)? s7.4.x.2: What happens if a man-in-the-middle or other attack generates a DHCP server message using the correct TID but with bogus data? In Figure 1, if router A or the link to the DHCP server is compromised, does the whole house of cards come tumbling down possiblluy? This needs to be discussed in the security section. s7.4.x.2: RFC 5007 is IPv6 specific. Should mention RFC 4388 for IPv4. s7.4.x.2: DHCP LEASEQUERY requests SHOULD be IPsec protected (not surprisingly) - is this envisioned? s7.4.3.2: I was wondering if it might be desirable to allow a small amount of leeway on expiration? Doing expiry at exactly the lifetime could mean that a renewal was missed. s8.1: What is the rationale for doing the queries twice in stage 1? s9.2, last para: ARP messages with link-local addresses? ARP is IPv4 specific - does this mean IPv4 link local (169.254/16) addresses or is this some sort of confusion with IPv6 link local? s10: The loss of attribute 'configuration' would seem to be a particular problem that is not discussed here. s13: Discussion of TID spoofing? s13: Securing LEASEQUERY requests? s15: As of this moment RFC 3736 is probably not normative since it is just not relevant to a stateful system. That might change if all its messages are considered.