Re: [dtn] Barry Leiba's No Objection on draft-ietf-dtn-tcpclv4-18: (with COMMENT)

[Brian Sipos <[e-mail]>]

Barry,
I'm including responses below with prefix "BS: "

On Thu, 2020-02-13 at 15:13 -0800, Barry Leiba via Datatracker wrote:
> Barry Leiba 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:
> ----------------------------------------------------------------------
> 
> TCPCL:  I’ve chosen to mentally pronounce this as “tee cee pickle”.  Just so
> you know...
> 
> Thank you for the work on this stuff.  I have a number of comments, a few of
> which are substantive but minor; the rest are editorial and don’t need
> explicit
> responses.
> 
> The substantive ones:
> 
> — Section 3.1 —
> 
>    Session State Changed:  The TCPCL supports indication when the
>       session state changes.
> 
> What does “supports indication” mean?  Are you saying that the TCPCL
> “indicates
> when the session state changes”?  Or does some other entity provide those
> indications?  If the latter, it would be better to say what entity or entities
> do that, something like, “The TCPCL supports indications of session state
> changes from the BP agent.”  It should also be clear, whatever entity provides
> the indications, whether they always happen or are optional.  For example, can
> we *rely* on getting “Sesson Negotiating” and “Established” indications, or is
> it an implementation/deployment/configuration choice that they’re produced? 
> (Same comment for Session Idle Changed, and all other items in this section
> that talk about indications.)
> 
BS: This text should read as "The TCPCL entity indicates ..." and this section
should more clearly use nouns "TCPCL entity" and "BP agent" to distinguish
between the implementation (entity) vs. the specification.
I will clarify mandatory vs. optional for each of the session and transfer state
changes.

> — Section 4.1 —
> 
>    If the
>    passive entity does not receive a Contact Header after some
>    implementation-defined time duration after TCP connection is
>    established, the entity SHALL close the TCP connection.
> 
> Should there be any recommendation about that implementation-defined time
> duration, as there is in the previous paragraph?  No judgment here, but just a
> question.  I’ll note that if I never close the TCP connection I can say that
> I’m obeying the “SHALL”, and it’s just that my implementation defines the time
> duration at 17 years.
> 
BS: CH timeout guidance will be added similar to the KEEPALIVE guidance of 10
minutes.

> — Section 4.4 —
> 
>    Once
>    established, there is no mechanism available to downgrade a TCPCL
>    session to non-TLS operation.  If this is desired, the entire TCPCL
>    session MUST be terminated and a new non-TLS-negotiated session
>    established.
> 
> I suggest that it’s unlikely that this will be necessary, and I suggest being
> stronger about not doing it.  Does this work for you?:
> 
> NEW
>    Once
>    established, there is no mechanism available to downgrade a TCPCL
>    session to non-TLS operation.  If such a downgrade is necessary,
>    the entire TCPCL session would have to be terminated and a new
>    non-TLS-negotiated session established.  This is NOT RECOMMENDED.
> END
> 
BS: I agree that this is not a recommended procedure, and since it involves
agent behavior outside of a TCPCL session (and the spec doesn't have any real
requirements about how and when sessions are established) I am going to strike
the second half of this paragraph and leave the statement about "... no
mechanism ..."

> — Section 6.1 —
> 
>    After sending a SESS_TERM message, an entity MAY continue a possible
>    in-progress transfer in either direction.  After sending a SESS_TERM
>    message, an entity SHALL NOT begin any new outgoing transfer for the
>    remainder of the session.  After receving a SESS_TERM message, an
>    entity SHALL NOT accept any new incoming transfer for the remainder
>    of the session.
> 
> Checking something here… according to this, if A sends SESS_TERM to B:
> - A MUST NOT begin a new transfer to B
> - B MUST NOT accept a new transfer from A
> - But B is allowed to begin a new transfer to A, and A can accept it
> 
> Is that as intended?
> 
BS: The phrasing is a bit ambiguous. I'm going to tie those requirements to the
TCPCL session being in the Ending state and give a concrete definition of the
Ending state based on the earlier state diagrams.

