[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.
|
Just an FYI, you can use |
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.
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.
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