Thank you for your close review.
I have made a PR in response to these comments:
PR https://github.com/tlswg/draft-ietf-tls-esni/pull/648
Detailed responses below.
## Editorial
> 10.1 »Security and Privacy Goals« starts with definitions
> (active/passive) that aren’t really Security/Privacy goals.
> Since “passive” is then used distinguishingly in only three places, it
> is not clear that this passage is particularly useful.
> It is also puzzling to sort filtering middleboxes under “passive”.
They are passive from the perspective of ECH's threat model;
this is part of why I think this paragraph is useful.
> 10.1: I cannot find a discussion of a “threat model” under this name
> in RFC 8446.
This is a citation for TLS 1.3. I have added a citation for
3552 behind threat model.
> 10.1: This section starts using the term “host” as a noun with a
> distinguishing quality that in this document is probably related to
> server_name, but not explained (“host” as a noun does not occur in RFC
> 8446).
I changed the term "host" to "server name".
>
> 10.2 appears to address integrity only, where the heading might
> suggest some discussion about confidentiality.
The final sentence addresses confidentiality or the lack thereof.
> I don’t understand the last sentence of 10.3 (“servers with high
> load”…).
I rewrote this a bit.
> 11.3: After »A "Y" or "N" value indicating if the extension is TLS WG recommends
> that the extension be supported.« — can’t parse »Adding a value with a
> value of "Y" requires Standards Action .« -- s/value/registration/ ?
Fixed.
> 10.11: I would have expected a brief discussion on how granularizing
> the padding to 32 byte steps cannot be used by an attacker to extract
> information beyond that granularity (by causing the client to vary the
> size before padding, straddling a step).
The attacker doesn't control CH at all, so I'm not sure why this
would be needed.
> ## Nits
>
> Please do a pass of replacing “which” with “that” where a restrictive
> clause is intended (usually easy to find by the lack of a comma, about
> two dozen occurrences)
This is one of those folklore grammar rules that does not
match actual English usage, like not splitting infinitives.
http://itre.cis.upenn.edu/~myl/languagelog/archives/001484.html
> Thank you for the many section references; two more could be used in
> »<p id="section-10.8-3">Note that, if the cookie includes a key name,
> analogous to Section 4 of« and »<p id="section-6.1-7.3.1”>[…] (see
> Appendix D.4 of <span>[<a href="" class="cite
> xref">RFC8446</a>]</span>) when ECH is negotiated.«
Done.
> There are at least 17 occurrences of ‘bytes’ for counting lengths in
> bytes and 3 of ‘octet’/‘octets’.
I changed these all to "bytes".
> Please expand HRR on first use.
Done.
> Please define “ECH key” (8 occurrences) and “ECH record” (5
> occurrences).
I am replacing ECH record with HTTPS record, which is already
defined. Defined ECH key.
> Please define “application profile” and “application profile standard”
> (10.4 uses “application using (D)TLS” — is that the same?)
I don't believe this is a necessary change. TLS 1.3 already uses
this term without definition in RFC 8446 S 9.1.
> 1: »the SNI remains the primary explicit signal used to determine the
> server's identity.« -- used by whom?
By observers. Addresed.
> Figure 2 uses private.example.com in the second variant where
> private.example.org is used in the first; may want to use .org here as well
Fixed.
> 5: it is slightly surprising that the definition list explaining
> config_id and cipher_suite does this in an order different from the
> TPL in the figure above
I take your point, but I think this is reflects the difference
between code (avoiding forward references) and text (top down).
> 5.1: The first figure has a cliff-hanger (length_of_padding); maybe
> add a reference to 6.1.3.
Done.
> 5.2: »This value does not include Handshake structure's four-byte
> header in TLS,«
Removed the comma.
> 6.1.1: »Including ClientHelloOuterAAD as the HPKE AAD binds the
> ClientHelloOuter to the ClientHelloInner, this preventing attackers«
> s/this/thus/ ?
Fixed.
> 6.1.3: »through -their length«
Fixed.
> 6.1.3: »that have sensitive-length fields« (hyphen ➔ space)
This appears to be some kind of rendering error. It's fine in
the markdown. I'll trust the RPC to fix it.
> 6.1.6 »yield a tracking vector« — for whom?
> ➔ supply the server with a tracking vector?
I think this is OK as-is.
> 6.1.7: »reference identity« -- define?
Added a reference to RFC 6125.
> 6.1.7: »(e.g. [RFC3986], Section 7.4 and [WHATWG-IPV4])«
> The "and" does not work here
Fixed.
> 6.2.2: »Correctly-implemented client will ignore those extensions.«
Fixed.
> 10.9: could use superscript in place of 2^64
> (This of course only applies to usage in text, which seems to be the
> only one here.)
Markdown again. I'll let the RPC address.
> 10.10.4: »out-of-scope. including, «
Fixed.
> I assume “page” in 11.3 is what is now called “registry group”?
Yes. Fixed.
I have made a PR in response to these comments:
PR https://github.com/tlswg/draft-ietf-tls-esni/pull/648
Detailed responses below.
## Editorial
> 10.1 »Security and Privacy Goals« starts with definitions
> (active/passive) that aren’t really Security/Privacy goals.
> Since “passive” is then used distinguishingly in only three places, it
> is not clear that this passage is particularly useful.
> It is also puzzling to sort filtering middleboxes under “passive”.
They are passive from the perspective of ECH's threat model;
this is part of why I think this paragraph is useful.
> 10.1: I cannot find a discussion of a “threat model” under this name
> in RFC 8446.
This is a citation for TLS 1.3. I have added a citation for
3552 behind threat model.
> 10.1: This section starts using the term “host” as a noun with a
> distinguishing quality that in this document is probably related to
> server_name, but not explained (“host” as a noun does not occur in RFC
> 8446).
I changed the term "host" to "server name".
>
> 10.2 appears to address integrity only, where the heading might
> suggest some discussion about confidentiality.
The final sentence addresses confidentiality or the lack thereof.
> I don’t understand the last sentence of 10.3 (“servers with high
> load”…).
I rewrote this a bit.
> 11.3: After »A "Y" or "N" value indicating if the extension is TLS WG recommends
> that the extension be supported.« — can’t parse »Adding a value with a
> value of "Y" requires Standards Action .« -- s/value/registration/ ?
Fixed.
> 10.11: I would have expected a brief discussion on how granularizing
> the padding to 32 byte steps cannot be used by an attacker to extract
> information beyond that granularity (by causing the client to vary the
> size before padding, straddling a step).
The attacker doesn't control CH at all, so I'm not sure why this
would be needed.
> ## Nits
>
> Please do a pass of replacing “which” with “that” where a restrictive
> clause is intended (usually easy to find by the lack of a comma, about
> two dozen occurrences)
This is one of those folklore grammar rules that does not
match actual English usage, like not splitting infinitives.
http://itre.cis.upenn.edu/~myl/languagelog/archives/001484.html
> Thank you for the many section references; two more could be used in
> »<p id="section-10.8-3">Note that, if the cookie includes a key name,
> analogous to Section 4 of« and »<p id="section-6.1-7.3.1”>[…] (see
> Appendix D.4 of <span>[<a href="" class="cite
> xref">RFC8446</a>]</span>) when ECH is negotiated.«
Done.
> There are at least 17 occurrences of ‘bytes’ for counting lengths in
> bytes and 3 of ‘octet’/‘octets’.
I changed these all to "bytes".
> Please expand HRR on first use.
Done.
> Please define “ECH key” (8 occurrences) and “ECH record” (5
> occurrences).
I am replacing ECH record with HTTPS record, which is already
defined. Defined ECH key.
> Please define “application profile” and “application profile standard”
> (10.4 uses “application using (D)TLS” — is that the same?)
I don't believe this is a necessary change. TLS 1.3 already uses
this term without definition in RFC 8446 S 9.1.
> 1: »the SNI remains the primary explicit signal used to determine the
> server's identity.« -- used by whom?
By observers. Addresed.
> Figure 2 uses private.example.com in the second variant where
> private.example.org is used in the first; may want to use .org here as well
Fixed.
> 5: it is slightly surprising that the definition list explaining
> config_id and cipher_suite does this in an order different from the
> TPL in the figure above
I take your point, but I think this is reflects the difference
between code (avoiding forward references) and text (top down).
> 5.1: The first figure has a cliff-hanger (length_of_padding); maybe
> add a reference to 6.1.3.
Done.
> 5.2: »This value does not include Handshake structure's four-byte
> header in TLS,«
Removed the comma.
> 6.1.1: »Including ClientHelloOuterAAD as the HPKE AAD binds the
> ClientHelloOuter to the ClientHelloInner, this preventing attackers«
> s/this/thus/ ?
Fixed.
> 6.1.3: »through -their length«
Fixed.
> 6.1.3: »that have sensitive-length fields« (hyphen ➔ space)
This appears to be some kind of rendering error. It's fine in
the markdown. I'll trust the RPC to fix it.
> 6.1.6 »yield a tracking vector« — for whom?
> ➔ supply the server with a tracking vector?
I think this is OK as-is.
> 6.1.7: »reference identity« -- define?
Added a reference to RFC 6125.
> 6.1.7: »(e.g. [RFC3986], Section 7.4 and [WHATWG-IPV4])«
> The "and" does not work here
Fixed.
> 6.2.2: »Correctly-implemented client will ignore those extensions.«
Fixed.
> 10.9: could use superscript in place of 2^64
> (This of course only applies to usage in text, which seems to be the
> only one here.)
Markdown again. I'll let the RPC address.
> 10.10.4: »out-of-scope. including, «
Fixed.
> I assume “page” in 11.3 is what is now called “registry group”?
Yes. Fixed.
On Fri, Mar 14, 2025 at 5:35 AM Carsten Bormann via Datatracker <noreply@xxxxxxxx> wrote:
Reviewer: Carsten Bormann
Review result: Ready with Nits
(Insert ARTART boilerplate here.)
Thank you for this draft, it is in very good shape.
The document is explicit about the different configurations the
protocol can be run in, the participants, their roles, the security
and privacy objectives that can be achieved as well as the security
considerations, and it discusses (and addresses!) deployment
considerations.
This specification is ready with nits.
A few editorial observations and a few nits below.
## Editorial
10.1 »Security and Privacy Goals« starts with definitions
(active/passive) that aren’t really Security/Privacy goals.
Since “passive” is then used distinguishingly in only three places, it
is not clear that this passage is particularly useful.
It is also puzzling to sort filtering middleboxes under “passive”.
10.1: I cannot find a discussion of a “threat model” under this name
in RFC 8446.
10.1: This section starts using the term “host” as a noun with a
distinguishing quality that in this document is probably related to
server_name, but not explained (“host” as a noun does not occur in RFC
8446).
10.2 appears to address integrity only, where the heading might
suggest some discussion about confidentiality.
I don’t understand the last sentence of 10.3 (“servers with high
load”…).
11.3: After »A "Y" or "N" value indicating if the extension is TLS WG recommends
that the extension be supported.« — can’t parse »Adding a value with a
value of "Y" requires Standards Action .« -- s/value/registration/ ?
10.11: I would have expected a brief discussion on how granularizing
the padding to 32 byte steps cannot be used by an attacker to extract
information beyond that granularity (by causing the client to vary the
size before padding, straddling a step).
## Nits
Please do a pass of replacing “which” with “that” where a restrictive
clause is intended (usually easy to find by the lack of a comma, about
two dozen occurrences)
Thank you for the many section references; two more could be used in
»<p id="section-10.8-3">Note that, if the cookie includes a key name,
analogous to Section 4 of« and »<p id="section-6.1-7.3.1”>[…] (see
Appendix D.4 of <span>[<a href="" class="cite
xref">RFC8446</a>]</span>) when ECH is negotiated.«
There are at least 17 occurrences of ‘bytes’ for counting lengths in
bytes and 3 of ‘octet’/‘octets’.
Please expand HRR on first use.
Please define “ECH key” (8 occurrences) and “ECH record” (5
occurrences).
Please define “application profile” and “application profile standard”
(10.4 uses “application using (D)TLS” — is that the same?)
1: »the SNI remains the primary explicit signal used to determine the
server's identity.« -- used by whom?
Figure 2 uses private.example.com in the second variant where
private.example.org is used in the first; may want to use .org here as well
5: it is slightly surprising that the definition list explaining
config_id and cipher_suite does this in an order different from the
TPL in the figure above
5.1: The first figure has a cliff-hanger (length_of_padding); maybe
add a reference to 6.1.3.
5.2: »This value does not include Handshake structure's four-byte
header in TLS,«
6.1.1: »Including ClientHelloOuterAAD as the HPKE AAD binds the
ClientHelloOuter to the ClientHelloInner, this preventing attackers«
s/this/thus/ ?
6.1.3: »through -their length«
6.1.3: »that have sensitive-length fields« (hyphen ➔ space)
6.1.6 »yield a tracking vector« — for whom?
➔ supply the server with a tracking vector?
6.1.7: »reference identity« -- define?
6.1.7: »(e.g. [RFC3986], Section 7.4 and [WHATWG-IPV4])«
The "and" does not work here
6.2.2: »Correctly-implemented client will ignore those extensions.«
10.9: could use superscript in place of 2^64
(This of course only applies to usage in text, which seems to be the
only one here.)
10.10.4: »out-of-scope. including, «
I assume “page” in 11.3 is what is now called “registry group”?
-- last-call mailing list -- last-call@xxxxxxxx To unsubscribe send an email to last-call-leave@xxxxxxxx
