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

Track the latest changes from upstream rustls and tokio-rustls #222

Closed
wants to merge 27 commits into from

Conversation

stevefan1999-personal
Copy link

This is blocked partially rustls/tokio-rustls#21 and awaiting for a while until the new rustls alpha version is released because there are some key changes (making ring optional, dynamic crypto provider). My own patch fork tracks all the latest changes.

@stevefan1999-personal stevefan1999-personal marked this pull request as ready for review September 23, 2023 11:27
@djc
Copy link
Member

djc commented Sep 23, 2023

This should also use alpha versions of rustls-native-certs, rustls-pemfile, and webpki-roots.

Cargo.toml Outdated Show resolved Hide resolved
examples/server.rs Outdated Show resolved Hide resolved
@@ -3,6 +3,9 @@ use std::sync::Arc;
use hyper::server::conn::AddrIncoming;
use rustls::ServerConfig;

#[cfg(feature = "ring")]
Copy link
Member

Choose a reason for hiding this comment

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

I don't think these ring guards make sense.

Choose a reason for hiding this comment

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

warning: unused imports: `CertificateDer`, `PrivateKeyDer`
 --> src\acceptor\builder.rs:4:17
  |
4 | use pki_types::{CertificateDer, PrivateKeyDer};
  |                 ^^^^^^^^^^^^^^  ^^^^^^^^^^^^^
  |
  = note: `#[warn(unused_imports)]` on by default

This makes sense if ring is not part of the features

Choose a reason for hiding this comment

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

Maybe I should just directly reference the type using full crate path.

Copy link
Member

Choose a reason for hiding this comment

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

IMO all of the ring guards in this module don't make sense. What errors do you get if you leave them out?

Choose a reason for hiding this comment

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

IMO all of the ring guards in this module don't make sense. What errors do you get if you leave them out?

A list of errors:

error[E0412]: cannot find type `CertificateDer` in this scope
  --> F:\.cache\cargo\registry\src\index.crates.io-6f17d22bba15001f\rustls-0.22.0-alpha.3\src\server\builder.rs:71:25
   |
71 |         cert_chain: Vec<CertificateDer<'static>>,
   |                         ^^^^^^^^^^^^^^ not found in this scope
   |
help: consider importing this struct
   |
1  + use pki_types::CertificateDer;
   |

error[E0412]: cannot find type `PrivateKeyDer` in this scope
  --> F:\.cache\cargo\registry\src\index.crates.io-6f17d22bba15001f\rustls-0.22.0-alpha.3\src\server\builder.rs:72:18
   |
72 |         key_der: PrivateKeyDer<'static>,
   |                  ^^^^^^^^^^^^^ not found in this scope
   |
help: consider importing this enum
   |
1  + use pki_types::PrivateKeyDer;
   |

error[E0412]: cannot find type `Error` in this scope
  --> F:\.cache\cargo\registry\src\index.crates.io-6f17d22bba15001f\rustls-0.22.0-alpha.3\src\server\builder.rs:73:31
   |
73 |     ) -> Result<ServerConfig, Error> {
   |                               ^^^^^ not found in this scope
   |
help: consider importing one of these items
   |
1  + use alloc::fmt::Error;
   |
1  + use core::error::Error;
   |
1  + use core::fmt::Error;
   |
1  + use crate::Error;
   |
     and 4 other candidates

error[E0412]: cannot find type `CertificateDer` in this scope
  --> F:\.cache\cargo\registry\src\index.crates.io-6f17d22bba15001f\rustls-0.22.0-alpha.3\src\server\builder.rs:89:25
   |
89 |         cert_chain: Vec<CertificateDer<'static>>,
   |                         ^^^^^^^^^^^^^^ not found in this scope
   |
help: consider importing this struct
   |
1  + use pki_types::CertificateDer;
   |

error[E0412]: cannot find type `PrivateKeyDer` in this scope
  --> F:\.cache\cargo\registry\src\index.crates.io-6f17d22bba15001f\rustls-0.22.0-alpha.3\src\server\builder.rs:90:18
   |
90 |         key_der: PrivateKeyDer<'static>,
   |                  ^^^^^^^^^^^^^ not found in this scope
   |
help: consider importing this enum
   |
1  + use pki_types::PrivateKeyDer;
   |

error[E0412]: cannot find type `Error` in this scope
  --> F:\.cache\cargo\registry\src\index.crates.io-6f17d22bba15001f\rustls-0.22.0-alpha.3\src\server\builder.rs:92:31
   |
92 |     ) -> Result<ServerConfig, Error> {
   |                               ^^^^^ not found in this scope
   |
help: consider importing one of these items
   |
1  + use alloc::fmt::Error;
   |
1  + use core::error::Error;
   |
1  + use core::fmt::Error;
   |
1  + use crate::Error;
   |
     and 4 other candidates

Copy link
Author

Choose a reason for hiding this comment

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

Oh I reckon, this is the problem:

    #[cfg(feature = "ring")]
    /// Create a builder for a client configuration with the default
    /// [`CryptoProvider`]: [`crate::crypto::ring::RING`].
    ///
    /// For more information, see the [`ConfigBuilder`] documentation.
    pub fn builder() -> ConfigBuilder<Self, WantsCipherSuites> {
        Self::builder_with_provider(crate::crypto::ring::RING)
    }

Should we break the ABI instead (i.e. rename this to builder_with_ring)

Choose a reason for hiding this comment

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

Yep, guess that would do. But I think the bigger problem is that rustls::ClientConfig was not liberal enough as a builder...

Copy link
Author

Choose a reason for hiding this comment

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

Maybe we should further remove ring as part of the default features...in exchange for a chaos on the crates that depends on hyper-rustls, but this would definitely promote a good use for custom crypto suites provider.

Copy link
Member

@djc djc Nov 16, 2023

Choose a reason for hiding this comment

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

No, we should mirror the API as it's setup in rustls alpha.4, which uses builder() with ring as a default and offers an alternative option to select a different provider.

(And note that most of those errors have nothing to do with ring, which is why they shouldn't be guarded with it.)

Choose a reason for hiding this comment

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

Acknowledged. Maybe this is the best we could do for now.

examples/client.rs Outdated Show resolved Hide resolved
examples/server.rs Outdated Show resolved Hide resolved
examples/server.rs Outdated Show resolved Hide resolved
examples/server.rs Outdated Show resolved Hide resolved
examples/server.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@cpu cpu mentioned this pull request Nov 17, 2023
15 tasks
@djc
Copy link
Member

djc commented Nov 24, 2023

I did an alternative to this in #233.

@cpu
Copy link
Member

cpu commented Nov 27, 2023

Since #233 landed I think we can close this one. Thanks for your contribution!

@cpu cpu closed this Nov 27, 2023
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

3 participants