> - - - - - - - -
> 
> Editorial comments:
> 
> — Section 1.1 —
> 
>    o  Policies or mechanisms for assigning X.509 certificates,
>       provisioning, deploying, or accessing certificates and private
>       keys, deploying or accessing certificate revocation lists (CRLs),
>       or configuring security parameters on an individual entity or
>       across a network.
> 
> When a list item contains commas, it’s customary to use semicolons to delimit
> the outer list, in order to avoid confusion.  So:
> 
> NEW
>    o  Policies or mechanisms for assigning X.509 certificates;
>       provisioning, deploying, or accessing certificates and private
>       keys; deploying or accessing certificate revocation lists (CRLs);
>       or configuring security parameters on an individual entity or
>       across a network.
> END
> 
BS: Makes sense and I will update the spec.

> — Section 2.1 —
> 
>    Idle Session:  A TCPCL session is idle while the only messages being
>       transmitted or received are KEEPALIVE messages.
> 
> It would be useful to have a forward reference here, “(see Section 5.1.1)”.
> 
>    Live Session:  A TCPCL session is live while any messages are being
>       transmitted or received.
> 
> Maybe “while any messages other than KEEPALIVEs are being...”
> 
BS: Makes sense and I will update this phrasing to be consistent with other
areas of the spec. The state is not about messages but about in-progress
transfers.

> — Section 3.3 —
> 
>    The states of a nominal TCPCL session (i.e. without session failures)
>    are indicated in Figure 4.
> 
> Why “nominal”?  Or do you mean to say “normal”?  (And “i.e.” needs a comma
> after it throughout the document, as does “e.g.”.)
> 
>       Session "Live" means transmitting or reeiving over a transfer
> 
> Typo: “receiving”
> 
BS: Will fix these typos and use the term "normal".

> — Section 3.4 —
> 
>       In a situation where network "goodput" is dynamic, the
>       transfer segmentation size can also be dynamic
> 
> It may be cute, but is there really a need to make up that word and use it
> just
> once?  Will everyone understand it, or will it just make some people (whose
> grasp of English isn’t as good as ours) pause and wonder for an unnecessary
> moment?  I don’t feel strongly about it — just asking the question.
> 
BS: Makes sense and I am rephrasing this to "TCP throughput" to be more
specific.

> — Section 4 —
> 
>    For example, some sessions MAY be opened proactively and
>    maintained for as long as is possible given the network conditions,
>    while other sessions MAY be opened only when there is a bundle that
> 
> I think these are not BCP 14 “MAY”, but just plain English “may” (or “might”).
> 
BS: Agreed, I'm removing the requirement terms from these statements.

> — Section 4.1 —
> 
>    Therefore, the entity MUST retry the
>    connection setup no earlier than some delay time from the last
>    attempt
> 
> This starts to look as though “the entity MUST retry the connection setup”,
> which is not what you mean.  You can avoid this by moving the negative, and I
> think it reads more cleanly that way:
> 
> NEW
>    Therefore, the entity MUST NOT retry the
>    connection setup without some delay time from the last attempt
> END
> 
BS: Agreed and going to rephrase this.

> — Section 4.3 —
> 
>    The first negotiation is on the TCPCL protocol version to use.  The
>    active entity always sends its Contact Header first and waits for a
>    response from the passive entity.  The active entity can repeatedly
>    attempt different protocol versions in descending order until the
>    passive entity accepts one with a corresponding Contact Header reply.
>    Only upon response of a Contact Header from the passive entity is the
>    TCPCL protocol version established and parameter negotiation begun.
> 
>    During contact initiation, the active TCPCL node SHALL send the
>    highest TCPCL protocol version on a first session attempt for a TCPCL
>    peer.  If the active entity receives a Contact Header with a
>    different protocol version than the one sent earlier on the TCP
>    connection, the TCP connection SHALL be closed.  If the active entity
>    receives a SESS_TERM message with reason of "Version Mismatch", that
>    node MAY attempt further TCPCL sessions with the peer using earlier
>    protocol version numbers in decreasing order.  Managing multi-TCPCL-
>    session state such as this is an implementation matter.
> 
> The latter paragraph repeats some of what’s in the former, as though they were
> written separately and pasted together.  May I suggest merging them this way?:
> 
> NEW
>    The first negotiation is on the TCPCL protocol version to use.  The
>    active entity always sends its Contact Header first and waits for a
>    response from the passive entity. During contact initiation, the
>    active TCPCL node SHALL send the highest acceptable TCPCL protocol
>    version on a first session attempt to a TCPCL peer.  If the active
>    entity receives a Contact Header with a different protocol version
>    than the one it had sent, the TCP connection SHALL be closed.  If
>    the active entity receives a SESS_TERM message with reason of
>    "Version Mismatch", that node MAY attempt further TCPCL sessions
>    with the peer, using earlier protocol version numbers in decreasing
>    order.  Managing multi-TCPCL-session state such as this is an
>    implementation matter.  Only upon response of an acceptable Contact
>    Header from the passive entity is the TCPCL protocol version
>    established and parameter negotiation begun.
> END
BS: Agreed and edited.

