-
Notifications
You must be signed in to change notification settings - Fork 141
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
allow tls keylog #1261
allow tls keylog #1261
Conversation
/// returns true if we should initialize a tls keylog | ||
/// based on the `SSLKEYLOGFILE` environment variable | ||
pub fn use_env_tls_keylog(&self) -> bool { | ||
self.danger_tls_keylog == "env_keylog" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@neonphog why not just make danger_tls_keylog
a bool? Making it a string invites people to wonder if it's actually some sort of parameter, but it's really just a flag that requires a magic string to activate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@maackle - It feels more enum-y to me though, like we might want to have other keylogging options (network?) in the future, hence the string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fyi, this fn in particular is really an internal api... it just so happens the different kitsune_p2p
crate (instead of kitsune_p2p_types
) needs to use it, hence the pub
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I guess I hadn't imagined there being any other future options for this, so this looks good
/// returns true if we should initialize a tls keylog | ||
/// based on the `SSLKEYLOGFILE` environment variable | ||
pub fn use_env_tls_keylog(&self) -> bool { | ||
self.danger_tls_keylog == "env_keylog" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I guess I hadn't imagined there being any other future options for this, so this looks good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Summary
danger_tls_keylog_path_prefix
which will write keylog files for quic tls sessionsTODO: