feat: add new Option<CryptoProvider> to client configuration
#138
Conversation
oleonardolima
force-pushed
the
expose-rust-cryptoprovider-to-user
branch
2 times, most recently
from
42a4352
to
2fc7c09
Compare
oleonardolima
force-pushed
the
expose-rust-cryptoprovider-to-user
branch
3 times, most recently
from
e80e4d7
to
68cef00
Compare
|
@thunderbiscuit I think the approach @LLFourn suggested would look something like this. I think some optimizations could be done on the usage of the features here :think: |
It adds a new field to the client `Config, expecting the `CryptoProvider` from the user. It uses aws-lc-rs or ring providers by default if any of these features are enabled. It's based on the suggestion comment at bitcoindevkit#135.
oleonardolima
force-pushed
the
expose-rust-cryptoprovider-to-user
branch
from
68cef00
to
25d9c6b
Compare
There was a problem hiding this comment.
This looks like it would do what was intended, but I don't feel confident enough in my grasp of rustls and the new CryptoProvider/ClientConfig builder constructs and their impact on the clients here to give a full ACK. I don't see any obvious issues with the diff however, other than it adds a layer of complexity for contributors. If a maintainer of the library would like this in I'm fine with the changes.
I don't really see users for this yet however, so it might be a case of over-engineering at the moment? Just my gut feeling.
|
Thanks for the review @thunderbiscuit! Indeed, it adds a new level of complexity for contributors and maintainers. I'm fine with not moving this one forward and closing the PR and related issues, unless the custom I think having both |
There was a problem hiding this comment.
To be clear I do want to use a custom CryptoProvider I want to try the pure rust crypto provider from RustCrypto. The PR here needs to fix the feature flags to have this make any sense though. There needs to be a feature that enables rust tls without any particular backend i.e. rustls. Then other features which enable specific backends like rustls-ring = ["rustls", "rustls/ring"]. From fixing that the CrypoProvider should have a logical default determined by whether rustls-ring or rustl-aws-lc-rs is enabled or not defaulting to one of them if they both are. If none of them are enabled and crypto_provider is None then panic.
| ))] | ||
| /// 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>( |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Hmmm I think before doing this PR we should do some refactoring. There is way too much feature gating logic to me.
- We shouldn't have a method like
new_proxy_ssl, just have a single method that starts an electrum TLS connection given aTcpStream. 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. - 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-clientrather 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)
There was a problem hiding this comment.
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.
Sweet! Thanks, I'll move forward with this then, addressing the issues you mentioned: add proper features, handle |
fixes #137