Telechat Review of draft-ietf-savi-dhcp-29
review-ietf-savi-dhcp-29-genart-telechat-davies-2013-02-27-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 wait for direction from your document shepherd
or AD before posting a new version of the draft.

Document: draft-ietf-savi-dhcp-29.txt
Reviewer: Elwyn Davies
Review Date: 19 August 2014
IETF LC End Date: -
IESG Telechat date: 4 September 2014

Summary:


This draft still has several open issues some of which could be 


classified as serious issues.  There are also several areas, especially 


in the specification of the state machine (or is it machines) where the 


document does not lend itself to the making of reliably interoperable 


implementations.




Major issues:
General: I noted in my two previous reviews of this document back in
2012 that the document was (desperately) in need of a general editorial
pass from an English mother tongue editor.  This has only partially been
carried out and the document still contains many grammatical errors.
Regrettably, the effect of this unclear and erroneous language makes
some areas of the document technically ambiguous and other areas


unnecessarily difficult to understand for readers. Additionally the 


structuring of certain parts (especially specification of the the state 


machine(s)) really needs major rework to make it readily implementable.






s4.3.5/s11.2:  Of the six possible properties that could be used to set 


up binding anchors, only two are discussed in this section.  What about 


the other four?  Are they usable with SAVI-DHCP or not?   I am 


particularly concerned about how SAVI-DHCP interacts with wi-fi networks 


which seems not to be considered at all - rather the pictures and 


descriptions all seem to imply wired networks with a one-to-one 


relationship between connected devices and SAVI device ports.  The 


problem of 'leaving clients' (not a very felicitous phrase) with 


networks other than one-to-one direct attachments is left open.  The 


wording at the end of s11.2, seems to indicate that SAVI-DHCP may not be 


useful other than for with one-to-one direct attachment.




s3/s4 (and elsewhere): The definitions in s3 and the descriptions of the
need to distinguish between upstream and downstream links would appear
to rule out the use of SAVI as currently specified in situations where
transit traffic and destination traffic share the same link.  Presumably


this rules out quite a few wi-fi networks, for example IPv6 networks 


with multiple subnets per link and possibly situations where there are 


multiple routers in series.






s7.5.1.2:  The local conflict resolution process will not work on wi-fi 


networks AFAICS because there is no way to stop the detection messages 


being sent to the node that originated the unmatched message.  Is this a 


problem or not?  Some discussion is desirable in any case.




Minor issues:

Introductory boilerplate: Since the earliest date on versions of this
draft is 2009, it is unclear why the pre-2008 material disclaimer is
relevant.  Could this be explained or removed if not necessary, please.

s1: The reference to RFC 2827 (aka BCP38) might better refer to the more
extensive RFC 3704 (BCP 84) - and, in any case, use the BCP reference
rather than the RFC to cover any future update.



s4.2.5/s4.2.6:  It is not clear to me why there needs to be a separate 


VALIDATING attribute.  It is required to be set with DHCP-Snooping and 


Data-Snooping and is only optional with DHCP-Trust.  However, it is 


unclear under what circumstances there could be any binding entries on 


an attachment that is only configured with DHCP-Trust since binding 


entries can only be created as a result of having one of the *-Snooping 


attributes set.  Consequently it seems that all that is needed is for 


the two *-Snooping attributes imply the effect of the VALIDATING attribute.






s6.4.2.2, item 2.1: Does there need to be any distinction between 


permanent and temporary addresses (RFC 3315 ss22.4 and 22.5)?  In 


particular, RFC 3041 indicates that temporary addresses may remain in 


use as source/destination addresses after their notional lifetimes have 


expired if a connection for which they were used remains open after the 


expiry.  Also the timeout parameters on permanent and temporary 


addresses are different in DHCPv6 (RFC3315 again).






s6.4.2.2/s11:  RFC 5007 recommends that LEASEQUERY/LEASEQUERY_REPLY 


messages should be protected by IPsec as described in s21 of RFC 3315 


when used by devices that are similar in operation to SAVI-DEVICEs (RFC 


5007 s5).  It is quite likely that DHCP servers would not answer 


