Comments from Sergey Agievich review #1396

ekr · 2025-09-11T18:19:35Z

No description provided.

ekr · 2025-09-11T18:22:54Z

paulwouters · 2025-09-11T18:44:45Z

draft-ietf-tls-rfc8446bis.md

@@ -3479,7 +3479,7 @@ ticket_lifetime:
  network byte order from the time of ticket issuance.
  Servers MUST NOT use any value greater than 604800 seconds (7 days).
  The value of zero indicates that the ticket should be discarded
-  immediately. Clients MUST NOT use tickets for longer than
+  immediately fater use. Clients MUST NOT use tickets for longer than


kaduk · 2025-09-11T19:15:28Z

draft-ietf-tls-rfc8446bis.md

@@ -3479,7 +3479,7 @@ ticket_lifetime:
  network byte order from the time of ticket issuance.
  Servers MUST NOT use any value greater than 604800 seconds (7 days).
  The value of zero indicates that the ticket should be discarded
-  immediately. Clients MUST NOT use tickets for longer than
+  immediately fater use. Clients MUST NOT use tickets for longer than


Suggested change

immediately fater use. Clients MUST NOT use tickets for longer than

immediately after use. Clients MUST NOT use tickets for longer than

fixes the spelling, but I don't actually think this is a good change to take -- it seems to change the meaning, suggesting that a ticket received with lifetime of 0 can be held onto for some (unspecified) period of time and then used once, which is rather different from what the semantics of a non-zero-lifetime ticket would be.

I have to admit I was trying to remember what we originally intended here. But I don't understand what it means to have a 0 lifetime otherwise. Do you think the implications are "this ticket actually cannot be used"?

I have a hazy recollection of something like "might not be usable but you could try it if you open a new connection right away". Other theories would relate to if the server, whether by protocol or implementation, was locked into a codepath of issuing a ticket but later decided it did not want to do so, or if it somehow helped with middlebox compatibility or something similar. But I don't see anything to suggest that either of those theories actually hold. I suspect that trawling the git history and email history would yield something, but did not attempt that (yet?).

OK. Let me pull this out and I'll take it to the WG list for resolution.

I found #424 which should be a good place to start looking.

Yeah, there are two things bound up in this one:

The time between when the ticket is issued and when it can be used. Or maybe between when the original connection ends and when the tickets issued on it can be used. My sense is that this is solidly ambiguous in that regard. My sense is that there is a floor on the lifetime of tickets that might allow them to be used some time after a reference point, but that time is pretty short. I don't know if the reference point is the issuance/receipt time or whether that time can extend while the connection remains active.

Whether the ticket is single use. That's a matter that we've resolved already: tickets should only be used once, otherwise they allow an observer to correlate activity, even where other signals do not. So this is probably not relevant here.

Adding "after use" is misleading for sure. We already say that about tickets, though with a "SHOULD" (not a "should"). I don't know about the other thing.

kaduk · 2025-09-11T19:16:29Z

draft-ietf-tls-rfc8446bis.md

@@ -1651,7 +1651,7 @@ a distinct message.

 The server's extensions MUST contain "supported_versions".
 Additionally, it SHOULD contain the minimal set of extensions necessary for the
-client to generate a correct ClientHello pair. As with the ServerHello, a
+client to generate a correct ClientHello pair. The ServerHello, a


This seems backward, a HelloRetryRequest is a ServerHello but not the other way around.

I actually meant to delete this entire clause, so it should just read "A HRR must not..."

kaduk · 2025-09-11T19:18:07Z

draft-ietf-tls-rfc8446bis.md

@@ -1900,7 +1900,7 @@ Servers MUST be prepared to receive ClientHellos that include this
 extension but do not include 0x0304 in the list of versions.

 A server which negotiates a version of TLS prior to TLS 1.3 MUST
-set ServerHello.version and MUST NOT send the "supported_versions"
+set ServerHello.legacy_version and MUST NOT send the "supported_versions"


I actually remember thinking about proposing this change prior to the publication of RFC 8446. My reasoning at the time was that once you've negotiated an older version of TLS you're using that version, so ServerHello.version is the proper description of that (no-longer-TLS 1.3) protocol field. Without a reference to a corresponding reference for TLS 1.2 or similar, it's a bit confusing, I admit. But I'd be more inclined to reference RFC 5246 and keep using ServerHello.version than to take this change.

Argh. You're right, this change is a mistake. I'll revert prior to landing

kaduk · 2025-09-11T19:21:31Z

draft-ietf-tls-rfc8446bis.md

