Re: [multipathtcp] WG Last Call for draft-ietf-mptcp-rfc6824bis

[Christoph Paasch <[e-mail]>]

Hello Phil,

please see inline:

On 18/06/18 - 13:59:12, [e-mail] wrote:
> More comments:
> Section 3.5 - 
> Option R (RST) - add (at the end of the bullet) "the Host A enters the CLOSED state directly" (I think)
> 
> << If a host receives a packet with a valid MP_FASTCLOSE option, it
>    shall process it as follows :>>
> I think the text that follows (apart from the last bullet) is only relevant for Option A, where the MP_FASTCLOSE is sent on an ACK - it doesn't apply for option R (where it's sent on a RST). Anyway, things could be laid out slightly differently so the section is clearer. 

Agreed.

> Section 3.6
> I wonder if it would be more logical for this section to be before Fast close?

Hmmm... Maybe yes. On the other hand, 3.6 is not vital to full MPTCP
operation. It is purely informational, thus having the MP_FASTCLOSE before
that could make sense.

> This section could do with a read through. A few things:
> << As discussed in Section 3.5 above, the MP_FASTCLOSE option provides a
>    connection-level reset roughly analagous to a TCP RST. Regular TCP
>    RST options remain used to at the subflow-level to indicate the
>    receiving host has no knowledge of the MPTCP subflow or TCP
>    connection to which the packet belongs.>>
> Can this paragraph be deleted? at least I found it a bit confusing that this text is not about this section. 

Agreed - I rephrased the beginning of the section, using your suggestion
from below.

> "remain used to at" is ugly
> 
> << However, in MPTCP, there may be many reasons for rejecting the
>    opening of a subflow, but these semantics cannot be carried in a
>    standard TCP RST. It would be beneficial for a host to the reasons
>    why its subflow has been closed with a RST, and thus whether it
>    should try to re-establish the subflow immediately, later, or never
>    again.  These semantics are carried in the MP_TCPRST option that can
>    be included on a TCP RST packet.>>
> I presume the new MP_TCPRST can be added to a RST, whenever it's created - the sentence seems to imply it's only when it wants to reject the opening of a subflow (ie in response to MP_JOIN). 
> I assume inclusion of the MP_TCPRST option is optional?
> 
> potential better phrasing:
> A host sends a TCP RST in order to close a subflow or reject an attempt to open a subflow (MP_JOIN). In order to inform the receiving host why a subflow is being closed or rejected, the TCP RST packet MAY include the MP_TCPRST Option. The host MAY use this information to decide, for example, whether it tries to re-establish the subflow immediately, later, or never. 

I used your text.

> 
> <<   o  Unspecified error (code 0x0).  This is the default error implying
>       the subflow is not longer available.  The receiving host SHOULD
>       take account of the 'T' bit in deciding whether to re-estbalish
>       this subflow.  The presence of this option shows that the RST was
>       generated by a MPTCP-aware device.>>
> 
> " The receiving host SHOULD take account of the 'T' bit in deciding whether to re-estbalish this subflow."
> Only this sub-bullet mentions the T bit, which may suggest that it should not be taken account of with the other reason codes.  I think the sentence should be deleted (using the T flag is mentioned earlier). 

Agreed.

> 
> <<enough ressources>>  enough resources

Fixed.

> 
> Section 3.7
> I guess it's OK to refer to it as RST rather than TCP RST?

I think so. What do others think?

> << The case described above is a specialized case of fallback, for when
>    the lack of MPTCP support is detected before any data is acknowledged
>    at the connection level on a subflow.  >>
> "The case above" - I'm not sure which of the previous paragraph or paragraphs you're referring to.
> "specialised" - maybe "specific" [is it really specialised?]
> "for when" -> when

I'm actually not sure what this paragraph is adding to the section.
Should we simply remove it?

> 
> S3.9.2
> << If the two communicating hosts immediately try to set up subflows
>    from all available addresses to all available addresses on the other
>    host, this could end up creating two subflows per path.  This is an
>    inefficient use of resources.>>
> is two subflows correct? Multiple I think

It would be two subflows on each path (one being initiated by each host).

