Skip to content

Commit 8600604

Browse files
mpenickmpenick
authored
CPP-825 Cloud should be verifying the peer certificate's CN (#308)
* Fix bug in OpenSSL DNS verification code where certain certificate's CN wouldn't be matched correctly * Fix bug in Socket DNS unit tests * Fix client certificate validation in unit tests
1 parent f5b008f commit 8600604

11 files changed

Lines changed: 226 additions & 119 deletions

‎cpp-driver/gtests/src/unit/http_server.cpp‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -57,9 +57,9 @@ void Server::close() {
5757
}
5858
}
5959

60-
bool Server::use_ssl(const String& key, const String& cert, const String& password /*= ""*/,
61-
const String& client_cert /*= ""*/) {
62-
return server_connection_->use_ssl(key, cert, password, client_cert);
60+
bool Server::use_ssl(const String& key, const String& cert, const String& ca_cert /*= ""*/,
61+
bool require_client_cert /*= false*/) {
62+
return server_connection_->use_ssl(key, cert, ca_cert, require_client_cert);
6363
}
6464

6565
Server::ClientConnection::ClientConnection(internal::ServerConnection* server_connection,

‎cpp-driver/gtests/src/unit/http_server.hpp‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,8 @@
1717
#ifndef HTTP_MOCK_SERVER_HPP
1818
#define HTTP_MOCK_SERVER_HPP
1919

20-
#define HTTP_MOCK_SERVER_IP "127.0.0.1"
20+
#define HTTP_MOCK_HOSTNAME "cpp-driver.hostname."
21+
#define HTTP_MOCK_SERVER_IP "127.254.254.254"
2122
#define HTTP_MOCK_SERVER_PORT 30443
2223

2324
#include "http_parser.h"
@@ -61,8 +62,8 @@ class Server {
6162
close_connnection_after_request_ = enable;
6263
}
6364

64-
bool use_ssl(const String& key, const String& cert, const String& password = "",
65-
const String& client_cert = "");
65+
bool use_ssl(const String& key, const String& cert, const String& ca_cert = "",
66+
bool require_client_cert = false);
6667

6768
void listen();
6869
void close();

‎cpp-driver/gtests/src/unit/http_test.cpp‎

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,61 @@
1+
/*
2+
Copyright (c) DataStax, Inc.
3+
4+
Licensed under the Apache License, Version 2.0 (the "License");
5+
you may not use this file except in compliance with the License.
6+
You may obtain a copy of the License at
7+
8+
http://www.apache.org/licenses/LICENSE-2.0
9+
10+
Unless required by applicable law or agreed to in writing, software
11+
distributed under the License is distributed on an "AS IS" BASIS,
12+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
See the License for the specific language governing permissions and
14+
limitations under the License.
15+
*/
16+
17+
#include "http_test.hpp"
18+
19+
using namespace datastax;
20+
using namespace datastax::internal::core;
21+
22+
SocketSettings HttpTest::use_ssl(const String& cn, bool is_server_using_ssl /*= true*/) {
23+
SocketSettings settings;
24+
25+
#ifdef HAVE_OPENSSL
26+
String ca_key = mockssandra::Ssl::generate_key();
27+
ca_cert_ = mockssandra::Ssl::generate_cert(ca_key, "CA");
28+
29+
key_ = mockssandra::Ssl::generate_key();
30+
cert_ = mockssandra::Ssl::generate_cert(key_, cn, ca_cert_, ca_key);
31+
32+
String client_key = mockssandra::Ssl::generate_key();
33+
String client_cert = mockssandra::Ssl::generate_cert(client_key, cn, ca_cert_, ca_key);
34+
35+
SslContext::Ptr ssl_context(SslContextFactory::create());
36+
37+
ssl_context->set_cert(client_cert.c_str(), client_cert.size());
38+
ssl_context->set_private_key(client_key.c_str(), client_key.size(), "",
39+
0); // No password expected for the private key
40+
41+
ssl_context->add_trusted_cert(ca_cert_.c_str(), ca_cert_.size());
42+
43+
settings.ssl_context = ssl_context;
44+
45+
if (is_server_using_ssl) {
46+
server_.use_ssl(key_, cert_, ca_cert_, true);
47+
}
48+
#endif
49+
50+
return settings;
51+
}
52+
53+
void HttpTest::use_ssl(const String& ca_cert, const String& ca_key, const String& cn) {
54+
#ifdef HAVE_OPENSSL
55+
key_ = mockssandra::Ssl::generate_key();
56+
cert_ = mockssandra::Ssl::generate_cert(key_, cn, ca_cert, ca_key);
57+
ca_cert_ = ca_cert;
58+
59+
server_.use_ssl(key_, cert_, ca_cert_, true);
60+
#endif
61+
}

‎cpp-driver/gtests/src/unit/http_test.hpp‎

Lines changed: 4 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -50,47 +50,16 @@ class HttpTest : public LoopTest {
5050
void start_http_server() { server_.listen(); }
5151
void stop_http_server() { server_.close(); }
5252

53-
datastax::internal::core::SocketSettings use_ssl(String cn = "127.0.0.1",
54-
bool is_server_using_ssl = true) {
55-
datastax::internal::core::SocketSettings settings;
56-
57-
#ifdef HAVE_OPENSSL
58-
datastax::String ca_key = mockssandra::Ssl::generate_key();
59-
ca_cert_ = mockssandra::Ssl::generate_cert(ca_key, cn);
60-
key_ = mockssandra::Ssl::generate_key();
61-
cert_ = mockssandra::Ssl::generate_cert(key_, cn, ca_cert_, ca_key);
62-
63-
datastax::internal::core::SslContext::Ptr ssl_context(
64-
datastax::internal::core::SslContextFactory::create());
65-
ssl_context->set_cert(cert().c_str(), cert().size());
66-
ssl_context->set_private_key(key().c_str(), key().size(), "",
67-
0); // No password expected for the private key
68-
ssl_context->add_trusted_cert(ca_cert().c_str(), ca_cert().size());
69-
70-
settings.ssl_context = ssl_context;
71-
72-
if (is_server_using_ssl) {
73-
server_.use_ssl(ca_key, ca_cert_, "", cert_);
74-
}
75-
#endif
76-
77-
return settings;
78-
}
53+
datastax::internal::core::SocketSettings use_ssl(const String& cn = HTTP_MOCK_HOSTNAME,
54+
bool is_server_using_ssl = true);
7955

80-
void use_ssl(const String& key, const String& cert, const String& ca_key, const String& ca_cert) {
81-
#ifdef HAVE_OPENSSL
82-
key_ = key;
83-
cert_ = cert;
84-
ca_cert_ = ca_cert;
85-
86-
server_.use_ssl(ca_key, ca_cert_, "", cert_);
87-
#endif
88-
}
56+
void use_ssl(const String& ca_cert, const String& ca_key, const String& cn);
8957

9058
private:
9159
datastax::String ca_cert_;
9260
datastax::String cert_;
9361
datastax::String key_;
62+
9463
mockssandra::http::Server server_;
9564
};
9665

‎cpp-driver/gtests/src/unit/mockssandra.cpp‎

Lines changed: 60 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727

2828
#include <openssl/bio.h>
2929
#include <openssl/dh.h>
30+
#include <openssl/x509v3.h>
3031

3132
#ifdef WIN32
3233
#include "winsock.h"
@@ -157,6 +158,8 @@ String Ssl::generate_cert(const String& key, String cn, String ca_cert, String c
157158
{ // Read CA from string
158159
BIO* bio = BIO_new_mem_buf(const_cast<char*>(ca_cert.c_str()), ca_cert.length());
159160
if (!PEM_read_bio_X509(bio, &x509_ca, NULL, NULL)) {
161+
X509_free(x509);
162+
X509_REQ_free(x509_req);
160163
BIO_free(bio);
161164
return "";
162165
}
@@ -169,6 +172,9 @@ String Ssl::generate_cert(const String& key, String cn, String ca_cert, String c
169172
BIO* bio = BIO_new_mem_buf(const_cast<char*>(ca_key.c_str()), ca_key.length());
170173
if (!PEM_read_bio_PrivateKey(bio, &pkey_ca, NULL, NULL)) {
171174
BIO_free(bio);
175+
X509_free(x509);
176+
X509_free(x509_ca);
177+
X509_REQ_free(x509_req);
172178
return "";
173179
}
174180
BIO_free(bio);
@@ -178,6 +184,21 @@ String Ssl::generate_cert(const String& key, String cn, String ca_cert, String c
178184
X509_free(x509_ca);
179185
EVP_PKEY_free(pkey_ca);
180186
} else {
187+
if (cn == "CA") { // Set the purpose as a CA certificate.
188+
X509_EXTENSION* x509_ex;
189+
X509V3_CTX x509v3_ctx;
190+
X509V3_set_ctx_nodb(&x509v3_ctx);
191+
X509V3_set_ctx(&x509v3_ctx, x509, x509, NULL, NULL, 0);
192+
x509_ex = X509V3_EXT_conf_nid(NULL, &x509v3_ctx, NID_basic_constraints,
193+
const_cast<char*>("critical,CA:TRUE"));
194+
if (!x509_ex) {
195+
X509_free(x509);
196+
X509_EXTENSION_free(x509_ex);
197+
return "";
198+
}
199+
X509_add_ext(x509, x509_ex, -1);
200+
X509_EXTENSION_free(x509_ex);
201+
}
181202
X509_NAME* name = X509_get_subject_name(x509);
182203
X509_NAME_add_entry_by_txt(name, "C", MBSTRING_ASC,
183204
reinterpret_cast<const unsigned char*>("US"), -1, -1, 0);
@@ -214,6 +235,18 @@ static void print_ssl_error() {
214235
fprintf(stderr, "%s\n", buf);
215236
}
216237

238+
static X509* load_cert(const String& cert) {
239+
X509* x509 = NULL;
240+
BIO* bio = BIO_new_mem_buf(const_cast<char*>(cert.c_str()), cert.length());
241+
if (PEM_read_bio_X509(bio, &x509, NULL, NULL) == NULL) {
242+
print_ssl_error();
243+
BIO_free(bio);
244+
return NULL;
245+
}
246+
BIO_free(bio);
247+
return x509;
248+
}
249+
217250
struct WriteReq {
218251
WriteReq(const char* data, size_t len, ClientConnection* connection)
219252
: data(data, len)
@@ -450,8 +483,8 @@ uv_loop_t* ServerConnection::loop() {
450483
}
451484

452485
bool ServerConnection::use_ssl(const String& key, const String& cert,
453-
const String& password /*= ""*/,
454-
const String& client_cert /*= ""*/) {
486+
const String& ca_cert /*= ""*/,
487+
bool require_client_cert /*= false*/) {
455488
if (ssl_context_) {
456489
SSL_CTX_free(ssl_context_);
457490
}
@@ -461,31 +494,43 @@ bool ServerConnection::use_ssl(const String& key, const String& cert,
461494
return false;
462495
}
463496

464-
SSL_CTX_set_default_passwd_cb_userdata(ssl_context_, (void*)password.c_str());
497+
SSL_CTX_set_default_passwd_cb_userdata(ssl_context_, (void*)"");
465498
SSL_CTX_set_default_passwd_cb(ssl_context_, on_password);
499+
SSL_CTX_set_verify(ssl_context_, SSL_VERIFY_NONE, NULL);
466500

467-
X509* x509 = NULL;
468-
{ // Read cert from string
469-
BIO* bio = BIO_new_mem_buf(const_cast<char*>(cert.c_str()), cert.length());
470-
if (PEM_read_bio_X509(bio, &x509, NULL, NULL) == NULL) {
501+
{ // Load server certificate
502+
X509* x509 = load_cert(cert);
503+
if (!x509) return false;
504+
if (SSL_CTX_use_certificate(ssl_context_, x509) <= 0) {
471505
print_ssl_error();
472-
BIO_free(bio);
506+
X509_free(x509);
473507
return false;
474508
}
475-
BIO_free(bio);
509+
X509_free(x509);
476510
}
477511

478-
if (SSL_CTX_use_certificate(ssl_context_, x509) <= 0) {
479-
print_ssl_error();
480-
X509_free(x509);
481-
return false;
512+
if (!ca_cert.empty()) { // Load CA certificate
513+
X509* x509 = load_cert(ca_cert);
514+
if (!x509) return false;
515+
if (SSL_CTX_add_extra_chain_cert(ssl_context_, x509) <= 0) { // Certificate freed by function
516+
print_ssl_error();
517+
X509_free(x509);
518+
return false;
519+
}
520+
if (require_client_cert) {
521+
X509_STORE* cert_store = SSL_CTX_get_cert_store(ssl_context_);
522+
if (X509_STORE_add_cert(cert_store, x509) <= 0) {
523+
print_ssl_error();
524+
return false;
525+
}
526+
SSL_CTX_set_verify(ssl_context_, SSL_VERIFY_PEER | SSL_VERIFY_FAIL_IF_NO_PEER_CERT, NULL);
527+
}
482528
}
483-
X509_free(x509);
484529

485530
EVP_PKEY* pkey = NULL;
486531
{ // Read key from string
487532
BIO* bio = BIO_new_mem_buf(const_cast<char*>(key.c_str()), key.length());
488-
if (PEM_read_bio_PrivateKey(bio, &pkey, on_password, (void*)password.c_str()) == NULL) {
533+
if (PEM_read_bio_PrivateKey(bio, &pkey, on_password, (void*)"") == NULL) {
489534
print_ssl_error();
490535
BIO_free(bio);
491536
return false;
@@ -508,43 +553,6 @@ bool ServerConnection::use_ssl(const String& key, const String& cert,
508553
}
509554
DH_free(dh);
510555

511-
if (!client_cert.empty()) {
512-
X509_STORE* trust_store = SSL_CTX_get_cert_store(static_cast<const SSL_CTX*>(ssl_context_));
513-
STACK_OF(X509_INFO) * stack_info;
514-
515-
{ // Read cert from string
516-
BIO* bio = BIO_new_mem_buf(const_cast<char*>(client_cert.c_str()), client_cert.size());
517-
if ((stack_info = PEM_X509_INFO_read_bio(bio, NULL, NULL, NULL)) == NULL) {
518-
print_ssl_error();
519-
BIO_free(bio);
520-
return false;
521-
}
522-
BIO_free(bio);
523-
}
524-
525-
if (stack_info) {
526-
for (int i = 0; i < sk_X509_INFO_num(stack_info); i++) {
527-
X509_INFO* info = sk_X509_INFO_value(stack_info, i);
528-
if (info->x509) {
529-
if (X509_STORE_add_cert(trust_store, info->x509) <= 0) {
530-
print_ssl_error();
531-
return false;
532-
}
533-
}
534-
if (info->crl) {
535-
if (X509_STORE_add_crl(trust_store, info->crl) <= -0) {
536-
print_ssl_error();
537-
return false;
538-
}
539-
}
540-
}
541-
}
542-
543-
sk_X509_INFO_pop_free(stack_info, X509_INFO_free);
544-
}
545-
546-
SSL_CTX_set_verify(ssl_context_, SSL_VERIFY_NONE, 0);
547-
548556
return true;
549557
}
550558

‎cpp-driver/gtests/src/unit/mockssandra.hpp‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -174,8 +174,8 @@ class ServerConnection : public RefCounted<ServerConnection> {
174174
SSL_CTX* ssl_context() { return ssl_context_; }
175175
const ClientConnections& clients() const { return clients_; }
176176

177-
bool use_ssl(const String& key, const String& cert, const String& password = "",
178-
const String& client_cert = "");
177+
bool use_ssl(const String& key, const String& cert, const String& ca_cert = "",
178+
bool require_client_cert = false);
179179

180180
void listen(EventLoopGroup* event_loop_group);
181181
int wait_listen();

‎cpp-driver/gtests/src/unit/tests/test_cloud_secure_connect_config.cpp‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -47,9 +47,9 @@
4747
#endif
4848

4949
#define SNI_LOCAL_DC "dc1"
50-
#define SNI_HOST "localhost"
50+
#define SNI_HOST HTTP_MOCK_HOSTNAME
5151
#define SNI_PORT 30002
52-
#define SNI_HOST_AND_PORT "localhost:30002"
52+
#define SNI_HOST_AND_PORT HTTP_MOCK_HOSTNAME ":30002"
5353
#define SNI_HOST_ID_1 "276b1694-64c4-4ba8-afb4-e33915a02f1e"
5454
#define SNI_HOST_ID_2 "8c29f723-5c1c-4ffd-a4ef-8c683a7fc02b"
5555
#define SNI_HOST_ID_3 "fb91d3ff-47cb-447d-b31d-c5721ca8d7ab"
@@ -91,9 +91,9 @@ class CloudSecureConnectionConfigTest : public HttpTest {
9191
tmp_zip_file_ = String(tmp, tmp_length) + PATH_SEPARATOR + CREDS_V1_ZIP_FILE;
9292

9393
ca_key_ = Ssl::generate_key();
94-
ca_cert_ = Ssl::generate_cert(ca_key_);
94+
ca_cert_ = Ssl::generate_cert(ca_key_, "CA");
9595
key_ = Ssl::generate_key();
96-
cert_ = Ssl::generate_cert(key_, "localhost", ca_cert_, ca_key_);
96+
cert_ = Ssl::generate_cert(key_, "", ca_cert_, ca_key_);
9797
}
9898

9999
const String& creds_zip_file() const { return tmp_zip_file_; }
@@ -335,11 +335,11 @@ class CloudMetadataServerTest : public CloudSecureConnectionConfigTest {
335335
CloudSecureConnectionConfigTest::SetUp();
336336

337337
StringBuffer buffer;
338-
full_config_credsv1(buffer, HTTP_MOCK_SERVER_IP, HTTP_MOCK_SERVER_PORT);
338+
full_config_credsv1(buffer, HTTP_MOCK_HOSTNAME, HTTP_MOCK_SERVER_PORT);
339339
create_zip_file(buffer.GetString());
340340
cloud_config_.load(creds_zip_file(), &config_);
341341

342-
use_ssl(key(), cert(), ca_key(), ca_cert()); // Ensure HttpServer is configured to use SSL
342+
use_ssl(ca_cert(), ca_key(), HTTP_MOCK_HOSTNAME); // Ensure HttpServer is configured to use SSL
343343

344344
ClusterSettings settings(config_);
345345
resolver_ = config_.cluster_metadata_resolver_factory()->new_instance(settings);

0 commit comments

Comments
 (0)