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

mandatory new field in key_updated compromises security #52

Closed
marten-seemann opened this issue Feb 17, 2020 · 2 comments · Fixed by #294
Closed

mandatory new field in key_updated compromises security #52

marten-seemann opened this issue Feb 17, 2020 · 2 comments · Fixed by #294
Assignees
Labels
future-versions issue will be tackled but not for the current iteration

Comments

@marten-seemann
Copy link
Collaborator

On a production system you probably don't want to log TLS secrets, even if you qlog (some of the) connections. The new field in the key_updated event therefore should not be mandatory.

I'm not sure I understand the old field either. If you're logging 1-RTT key updates and their sequence numbers, the key would already be written to the qlog, so there's no need to export it again. Or am I missing something?

Maybe it would be a good idea to keep key material to the SSLKEYLOGFILE and not even offer an option to write them to qlog?

@rmarx
Copy link
Contributor

rmarx commented Feb 17, 2020

Additionally, we should add an "owner" field to the key_update event.

Now, difference between client/server keys is made with the trigger and also the KeyType: this should be made more consistent with the other events. See also #44.

An endpoint would then emit separate events for client and server key updates, which should work event if key calculation is delayed (though not 100% sure yet).

@rmarx rmarx added the future-versions issue will be tackled but not for the current iteration label Oct 31, 2020
@LPardue
Copy link
Member

LPardue commented Mar 25, 2023

I support not logging secrets

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
future-versions issue will be tackled but not for the current iteration
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants