Subject: Re: [dtn] AD review of draft-ietf-dtn-tcpclv4-12

From: Magnus Westerlund <[email-masked]>
Date: Tue, 17 Sep 2019 19:47:00 +0000

Hi,

Sorry about the delay in reviewing your update based on my AD review
comments. Below I have commented on my view how the issue have been
dealt with. 

We appear to have a small number of things that are still not fully
resolved in my view so please chech these and comment. 

I am also sorry that my mail client apparently eat the numbering. 

On Thu, 2019-08-01 at 13:31 +0000, Magnus Westerlund wrote:
> Hi,
>  
> I have now performed my AD review of draft-ietf-dtn-tcpclv4-12. I
> think most are minor comments, however the TLS and security related
> ones may be more problematic to resolve. I will now be on vacation so
> you know you will not receive any quick replies from me before the
> end of the month.
>  
> Section 1: What are the applicability of TCPCLv4 to BPv6? I wonder as
> RFC5050 is referenced. 

Thanks for the new scope section making things much clearer. My
interpretation now is that this document only is for BPv7. 

> 
> Section 1.1: Session State Changed. Some editorial issues. Missing
> “:” initially. Then double “..” on Terminated. 

Fixed. 

> 
> Section 1.1: 
>    Session Idle Changed  The TCPCL supports indication when the
> live/idle sub-state changes.  This occurs only when the top-level
> session state is Established.  Because TCPCL transmits serially over
> a TCP connection, it suffers from "head of queue blocking" this
> indication provides information about when a session is available for
> immediate transfer start.
> 
> So in which direction are this change indicated/reported, both or
> only in one of them, or any as implied by Section 2.1’s definition of
> Live Session? 

Fixed

> 
> Section 1.1: Transmission Intermediate Progress: Segment is not
> defined prior to this. Maybe a forward pointe? Or should maybe the
> whole subsection (1.1) be moved to after definitions? 

Fixed

> 
> Section 2: Is there a point to use the RFC 8174 language that makes
> only capital words have special meaning? 

Addressed

> 
> Section 3.1: “One of these parameters is a singleton endpoint
> identifier for each node (not the singleton Endpoint Identifier (EID)
> of any application running on the node) to denote the bundle-layer
> identity of each DTN node.”
> 
> The above quote does imply something that at least BPBis isn’t making
> clear that a particular application agent would have its own EID. It
> is not clear that there are a one-to-one relationship between bundle
> nodes and application agents. Can you please clarify what the
> relationship is and lets figure out if that needs to be clarified
> back to BPbis. 

Ok, so the new text addresses this. 

> 
> Section 3.1: “Bundle interleaving can be accomplished by
> fragmentation at the BP layer or by establishing multiple TCPCL
> sessions between the same peers.”
> 
> Are there clear rules established for how many TCPCL sessions in
> parallel that may be established? By the end of my reading this is
> unanswered. 

Answered, no built in limiation. Resource limited. 

> 
> Section 3.1: XFER_REFUSE does that indicate that the bundle has
> already been received. How else does one separate other reasons for
> refusing a bundle versus that one have received it prior?

I don't think this needs more texf, section 5.2.4 is clear on that the
reason code will make resolve the reason. 

> 
> Section 3:1:    Once a session is established established, TCPCL is a
> symmetric protocol between the peers. 
> Double established
> 

Fixed.

> Figure 4: “Close message” is this TCP level message, if that is the
> case can that be clarified by prefix with TCP?

Fixed. 

> 
> Section 3.2: 
>    Notes on Established Session states: 
>       Session "Live" means transmitting or reeiving over a transfer
>       stream.
> 
>       Session "Idle" means no transmission/reception over a transfer
>       stream.
> 
>      Session "Closing" means no new transfers will be allowed.
> 
> Note that “Closing” is not used in Figure 4, it is called ending.
> Note spelling error on “live” receving.

Fixed

> 
> Section 3.2: Figure 5 and 6 uses PCH without explanation. Figure 5
> could probably also benefit by expanding CH as Contact Header.

Fixed. 

> 
> Figure 8 and 10: Uses [SESSTERM] is this the same as using the
> SESS_TERM message, or some other procedure. Please clarify. 

Okay looking at this again it is clear that [SESSTERM] is used for any
type of session termination. However, as SESSTERM appears to only be
used in the figures in that meaning, and being defined in Figure 13,
despite existing in 8, 9 and 10 also. Also considering that Figure 13
and 14 are signalled termination and the other are failure cases I
think a sentence or two should be spent to explain its meaning prior to
its usage. 

