Re: [quicwg/base-drafts] Rework Key Update (#2237)

[Nick Banks <[e-mail]>]

nibanks approved this pull request.

LGTM. I fully support this PR. Just some small nits for the most part.

>  
-Once the 1-RTT keys are established and the short header is in use, it is
-possible to update the keys. The KEY_PHASE bit in the short header is used to
-indicate whether key updates have occurred. The KEY_PHASE bit is initially set
-to 0 and then inverted with each key update.
+Once the 1-RTT keys are established and confirmed through the use of the
+KEYS_READY frame, it is possible to update the keys used to protect packets.

To me, the name KEYS_READY seems to indicate that the keys are about to be used, not that they are in use now. I think I'd prefer something like KEYS_ACTIVE or KEY_IN_USE. Just a personal preference though.

> @@ -1259,14 +1259,14 @@ Client                                                  Server
 Initial[0]: CRYPTO[CH] ->
 
                                  Initial[0]: CRYPTO[SH] ACK[0]
-                       Handshake[0]: CRYPTO[EE, CERT, CV, FIN]
+           Handshake[0]: KEYS_READY, CRYPTO[EE, CERT, CV, FIN]
                                  <- 1-RTT[0]: STREAM[1, "..."]
 
 Initial[1]: ACK[0]

Is this Initial necessary? Assuming it is coalesced with the Handshake that contains KEYS_READY, wouldn't that cause the Initial keys to be thrown away, implicitly ACK'ing outstanding Initial packets?

> @@ -5012,6 +5020,44 @@ Reason Phrase:
   This SHOULD be a UTF-8 encoded string {{!RFC3629}}.
 
 
+## KEYS_READY Frame {#frame-keys-ready}
+
+An endpoint sends a KEYS_READY frame (type=0x1e) to signal that it has installed
+keys for reading and writing packets.  Receipt of this frame in a packet

```suggestion
keys for both reading and writing packets.  Receipt of this frame in a packet
```
To be a bit more clear, it seems you are only supposed to send this when you are both keys are ready for use, not one or the other, correct?

>  
-                           1-RTT[1]: STREAM[55, "..."], ACK[0]
+               1-RTT[1]: KEYS_READY, STREAM[55, "..."], ACK[0]
                                        <- Handshake[1]: ACK[0]

Similarly with this Handshake packet. Is it necessary, assuming it was coalesced with the 1-RTT packet? On a side note, if we don't get rid of it, I think it might be better to put the Handshake packet before the 1-RTT packet in the list, since that's the order they'd be coalesced (ditto for the 0-RTT example below).

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/2237#pullrequestreview-202998585