Subject: [Gen-art] Gen-art telechat review of draft-ietf-savi-dhcp-14

From: Elwyn Davies <[email-masked]>
Date: Wed, 29 Aug 2012 15:14:58 +0100

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?