Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: add new Option<CryptoProvider> to client configuration #138

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

feat: add new Option<CryptoProvider> to client configuration #138

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions src/client.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,13 +110,36 @@ impl ClientType {
pub fn from_config(url: &str, config: &Config) -> Result<Self, Error> {
if url.starts_with("ssl://") {
let url = url.replacen("ssl://", "", 1);

#[cfg(all(
any(feature = "use-rustls", feature = "use-rustls-ring"),
not(feature = "use-openssl")
))]
let client = match config.socks5() {
Some(socks5) => RawClient::new_proxy_ssl(
url.as_str(),
config.validate_domain(),
socks5,
config.timeout(),
config.crypto_provider(),
)?,
None => RawClient::new_ssl(
url.as_str(),
config.validate_domain(),
config.timeout(),
config.crypto_provider(),
)?,
};

#[cfg(feature = "openssl")]
let client = match config.socks5() {
Some(socks5) => RawClient::new_proxy_ssl(
url.as_str(),
config.validate_domain(),
socks5,
config.timeout(),
)?,

None => {
RawClient::new_ssl(url.as_str(), config.validate_domain(), config.timeout())?
}
Expand Down
23 changes: 23 additions & 0 deletions src/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
use std::time::Duration;

#[cfg(any(feature = "use-rustls", feature = "use-rustls-ring"))]
use rustls::crypto::CryptoProvider;

/// Configuration for an electrum client
///
/// Refer to [`Client::from_config`] and [`ClientType::from_config`].
Expand All @@ -12,6 +15,9 @@ pub struct Config {
socks5: Option<Socks5Config>,
/// timeout in seconds, default None (depends on TcpStream default)
timeout: Option<Duration>,
/// An optional [`CryptoProvider`] for users that don't want either default `aws-lc-rs` or `ring` providers
#[cfg(any(feature = "use-rustls", feature = "use-rustls-ring"))]
crypto_provider: Option<CryptoProvider>,
/// number of retry if any error, default 1
retry: u8,
/// when ssl, validate the domain, default true
Expand Down Expand Up @@ -60,6 +66,13 @@ impl ConfigBuilder {
self
}

/// Sets the custom [`CryptoProvider`].
#[cfg(any(feature = "use-rustls", feature = "use-rustls-ring"))]
pub fn crypto_provider(mut self, crypto_provider: Option<CryptoProvider>) -> Self {
self.config.crypto_provider = crypto_provider;
self
}

/// Sets the retry attempts number
pub fn retry(mut self, retry: u8) -> Self {
self.config.retry = retry;
Expand Down Expand Up @@ -135,6 +148,14 @@ impl Config {
pub fn builder() -> ConfigBuilder {
ConfigBuilder::new()
}

/// Get the configuration for `crypto_provider`
///
/// Set this with [`ConfigBuilder::crypto_provider`]
#[cfg(any(feature = "use-rustls", feature = "use-rustls-ring"))]
pub fn crypto_provider(&self) -> Option<&CryptoProvider> {
self.crypto_provider.as_ref()
}
}

impl Default for Config {
Expand All @@ -144,6 +165,8 @@ impl Default for Config {
timeout: None,
retry: 1,
validate_domain: true,
#[cfg(any(feature = "use-rustls", feature = "use-rustls-ring"))]
crypto_provider: None,
}
}
}
94 changes: 82 additions & 12 deletions src/raw_client.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,14 +23,11 @@ use bitcoin::{Script, Txid};
use openssl::ssl::{SslConnector, SslMethod, SslStream, SslVerifyMode};

#[cfg(all(
any(
feature = "default",
feature = "use-rustls",
feature = "use-rustls-ring"
),
any(feature = "use-rustls", feature = "use-rustls-ring"),
not(feature = "use-openssl")
))]
use rustls::{
crypto::CryptoProvider,
pki_types::ServerName,
pki_types::{Der, TrustAnchor},
ClientConfig, ClientConnection, RootCertStore, StreamOwned,
Expand Down Expand Up @@ -368,7 +365,13 @@ impl RawClient<ElectrumSslStream> {
socket_addrs: A,
validate_domain: bool,
timeout: Option<Duration>,
crypto_provider: Option<&CryptoProvider>,
) -> Result<Self, Error> {
#[cfg(feature = "use-rustls")]
use rustls::crypto::aws_lc_rs::default_provider;
#[cfg(feature = "use-rustls-ring")]
use rustls::crypto::ring::default_provider;

debug!(
"new_ssl socket_addrs.domain():{:?} validate_domain:{} timeout:{:?}",
socket_addrs.domain(),
Expand All @@ -378,16 +381,27 @@ impl RawClient<ElectrumSslStream> {
if validate_domain {
socket_addrs.domain().ok_or(Error::MissingDomain)?;
}

let crypto_provider = match crypto_provider {
Some(provider) => provider.to_owned(),

#[cfg(feature = "use-rustls")]
None => default_provider(),

#[cfg(feature = "use-rustls-ring")]
None => default_provider(),
};

match timeout {
Some(timeout) => {
let stream = connect_with_total_timeout(socket_addrs.clone(), timeout)?;
stream.set_read_timeout(Some(timeout))?;
stream.set_write_timeout(Some(timeout))?;
Self::new_ssl_from_stream(socket_addrs, validate_domain, stream)
Self::new_ssl_from_stream(socket_addrs, validate_domain, stream, crypto_provider)
}
None => {
let stream = TcpStream::connect(socket_addrs.clone())?;
Self::new_ssl_from_stream(socket_addrs, validate_domain, stream)
Self::new_ssl_from_stream(socket_addrs, validate_domain, stream, crypto_provider)
}
}
}
Expand All @@ -397,10 +411,13 @@ impl RawClient<ElectrumSslStream> {
socket_addr: A,
validate_domain: bool,
tcp_stream: TcpStream,
crypto_provider: CryptoProvider,
) -> Result<Self, Error> {
use std::convert::TryFrom;

let builder = ClientConfig::builder();
let builder = ClientConfig::builder_with_provider(crypto_provider.into())
.with_safe_default_protocol_versions()
.map_err(Error::CouldNotBuildWithSafeDefaultVersion)?;

let config = if validate_domain {
socket_addr.domain().ok_or(Error::MissingDomain)?;
Expand Down Expand Up @@ -441,6 +458,7 @@ impl RawClient<ElectrumSslStream> {
#[cfg(any(feature = "default", feature = "proxy"))]
/// Transport type used to establish a connection to a server through a socks proxy
pub type ElectrumProxyStream = Socks5Stream;

#[cfg(any(feature = "default", feature = "proxy"))]
impl RawClient<ElectrumProxyStream> {
/// Creates a new socks client and tries to connect to `target_addr` using `proxy_addr` as a
Expand All @@ -467,14 +485,66 @@ impl RawClient<ElectrumProxyStream> {
Ok(stream.into())
}

#[cfg(any(
feature = "use-openssl",
feature = "use-rustls",
feature = "use-rustls-ring"
#[cfg(all(
any(
feature = "default",
feature = "use-rustls",
feature = "use-rustls-ring"
),
not(feature = "use-openssl")
))]
/// Creates a new TLS client that connects to `target_addr` using `proxy_addr` as a socks proxy
/// server. The DNS resolution of `target_addr`, if required, is done through the proxy. This
/// allows to specify, for instance, `.onion` addresses.
pub fn new_proxy_ssl<T: ToTargetAddr>(
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this adding a new feature where you can use a proxy with rustls? I'm a little confused why this is being added in this PR.

Copy link
Collaborator Author

@oleonardolima oleonardolima Sep 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did it because CryptoProvider is specific to rustls, so I had to split it into rustls and openssl feature-gated fns, in order to correctly expect the crypto_provider parameter.

Copy link

@LLFourn LLFourn Sep 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm I think before doing this PR we should do some refactoring. There is way too much feature gating logic to me.

  1. We shouldn't have a method like new_proxy_ssl, just have a single method that starts an electrum TLS connection given a TcpStream. Of course then we have a convenience method to do the initial connection to a url string if they don't want proxies. But if they do then it's fine to have them set up the proxy connection themselves.
  2. Do we even need openssl as a dependency at all? If someone wants to use socks5 then can depend on openssl themelves or use one of the many pure rust socks5 client libraries like socks5-client. We can have an example showing how to do tor. IMO it'd be better to try and use socks5-client rather than openssl. For TLS we can just be opinionated and use rustls.

This seems like something you would want to do anyway @oleonardolima for arti?

I'll turn this into an issue that I think we should tackle first. (here: #146)

Copy link
Collaborator Author

@oleonardolima oleonardolima Sep 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I do agree with both (1) and (2), there are also issues with other features such s proxy previously mentioned by tnull, refer to #91, which is basically mandatory and we can't use the client with --no-default-features.

Indeed, with arti-client I do need the client to receive a data stream (e.g. TcpStream, arti-client::DataStream), and was planning on such refactoring. It also requires being async, which detangling all existing features will make it easy to have an async version.

Thanks for opening the issue, I'll move this PR back to draft for now.

PS I also commented on the issue, for anyone else who didn't look at this PR.

target_addr: T,
validate_domain: bool,
proxy: &crate::Socks5Config,
timeout: Option<Duration>,
crypto_provider: Option<&CryptoProvider>,
) -> Result<RawClient<ElectrumSslStream>, Error> {
#[cfg(feature = "use-rustls")]
use rustls::crypto::aws_lc_rs::default_provider;
#[cfg(feature = "use-rustls-ring")]
use rustls::crypto::ring::default_provider;

let target = target_addr.to_target_addr()?;

let mut stream = match proxy.credentials.as_ref() {
Some(cred) => Socks5Stream::connect_with_password(
&proxy.addr,
target_addr,
&cred.username,
&cred.password,
timeout,
)?,
None => Socks5Stream::connect(&proxy.addr, target.clone(), timeout)?,
};
stream.get_mut().set_read_timeout(timeout)?;
stream.get_mut().set_write_timeout(timeout)?;

let crypto_provider = match crypto_provider {
Some(provider) => provider.to_owned(),

#[cfg(feature = "use-rustls")]
None => default_provider(),

#[cfg(feature = "use-rustls-ring")]
None => default_provider(),
};

RawClient::new_ssl_from_stream(
target,
validate_domain,
stream.into_inner(),
crypto_provider,
)
}

#[cfg(feature = "use-openssl")]
/// Creates a new TLS client that connects to `target_addr` using `proxy_addr` as a socks proxy
/// server. The DNS resolution of `target_addr`, if required, is done through the proxy. This
/// allows to specify, for instance, `.onion` addresses.
pub fn new_proxy_ssl<T: ToTargetAddr>(
target_addr: T,
validate_domain: bool,
Expand Down
5 changes: 5 additions & 0 deletions src/types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -318,6 +318,9 @@ pub enum Error {
#[cfg(any(feature = "use-rustls", feature = "use-rustls-ring"))]
/// Could not create a rustls client connection
CouldNotCreateConnection(rustls::Error),
#[cfg(any(feature = "use-rustls", feature = "use-rustls-ring"))]
/// Could not create the `ClientConfig` with safe default protocol version
CouldNotBuildWithSafeDefaultVersion(rustls::Error),

#[cfg(feature = "use-openssl")]
/// Invalid OpenSSL method used
Expand Down Expand Up @@ -365,6 +368,8 @@ impl Display for Error {
Error::MissingDomain => f.write_str("Missing domain while it was explicitly asked to validate it"),
Error::CouldntLockReader => f.write_str("Couldn't take a lock on the reader mutex. This means that there's already another reader thread is running"),
Error::Mpsc => f.write_str("Broken IPC communication channel: the other thread probably has exited"),
#[cfg(any(feature = "use-rustls", feature = "use-rustls-ring"))]
Error::CouldNotBuildWithSafeDefaultVersion(_) => f.write_str("Couldn't build the `ClientConfig` with safe default version"),
}
}
}
Expand Down
Loading