Paul Thanks you for your review. I have created a PR that addresses a number of these.
https://github.com/tlswg/draft-ietf-tls-esni/pull/632 Detailed responses below: > Section 1 > > that allows clients to encrypt their ClientHello to such a deployment. > > What is "such a deployment" ? Fixed. > DoH [RFC8484] and DPRIVE [RFC7858] [RFC8094] > > DPRIVE is a working group, not a protocol. Why not say DoH and DoT ? Expanded all of these inline. > Section 3.2 > > The server must publish this for all the domains it serves via Shared or Split Mode. > > This is confusing. "The server" might be misunderstood to be the TLS server itself. I think > what is meant is "domains that wish to use the client-facing server must publish the appropriate > public key and meta information somewhere/somehow, for example via DNS" The relevant point is that the server operator must do so, so I have changed it accordingly. > Section 4 > > Clients MUST ignore any ECHConfig structure whose public_name > is not parsable as a dot-separated sequence of LDH labels, as > defined in [RFC5890], Section 2.3.1 or which begins or end with > an ASCII dot. > > Wouldn't this prevent the root zone from ever running ECH? It might not matter, > but why not allow it :P > > Clients additionally SHOULD ignore the structure if the final LDH > label either consists of all ASCII digits (i.e. '0' through '9') > or is "0x" or "0X" followed by some, possibly empty, sequence > of ASCII hexadecimal digits (i.e. '0' through '9', 'a' through > 'f', and 'A' through 'F'). > > This would not allow ECH to work for "00.com", "0x01.com" or "0x07.com", which are > all currently operational domains. > > I don't think this document should try to define what a legal DNS name is. Perhaps > refer to "Host name" in RFC 8499 (or BCP219) and only state "leave out any potential > trailing dot" ? Maybe just separately say "SNI's cannot be in the textual form of an > IP address, as connections to IP addresses omit the SNI entirely" ? > > It is a bit sad that ECH currently does not work with a URI that points to an IP > address. Especially for the split-mode. But I understand that would be a little > more than just ECH/encrypted-SNI. > > > Additionally, clients MAY ignore the ECHConfig if the length of > any label in the DNS name is longer than 63 octets, as this is > the maximum length of a DNS label. > > Why is this a MAY? Why not a MUST or SHOULD? I generally agree with these points, but others were primarily responsible for the DNS section, so I have filed issue 628: https://github.com/tlswg/draft-ietf-tls-esni/issues/628 > Section 4.2 > > See Appendix A for guidance on which types of extensions are appropriate for this structure. > > If this is guidance, then it probably does not belong in an appendix, but in its own real Section ? > Also, since Appendix A is a single paragraph, why not just fold it in right here? I think this is a taste issue, so I'm comfortable leaving it where it is, but if you insist, I will change it. > Section 6.1.6 > > If an earlier TLS version was negotiated, the client MUST NOT enable the False Start optimization > > How is this possible? If you sent and received ECH, then you cannot have negotiated an "earlier" TLS version? > At least not until we have TLS 1.4. And if we have that, wouldn't an earlier version 1.3 be fine for ECH? This section is about where you are handshaking with ClientHelloOuter, in which case it is possible that you are not doing ECH, in which case False Start would be possible. > Section 10 > > Downgrade resistance - makes me think of the tls-dnssec pin discussion :-) > > If the HPKE private key is lost and replaced, and the ECH DNS entries are being > updated, there is a time period (depending on TTL and cache) where the tls client > connecting to the client-facing TLS server will not be able to distinguish the > connection from an attack, and thus not downgrade to try without ECH. This is an > outage. Unfortunately, DoH has no way of asking a "refreshed DNS record" to get > its DoH server to drop the cached record. I guess the SVCB/HTTPS record could > introduce a syntax with a prefix on the old keyid or something with an explicit > signal that points to the updated (uncached because it only exists when such an > outage appears). But this might be a bit complicated for a failure case. It is probably > best to postpone such work until it shows to be needed in practice. I don't think I follow the scenario you have in mind here. Suppose that the server was using key A and publishes an appropriate record. It then loses the key and starts using B. If a client comes in using key A, the server is supposed to follow the ECH configuration correction procedure in S 6.1.6 and provide a retry configuration with key B. The client will then retry with B. This isn't an outage, though it has a negative performance impact, which is why it's best to have keys overlap. > Section 10.2 > > Another instance of "DoH/DPRIVE" instead of "DoH/DoT" Rewrote the text. > Section 10.5 > > It MAY send such values in the ClientHelloInner. > > Since it is talking about "such values", this should be "It SHOULD" The reason for "MAY" here is we aren't telling it to send the values at all. We've already told them not to send them in the outer, so Inner is all that's permitted. I'm not sure how a SHOULD helps. > Section 10.10.2 > > I do not understand this section. I think it is trying to say "dont share > the secrets with too many operators", and not "don't share this secret > with many servers/domain under the same administrative control" ? But > it reads like it is recommending that an operator uses multiple smaller > ECH groups preferably over less but bigger groups? I am also struggling with this text. Maybe some other author can help explain what this is trying to say? > Section 10.10.3 > > 10.10.3. Prevent SNI-Based Denial-of-Service Attacks > > The entire section does not say how to prevent attacks ? Maybe drop the > word "Prevent" from the heading? Done. > Section 10.10.5 > > It is RECOMMENDED that servers rotate keys frequently > > What is frequently? hourly? daily? weekly? annually ? Good question. I have filed the following issue. https://github.com/tlswg/draft-ietf-tls-esni/issues/629 > Section 11.3: > > These extraneous ECH configurations SHOULD have invalid keys, > and public names which the server does not respond to. > > Why not MUST ? I have filed an issue: https://github.com/tlswg/draft-ietf-tls-esni/issues/630 > How does a client know which public names the server does not respond to? The client doesn't know. It ignores these configurations > > Should it use known-invalid DNS names, eg "invalid:com", or some randomized > long valid but unlikely DNS name? Guidaance would be useful. > > Also, the ensure paragraph has nothing to do with IANA and should be moved > to another location in the document. Filed an issue so I don't forget https://github.com/tlswg/draft-ietf-tls-esni/issues/631 > Should the Notes: part of this registration mention these as "grease entries" ? Done. > NITS: > > Section 1 > > there are several ways in which an on-path attacker can learn private information about the connection > > Passive or active or both? All of these ways are passive, but on-path is in our threat model, so I would prefer to leave it as-is. > > Section 10.5 > > a different ALPN lists -> a different ALPN list Fixed by removing "a". > Section 10.10.4 > > Thus, our strategy for mitigating network ossification > > Use "the" instead of "our". It is unclear who "our" would be in this context. Fixed. > > Section 10.10.5 > > This design is not forward secret > > Maybe say "is not forward secure" or "has no forward secrecy" ? Changed to "does not provide forward secrecy" > > Section 10.10.6 > > The client authenticates the identity of the backend origin > server, thereby avoiding unnecessary MiTM attacks. > > Aren't MITM attacks always "unnecessary" ? Also wouldn't it be > "avoiding necessary" ? > > I think what is meant is "avoiding decryption / re-encryption in the path, > avoiding the need to downgrade the TLS security with trusted third parties" I rewrote this a bit. > Section 11.3 > > The registration policy for for > > Remove duplicate "for". I am failing at finding this. Can you send the whole paragraph. -Ekr On Fri, Oct 11, 2024 at 4:32 PM Paul Wouters <paul.wout...@aiven.io> wrote: > > AD review draft-ietf-tls-esni-22 > > Thanks for this document. It is a very interesting technology and I want > to thank everyone who worked on this. As expected, I found no major issues > in it :-) I do have a few minor questions and nits below: > > > Section 1 > > that allows clients to encrypt their ClientHello to such a > deployment. > > What is "such a deployment" ? > > DoH [RFC8484] and DPRIVE [RFC7858] [RFC8094] > > DPRIVE is a working group, not a protocol. Why not say DoH and DoT ? > > Section 3.2 > > The server must publish this for all the domains it serves via > Shared or Split Mode. > > This is confusing. "The server" might be misunderstood to be the TLS > server itself. I think > what is meant is "domains that wish to use the client-facing server must > publish the appropriate > public key and meta information somewhere/somehow, for example via DNS" > > Section 4 > > Clients MUST ignore any ECHConfig structure whose public_name > is not parsable as a dot-separated sequence of LDH labels, as > defined in [RFC5890], Section 2.3.1 or which begins or end with > an ASCII dot. > > Wouldn't this prevent the root zone from ever running ECH? It might not > matter, > but why not allow it :P > > Clients additionally SHOULD ignore the structure if the final LDH > label either consists of all ASCII digits (i.e. '0' through '9') > or is "0x" or "0X" followed by some, possibly empty, sequence > of ASCII hexadecimal digits (i.e. '0' through '9', 'a' through > 'f', and 'A' through 'F'). > > This would not allow ECH to work for "00.com", "0x01.com" or "0x07.com", > which are > all currently operational domains. > > I don't think this document should try to define what a legal DNS name is. > Perhaps > refer to "Host name" in RFC 8499 (or BCP219) and only state "leave out any > potential > trailing dot" ? Maybe just separately say "SNI's cannot be in the textual > form of an > IP address, as connections to IP addresses omit the SNI entirely" ? > > It is a bit sad that ECH currently does not work with a URI that points to > an IP > address. Especially for the split-mode. But I understand that would be a > little > more than just ECH/encrypted-SNI. > > > Additionally, clients MAY ignore the ECHConfig if the length of > any label in the DNS name is longer than 63 octets, as this is > the maximum length of a DNS label. > > Why is this a MAY? Why not a MUST or SHOULD? > > Section 4.2 > > See Appendix A for guidance on which types of extensions are > appropriate for this structure. > > If this is guidance, then it probably does not belong in an appendix, but > in its own real Section ? > Also, since Appendix A is a single paragraph, why not just fold it in > right here? > > > Section 6.1.6 > > If an earlier TLS version was negotiated, the client MUST NOT > enable the False Start optimization > > How is this possible? If you sent and received ECH, then you cannot have > negotiated an "earlier" TLS version? > At least not until we have TLS 1.4. And if we have that, wouldn't an > earlier version 1.3 be fine for ECH? > > > Section 10 > > Downgrade resistance - makes me think of the tls-dnssec pin discussion :-) > > If the HPKE private key is lost and replaced, and the ECH DNS entries are > being > updated, there is a time period (depending on TTL and cache) where the tls > client > connecting to the client-facing TLS server will not be able to distinguish > the > connection from an attack, and thus not downgrade to try without ECH. This > is an > outage. Unfortunately, DoH has no way of asking a "refreshed DNS record" > to get > its DoH server to drop the cached record. I guess the SVCB/HTTPS record > could > introduce a syntax with a prefix on the old keyid or something with an > explicit > signal that points to the updated (uncached because it only exists when > such an > outage appears). But this might be a bit complicated for a failure case. > It is probably > best to postpone such work until it shows to be needed in practice. > > Section 10.2 > > Another instance of "DoH/DPRIVE" instead of "DoH/DoT" > > Section 10.5 > > It MAY send such values in the ClientHelloInner. > > Since it is talking about "such values", this should be "It SHOULD" > > Section 10.10.2 > > I do not understand this section. I think it is trying to say "dont share > the secrets with too many operators", and not "don't share this secret > with many servers/domain under the same administrative control" ? But > it reads like it is recommending that an operator uses multiple smaller > ECH groups preferably over less but bigger groups? > > Section 10.10.3 > > 10.10.3. Prevent SNI-Based Denial-of-Service Attacks > > The entire section does not say how to prevent attacks ? Maybe drop the > word "Prevent" from the heading? > > Section 10.10.5 > > It is RECOMMENDED that servers rotate keys frequently > > What is frequently? hourly? daily? weekly? annually ? > > Section 11.3: > > These extraneous ECH configurations SHOULD have invalid keys, > and public names which the server does not respond to. > > Why not MUST ? > > How does a client know which public names the server does not respond to? > > Should it use known-invalid DNS names, eg "invalid:com", or some randomized > long valid but unlikely DNS name? Guidaance would be useful. > > Also, the ensure paragraph has nothing to do with IANA and should be moved > to another location in the document. > > Should the Notes: part of this registration mention these as "grease > entries" ? > > > > NITS: > > Section 1 > > there are several ways in which an on-path attacker can learn > private information about the connection > > Passive or active or both? > > Section 10.5 > > a different ALPN lists -> a different ALPN list > > > Section 10.10.4 > > Thus, our strategy for mitigating network ossification > > Use "the" instead of "our". It is unclear who "our" would be in this > context. > > Section 10.10.5 > > This design is not forward secret > > Maybe say "is not forward secure" or "has no forward secrecy" ? > > Section 10.10.6 > > The client authenticates the identity of the backend origin > server, thereby avoiding unnecessary MiTM attacks. > > Aren't MITM attacks always "unnecessary" ? Also wouldn't it be > "avoiding necessary" ? > > I think what is meant is "avoiding decryption / re-encryption in the path, > avoiding the need to downgrade the TLS security with trusted third parties" > > Section 11.3 > > The registration policy for for > > Remove duplicate "for". > > > Paul > >
_______________________________________________ TLS mailing list -- tls@ietf.org To unsubscribe send an email to tls-le...@ietf.org
