Add TLS testing via CCM - #489
Merged
Merged
Conversation
This does two things: 1. Bumps the default ScyllaDB version to 2026.2.2. 2. Ensures that the version is fully qualified, dramatically improving execution time thanks to CCM not having to hit S3 to resolve the version.
There was a discrepancy between the documentation and the actual default value of the SSL verification option in the `cassandra.h` header file. The documentation stated that the default was `CASS_SSL_VERIFY_PEER_CERT`, but in reality, it was set to `CASS_SSL_VERIFY_NONE`. This commit updates the documentation to reflect the correct default value. Note that this change is only to align the documentation with the actual behavior of the code. The default value of the SSL verification option should likely be changed to enable verification, but that requires further changes, so is left for a follow-up.
This will allow us to use the latest version of the driver's repo which has CCM bridge extracted, so we can use it as a dev-dependency for our testing.
This will allow us to write CCM tests for TLS and other features in the future, without using the C++ CCM harness, leaving the tests in Rust and making them easier to maintain.
The run-test-unit target spins up no real cluster, but the upcoming TLS integration tests use CCM to start a genuine ScyllaDB cluster. Exclude any test whose path contains "ccm" here; those tests run as part of the Scylla integration job instead. This is a no-op until the ccm tests are introduced.
📝 WalkthroughWalkthroughThe change adds CCM-managed three-node TLS integration tests for the C API. The tests generate certificates, configure ScyllaDB encryption, and validate CA trust, hostname verification, verification flags, and mutual TLS. CCM tests now run through the Scylla integration target. Rust dependencies and the default Scylla version are updated. The workflow preserves failed-cluster logs, and the pull request template adds documentation checklist items. Sequence Diagram(s)sequenceDiagram
participant Makefile
participant CCM
participant ScyllaDBCluster
participant CAPI
Makefile->>CCM: start the configured test cluster
CCM->>ScyllaDBCluster: create and configure three nodes
CAPI->>ScyllaDBCluster: connect with TLS settings
ScyllaDBCluster-->>CAPI: return connection and health-check results
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Warning Review ran into problems🔥 ProblemsLinked repositories: Couldn't analyze
Errors logged to '/home/jailuser/git/.git/lfs/logs/20260805T161824.155822365.log'. Errors logged to '/home/jailuser/git/.git/lfs/logs/20260805T161943.743298817.log'. Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds end-to-end TLS validation for the C API using CCM-managed ScyllaDB clusters.
Changes:
- Adds TLS, identity-verification, and client-authentication integration tests.
- Updates Rust dependencies and ScyllaDB test version.
- Corrects SSL default documentation and expands the PR checklist.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
.github/pull_request_template.md |
Adds documentation checklist items. |
Makefile |
Runs CCM tests and updates ScyllaDB version. |
include/cassandra.h |
Documents the actual SSL verification default. |
scylla-rust-wrapper/Cargo.lock |
Locks updated and new dependencies. |
scylla-rust-wrapper/Cargo.toml |
Adds CCM/certificate dependencies and updates driver revisions. |
scylla-rust-wrapper/tests/integration/main.rs |
Registers the CCM test module. |
scylla-rust-wrapper/tests/integration/ccm/mod.rs |
Defines CCM test infrastructure and version warnings. |
scylla-rust-wrapper/tests/integration/ccm/tls.rs |
Implements end-to-end C API TLS tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
include/cassandra.h-4312-4312 (1)
4312-4312: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winSecurity Misconfiguration (CWE-295): Improper Certificate Validation
Reachability: External · Exploitability: Moderate
Document the TLS risk of the default.
Add a warning that
CASS_SSL_VERIFY_NONEdisables certificate and identity verification and permits man-in-the-middle attacks. Recommend explicit peer certificate and identity verification. State that changing the default is planned for a follow-up.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@include/cassandra.h` at line 4312, Update the SSL configuration documentation near the CASS_SSL_VERIFY_NONE default to warn that it disables certificate and identity verification, allowing man-in-the-middle attacks. Recommend explicitly enabling peer certificate and identity verification, and note that changing this default is planned for a follow-up.
🧹 Nitpick comments (1)
scylla-rust-wrapper/tests/integration/ccm/tls.rs (1)
140-145: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe CA private key is written but never used.
ca.keyis written into the cluster directory. Onlyca.crtis referenced, by theclient_encryption_options.truststoresetting at Line 152. The client-side trust material comes fromca.pem()in memory. Remove this write, or add a comment that explains why the CA private key must exist on disk. It also adds anotherFile::create_newcall that fails on a re-used cluster directory.♻️ Proposed removal
- let ca_key_file_path = cluster.cluster_dir().join("ca.key"); - let mut ca_key_file = File::create_new(&ca_key_file_path).await.unwrap(); - ca_key_file - .write_all(ca.key().serialize_pem().as_bytes()) - .await - .unwrap(); - let ca_cert_file_path = cluster.cluster_dir().join("ca.crt");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scylla-rust-wrapper/tests/integration/ccm/tls.rs` around lines 140 - 145, Remove the unused ca_key_file_path and ca_key_file creation/write block near the CA certificate setup; retain the in-memory ca.pem() trust material and ca.crt handling unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scylla-rust-wrapper/tests/integration/ccm/mod.rs`:
- Around line 29-36: Extract the predicate currently assigned to is_partial into
a free function named is_partial_version, preserving the existing release,
release-candidate, unstable-latest, and no-colon behavior. Update the caller to
use it, and add Rust unit tests covering all five cases shown in the review,
including the intended fully qualified classification for bare versions without
a colon.
---
Other comments:
In `@include/cassandra.h`:
- Line 4312: Update the SSL configuration documentation near the
CASS_SSL_VERIFY_NONE default to warn that it disables certificate and identity
verification, allowing man-in-the-middle attacks. Recommend explicitly enabling
peer certificate and identity verification, and note that changing this default
is planned for a follow-up.
---
Nitpick comments:
In `@scylla-rust-wrapper/tests/integration/ccm/tls.rs`:
- Around line 140-145: Remove the unused ca_key_file_path and ca_key_file
creation/write block near the CA certificate setup; retain the in-memory
ca.pem() trust material and ca.crt handling unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: QUIET
Plan: Pro Plus
Run ID: 0e1581b5-ef2f-4d10-a36c-9ea1cc303fa9
⛔ Files ignored due to path filters (1)
scylla-rust-wrapper/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
.github/pull_request_template.mdMakefileinclude/cassandra.hscylla-rust-wrapper/Cargo.tomlscylla-rust-wrapper/tests/integration/ccm/mod.rsscylla-rust-wrapper/tests/integration/ccm/tls.rsscylla-rust-wrapper/tests/integration/main.rs
Lorak-mmk
requested changes
Aug 5, 2026
wprzytula
force-pushed
the
add-tls-testing
branch
from
August 5, 2026 13:25
87676bb to
f2de8a9
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated no new comments.
Suppressed comments (1)
scylla-rust-wrapper/tests/integration/ccm/tls.rs:265
- The positive paths issue only one untargeted query, so they can pass after establishing TLS to a single node even if the other two nodes' per-node certificates cannot be verified (for example, if every certificate is checked against the first contact point). This leaves the per-node TLS behavior untested; route a health query explicitly to each node with
cass_statement_set_host, or assert that connection pools are established for all three nodes. The upstream test being ported explicitly checks that the session is fully connected to the expected node count.
let exec_fut =
cass_session_execute(session_raw.borrow(), statement_raw.borrow().into_c_const());
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scylla-rust-wrapper/tests/integration/ccm/tls.rs (1)
364-369: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winWeak negative TLS assertions accept any error. Each assertion checks only that the code is not
CASS_OK. A setup failure — missing contact points, truncated certificate file, cluster not initialized — also produces a non-OK error, so the assertion passes without proving TLS verification rejected the peer. Useassert_cass_error_eq()to pin the expectedCassErrorvariant at each site:
scylla-rust-wrapper/tests/integration/ccm/tls.rs#L364-L369: assertCASS_ERROR_SSL_INVALID_PEER_CERTfor untrusted server CA.scylla-rust-wrapper/tests/integration/ccm/tls.rs#L405-L409: assertCASS_ERROR_SSL_IDENTITY_MISMATCHfor SAN mismatch underPEER_IDENTITY.scylla-rust-wrapper/tests/integration/ccm/tls.rs#L459-L463: assertCASS_ERROR_SSL_NO_PEER_CERTwhen server requires client certificate.scylla-rust-wrapper/tests/integration/ccm/tls.rs#L520-L525: assertCASS_ERROR_SSL_INVALID_PEER_CERTwhenPEER_CERTrejects untrusted chain.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scylla-rust-wrapper/tests/integration/ccm/tls.rs` around lines 364 - 369, Replace the weak non-OK assertions in scylla-rust-wrapper/tests/integration/ccm/tls.rs at lines 364-369, 405-409, 459-463, and 520-525 with assert_cass_error_eq(). Expect CASS_ERROR_SSL_INVALID_PEER_CERT at 364-369 and 520-525, CASS_ERROR_SSL_IDENTITY_MISMATCH at 405-409, and CASS_ERROR_SSL_NO_PEER_CERT at 459-463.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scylla-rust-wrapper/tests/integration/ccm/tls.rs`:
- Around line 75-84: Update the TLS test file-writing flow around the File
handles like cert_file, key_file, ca_key_file, and ca_cert_file to explicitly
call flush() after write_all() and before each handle is dropped. Keep the
existing create_new and write_all logic intact, but ensure every buffered file
write is forced to disk before moving on. Apply the same change in the cert/key
setup and the CA material setup so db.cert, db.key, ca.key, and ca.crt are fully
written.
---
Nitpick comments:
In `@scylla-rust-wrapper/tests/integration/ccm/tls.rs`:
- Around line 364-369: Replace the weak non-OK assertions in
scylla-rust-wrapper/tests/integration/ccm/tls.rs at lines 364-369, 405-409,
459-463, and 520-525 with assert_cass_error_eq(). Expect
CASS_ERROR_SSL_INVALID_PEER_CERT at 364-369 and 520-525,
CASS_ERROR_SSL_IDENTITY_MISMATCH at 405-409, and CASS_ERROR_SSL_NO_PEER_CERT at
459-463.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: QUIET
Plan: Pro Plus
Run ID: 1ae37cb9-124e-4e1e-ab08-162a5f1c106d
⛔ Files ignored due to path filters (1)
scylla-rust-wrapper/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
.github/pull_request_template.md.github/workflows/build-lint-and-test.ymlMakefileinclude/cassandra.hscylla-rust-wrapper/Cargo.tomlscylla-rust-wrapper/tests/integration/ccm/mod.rsscylla-rust-wrapper/tests/integration/ccm/tls.rsscylla-rust-wrapper/tests/integration/main.rs
Introduce end-to-end TLS integration tests driven through the C API against a real, TLS-enabled ScyllaDB cluster started via CCM (using the scylla-ccm-bridge crate). This is a port of the ScyllaDB Rust Driver's ccm::tls tests. This commit adds: - the rcgen dev-dependency for on-the-fly certificate generation, - the ccm test module wiring, - the shared harness (CA/cert generation, per-node TLS configuration, a C-API connect + health-check helper), - the first test, connect_tls_no_client_auth, covering PEER_IDENTITY with a matching SAN, verification disabled (NONE), and the failure to connect when the server CA is not trusted. The tests connect by node IP, so identity verification is exercised against the iPAddress SAN. Every `ccm` invocation carries a hefty fixed startup cost, so the harness warns if SCYLLA_TEST_CLUSTER names a partial version. scylla-ccm re-resolves the cluster version on every invocation, and a partial one (e.g. `release:2026.1`) forces it to list an S3 bucket and sleep for a random 0-5 seconds each time, which otherwise dominates the runtime of the whole suite. The nodes are deliberately configured one at a time, even though that serialises three `ccm` invocations: each one rewrites its own node.conf non-atomically while loading, and thus reading, every other node's, so configuring them in parallel makes ccm choke on a half-written file.
Failed Rust CCM runs previously would leave no diagnostics: `scylla-ccm- bridge` removes the cluster and its temp config dir on panic unless TEST_KEEP_CLUSTER_ON_FAILURE=true, and its logs live under `/tmp/ccm- rust/<temp>/<cluster>/node*/logs`, which the upload glob did not match. Set `TEST_KEEP_CLUSTER_ON_FAILURE=true` for the Scylla integration step (CI only, so local runs stay clean) and add the `/tmp/ccm-rust` layout to CCM_LOGS_PATTERN so the existing 'Upload CCM logs' step collects them.
Port of the Rust Driver's test_tls_verifies_hostname. Each node presents a certificate whose SAN (1.1.1.1) does not match its actual IP. With PEER_IDENTITY the driver must reject the connection; with NONE it connects, confirming that verification is what makes the difference.
Port of the Rust Driver's test_connect_tls_with_client_auth. The cluster is configured with require_client_auth = true. The C-API connect helper is extended to optionally install a client certificate and private key (cass_ssl_set_cert / cass_ssl_set_private_key). The test asserts that a connection without a client certificate is rejected, and that presenting a certificate/key signed by the trusted CA succeeds.
Add an ignored test encoding the desired semantics of PEER_CERT: it should validate only the certificate chain and tolerate an IP SAN mismatch, while still rejecting an untrusted CA. This is not implemented yet (PEER_CERT currently behaves like PEER_IDENTITY because the Rust driver always verifies the node IP), so the test is #[ignore]d. Un-ignore it once chain-only PEER_CERT support lands.
run-test-integration-scylla now also runs the Rust CCM integration tests (via cargo test --test integration ccm) after the C++ suite. The CCM cluster version is taken from SCYLLA_VERSION (passed as SCYLLA_TEST_CLUSTER to the scylla-ccm-bridge crate), and CCM data is kept under /tmp. Also document that SCYLLA_VERSION should be fully qualified. scylla-ccm re-resolves the version on every `ccm` invocation, so a partial one (such as the default release:2025.3) makes each invocation list an S3 bucket and sleep for a random 0-5 seconds. Note: the workflow's CCM log-artifact glob may need extending to cover /tmp/ccm-rust if these logs are to be uploaded on failure.
wprzytula
force-pushed
the
add-tls-testing
branch
from
August 5, 2026 15:59
f2de8a9 to
b3d9200
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated no new comments.
Suppressed comments (1)
scylla-rust-wrapper/tests/integration/ccm/tls.rs:139
TEST_KEEP_CLUSTER_ON_FAILUREdoes not preserve failures from this configuration callback. In the pinned bridge,run_ccm_test_with_configurationinvokesconfigure(cluster).awaitbefore itscatch_unwind; every file/configuration operation here usesunwrap(), so a write orupdateconfpanic drops and removes the cluster before the workflow can upload its logs. Catch configuration panics locally, callcluster.mark_as_failed(), and then resume the panic (or fix the bridge to protect the configuration callback too).
|mut cluster: Cluster| async move {
There was a problem hiding this comment.
Actionable comments posted: 1
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
scylla-rust-wrapper/tests/integration/ccm/mod.rs-58-77 (1)
58-77: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMove the pure unit test out of the
ccm::path.Lines 4-7 state that every test under
ccm::is excluded frommake run-test-unitby the path substringccm.classifies_cluster_versionsmatches that substring, so it runs only in the Scylla integration job, which requires a cluster. The test needs no cluster.Place
is_partial_versionand its tests in a module whose path does not containccm, or exclude it explicitly from the filter.As per coding guidelines: "Implement Rust unit tests for introduced features or changes."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scylla-rust-wrapper/tests/integration/ccm/mod.rs` around lines 58 - 77, Move the pure is_partial_version logic and its classifies_cluster_versions unit test out of any module path containing ccm so it is included in make run-test-unit. Preserve all existing assertions and keep cluster-dependent integration tests under ccm unchanged; alternatively, explicitly exempt this test from the ccm path filter.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scylla-rust-wrapper/tests/integration/ccm/tls.rs`:
- Around line 52-59: Update the TLS CCM test setup around cluster_3_nodes and
run_ccm_tls_test so each active test uses a distinct cluster name, or verify and
rely on per-name serialization in run_ccm_test_with_configuration. Also make CA
setup resilient to preserved cluster directories by replacing the unconditional
File::create_new calls for ca.key and ca.crt with cleanup or safe overwrite
behavior.
---
Other comments:
In `@scylla-rust-wrapper/tests/integration/ccm/mod.rs`:
- Around line 58-77: Move the pure is_partial_version logic and its
classifies_cluster_versions unit test out of any module path containing ccm so
it is included in make run-test-unit. Preserve all existing assertions and keep
cluster-dependent integration tests under ccm unchanged; alternatively,
explicitly exempt this test from the ccm path filter.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: QUIET
Plan: Pro Plus
Run ID: d61106e8-d2d2-46d0-bfcf-7aec1d201ec6
⛔ Files ignored due to path filters (1)
scylla-rust-wrapper/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
.github/pull_request_template.md.github/workflows/build-lint-and-test.ymlMakefileinclude/cassandra.hscylla-rust-wrapper/Cargo.tomlscylla-rust-wrapper/tests/integration/ccm/mod.rsscylla-rust-wrapper/tests/integration/ccm/tls.rsscylla-rust-wrapper/tests/integration/main.rs
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
scylladb/scylladb(auto-detected)
Lorak-mmk
approved these changes
Aug 6, 2026
Comment on lines
178
to
182
|
|
||
| ifndef SCYLLA_VERSION | ||
| SCYLLA_VERSION := release:2025.3 | ||
| SCYLLA_VERSION := release:2026.2.2 | ||
| endif | ||
|
|
Contributor
There was a problem hiding this comment.
I invite you to configure Renovate to bump this automatically.
wprzytula
added a commit
that referenced
this pull request
Aug 7, 2026
## Summary This PR follows up #489 and cleans up setting verify flags for TLS. ## What's done ### Repaired the default (security-critical) There was a bug that caused mismatch between documentation (`VERIFY_PEER_CERT`) and reality (`VERIFY_NONE`). By fixing the bug, the real default moves from "no verification at all" to verifying the peer's certificate, . So for callers who never called `cass_ssl_set_verify_flags()`, a trusted certificate must now be configured with `cass_ssl_add_trusted_cert()`, otherwise every connection fails. The previous behaviour remains available as `CASS_SSL_VERIFY_NONE`. ### Fixed `CASS_SSL_VERIFY_PEER_CERT` semantics After Rust Driver turned on hostname verification by IP in SAN (scylladb/scylla-rust-driver#1491), semantics of our implementation silently changed, requiring identity verification even with `CASS_SSL_VERIFY_PEER_CERT` option set. To support chain-only validation, a bit hacky logic via custom verify callback was introduced: after openssl finishes identity verification (either with success or failure), the callback is fired; the callback matches on an error and ignores errors related to failing identity verification. This emulates no identity verification, just certificate chain verification. <details><summary>Detailed description of the hack by Opus</summary> <p> #### Support for `PEER_CERT` is hacked in — but the hack looks solid `CASS_SSL_VERIFY_PEER_CERT` means "certificate is present and valid" — chain validation *without* an identity check. We could not express that, because the Rust driver pins the identity itself, in `network/connection.rs`: ```rust crate::network::tls::Tls::OpenSsl010(mut ssl) => { ssl.param_mut() .set_ip(node_address.ip())?; ``` That runs per-connection, on the `Ssl` created from our `SslContext`, so nothing we configure on the `SSL_CTX` can undo it. `PEER_CERT` therefore behaved identically to `PEER_IDENTITY`, and an intermediate commit in this PR does no more than log a warning saying so. The way out is a **custom verification callback that tolerates exactly the identity-mismatch errors**. When the identity check fails, OpenSSL's `check_id()` does not abort outright — it sets the error and asks the verify callback, continuing if the callback returns 1: ```c if (vpm->ip != NULL && X509_check_ip(...) <= 0) { if (!check_id_error(ctx, X509_V_ERR_IP_ADDRESS_MISMATCH)) return 0; } ``` So `PEER_CERT` installs: ```rust extern "C" fn verify_peer_cert_callback(preverify_ok: c_int, x509_ctx: *mut X509_STORE_CTX) -> c_int { if preverify_ok == 1 { return 1; } let error = unsafe { X509_STORE_CTX_get_error(x509_ctx) }; match error { X509_V_ERR_HOSTNAME_MISMATCH | X509_V_ERR_EMAIL_MISMATCH | X509_V_ERR_IP_ADDRESS_MISMATCH => { unsafe { X509_STORE_CTX_set_error(x509_ctx, X509_V_OK) }; 1 } _ => 0, } } ``` Why I think this is solid: - It is **narrow**. Only the three identity-mismatch codes are accepted. An untrusted issuer, an expired certificate, a self-signed leaf, a broken chain — all still fail, at any depth. It cannot degrade into "accept anything". - It is **not a bypass of chain validation**. `SSL_VERIFY_PEER` stays on and the full chain walk runs unchanged; we only suppress the identity check that the layer below forces on us. - It **only applies to `PEER_CERT`**. `PEER_IDENTITY` still passes `None`, so switching flags in either direction is clean. - Resetting the error to `X509_V_OK` keeps `SSL_get_verify_result()` consistent with the handshake outcome; returning 1 alone would let the handshake succeed while leaving a stale mismatch error on the session. - It is **covered by the test** that asserts both directions: mismatched SAN accepted, untrusted CA rejected. The honest caveat: this is a workaround for the driver forcing `set_ip()`. The clean fix belongs in the Rust Driver — letting the caller opt out of identity verification — after which the callback can go away. </p> </details> ### Flags handling - `CASS_SSL_VERIFY_PEER_IDENTITY` is now supported. After Rust Driver turned on hostname verification by IP in SAN (scylladb/scylla-rust-driver#1491), this is easily turned on. - `CASS_SSL_VERIFY_PEER_IDENTITY_DNS` is mapped to `CASS_SSL_VERIFY_PEER_IDENTITY` with a warning — the driver verifies the IP, not a hostname, and hostname resolution is a separate feature we do not implement. - `cass_ssl_set_verify_flags()` now treats the flags as the bitmask they are, so documented combinations such as `CASS_SSL_VERIFY_PEER_CERT | CASS_SSL_VERIFY_PEER_IDENTITY` work. Values carrying **unknown bits** log an error and **leave the settings untouched**, rather than silently switching verification on. ### Updated documentation - Noted that certificates that identify a node only by subject **common name** are rejected — they need an `iPAddress` subject alternative name. OpenSSL's `X509_VERIFY_PARAM_set1_ip()`, which the Rust driver uses, never falls back to the subject for IP checks, unlike the C/C++ driver's own verification code. This is documented in `cassandra.h` and in the TLS guide. ## Possible follow-ups - Upstream cpp-driver documents `CASS_SSL_VERIFY_PEER_CERT` as the default; we may deliberately default to the stricter `CASS_SSL_VERIFY_PEER_IDENTITY`, which is the Rust Driver's choice. - Real `CASS_SSL_VERIFY_PEER_IDENTITY_DNS` (requires Rust Driver's support). - Push identity-verification opt-out into the Rust Driver, so the `PEER_CERT` callback can be dropped.
Merged
This file contains hidden or 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
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.
Background / Motivation
Someone noticed that our TLS verification default did not match what we
document.
include/cassandra.hclaimed:while
cass_ssl_new_no_lib_init()actually did:CASS_SSL_VERIFY_NONEis0x00, which happens to beSSL_VERIFY_NONEtoo, so the effective default was no peer verification whatsoever — the exact opposite of what the header promised, and a security-relevant discrepancy: auser who read the docs and did not call
cass_ssl_set_verify_flags()believed the server certificate was being validated when it was not.Looking into it, I found the more fundamental problem: none of this was tested. There was no coverage of TLS at all on the Rust side — no test would have caught the wrong default, and no test would catch us breaking it again.
cass_ssl_set_verify_flags()was similarly untested, and it showed: unknown flag values silently fell through to a catch-all arm that enabledSSL_VERIFY_PEER, so passing garbage quietly changed the security settings.This PR does one thing: adds real end-to-end TLS tests.
A follow up PR will use them to validate the fixed defaults and the flag handling, and finally implement correctly the one flag (
CASS_SSL_VERIFY_PEER_CERT) we did not respect before due to Rust Driver's behavioural change (turning on hostname verification in OpenSSL - scylladb/scylla-rust-driver#1491).What's done
Appetizers
Updated docs to mention TLS verification's actual default
There was a discrepancy between the documentation and the actual default value of the SSL verification option in the
cassandra.hheader file. The documentation stated that the default wasCASS_SSL_VERIFY_PEER_CERT, but in reality, it was set toCASS_SSL_VERIFY_NONE.Note that this change is only to align the documentation with the actual behavior of the code. The default value of the SSL verification option should likely be changed to enable verification, but that requires further changes, so is left for a follow-up.
Bumped Rust Driver
cargo update.scylla-ccm-bridge, which we will use for writing CCM tests in Rust.Tests written
End-to-end TLS tests that drive the driver through its C API, exactly as a C/C++ consumer would, against a real TLS-enabled ScyllaDB cluster. They are ports of the Rust Driver's
ccm::tlstests.The clusters are managed from Rust via the
scylla-ccm-bridgecrate from the Rust Driver, added as a dev-dependency. That lets us write CCM tests in Rust rather than going through the C++ CCM harness, which makes them far easier to maintain. Certificates are generated on the fly withrcgen, so there is nothing checked in and nothing to rotate: the harness creates a CA, issues a per-node server certificate, installs it viaccm <node> updateconf, and hands the CA to the test so it can build a matching client trust store.One practical note for anyone running these locally: pass a fully-qualified cluster version.
scylla-ccmre-resolves the version on everyccminvocation, and a partial one such asrelease:2026.2makes it list an S3 bucket and sleep for a random 0–5 seconds each time, which dominates the runtime (~31 s vs ~8 s for the whole suite). The default in the Makefile is fully qualified, and the harness warns ifSCYLLA_TEST_CLUSTERis not.Pre-review checklist
[ ] I have enabled appropriate tests inMakefilein{SCYLLA,CASSANDRA}_(NO_VALGRIND_)TEST_FILTER.[ ] I added appropriateFixes:annotations to PR description.