>    the node MAY either terminate the session
>    (with a reason code of "Version mismatch") or the node MAY adapt its
>    operation to conform to the older version of the protocol.
> 
> The first “the node MAY” is already factored out of the “either”, so you can
> strike the second instance:
> 
> NEW
>    the node MAY either terminate the session
>    (with a reason code of "Version mismatch") or adapt its
>    operation to conform to the older version of the protocol.
> END
> 
BS: Agreed and edited.

> 
> — Section 4.6 —
> 
>       The order and
>       mulitplicity of these Session Extension Items MAY be significant,
>       as defined in the associated type specification(s).
> 
> This is not a BCP 14 “MAY”, but just a plain English “may” (or “might”).
> 
BS: This is actually a requirement for implementations to not assume that order
is insignificant. I'm going to change this from a "MAY be" to an "is" statement
of fact.

> — Section 4.7 —
> 
>       If the
>       either Transfer MRU or Segment MRU is unacceptable
> 
> “If either the” — words swapped.
> 
BS: Agreed and fixed.

>    Session Keepalive:  Negotiation of the Session Keepalive parameter is
>       performed by taking the minimum of this two contact headers'
>       Keepalive Interval.
> 
> “of the two” and “Intervals”.
> 
BS: Agreed and fixed.

>       It can be a
>       reasonable security policy to both require or disallow the use of
>       TLS depending upon the desired network flows.
> 
> You can’t both require and disallow at the same time.  You mean “either”, not
> “both”.
> 
BS: Agreed and fixed.

> — Section 4.8 —
> 
>    Flags:  A one-octet field containing generic bit flags
> 
> This field is called “Item Flags” in the diagram, and should be called that
> here as well.
BS: Agreed and fixed.

> 
> — Section 5.1.1 —
> 
>    Note: The Keepalive Interval SHOULD NOT be chosen too short as TCP
>    retransmissions MAY occur in case of packet loss.
> …
>    KEEPALIVE messages MAY experience noticeable latency.
> 
> These are not BCP 14 “MAY”, but just plain English “may” (or “might”).
> 
BS: Agreed and fixed.

> — Section 5.2 —
> 
>    The choice of the length to use for segments is
>    an implementation matter, but each segment MUST be no larger than the
> 
> As I recommended earlier, I think MUST NOT is clearer than MUST in sentences
> such as this, so I suggest, “but each segment MUST NOT be larger than”.
> 
BS: Agreed and fixed.

> — Section 5.2.2 —
> 
>       The order and
>       mulitplicity of these transfer extension items MAY be significant,
>       as defined in the associated type specification(s).
> 
> This is not a BCP 14 “MAY”, but just a plain English “may” (or “might”).
> 
BS: Will apply same treatment as earlier transfer items.

> — Section 5.2.4 —
> 
>    As data segments and
>    acknowledgments MAY cross on the wire
> 
> This is not a BCP 14 “MAY”, but just a plain English “may” (or “might”).
> 
BS: Agreed and fixed.

>    bundle after transmitting a XFER_REFUSE message since messages MAY
>    cross on the wire;
> 
> This is not a BCP 14 “MAY”, but just a plain English “may” (or “might”).
> 
BS: Agreed and fixed.

>    Note: If a bundle transmission is aborted in this way, the receiver
>    MAY not receive a segment with the 'END' flag set to 1 for the
>    aborted bundle.
> 
> This is not a BCP 14 “MAY”, but just a plain English “may” (or “might”).  But,
> perhaps what you really want here is “will not”?
> 
BS: Changed to "does not" as statement of fact.

> — Section 5.2.5 —
> 
>    Flags:  A one-octet field containing generic bit flags
> 
> This field is called “Item Flags” in the diagram, and should be called that
> here as well.
> 
BS: Agreed and fixed.