> " could end up creating two subflows per path" -> "could create multiple subflows over the same path"
> 
> << If a host does not support TCP simultaneous open, it
>    is RECOMMENDED that some element of randomization is applied to the
>    time waited before opening new subflows, so that only one subflow
>    exists between a given address pair.  If, however, hosts signal
>    additional ports to use (for example, for leveraging ECMP on-path),
>    this heuristic need not apply.>>
> "time waited" - ugly phrase?

Changed to "time to wait"

> "exists" -> is created
> "need not apply" -> is not appropriate

Both fixed.

> 
> Section 5 - Security
> This section needs updating to reflect the changes in the bis.:
> 
> Add mention of ADD_ADDR, now with HMAC and reliability,
> Discuss the revised MP_CAPABLE
> I'm not sure if the new fast close option (option R) has any security impact?
> Discuss the MP_PRIO change (no off path address indication)
> Discuss downgrade attacks (to v0) during connection initiation

Will work on those.

> Section 8 IANA
> << This document updates [RFC6824] and as such IANA is requested to
>    update the TCP option space registry to point to this document for
>    Multipath TCP>>
> This document obsoletes rather than updates...   I suspect this means all the MPTCP options need to be included in this document, but we can leave IANA to decide that. 

Changed "updates" to "obsoletes".

> 
> <<  |    C    |   Do not attempt to connect to   |    This document,    |
>    |         |          source address          |     Section 3.1      |
> >>
> I don't think this is a good summary of flag C's meaning.

Changed to "Don't reuse source address for new subflows."

> 
> << The
>    contents of this sub-registry are to to this document, as follows:
> >>
> Is this sentence needed?

I don't think so.

> Appendix B TCP Fast Open
> Maybe rephrae the title as "TCP Fast Open and MPTCP" or "Considerations about using TCP Fast Open and MPTCP"?

Picked "TCP Fast Open and MPTCP".

> This section could do with a read through, it could do with a few minor re-phrasings
> 
> 
> <<   TCP Fast Open (TFO) is an experimental TCP extension, described in
>    [RFC7413], which has been introduced with the objective of gaining
>    one RTT before transmitting data.>>
> " gaining one RTT"?  maybe: which allows data to be sent one RTT earlier than with regular TCP. 

Changed.

> << It achieves this by sending the SYN-
>    segment together with data>>
> re-phrase (I think data is referring ot the cookie - but you havent yet mentioned it - rather than user data)

'data' here is referring to user data. I changed it to "with the application's data", to make it more clear.

> 
> "resp." = ?

Changed to respectively.

> << TFO and MPTCP can be combined provided that the total length of their
>    options does not exceed the maximum 40 bytes possible in TCP>>
> "their" is wrong, as there may be other options. -> "all the"

Fixed.

> << We also note that this does not entail a loss
>    of functionality, because TFO-data is always sent when only one path
>    is active.>>
> -> "TFO-data is only ever sent when only a single path is active"
> How do you guarantee this? Not sure I understand. 

I rephrased this to say that TFO is only ever used on the initial subflow
before any other subflows are attempted.

> << To solve this, the TFO data should not be considered part of the Data
>    Sequence Number space:>>
> Is this a SHOULD NOT?  Does it imply a change to rfc7413 (the tfo rfc)? I guess this is really a suggestion to the implementer?

I think, it should even be a "MUST NOT". Otherwise, implementations won't
interoperate. As this is in the appendix, I change it to "must not".

> Section B.3
> I assume this sub-section is an example (so no SHOULD or even MAY are needed)

Indeed - changed the MUST to a must.

> 
> << client                                                         server
>      |                                                              |
>      |    S 0(0) <MP_CAPABLE>, <TFO cookie request>                 |
>      | -----------------------------------------------------------> |
> >>
> Can you explain the S 0(0) nomenclature/ please.

tcpdump-style sequence number and length of the segment.

Is it ok, if I change the figure for the first time we use this notation to:
S Seq=0(Length=0)

And from there on leave the one we have now?

> Appendix D Finite state machine
> I presume we don't need similar FSM for fastclose?
> 
> 
> Section 2:
> Need to add sub-sections on closing subflow & fast close & fallback

I will work on those.

> 
> S2.7 - I think the third bullet about notable features could be slightly altered, with enhanced security for ADD_ADDR and MP_CAPABLE. You could also add a mention of rfc7430

Yes, we should do this.

