[tcpm] AD review of draft-ietf-tcpm-converters-14

[Mirja Kuehlewind <[e-mail]>]

Hi authors,

I finally did my AD review for draft-ietf-tcpm-converters-14 (actually I'm still not fully finished with the review of the appendix but would like to send out this first batch of questions now).

As you can see below, I have a whole bunch of questions/comments. Some of these may only be editorial in the end but I would like to make sure that I understand all the technical parts correctly before we move on with the processing. 

I think there are basically two main technical points I would like to get some clarification on, both probably related to things that have been introduced later in the life time of the doc but maybe not consequently been changed throughout the whole doc. 

One is besiaclly point 8 below but there are some related other points below: It is not fully clear which packets can carry Convert messages. I think initially Convert messages were only supposed to be in the SYN and SYN/ACK. But then later you enabled it also for other packets, however, it seem the only real case that needs this is when an error message is send by the client. But sending Convert messages in other packet than the SYN and the SYN/ACK seems more complicated because all TCP payload data is transmitted reliable and must be ack'ed and evil. retransmitted. Maybe it’s okay if only an error message is send and a reset right after (so no retransmission), however, this is not specified, and I wonder if the complexity is really needed worth the benefit of being able to log more concrete errors in the converter.

The other point is basically point 12 below about the TCP option field in the Connect TLV (also related to point 17 below). This is not well enough specified and seem also quite complex as a generic mechanism. I would think the converter should usually try to use the same option as the client did send in his SYN to the converter, if supported by the converter TCP implementation. The TFO cookie might be a special case. I’m not sure if that needs to be generalised or if having a separate TLV for that one case might be simpler. Please also reconsider this but in any case it needs more explanation in the spec.

Please see all my questions/comments below. I may send another message, when I have reviewed the appendix tomorrow (hopefully with none or only few additional comments).

Thanks!
Mirja

----------------------

1) I was initially a bit confused about the setup in Figure 6 and respectively the paragraph just before that figure in sec 3.2: It wasn't clear to me how the server might know the converter address (or if the proxy maybe has to be on path). I assume the “use case” is that there was a previous connection using the converter and now the server tries to reconnect but of course only knows the converter address, or something? Maybe you could add slightly more text here. I also I would recommend to explicitly mention that this setup still has to be explicitly configured by the client but using an out of band protocol (which is out or scope for this spec). Further I think it would be good to mention here already that the converter also inserts the respective TCP option in the SYN to the client (if possible due to space limits) and refer to section 4.2 for a concrete example.

Btw. shouldn't there be some discussion about MTU issue if options are added by the converter?

2) The following paragraph in section 3.2. seems to specify protocol requirements but don’t use normative language:

"If the downstream (or upstream) connection fails for some reason
   (excessive retransmissions, reception of an RST segment, etc.), then
   the Converter should force the tear-down of the upstream (or
   downstream) connection.

   The same reasoning applies when the upstream connection ends.  In
   this case, the Converter should also terminate the downstream
   connection by using FIN segments.  If the downstream connection
   terminates with the exchange of FIN segments, the Converter should
   initiate a graceful termination of the upstream connection.”

I recommend to use normativen language here (SHOULD instead of should; or maybe even MUST?). However, as this text is part of a kind of overview section, it doesn’t seem to be a really good fit to use normative language in that section. So maybe the whole discussion about use of TFO (or not) should be moved to an own section?


3) Just to double-check, I'm wondering a bit about this phrasing in sec 3.3.1:
"If no entry is found, the Transport Converter MUST silently
   ignore the packet.”
That means the converter will drop the packet (with no other action), right? Why do you use term ignore here (instead of drop or discard)? Or does this have any other implication?
Note that this term is used several times in the doc.

