Subject: [Gen-art] Gen-art review of draft-ietf-savi-dhcp-12

From: Elwyn Davies <[email-masked]>
Date: Fri, 23 Mar 2012 00:01:08 +0000

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.