search

buttplugio / buttplug

Rust Implementation of the Buttplug Sex Toy Control Protocol
https://buttplug.io
Other
896 stars 65 forks source link

Remove OpenSSL? #575

Closed palfrey closed 1 year ago

palfrey commented 1 year ago

Given there's now rustls, I was wondering why there was still the use of OpenSSL here, especially given I'm seeing vendoring issues with it in Intiface. Had a quick hack locally, and I've got some changes that entirely remove OpenSSL if you're interested in a PR for that? Happy to then make the changes in Intiface to do the equivalent fixes.

Thoughts?

qdot commented 1 year ago

Oh, huh, sure, throw a PR in and I'll take a look. Thanks!

qdot commented 1 year ago

Merged, I'll close this once I get the next release out (which may be today)

qdot commented 1 year ago

Ooook well spoke a bit too soon.

Somehow, this compiles in the library, but I'm guessing that's because there's nothing in our tests that exercises the branch in a way where the tls connection is used.

When I try to depend on this up in intiface central, where we do take the tls branch, I'm getting this:

error[E0277]: the trait bound `tokio_native_tls::TlsConnector: From<Arc<ClientConfig>>` is not satisfied
   --> C:\Users\qdot\code\buttplug\buttplug\src\core\connector\transport\websocket\websocket_client.rs:144:29
    |
144 |       Some(Arc::new(config).into())
    |                             ^^^^ the trait `From<Arc<ClientConfig>>` is not implemented for `tokio_native_tls::TlsConnector`
    |
    = help: the trait `From<native_tls::TlsConnector>` is implemented for `tokio_native_tls::TlsConnector`
    = note: required for `Arc<ClientConfig>` to implement `Into<tokio_native_tls::TlsConnector>`

We're using a rustls ClientConfig in the tls block now, but async-tungstenite wants a tokio-native-tls TlsConnection, which we don't have a From impl for.

qdot commented 1 year ago

I'm down for swapping to something like tokio-tungstenite if that's a quick fix too. I have no real preference here, I chose async-tungstenite years ago just 'cause it was around and working during the early post-1.36 days.

palfrey commented 1 year ago

Argh. So, cargo why tokio-native-tls on buttplug gets you nothing. It uses rustls-native-certs, but not tokio-native-tls. Do the same command on intiface-central/intiface-engine-flutter-bridge, and you get it turning up in async-tungstenite, and that activates the code at https://github.com/sdroege/async-tungstenite/blob/c4fb3afb8a94f2c89508c45bbd09215097b315bc/src/tokio.rs#L65 which goes "huh, lets activate the other versions of the function".

https://github.com/buttplugio/buttplug/pull/577 resolves this, and simplifies the earlier changes TBH.

qdot commented 1 year ago

Oof, I really need to clean up my features, or at least make sure I build with more variations in CI. Thanks for the fix on this, I'll file myself a followup to cover more features in CI too.

qdot commented 1 year ago

Fixed in v7.1.5

qdot commented 1 year ago

Heads up, as a continuation of this work (and because I kept getting openssl vendored builds in my android builds above this that would take 15 minutes), in v7.1.12 we're moving to tokio-tungstenite and rustls-webpki-roots. Just removed OpenSSL from my upper level builds!

Thanks for the work on this @palfrey, your original commit stack was super helpful in making this change quick.