4) Also a quick question about this in sec 3.3.1:
"A Transport Converter may operate in address preservation mode (that
   is, the Converter does not rewrite the source IP address (i.e.,
   C==T)) or address sharing mode (that is, an address pool is shared
   among all Clients serviced by the Converter (i.e., C!=T)); refer to
   Appendix D for more details.  Which behavior to use by a Transport
   Converter is deployment-specific.”
I guess preservation mode can only be used if the converter is on path. I think that should be explicitly noted here. 

5) Sec 3.3.2: To be honest I’m not sure I fully understand this section, especially this sentence:
"Upon receipt of a secondary subflow by the Transport Converter from a
   Client, the Converter follows the same behavior specified in
   Section 3.3.1 for processing Non-SYNs.”
Do you mean by "receipt of a secondary subflow” the SYN on that subflow or other data? For the SYN I would expect that there is no payload data and therefore nothing to proxy. For other subflow packets, I guess you need to reorder first, so probably it would be good to mention that…? Maybe it’s just me misunderstanding something but I believe more explanation is needed here.

6) Can you maybe add some more text (in section 5 maybe) why a separate port number is used/needed? As the client has to be explicitly configured it could easily be configured with an address and port number. Is this to avoid collisions with other protocols? What would be the scenario here?

7) Sec 5.1:
"The Unassigned field MUST be set to zero in this version of the
   protocol.  These bits are available for future use [RFC8126].”
Why is there a reference to RFC8126 here?

8) Also sec 5.1 but probably editorial:
"Data added by the Convert Protocol to the TCP bytestream is
   unambiguously distinguished from payload data by the Total Length
   field in the Convert messages.”
I believe what you want to say here is that the total length is used to figure out where potential other payload data might start. However, what I would understand from this sentence is that the total length field can be used to distinguish SYNs that carry the Convert protocol from SYNs that may carry other payload only, which I don’t think is the case.

8) I first have a more general question about the function of the protocol. Sec 5 says:
"Convert messages may appear only in a SYN, SYN+ACK, or ACK.”
I’m not sure I understand the ACK case here because if you add a CONVERT message to a TCP packet that means you add payload data. I was reading this as pure ACK but that can't be the case. So do you mean by this you can only send new data if your previous data was ACK or something else…?
However, I believe there is usually just one message from the client in the SYN and the one message from the converter in the SYN/ACK, and then the connection is either closed or switched to the actual payload transmission. So I was assuming that's the only communication pattern you can have. Or is the idea that multiple client-converter exchanges can be done on the same TCP connection also after the TCP handshake and then any time in the connection you would be able to switch over to sending other payload data? I think this need further clarification in the draft.

Note that section 7 also say:
"In this section, we only discuss
   the middleboxes that modify SYN and SYN+ACK packets since the Convert
   Protocol places its messages in such packets.”

9) This point is probably related to the previous point. In section 5.1 you say that the connection MUST be reset (if length is zero) but in all other sections you say the connection must be closed if there is a problem. Assuming the Convert protocol will only be used in SYN and SYN/ACK, reset is probably more appropriate. If it can be used also in later packets you probably have to say something like “close or reset if the handshake is not completed”…? 

E.g. see sec 5.2.1:
"If two or more
   instances of the same TLV are exchanged over a Convert connection,
   the associated TCP connections MUST be closed.”
Is that close or reset? 

Also related:
The next section (5.2.2) says:
"Type 0x0 is a reserved valued.  Implementations MUST discard messages
   with such TLV.”
(Nit s/valued/value/)
And in sec 5.2.5:
"Connect TLVs witch such messages MUST be discarded by the Transport
   Converter."
I guess every time you say in the document that a message is discarded that would  also lead somehow to closing the connection most likely by to sending a reset, no? Or should the SYN just be drop and no reply send for any reason? Please clarify.

10) Probably editorial in sec 5.2.4:
"A Transport Converter SHOULD include in this
   list the TCP options that it accepts from Clients; these options are
   included by the Transport Converter in the SYN packets that it sends
   to initiate connections.”
I had to read the second half twice because it suddenly talks about a completely different “scenario” than replying to an Info TLV. Maybe some rewording could help a bit like
"A Transport Converter SHOULD include in this
   list the TCP options that it accepts from Clients; these options are
   also included by the Transport Converter in the SYN packets if it
   initiates connections to the client.”
Or maybe even use normative language here: 
“the Transport Convert SHOULD also include the same option in the SYN if 
   it initiates a connection to the client.”

11) sec 5.2.5: Maybe also be slightly more clear here:
"For incoming connections destined to a Client
   serviced via a Transport Converter, these fields convey the source
   port number and IP address.”
Proposed
"For incoming connections destined to a Client
   serviced via a Transport Converter, these fields convey the source
   port number and IP address of the SYN packet received by the 
   Transport Converter from the server.”

12) I have a couple of question about the TCP options field in the Connect TLV, basically this text in section 5.2.5:
"   Upon reception of a Connect TLV, and absent any policy (e.g., rate-
   limit) or resource exhaustion conditions, a Transport Converter
   attempts to establish a connection to the address and port that it
   contains.  The Transport Converter MUST use by default the TCP
   options that correspond to its local policy to establish this
   connection.  These are the options that it advertises in the
   Supported TCP Extensions TLV.


Bonaventure, et al.        Expires May 7, 2020                 [Page 22]
Internet-Draft              Convert Protocol               November 2019


   Upon reception of an extended Connect TLV, and absent any rate limit
   policy or resource exhaustion conditions, a Transport Converter MUST
   attempt to establish a connection to the address and port that it
   contains.  It MUST include the options of the 'TCP Options' sub-field
   in the SYN sent to the Server in addition to the TCP options that it
   would have used according to its local policies.  For the TCP options
   that are listed without an optional value, the Transport Converter
   MUST generate its own value.  For the TCP options that are included
   in the 'TCP Options' field with an optional value, it MUST copy the
   entire option for use in the connection with the destination peer.
   This feature is required to support TCP Fast Open.”

First of all the upper paragraph is probably old and should have been removed, right?

Then I’m not certain about the new approach with the TCP option field. If the converter actually ends up in a split connection (with e.g. MPTCP on client side but without it on server side), I thought it can only announce those options in the SYN to the server that are actually implemented in the converter and it will be able to support later in the connection. So blindly copying the options provided by the client doesn’t seem right. Or what do I miss?

I understand the case where you want to use TFO between the client and the convert but can’t use it anymore between the the client and the server then. However you can only use TFO with the second connection you make to a server. So in the first connection either TFO was used between the convert and the server and the convert could now use it again with the cookie it received or TFO was used end-to-end in the first connection and it now clear to me why the converter is used now. Maybe I’m missing something but I think the drafts would at least need more explanation about the motivation to have this.

If it’s really only about TFO cookies and we are sure we want to have that feature, maybe an own TLV would be simpler?

13) In sec 5.2.6 do you mean by "extended TCP header” the TCP option space? I’m not sure that "extended TCP header” is a clearly defined term; as least I’m not 100% sure what you mean…

14) nit in sec 5.2.7:
s/The Cookie TLV (Figure 20 is an optional/The Cookie TLV (Figure 20) is an optional/ -> missing closing bracket

15) Sec 5.2.7:
Question about normative language:
"If the received SYN did not contain a Cookie TLV, and cookie
   validation is required, the Transport Converter should compute a
   Cookie bound to this Client address and return a Convert message
   containing the fixed header, an Error TLV set to "Missing Cookie" and
   the computed Cookie and close the connection.”
Is this maybe a MAY condition? All of it? Or something like this
"If the received SYN did not contain a Cookie TLV, and cookie
   validation is required, the Transport Converter MAY compute a
   Cookie bound to this Client address. It MUST return a Convert message
   containing the fixed header and Error TLV set to "Missing Cookie” and MAY add
   the computed Cookie. After sending the Error TLV is MUST/SHOULD close/reset   
   the connection.”
