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

key_updated / key_discarded: rename generation to key_phase #390

Merged
merged 1 commit into from Feb 8, 2024

Conversation

marten-seemann
Copy link
Collaborator

Fixes #376. Also changing the type to uint64, since I don't see why the number of key updates would be limited to MaxUint32.

Copy link
Contributor

@rmarx rmarx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but I'd like to have approval from @hlandau on this as well to confirm it solves his original issue (somehow can't assign him as a reviewer though...)

@hlandau
Copy link

hlandau commented Feb 7, 2024

Looks good to me.

My understanding of how these events will be used is something like (for a client):

t=0: Initial EL provisioned; key_updated(client_initial_secret, new=...)
t=0: Initial EL provisioned; key_updated(server_initial_secret, new=...)

...

t=a: Handshake EL provisioned; key_updated(client_handshake_secret, new=...)
t=a; Handshake EL provisioned; key_updated(server_handshake_secret, new=...)

...

t=b; Initial EL dropped; key_discarded(client_initial_secret)
t=b; Initial EL dropped; key_discarded(server_initial_secret)

...

t=c; 1-RTT EL provisioned, key_updated(client_1rtt_secret, generation=0, new=...)
t=c; 1-RTT EL provisioned; key_updated(server_1rtt_secret, generation=0, new=...)

...

t=d; Handshake EL dropped; key_discarded(client_handshake_secret)
t=d; Handshake EL dropped; key_discarded(server_handshake_secret)

...
Key update:

t=m; Key update; key_updated(client_1rtt_secret, generation=1, old=..., new=...)
t=m; Key update; key_updated(server_1rtt_secret, generation=1, old=..., new=...)
(previous keys are kept around for a while to handle in-flight packets)

t=n; Key update completed; key_discarded(client_1rtt_secret, generation=0)
t=n; Key update completed; key_discarded(server_1rtt_secret, generation=0)

This works and gives good understanding to a qlog consumer of when a set of keys is available and handles time periods where multiple keys are available well. LGTM.

@marten-seemann
Copy link
Collaborator Author

The important part here is that the key phase keeps increasing:

Key update:

t=m; Key update; key_updated(client_1rtt_secret, key_phase=1, old=..., new=...)
t=m; Key update; key_updated(server_1rtt_secret, key_phase=1, old=..., new=...)

t=m; Key update; key_updated(client_1rtt_secret, key_phase=2, old=..., new=...)
t=m; Key update; key_updated(server_1rtt_secret, key_phase=2, old=..., new=...)

t=m; Key update; key_updated(client_1rtt_secret, key_phase=3, old=..., new=...)
t=m; Key update; key_updated(server_1rtt_secret, key_phase=3, old=..., new=...)

@marten-seemann marten-seemann merged commit dd8882e into main Feb 8, 2024
5 checks passed
@rmarx rmarx mentioned this pull request Feb 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clarify security: key events
3 participants