Last Call Review of draft-ietf-savi-dhcp-
review-ietf-savi-dhcp-genart-lc-davies-2012-05-17-00

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?
>