> 
> Section 3.3: “   Many other policies can be established in a TCPCL
> network between these two extremes.”
> 
> The list above includes three items, so the two extremes needs to be
> enumerated. 

Addressed. 

> 
> Section 4.1: Can TCPCLv3 and TCPCLv4 coexist on Port 4556? Based on
> 9.1 the answer is yes, please clarify here.

Fixed

> 
> Section 4.1: “Therefore, the entity MUST retry the connection setup
> no earlier than some delay time from the last attempt, and it SHOULD
> use a (binary) exponential backoff mechanism to increase this delay
> in case of repeated failures.”
> 
> Any recommended upper limit for the backoff?

Addressed

> 
> Section 4.2:    Version:  A one-octet field value containing the
> value 4 (current version of the protocol). 
> 
> I think the use of “the protocol” is unclear, maybe call it “the TCP
> convergence layer”. This to avoid confusion with BPv7. 

Fixed. 

> 
> Section 4.2: Please define how to set and ignore reserved bits in the
> Flags field so that it may be extended in the future. 

Okay, it is defined to not be set, which is likely to be interpret to
mean that they should be 0. However, a lazy implementor could also
interpret that they do not need to set their bit-values at all and may
have a system that have random values at initialization of a memory
buffer. I would recommend to use ".. SHALL be set to zero by the
sender."

> 
> Section 4.3 and 4.4: Due to how the Contact Header relate to TLS
> there is clear risk for a TLS stripping attack where the CAN_TLS flag
> is cleared. I think there need to be some thought about mitigation of
> this weakness. Depending on the expected mix of entities and their
> capabilities one can either have policy for a deployment where one
> mandates TLS being used, thus preventing the bid-down by not being
> according to policy. It is more difficult to mitigate in a deployment
> where one have some entities that doesn’t support TLS, unless one can
> some way securely learn which entities support it or not and thus can
> detect the manipulation. One can potentially also first attempt to do
> a TLS handshake for the best version one supports. Then run the CH
> inside the TLS to prevent TCPCL version and other flags to be
> manipulated. But that doesn’t solve the down-bid. I did note the
> negotiation in Section 4.7 and the relation to Security Policy. Maybe
> the solution is to write some text on the risk of TLS striping in
> Section 8 and add forward pointers in 4.7 to that risk. 

Section 8 text makes the biddown clear and indicates the policy
mitigation. I think this is sufficient. 

> 
> Section 4.4: Dealing with new TLS versions. BCP 195 does not appear
> to me to define how to deal with newer versions. However, as TLS 1.3
> already exist I think this is from the start a relevant question. 

I am asking the security ADs if it is current and sufficient with the
BCP 195 reference. 

> 
> Section 4.4: So what about entity authentication? Will the TCPCL
> entity have a name / identity that can be authenticated so that one
> know that one are talking to the right entity. And is the solution
> for this a classical PKI, or something else? Also does the passive
> entity expect the active (TLS client) to authenticate itself also? 

Mostly good new text on the authentication. Here I am going beyond my
expertise, but based on that RFC 5280 has been updated several times, I
think you need to figure out if any of these updates should be
indicated as necssary for TCPCLv4. The relevant updates are RFC 6818
which makes clarifications on RFC 5280 and then the intenationalization
in RFCs 8398, 8399. 

The other concern I have is the SNI sentence: "The TLS handshake SHOULD
include a Server Name Indication from the active peer." First of all
you need a reference for server name indication. It is an TLS extension
and not included in the main TLS specification. For TLS 1.2 it is
section 3 of RFC 6066. The second part is that I don't think it is
clearly specified what should be in the SNI field in TLS. Looking at
the DTN URI scheme definition I would guess that only the node-name
part of the URI should be included, or? 

This also relates to the section 4.4.2 TLS host name validation part.
It says that certificates subjectAltName should be the Node ID. That
raises question marks as no part the DTN URI format defines something
as Node ID. The second is what happens if someone uses another URI
scheme with BP? Or maybe this is simple to resolve if the whole URI
should be used, but that looks less than obvious at least in the case
of DTN. And I haven't looked at the IPN scheme. 

Sorry about being very cautious here. But, I know how much trouble I
have had with URIs and their application in the protocol when I worked
with RTSP 2.0. 

> 
> Section 4.8: Please define how to set and ignore the reserved bits.

Same as above about zero. 

> 
> Section 4.5: Based on that many later sections just refer to the
> Message Header, shouldn’t the section title for section 4.5 be
> Message Header? Now the first mentioning of “Message Header” are in
> Figure 16’s title. 

