Rustls support with dynamic choice of TLS provider for cloud #1254

muzarski · 2025-02-19T16:49:31Z

Bases on #1190
Closes: #1190
Fixes: #771

What changes compared to the base PR:

__tls feature is removed
TLS provider for the cloud is not statically predetermined based on enabled feature set. Users are now able to choose the provider in runtime via CloudSessionBuilder::new().
renamed cloud.rs example to cloud-openssl.rs. Introduced analogous cloud-rustls.rs example.

Pre-review checklist

I have split my patch into logically separate commits.
All commit messages clearly explain what they change and why.
I added relevant tests for new features and bug fixes.
All commits compile, pass static checks and pass test.
PR description sums up the changes and reasons why they should be introduced.
I have provided docstrings for the public items that I want to introduce.
I have adjusted the documentation in ./docs/source/.
I added appropriate Fixes: annotations to PR description.

github-actions · 2025-02-19T16:55:31Z

cargo semver-checks detected some API incompatibilities in this PR.
Checked commit: bf62952

See the following report for details:

cargo semver-checks output

./scripts/semver-checks.sh --baseline-rev 370506ec442f54849c92373bd2f1fa31ba4196c2
+ cargo semver-checks -p scylla -p scylla-cql --baseline-rev 370506ec442f54849c92373bd2f1fa31ba4196c2
     Cloning 370506ec442f54849c92373bd2f1fa31ba4196c2
    Building scylla v0.15.0 (current)
       Built [  34.821s] (current)
     Parsing scylla v0.15.0 (current)
      Parsed [   0.050s] (current)
    Building scylla v0.15.0 (baseline)
       Built [  21.895s] (baseline)
     Parsing scylla v0.15.0 (baseline)
      Parsed [   0.050s] (baseline)
    Checking scylla v0.15.0 -> v0.15.0 (no change)
     Checked [   0.122s] 127 checks: 119 pass, 8 fail, 0 warn, 0 skip

--- failure auto_trait_impl_removed: auto trait no longer implemented ---

Description:
A public type has stopped implementing one or more auto traits. This can break downstream code that depends on the traits being implemented.
        ref: https://doc.rust-lang.org/reference/special-types-and-traits.html#auto-traits
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.39.0/src/lints/auto_trait_impl_removed.ron

Failed in:
  type TranslationError is no longer UnwindSafe, in /home/runner/work/scylla-rust-driver/scylla-rust-driver/scylla/src/errors.rs:534
  type TranslationError is no longer RefUnwindSafe, in /home/runner/work/scylla-rust-driver/scylla-rust-driver/scylla/src/errors.rs:534

--- failure enum_missing: pub enum removed or renamed ---

Description:
A publicly-visible enum cannot be imported by its prior path. A `pub use` may have been removed, or the enum itself may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.39.0/src/lints/enum_missing.ron

Failed in:
  enum scylla::cloud::CloudConfigError, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-370506ec442f54849c92373bd2f1fa31ba4196c2/3b41bd21bae523765ebd398bd1e6895e32f39d05/scylla/src/cloud/config.rs:12
  enum scylla::client::session_builder::CloudMode, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-370506ec442f54849c92373bd2f1fa31ba4196c2/3b41bd21bae523765ebd398bd1e6895e32f39d05/scylla/src/client/session_builder.rs:46

--- failure feature_missing: package feature removed or renamed ---

Description:
A feature has been removed from this package's Cargo.toml. This will break downstream crates which enable that feature.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#cargo-feature-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.39.0/src/lints/feature_missing.ron

Failed in:
  feature cloud in the package's Cargo.toml
  feature ssl in the package's Cargo.toml

--- failure inherent_method_missing: pub method removed or renamed ---

Description:
A publicly-visible method or associated fn is no longer available under its prior name. It may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.39.0/src/lints/inherent_method_missing.ron

Failed in:
  GenericSessionBuilder::ssl_context, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-370506ec442f54849c92373bd2f1fa31ba4196c2/3b41bd21bae523765ebd398bd1e6895e32f39d05/scylla/src/client/session_builder.rs:354

--- failure method_parameter_count_changed: pub method parameter count changed ---

Description:
A publicly-visible method now takes a different number of parameters.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#fn-change-arity
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.39.0/src/lints/method_parameter_count_changed.ron

Failed in:
  scylla::client::session_builder::GenericSessionBuilder::new now takes 0 parameters instead of 1, in /home/runner/work/scylla-rust-driver/scylla-rust-driver/scylla/src/client/session_builder.rs:84

--- failure module_missing: pub module removed or renamed ---

