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

[Brian Sipos <[e-mail]>]

Marius,

Thank you for these comments. I've been working through addressing them as well as I can.

Two of these need some further discussion though:


  *   Your comment about the USE_TLS flag is valid, but there had been discussion at an earlier IETF of whether the TLS behavior of TCPCLv4 is an integral part of the protocol and the conclusion was that it is integral to TCPCL and is to be mandatory-to-implement (but not mandatory-to-use). For this reason it's treated differently than any other extension which may come down the road.
  *   The comment about SHUTDOWN sequencing brings to light a possible consistency issue with the current spec and also with TCPCLv3. As it currently stands there is really no hard condition for closing a TCP connection after sending or receiving a TCPCL SHUTDOWN message. Since as currently stated (which is a carry-over from TCPCLv3) a node may send a SHUTDOWN but then continue to send a huge number of remaining XFER_SEGMENT messages, the receiver has no good logic to distinguish between a shutdown-but-wait-for-data behavior vs. a shutdown-and-close-connection behavior. I suppose the TCPCLv4 spec could explicitly mention both of these options as valid choices which would explain the "MAY" condition of Page 26.

________________________________
From: dtn <[e-mail]> on behalf of Marius Feldmann <[e-mail]>
Sent: Sunday, November 12, 2017 11:21
To: [e-mail]
Subject: Re: [dtn] draft-ietf-dtn-tcpclv4-02 comments

Hello,

below you will find some further comments about TCPCLv4 as described in [1].

Regards

Marius

[1] https://tools.ietf.org/html/draft-ietf-dtn-tcpclv4-02

----

Abstract:
* "Bundle Protocl" -> "Bundle Protocol"
* "...updates to the Bundle Protocol contents, encodings, and
convergence layer requirements in Bundle Protocl Version 7." - This
statement is not precise in my opinion. The current BP 7 draft only
provides general requirements in regards to the CL. Which precise
requirements does this statement point to?

Page 1:
* Page numbers provided in the table of contents are not correct
(increment by 1 to correct them).

Page 2:
* Maybe TCPCL could be motivated a bit better (currently it is just
stated that BP needs a CL)? Which precise problem(s) does TCPCL solve?

Page 3:
* "The locations of the TCPCL and the BP in the Internet model protocol
stack are shown in Figure 1." - Which protocol stack model is shown in
figure 1? Usually, we should stay with the one provided in RFC 1122
(which does not mention a physical layer and a presentation layer). It
seems this figure shows a combination of different models.

Page 4:
* Capitalized "Session" - should be written with lower case "s".
* Long form of "MRU" should be stated.
* Maybe the section "3.  General Protocol Description" should be
restructured into a section "3.1 Overview" (including the current
section 3.1) and "3.2 Example Message Exchange". Currently, the
structure is a bit strange with a long section introduction and a short
section 3.1. - in my opinion this could be merged to an "overview"
section 3.1.
* "XFER_INIT" may be mentioned in the section "3. General Protocol
Description" due to its appearance in figure 2.

Page 6:
* "The following figure visually depicts..." - Remove "visually". A
figure is a visual entity by definition, isn't it?

Page 7:
* Maybe figure 2 should also point out the parameters used in the
contact header?

Page 8:
* "Destination port number 4556 has been assigned by IANA as the
well-known port number for the TCP convergence layer." - Ports between
1024 and 49151 are named "Registered Ports", not "well-known", see RFC 6335.

Page 10:
* I was wondering if the "CAN_TLS" flag (which is actually the only
supported flag) could not be expressed as header extension item. If
expressed as header extension item it is also rendered possible to
differentiate between optional or enforced TLS.

Page 11:
* "reason code" is mentioned without introducing the concept globally,
maybe it should be introduced before or at least a reference should be
provided.
* "Each type This specification does..." - "Each type" should be
removed, I guess.

Page 13:
* "The message types defined in this specificaiton are listed in Table
3." - "specificaiton"; furthermore the information about table 3 is
redundant with the sentence before.

Page 21:
* "receving" -> "receiving", "DATA_SEGEMNT" -> "DATA_SEGMENT"

Page 22:
* "As bundles can be large, the TCPCL supports an optional mechanism by
which a receiving node MAY indicate to the sender that it does not want
to receive the corresponding bundle." - As it is stated in the next
sections, there are more reasons for transfer refusal besides the "size
of the bundle". Thus, the section may not be introduced in this way.

Page 26:
* "After receiving a SHUTDOWN message in response, both sides MAY close
the TCP connection." - Why is it "MAY", not "MUST"? The only reason may
be a reuse of the connection for a further TCPCL session. Before it is
mentioned that a contact header is sent via a new TCP connection (e.g.
page 9 "Once a TCP connection is established, both parties exchange a
contact header."). Which sence does it make to keep the TCP connection?
Either the "MAY" should be transformed to a "MUST" or the state in which
a contact header may be sent should be clarified.

----


On 19.07.2017 14:50, Brian Sipos wrote:
> 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
> _______________________________________________
> dtn mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/dtn
dtn Info Page - Internet Engineering Task Force<https://www.ietf.org/mailman/listinfo/dtn>
www.ietf.org
"This list is for discussions related to the formation of a Delay Tolerant Networking (DTN) working group. The IRTF DTNRG research group has worked on the particular ...



_______________________________________________
dtn mailing list
[e-mail]
https://www.ietf.org/mailman/listinfo/dtn
dtn Info Page - Internet Engineering Task Force<https://www.ietf.org/mailman/listinfo/dtn>
www.ietf.org
"This list is for discussions related to the formation of a Delay Tolerant Networking (DTN) working group. The IRTF DTNRG research group has worked on the particular ...