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

[Christian Huitema <[e-mail]>]

huitema commented on this pull request.

Some nits. We need a bit more clarity on the sending of KEY ACTIVE during the handshake, but otherwise this is good.

>  
-The KEY_PHASE bit allows a recipient to detect a change in keying material
-without necessarily needing to receive the first packet that triggered the
-change.  An endpoint that notices a changed KEY_PHASE bit can update keys and
-decrypt the packet that contains the changed bit.
+The Key Phase bit is used to indicate which packet protection keys are used to

I am confused. Line 866 mentions a Key Update bit, and here we see a Key Phase bit. Is that the same bit, or do we now have 2 bits?

> @@ -863,7 +863,7 @@ This protection applies to the least-significant bits of the first byte, plus
 the Packet Number field.  The four least-significant bits of the first byte are
 protected for packets with long headers; the five least significant bits of the
 first byte are protected for packets with short headers.  For both header forms,
-this covers the reserved bits and the Packet Number Length field; the Key Phase
+this covers the reserved bits and the Packet Number Length field; the Key Update
 bit is also protected for packets with a short header.

See comments in Key Update section. I would vote for keeping the Key Phase name.

> +corresponding key and IV are created from that secret as defined in
+{{protection-keys}}.  The header protection key is not updated.
+
+The endpoint toggles the value of the Key Phase bit, and uses the updated key
+and IV to protect all subsequent packets.
+
+An endpoint MUST NOT initiate a key update prior to having received a
+KEYS_ACTIVE frame in a packet from the current key phase.  A subsequent key
+update can only be performed after the endpoint has successfully processed a
+KEYS_ACTIVE frame from a packet with a matching key phase.  This ensures that
+keys are available to both peers before another can be initiated.
+
+Once an endpoint has successfully processed a packet with the same key phase, it
+can send a KEYS_ACTIVE frame.  Endpoints MAY defer sending a KEYS_ACTIVE frame
+after a key update (see {{key-update-old-keys}}).
+

I assume that "processed a packet" means "received, successfully decrypted, and processed the content of the frames." But I have to guess. Encrypting  and sending is a form of processing too. The term first occurs in section 8.1, and it is a bit puzzling there too. I would suggest adding a formal definition in "1.2. Terms and Definitions ".

> +## Responding to a Key Update
+
+An endpoint that sends a KEYS_ACTIVE frame can accept further key updates.  A
+key update can happen even without seeing a KEYS_ACTIVE frame from the peer.  If
+a packet is received with a key phase that differs from the value the endpoint
+used to protect the packet containing its last KEYS_ACTIVE frame, the endpoint
+creates a new packet protection secret for reading and the corresponding key and
+IV.  An endpoint uses the same key derivation process as its peer uses to
+generate keys for receiving.
+
+If the packet protection is successfully removed using the updated key and IV,
+then the keys the endpoint initiates a key update in response, as described in
+{{key-update-initiate}}.  An endpoint that responds to a key update MUST send a
+KEYS_ACTIVE frame to indicate that it is both sending and receiving with updated
+keys, though it MAY defer sending the frame (see {{key-update-old-keys}}).
+

Isn't that a restatement of the lines 1167-1169? Why do we have the same text twice, with slightly different phrasing, "processed" vs. "packet protection is successfully removed" ?

> +corresponding key and IV are created from that secret as defined in
+{{protection-keys}}.  The header protection key is not updated.
+
+The endpoint toggles the value of the Key Phase bit, and uses the updated key
+and IV to protect all subsequent packets.
+
+An endpoint MUST NOT initiate a key update prior to having received a
+KEYS_ACTIVE frame in a packet from the current key phase.  A subsequent key
+update can only be performed after the endpoint has successfully processed a
+KEYS_ACTIVE frame from a packet with a matching key phase.  This ensures that
+keys are available to both peers before another can be initiated.
+
+Once an endpoint has successfully processed a packet with the same key phase, it
+can send a KEYS_ACTIVE frame.  Endpoints MAY defer sending a KEYS_ACTIVE frame
+after a key update (see {{key-update-old-keys}}).
+

A lot of that text is going to be specified again in the following paragraphs. Do we need a paraphrase here?