@@ -4111,8 +4112,7 @@ user_canceled:
 {:br }

 Either party MAY initiate a close of its write side of the connection by
-sending a "close_notify" alert. Any data received after a "close_notify" alert has
-been received MUST be ignored. If a transport-level close is received prior
+sending a "close_notify" alert. If a transport-level close is received prior


Why this change? If you receive a close_notify and then more data from the peer, shouldn't you be ignoring that?

It's already stated above.

Ah, didn't read enough context. Thanks.

kaduk · 2025-09-11T19:22:38Z

draft-ietf-tls-rfc8446bis.md

@@ -4476,7 +4476,7 @@ been computed, that secret SHOULD be erased.
 ## Updating Traffic Secrets  {#updating-traffic-keys}

 Once the handshake is complete, it is possible for either side to
-update its sending traffic keys using the KeyUpdate handshake message
+update its sending traffic key using the KeyUpdate handshake message


We expand the secret into a traffic key and a traffic IV; I'd keep this one as plural.

An IV isn't a key.

That's fair. Do you prefer "key" or "key material"?

Suggested change

update its sending traffic key using the KeyUpdate handshake message

update its sending traffic key and IV using the KeyUpdate handshake message

Suggested change

update its sending traffic key using the KeyUpdate handshake message

update its application traffic secret using the KeyUpdate handshake message

I think it is important to be precise about application traffic secret vs. handshake traffic secret. Isn't the intention in this section to talk about the former?

Per Paul's instructions to do the minimal changes I'm going to revert this change. We use "keys" plural all over the place in this section.

ekr

OK so this PR could still use some work.... Thanks for catching some issues @kaduk. Responses below.

ekr · 2025-09-11T23:37:00Z

draft-ietf-tls-rfc8446bis.md

@@ -1651,7 +1651,7 @@ a distinct message.

 The server's extensions MUST contain "supported_versions".
 Additionally, it SHOULD contain the minimal set of extensions necessary for the
-client to generate a correct ClientHello pair. As with the ServerHello, a
+client to generate a correct ClientHello pair. The ServerHello, a


I actually meant to delete this entire clause, so it should just read "A HRR must not..."

ekr · 2025-09-11T23:37:20Z

draft-ietf-tls-rfc8446bis.md

@@ -1900,7 +1900,7 @@ Servers MUST be prepared to receive ClientHellos that include this
 extension but do not include 0x0304 in the list of versions.

 A server which negotiates a version of TLS prior to TLS 1.3 MUST
-set ServerHello.version and MUST NOT send the "supported_versions"
+set ServerHello.legacy_version and MUST NOT send the "supported_versions"


Argh. You're right, this change is a mistake. I'll revert prior to landing

ekr · 2025-09-11T23:37:41Z

draft-ietf-tls-rfc8446bis.md

@@ -4111,8 +4112,7 @@ user_canceled:
 {:br }

 Either party MAY initiate a close of its write side of the connection by
-sending a "close_notify" alert. Any data received after a "close_notify" alert has
-been received MUST be ignored. If a transport-level close is received prior
+sending a "close_notify" alert. If a transport-level close is received prior


It's already stated above.

This reverts commit ffabe24.

martinthomson · 2025-09-12T04:27:59Z

draft-ietf-tls-rfc8446bis.md

@@ -1651,7 +1651,7 @@ a distinct message.

 The server's extensions MUST contain "supported_versions".
 Additionally, it SHOULD contain the minimal set of extensions necessary for the
-client to generate a correct ClientHello pair. As with the ServerHello, a
+client to generate a correct ClientHello pair. A


The original text was harmless, but this is fine.

martinthomson · 2025-09-12T04:32:38Z

draft-ietf-tls-rfc8446bis.md

@@ -3479,7 +3479,7 @@ ticket_lifetime:
  network byte order from the time of ticket issuance.
  Servers MUST NOT use any value greater than 604800 seconds (7 days).
  The value of zero indicates that the ticket should be discarded
-  immediately. Clients MUST NOT use tickets for longer than
+  immediately fater use. Clients MUST NOT use tickets for longer than


Yeah, there are two things bound up in this one:

The time between when the ticket is issued and when it can be used. Or maybe between when the original connection ends and when the tickets issued on it can be used. My sense is that this is solidly ambiguous in that regard. My sense is that there is a floor on the lifetime of tickets that might allow them to be used some time after a reference point, but that time is pretty short. I don't know if the reference point is the issuance/receipt time or whether that time can extend while the connection remains active.