Description:
A publicly-visible module cannot be imported by its prior path. A `pub use` may have been removed, or the module may have been renamed, removed, or made non-public.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.39.0/src/lints/module_missing.ron

Failed in:
  mod scylla::cloud, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-370506ec442f54849c92373bd2f1fa31ba4196c2/3b41bd21bae523765ebd398bd1e6895e32f39d05/scylla/src/cloud/mod.rs:1

--- failure struct_missing: pub struct removed or renamed ---

Description:
A publicly-visible struct cannot be imported by its prior path. A `pub use` may have been removed, or the struct itself may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.39.0/src/lints/struct_missing.ron

Failed in:
  struct scylla::cluster::metadata::UntranslatedPeer, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-370506ec442f54849c92373bd2f1fa31ba4196c2/3b41bd21bae523765ebd398bd1e6895e32f39d05/scylla/src/cluster/metadata.rs:175
  struct scylla::cloud::CloudConfig, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-370506ec442f54849c92373bd2f1fa31ba4196c2/3b41bd21bae523765ebd398bd1e6895e32f39d05/scylla/src/cloud/config.rs:33

--- failure struct_pub_field_missing: pub struct's pub field removed or renamed ---

Description:
A publicly-visible struct has at least one public field that is no longer available under its prior name. It may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.39.0/src/lints/struct_pub_field_missing.ron

Failed in:
  field ssl_context of struct SessionConfig, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-370506ec442f54849c92373bd2f1fa31ba4196c2/3b41bd21bae523765ebd398bd1e6895e32f39d05/scylla/src/client/session.rs:123
  field cloud_config of struct SessionConfig, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-370506ec442f54849c92373bd2f1fa31ba4196c2/3b41bd21bae523765ebd398bd1e6895e32f39d05/scylla/src/client/session.rs:184

     Summary semver requires new major version: 8 major and 0 minor checks failed
    Finished [  58.188s] scylla
    Building scylla-cql v0.4.0 (current)
       Built [  10.818s] (current)
     Parsing scylla-cql v0.4.0 (current)
      Parsed [   0.029s] (current)
    Building scylla-cql v0.4.0 (baseline)
       Built [  10.999s] (baseline)
     Parsing scylla-cql v0.4.0 (baseline)
      Parsed [   0.028s] (baseline)
    Checking scylla-cql v0.4.0 -> v0.4.0 (no change)
     Checked [   0.114s] 127 checks: 127 pass, 0 skip
     Summary no semver update required
    Finished [  22.655s] scylla-cql
make: *** [Makefile:61: semver-rev] Error 1

Lorak-mmk

Looks nice!
I wonder if for 2.0/3.0 it would make more sense to use (sealed) traits for TLS-related data instead of enums.
It should reduce amount of conditional compilation flags, but at the same time worsen the discoverability in docs.

scylla/Cargo.toml

scylla/src/client/session.rs

scylla/src/cloud/mod.rs

scylla/src/network/connection.rs

scylla/src/client/session.rs

Lorak-mmk · 2025-02-21T19:57:56Z

scylla/src/cloud/config.rs

+                    let supported = builder.crypto_provider().signature_verification_algorithms;
+                    builder
+                        .dangerous()
+                        .with_custom_certificate_verifier(Arc::new(NoCertificateVerification {
+                            supported,
+                        }))


I assume that the NoCertificateVerification behaves the same as SslVerifyMode::NONE for openssl? It doesn't disable more verifications than openssl?

I'd need to investigate to answer that. Maybe @wprzytula knows something about it.

I know nothing more in this regard. This is work of @nrxus.

This is not really important for now anyway, right? Its just cloud, which is behind an unstable feature.
We will need to verify that before releasing it officially though.

the intention is certainly to be the same as SSlVerifyMode::NONE which as far as I know just disables all verification of the certificate provided by the server so I added a custom certificate verifier that always returns Ok. I think I based this off: rustls/rustls#578 (comment)

docs/source/connecting/connecting.md

scylla/src/cloud/config.rs

muzarski · 2025-02-24T12:57:06Z

Rebased on main (clippy fixes)

muzarski · 2025-02-24T14:11:18Z

v1.1: Addressed @Lorak-mmk comments:

extracted TlsInfo::get_dc_tls_context() method
using if let ... syntax in TlsCert::[openssl/rustls]_ca method.

muzarski · 2025-02-24T14:19:18Z

* using `if let ...` syntax in `TlsCert::[openssl/rustls]_ca` method.

Ehhh... Now when we enable unstable-cloud and only one of openss-x/rustls-x, the compiler produces following warning:

 error: irrefutable `if let` pattern
   --> scylla/src/cloud/config.rs:331:12
    |
