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

[Mirja Kuehlewind <[e-mail]>]

Hi Med,

Regarding the port, I think that#s a good approach. As the whole working group is cc’ing, let’s wait a week or so to see if there are only objection as you propose.

Mirja





> On 24. Jan 2020, at 13:42, <[e-mail]> <[e-mail]> wrote:
> 
> Hi Mirja, 
> 
> We are currently preparing changes to take into account your review. Our plan is to have a revised version by the next week but I would like to report on this comment: 
> 
>> 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?
> 
> After carefully re-reading https://tools.ietf.org/html/rfc7605#section-7.1 and taking into account the deployment models, an IP address will be needed to be supplied/discovered anyway... so the port number can be supplied/discovered as well. 
> 
> Unless there is an objection, we will update the draft to only register a service name without asking for any specific port. The name will be typically used to build service labels that can be then used to retrieve a service port number using, e.g., DNS SRV. 
> 
> Thank you.
> 
> Cheers,
> Olivier & Med
> 
>> -----Message d'origine-----
>> De : Mirja Kuehlewind [mailto:[e-mail]]
>> Envoyé : jeudi 9 janvier 2020 22:28
>> À : [e-mail]
>> Cc : tcpm IETF list
>> Objet : AD review of draft-ietf-tcpm-converters-14
>> 
>> 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.
>> 
>> 
>> 
>> 
>> 
>> 
>> 
>> 
>> 
>