Subject: Re: [dtn] I-D Action: draft-ietf-dtn-tcpclv4-09.txt

From: Brian Sipos <[email-masked]>
Date: Sun, 28 Oct 2018 19:40:58 +0000

Joerg,
I'm making edits based on your comments below, and have included
replies inline below also.

On Mon, 2018-08-13 at 22:36 +0200, Joerg Ott wrote:
> [Now including the DTN WG list]
> 
> Hi everybody,
> 
> I am guilty of having had less than adequate time lately to look at
> the
> detailed evolution of this draft.  I did a detailed review that
> yielded
> a number of comments.
> 
> As a meta note up front: I am not super-happy that the document more
> than doubled in size, while the functionality did not grow that much
> (luckily).  There seem to be lots of precautions taken for future-
> proofing, while some of those could be addressed by another rev.
> I specifically also went back to v3 to compare the changes.
> 
> I am also puzzled about the generous choice of field sizes given that
> the bundle protocol itself uses encodings that aim at wire
> efficiency.
> 
> I don't wanna bring up everything at once but here are a few bits
> that
> I really think need to be fixed.  So let me keep the discussion
> focused
> for now (we can look at nits later on):
> 
> 1. Section 4.3 seems to suggest on p21 (a node may adapt its
> operation
>     to an older version of the protocol) the two endpoints might
> choose
>     using different protocols versions, which is surely problematic.
>     The outcome of this negotiation must be unambiguous.
> 
I've added specific logic based on the two roles (active vs. passive)
to disambiguate the behavior and make understanding when the session
transitions state to parameter negotiation.

> 2. Section 4.4 discusses TLS but this isn't enough.  TLS has lots of
>     parameters, TLS clients may and servers must have certificates.
>     All this interaction needs to be spelled out.  What about the APN
>     to be used for TLS connection setup.  And, of course, we should
> use
>     TLS 1.3.
> 
As was mentioned by others in earlier replies, this spec relies on
RFC7525 for TLS best practices, version support, and default
ciphersuite support and use.

> 3. Splitting XFER_INIT from XFER_SEGMENT is not useful.  Nodes would
> not
>     want to wait for a reply but use pipelining.  Hence, creating a
>     SEGMENT must follow INIT dependency appears unnecessarily complex
>     in terms of state space.
> 
>     Simple suggestion: Just keep session transfer but, if the S bit
> is
>     set, include the total bundle length field and the bundle
> transfer
>     id.  You achieve the same, save a few bytes, and the sender won't
>     wait transmitting anyway.  And one possible error condition goes
>     away.
> 
>     The only reason LENGTH was a separate message before is that we
>     needed/wanted to be backwards compatible to earlier versions of
> the
>     draft and could not grow the header of the DATA_SEGMENT message.
> 
I understand the original rationale for splitting the message data, but
because of the new extension items for transfers the XFER_INIT message
makes a convenient host for this data. I don't see any reason to remove
the separate message type, as it makes the XFER_* messages single-
purpose and with little in-message variability.

> 4. Section 5.1.1 Keep alive has lost its function.  According to the
>     current text, a keep-alive should be transmitted when the timer
>     expires.  Fine, but this too late because the other entity will
>     time out the connection.  The earlier guidance was that the
> session
>     timeout occurs if no data was received for _twice_ the keep alive
>     interval.  But this part seems to have gone.  The text would need
>     to be explicit about when to schedule sending keep-alive messages
>     and when to time out.
> 
I agree about the wording of this text, and I've added SHOULD
statements about the default/minimum timeout being twice the keepalive
interval.

> 5. The extension items (both for setup and transfer) are generously
>     dimensioned but not a single one is defined.  I would say that
>     8 bits parameter space and 16 bits length should be sufficient.
>     It is also useful to define the encoding only in one place since
>     the two are largely identical.
> 
I agree that the namespace and allowed size of these items is generous,
but also for a TCP-based protocol I'm not sure how much value there is
in trimming down individual octets to save space. For my own purposes,
BP/TCPCL would be used for MiB/GiB sized data so the potential overhead
per-session or per-transfer would be negligible.

About the different extension point definitions, are you suggesting
that there would be only a single registry of Extension Item Types and
each registered type ID could be used as either Session extension or a
Transfer extension (or potentially both)?
I feel like this would add complexity to the definition of each
extension item type without really buying much in terms of efficiency
(one IANA registry instead of two).

> 6. Echoing the SESS_TERM message as an ACK is a good idea.  Should
>     we include a nonce here to make sure that an ACK is different
> from
>     an independently issued SESS_TERM?
> 
Yes, this is a reasonable and valuable thing to have. I'm not sure that
an actual (random) nonce is necessary here though, as each session
really only allows a single SESS_TERM in a normal session lifetime.
Besides any TLS use of PRNGs, the TCPCL is otherwise fully
deterministic in its message contents. A sequence number wouldn't help
here because there really should only be a single SESS_TERM out of each
node in ideal sequencing.
Would it be sufficient to have a message header flag indicating ACK, so
that normally one peer would send SESS_TERM with the flag not set and
the other would reply with the flag set?

> 7. Session termination in section 6.1 got more complicated.  The text
>     now alllows sending even more segments, whereas the v3 spec
>     essentially said that no data must follow afterwards.  If I
> receive
>     a SESS_TERM, I should be sure that I can release data processing
>     resources.
> 
The point of SESS_TERM here is to guarantee that there can be no more
transfers begun after that point. It does not allow for any resource
cleanup, and no cleanup on a session can be done until the TCP
connection is closed. Do you think there needs to be specific
admonition text added to explain this?

> 8. Why not always have the reason code and define one to be
> unpsecified
>     in SESS_TERM rather than having two-stage processing?
> 
I agree with this, and am adjusting SESS_TERM to have an Unknown reason
code similar to what XFER_REFUSE has.

> 
> Major editorial:
> 
> We have too many terms and definitions.  Why are we discussing a
> TCPCL
> entity with a complex figure 2, when there is nowhere inside the
> document any need visible for discussing more than a single TCPCL
> connection at a time.  This is rather confusing.
> 
> We have many state diagrams (fundamentally a good idea) but they are
> hard to follow and sometimes very simple.  They are also not referred
> to in the mandatory text.  Maybe we can have fewer states and a
> stronger relation between text and diagrams?  Btw, not all acronyms
> are
> defined here.
> 
This was really the result of earlier feedback on understanding and
implementing the protocol. The very fact that a single node
could/should support multiple sessions between other nodes was a point
of confusion for some reviewers. The fact that it wasn't explicitly
mentioned that one node can have multiple sessions, even multiple
sessions with the same peer, was previously unstated and led to
confusions. I personally have no problem with lots of non-normative
text that helps understanding the implications of the normative text.
The point of these diagrams is to _not_ be normative but to assist in
understanding how the many requirements interact with each other to
build a stateful protocol.

> "CAN_TLS" is not the best name.
> 
I have absolutely no investment in this flag name, so open to
suggestions. I just chose a simple short one.

> Cheers,
> Jörg