331 |         if let TlsCert::OpenSsl010(ca) = self {
    |            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: this pattern will always match, so the `if let` is useless
    = help: consider replacing the `if let` with a `let`
    = note: `-D irrefutable-let-patterns` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(irrefutable_let_patterns)]`

muzarski · 2025-02-24T14:20:50Z

Is adding #[allow(irrefutable_let_patterns)] acceptable here? @Lorak-mmk

Lorak-mmk · 2025-02-24T14:25:00Z

Is adding #[allow(irrefutable_let_patterns)] acceptable here? @Lorak-mmk

Yes

muzarski · 2025-02-24T14:30:41Z

v1.2: Silenced irrefutable_let_patterns compiler warnings emitted when cloud::config::TlsCert consists of only one variant.

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.

`UntranslatedPeer` is only used for address translation purposes and is not part of cluster metadata tables, so it makes sense for it to reside alongside `AddressTranslator`.

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. Currently, 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. The next commit introduces the runtime choice for cloud TLS provider. Co-authored-by: Andres Medina <[email protected]>

New types: - public CloudTlsProvider - It's a simple enum that allows the users to choose the provider dynamically (not statically based on the enabled features). It's used in public API in `CloudSessionBuilder::new()` where its instance needs to be passed alongside path to the cloud configuration file. - private TlsCert - an enum that holds the certificate data parsed by corresponding tls provider library. It's currently used only in `Datacenter` struct. cloud unit tests: - I deduplicated common test logic, and introduced analogous tests for rustls. cloud integration tests: - After this commit, nothing changes in this matter. I decided to leave the tests using the openssl backend (see `create_new_session_builder` in test utils). In the future, we might introduce the tests for rustls as well. Docs: - I updated the connecting.md to reflect the changes in the public API.

Co-authored-by: Wojciech Przytuła <[email protected]>

Because soon we will also have a tls-rustls.rs example.

Co-authored-by: Wojciech Przytuła <[email protected]>

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.

The number of feature checks keeps increasing, so we need to impose some order on them or else we will regret.

More cargo checks are added to the CI that try various combinations of TLS and cloud related features.

muzarski · 2025-02-25T18:14:10Z

@muzarski Please fix the conflict and we can merge!

I'm awaiting the day we will be able to bump MSRV to a version that supports resolver v3 and drop Cargo.lock.msrv. About 6 months left.

@Lorak-mmk resolved the conflicts. CI passed

Lorak-mmk · 2025-02-25T18:16:25Z

Doesn't this PR also close #771 ?

muzarski · 2025-02-25T18:17:26Z

Doesn't this PR also close #771 ?

After reading the discussion from the issue, I believe that this PR closes it.

Lorak-mmk · 2025-02-25T18:18:12Z

Ok, please add it to the PR description so that it is closed when I merge this.

muzarski · 2025-02-25T18:18:27Z

Ok, please add it to the PR description so that it is closed when I merge this.

done

github-actions bot added the semver-checks-breaking cargo-semver-checks reports that this PR introduces breaking API changes label Feb 19, 2025

muzarski force-pushed the rustls-support-muzarski branch 7 times, most recently from e17e4e7 to 666879a Compare February 20, 2025 14:33

muzarski marked this pull request as draft February 20, 2025 14:41

muzarski force-pushed the rustls-support-muzarski branch 5 times, most recently from 0997b6f to 56469a4 Compare February 20, 2025 15:34

muzarski marked this pull request as ready for review February 20, 2025 15:34

muzarski requested a review from Lorak-mmk February 20, 2025 15:36

muzarski self-assigned this Feb 20, 2025

muzarski added this to the 1.0.0 milestone Feb 20, 2025

muzarski added the API-stability Part of the effort to stabilize the API label Feb 20, 2025

Lorak-mmk requested changes Feb 21, 2025

View reviewed changes

muzarski force-pushed the rustls-support-muzarski branch from 56469a4 to fa4c2c2 Compare February 24, 2025 12:56

muzarski force-pushed the rustls-support-muzarski branch from fa4c2c2 to 00199db Compare February 24, 2025 14:09

muzarski force-pushed the rustls-support-muzarski branch from 00199db to eff5e41 Compare February 24, 2025 14:29

wprzytula and others added 5 commits February 25, 2025 18:54

connection: remove CloudConfig from ConnectionConfig

b299f14

It's no longer necessary, as CloudConfig is now passed inside TlsProvider.

metadata: derive Debug for UntranslatedPeer

9ef8bb9

metadata: expose only getters for UntranslatedPeer

b9f9955

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.

metadata: UntranslatedPeer holds borrowed &str

66537c5

As address translation should not need ownership of neither datacenter nor rack, let's not require cloning them.

metadata: move UntranslatedPeer to address_translator.rs

0402867

`UntranslatedPeer` is only used for address translation purposes and is not part of cluster metadata tables, so it makes sense for it to reside alongside `AddressTranslator`.

muzarski force-pushed the rustls-support-muzarski branch from f8d49e5 to 22f8d77 Compare February 25, 2025 17:54

nrxus and others added 15 commits February 25, 2025 18:58

connection: support rustls

9465e6b

Rustls is now supported as ordinary TLS provider, for far only for the non-cloud case. Co-authored-by: Wojciech Przytuła <[email protected]>

cloud: extract TlsInfo::get_dc_tls_context method

ab33659

document rustls where relevant

8060db5

Co-authored-by: Wojciech Przytuła <[email protected]>

examples: rename tls.rs -> tls-openssl.rs

f572336

Because soon we will also have a tls-rustls.rs example.

add rustls example

42a03e3

Co-authored-by: Wojciech Przytuła <[email protected]>

examples: rename cloud.rs -> cloud-openssl.rs

cce794d

examples: add cloud-rustls example

7b5d3ac

rename feature: cloud -> unstable-cloud

7c95833

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.

network: extract tls module from connection

a8de364

As the tls module got quite large, it makes sense to extract it to the network supermodule.

rust.yml: reorder and comment feature checks

9d618fb

The number of feature checks keeps increasing, so we need to impose some order on them or else we will regret.

rust.yml: TLS and cloud feature set checks

d8b870b

More cargo checks are added to the CI that try various combinations of TLS and cloud related features.

tls: update certs (scylladb#4)

bf62952

muzarski force-pushed the rustls-support-muzarski branch from 22f8d77 to bf62952 Compare February 25, 2025 17:59

Lorak-mmk approved these changes Feb 25, 2025

View reviewed changes

Lorak-mmk merged commit 58d1aef into scylladb:main Feb 25, 2025
12 checks passed

Lorak-mmk mentioned this pull request Mar 4, 2025

Release 1.0.0 #1265

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rustls support with dynamic choice of TLS provider for cloud #1254

Rustls support with dynamic choice of TLS provider for cloud #1254

muzarski commented Feb 19, 2025 •

edited

Loading

github-actions bot commented Feb 19, 2025 •

edited

Loading

Lorak-mmk left a comment

Lorak-mmk Feb 21, 2025

muzarski Feb 24, 2025

wprzytula Feb 25, 2025

Lorak-mmk Feb 25, 2025

nrxus Feb 25, 2025

muzarski commented Feb 24, 2025

muzarski commented Feb 24, 2025

muzarski commented Feb 24, 2025

muzarski commented Feb 24, 2025

Lorak-mmk commented Feb 24, 2025

muzarski commented Feb 24, 2025

muzarski commented Feb 25, 2025

Lorak-mmk commented Feb 25, 2025

muzarski commented Feb 25, 2025 •

edited

Loading

Lorak-mmk commented Feb 25, 2025

muzarski commented Feb 25, 2025

Rustls support with dynamic choice of TLS provider for cloud #1254

Rustls support with dynamic choice of TLS provider for cloud #1254

Conversation

muzarski commented Feb 19, 2025 • edited Loading

Pre-review checklist

github-actions bot commented Feb 19, 2025 • edited Loading

Lorak-mmk left a comment

Choose a reason for hiding this comment

Lorak-mmk Feb 21, 2025

Choose a reason for hiding this comment

muzarski Feb 24, 2025

Choose a reason for hiding this comment

wprzytula Feb 25, 2025

Choose a reason for hiding this comment

Lorak-mmk Feb 25, 2025

Choose a reason for hiding this comment

nrxus Feb 25, 2025

Choose a reason for hiding this comment

muzarski commented Feb 24, 2025

muzarski commented Feb 24, 2025

muzarski commented Feb 24, 2025

muzarski commented Feb 24, 2025

Lorak-mmk commented Feb 24, 2025

muzarski commented Feb 24, 2025

muzarski commented Feb 25, 2025

Lorak-mmk commented Feb 25, 2025

muzarski commented Feb 25, 2025 • edited Loading

Lorak-mmk commented Feb 25, 2025

muzarski commented Feb 25, 2025

muzarski commented Feb 19, 2025 •

edited

Loading

github-actions bot commented Feb 19, 2025 •

edited

Loading

muzarski commented Feb 25, 2025 •

edited

Loading