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

[<[e-mail]>]

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.
> 
> 
> 
> 
> 
> 
> 
> 
>