For the record: Version 15 of draft-ietf-savi-dhcp from 2012-09-11 addressed to some extent most of the issues listed here. However the points in the summary were not covered and the document is still difficult to understand and I would be doubtful that interoperable implementations could be generated from the specification as it stands. I understand that this is hopefully being addressed at present and I will carry out a fuller re-review when this work is published. I would be grateful if the authors or document shepherd could ping me when this is done as gen-art reviewers are not on the usual automatic notification list. Regards, Elwyn On Wed, 2012-08-29 at 15:15 +0100, Elwyn Davies 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-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? >