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 protected]> wrote: > 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!) > _______________________________________________ Gen-art mailing list [email protected] https://www.ietf.org/mailman/listinfo/gen-art
