Re: [quicwg/base-drafts] Rewrite key update section (#3050)

[ianswett <[e-mail]>]

ianswett commented on this pull request.

This generally looks like an improvement, some suggestions.

> +: Keys of packets other than the 1-RTT packets are never updated; their keys are
+  derived solely from the TLS handshake state.
+
+The endpoint that initiates a key update also updates the keys that it uses for
+receiving packets.  These keys will be needed to process packets the peer sends
+after updating.
+
+An endpoint SHOULD retain old keys so that packets sent by its peer prior to
+receiving the key update can be processed.  Discarding old keys too early can
+cause delayed packets to be discarded.  Discarding packets will be interpreted
+as packet loss by the peer and could adversely affect performance.
+
+
+## Responding to a Key Update
+
+Once an endpoint has acknowledged a packet in the current key phase, a peer is

I misread this a few times at first, so how about "A peer is permitted to initiate a key update after receiving an acknowledgement of a packet in the current key phase."  Or you could remove this sentence entirely given there's a MUST NOT above.

> +keys for receiving.
+
+If a packet is successfully processed using the next key and IV, then the peer
+has initiated a key update.  The endpoint MUST update its send keys to the
+corresponding key phase in response, as described in {{key-update-initiate}}.
+Sending keys MUST be updated before sending an acknowledgement for the packet
+that was received with updated keys.  By acknowledging the packet that triggered
+the key update in a packet protected with the updated keys, the endpoint signals
+that the key update is complete.
+
+An endpoint can defer sending the packet or acknowledgement according to its
+normal packet sending behaviour; it is not necessary to immediately generate a
+packet in response to a key update.  The next packet sent by the endpoint will
+use the updated keys.  The next packet that contains an acknowledgement will
+cause the key update to be completed.  If an endpoint detects a second update
+before it has sent any packets with updated keys or an acknowledgement for the

It has to send an acknowledgement, so I think this can be more restrictive?
```suggestion
before it has sent any packets with updated keys containing an acknowledgement for the
```

> +keys twice without awaiting confirmation.  An endpoint MAY treat consecutive key
+updates as a connection error of type KEY_UPDATE_ERROR.
+
+An endpoint that receives an acknowledgement that is carried in a packet
+protected with old keys where any acknowledged packet was protected with newer
+keys MAY treat that as a connection error of type KEY_UPDATE_ERROR.  This
+indicates that a peer has received and acknowledged a packet that initiates a
+key update, but has not updated keys in response.
+
+
+## Timing of Receive Key Generation {#receive-key-generation}
+
+Endpoints responding to an apparent key update MUST NOT generate a timing
+side-channel signal that might indicate that the Key Phase bit was invalid (see
+{{header-protect-analysis}}).  Endpoints can use dummy packet protection keys in
+place of discarded keys when key updates are not permitted; using dummy keys

There's no way to indicate key updates are not permitted at all, correct?  Maybe say "not yet permitted" to be slightly clearer that it's too early/fast, and key updates are not an optional feature.

> +has initiated a key update.  The endpoint MUST update its send keys to the
+corresponding key phase in response, as described in {{key-update-initiate}}.
+Sending keys MUST be updated before sending an acknowledgement for the packet
+that was received with updated keys.  By acknowledging the packet that triggered
+the key update in a packet protected with the updated keys, the endpoint signals
+that the key update is complete.
+
+An endpoint can defer sending the packet or acknowledgement according to its
+normal packet sending behaviour; it is not necessary to immediately generate a
+packet in response to a key update.  The next packet sent by the endpoint will
+use the updated keys.  The next packet that contains an acknowledgement will
+cause the key update to be completed.  If an endpoint detects a second update
+before it has sent any packets with updated keys or an acknowledgement for the
+packet that initiated the key update, it indicates that its peer has updated
+keys twice without awaiting confirmation.  An endpoint MAY treat consecutive key
+updates as a connection error of type KEY_UPDATE_ERROR.

I'll note that this means every time there is a key update, there is a small window of time where a peer can reset the connection unless the receiver successfully decrypts the incoming packet before sending the connection close.  Maybe what you're intending is that the receiver detects a second update and is processed, but I wasn't sure.

> +keys.
+
+Packets with higher packet numbers MUST be protected with either the same or
+newer packet protection keys than packets with lower packet numbers.  An
+endpoint that successfully removes protection with old keys when newer keys were
+used for packets with lower packet numbers MUST treat this as a connection error
+of type KEY_UPDATE_ERROR.
+
+
+## Receiving with Different Keys {#old-keys-recv}
+
+For receiving packets during a key update, packets protected with older keys
+might arrive if they were delayed by the network.  Retaining old packet
+protection keys allows these packets to be successfully processed.
+
+As packet protected with keys from the next key phase use the same Key Phase

```suggestion
As packets protected with keys from the next key phase use the same Key Phase
```

> +channel that might reveal which keys were used to remove packet protection.
+
+Alternatively, endpoints can retain only two sets of packet protection keys,
+swapping previous for next after enough time has passed to allow for reordering
+in the network.  In this case, the Key Phase bit alone can be used to select
+keys.
+
+An endpoint SHOULD allow a period between one and two times the Probe Timeout
+(PTO; see {{QUIC-RECOVERY}}) after a key update before it creates the next set
+of packet protection keys.  These MAY replace the previous keys at that time.
+With the caveat that PTO is a subjective measure - that is, a peer could have a
+different view of the RTT - this time is expected to be long enough that any
+reordered packets would be declared lost by a peer even if they were
+acknowledged and short enough to allow for subsequent key updates.
+
+Endpoints SHOULD allow for the possibility that a peer is not able to handle a

I'm confused by the one to two times PTO vs three times PTO here.  I prefer this second paragraph that states you need to way 3*PTO after key update is complete, minus the fact I'm unclear what "a peer is not able to handle a key update during this period" means?

-- 
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/3050#pullrequestreview-294411517