-
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
Closed
ton-anywhere
wants to merge
46
commits into
rust-bitcoin:master
from
ton-anywhere:bitreq-client-builder
Closed
Changes from 11 commits
Commits
Show all changes
46 commits
Select commit
Hold shift + click to select a range
13e0cfd
update test_https to use local http server
ton-anywhere e879020
add test_https_with_client
ton-anywhere 182e5fb
WIP: add ClientBuilder for configuring Client instances
ton-anywhere 92ca975
WIP: pass ClientConfig struct to tls layer
ton-anywhere 5af7539
WIP: include feature on ClientConfig import
ton-anywhere b6650ba
WIP append custom cert
ton-anywhere bf1735d
WIP: update tests
ton-anywhere da22cf9
rename TlsConfig cert attribute
ton-anywhere a01979a
remove comment
ton-anywhere d89f559
style adjustment
ton-anywhere 04a1c3a
add example
ton-anywhere 7011045
Code review adjustment: Use AsyncConnection::new instead of new_with_…
ton-anywhere deb5397
WIP: include certificates on TlsConfig struct
ton-anywhere 5a97e80
style adjustment
ton-anywhere a61b1ec
make rustls_stream mod public temporarily
ton-anywhere 8000c6c
WIP: create Certificates wrapper on rustls_stream mod
ton-anywhere 9b4a839
WIP use custom error when appending a certificate
ton-anywhere 8d7ff6c
WIP remove moved code
ton-anywhere 0538906
WIP remove unused field from TlsConfig
ton-anywhere 5fc031b
add Certificates module
ton-anywhere 9c11211
adjust privacy on structs
ton-anywhere 22c2b2f
add new docs
ton-anywhere 97b5d56
update doc and example
ton-anywhere 4a08c7d
remove comment
ton-anywhere 2cf40aa
Adjust custom_cert example feature
ton-anywhere 937e3ba
fix: correct feature flag for CustomClientConfig import
ton-anywhere e682f92
List adjustments
ton-anywhere fd47ea1
fix Cargo fmt adjustments
ton-anywhere 728ceb4
Rename `certificate` to `cert_der`
ton-anywhere c9d941d
take ownership of cert_der on append_certificate
ton-anywhere 0f67697
rename parameter cert_der on Doc for with_root_certificate
ton-anywhere 65e7c41
Reuse existing TLSConfig if possible - allows for multiple certificat…
ton-anywhere ecf4d12
Update TlsConfig::new and ClientBuilder::with_root_certificate to ret…
ton-anywhere c11c920
Update custom_cert example with new return from with_root_certificate…
ton-anywhere cc84c5c
Fix test flags
ton-anywhere abf89cf
warnings fix
ton-anywhere e8f47b6
fix: unresolved import - Refactor client mod to allow cleaner conditi…
ton-anywhere 97124a9
Gate tls modules declarations
ton-anywhere 8382cc1
Fix doctest
ton-anywhere 05f8e5d
remove connector caching with client_config - always create new conne…
ton-anywhere 2ef687e
Improve code organization: Move Client and ClientImpl declarations be…
ton-anywhere bfe2763
wrap ClientConfig with Arc smart pointer to reduce memory usage
ton-anywhere b2fe156
Merge branch 'master' into bitreq-client-builder
ton-anywhere f3851fb
use Arc clone instead of option clone
ton-anywhere 6ac03d7
Wrap Certificates with arc and inject from Client - load root_certs o…
ton-anywhere bf3a952
bump default Client default connection pool(capacity) to 10
ton-anywhere 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
Some comments aren't visible on the classic Files Changed page.
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,25 @@ | ||
| //! This example demonstrates the client builder with custom DER certificate. | ||
| // to run cargo run --example custom_cert --features async | ||
|
|
||
| #[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(); | ||
|
|
||
| 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 |
|---|---|---|
|
|
@@ -9,6 +9,8 @@ use std::io; | |
| use std::net::TcpStream; | ||
| use std::sync::OnceLock; | ||
|
|
||
| #[cfg(feature = "rustls")] | ||
| use crate::client::ClientConfig as CustomClientConfig; | ||
| #[cfg(all(feature = "native-tls", not(feature = "rustls")))] | ||
| use native_tls::{HandshakeError, TlsConnector, TlsStream}; | ||
| #[cfg(feature = "rustls")] | ||
|
|
@@ -63,6 +65,52 @@ fn build_client_config() -> Arc<ClientConfig> { | |
| Arc::new(config) | ||
| } | ||
|
|
||
| #[cfg(feature = "rustls")] | ||
| fn build_root_certificates() -> RootCertStore { | ||
|
ton-anywhere marked this conversation as resolved.
Outdated
|
||
| let mut root_certificates = RootCertStore::empty(); | ||
|
|
||
| // 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, | ||
| ) | ||
| })); | ||
| } | ||
| root_certificates | ||
| } | ||
|
|
||
| #[cfg(feature = "rustls")] | ||
| fn append_certificate(mut certificates: RootCertStore, certificate: Vec<u8>) -> RootCertStore { | ||
| match certificates.add(&rustls::Certificate(certificate)) { | ||
| Ok(_) => println!("Certificate added successfully"), | ||
| Err(e) => println!("Failed to add certificate: {:?}", e), | ||
| } | ||
| certificates | ||
| } | ||
|
|
||
| #[cfg(feature = "rustls")] | ||
| fn build_rustls_client_config(certificates: RootCertStore) -> Arc<ClientConfig> { | ||
| let config = ClientConfig::builder() | ||
| .with_safe_defaults() | ||
| .with_root_certificates(certificates) | ||
| .with_no_client_auth(); | ||
| Arc::new(config) | ||
| } | ||
|
|
||
| #[cfg(feature = "rustls")] | ||
| pub(super) fn wrap_stream(tcp: TcpStream, host: &str) -> Result<SecuredStream, Error> { | ||
| #[cfg(feature = "log")] | ||
|
|
@@ -106,6 +154,33 @@ pub(super) async fn wrap_async_stream( | |
| Ok(AsyncHttpStream::Secured(Box::new(tls))) | ||
| } | ||
|
|
||
| #[cfg(all(feature = "rustls", feature = "tokio-rustls"))] | ||
| pub(super) async fn wrap_async_stream_with_configs( | ||
|
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. same as wrap_async_stream but with additional parameter custom_client_config |
||
| tcp: AsyncTcpStream, | ||
| host: &str, | ||
| custom_client_config: CustomClientConfig, | ||
| ) -> Result<AsyncHttpStream, Error> { | ||
| #[cfg(feature = "log")] | ||
| log::trace!("Setting up TLS parameters for {host}."); | ||
| let dns_name = match ServerName::try_from(host) { | ||
| Ok(result) => result, | ||
| Err(err) => return Err(Error::IoError(io::Error::new(io::ErrorKind::Other, err))), | ||
| }; | ||
|
|
||
| let certificates = build_root_certificates(); | ||
| let custom_certificate = custom_client_config.tls.unwrap().custom_certificate; | ||
| let certificates = append_certificate(certificates, custom_certificate); | ||
| let client_config = build_rustls_client_config(certificates); | ||
| let connector = TlsConnector::from(CONFIG.get_or_init(|| client_config).clone()); | ||
|
|
||
| #[cfg(feature = "log")] | ||
| log::trace!("Establishing TLS session to {host}."); | ||
|
|
||
| let tls = connector.connect(dns_name, tcp).await.map_err(Error::IoError)?; | ||
|
|
||
| Ok(AsyncHttpStream::Secured(Box::new(tls))) | ||
| } | ||
|
|
||
| #[cfg(all(feature = "native-tls", not(feature = "rustls")))] | ||
| pub type SecuredStream = TlsStream<TcpStream>; | ||
|
|
||
|
|
||
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
Binary file not shown.
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.
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
