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.

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

(WIP — do not merge) Switch from native-tls to rustls #151

Closed

Conversation

8573
Copy link

@8573 8573 commented Sep 20, 2018

This is an unfinished patch to switch irc from depending on tokio-tls and native-tls, which @aatxe says "has caused a reasonable amount of annoyance", to tokio-rustls and (through it) rustls.

This patch uses the 0.8.0-alpha version of tokio-rustls, which has a tokio-tls-esque API. Even if this patch were finished, it should not be applied until that tokio-rustls API fully is released.

While irc compiles with this patch applied, the following work remains to be done:

  • Implement CertFP — rustls provides a less convenient API for client certificate authentication than does native-tls, and I (@8573) don't understand cryptographic APIs well enough to implement CertFP with it. This WIP version of my patch provides only a stub implementation of the necessary rustls trait, ResolvesClientCert, which implementation will not actually provide any client certificate to rustls, even if one is specified in the irc-level client Config.

  • Run irc's test suite with this patch applied.

  • Confirm that irc's dependents still work with this patch applied.

This is an unfinished patch to switch `irc` from depending on
[`tokio-tls`] and [`native-tls`], which @aatxe says "has caused a
reasonable amount of annoyance", to [`tokio-rustls`] and (through it)
[`rustls`].

This patch uses the 0.8.0-alpha version of `tokio-rustls`, which has a
`tokio-tls`-esque API. Even if this patch were finished, it should not
be applied until that `tokio-rustls` API fully is released.

While `irc` compiles with this patch applied, the following work
remains to be done:

- Implement CertFP — `rustls` provides a less convenient API for
  client certificate authentication than does `native-tls`, and I
  (@8573) don't understand cryptographic APIs well enough to implement
  CertFP with it. This WIP version of my patch provides only a stub
  implementation of the necessary `rustls` trait,
  [`ResolvesClientCert`], which implementation will not actually
  provide any client certificate to `rustls`, even if one is specified
  in the `irc`-level client `Config`.

- Run `irc`'s test suite with this patch applied.

- Confirm that `irc`'s dependents still work with this patch applied.

[`ResolvesClientCert`]: <https://docs.rs/rustls/0.13.1/rustls/trait.ResolvesClientCert.html>
[`native-tls`]: <https://crates.io/crates/native-tls>
[`rustls`]: <https://crates.io/crates/rustls>
[`tokio-rustls`]: <https://crates.io/crates/tokio-rustls>
[`tokio-tls`]: <https://crates.io/crates/tokio-tls>
@8573
Copy link
Author

8573 commented Sep 20, 2018

I can run irc's test suite (I assume) and check that my irc-bot still works with this patched version of irc, but I'm not sure I, being unlearned in cryptography, should try to plug sundry components together to be the one to get a working CertFP implementation.

@retep998
Copy link
Collaborator

Note that native-tls is able to avoid building any C dependencies on Windows whereas rustls would pull in ring which has to build a C library.

@DoumanAsh
Copy link
Contributor

As of 0.13 ring is pretty safe to be build on windows.

Though making feature to choose between native-tls and rustls could be convenient

@aatxe
Copy link
Owner

aatxe commented Sep 22, 2018

Just a note to clarify the "reasonable amount of annoyance" comment, it was directed mainly at OpenSSL which seems to break builds frequently, rather than native-tls in general.

@retep998
Copy link
Collaborator

@DoumanAsh Unless you're using pc-windows-gnu, in which case you'd have to setup a whole MinGW toolchain instead of using the stuff bundled with Rust.

@tirz
Copy link
Contributor

tirz commented Mar 4, 2020

Any news on this topic ?

If needed, I will be happy to fill a PR for the RustTls support.

I guess I will have to:

  • put native-tls and tokio-tls on a feature (it will allow us to compile the project without them - and will make it much easier to compile for android)
  • add a new feature with tokio-rustls and webpki-roots (and choose which one should be enabled by default)
  • then, it may be nice to isolate the ssl logic to a ssl.rs for readability
  • finally, cleanup/refactor conn.rs since we may have some code duplication

I can send all in one PR or split each task into a their own PR.

@8573
Copy link
Author

8573 commented Mar 6, 2020

@tirz: If you're familiar with the relevant cryptography, that sounds good to me. I certainly am unlikely to finish this work myself.

@tirz
Copy link
Contributor

tirz commented Mar 6, 2020

@tirz: If you're familiar with the relevant cryptography, that sounds good to me. I certainly am unlikely to finish this work myself.

I successfully implemented it on my fork: https://github.com/tirz/irc/tree/feature-ssl_backends
I will fill a PR soon.

Edit: I am just waiting for the proxy feature to be reviewed and then I will still have to implement a custom certificate selection for rustls.

@tirz tirz mentioned this pull request Mar 7, 2020
@aatxe
Copy link
Owner

aatxe commented Mar 30, 2020

gonna close this PR since we're hopefully accepting #203 (as soon as Travis goes through)

thank you for the initial work on this though, @8573

@aatxe aatxe closed this Mar 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants