Re: [multipathtcp] 6824bis -11 review

[Alan Ford <[e-mail]>]

Hi Rahul,

Thank you very much indeed for this review.

> On 28 Jul 2018, at 08:38, Rahul Arvind Jadhav <[e-mail]> wrote:
> 
> Hi,
> 
> As promised  in IETF102, here is my review for 6824bis-11. As Phil had suggested, I didn't take the trouble to check if my comments overlap with any of the previous discussions/comments. 
> 
> 1. Section 3.6: Subflow Reset
> [RJ]: Is MP_TCPRST a SHOULD or MUST to be included along with the TCP_RST ? Can this be stated explicitly in section 3.6?

It’s a MAY :)  We say:

"These semantics are carried in the MP_TCPRST option that can be included on a TCP RST packet.”

We should change that to “MAY be included”.

> 2. Section 3.4.2. Remove Address
>   "The sending and receipt (if no keepalive response was received) of
>   this message SHOULD trigger the sending of RSTs by both hosts on the
>   affected subflow(s) "
> [RJ]: Since a MP_TCPRST should be associated with this RST, what should be the corresponding Reason Code that can be used in MP_TCPRST for this case? I checked existing reason codes in section 3.6 and none of those made sense (may be except for 0x0 ?).

That’s an interesting one. There may be some benefit (maybe for middleboxes, or maybe for security) to sharing this information in the RST with an explicit reason code. We could add one - but I would be interested in hearing other opinions.

> 3. Section 3.4.1. Address Advertisement 
>   "A host can send an ADD_ADDR message with an already assigned Address
>   ID, but the Address MUST be the same as previously assigned to this
>   Address ID, and the Port MUST be different from one already in use
>   for this Address ID."
> [RJ]: Didn't understand the rationale for allowing reuse of Address ID only for port change and not for IP change.

I cannot recall the rationale here either.

Furthermore - as written - this would seem to prevent retransmission. Which is crazy.

If nobody can recall the reason for this I would suggest we remove it.

> 4. Section 3.4.1. Address Advertisement
> [RJ]: For long-lived MPTCP connection, the ADD_ADDR needs to be resent if the sub-flow is closed and if it needs to be reopened in the future. Is this correct?

No. Unless an Address + ID has been explicitly removed, it remains valid.

> The reason for me to believe this is, if REMOVE_ADDR is sent and if the sub-flow is active/functioning, then the REMOVE_ADDR will be ignored. This means that when the sub-flow is closed, the corresponding Address ID is removed. Thus it means to establish a new sub-flow from the peer an ADD_ADDR has to be sent.

Hmm. That’s not the intention, but I appreciate things are not completely clear. We say:

   The receipt of REMOVE_ADDR SHOULD first trigger
   the sending of a TCP keepalive [RFC1122] on the path, and if a
   response is received the path SHOULD NOT be removed.

So if REMOVE_ADDR is received for an already-active path, the intention is that it should be silently ignored.

I appreciate this does not explicitly state the logical next step from this. My first thought was that the Address ID and address in the table are still active, so if the subflow later was terminated, the far end would be legitimately able to re-establish it. But that might not be best.

I’m not immediately sure what’s best here for clarification. Either:
a) state that if the REMOVE_ADDR is ignored, the address remains active in the table for the remainder of the MPTCP connection;
b) state that if the REMOVE_ADDR is ignored, the subflow remains active but the address is removed from the table for any future connections.

This is to cover a corner case which should never happen, of course. I think option b is probably best.

> 5. passive opener
> [RJ]: The term "passive opener" is used several times in the document. This term needs to be clarified in Terminology section

Thank you. We got told off for “server” in the early days, hence why that appeared. You are right we need to be consistent.

> 6. Section 3 Figure 3
> [RJ]: Length field in figure 3 encompasses Kind, Length and Option payload. This should be explicitly stated.

That is explicitly stated in the RFC793 definition of TCP options. However, I see no harm in repeating it here.

> 7. Section 3.1 says: "it is expected that A will have generated a key locally before the initial SYN" 
> [RJ]: Is there any advantage of generating the key in advance? The key generation can be avoided by A if node B responds without MP_CAPABLE. Can this be left to the implementation?

I think so yes. This is probably just a hangover from MPTCP v0, and is definitely an implementation detail.

> 8. Section 3.2 says: The MP_JOIN option includes an "Address ID".  This is an identifier that only has significance within a single connection,
> [RJ]: The significance of "Address ID" is only within the connection and __only on the peer__ who generates the ID. Is this correct? 

Yes. We can clarify.

> 9. Section 3.3 says: M = Data Sequence Number (DSN), Subflow Sequence Number (SSN), Data-Level Length, and Checksum present
> [RJ]: and Checksum present (if negotiated)

Thank you.

> 10. [RJ]: Can an attacker spoof MP_FAIL option to the receiver with DSN=0 (infinite mapping)? This will cause the receiver to discard all the data following DSN and the failed data won't be DATA_ACKed. Is there any way to protect this?

MP_FAIL DSN must be within the DSN window.

> 11. [RJ]: I was hoping to find a section in the introduction which summarizes changes in v1. This could be of help for someone who aims to migrate/understand briefly the implications of switching to v1. 

One has been circulated on the list; it could be included in an Appendix.

> ------------Editorials:
> Section 2.7:
> Currently: "To meet the threats identified in ..."
> Should be: "To address the threats identified in ..."
> 
> Section 2.7:
> Currently: "keys are sent in the clear in the MP_CAPABLE messages"
> Should be: "keys are sent in clear in the MP_CAPABLE messages"
> 
> Section 3.1:
> Spellfix: requsted -> requested
> Spellfix: wishs -> wishes
> 
> Section 3.1:
> Currently: "The key MUST be hard to guess, and it MUST be unique for the sending host at any one time."
> Should be: "The key MUST be hard to guess, and it MUST be unique for the sending host across all MPTCP connections."
> 
> Section 3.1:
> Currently: "there is not a collision"
> Should be: "there is no collision"
> 
> Section 3.1:
> Currently: "The receiver will acknowledge this data a the connection"
> Should be: "The receiver will acknowledge this data at the connection"
> 
> Section 3.4.1:
> Spellfix: assinged -> assigned
> 
> Section 3.6:
> Currently: "Regular TCP RST options remain used to at the subflow-level"
> Should be: "Regular TCP RST options continue to be used at the subflow-level"
> 
> Section 3.6:
> Spellfix: re-estbalish -> re-establish
> Spellfix: ressources -> resources
> 
> Section 3.6:
> Currently: "It would be beneficial for a host to the reasons ..."
> Should be: "It would be beneficial for a host to provide the reasons …"


Thank you again for all this feedback.

Best regards,
Alan