Subject: [dtn] Alexey Melnikov's No Objection on draft-ietf-dtn-tcpclv4-18: (with COMMENT)

From: Alexey Melnikov via Datatracker <[email-masked]>
Date: Thu, 20 Feb 2020 04:45:22 -0800

Alexey Melnikov has entered the following ballot position for
draft-ietf-dtn-tcpclv4-18: No Objection

When responding, please keep the subject line intact and reply to all
email addresses included in the To and CC lines. (Feel free to cut this
introductory paragraph, however.)

Please refer to https://www.ietf.org/iesg/statement/discuss-criteria.html
for more information about IESG DISCUSS and COMMENT positions.

The document, along with other ballot positions, can be found here:
https://datatracker.ietf.org/doc/draft-ietf-dtn-tcpclv4/

----------------------------------------------------------------------
COMMENT:
----------------------------------------------------------------------

[Updated. See the new text at the end.]
Thank you for this well written document. It was a pleasure to read!

I agree with Suresh, text about TLS 1.2 compatibility looks dodgy.

I also have some comments I would really like to see replies to:

The document never states byte order for 16/32/64 bit fields. As you are not
using CBOR (or any other format), this can't be presumed to be known.

4.7.  Session Parameter Negotiation

   Enable TLS:  Negotiation of the Enable TLS parameter is performed by
      taking the logical AND of the two contact headers' CAN_TLS flags.
      A local security policy is then applied to determine of the
      negotiated value of Enable TLS is acceptable.  It can be a
      reasonable security policy to both require or disallow the use of
      TLS depending upon the desired network flows.  Because this state
      is negotiated over an unsecured medium, there is a risk of a TLS
      Stripping as described in Section 8.  If the Enable TLS state is
      unacceptable, the node SHALL terminate the session with a reason
      code of "Contact Failure".  Note that this contact failure reason
      is different than a failure of TLS handshake or TLS authentication
      after an agreed-upon and acceptable Enable TLS state.  If the
      negotiated Enable TLS value is true and acceptable then TLS
      negotiation feature (described in Section 4.4) begins immediately
      following the contact header exchange.

While this text is not wrong, I think it is in a wrong section. The rest of
Section 4.7 talks about SESS_INIT message, while the TLS flag was sent in
Contact Header and was already negotiated by this point.

9.1.  Port Number

   Within the port registry of [IANA-PORTS], TCP port number 4556 has
   been previously assigned as the default port for the TCP convergence
   layer in [RFC7242].  This assignment is unchanged by TCPCL version 4,
   but the assignment reference is updated to this specification.  Each
   TCPCL entity identifies its TCPCL protocol version in its initial
   contact (see Section 9.2), so there is no ambiguity about what
   protocol is being used.  The related assignments for UDP and DCCP
   port 4556 (both registered by [RFC7122]) are unchanged.

          +------------------------+----------------------------+
          | Parameter              | Value                      |
          +------------------------+----------------------------+
          | Service Name:          | dtn-bundle                 |
          |                        |                            |
          | Transport Protocol(s): | TCP                        |

Is there another document that will define use over DCCP?

9.6.  XFER_REFUSE Reason Codes
9.7.  SESS_TERM Reason Codes

In both of these sections: I don't think the document say anywhere how
recipients of unrecognized reason codes should handle them. I think the
document should say that they must be treated as "Unknown".

**********************************************************************
* Note, that I am conducting an experiment when people aspiring to be*
* Area Directors get exposed to AD work ("AD shadowing experiment"). *
* As a part of this experiment they get to review documents on IESG  *
* telechats according to IESG Discuss criteria document and their    *
* comments get relayed pretty much verbatim to relevant editors/WGs. *
* As an AD I retain responsibility in defending their position when  *
* I agree with it.                                                   *
* Recipients of these reviews are encouraged to reply to me directly *
* about perceived successes or failures of this experiment.          *
**********************************************************************

I also have some comments on Benjamin's comments below marked with "[[Alexey]]:"

The following comments were provided by Benjamin Schwartz <[email-masked]>:

Benjamin would have balloted *No Objections* on this document. He wrote:

## Abstract

   This version of the TCPCL protocol
   is based on implementation issues in the earlier TCPCL Version 3 of
   RFC7242 and updates to the Bundle Protocol (BP) contents

This would be better phrased as “This version of the TCPCL protocol resolves
implementation issues in …” or similar.

## Section 2.1
The paragraph labeled “TCP Connection” might be better labeled “Connection”,
since that is the term being defined.  Additionally, it might be clearer to
define the “connection” as being the TLS connection when TLS is in use.

## Section 3.1
Terminology: “overlaying” isn’t really a word.  Please rephrase.

## Section 3.2
TCP already provides segmentation and ACKs, so why does TCPCL replicate this
functionality?  The draft should mention the rationale.  (I imagine it’s
because the Berkeley Sockets API doesn’t surface TCP ACKs in a clear way, or
because TLS doesn’t provide authenticated ACKs.)

TCP already provides keepalive functionality.  Why is it replicated in TCPCL?

[[Alexey: I think it was observed that keepalive at the protocol level (or in
TLS) work better than TCP keepalives when NATs are used. I am wondering if
there is any up-to-date information on this.]]

# Section 3.4
“MRU” appears here for the first time without being defined.  Similarly,
reference is made to a “Transfer MRU”, which also has not yet been defined. 
Please add forward references or reorder the definitions.

# Section 4.4.1

Making compliance with BCP 195 (or any successor) “mandatory” seems too strong,
especially since existing implementations will be instantly noncompliant
whenever such a successor is published.  I would limit the requirement strength
to SHOULD when referring to a BCP.

[[Alexey: it might be better to use MUST for BCP 195 (maybe reference a
specific RFC) and SHOULD for successors?]]

# Section 8.9

   There is the possibility of a "data dribble" attack in which an
   entity presents a very small Segment MRU which causes transfers to be
   split among an large number of very small segments and causes the
   segmentation overhead to overwhelm the network througput.

I’m not sure what it means to “overwhelm the network throughput”, but surely
TCP congestion control is already a sufficient mitigation against this concern?
 Similarly for keepalive spam.