Fixed.

> 
> 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 (i.e.
>    send an XFER_SEGMENT message) 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. 
> 
> To me it seems that the above paragraph contains one contradiction.
> The parenthesis in the second sentence (i.e. send an XFER_SEGMENT
> message) appears to be false. Because as the beginning of the
> sentence implies that it can’t start a new transfer. However
> SESS_TERM is allowed to be inserted in between XFER_SEGEMENT messages
> for a transfer ID, i.e. XFER_SEGMENT(ID=7), SESS_TERM,
> XFER_SEGMENT(ID=7, end) is an allowed sequence. Can you please
> clarify.

Addressed. 

> 
> Section 8. 
> If this identifier is used outside of a TLS-secured session or 
>    without further verification as a means to determine which bundles
>    are transmitted over the session, then the node that has falsified
>    its identity would be able to obtain bundles that it otherwise
> would
>    not have.
> 
> I don’t see how a entity could trust the in SESS_INIT provided EID
> more just because it is in TLS unless there are some mechanism for
> binding the EID to the TLS session endpoint (client or server). Some
> type of authentication is needed to prove the identity. 
> 

I think the new text in Section 8 and the clarified TLS procedures
resolves this issue. 

> Section 8.
> Therefore, an entity SHALL NOT use the EID value of an
> unsecured contact header to derive a peer node's identity unless it
>    can corroborate it via other means.
> 
> The EID value is not part of the CH, only SESS_INIT. Is this a left
> over from TCPCLv3 or earlier versions?

Fixed.

> 
> Section 8. Needs text on  TLS stripping attack due to the optional
> TLS usage.

Fixed. 

> 
> Section 9: “In this section, registration procedures are as defined
> in [RFC8126].” … defined as in …
> 

Fixed.

> Section 9: “Some of the registries below are created new for TCPCLv4
> but share code values with TCPCLv3.”
> 
> I don’t think “share” is the right word here. Maybe “Some of the
> registries have been defined as version specific to TCPCLv4, and
> imports some or all codepoints from TCPCLv3.”

Fixed.

> 
> Section 9.3: Is RFC Required unnecessary strict? Considering the
> quite large name space, I would think that specification required
> would be a suitable policy? Just trying to understand what you think
> is gained by having someone publish the extension as an RFC, in any
> stream including the independent. 

So this was changed to Expert Review and that is fine. Any registration
requirements or special considerations for the Expert? 

> 
> Section 9.4: Same question as for 30)

Addressed. 

> 
> Section 9.5: Here RFC required may be suitable. However, does it make
> sense to allocate 2 or 4 points also here for Private/Experiments?

Addressed. 
> 
> Section 9.6: Here I would consider Expert Review with a specification
> requirement. I also do not understand why so much is reserved, a
> reason for that? 
> 
> I would expect that the policy for 9.6-9.8 to be aligned so may
> require change if change is decided on 33. 

Fixed. 
> 
> A big question I have after having read this document is how one
> discover / determine the IP+port(s) to connect to for a given EID. Is
> this currently completely deployment specific? And how bound is this
> to Bundle routing? 
> 
> What may be missing is the section that tells the intended
> implementor that in addition to follow this specification you do need
> a solution to the following things, like for example authentication
> framework that allows one to verify the TLS connects to EID mappings.
> Or an address resolution protocol that maps EID to IP address and
> port pairs for cases when routing simply give you Forward this bundle
> to EID using TCPCLv4.

So the scope of the document clarifies things and points to some needs.
So I will consider this one resolved. 

Cheers

Magnus Westerlund 

----------------------------------------------------------------------
Network Architecture & Protocols, Ericsson Research
----------------------------------------------------------------------
Ericsson AB                 | Phone  +46 10 7148287
Torshamnsgatan 23           | Mobile +46 73 0949079
SE-164 80 Stockholm, Sweden | mailto: [email-masked]
----------------------------------------------------------------------

---

From: Magnus Westerlund <[email-masked]>
Date: Thu, 19 Sep 2019 11:14:48 +0000

Hi,

Yesterday's interim meeting did discuss the EID and SNI relation and
our conclusion as I remember them was that the whole URI is put in the
SNI field, and future scheme specific definition may define a sub-set
of the URI is included. 

Due to me reviewing another document that discussed TLS server name
indication (SNI) one thing did get flagged in my head: 

