-
Notifications
You must be signed in to change notification settings - Fork 65
[bitreq] Bitreq client builder #502
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
Changes from 28 commits
13e0cfd
e879020
182e5fb
92ca975
5af7539
b6650ba
bf1735d
da22cf9
a01979a
d89f559
04a1c3a
7011045
deb5397
5a97e80
a61b1ec
8000c6c
9b4a839
8d7ff6c
0538906
5fc031b
9c11211
22c2b2f
97b5d56
4a08c7d
2cf40aa
937e3ba
e682f92
fd47ea1
728ceb4
c9d941d
0f67697
65e7c41
ecf4d12
c11c920
cc84c5c
abf89cf
e8f47b6
97124a9
8382cc1
05f8e5d
2ef687e
bfe2763
b2fe156
f3851fb
6ac03d7
bf3a952
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| //! This example demonstrates the client builder with custom DER certificate. | ||
| //! to run: cargo run --example custom_cert --features async-https-rustls | ||
|
|
||
| #[cfg(not(feature = "async-https-rustls"))] | ||
| fn main() { | ||
| println!("This example requires the 'async-https-rustls' feature."); | ||
| } | ||
|
|
||
| #[cfg(feature = "async-https-rustls")] | ||
| fn main() -> Result<(), bitreq::Error> { | ||
| let runtime = tokio::runtime::Builder::new_current_thread() | ||
| .enable_io() | ||
| .build() | ||
| .expect("failed to build Tokio runtime"); | ||
|
|
||
| runtime.block_on(request_with_client()) | ||
| } | ||
|
|
||
| #[cfg(feature = "async-https-rustls")] | ||
| async fn request_with_client() -> Result<(), bitreq::Error> { | ||
| let url = "http://example.com"; | ||
| let cert_der = include_bytes!("../tests/test_cert.der"); | ||
| let client = bitreq::Client::builder().with_root_certificate(cert_der.as_slice()).build(); | ||
| // OR | ||
| // let cert_der: &[u8] = include_bytes!("../tests/test_cert.der"); | ||
| // let client = bitreq::Client::builder().with_root_certificate(cert_der).build(); | ||
| // OR | ||
| // let cert_vec: Vec<u8> = include_bytes!("../tests/test_cert.der").to_vec(); | ||
| // let client = bitreq::Client::builder().with_root_certificate(cert_vec.as_slice()).build(); | ||
|
|
||
| let response = client.send_async(bitreq::get(url)).await.unwrap(); | ||
|
|
||
| println!("Status: {}", response.status_code); | ||
| println!("Body: {}", response.as_str()?); | ||
|
|
||
| Ok(()) | ||
| } |
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,63 @@ | ||||||||
| #[cfg(feature = "rustls")] | ||||||||
| use rustls::RootCertStore; | ||||||||
| #[cfg(feature = "rustls-webpki")] | ||||||||
| use webpki_roots::TLS_SERVER_ROOTS; | ||||||||
|
|
||||||||
| use crate::Error; | ||||||||
|
|
||||||||
| #[derive(Clone)] | ||||||||
| pub(crate) struct Certificates { | ||||||||
| pub(crate) inner: RootCertStore, | ||||||||
| } | ||||||||
|
|
||||||||
| impl Certificates { | ||||||||
| pub(crate) fn new(certificate: Option<&Vec<u8>>) -> Result<Self, Error> { | ||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since this is just a
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @DanGould Great catch. Yes, you're correct. We must receive a DER encoded certificate. What do you think? |
||||||||
| let certificates = Self { inner: RootCertStore::empty() }; | ||||||||
|
|
||||||||
| if let Some(certificate) = certificate { | ||||||||
| certificates.append_certificate(certificate) | ||||||||
| } else { | ||||||||
| Ok(certificates) | ||||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| #[cfg(feature = "rustls")] | ||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need support for
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @TheBlueMatt heads up - we currently don't support native-tls for async 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 https://github.com/rust-bitcoin/corepc/blob/master/bitreq/src/connection/rustls_stream.rs#L150 Would you prefer to:
Happy to implement either way.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Huh? Its definitely called for async connection, just only in some feature flag combos.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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: 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)?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For record(no need to answer) native-tls https failing on mastertests command
failing tests ErrorError::HttpsFeatureNotEnabled Evidence
WhyAs 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
FixThis 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 |
||||||||
| pub(crate) fn append_certificate(mut self, certificate: &[u8]) -> Result<Self, Error> { | ||||||||
| let mut certificates = self.inner; | ||||||||
| certificates | ||||||||
| .add(&rustls::Certificate(certificate.to_owned())) | ||||||||
| .map_err(Error::RustlsAppendCert)?; | ||||||||
| self.inner = certificates; | ||||||||
| Ok(self) | ||||||||
| } | ||||||||
|
|
||||||||
| #[cfg(feature = "rustls")] | ||||||||
| pub(crate) fn with_root_certificates(mut self) -> Self { | ||||||||
| let mut root_certificates = self.inner; | ||||||||
|
|
||||||||
| // Try to load native certs | ||||||||
| #[cfg(feature = "https-rustls-probe")] | ||||||||
| if let Ok(os_roots) = rustls_native_certs::load_native_certs() { | ||||||||
| for root_cert in os_roots { | ||||||||
| // Ignore erroneous OS certificates, there's nothing | ||||||||
| // to do differently in that situation anyways. | ||||||||
| let _ = root_certificates.add(&rustls::Certificate(root_cert.0)); | ||||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| #[cfg(feature = "rustls-webpki")] | ||||||||
| { | ||||||||
| #[allow(deprecated)] | ||||||||
| // Need to use add_server_trust_anchors to compile with rustls 0.21.1 | ||||||||
| root_certificates.add_server_trust_anchors(TLS_SERVER_ROOTS.iter().map(|ta| { | ||||||||
| rustls::OwnedTrustAnchor::from_subject_spki_name_constraints( | ||||||||
| ta.subject, | ||||||||
| ta.spki, | ||||||||
| ta.name_constraints, | ||||||||
| ) | ||||||||
| })); | ||||||||
| } | ||||||||
| self.inner = root_certificates; | ||||||||
| self | ||||||||
| } | ||||||||
| } | ||||||||



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.
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 inrustls_stream(or somewhere), but that should be pretty straightforward.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.
@TheBlueMatt makes sense. Will work on that tomorrow! Thanks
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.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
https://github.com/rust-bitcoin/corepc/pull/502/changes#diff-4501e006f554583aacf6f0039d2c12e149a90447ec1af22f3c68d74820932c5fR28
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.
Ah, panicing isn't really "fallible" in Rust lingo :). Yes, we should return a Result.
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.
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
