Extend ClientConfig to allow for setting key_log

[graves]

This addresses #286

It extends rustls::ClientConfig with a ClientConfigKeyLogExt to allow setting key_log on the ClientConfig as part of the builder pattern.

let tls_config = rustls::ClientConfig::builder()
  .with_native_roots()? 
  .with_no_client_auth()
  .with_key_log_file(); 

This seemed useful and looked like an easy addition so here it is. Writing a unit test seemed like more overhead than it was worth so I left it out but if you'd like a test just let me know.

Thanks!

[cpu]

Thanks! I have some doc nits for consideration but the mechanics look good.

[cpu]

I think this is true for with_key_log_file(), but not for with_key_log() right? There's probably enough explanation on with_key_log_file() to remove this bit to avoid confusion.

[cpu]

I'd be tempted to put a warning after this headline similar to the one the Rusts docs have for KeyLog to emphasize this should be used carefully:

Secrets passed over the interface are extremely sensitive and can break the security of past, present and future sessions.

[cpu]

Maybe worth using the word "replace" somewhere here similar to the other fn to emphasize if you call with_key_log(...) and then with_key_log_file(...) it isn't setting up both, it's stomping the first.

[djc]

I'm not sure this is worth doing? It seems like this would be easy enough to do using existing API, and I'm not sure how easy we want to make this?

[graves]

I understand where you're coming from but I feel like people building products that use TLS should have ergonomic access to the available APIs that let them debug their programs. I saw in the issue thread that you'd accept a PR so I opened one.

I can see myself using this as I continue to open PRs for rustls projects but again, I understand your concern and take no affront to you closing the issue and the PR.

[djc]

Let's see what the other maintainers think...

[ctz]

I feel like the the ergonomic builder-style API should be limited to things that we expect all users to make a decision on, and the less ergonomic and less discoverable setting-fields-on-ClientConfig-style API should be used for the rest. So in that way I don't really think we should do this.

In general I don't really want to put the keylog/SSLKEYLOGFILE stuff too far "forward" in the API. I've even been considering gating/defanging KeyLogFile on debug builds to follow the "MUST NOT" in https://tlswg.org/sslkeylogfile/draft-ietf-tls-keylogfile.html#name-applicability-statement

[djc]

I saw in the issue thread that you'd accept a PR so I opened one.

Yeah, my thinking has evolved since then. Sorry for turning down your PR!

[graves]

Hey, no problem, I'm here to make things better, not worse! Thanks for the review and information.