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

Rustls support #1182

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Rustls support #1182

wants to merge 6 commits into from

Conversation

nrxus
Copy link
Contributor

@nrxus nrxus commented Jan 27, 2025

Fixes #293

This is based on the work and discussion on #911

Movement on that PR seems to have stalled but it seemed like you all wanted to do this as part of the other breaking changes for v1.0 so I wanted to get this PR into your eyes prior to that.

Main breaking changes are:

  • Renamed ssl items that were meant to be generic over the TLS drivers o be tls instead as that is more accurate (although I know they are sometimes used interchangeably) (e.g., ssl_context to tls_context)
  • Renamed ssl feature flag to be openssl
  • The cloud feature flag no longer auto-enables openssl. This is done so users can choose if they want to enable openssl and/or rustls. We could add a check in the code that says that if cloud is enabled and there is no tls-related feature flag then hard compile error. It currently will already not compile but the error won't be as nice I think.

Other caveats:

  • For the cloud feature, if both openssl and rustls are enabled, the current code defaults to using openssl to keep it slightly more consistent with the old code. I think defaulting to rustls may provide a slightly better experience since it doesn't require any linking (dynamic nor static) but I wasn't sure what you all would prefer, it's easy enough to change.

  • For non-cloud, if both openssl and rustls are enabled, the user has a choice at runtime of what TlsContext to pass.

  • I do not have a cloud account for scylla so I have not manually tested the cloud changes.

  • The certificates in the test directory use a version that rustls does not support so they are not useful for the rustls example. I was unable to create new certs from the existing self-signed CA in the test directory because I do not know the passphrase. I have, however, tested that it works using my own certificates in a self-hosted 3-node Scylla instance in AWS that has TLS enabled for client-node communication.

  • The existing tests only test when using openssl, we could (and probably should) add tests for the rustls feature but that requires making the right certs that rustls can use.

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.

nrxus added 6 commits January 27, 2025 09:25
this allows for future TLS providers to be added
also change some reference to SSL to TLS where relevant
otherwise the error shows up too late in a non-terminal way whereas
the error is terminal
@github-actions github-actions bot added the semver-checks-breaking cargo-semver-checks reports that this PR introduces breaking API changes label Jan 27, 2025
Copy link

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

See the following report for details:

cargo semver-checks output
./scripts/semver-checks.sh --baseline-rev bb824b61cefb8426cc00aef563ce27882cf0aa7c
+ cargo semver-checks -p scylla -p scylla-cql --baseline-rev bb824b61cefb8426cc00aef563ce27882cf0aa7c
     Cloning bb824b61cefb8426cc00aef563ce27882cf0aa7c
    Building scylla v0.15.0 (current)
       Built [  41.834s] (current)
     Parsing scylla v0.15.0 (current)
      Parsed [   0.054s] (current)
    Building scylla v0.15.0 (baseline)
       Built [  22.271s] (baseline)
     Parsing scylla v0.15.0 (baseline)
      Parsed [   0.050s] (baseline)
    Checking scylla v0.15.0 -> v0.15.0 (no change)
     Checked [   0.107s] 107 checks: 103 pass, 4 fail, 0 warn, 0 skip

--- failure enum_variant_missing: pub enum variant removed or renamed ---

Description:
A publicly-visible enum has at least one variant 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.38.0/src/lints/enum_variant_missing.ron

Failed in:
  variant CloudConfigError::Ssl, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-bb824b61cefb8426cc00aef563ce27882cf0aa7c/8e88ff90ea17547a103649e6571db48be24ce469/scylla/src/cloud/config.rs:26

--- 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.38.0/src/lints/feature_missing.ron

Failed in:
  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.38.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-bb824b61cefb8426cc00aef563ce27882cf0aa7c/8e88ff90ea17547a103649e6571db48be24ce469/scylla/src/client/session_builder.rs:357

--- 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.38.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-bb824b61cefb8426cc00aef563ce27882cf0aa7c/8e88ff90ea17547a103649e6571db48be24ce469/scylla/src/client/session.rs:171

     Summary semver requires new major version: 4 major and 0 minor checks failed
    Finished [  65.770s] scylla
    Building scylla-cql v0.4.0 (current)
       Built [  11.125s] (current)
     Parsing scylla-cql v0.4.0 (current)
      Parsed [   0.035s] (current)
    Building scylla-cql v0.4.0 (baseline)
       Built [  10.919s] (baseline)
     Parsing scylla-cql v0.4.0 (baseline)
      Parsed [   0.034s] (baseline)
    Checking scylla-cql v0.4.0 -> v0.4.0 (no change)
     Checked [   0.110s] 107 checks: 107 pass, 0 skip
     Summary no semver update required
    Finished [  22.975s] scylla-cql