Whether the ticket is single use. That's a matter that we've resolved already: tickets should only be used once, otherwise they allow an observer to correlate activity, even where other signals do not. So this is probably not relevant here.

Adding "after use" is misleading for sure. We already say that about tickets, though with a "SHOULD" (not a "should"). I don't know about the other thing.

martinthomson · 2025-09-12T04:34:29Z

draft-ietf-tls-rfc8446bis.md

@@ -4476,7 +4476,7 @@ been computed, that secret SHOULD be erased.
 ## Updating Traffic Secrets  {#updating-traffic-keys}

 Once the handshake is complete, it is possible for either side to
-update its sending traffic keys using the KeyUpdate handshake message
+update its sending traffic key using the KeyUpdate handshake message


Suggested change

update its sending traffic key using the KeyUpdate handshake message

update its sending traffic key and IV using the KeyUpdate handshake message

agievich · 2025-09-12T10:48:00Z

Again about ServerHello.version vs ServerHello.legacy_version.

Maybe

A server which negotiates a version of TLS prior to TLS 1.3 MUST
set ServerHello.legacy_version (formerly ServerHello.version) and 
MUST NOT send the "supported_versions"

paulwouters · 2025-09-12T13:40:58Z

As these comments are coming in at the last possible moment in the process, please limit changes to the absolute minimum required and avoid "it would be nice" items.

ekr · 2025-09-12T16:07:42Z

Again about ServerHello.version vs ServerHello.legacy_version.

Maybe

A server which negotiates a version of TLS prior to TLS 1.3 MUST
set ServerHello.legacy_version (formerly ServerHello.version) and 
MUST NOT send the "supported_versions"

The point is that in the previous version (which is what is happening here) it is called ".version"

slightly more technically correct to talk about "key" rather than "keys". This reverts commit 31a02ac.

ekr · 2025-09-12T16:20:17Z

OK, after doing the archaeology on this ticket_lifetime issue (thanks, Ben), the original PR from @grittygrease just seems to introduce this "discarded immediately" text with no explanation. I agree that this isn't great, but IMO the conservative thing here is to leave the text as it originally was:

Because the lifetime is from receipt, if you just don't do anything special for 0 you'll mostly get the immediate discard semantics (so arguably this text is redundant).
The "at most once" semantics are redundant, as suggested by MT.
Any other new semantics would clearly be us trying to invent something new at this point.

So I suggest we just leave this text as it is in the draft, which I plan to do unless someone objects ASAP. Sorry for the false alarm.

This reverts commit d3c0cee.

ekr added 7 commits September 11, 2025 11:08

Client -> Server

237f07f

Remove forward reference

ecfd69b

version -> legacy_version

ffabe24

Discarded immediately after use

d3c0cee

Remove duplication

1d61fcc

KLey

31a02ac

Sharpen ordering text

60cefb1

ekr requested a review from chris-wood September 11, 2025 18:22

paulwouters reviewed Sep 11, 2025

View reviewed changes

kaduk reviewed Sep 11, 2025

View reviewed changes

ekr commented Sep 11, 2025

View reviewed changes

ekr added 2 commits September 11, 2025 16:48

Revert "version -> legacy_version"

18cd703

This reverts commit ffabe24.

Fix editorial error

20108f4

martinthomson reviewed Sep 12, 2025

View reviewed changes

Revert a change which would be too disruptive even though it's

3cb87d0

slightly more technically correct to talk about "key" rather than "keys". This reverts commit 31a02ac.

Revert "Discarded immediately after use"

1c910d2

This reverts commit d3c0cee.

ekr merged commit 46b909e into tlswg:main Sep 13, 2025
2 checks passed

	immediately fater use. Clients MUST NOT use tickets for longer than
	immediately after use. Clients MUST NOT use tickets for longer than

	update its sending traffic key using the KeyUpdate handshake message
	update its sending traffic key and IV using the KeyUpdate handshake message

	update its sending traffic key using the KeyUpdate handshake message
	update its application traffic secret using the KeyUpdate handshake message

Comments from Sergey Agievich review #1396

Comments from Sergey Agievich review #1396

Uh oh!

Conversation

ekr commented Sep 11, 2025

Uh oh!

ekr commented Sep 11, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ekr Sep 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ekr left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agievich commented Sep 12, 2025

Uh oh!

paulwouters commented Sep 12, 2025

Uh oh!

ekr commented Sep 12, 2025

Uh oh!

ekr commented Sep 12, 2025

Uh oh!

ekr Sep 12, 2025 •

edited

Loading

ekr left a comment •

edited

Loading