Last Call Review of draft-ietf-savi-dhcp-
review-ietf-savi-dhcp-genart-lc-davies-2012-03-23-00

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.