LEASEQUERY messages from addresses that are not IP addresses leased by 


the server unless the messages were received over a trusted IPsec 


connection.  This issue is not discussed in the draft.  The suggestion 


in RFC 5007 indicates that pre-shared keys would be feasible because of 


the small number and relatively stable association of DHCP server(s) and 


SAVI-DHCP devies. This also means that sending LEASEQUERY to the 


ALL_DHCP_SERVERS multicast address is quite likely not to elicit responses.






s7: The previous comment applies to the use of DHCPLEASEQUERY and 


LEASEQUERY in the Data-Snooping state machine also.






s6.4.2: Forwarding of DHCP messages: There is some inconsistent logic 


here - some messages that are snooped on are specifically noted as 'MUST 


be forwarded' whereas nothing is said about forwarding messages that are 


not 'specially processed'.  The SAVI-DEVICE is *snooping* on all these 


messages; thus all of the messages apart from LEASEQUERY_REPLYs 


resulting from LEASEQUERY requests generated by the SAVI-DEVICE MUST be 


forwarded to their intended destination.  This could be covered by a 


general statement about all DHCP messages passing through the 


SAVI-DEVICE, eliminating all the confusing statements about certain 


messages having to be forwarded.






s7: Specifying the Data-Snooping state machine as a modification of the 


DHCP-Snooping state machine is confusing.  If they really can be 


combined it would be much nicer to specify the machine in one place and 


have done with it.  If not they should be specified separately.  It is 


currently difficult to know if the combined machine will work correctly 


if it gets driven by a combination of DHCP and data snooping events.  In 


particular, if the s7 machine really is a superset of the s6 machine the 


events EVE_DHCP_RELEASE, EVE_DHCP_DECLINE and TIMEOUT (shown in Figure 


10) should trigger a transition back to the NO_BIND state but they 


aren't mentioned in Figure 14.




s8, para 2:



DHCP and NDP (Neighbor Discovery Protocol) [RFC4861]
   messages that may cause state transit are classified as control
   packet.


So does this mean that DHCP and NDP messages that do not affect the 


state machines are treated as data packets? This may or may not be 


inconsistent with the statements in s8.2.




s8.1/s8.2:



The SAVI device MAY record any violation.



Is any mechanism or suggestion needed to avoid log overload?


Also the term 'violation' is not explicitly defined.  Presumably it 


means any packet that is dropped as a result of failing validation.




Nits/editorial comments:

s1: definition of "binding anchor":  Although binding anchors are
discussed in detail in Section 3 (which is pointed to) the fundamental
nature of "binding anchor" in this work means that an outline definition
is desirable in s1.

s1, para 1:
Strictly, the mechanism does not affect the validity or otherwise of the
source IP addresses used, but provides a validation mechanism that
checks that validity.  I suggest:
OLD:
   Compared with [RFC2827], which provides
   prefix granularity source IP address validity, this mechanism can
   benefit the network with finer-grained validity and traceability of
   source IP addresses.
NEW:
   Compared with [RFC2827], which provides validation of
   source IP addresses at the granularity of subnet prefixes, this
   mechanism can benefit the network with finer-grained validation and
   traceability of the origin of forged source IP addresses.

s3:



Binding anchor: A "binding anchor" is defined to be a link layer
property of network attachment in [RFC7039].  A list of proper
binding anchors can be found in Section 3.2 of [RFC7039].



Three issues:
- This definition is subtly different from the form in RFC 7039 (s2,
bullet 2).  There a binding anchor is not a separate link layer property
but an *indirection* to an existing link layer property with the
appropriate value for a host's network attachment bound to the
legitimate IP address in such a way that a SAVI mechanism can verify
legitimate usage of the IP address in the link layer.
- I would suggest that 'proper' is not the appropriate word.  'Possible'
seems more in keeping with s3.2 of RFC 7039.
- RFC 7039 uses "link-layer" rather than "link layer".  This draft has
both (c.f. s5, bullet 1).  Please be consistent - matching RFC 7039 is


probably best so select "link-layer".  There are also three instances of 


layer-2 in s7.1; these should also be link-layer..



