Sure On Sep 5, 2014, at 1:16 AM, Ted Lemon <[email protected]> wrote:
> 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!) >> >
signature.asc
Description: Message signed with OpenPGP using GPGMail
_______________________________________________ Gen-art mailing list [email protected] https://www.ietf.org/mailman/listinfo/gen-art