So RFC 6066 defines SNI as a DNS Hostname, however the data type is
opaque bytes. However, an DTN or IPN URI is not a host name, so I think
we may have a clash with what is written in RFC 6066:

   Currently, the only server names supported are DNS hostnames;
   however, this does not imply any dependency of TLS on DNS, and other
   name types may be added in the future (by an RFC that updates this
   document).  The data structure associated with the host_name
nameType
   is a variable-length vector that begins with a 16-bit length.  For
   backward compatibility, all future data structures associated with
   new NameTypes MUST begin with a 16-bit length field.  TLS MAY treat
   provided server names as opaque data and pass the names and types to
   the application.

   "HostName" contains the fully qualified DNS hostname of the server,
   as understood by the client.  The hostname is represented as a byte
   string using ASCII encoding without a trailing dot.  This allows the
   support of internationalized domain names through the use of A-
labels
   defined in [RFC5890].  DNS hostnames are case-insensitive.  The
   algorithm to compare hostnames is described in [RFC5890], Section
   2.3.2.4.

   Literal IPv4 and IPv6 addresses are not permitted in "HostName".

I think it is very worth asking someone with more insight into TLS what
issues would arise if one attempt to put the general EID into the
HotsName definition. 

Cheers

Magnus

On Tue, 2019-09-17 at 19:47 +0000, Magnus Westerlund wrote:
> _______________________________________________
> dtn mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/dtn
-- 
Cheers

Magnus Westerlund

---

From: Stephen Farrell <[email-masked]>
Date: Thu, 19 Sep 2019 12:37:33 +0100

Hiya,

> issues would arise if one attempt to put the general EID into the

Yeah, that could be liable to breakage. I think
(but haven't checked) that various TLS libraries
and applications implement no or some checks on
the value in the SNI extension in what's maybe
an unpredictable manner.

Here's a code comment from OpenSSL, in the code
that parses SNI on the server side:

"
* Although the intent was for server_name to be extensible, RFC 4366
* was not clear about it; and so OpenSSL among other implementations,
* always and only allows a 'host_name' name types.
* RFC 6066 corrected the mistake but adding new name types
* is nevertheless no longer feasible, so act as if no other
* SNI types can exist, to simplify parsing.
"

So OpenSSL does insist on the SNI containing
exactly one host_name (NameType 0 from RFC6066).

From that code, I think the library might be ok
with basically any string value but there is a
callback that applications using the library
can provide, so implementations of that might be
more strict and break if something that's not
syntactically valid as a DNS name were used.

Cheers,
S.

---

From: Magnus Westerlund <[email-masked]>
Date: Tue, 24 Sep 2019 10:54:19 +0000

Hi,

On the particular issue of BCP 195

> I am asking the security ADs if it is current and sufficient with the

As they have defined a mechanism that ensures that a server that supports both
TLS 1.3 and TLS 1.2 can indicate that is supports TLS 1.3 in TLS 1.2 there is no
potential for downgrades here. 

So the simple answer is that BCP 195 is sufficient. 

Cheers

Magnus Westerlund

---

From: Magnus Westerlund <[email-masked]>
Date: Fri, 27 Sep 2019 08:30:39 +0000

Hi,

So looking at -14 I think the remaining issue is only the question if the SNI
field is the right one. I think you may have to educate me here that this is the
right way. However, please my comment below. 

> think a sentence or two should be spent to explain its meaning prior to

The changes do make the process around SESS_TERM message clearer. I think this
is now resolved. 

> which makes clarifications on RFC 5280 and then the intenationalization

This part is resolved. 

> part of the URI should be included, or? 

The new text is very clear on how the SNI field is to be used. However, now it
is Client Hostname Indication rather than the Server Name Indication field.
Without understanding the name and addressing schemes of DTN I have a hard time
to judge if this makes sense or not. I find it a bit backwards to say the least.
If a BP Agent (Charlie) has bundle that is to be delivered to dtn://foo/bar and
does a name resolution of "foo" to learn that foo supports TCPCLv4 and has a
host name of node4.foo.com which maps to an ip 100.10.10.50:5432. The fact that
when Charlie is establishing the TCP/TLS connection it will do a TCP connect to
100.10.10.50:5432 and then the TLS SNI Field says "Charlie". How will the server
entitiy at 100.10.10.50:5432 location know that Charlie is suppored to talk to
node4.foo.com rather than node9.delta.com if they actually are co-located? I
would have expected that SNI would indicate the host, in this case node4.foo.com
rather than Charlie.

> of DTN. And I haven't looked at the IPN scheme. 

To me this part now looks clear. 

Cheers
 
Cheers

Magnus Westerlund