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

From: Elwyn Davies <[email-masked]>
Date: Mon, 01 Sep 2014 13:11:36 +0100

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!)

---

From: Ted Lemon <[email-masked]>
Date: Thu, 4 Sep 2014 11:16:47 -0400

Thanks for the careful review, Elwyn.   The document is undergoing a new english language pass and some additional rework.   I've pushed the document out to the next telechat because that work isn't done yet and I think addressing the points you've raised will make the document easier for the IESG to review as well.

Authors: it looks like Elwyn has identified a couple of issues we weren't previously thinking about that are worth addressing, and should be straightforward to address.   Can you address them?

On Sep 1, 2014, at 8:11 AM, Elwyn Davies <[email-masked]> wrote:

> 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:
> 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.
> 
> 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:
> 
> 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:
> 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:
> 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:
> 
> 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:
> It would be clearer to use 'empty attribute set' instead of 'no attribute' throughout.
> 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:
> '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.
> 
> 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!)
>

---

From: Elwyn Davies <[email-masked]>
Date: Thu, 04 Sep 2014 17:27:51 +0100

Ack.

Will look for the updated version (and I guess rereview!)

/Elwyn

---

From: "Fred Baker (fred)" <[email-masked]>
Date: Thu, 4 Sep 2014 19:56:19 +0000

Sure