-
Notifications
You must be signed in to change notification settings - Fork 51
[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
Open
APonce911
wants to merge
24
commits into
rust-bitcoin:master
Choose a base branch
from
APonce911:bitreq-client-builder
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+346
−9
Open
Changes from all commits
Commits
Show all changes
24 commits
Select commit
Hold shift + click to select a range
13e0cfd
update test_https to use local http server
APonce911 e879020
add test_https_with_client
APonce911 182e5fb
WIP: add ClientBuilder for configuring Client instances
APonce911 92ca975
WIP: pass ClientConfig struct to tls layer
APonce911 5af7539
WIP: include feature on ClientConfig import
APonce911 b6650ba
WIP append custom cert
APonce911 bf1735d
WIP: update tests
APonce911 da22cf9
rename TlsConfig cert attribute
APonce911 a01979a
remove comment
APonce911 d89f559
style adjustment
APonce911 04a1c3a
add example
APonce911 7011045
Code review adjustment: Use AsyncConnection::new instead of new_with_…
APonce911 deb5397
WIP: include certificates on TlsConfig struct
APonce911 5a97e80
style adjustment
APonce911 a61b1ec
make rustls_stream mod public temporarily
APonce911 8000c6c
WIP: create Certificates wrapper on rustls_stream mod
APonce911 9b4a839
WIP use custom error when appending a certificate
APonce911 8d7ff6c
WIP remove moved code
APonce911 0538906
WIP remove unused field from TlsConfig
APonce911 5fc031b
add Certificates module
APonce911 9c11211
adjust privacy on structs
APonce911 22c2b2f
add new docs
APonce911 97b5d56
update doc and example
APonce911 4a08c7d
remove comment
APonce911 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| //! This example demonstrates the client builder with custom DER certificate. | ||
| //! to run: cargo run --example custom_cert --features async-https-rustls | ||
|
|
||
| #[cfg(feature = "async")] | ||
| 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()) | ||
| } | ||
|
|
||
| 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(()) | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,64 @@ | ||
| #[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> { | ||
| let certificates = Self { inner: RootCertStore::empty() }; | ||
|
|
||
| let result = if let Some(certificate) = certificate { | ||
| certificates.append_certificate(certificate) | ||
| } else { | ||
| Ok(certificates) | ||
| }; | ||
| result | ||
| } | ||
|
|
||
| #[cfg(feature = "rustls")] | ||
| pub(crate) fn append_certificate(mut self, certificate: &Vec<u8>) -> Result<Self, Error> { | ||
| let mut certificates = self.inner; | ||
| certificates | ||
| .add(&rustls::Certificate(certificate.clone())) | ||
| .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 | ||
| } | ||
| } |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.