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-14.txt Reviewer: Elwyn Davies Review Date: 28 August 2012 IETF LC End Date: - IESG Telechat date: 30 August 2012 Summary: Not ready. The explanation of SAVI Protection Perimeters and other matters in s2 of RFC 6620 is very significantly better than the work in this draft, and appears in many ways to be equally applicable. Either duplicating and adapting the RFC 6620 work in this draft or, better still, referencing it so far as is possible and providing some differences would be a significant improvement for this draft. There are still a significant number of language issues including a large number of missing indefinite articles, not all of which I have noted below. Major issues: Minor issues: s1, para 3: Suggestions for other SAVI mechanisms would be a good idea. I think FCFS is mentioned later. s5.1, para 2: > Considering the trivial of configuration, it is expected SAVI-DHCP > function will not cause a break of the network even though an > attribute is not configured. Thus, data packet filtering will not be > performed on binding anchor with no attribute. I cannot parse the first sentence of this para. It strikes me that having a default of 'let everything through' is not very robust. This is discussed in RFC 6620 where ports (which are actually what get attributes in most cases) are either Trusted or not. s6.1: I failed to note during LC that recording the Lifetime of a binding is not very useful. If recording the Lifetime is needed then one really wants to record the expiry time of the binding (i.e. the DHCP lease) - logically recording the Lifetime is useless without the start time or the implementation has to be specified to count down the lifetimes of all bindings. s6.2: Why should this draft seek to constrain the implementation of the proposal by mandating that a SAVI device 'MUST only use' a binding table that is not necessarily associated with the SAVI code? Leave it to the implementor to decide. s7.3.2, s7.4.1.2 and s13.2 (and other places): The concept of 'binding entry limit' is not defined. It is unclear in s7 whether this is supposed to be a per anchor or per device limit. It is also unclear what the purpose of having such a limit is until s13.2. The text in s7.3.2: > For any message that may > trigger a new binding, the binding entry limit (cf. Section 13.2) on > the corresponding binding anchor MUST NOT have not been reached. implies a per-anchor limit but apparently conflicts with the text in s7.4.1.2 which might imply a per device limit: > The binding entry limit can be exceeded when setting up bindings for > all addresses in a REPLY message. If there is enough binding entry > resources, corresponding new entries MUST be generated even when the > binding number limit is exceeded. In case that there is not enough > resources left, the message MUST be discarded. The MUST NOT in the s7.3.2 is not a protocol specification issue - the SAVI node has no control over how many addresses are given to an interface by DHCP so that the count can exceed the specified max - SAVI just has to cope with this. S7.3.2 should refer to s13.2 to explain that excessive bindings is a potential security risk. Altogether this sections need to be made consistent. In s13.2 a mitigation for binding entry limit overload is suggested to be limiting the DHCP Request rate. It is unclear that SAVI has any control over this - or is it intended that SAVI should monitor the request rate and discard excess messages? s7.4.2.2: (DHCP)LEASEQUERY requests SHOULD be authenticated (probably with IPSEC - not surprisingly) - is this envisioned? The answer to this question when asked in the previous review misinterpreted the question. A discussion of how LEASEQUERY messages will be authenticated is required. Nits/editorial comments: Abstract: s/on SAVI/on a SAVI/ Abstract; s1 para 1; and more generally: 'filter' - to what end? filter just means distinguish the packets - 'identify', 'reject' or 'discard' is what you mean maybe. More generally in this document the use of 'filter' with the implicit (I take it) meaning of discard is confusing. There is a particular case in the second para of s5.1 where this is problematic. Abstract: Links do not generate packets. Abstract: s/complement of/complement to/ s1, para 2: The term 'binding recovery procedure' is not defined. It would be useful to have some idea of why we want to define one before saying it isn't discussed in [BA2007]. s1, para 3: 'only DHCP addresses': Presumably you mean again 'IP addresses assigned to an interface via DHCP' . It would probably be helpful to define a term for this rather than assuming 'DHCP address'. s3: 'SAVI-DHCP': I am unclear what is being named here. Is this the whole set of devices or a mechanism? What is a 'SAVI instance'? Do you mean 'SAVI capability'? - also applies to definition of '(Non-)SAVI device' definitions. s4, para 1: s/DHCP relay/A DHCP relay/ s4, para 2: > Protection perimeter (refer to Section 4 in [savi-framework]) is > formed by SAVI Device A and SAVI Device B for scalability. What has scalability to do with this? s6.2, para 1: The two sentences need to be interchanged so that the description of the table content comes first. s5.1, para 1: > thus, server type DHCP messages > from the binding anchor with no attribute will be discarded. For consistency this should be about 'DHCP server/relay' messages. s5.1, para 2: > For example, in Figure 1, on the binding anchor of SAVI Device A > connected to Router B, it is unreasonable to set up bindings for the > host behind Router B, or filter out traffic from Router B, or allow > forged DHCP message from Router B. Thus, no attribute should be > configured on this binding anchor. Some explanation of why it is unreasonable to do these things would be desirable. After reading this several times, I think that is trying to say that we have to let all the data packets through but discard (bogus) DHCP messages, but I am not sure about this. As I mentioned in the previous review it is unclear how bogus DHCP messages would be distinguished in a case where the binding anchors are not configured. s5.2: > SAVI-Validation attribute is used on a binding anchor on which the > source address of data packet and DHCP message is to be validated. > The filtering process on the binding anchor with such attribute is > described in section 9. Section 9 talks about 'data packets and control packets'. The latter are more than just DHCP messages. s5.4, para 3: > However only when a > binding anchor is on the protection perimeter and connected to > another link, then it can have no attribute after configuration. I don't understand the phrase' connected to another link. A binding anchor is associated with a link/interface... what is connected to another link? s7: It would be worth stating that the procedure uses a state machine. s7.3.2: As noted in the previous review, the term 'control messages' from s9.2 covers NDP messages as well as DHCP messages. It would be clearer to use server/relay DHCP message term as only DHCP mesages cause events. s7.3.2/s7.5: What happens if a control message arrives but does not generate a valid event? The authors made a statement about this in response to a previous review but have not specified what happens in the document (apparently discarding the event with no action). s7.4.1.2: > If the binding anchor is not a link layer address and there is not a > mapping table from the link layer address to the binding anchor, the > message SHOULD be discarded. What else might happen here if the message is NOT discarded? It strikes me that this is a node misconfiguration rather than a protocol error. s7.4.2.2: > If there is no such an entry, the SAVI device > MUST send a LEASEQUERY [RFC5007] message querying by IP address to > All_DHCP_Relay_Agents_and_Servers multicast address [RFC3315] or a > configured server address. RFC 5007 is IPv6 specific. If the EVE_DHCP_REPLY_NOLEASE event is generated as a result of a DHCPv4 event then the relevant RFC is 4388 and it sends DHCPLEASEQUERY messages. s8.1: What is the rationale for doing the queries twice in stage 1? - needs explaining still. s13: Discussion of TID spoofing? _______________________________________________ Gen-art mailing list [email protected] https://www.ietf.org/mailman/listinfo/gen-art
