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 resolve these comments along with any other Last Call comments
you may receive.
Document: draft-ietf-savi-dhcp-12.txt
Reviewer: Elwyn Davies
Review Date: 22 March 2012
IETF LC End Date: 20 March 2012
IESG Telechat date: (if known) -
Summary:
Not ready. I regret to say that this document is perhaps the least
well-structured draft that I have reviewed over the last 7 years. As
documented below it appears to have a sizeable number of (probably)
minor issues together with a myriad of editorial nits, I haven't tried
to document the language nits as this would be far too burdensome.
As regards how to proceed, I would consider discussing the attributes
(i.e. the link/anchor type classification first). Then get concrete
about the relevant control messages and their directions
(client->server, server->client) Then the state machine which applies
only to SAVI-Validation class doing the usual trick of giving an outline
before going into gory detail (currently s7.). Error transitions need
more attention. All in all, the plethora of forward references to
things that haven't been introduced properly needs to be fixed.
Nits/editorial comments:
General: The draft *desperately* needs a general editorial pass from a
English mother tongue editor. There are many instances of missing
articles, wrong multiplicity, erroneous words etc that distract from
easy reading, e.g., s1 para 1:
OLD:
This document describes the procedure for creating binding between
DHCP address and binding anchor on SAVI device [I-D.ietf-savi-
framework]. The removal and restoration of the bindings are also
specified in this document.
NEW
This document describes the procedure for creating a binding between
an address allocated to a network attachment point by DHCP and a
suitable binding anchor on a SAVI device [I-D.ietf-savi-
framework]. The removal and restoration of such bindings are also
specified in this document.
I do not have time or energy to list all the problems here. Some of
them appear as issues below.
Issues:
Abstract: The abstract should not contain references.
s1. The first paragraph of s1 is very difficult to parse. Checking up
in the SAVI framework, the term 'binding anchor' (which should really
have its definition copied here) is supposed to be a link layer property
of the host's network attachment that is connected to the IP address. A
possible rewording in shown above, but in any case the term 'DHCP
address' in this context is unclear - it could mean the DHCP
server/relay address instead of what I take it is meant (as indicated
much better in the abstract) , i.e., the address allocated by the DHCP
server to the network attachment point. A pointer to the list of
possible types of binding anchor in s3.2 of the framework might help a bit.
s1, para 2/s15.2: The reference for IP Source Guard is incomplete (it
doesn't specify the draft name).
s1, paras 4 and 5: Reversing the order of these paragraphs would make
better sense (general -> particular rather than vice versa).
s4, para1: s/At least one DHCP server must be deployed in the network,
and DHCP relay may be used to.../One or more DHCP servers mediate the
allocation and distribution of IP addresses to hosts requesting them
using the DHCP protocol. DHCP relays may be needed to../
s4, para 1: What is a SAVI device? The picture doesn't help.
s4, para 3: SUGGESTED is not an RFC2119 term. s/SUGGESTED/RECOMMENDED/
perhaps.
Figure 1: The figure is poorly drawn with inconsistent use of shapes
(router A and router B differ in representation). My previous
understanding was that the protection perimeter would effectively
enclose at least Client A assuming that is to be 'protected' from
spoofing. I presume there are supposed to be connections between (e.g.
SAVI Device A and Router B) but the character selected is
inappropriate). A key to the shapes/devices would be helpful.
s5: 'Two main data structures': Three are described in s5. Which one is
NOT a main data structure?
s5: The discussion of the BST and the FT as control plane and data plane
representations strikes me as irrelevant to the definition of the
mechanism. This is purely an implementation decision. The BST is
created. If the implementation chooses to clone certan columns of the
table from the control processor to the forwarding processor (which is
what seems to be implicit in the discussion), so be it, but it need not
concern this document. There seems to be some implication (see the
comment on s5.2 below) that moving the data from BST to FT is not a
simple matter of copying, but since nothing is said about this other
than the oblique reference in s5.2, I guess we assume that the FT is
just a copy of the relevant columns in the BST for which the state is
BOUND. But of course we haven't defined what goes in the state column
yet. Doh!
s5.1, para 1: Expansion of TID acronym comes after first usage.
s5.1: The values in the State column are not properly defined until s7.2.
s5.2: The last sentence claims that the FT table should be updated by
'some other means' in an IPv6 environment 'as explained in s4'. I can't
see any such explanation.
s5.3: Why should this draft seek to constrain the implementation of the
proposal by mandating that a SAVI device 'MUST NOT set up' a binding
table that was not previously required? This seems to me to be an
implementation decision if it turns out that this is the most convenient
way to implement the proposal.
s5.3: Forward reference to event EVE_DCP_REPLY_NULL. At this point in
the document the reader has no idea what this acronym is/stands for..
and it isn't explained here either.
s6: A sentence explaining what the purpose of the attributes is supposed
to be would be useful.
s6, para 2: "Attribute of each binding anchor is configurable.": How?
And when? Is this expected to be a manual or management protocol process?
s6: Setting out the mutual exclusions as a table would help. Currently
only one end of the relationship is specified (e.g., SAVI-SAVI is also
mutually exclusive with SAVI-BindRecovery but this is specified only in
the second one (s6.5) and not in the first (s6.4)). Messy! This table
could also explicitly show what attributes *are* compatible. Its rather
a matter of guesswork at the moment.
s6.x: the phrase 'server(or server/relay type) DHCP message is not
properly defined. Presumably it means messages expected to be
originated by a DHCP server or relay. Maybe a list of acceptable
messages would be helpful.
s6.1: 'To filter bogus DHCP server message by default': How does the
device know that the server message is bogus? Does this include DHCP
messages coming from a link with the SAVI-DHCP-Trust attribute that are
headed out onto this unconfigured link? Or is this only talking about
incoming DHCP server messages? Possibly this is implied by the last
sentence.
s6.3: So for example, if a malicious node can get a spoofed DHCP
server or other control message into Router A (since it is a router I
guess we must assume there are other connections to it than just the
DHCP server) headed towards SAVI Device B, it will be passed on/acted
on? Does this matter? Or is this a limitation? Contrast this with the
statement at the end of S6.1 regarding the link from SAVI Device A to
Router B.
s6.4: '...from which the data traffic is not to be checked.': What about
control messages?
s6.5: It would be sensible to explain here why the device needs to do
BindRecovery rather than directing us to s8.1.
s7: Please say it is a state machine.
s7.1: I don't think that you can 'attach a node to a binding anchor'.
The anchor is a property of the link. the node is attached to a link
that is associated with the binding anchor.
s7.3.2: The data value 'binding entry count' associated with each
binding anchor would be best described as part of the state machine
rather than just in the security section where it looks a bit like an
afterthought.
s7.3.2: Do we know that all possible/relevant control messages are
covered here? I note that the constraints in s9.2 mentioned in para 1
only cover a small subset of these messages. It is written as if s9.2
applies to all the messages. Also s9.2 covers other control messages
than are mentioned in s7.3.2. In particular are their control messages
form RFC 3736 that need to be explicitly mentioned (so as to be ignored
or cause an error?)
s7.3.2/s7.5: What happens if a control message arrives but does not
generate a valid event? Does this have any effect on the state machine?
Is it an error transition (especially if the binding is in the INIT_BIND
state)?
s7.4.x.2: What happens if a man-in-the-middle or other attack generates
a DHCP server message using the correct TID but with bogus data? In
Figure 1, if router A or the link to the DHCP server is compromised,
does the whole house of cards come tumbling down possiblluy? This needs
to be discussed in the security section.
s7.4.x.2: RFC 5007 is IPv6 specific. Should mention RFC 4388 for IPv4.
s7.4.x.2: DHCP LEASEQUERY requests SHOULD be IPsec protected (not
surprisingly) - is this envisioned?
s7.4.3.2: I was wondering if it might be desirable to allow a small
amount of leeway on expiration? Doing expiry at exactly the lifetime
could mean that a renewal was missed.
s8.1: What is the rationale for doing the queries twice in stage 1?
s9.2, last para: ARP messages with link-local addresses? ARP is IPv4
specific - does this mean IPv4 link local (169.254/16) addresses or is
this some sort of confusion with IPv6 link local?
s10: The loss of attribute 'configuration' would seem to be a particular
problem that is not discussed here.
s13: Discussion of TID spoofing?
s13: Securing LEASEQUERY requests?
s15: As of this moment RFC 3736 is probably not normative since it is
just not relevant to a stateful system. That might change if all its
messages are considered.
_______________________________________________
Gen-art mailing list
[email protected]
https://www.ietf.org/mailman/listinfo/gen-art