make: *** [Makefile:61: semver-rev] Error 1

@muzarski muzarski added this to the 0.16.0 milestone Jan 27, 2025
@wprzytula
Copy link
Collaborator

@nrxus, please let us know the next time when you are planning to create a PR with nontrivial changes, so that we discuss it first. We've already started working on this issue; being in-sync could have brought benefit to both of us and avoid work duplication.

Apart from that, we're happy to see another contribution from you! I'm going to review it soon and compare it to our intended solution.

@wprzytula wprzytula self-requested a review January 27, 2025 18:15
@Lorak-mmk Lorak-mmk self-requested a review January 27, 2025 18:16
@nrxus
Copy link
Contributor Author

nrxus commented Jan 27, 2025

@nrxus, please let us know the next time when you are planning to create a PR with nontrivial changes, so that we discuss it first. We've already started working on this issue; being in-sync could have brought benefit to both of us and avoid work duplication.

Apart from that, we're happy to see another contribution from you! I'm going to review it soon and compare it to our intended solution.

That's my bad, I totally meant to message on the old PR first but I was first waiting to see if I could even do this PR on my own or not (I am no TLS expert) and once it worked I got triggered happy on making the PR. I'll definitely contact you all first when I see a change becoming non-trivial.

Feel free to close this PR if taking it in is more work than just finishing what you all already had in mind (:

Comment on lines +97 to +115
impl TlsInfo {
fn from_pem(cert: &[u8], key: &[u8]) -> Result<Self, TlsError> {
#[cfg(feature = "openssl")]
{
let cert = openssl::x509::X509::from_pem(cert)?;
let key = openssl::pkey::PKey::private_key_from_pem(key)?;
Ok(TlsInfo::OpenSsl { key, cert })
}

#[cfg(all(not(feature = "openssl"), feature = "rustls"))]
{
use rustls::pki_types::pem::PemObject;
let key = rustls::pki_types::PrivateKeyDer::from_pem_slice(key)?;
let cert_chain: Vec<_> = rustls::pki_types::CertificateDer::pem_slice_iter(cert)
.collect::<Result<_, _>>()?;
Ok(TlsInfo::Rustls { cert_chain, key })
}
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

💭 This seems to work. In case of both openssl and rustls features being enabled, openssl is chosen. Fortunately, as this is the creation of TlsInfo, it's the only place where we have to do such logic based on features; in other places we're just matching on TlsInfo.

@wprzytula
Copy link
Collaborator

wprzytula commented Jan 28, 2025

@nrxus Will it be OK for you if I just cherry-pick your commits and rework them more or less for our implementation? This is a valuable work for us!

@nrxus
Copy link
Contributor Author

nrxus commented Jan 28, 2025

@nrxus Will it be OK for you if I just cherry-pick your commits and rework them more or less for our implementation? This is a valuable work for us!

Yeah that's absolutely okay! I am glad it's useful, I look forward to all the nice changes in the next release

@wprzytula
Copy link
Collaborator

@nrxus Could you please elaborate on the following:

don't ignore openssl connection error

otherwise the error shows up too late in a non-terminal way whereas
the error is terminal

?

What did you observe under what circumstances?

@nrxus
Copy link
Contributor Author

nrxus commented Jan 29, 2025

@nrxus Could you please elaborate on the following:

don't ignore openssl connection error

otherwise the error shows up too late in a non-terminal way whereas
the error is terminal

?

What did you observe under what circumstances?

I had done something wrong in the creation of the certs if I remember correctly (the CN didn't match the IP of the nodes and the self-signed CA didn't have the root CA extension marker). Then when connecting my program would not immediately show up the error but instead it would show errors as it attempted to retry every so often, but it wasn't even an error log (I don't remember if it was a debug or info log though). Once I stopped ignoring the error then the error was a lot more clear and it in fact immediately failed when creating the session ( as expected).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-checks-breaking cargo-semver-checks reports that this PR introduces breaking API changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rustls support
3 participants