Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove handshake confirmed test for KeyUpdate #3212

Closed
ekr opened this issue Nov 10, 2019 · 8 comments
Closed

Remove handshake confirmed test for KeyUpdate #3212

ekr opened this issue Nov 10, 2019 · 8 comments
Labels
-tls design An issue that affects the design of the protocol; resolution requires consensus.

Comments

@ekr
Copy link
Collaborator

ekr commented Nov 10, 2019

S 6.1 Says

An endpoint MUST NOT initiate a key update prior to having confirmed
the handshake (Section 4.1.2). An endpoint MUST NOT initiate a
subsequent key update prior unless it has received an acknowledgment
for a packet that was sent protected with keys from the current key
phase.

However, I believe that the requirement to have confirmed the
handshake is redundant. As a reminder, the conditions under which the
handshake is confirmed are:

(1) the handshake is complete, and
(2) the endpoint has received an acknowledgment for a
packet sent with 1-RTT keys

  1. On the client side, the 1-RTT keys aren't available until the handshake
    is complete, so it's not possible to send 1-RTT packet and therefore
    not possible to have them ACKed.

  2. On the server side, the 1-RTT keys are available before the handshake
    is complete, but because the server is forbidden from reading any
    1-RTT data before processing CFIN (S 5.6), it also cannot have
    received an acknowledgement for a packet sent with 1-RTT keys.

In other words, condition (2) implies condition (1), no?

@martinthomson martinthomson added -tls design An issue that affects the design of the protocol; resolution requires consensus. labels Nov 11, 2019
@martinthomson
Copy link
Member

Rather than get bogged down in the minutiae of the requirement and the nature of confirmation, let's just say that this was added when we added the definition of "confirmed".

But an endpoint that can accept 1-RTT in key phase 0 can also receive 1-RTT in key phase 1 equally well. So there is no functional reason not to allow updates as soon as 1-RTT keys are available. I think that we should just loosen this entirely. It might not be a good idea to waste a key phase like that, but it certainly doesn't break anything.

@kazuho
Copy link
Member

kazuho commented Nov 11, 2019

So there is no functional reason not to allow updates as soon as 1-RTT keys are available. I think that we should just loosen this entirely. It might not be a good idea to waste a key phase like that, but it certainly doesn't break anything.

If we are to loosen the requirement, I would prefer also adding the 3 PTO recommendation regarding the transition from 1-RTT key (phase 0) to 1-RTT key (phase 1).

Because there is a concern regarding performance and complexity.

When 0-RTT is used, the client's send key transitions from 0-RTT key -> 1-RTT key (phase 0) -> 1-RTT key (phase 1) -> ...

Implementation-wise, it would be best to recommend every transition between these keys to be no earlier than 3 PTO, because then, the server can have a consistent rule on when it would be creating new key, replacing the old one.

At the moment, we are near to that, by saying that the 1-RTT key cannot be updated until an ACK for an 1-RTT packet is being received. Just loosening this rule would require server developers to have different timing on how it drops 0.5-RTT key vs. 1-RTT keys - it would be a complication.

@nibanks
Copy link
Member

nibanks commented Nov 11, 2019

IMO we don't need to add a 3 PTO before key change. We've done performance tests with msquic and changing the key after each key update is ACK'ed and it showed absolutely no impact on throughput. As far as extra complexity it's hard to say. We implemented key update as it was spec'ed now and didn't have to make any complicated extra logic to get to this state.

@kazuho
Copy link
Member

kazuho commented Nov 11, 2019

@nibanks

We've done performance tests with msquic and changing the key after each key update is ACK'ed and it showed absolutely no impact on throughput. As far as extra complexity it's hard to say. We implemented key update as it was spec'ed now and didn't have to make any complicated extra logic to get to this state.

I'm not sure if what you describe is following the recommendation. Quoting from the spec:

  • Endpoints SHOULD wait three times the PTO before initiating a key update after receiving an acknowledgment that confirms that the previous key update was received. (section 6.5)
  • Endpoints SHOULD instead defer the creation of the next set of receive packet protection keys until some time after a key update completes, up to three times the PTO. (section 6.3)

Depending on how the peer is implemented, updating the key as soon as receiving an ACK would cause packet drops.

@nibanks
Copy link
Member

nibanks commented Nov 11, 2019

I'm saying that while in a theoretical sense waiting the 3 PTO (in any of the scenarios) might possibly reduce packet loss, with msquic to msquic it made zero difference.

@mnot mnot added this to Triage in Late Stage Processing Nov 12, 2019
@mnot mnot moved this from Triage to Design Issues in Late Stage Processing Nov 12, 2019
@martinthomson martinthomson changed the title Remove handshake confirmed test for KeyUpdte Remove handshake confirmed test for KeyUpdate Nov 12, 2019
@martinthomson
Copy link
Member

@ekr, I took another look at this and reviewed the minutes. I think that the outcome is no change.

An endpoint MUST NOT initiate a key update prior to having confirmed
the handshake (Section 4.1.2). An endpoint MUST NOT initiate a
subsequent key update prior unless it has received an acknowledgment
for a packet that was sent protected with keys from the current key
phase.

The first sentence talks about the first key update. While we all agree that this is a stricter restriction than absolutely necessary, the definition we agreed for "confirmed" (with HANDSHAKE_DONE) is now distinct enough to warrant the special text in that case. As long as we are OK with that outcome, I don't think that this needs to change (@kazuho certainly argued for some way to hold the first key update back, which I think was derived from not wanting too many keys active at the same time).

If we wanted to allow more aggressive updates, then I would propose the most aggressive option:

An endpoint MAY initiate the first key update immediately after 1-RTT keys are available. For the first key update, as a receiver has no difficulty distinguishing between the first two sets of 1-RTT packet protection keys, endpoints can trigger a key update with the first 1-RTT packets they send.

The second sentence uses the word "subsequent" and so applies to all other key updates. That doesn't need to change.

@kazuho
Copy link
Member

kazuho commented Nov 28, 2019

@martinthomson

As long as we are OK with that outcome, I don't think that this needs to change (@kazuho certainly argued for some way to hold the first key update back, which I think was derived from not wanting too many keys active at the same time).

+1 to not changing status quo.

My argument was that there should be a gap between when an endpoint starts using 1-RTT keys and when it issues the first key update, based on the agreement is that the minimum number of keys that a endpoint is required to possess should be 2.

Unlike Initial or Handshake keys, 0-RTT key does not have a defined moment when it is to be discarded. Instead, it is discarded some time after exchange happens using the first set of 1-RTT keys, which IMO is exactly the same as how key update retires an old set of 1-RTT keys.

Based on this view, it is natural to hold two set of application traffic keys (regardless of the key being 0-RTT or 1-RTT), rotating them as necessary. As we have discussed and agreed, there needs to be a gap between each installation event of a new traffic key.

In status quo, the minimum gap between the use of the first 1-RTT key and the use of the second 1-RTT key is 1-RTT. The recommended gap between the following key update events are 3 PTO. Even though the size of the gaps are different, they both serve as a window at which the old key (could be either 0-RTT key or 1-RTT key) can coexist. This is fine.

My argument was that if we are to change the requirement, it would make sense to consider aligning the size of the recommended gap between the use of 1-RTT key and the use of the second 1-RTT key to 3 PTO.

@larseggert
Copy link
Member

Closed with no action, per the discussion

Late Stage Processing automation moved this from Design Issues to Text Incorporated Dec 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-tls design An issue that affects the design of the protocol; resolution requires consensus.
Projects
Late Stage Processing
  
Issue Handled
Development

No branches or pull requests

5 participants