Thank you for your review. I have created a PR to address some of these comments.
https://github.com/tlswg/draft-ietf-tls-esni/pull/650 > The following comments are not blocking, but I would really appreciate feedback > to help understand what is intended: > > 1) In section 6.1 what is required for interop? On Offering ECH I see a list of > seven requirements and I thought I saw at least some of these 7 requirements > were necessary for correct operation of the remaining algorithm. (e.g., For me, > Rule 1 suggests a need for TLS 1.3 or higher and if not supplied, a later rule > says this results in a server reject for ECH, etc). However, I found this > sentence after the list: “Note that these rules may change in the presence of > an application profile specifying otherwise.” - I am trying to understand if > any of these requirements are actually required for correct operation: Does > that additional sentence simply mean these are a default and all these > requirements can be changed ? or something else? > > 2) A similar phrase follows the seven requirements in section 6.2. What is > intended to happen here? I don't actually see the text in 6.2. Can you quote the larger passage? This is a good catch. Obviously, anything here that isn't a MUST is not truly necessary. This leaves us with: > 1. It MUST offer to negotiate TLS 1.3 or above. This isn's strictly required for interop but I don't think but will produce a non-conformant appearing transcript because you will get TLS 1.3 in the ServerHello but not in the ClientHello. > 2. If it compressed any extensions in EncodedClientHelloInner, it > MUST copy the corresponding extensions from ClientHelloInner. > The copied extensions additionally MUST be in the same relative > order as in ClientHelloInner. I think this is strictly required, because otherwise things won't actually work. > 3. It MUST copy the legacy_session_id field from ClientHelloInner. > This allows the server to echo the correct session ID for TLS > 1.3's compatibility mode (see Appendix D.4 of [RFC8446]) when ECH > is negotiated. This actually isn't required for DTLS, as DTLS does not use this feature (See RFC 9147; Section 5). > 6. When the client offers the "pre_shared_key" extension in > ClientHelloInner, it SHOULD also include a GREASE > "pre_shared_key" extension in ClientHelloOuter, generated in the > manner described in Section 6.1.2. The client MUST NOT use this > extension to advertise a PSK to the client-facing server. (See > Section 10.12.3.) When the client includes a GREASE > "pre_shared_key" extension, it MUST also copy the > "psk_key_exchange_modes" from the ClientHelloInner into the > ClientHelloOuter. I think this MUST is required for the same reason as 1, but won't cause an interop problem if you don't. > 7. When the client offers the "early_data" extension in > ClientHelloInner, it MUST also include the "early_data" extension > in ClientHelloOuter. This allows servers that reject ECH and use > ClientHelloOuter to safely ignore any early data sent by the > client per [RFC8446], Section 4.2.10. I think this is required for interop. In any case, I think we should remove the waffle text, as updated RFCs can change anything. This is a question for the WG, however. I have filed https://github.com/tlswg/draft-ietf-tls-esni/issues/651 for the WG to consider. > === > The following are editorial comments, please consider for the next revision: > > 1) The appendix contains a normative requirement which seems odd to position > this requirement in an appendix: “Any future information or hints that > influence ClientHelloOuter SHOULD be specified as ECHConfig extensions. “ Agreed. I moved this up to the extensions text. > 2) It would have been nice for me if the acronym KEM was expanded on first use. Fixed. > 3) I would appreciate a forward reference for the first mention of “GREASE ECH” > in the text of 6.1 that says “and sends GREASE ECH otherwise.” referencing the > text of section 6.3 - I did guess!!! Fixed. > 4) I would appreciate a single sentence or so simply explaining what GREASE ECH > was seeking to achieve in the currently empty text of section 6.2. (A note > saying “see also section 10.10.4” would be super helpful) Added soem text. > 5) Please clarify address: “Clients can implement a new transport connection in > a way that best > suits their deployment. For example, clients can reuse the same IP > address when establishing the new transport connection or they can > choose to use a different IP address if provided with options from > DNS. “ > - I think this means destination IP address (because of the mention of DNS), > but maybe not, because a client can also use alternative valid source > addresses. I would appreciate just a little more clarity - perhaps even just > something like: “Clients can reuse the same IP addresses when establishing the > new transport connection or they can choose to use a different pair of IP > addresses (e.g., if provided with options from DNS).” - if that was what was > intended? It's server IP. The client's IP is generally not withing the client's control. I added some text. > 6) To me it is really odd to require a server to be careful!!! > See: “The server MUST be careful not to unnecessarily reject connections if the > same ECHConfig id or keypair is used in multiple ECHConfigs with distinct > public names.” - This seems to me to require some rewording to communicate the > requirement and I assume the care is needed when configuring the server, and > the server MUST NOT unnecessarily reject connections … Rewritten. > 7) See: “The design of ECH as specified in this document necessarily requires > changes to client, client-facing server, and backend server.” - would it read > better with the “the” or “a” before client? Added "the". > 8) See: “unencrypted.This means differences” - missing space before period. Fixed. > 9) See: “Notes: Any notes associated with the entry” missing final period. Fixed, along with another period in the previous entry. > 10) Please check use of “which” below and update as needed: One rule I’ve heard > is that uf the details are crucial to the sentence, use that. If they aren’t > crucial, use which: “Section 11.3 describes a set of Reserved extensions which > will never be registered.” /which/that/? “Some use cases which depend on > information ECH encrypts may break > with the deployment of ECH.” - /which/that/ ? > “Depending on implementation details and deployment settings, use > cases which depend on “ - /which/that/ ? > “Values which are independent of the true server name, or other > information the client wishes to protect, MAY be included in > ClientHelloOuter.” - /which/that/ ? > “Values which depend on the contents of ClientHelloInner, such as the > true server name, can influence how client-facing servers process > this message.” - /which/that/ ? > “A malicious attacker may craft a packet which takes excessive resources > to decompress” - /which/that/ ? > “These requirements prevent an attacker from performing a packet > amplification attack, by crafting a ClientHelloOuter which > decompresses to a much larger ClientHelloInner.” - /which/that/ ? This is one of those folklore rules of English like split infinitives that is not actually reflective of actual writing. See http://itre.cis.upenn.edu/~myl/languagelog/archives/001484.html . I think these are all sufficiently clear whichever word is used, but if you disagree on one, then please flag it and I'll take a look. -Ekr On Fri, Apr 25, 2025 at 11:23 PM Gorry Fairhurst via Datatracker < nore...@ietf.org> wrote: > Gorry Fairhurst has entered the following ballot position for > draft-ietf-tls-esni-24: No Objection > > When responding, please keep the subject line intact and reply to all > email addresses included in the To and CC lines. (Feel free to cut this > introductory paragraph, however.) > > > Please refer to > https://www.ietf.org/about/groups/iesg/statements/handling-ballot-positions/ > for more information about how to handle DISCUSS and COMMENT positions. > > > The document, along with other ballot positions, can be found here: > https://datatracker.ietf.org/doc/draft-ietf-tls-esni/ > > > > ---------------------------------------------------------------------- > COMMENT: > ---------------------------------------------------------------------- > > Thank you for preparing this I-D. Thanks also for the TSV-ART review > provided > by Tommy Pauly. I agree with that review conclusion: this protocol > extension is > defined to work with various transport cases (TLS over TCP, DTLS over UDP, > QUIC, etc), and otherwise does not fundamentally change their properties. > > The following comments are not blocking, but I would really appreciate > feedback > to help understand what is intended: > > 1) In section 6.1 what is required for interop? On Offering ECH I see a > list of > seven requirements and I thought I saw at least some of these 7 > requirements > were necessary for correct operation of the remaining algorithm. (e.g., > For me, > Rule 1 suggests a need for TLS 1.3 or higher and if not supplied, a later > rule > says this results in a server reject for ECH, etc). However, I found this > sentence after the list: “Note that these rules may change in the presence > of > an application profile specifying otherwise.” - I am trying to understand > if > any of these requirements are actually required for correct operation: Does > that additional sentence simply mean these are a default and all these > requirements can be changed ? or something else? > > 2) A similar phrase follows the seven requirements in section 6.2. What is > intended to happen here? > > === > The following are editorial comments, please consider for the next > revision: > > 1) The appendix contains a normative requirement which seems odd to > position > this requirement in an appendix: “Any future information or hints that > influence ClientHelloOuter SHOULD be specified as ECHConfig extensions. “ > > 2) It would have been nice for me if the acronym KEM was expanded on first > use. > > 3) I would appreciate a forward reference for the first mention of “GREASE > ECH” > in the text of 6.1 that says “and sends GREASE ECH otherwise.” > referencing the > text of section 6.3 - I did guess!!! > > 4) I would appreciate a single sentence or so simply explaining what > GREASE ECH > was seeking to achieve in the currently empty text of section 6.2. (A note > saying “see also section 10.10.4” would be super helpful) > > 5) Please clarify address: “Clients can implement a new transport > connection in > a way that best > suits their deployment. For example, clients can reuse the same IP > address when establishing the new transport connection or they can > choose to use a different IP address if provided with options from > DNS. “ > - I think this means destination IP address (because of the mention of > DNS), > but maybe not, because a client can also use alternative valid source > addresses. I would appreciate just a little more clarity - perhaps even > just > something like: “Clients can reuse the same IP addresses when establishing > the > new transport connection or they can choose to use a different pair of IP > addresses (e.g., if provided with options from DNS).” - if that was what > was > intended? > > 6) To me it is really odd to require a server to be careful!!! > See: “The server MUST be careful not to unnecessarily reject connections > if the > same ECHConfig id or keypair is used in multiple ECHConfigs with distinct > public names.” - This seems to me to require some rewording to communicate > the > requirement and I assume the care is needed when configuring the server, > and > the server MUST NOT unnecessarily reject connections … > > 7) See: “The design of ECH as specified in this document necessarily > requires > changes to client, client-facing server, and backend server.” - would it > read > better with the “the” or “a” before client? > > 8) See: “unencrypted.This means differences” - missing space before period. > > 9) See: “Notes: Any notes associated with the entry” missing final > period. > > 10) Please check use of “which” below and update as needed: One rule I’ve > heard > is that uf the details are crucial to the sentence, use that. If they > aren’t > crucial, use which: “Section 11.3 describes a set of Reserved extensions > which > will never be registered.” /which/that/? “Some use cases which depend on > information ECH encrypts may break > with the deployment of ECH.” - /which/that/ ? > “Depending on implementation details and deployment settings, use > cases which depend on “ - /which/that/ ? > “Values which are independent of the true server name, or other > information the client wishes to protect, MAY be included in > ClientHelloOuter.” - /which/that/ ? > “Values which depend on the contents of ClientHelloInner, such as the > true server name, can influence how client-facing servers process > this message.” - /which/that/ ? > “A malicious attacker may craft a packet which takes excessive resources > to decompress” - /which/that/ ? > “These requirements prevent an attacker from performing a packet > amplification attack, by crafting a ClientHelloOuter which > decompresses to a much larger ClientHelloInner.” - /which/that/ ? > > Thanks again for this detailed and useful specification. > Gorry > > > >
_______________________________________________ TLS mailing list -- tls@ietf.org To unsubscribe send an email to tls-le...@ietf.org
