TSVDIR review of draft-ietf-dhc-dhcpv4-bulk-leasequery

Hi, all,

I've reviewed this document as part of the transport area directorate's 
ongoing effort to review key IETF documents. These comments were written 
primarily for the transport area directors, but are copied to the 
document's authors for their information and to allow them to address 
any issues raised. The authors should consider this review together with 
any other last-call comments they receive. Please always CC 
tsv-dir(_at_)ietf(_dot_)org if you reply to or forward this review.
This request was received Feb. 2, 2012.

This document describes an extension to DHCPv4 for the bulk query and 
transfer of current lease status over TCP.
The following transport issues were noted, and are significant:

UPDATES- The document updates DHCP with support for TCP, and as such 
this document seems like it should UPDATE RFC2131
PORT USE- Although DHCPv4 has an assigned TCP port, this document should 
clearly indicate that there is no other specified use of that port other 
than the bulk lease query described in this document
It should further explain why no other existing DHCP exchanges are not 
valid on the TCP port.
CONNECTION MANAGEMENT- The document describes the use of TCP connections 
for bulk transfer, but needs to be more specific about which side (relay 
client or server) closes the connection, under what circumstances, and 
with what mechanism (e.g., graceful CLOSE vs. ABORT, as per RFC 793)
sec 7.3 indicates some conditions for terminating connections; this 
section should indicate which side performs this, and by what method 
(CLOSE, ABORT)
this sec also allows connections to stay open after all pending 
transactions are complete (MAY); the rationale for this should be given, 
or the connection MUST be closed.
the same issue applies to sec 7.8 and 8 throughout; sec 8.5 is 
particularly problematic on this issue because it discusses aborting a 
request using in-band data, which may not be available if the connection 
is closed using ABORT. the state of other pending connection shsould be 
discussed too.
TIMEOUTS- Sec 6.3 defines a timeout for the TCP connection; is this 
intended to supercede any TCP timeout? or is it intended to be the min 
of the TCP timeout and the specified one?
This section should more carefully explain behavior when a connection is 
dropped and the reason - e.g., timeout, abort, close.
INTERLEAVING- sec 7.7 says that the server MUST interleave replies; 
there doesn't seem a valid reason for this requirement. clearly the 
receiver MUST tolerate interleaved replies. having the server interleave 
replies is relevant only if each request/reply has its own timeout -- 
which should be overridden if there is another response in progress 
anyway. This issue should be more clearly explained and motivated.
There were some other issues noted in this document. These comments 
appear below, and although not focused on transport issues, they 
represent significant issues that IMO should be addressed as well.
NOTE - regarding some terminology issues, I did not survey current DHCP 
RFCs for consistency, but IMO these terms should be corrected even if 
they are then inconsistent with existing specs.
Joe

-----------

Major non-transport issues:

- In many places the doc allows inaction to substitute for either 
positive or negative confirmation. IMO, this invites implementation 
errors, and should be avoided. E.g., status return codes, data source 
option missing, query-for-all-configured-IPs, etc.
- the data source option reserved codes need more detailed 
specification. if these are intended for future use, then they MUST be 
ignored by the receiver (at least). if they are to mean anything at all, 
at least one bit (typically all of them) MUST be set to zero by the 
transmitter for implementations that do not support any of the component 
fields.
further, the length of this option MUST be 1

- The protocol supports bulk transfer that is not data driven. This 
could represent a security vulnerability by exposing information that 
may not be on the data path (and thus already accessible) to a relay 
agent. This should be discussed in the security considerations section.
- Integer quantities should be referred to as "unsigned 32-bit integers" 
throughout.
- "VPN" is used throughout to refer to "private" addresses (RFC 1918); a 
VPN is not just private addressing.
- (this is a nit with all IDs, FWIW) SHOULD and SHOULD NOT are used 
throughout without context. Any time SHOULD is used instead of MUST (or 
SHOULD NOT rather than MUST NOT), it is useful to explain a viable 
exception. If no such exception exists, the rationale for selecting 
SHOULD over MUST should be included.
- It would be useful to explain why STATUS-CODE strings MUST NOT be 
null-terminated; is that a protocol violation, or are you just saying 
that NULLs are valid in these strings? the description should be clear 
that the string field describes the string length without any 
termination characters.
- start-time-of-state is expressed as an offset from base-time; this 
appears to be the only time indicated as an offset rather than as an 
absolute. That inconsistency invites implementation errors; IMO, this 
should be absolute too.
- option lengths: throughout, the doc refers to option lengths as being 
"an octet"; they are *indicated* in one octet. Some are constant (e.g., 
DCHP-STATE), some allow the contents of the octet to vary. Again, this 
is an *unsigned integer* octet.
- some of the information provided (in DHCP-STATE) goes beyond that of 
DHCP. It would be useful to motivate the need for this information in a 
bulk query, and why it is not similarly available for nodes using UDP 
(e.g., as an extension to DHCPv4, not just to the bulk transfer 
command). again, absence of state information should not indicate state. 
State should always be expressed explicitly. these states are further 
meaningless without a state diagram explaining them. if such a diagram 
is not possible (as noted at end of sec 6.2.7), then the states are 
irrelevant and the option should not be included.
- in sec 6.2.9 the term "not allowed" should be explained - are they 
reserved for future use and thus ignored? or are they "not allowed" - in 
which case the doc should indicate handling if they appear.
- sec 7.4 states that the clock skew of zero is desired; this assumes 
E2E delays under 1sec. An explanation of why this is desired should be 
given, as well as the consequences of it not.
Other non-transport issues:

- The document includes definitions and references to irrelevant 
deployment and implementation issues, notably DSLAMs, concentrators, and 
access concentrators. These should not appear formally; they should be 
used only to usefully illustrate currently intended uses.
- The doc refers to "real-time", which could imply requirements not 
supported. This should be replaced with "rapid".
- "absolute time" *indicates* (rather than contains) the number of 
seconds...
- "third party agent" should be explained, i.e., neither DHCP client nor 
DHCP server.
- "downstream" and "upstream" should be defined as both away from the 
server *and towards the client* ("away from the server" has two directions).
- sec 3 should refer to relays, and returning the entire set or 
individual bindings; there is no reason to explain the goals in terms of 
access concentrators.
- sec 3.2 appears to provide contradictory advice - caching is required, 
but should be avoided? it would be useful to resolve this inconsistency.
- sec 3.3 refers to 'fast path', but this term doesn't make sense in 
this context because fast-path is a forwarding issue. it would be useful 
to explain what you mean, and pick a different term
----




_______________________________________________
Ietf mailing list
Ietf(_at_)ietf(_dot_)org
https://www.ietf.org/mailman/listinfo/ietf