Add SSL/TLS support #70

Gor027 · 2022-08-16T08:34:14Z

Pre-review checklist

I have split my patch into logically separate commits.
All commit messages clearly explain what they change and why.
PR description sums up the changes and reasons why they should be introduced.
I have enabled appropriate tests in .github/workflows/build.yaml in gtest_filter.

This PR is dependent upon #69

This PR adds SSL/TLS support to the driver.
Integration tests that are enabled in the C++ driver and are actually run during Jenkins builds, are also enabled for the cpp-rust-driver. One of the reasons that the SSL tests are disabled for cpp-driver is that during the setup scylla-ccm fails while trying to start a Scylla cluster. So, to make sure the SSL properly works, I have also run the integration tests on a Cassandra cluster.

piodul · 2022-08-25T13:09:35Z

scylla-rust-wrapper/src/ssl.rs

+pub unsafe extern "C" fn cass_ssl_new() -> *const CassSsl {
+    cass_ssl_new_no_lib_init()
+}


The difference between cass_ssl_new and cass_ssl_new_no_lib_init is that the first is supposed to perform some additional initialization. What does this initialization do in the original implementation?

Yes, in cass_ssl_new, openssl::init() should be explicitly called. SslContextBuilder::new() does that anyway, so I thought that should be enough.

piodul · 2022-08-25T13:35:30Z

scylla-rust-wrapper/src/cluster.rs

