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

Fixes for CPP-928, and trusted certs #493

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

maxdymond
Copy link
Contributor

This PR consists of two commits:


CPP-928 Ensure server name information flows through from contact point configuration

Previously during Address name resolution, the server name information for a given Address
was lost. This fix ensures that the server name information flows through during the name
resolution process for a given Address.


Iterate over all certificates in a trusted cert BIO, not just the first

Previously the code which loaded a trusted certificate from file only
assumed that there was a single certificate in that file, meaning that
using a certificate bundle for certificate verification would not work.

This fix allows the driver to read multiple trusted certificates out
of a BIO and provision them in the trusted certificate store.


Please let me know if you have any comments!

…nt configuration

Previously during Address name resolution, the server name information for a given Address
was lost. This fix ensures that the server name information flows through during the name
resolution process for a given Address.
Previously the code which loaded a trusted certificate from file only
assumed that there was a single certificate in that file, meaning that
using a certificate bundle for certificate verification would not work.

This fix allows the driver to read multiple trusted certificates out
of a BIO and provision them in the trusted certificate store.
@mpenick
Copy link
Contributor

mpenick commented Mar 10, 2021

Hey, I think domain name verification is working as intended. The driver uses reverse DNS to lookup the name for certificate validation (which is a side effect of Cassandra using IP addresses internally). We have some SSL documentation that makes the use of those flags a bit clearer. I agree the header docs could do a better job of explaining those.

Cassandra uses IP addresses internally so those can be used directly for verification or a domain name can be used via reverse DNS (PTR record).

CassSsl* ssl = cass_ssl_new();

// CASS_SSL_VERIFY_PEER_IDENTITY_DNS (domain name)
cass_ssl_set_verify_flags(ssl, CASS_SSL_VERIFY_PEER_CERT | CASS_SSL_VERIFY_PEER_IDENTITY_DNS);

CassCluster* cluster = cass_cluster_new();

// Enable reverse DNS
cass_cluster_set_use_hostname_resolution(cluster, cass_true);

// ...

* Notes:
* - CASS_SSL_VERIFY_PEER_IDENTITY and CASS_SSL_VERIFY_PEER_IDENTITY_DNS are
* mutually exclusive options.
* - The certificate Common Name is only checked against the IP address or
Copy link
Contributor

Choose a reason for hiding this comment

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

The certificate Common Name is only checked against the IP address or hostname if there are no Subject Alternative Names in the certificate.

Maybe this is a bug. If it's present in a SAN or the CN the it should probably be valid? I'd need to dig into that more.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From ssl_openssl_impl.cpp

  static Result match(X509* cert, const Address& address) {
    Result result = match_subject_alt_names_ipadd(cert, address);
    if (result == NO_SAN_PRESENT) {
      result = match_common_name_ipaddr(cert, address.hostname_or_address());
    }
    return result;
  }

  static Result match_dns(X509* cert, const String& hostname) {
    Result result = match_subject_alt_names_dns(cert, hostname);
    if (result == NO_SAN_PRESENT) {
      result = match_common_name_dns(cert, hostname);
    }
    return result;
  }

Those functions only return NO_SAN_PRESENT if X509_get_ext_d2i returns NULL, otherwise it then checks the SAN stack and returns either NO_MATCH, INVALID_CERT or MATCH.

@@ -556,13 +540,30 @@ SslSession* OpenSslContext::create_session(const Address& address, const String&
}

CassError OpenSslContext::add_trusted_cert(const char* cert, size_t cert_length) {
X509* x509 = load_cert(cert, cert_length);
if (x509 == NULL) {
BIO* bio = BIO_new_mem_buf(const_cast<char*>(cert), cert_length);
Copy link
Contributor

Choose a reason for hiding this comment

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

+1 on loading the whole cert chain. Awesome!

@kw217
Copy link
Contributor

kw217 commented Jan 10, 2022

@mpenick @maxdymond Happy New Year! I've pulled out the uncontroversial part of this PR (the change to read all certs) into a separate PR; @mpenick please can you re-review and merge? It's just a cherry-pick, no code changes since you reviewed above. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants