Rustls support #1190
Open
Rustls support #1190
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ConnectionKeeper is no longer there.
The impl of Default for PoolConfig is only useful for tests. Co-authored-by: Karol Baryła <[email protected]>
It was only passed around back and forth, but unused apart from that. The keepalive interval that is used in keepaliver comes from ConnectionConfig, not PoolConfig.
This updates openssl to its latest available patch release. The version that we required had some security vulnerabilities reported.
As openssl crate is still before its 1.0 (stable) release, we need to prepare for its possible introduces breaking changes. As it's present in our public API, we hide it behind a versioned feature flag. This way, if in the future openssl is released in 0.11 or 1.0 version, we'll simply add the new feature flag and gradually deprecate the old one. As a side note, openssl crate hasn't issued a major release for 7 years now, so it's plausible that it's not going to happen (at least soon). Co-authored-by: Wojciech Przytuła <[email protected]>
wprzytula
force-pushed
the
rustls-support
branch
from
26473ca to
148cabf
Compare
|
See the following report for details: cargo semver-checks output |
github-actions
bot
added
the
semver-checks-breaking
This allows for future TLS providers to be added, including rustls. Co-authored-by: Wojciech Przytuła <[email protected]>
otherwise the error shows up too late in a non-terminal way whereas the error is terminal
The idea is that `{Connection,Pool}Config` is the same for all
connections, whereas `Host{Connection,Pool}Config` is customized for
a specific endpoint. This commit leverages type system to better convey
this difference.
This step improves types safety and prepares for next steps.
This just changes the style from `if let` to `let else` for better readability. This commit should be viewed without whitespace difference.
…nfig It makes great sense, because the CloudConfig is the subject of the operation.
TlsProvider is the last abstraction about TLS in the driver.
The full picture looks like this:
┌─←─ TlsContext (openssl::SslContext / rustls::ClientConfig)
│
├─←─ CloudConfig (powered by either TLS backend)
│
│ gets wrapped in
│
↳ TlsProvider (same for all connections)
│
│ produces
│
↳ TlsConfig (specific for the particular connection)
│
│ produces
│
↳ Tls (wrapper over TCP stream which adds encryption)
As HostPoolConfig is created by cloning required fields of PoolConfig anyway, there's no point in taking PoolConfig by move.
Before, PoolConfig would be re-created for each attempt to recreate the control connection pool. It's redundant, because each time it would be exactly the same, and it requires cloning ConnectionConfig. Now, PoolConfig is only created once and stored in MetadataReader. Before, only ConnectionConfig would be stored.
It seems that when I originally wrote serverless, for some reason I hand-crafted address translation logic specifically for serverless, making it fully separate from the standard AddressTranslator trait. In this commit I successfully implement cloud address translation inside the standard AddressTranslator framework, decluttering PoolRefiller logic from cloud-related code. CloudConfig becomes an AddressTranslator itself, so it needn't be passed separately anymore. This further reduces code complication arising from cloud-related codepaths.
It's no longer necessary, as CloudConfig is now passed inside TlsProvider.
If in the future we choose to store data differently in UntranslatedPeer, we will now have freedom to do so, only preserving the getters. Otherwise, we would have to retain legacy fields in such case.
As address translation should not need ownership of neither datacenter nor rack, let's not require cloning them.
wprzytula
force-pushed
the
rustls-support
branch
2 times, most recently
from
dbaba83 to
a07efdc
Compare
Rustls is now supported as ordinary TLS provider, for far only for the non-cloud case. Co-authored-by: Wojciech Przytuła <[email protected]>
rustls is now supported in cloud, too. The choice between rustls and openssl is done based on enabled features: - if openssl-010 feature is enabled, openssl is used for cloud; - else if rustls-023 feature is enabled, rustls is used for cloud; - else compile error is raised. It is subject to discussion in the PR is such logic based on features is acceptable. Co-authored-by: Andres Medina <[email protected]>
Co-authored-by: Wojciech Przytuła <[email protected]>
Co-authored-by: Wojciech Przytuła <[email protected]>
This is analogous to tls-rustls.rs example.
As address translation no longer needs owned Strings, it's enough to pass references to UntranslatedEndpoint to `open_connection` & friends. Note that, unfortunately, we can't fully elide the clone in `start_opening_connection()`, because the endpoint may be mutated (in the shard-aware case) for the purpose of opening a connection to the shard-aware port. Nonetheless, now the limitation that requires us to clone is in internal code, not in the user-facing API, which is better for us.
As the Serverless Cloud is a highly unstable feature, it must be marked as such in order not to break API after 1.0 in case we need to introduce breaking changes to the feature.
As the tls module got quite large, it makes sense to extract it to the network supermodule.
wprzytula
force-pushed
the
rustls-support
branch
from
a07efdc to
a59b80d
Compare
muzarski
reviewed
Feb 3, 2025
There was a problem hiding this comment.
I noticed that we do not do cargo checks when unstable-cloud and rustls-x features are enabled. I think we should do that. Probably should check the following combinations:
openssl-x,rustls-x,unstable-cloudopenssl-x,unstable-cloud,rustls-x,unstable-cloudopenssl-x,rustls-xopenssl-x(done intls.yml, but it currently does not work)rustls-x(makes sense to put it intls.ymlonce it's fixed )
Also, previously we would run a tls example in tls.yml. We should add a step to run tls-rustls example as well (once workflow is fixed).
scylla/src/network/connection.rs
Outdated
Comment on lines
271
to
276
| #[cfg(feature = "openssl-010")] | ||
| impl From<openssl::error::ErrorStack> for TlsError { | ||
| fn from(error: openssl::error::ErrorStack) -> Self { | ||
| TlsError::OpenSsl010(error) | ||
| } | ||
| } |
There was a problem hiding this comment.
Why not use #[from] attribute?
There was a problem hiding this comment.
I took this code from @nrxus; will fix this.
| @@ -369,7 +460,7 @@ pub(crate) struct ConnectionConfig { | |||
| pub(crate) tcp_keepalive_interval: Option<Duration>, | |||
| pub(crate) timestamp_generator: Option<Arc<dyn TimestampGenerator>>, | |||
| #[cfg(feature = "__tls")] | |||
| pub(crate) tls_config: Option<TlsConfig>, | |||
| pub(crate) tls_provider: Option<TlsProvider>, | |||
There was a problem hiding this comment.
Ok, now I understand the difference between HostConnectionConfig and ConnectionConfig.
8 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No cover letter, because GitHub ate it.The Big Picture
Before
Before,
opensslwas the only supported TLS backend. (therefore, all TLS abstractions in the driver use theSslprefix). Written in C, it's a bit cumbersome to enable in the driver: one needs to installopenssllibrary using the system package manager.There were two distinct cases:
SSL abstractions before
After
The main goal of this PR is to add support for another TLS backend:
rustls, which is written in Rust and hence easier to include in the project as a usual Rust dependency. As a bonus, its authors claims it's faster thanopenssl.As we're going to support more than one TLS backend, we need to create abstractions that hide the implementation details, including which backend is used.
Ssl*abstractions specific foropenssl(e.g.,SslContext), toTls*(e.g.,TlsContext), which are made "generic" over the backend (actually, they are enums with#[cfg(feature)]-activated backend-specific variants).Now, the two cases (global context and cloud) can both be done with
opensslorrustlsas a backend, so there are 4 combinations in total. As all combinations should be supported, new abstractions were designed to encapsulate the exact case and backend used. Thanks to that, the code is not only not more complicated after this PR, but it's actually less complicated! Comparison of the following schema with the previous one should convince you.TLS abstractions after
The Details
Features
"__tls", which gates the commonTls*abstractions.Rationale:
As thoseTls*are enums that are empty (and this is problematic) when no TLS backend is enabled, an explicit compile error is raised in such scenario. Users should not enable this feature on their own; it's a dependency of all TLS-dependant features."openssl"->"openssl-010"(Remove types from pre-1.0 crates from the public API, or hide them behind features #771)"rustls-023"(Remove types from pre-1.0 crates from the public API, or hide them behind features #771)"cloud"->"unstable-cloud"Rationale:
The Serverless Cloud is by no means a stable feature and it's likely that we'll have to break its API in the future.rustlssupportGlobal TLS context case
SessionBuildernow acceptsOption<impl Into<TlsContext>>, allowing passing bothopenssl::SslContextandrustls::ClientConfig.Sessions that differ by the TLS backend use, which may be handy for comparative benchmarks between the backends and choosing the best fit.Serverless Cloud
Which backend is employed is now chosen based on the set of enabled features:
"openssl-010"feature is enabled,opensslis used for cloud;"rustls-023"feature is enabled,rustlsis used for cloud;Miscellaneous
Newly-introduced
Host*ConfigAs @Lorak-mmk correctly observed, we could leverage the type system to distinguish global pool/connection config from one that is already made specific for a particular endpoint. This way we avoid non-obvious mutation that happened in ConnectionConfig before this PR in the cloud case.
Host{Pool,Connection}Configis introduced to bring type safety and hide the necessary adjustments done in the cloud case (setting up SNI for a particular node) in theConnectionConfig->HostConnectionConfigconversion. In the future, there might be more differences between*ConfigandHost*Config, which will further justify this duplication.Refactored Serverless address translation
Use AddressTranslator instead of hand-crafted logic for serverless.
It seems that when I originally wrote serverless, for some reason I hand-crafted address translation logic specifically for serverless, making it fully separate from the standard
AddressTranslatortrait.In this PR I successfully implement cloud address translation inside the standard
AddressTranslatorframework, declutteringPoolRefillerlogic from cloud-related code.CloudConfigbecomes anAddressTranslatoritself, so it needn't be passed separately anymore. This further reduces code complication arising from cloud-related codepaths.Changes to
UntranslatedEndpointUntranslatedEndpointnow:#[non_exhaustive]public fields before) - for more flexibility in the future;&strinstead ofString, in order to elide allocations upon translation.UntranslatedEndpointis constructed right before translation fromPeerEndpoint, so now we avoidStringallocation.Bumped
opensslversionOlder releases miss important security patches.
Other fixes
Minor ones, you'll see them yourselves in the commits.
Credits
Special thanks to @nrxus, who created a PR that I based mine on.
Thanks to @Lorak-mmk and @nemosupremo for ideas and code inspiration.
Fixes: #293
Closes: #1182
Closes: #911
Pre-review checklist
./docs/source/.Fixes:annotations to PR description.