[bitreq] Bitreq client builder#502
Conversation
APonce911
changed the title
|
Holla if you want/need any review mate. Otherwise, for me at least, while there are WIP commits I'll assume you are happily working on it still. |
|
Holla at me also if you want me to kick CI into action for you. Once you have a patch merged CI will run automatically. You could just do a basic fix somewhere else in the repo if this is going to be long running work. |
|
Thanks @tcharding. My idea with this draft was to give a bit of visibility of where we're at and to guarantee that the solution is in the right path. No formal code review is needed yet. Thanks for the tip about the CI, so far it's ok not to run. |
| } | ||
|
|
||
| #[cfg(all(feature = "rustls", feature = "tokio-rustls"))] | ||
| pub(super) async fn wrap_async_stream_with_configs( |
There was a problem hiding this comment.
same as wrap_async_stream but with additional parameter custom_client_config
APonce911
requested review from
TheBlueMatt,
oleonardolima and
tnull
as code owners
|
@tcharding hey mate, can you please run the CI once? It's ready for review. |
TheBlueMatt
left a comment
TheBlueMatt
left a comment
There was a problem hiding this comment.
Obv needs docs, but at a high level looks good to me.
bitreq/src/client.rs
Outdated
| Self { capacity: 1, client_config: None } | ||
| } | ||
|
|
||
| pub fn with_root_certificate<T: Into<Vec<u8>>>(mut self, certificate: T) -> Self { |
There was a problem hiding this comment.
Its pretty awkward that we don't error here but then just ignore certs silently. IMO we kinda need to actually make this fallible, store a root cert store in ClientConfig, and pass that around instead. We'll have to have an abstract root cert store in rustls_stream (or somewhere), but that should be pretty straightforward.
There was a problem hiding this comment.
@TheBlueMatt makes sense. Will work on that tomorrow! Thanks
There was a problem hiding this comment.
@TheBlueMatt updated the PR with your suggestion. I moved the new certificates logic to a new module. We still have lots of stuff in the rustls_stream module, but I thought refactoring that would make this PR too large.
There was a problem hiding this comment.
This still looks like its currently infallible and not parsing the cert? We should validate the cert here.
There was a problem hiding this comment.
Hi @TheBlueMatt. Currently the method is fallible, it panics on expect() if certificate appending fails. However, I intentionally kept the return type as Self (not Result<Self, Error>) to maintain fluent builder chaining without requiring callers to handle Result at every step. Would you prefer that I return Result<Self, Error> here?
About parsing, I don't know if I understand you're question well, I'm sorry. But we're currently parsing it on the new Certificates module. I tried to keep this Client interface simple and more details about certificate management inside this module.
There was a problem hiding this comment.
Ah, panicing isn't really "fallible" in Rust lingo :). Yes, we should return a Result.
There was a problem hiding this comment.
Updated the return type @TheBlueMatt. Let me know about the cert parsing you've mentioned.
With the updated code we're receiving encoding errors as well.
Print from a test I've run with an invalid certificate
DanGould
left a comment
DanGould
left a comment
There was a problem hiding this comment.
Great start here that's gonna solve a real problem for me. Saw something that's nice to have when we're dealing with loose types. Use cert_der: Vec<u8> if the type signature expects DER encoding please
| } | ||
|
|
||
| impl Certificates { | ||
| pub(crate) fn new(certificate: Option<&Vec<u8>>) -> Result<Self, Error> { |
There was a problem hiding this comment.
Since this is just a Vec<u8> tell me what format you're expecting input both in the param name and docstring
| pub(crate) fn new(certificate: Option<&Vec<u8>>) -> Result<Self, Error> { | |
| /// Pass a new certificate in DER [?] format | |
| pub(crate) fn new(certificate_der: Option<&Vec<u8>>) -> Result<Self, Error> { |
There was a problem hiding this comment.
@DanGould Great catch. Yes, you're correct. We must receive a DER encoded certificate.
I've updated the variable to cert_der to give more clarity. About the doc, I've written on the Client class since it's public.
What do you think?
| } | ||
| } | ||
|
|
||
| #[cfg(feature = "rustls")] |
There was a problem hiding this comment.
We need support for native-tls, too.
There was a problem hiding this comment.
@TheBlueMatt heads up - we currently don't support native-tls for async
connections. The AsyncConnection only calls wrap_async_stream for the
tokio-rustls feature:
https://github.com/rust-bitcoin/corepc/blob/master/bitreq/src/connection.rs#L275
Interestingly, wrap_async_stream exists for tokio-native-tls too but isn't
being called from AsyncConnection:
https://github.com/rust-bitcoin/corepc/blob/master/bitreq/src/connection/rustls_stream.rs#L150
Would you prefer to:
- Add native-tls support in this PR by hooking up the existing
wrap_async_stream for tokio-native-tls? - Defer it to a follow-up PR?
Happy to implement either way.
There was a problem hiding this comment.
Huh? Its definitely called for async connection, just only in some feature flag combos.
There was a problem hiding this comment.
@TheBlueMatt I've been working on the native-tls implementation, but the changes ended up being quite large. Adding them on top of this PR would (in my opinion) make it significantly harder to review and test properly.
That's why I created a separate draft PR here to continue that work:
APonce911#1
In my view, this PR already delivers value for users relying on rustls.
What do you think about keeping this one strictly focused on rustls (and handling native-tls in the other PR)?
There was a problem hiding this comment.
No, we should definitely do both. We shouldn't land things that leave the codebase in a broken state, and that PR is not particularly large. There's a lot of random code to review in this PR as is that handles the no-support case which I'd rather not do.
There was a problem hiding this comment.
@TheBlueMatt No problem then, will merge both features. New PR or we keep in this one?
I don't know if I understand your point about broken state, do you mean having it implemented partially by features or do you see something that could cause a regression?
There was a problem hiding this comment.
For record(no need to answer)
native-tls https failing on master
tests command
cargo test -F async-https-native-tls -- --no-capture --test-threads=1
failing tests
#[tokio::test]
#[cfg(all(feature = "native-tls", not(feature = "rustls"), feature = "tokio-native-tls"))]
async fn test_https() {
assert_eq!(get_status_code(bitreq::get("https://example.com")).await, 200);
assert_eq!(get_status_code(bitreq::get("https://example.com")).await, 200);
}
#[tokio::test]
#[cfg(all(feature = "native-tls", not(feature = "rustls"), feature = "tokio-native-tls"))]
async fn test_https_with_client() {
setup();
let client = bitreq::Client::new(1);
let response = client.send_async(bitreq::get("https://example.com")).await.unwrap();
assert_eq!(response.status_code, 200);
}
Error
Error::HttpsFeatureNotEnabled
Evidence
Why
As i've mentioned, https for native-tls is currently not working, it's feature gated for tokio-rustls ONLY. This error is triggered on the AsyncConnection::new connection.rs#L275
Fix
This is corrected it the draft PR I've mentioned above, together with the custom root certificate support. Will merge on this PR or open a new one depending on the answer above
bitreq/src/client.rs
Outdated
| Self { capacity: 1, client_config: None } | ||
| } | ||
|
|
||
| pub fn with_root_certificate<T: Into<Vec<u8>>>(mut self, certificate: T) -> Self { |
There was a problem hiding this comment.
This still looks like its currently infallible and not parsing the cert? We should validate the cert here.
…onal compiling logic
…low ClientBuilder
…nce per client instead of per connection.
| pub(crate) use self::enabled::*; | ||
|
|
||
| #[cfg(not(all(feature = "rustls", feature = "tokio-rustls")))] | ||
| mod disabled { |
There was a problem hiding this comment.
Huh? We need to implement custom root cert support for native-tls as well, not just blindly ignore what the downstream code gives us.
There was a problem hiding this comment.
I don't see where we're blindly ignoring. Can you please point out? Will gladly adjust it.
Tls currrently depends on Async, rustls and tokio-rustls. Since many functions are depending on ClientConfig we have to have it defined even when some of those features are off. Otherwise Rust won't compile due to missing types.
about native-tls I've answed here
|
Larger PR implementing both rustls and native-tls here: #516 |
4 participants
Summary
This PR addresses issue #473 by creating a Client Builder able to receive custom root certificates at runtime.
The ClientBuilder requires the same features as Client (async-https-rustls).
Changes