> 
> 
> Section 2.1 & 3.1 - can you remind me why we changed MP_CAPABLE so that Host A's key is sent only on the ACK (not on the SYN)? Is this so it's only sent when we know host B is MPTCP capable, which is therefore a slight security improvement?  (Just send a link to earlier emails, if that's easier)

The reason for this is because now that we send the keys reliably in the
third ACK / first data-segment, we actually don't need to send the key in
the SYN anymore. Thus, we can reduce the size of the MP_CAPABLE in the
SYN-segment and gain some bytes.


I created another pull-request for review with most of the minor edits at https://github.com/multipath-tcp/draft-ietf-mptcp-rfc6824bis/pull/5


Christoph


> 
> Thanks
> phil
> -----Original Message-----
> From: Eardley,PL,Philip,TUD1 R 
> Sent: 14 June 2018 11:56
> To: '[e-mail]' <[e-mail]>; [e-mail]
> Subject: RE: WG Last Call for draft-ietf-mptcp-rfc6824bis
> 
> A few comments as I work through the document, basically minor so far
> 
> Section 3.1
> << Given the SYN exchange is different between v1 and v0
>    the exchange cannot be immediately downgraded, and therefore if the
>    far end has requested a lower version then the initiator SHOULD
>    respond with an ACK without any MP_CAPABLE option, to fall back to
>    regular TCP.>>
> I think it should say "to ensure the exchange cannot..."
> 
> 
> << The first 4 bits of the first octet in the MP_CAPABLE option
>    (Figure 4) define the MPTCP option subtype (see Section 8; for
>    MP_CAPABLE, this is 0), and the remaining 4 bits of this octet
>    specify the MPTCP version in use (for this specification, this is 1).
> >>
> "The first 4 bits of the first octet in the MP_CAPABLE option" Fig 4 shows two earlier octets (Kind & Length) - I know these arent the MP-CAPABLE option, but I think it might be worth spelling out what they are. 
> 
> Would it be better strictly to have "0x0" rather than "0"? (ie match the IANA registry)
> 
> Section 3.4.1
> <<A set of four flags are present after the subtype and before the
>    Address ID.  Only the rightmost bit - labelled 'E' - is assinged
>    today.  The other bits are currently unassigned and MUST be set to
>    zero by a sender and MUST be ignored by the receiver.
> 
>    The 'E' bit exists to provide reliability for this option.  Because
>    this option will often be sent on pure ACKs, there is no guarantee of
>    reliability.  Therefore, a receiver receiving a fresh ADD_ADDR option
>    (where E=0), will send the same option back to the sender, but not
>    including the HMAC, and with E=1.  The lack of this echo can be used
>    by the initial ADD_ADDR sender to retransmit the ADD_ADDR according
>    to local policy.
> >>
> 4 comments:
> Assinged /assigned
> 
> The 'E' bit - better to say flag?
> 
> Section 2.3 needs to be amended to show the reply /echo message for ADD-ADDR. I suggest the pic in S2.3 also shows the E value. 
> 
> Not sure - does this open security hole, where an attacker pretends an ADD-ADDR has been received? (the Echo doesn't include an hmac, so is easy to spoof)
> 
> And in Fig 12
> |        Truncated HMAC (8 octets, if length > 10 octets)       |
> 
> Could this be made more straightforward? Ie spell out when length is >10 octets ie IPv6 (if I understand it). This also seems to mean that IPv4 doesn't include the HMAC, is this right??
> 
> Thanks
> phil
> 
> -----Original Message-----
> From: multipathtcp [mailto:[e-mail]] On Behalf Of [e-mail]
> Sent: 05 June 2018 09:14
> To: [e-mail]
> Subject: [multipathtcp] WG Last Call for draft-ietf-mptcp-rfc6824bis
> 
> This starts a WG Last Call for draft-ietf-mptcp-rfc6824bis. Please send comments by the end of June. 
> 
> Please note there are three IPR disclosures (we're working on getting them added to the rfc6824bis page): 
> 
> * two are inherited from RFC6824  https://datatracker.ietf.org/ipr/search/?submit=draft&id=draft-ietf-mptcp-multiaddressed    
> * one is inherited from draft-paasch-mptcp-syncookies (which got include in rfc6824bis) https://datatracker.ietf.org/ipr/2678/ 
> 
> Thanks,
> Phil & Yoshi
> _______________________________________________
> multipathtcp mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/multipathtcp
> 
> _______________________________________________
> multipathtcp mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/multipathtcp