Default client config #128
Conversation
|
@djc Dang! That was fast! 😅 I think that would almost 100% work. Would you be opposed to adding |
| #[cfg(feature = "hyper-rustls")] | ||
| impl DefaultClient { | ||
| /// Create a new default HTTP client with the given TLS configuration | ||
| pub fn with_config(config: rustls::ClientConfig) -> Self { |
There was a problem hiding this comment.
This does make rustls part of the public API, which is something I was trying to avoid before. Not sure about the trade-off here... @cpu thoughts?
I guess we're leaking crypto::Unspecified and crypto::KeyRejected in the Error type anyway so the additional rustls leakage might not matter much?
There was a problem hiding this comment.
IMO it feels like something that will be annoying in the future. I could swallow that speculative worry if the motivator was strong but in this case I'm not positive it is.
RFC 8555 §6.1 is unambiguous about HTTPS being REQUIRED. That's part of why Pebble's ACME interface isn't http:// despite being for local testing.
Is reading a CA PEM file and configuring a HyperClient really enough of a burden to want to fuse our release cycle w/ rustls? can we make that easier somehow instead?
There was a problem hiding this comment.
can we make that easier somehow instead?
The original issue made a proposal which makes it easier (or at least doesn't require API leakage)... but historically we haven't wanted to facilitate skipping verification.
There was a problem hiding this comment.
but historically we haven't wanted to facilitate skipping verification.
Right, I mean make it easier to load the server's non-standard trust anchor so we're doing HTTPS + validating the presented cert chain.
There was a problem hiding this comment.
How about something like this: #131 (some polish probably required)
I think with a bit of refactoring we could switch the Pebble tests to using that as well.
There was a problem hiding this comment.
I guess we're leaking crypto::Unspecified and crypto::KeyRejected in the Error type anyway so the additional rustls leakage might not matter much?
yeah 😓 Here's a first step towards wrangling that: #130
I don't think I want to re-export |
Yeah, that's fair. Screw it, I think what you've got there is more than generous. I say ship it 😂 |
djc
force-pushed
the
default-client-config
branch
from
8ec6c05 to
0e55e6d
Compare
|
Closing in favor of #131. |
Fixes #127.
cc @the-wondersmith what do you think? Does this adequately support your needs?