> +place of discarded keys when key updates are not permitted.
+
+
+## Using Old Keys {#key-update-old-keys}
+
+During a key update, packets protected with older keys might arrive if they were
+delayed by the network.  If those old keys are available, then they can be used
+to remove packet protection.
+
+After a key update, an endpoint MAY delay sending the KEYS_ACTIVE frame by up to
+three times the Probe Timeout (PTO, see {{QUIC-RECOVERY}}) to minimize the
+number of active keys it maintains.  During this time, an endpoint can use old
+keys to process delayed packets rather than enabling a new key update.  This
+only applies to key updates that use the Key Phase bit; endpoints MUST NOT defer
+sending of KEYS_ACTIVE during and immediately after the handshake.
+

"key updates that use the Key Phase bit". That's a weird way to put it. Maybe a reference to the ladder diagram in the transport spec?


> +corresponding key and IV are created from that secret as defined in
+{{protection-keys}}.  The header protection key is not updated.
+
+The endpoint toggles the value of the Key Phase bit, and uses the updated key
+and IV to protect all subsequent packets.
+
+An endpoint MUST NOT initiate a key update prior to having received a
+KEYS_ACTIVE frame in a packet from the current key phase.  A subsequent key
+update can only be performed after the endpoint has successfully processed a
+KEYS_ACTIVE frame from a packet with a matching key phase.  This ensures that
+keys are available to both peers before another can be initiated.
+
+Once an endpoint has successfully processed a packet with the same key phase, it
+can send a KEYS_ACTIVE frame.  Endpoints MAY defer sending a KEYS_ACTIVE frame
+after a key update (see {{key-update-old-keys}}).
+

Initial to Handshake, and Handshake to 1-RTT. Are these special cases of Key Update?

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

Sending the KEY ACTIVE there seems wrong. The logic "I have processed a packet at this key level". That has not happened yet, will only happen at the sending of `<- Handshake[1]: ACK[0]`. That's the same reason why you correctly do not place a Key Active frame in the packet `<- 1-RTT[0]: STREAM[1, "..."]`

> @@ -1283,15 +1281,13 @@ Initial[0]: CRYPTO[CH]
 0-RTT[0]: STREAM[0, "..."] ->
 
                                  Initial[0]: CRYPTO[SH] ACK[0]
-                        Handshake[0] CRYPTO[EE, CERT, CV, FIN]
+           Handshake[0]: KEYS_ACTIVE, CRYPTO[EE, CERT, CV, FIN]
                           <- 1-RTT[0]: STREAM[1, "..."] ACK[0]

Same issue. You cannot send Key Active there because the server has not yet received an Handshake packet from the peer.

> @@ -3520,7 +3520,7 @@ carries ACKs in either direction.
 
 ~~~
 +-+-+-+-+-+-+-+-+
-|1|1| 0 |R R|P P|
+|1|1| 0 | R | P |
 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

Unrelated change. Makes the PR harder to read.

>  #### Abandoning Initial Packets {#discard-initial}
 
-A client stops both sending and processing Initial packets when it sends its
-first Handshake packet.  A server stops sending and processing Initial packets
-when it receives its first Handshake packet.  Though packets might still be in
-flight or awaiting acknowledgment, no further Initial packets need to be
-exchanged beyond this point.  Initial packet protection keys are discarded (see
-Section 4.10 of {{QUIC-TLS}}) along with any loss recovery and congestion
-control state (see Sections 5.3.1.2 and 6.9 of {{QUIC-RECOVERY}}).
+Endpoints cease both sending and processing Initial packets when it both sends
+and receives a Handshake or 1-RTT packet containing a KEYS_ACTIVE frame.  Though
+packets might still be in flight or awaiting acknowledgment, no further Initial
+packets need to be exchanged beyond this point.  Initial packet protection keys
+are discarded (see Section 4.10 of {{QUIC-TLS}}) along with any loss recovery
+and congestion control state (see Sections 5.3.1.2 and 6.9 of {{QUIC-RECOVERY}}).
 

The server side specification feels wrong. The server does not know that the peer has received the server initial until it receives the KEY ACTIVE (Handshake) from the client. Yes, there is an "implicit" dependency that the Handshake cannot be processed without receiving first the Initial packet. There will be cases where Initial is lost, and both Initial and Handshake have to be retransmitted.

> @@ -3843,7 +3847,7 @@ short packet header.
  0                   1                   2                   3
  0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
 +-+-+-+-+-+-+-+-+
-|0|1|S|R|R|K|P P|
+|0|1|S| R |K| P |
 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

Unrelated change.

-- 
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-203046954