+
+    let mut ssl_context_builder = SslContextBuilder::new(SslMethod::tls_client()).unwrap();
+
+    if cass_ssl.trusted_cert.is_some() {


Use if let Some(trusted_cert) = &cass_ssl.trusted_cert please, instead of is_some() + unwrap(). The less places which can potentially panic in the code, the better.

piodul · 2022-08-25T13:49:13Z

scylla-rust-wrapper/src/cluster.rs

+        // This should contain the entire certificate chain starting with the certificate itself.
+        let cert_chain = cass_ssl.cert_chain.as_ref().unwrap();
+        let cert = cert_chain.get(0).unwrap();
+        ssl_context_builder.set_certificate(cert).unwrap();


There are some ssl_context_builder.set_xyz operations here which can fail, and you unwrap them. Are they supposed to always succeed? If they do, please add comments which explain that. If not, then I think that the implementation need to be rethought so that those operations are performed in functions that can return errors.

It is worth noting that the original implementation's cass_cluster_set_ssl is very simple and doesn't call any functions from the openssl library. If we are using openssl directly like the original impl, then maybe our implementation of the SSL-related functions can mirror the original ones more closely?

The C++ driver implementation creates an OpenSslContext and sets the necessary configuration explicitly, however, rust-openssl obliges to use the SslContextBuilder to do so, and then consume the builder to create a SslContext. Another limitation is that SslContextBuilder cannot be cloned, so it makes us create the builder once and immediately consume it, as it is done in the cass_cluster_set_ssl. The SslContext setters, e.g. cass_ssl_add_trusted_cert, cass_ssl_set_verify_flags, validate the user-provided configuration, and cass_cluster_set_ssl only sets them in the SslContextBuilder, so I think those unwrap calls should always succeed.

Gor027 · 2022-08-29T15:58:30Z

V2:

The SSL support is reimplemented, this time based on the openssl-sys crate which provides rust bindings for the original OpenSSL library. With this approach, it is possible to mirror the behavior of the SSL-related functions more closely. As before, the functionality was tested manually on ScyllaDB (some integration tests are enabled), and it was also tested on a Cassandra cluster.

avelanarius · 2022-08-30T14:19:12Z

.github/workflows/build.yml

@@ -39,6 +39,7 @@ jobs:
 :BasicsTests.*\
 :PreparedTests.*\
 :CassandraTypes/CassandraTypesTests/*.Integration_Cassandra_*\
+:SslNoClusterTests*:SslNoSslOnClusterTests*\


@Gor027 Also run tests with Cassandra (not only Scylla 5.0.0) so that we can test (almost?) all SSL tests.

After merging this PR, I will add running SSL-related tests on Cassandra.

piodul

There seems to be some error checking/reporting logic missing from the ported function (ssl_log_errors, ERR_peek_error).

piodul · 2022-09-08T10:06:11Z

scylla-rust-wrapper/src/ssl.rs

+    let trusted_store: *mut X509_STORE = X509_STORE_new();
+
+    SSL_CTX_set_cert_store(ssl_context, trusted_store);
+


There are some calls missing from the original implementation here:

SSL_CTX_set_verify(ssl_ctx_, SSL_VERIFY_NONE, ssl_no_verify_callback); // Limit to TLS 1.2 for now. TLS 1.3 has broken the handshake code. SSL_CTX_set_max_proto_version(ssl_ctx_, TLS1_2_VERSION);

I guess SSL_VERIFY_NONE is the default mode, but I agree, that it is better to set it manually. I am not sure though, whether should we limit the maximum protocol version, as it works fine currently, perhaps with the latest protocol version available in openssl-sys.

OK, I agree about the TLS version. The original comment is probably quite old.

piodul · 2022-09-08T10:25:44Z

scylla-rust-wrapper/src/ssl.rs

+pub unsafe extern "C" fn cass_ssl_set_verify_flags(ssl: *mut CassSsl, flags: i32) {
+    let ssl = clone_arced(ssl);
+
+    SSL_CTX_set_verify(ssl.ssl_context, flags, None);


I don't think this is correct. The flags should be of type CassSslVerifyFlags, which is a custom enum defined by the driver, which I'm not sure if are a valid argument to SSL_CTX_set_verify. The original implementation seems to do something different - it stores the flags in a field which is later used in a custom verification logic.

The CassSslVerifyFlags partially matches with the expected argument for the SSL_CTX_set_verify. The C++ driver overrides the OpenSSL handshake implementation to verify the client's hostname and IP address, however, OpenSSL 1.0.2 and newer are able to verify hostname and IP addresses themselves. So, the flags CASS_SSL_VERIFY_PEER_IDENTITY and CASS_SSL_VERIFY_PEER_IDENTITY_DNS can be either removed or considered to be CASS_SSL_VERIFY_PEER_CERT. Although, I have tested setting those 2 additional flags while establishing a connection with a Scylla and Cassandra cluster, and it seemed to work.

Here is how CassSslVerifyFlags is defined:

typedef enum CassSslVerifyFlags_ { CASS_SSL_VERIFY_NONE = 0x00, CASS_SSL_VERIFY_PEER_CERT = 0x01, CASS_SSL_VERIFY_PEER_IDENTITY = 0x02, CASS_SSL_VERIFY_PEER_IDENTITY_DNS = 0x04 } CassSslVerifyFlags;

And here is how the flags for the mode argument of SSL_CTX_set_verify are defined:

# define SSL_VERIFY_NONE 0x00 # define SSL_VERIFY_PEER 0x01 # define SSL_VERIFY_FAIL_IF_NO_PEER_CERT 0x02 # define SSL_VERIFY_CLIENT_ONCE 0x04 # define SSL_VERIFY_POST_HANDSHAKE 0x08

According to the man page here: https://linux.die.net/man/3/ssl_ctx_set_verify, SSL_VERIFY_CLIENT_ONCE and SSL_VERIFY_POST_HANDSHAKE are ignored in client mode (the driver is a client). CASS_SSL_VERIFY_PEER_IDENTITY and CASS_SSL_VERIFY_PEER_IDENTITY_DNS "seem to work" because they map to the values I mentioned in the previous sentence.

Ignoring the flags is not the proper behavior. The driver flags and the SSL flags do not mean the same.

I guess SSL_VERIFY_PEER also verifies the peer's hostname and IP address. I think we barely can simulate the behavior of the C++ driver in this case, because the handshake(where the certificate verification happens) should occur while trying to establish a session between the client and the database, and this is being done on the Rust driver side.

OK. In that case, let's print a warning to stderr in case one of the non-supported flags is provided and leave a TODO comment in the code. Furthermore, please explicitly map CASS_XYZ values to SSL_XYZ values in the code - instead of passing CassSslVerifyFlags value to the SSL function.

piodul · 2022-09-12T07:48:27Z

scylla-rust-wrapper/src/ssl.rs

+        CASS_SSL_VERIFY_NONE => {
+            SSL_CTX_set_verify(ssl.ssl_context, SslVerifyMode::NONE.bits(), None)
+        }
+        CASS_SSL_VERIFY_PEER_CERT => {


The CASS_SSL_VERIFY_ constants are flags, they are not mutually exclusive variants of an enum. The user can specify more than one of them by OR-ing them together.

Notice that their values are powers of two:

typedef enum CassSslVerifyFlags_ { CASS_SSL_VERIFY_NONE = 0x00, CASS_SSL_VERIFY_PEER_CERT = 0x01, CASS_SSL_VERIFY_PEER_IDENTITY = 0x02, CASS_SSL_VERIFY_PEER_IDENTITY_DNS = 0x04 } CassSslVerifyFlags;

Currently, there is no support for flags other than CASS_SSL_VERIFY_NONE and CASS_SSL_VERIFY_PEER_CERT, so in my opinion OR-ing with CASS_SSL_VERIFY_PEER_IDENTITY or CASS_SSL_VERIFY_PEER_IDENTITY_DNS should not be accepted.

Why not? If the user provides CASS_SSL_VERIFY_PEER_CERT, then we should enable the functionality provided by this flag. If they additionally provide CASS_SSL_VERIFY_PEER_IDENTITY, we should print a warning and ignore it, but I don't see why this should disable the first flag.

I agree that it would be the best to return an error when an unsupported flag is provided, but the function doesn't really allow us to do that.

piodul

A few small comments remaining, otherwise LGTM.

piodul · 2022-09-14T07:28:24Z

scylla-rust-wrapper/src/ssl.rs

+        }
+        _ => {
+            // Check at position 1 the bit is set to 1
+            if (flags >> 1) & 1 == 1 {


Why use number literals when you can use constants?

if flags & CASS_SSL_VERIFY_PEER_IDENTITY_DNS != 0 { // ... }

piodul · 2022-09-14T07:40:41Z

scylla-rust-wrapper/src/ssl.rs

+        CASS_SSL_VERIFY_NONE => {
+            SSL_CTX_set_verify(ssl.ssl_context, SslVerifyMode::NONE.bits(), None)
+        }
+        CASS_SSL_VERIFY_PEER_CERT => {


Why not? If the user provides CASS_SSL_VERIFY_PEER_CERT, then we should enable the functionality provided by this flag. If they additionally provide CASS_SSL_VERIFY_PEER_IDENTITY, we should print a warning and ignore it, but I don't see why this should disable the first flag.

I agree that it would be the best to return an error when an unsupported flag is provided, but the function doesn't really allow us to do that.

Gor027 · 2022-09-20T13:25:39Z

After logging support is merged into the master, the error checking/reporting functionality can be added to the SSL-related functions.

Added SSL/TLS support to the driver. The rust driver is now imported with additional "ssl" feature as it is required to enable SSL support in it. Enabled integration tests for ssl, only those that are enabled for C++ driver too.

avelanarius requested review from jul-stas and avelanarius August 18, 2022 11:29

Gor027 force-pushed the add_ssl_support branch 2 times, most recently from daad426 to adf2370 Compare August 25, 2022 10:39

piodul requested changes Aug 25, 2022

View reviewed changes

Gor027 force-pushed the add_ssl_support branch 2 times, most recently from 69f663f to e209aa0 Compare August 29, 2022 15:52

avelanarius reviewed Aug 30, 2022

View reviewed changes

Gor027 force-pushed the add_ssl_support branch 2 times, most recently from e83d197 to 2de589a Compare September 6, 2022 14:13

Gor027 requested a review from piodul September 6, 2022 14:36

Gor027 force-pushed the add_ssl_support branch from 2de589a to ba3445d Compare September 8, 2022 08:06

piodul requested changes Sep 8, 2022

View reviewed changes

Gor027 force-pushed the add_ssl_support branch 2 times, most recently from f184658 to 2aa2697 Compare September 9, 2022 16:40

piodul reviewed Sep 12, 2022

View reviewed changes

Gor027 force-pushed the add_ssl_support branch from 2aa2697 to 9a83f6d Compare September 12, 2022 13:22

piodul reviewed Sep 14, 2022

View reviewed changes

Gor027 force-pushed the add_ssl_support branch 2 times, most recently from edd0a92 to c230fce Compare September 20, 2022 13:21

Add SSL/TLS support

7228598

Added SSL/TLS support to the driver. The rust driver is now imported with additional "ssl" feature as it is required to enable SSL support in it. Enabled integration tests for ssl, only those that are enabled for C++ driver too.

Gor027 force-pushed the add_ssl_support branch from c230fce to 7228598 Compare September 20, 2022 15:41

avelanarius merged commit eb9d9c6 into scylladb:master Sep 20, 2022

Gor027 mentioned this pull request Oct 2, 2022

CassSsl #34

Closed

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add SSL/TLS support #70

Add SSL/TLS support #70

Gor027 commented Aug 16, 2022 •

edited

Loading

piodul Aug 25, 2022

Gor027 Aug 26, 2022

piodul Aug 25, 2022

piodul Aug 25, 2022

Gor027 Aug 26, 2022

Gor027 commented Aug 29, 2022

avelanarius Aug 30, 2022

Gor027 Sep 6, 2022

piodul left a comment

piodul Sep 8, 2022

Gor027 Sep 8, 2022

piodul Sep 9, 2022

piodul Sep 8, 2022

Gor027 Sep 8, 2022

piodul Sep 9, 2022

Gor027 Sep 9, 2022 •

edited

Loading

piodul Sep 9, 2022

piodul Sep 12, 2022

Gor027 Sep 12, 2022

piodul Sep 14, 2022

piodul left a comment

piodul Sep 14, 2022

piodul Sep 14, 2022

Gor027 commented Sep 20, 2022


		let mut ssl_context_builder = SslContextBuilder::new(SslMethod::tls_client()).unwrap();

		if cass_ssl.trusted_cert.is_some() {

		let trusted_store: *mut X509_STORE = X509_STORE_new();

		SSL_CTX_set_cert_store(ssl_context, trusted_store);

Add SSL/TLS support #70

Add SSL/TLS support #70

Conversation

Gor027 commented Aug 16, 2022 • edited Loading

Pre-review checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gor027 commented Aug 29, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piodul left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gor027 Sep 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piodul left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gor027 commented Sep 20, 2022

Gor027 commented Aug 16, 2022 •

edited

Loading

Gor027 Sep 9, 2022 •

edited

Loading