Not fully sure what is the intention here…

15) sec 5.2.8:
What's the purpose of the client sending an Unsupported Version error? If the converter replies with a different version than requested used by the client, that's simply a fatal error/implementation error and the client can only reset the connection I think.

More generally if I read this correctly there are only 3 errors that could be sent by the client. I guess those errors would be send in the first packet after the SYN/ACK is received…? Related to my question above, I think it would be much easier to only have Convert message in the SYN and SYN/ACK and no explicit error message from the client. Otherwise more explanation in the draft would be need how this actually is supposed to work.

16) sec 5.2.8:
"A Client which receives this error code MUST cache the received
      Cookie and include it in subsequent Convert messages sent to that
      Transport Converter.”
This seems to be a slightly weird normative MUST. Sure you have to cache the cookie in oder to be able to use the converter, however, there may be cases where you loose the cache and then sending a Connect without the cookie to get a new Cookie is a valid option. So you may use SHOULD here or even better reword it completely...

17) There is an Unsupported TCP Option in section 5.2.8. However, this error is not mentioned in 5.2.5. In contrast sec 5.2.5 says that the converter MUST open a connection and MUST use these options. This is related to my point 12 above but it was not clear to me that options can be rejected, however, this whole part seems to make the protocol really complicated and as I said if the only use case is TFO I would prefer to rather have a separate specific TLV for that case only.

18) I think section 6.7 should still say something about if this option is supported or not…

19) section 7:
"Consider a middlebox that removes the SYN payload.  The Client can
   detect this problem by looking at the acknowledgment number field of
   the SYN+ACK returned by the Transport Converter.  The Client MUST
   stop to use this Transport Converter given the middlebox
   interference.”
If the converter received a SYN without a Convert message in the payload on the respective port, it is supposed to anyway send a SYN/ACK? I would assume it would rather send a RESET, no? I think that needs to be further specified.

20) Sec 7 also says:
“If an Error was returned by the Transport Converter, a message to close
   the connection would normally follow from the Converter.”
However sec 5.2.8 says:
"Upon reception of an Error TLV, a
   Client MUST close the associated connection.”
I assume one of these is not correct… please clarify which end is suppose to do what in case of an error.

21) The text in sec 7 then further says:
"If no such
   message is received, the Client may continue to use this Converter.”
Isn’t that a bit dangerous?  

22) section 8.5: Why are the authentication mechanism discussed in this section MPTCP specific? I think the same mechanisms could also be applied to Converters supporting other options, no?

23) sec 8.3:
"Means to protect against SYN flooding attacks MUST also be enabled [RFC4987].”
Not sure if the use of MUST is clear here. Which part of RFC4987 does this MUST relate to? All of it? Maybe a SHOULD or no normative language is more appropriate?

24) sec 9.2: Maybe call the new registry the "The TCP Convert Protocol (Convert)
   Parameters” (TCP added)…?

25) sec 9.2.2:
"The values in the range 192-255 can be assigned for Private Use.”
IANA does not assigned any value for Private use, they can just be used, so this should be
"The values in the range 192-255 are reserved for Private Use.”
if that is what you want?

26) also on section 9.2.X: "Specification Required" includes having an expert review and RFC8126 says:
"As with Expert Review (Section 4.5), clear guidance to the designated
   expert should be provided when defining the registry, and thorough
   understanding of Section 5 is important.”
So please provide further guidance about what the expert is supported to evaluate. 

27) Not sure if the following references need to be normative as there no normative requirement connect to them but only an optional case is discussed: RFC4279, RFC7250 …?

However, RFC7323, RFC2018, RFC6978, RFC6978, RFC2827, RFC6181 should probably be normative references…?

28) The term “experimental option” is used with two different meaning in the intro of section 6 (options that are defined in an exp RFC) and in section 6.9 (options with kind 254 and 255). Please clarify this at both places.