Suggested rewording:
   Binding Anchor: A selected link-layer property of a host's network
   attachment that can be verifiably bound to the legitimate IP address
   associated with that attachment point as described in Section 2 of
   [RFC7039].  Section 3.2 of [RFC7039] has a list of possible
   properties that might be used as binding anchors.

s3:



Attribute: A configurable property of each network attachment which
indicates the actions to be performed on packets received from the
network attachment.



This is confusing because network attachment is not associated with a
device here.  Presumably it is the SAVI device that will be doing the
actions whereas the binding anchor has talked about the host's network
attachment.

s3, Upstream links: As written this appears to imply that these links
are between non-SAVI devices.  I take it that what is really meant is
links between a SAVI device and a non-SAVI device outside the monitored
network (i.e., not a monitored host or an access device as mentioned in
the Indirect Attachment definition).  The same implication is, to a
lesser extent, true of the Downstream links definition.

s3, CUT VERTEX:  Why is this term all in upper case? The definition uses
lower case.

s4, Figure 1: It would avoid confusing the association of the 'upstream
link' label if it was placed outside the 'Protection Perimeter' outline.
My initial reading was that 'upstream link' labelled the perimeter box.
 It might also be less confusing if the horizontal lines of the
perimeter box used a less dense line (e.g., . . . . . instead of
.........) as the difference between the perimeter and the edges of
devices is not very clear at a distance.

s4:



Networks are not isolated and traffic from other networks, i.e.,
transit traffic specified in [RFC6620], may get into the network
with SAVI-DHCP deployed through the upstream links.  Since SAVI
solutions are limited to check traffic generated from local link,
SAVI-DHCP is not to set up bindings for addresses assigned in other
networks.




Presumably in a network that deals with end hosts as shown in Figure 1
there is likely to be quite a lot of traffic coming from other networks
that isn't transit traffic.  Singling out transit traffic is confusing
particularly in the Figure 1 example since the complete SAVI protection
perimeter and the devices not on the upstream link are a 'leaf' network
with no outlets to other networks.  So I guess it should be e.g. rather
than i.e., and it would be good to actually mention the traffic destined
for the validated end hosts!

s4.2, para 2:



Before configuration, an attachment is with no attribute.  An
attachment MAY be configured to have one or more compatible
attributes (refer to Section 4.2.6).



s/is with no attribute/starts out with an empty attribute set/


It would be clearer to use 'empty attribute set' instead of 'no 


attribute' throughout.



s/MAY/can/ - this is something the administrator controls and not an
implementation constraint for the mechanism.

s4.2, para 2:



The attributes of each attachment MUST be configured before the
SAVI-DHCP function is enabled.



Again this is an administrator control and not an RFC 2119 requirement,


but as written it appears to make it impossible to reconfigure a 


SAVI-DEVICE without disabling and reenabling SAVI-DHCP.  Not very 


operationally friendly!






s4.2, para 3: s/SUGGESTED/RECOMMENDED/ - SUGGESTED is not in the RFC 


2119 set. (also applies to s6.4.2.2 item 2.1 - but SUGGESTED is spelt 


SUGGESUTED here).






s4.2.1: It might be desirable to rename the 'Trust' attribute as 


'Full-Trust' to make it clear what it means and avoid issues with 


misremembered configurations.




s4.2.2, last para:



   The implementation for DHCPv6 can refer to
   [I-D.ietf-opsec-dhcpv6-shield] for more details.


'implementation'??? This does not refer to an implementation of the 


SAVI-DHCP function.  I presume the draft is suggesting that SAVI-DHCP 


might be combined with a deployment of the DHCPv6 Shield technology on 


relevant links.




s4.3.5, para 1: s/By default,t his/By default, this/



s6.4.1.2: Given the distance between s2 and this section, it would be 


useful to expand IA on its first use in this section.






ss6.4.2.2/6.4.3.2:  It would be much clearer if the new state for the 


'Following Actions' in each case was given an explicit "New State" line 


and also separating the different events into separate subsections (OK I 


know it is then 5 levels deep, but that is the way of state machines!)