Re: [dtn] draft-ietf-dtn-tcpclv4-02 comments

[Brian Sipos <[e-mail]>]

Matt,
This is some great feedback, I've in-lined responses below and also
copied the DTN mailing list for their consideration.

On Tue, 2017-07-18 at 00:28 -0700, Matt Wronkiewicz wrote:
> DTN WG team,
> 
> I read your draft for TCPCLv4 and look forward to implementing it.
> Listed are some comments on the version at
> <https://tools.ietf.org/html/draft-ietf-dtn-tcpclv4-02>:
> 
> 
> 1. The contact header does not specify its total length. This may
> cause a problem with header extensions because it is unclear at what
> point a node should stop waiting for additional data, set the session
> parameters, and start sending messages or upgrade security. The
> contact header should include a length field that counts the
> remaining
> bytes including extensions.
> 
> If it does not include a length field, there are some additional
> ambiguities.

This was definitely an oversight and needs to be added in. The contact
header is supposed to be fully self-contained data, which right now it
is not. We could add in an extension item count or an all-extension
octet size. The size may be more helpful.
I also didn't catch this earlier because I added extension handling to
the spec but did not include this in the test implementation; oops!

> 
> 1.a. A critical header extension with no other flags could be
> misinterpreted as a XFER_SEGMENT message, and vice versa. An
> implementation must reject XFER_SEGMENT messages that do not follow
> XFER_INIT with the same transfer id. It must not send them before
> sending XFER_INIT. This should be clarified in 5.4.3. Data
> Transmission (XFER_SEGMENT).
> 
> 1.b. In Table 2: Header Extension Item Flags, several reserved bits
> must be zero, or the header extension would collide with other
> message
> types that are allowed after the session is established.
> 
> 
> 2. In Section 4.2. Validation and Parameter Negotiation, 3rd
> paragraph, if two nodes send different version numbers in the contact
> header, it is specified that one of the nodes must send a SHUTDOWN
> with a version mismatch. There is no opportunity for the other side
> to
> downgrade to the older version. There is also no way for the older
> version to negotiate a session, because it has not received the
> connection flags and cannot interpret the remote EID.
> 
I can add some guidance to this section. There is no mechanism for in-
TCP-connection downgrade, so the recommended behavior is to re-connect
on the same TCP port but then provide a TCPCLv3 contact header and see
if the listening agent accepts that instead. A similar procedure could
be use to try TCPCLv3 first then v4 after. The listener (TCP server)
has discretion to look at the start of the contact header before
sending its only reply header.

> 
> 3. The inability for two nodes supporting multiple TCPCL versions to
> handshake is concerning. There are very few TCPCLv3 installations, so
> backward compatibility is less of a worry. But if TCPCLv4 is
> successful it will be widely deployed when there is a need for a new
> version. It would be good to provide some mechanism for nodes that
> implement both TCPCLv4 and v5 to establish a connection with a node
> that only implements v4.
> 
> One way to allow forward compatibility would be to require v4 nodes
> to
> send a VERSION_MISMATCH in response to a newer version, after which
> the newer node may send a compatible contact header or disconnect.
> 
Similar to above, do you think the one-version-per-connection-attempt
is acceptable? Some additional informational language could be added to
explain this situation.

> 
> 4. Nodes implementing TCPCLv4 will presumably already have functions
> for CBOR encoding and decoding. Restructuring the messages to use it
> would remove a lot of padding. It would also provide more flexibility
> and robustness for adding header extensions and new protocol
> versions.
> I recognize this is a drastic change that you have probably already
> discussed.
> 
We definitely had discussed the original proposed SDNV vs. CBOR vs.
fixed-length-fields and settled on current fixed-length due to the
convergence layer being a lower-level protocol than BP itself. This was
due to concern about complexity in a TCPCL implementation. There is
also the possibility, I believe, of a pure bundle forwarder which
speaks TCPCL but does not actually examine the bundle contents and
simply forwards bundle, as opaque data, to some actual BP node.

> 
> 5. Table 13: SHUTDOWN Reason Codes is missing "Resource Exhaustion",
> listed in Table 8: SHUTDOWN Reason Codes.
> 
> 
> 6. In Table 8: SHUTDOWN Reason Codes, in the description for Resource
> Exhaustion, the word "resource" is misspelled.
> 
These will be corrected.

> 
> 7. The shutdown procedure is not specified when one node requires TLS
> and the other does not set USE_TLS. It is unclear whether the
> strict-TLS node should send SHUTDOWN with "Contact Failure" or "TLS
> Failure". A more descriptive "TLS Required" failure code could be
> added.
> 
I agree that we should distinguish between "TLS Agreement Failure" when
the endpoint policy disallows the agreed Enable TLS state, and "TLS
Handshake Failure" where TLS is enabled and attempted but the handshake
itself fails.

> 
> 8. In section 4.1.1. Header Extension Items, in the entry for Flags,
> the word "unknown" is misspelled.
> 
This will be corrected.

> 
> Thank you for your work on the DTN WG, and for considering my
> feedback.
> 
> Matt Wronkiewicz