Commit 0121c4ee authored by Mingtao Yang's avatar Mingtao Yang Committed by Facebook GitHub Bot

SSLContext: rename misleading loadClientCAList

Summary:
`loadClientCAList`, unlike `loadTrustedCertificates`, does not *append*
"client ca"s to the current context, it *overrides* the list of acceptable
client CA names.

`load` vs `set` have subtle, but recognizable, semantic differences. There
is a currently a bug in wangle::SSLContextManager that could have been
prevented if the name was more wisely chosen.

Rename to `setSupportedClientCertificateAuthorityNames`, which is verbose but
* Makes the `load` vs `set` distinction clear.
* Makes it clear that this has nothing to do with changing how client CAs are
  configured. All this does is set a list of supported names; nothing more.
  Existing call sites imply that authors misunderstood what this function
  was doing.

Reviewed By: kylemirz

Differential Revision: D32408104

fbshipit-source-id: acf55c2e0b206e28e13e5922d719b43289a1c0f7
parent 81f781c5
......@@ -499,13 +499,29 @@ void SSLContext::loadTrustedCertificates(X509_STORE* store) {
SSL_CTX_set_cert_store(ctx_, store);
}
void SSLContext::loadClientCAList(const char* path) {
auto clientCAs = SSL_load_client_CA_file(path);
if (clientCAs == nullptr) {
LOG(ERROR) << "Unable to load ca file: " << path << " " << getErrors();
return;
void SSLContext::setSupportedClientCertificateAuthorityNames(
std::vector<ssl::X509NameUniquePtr> names) {
// SSL_CTX_set_client_CA_list *takes ownership* of a STACK_OF(X509_NAME)
// where each pointer in the STACK is an *owned* X509.
ssl::OwningStackOfX509NameUniquePtr nameList(sk_X509_NAME_new(nullptr));
if (!nameList) {
throw std::runtime_error(
"SSLContext::setSupportedClientCertificateAuthorityNames failed to allocate name list");
}
// Release ownership of the X509_NAME into the stack.
//
// If exceptions are thrown, all elements are properly cleaned up because of
// the *UniquePtr wrappers.
for (auto& name : names) {
if (!sk_X509_NAME_push(nameList.get(), name.release())) {
throw std::runtime_error(
"SSLContext::setSupportedClientCertificateAuthorityNames failed to add X509_NAME");
}
SSL_CTX_set_client_CA_list(ctx_, clientCAs);
}
// And finally pass ownership over the whole X509_NAME stack to OpenSSL
SSL_CTX_set_client_CA_list(ctx_, nameList.release());
}
void SSLContext::passwordCollector(
......
......@@ -398,11 +398,48 @@ class SSLContext {
* @param store X509 certificate store.
*/
virtual void loadTrustedCertificates(X509_STORE* store);
/**
* Load a client CA list for validating clients
* setSupportedClientCertificateAuthorityNames sets the list of acceptable
* client certificate authoritites that will be sent to the client.
*
* This corresponds to the `certificate_authorities` extension.
*
* This function does *not* alter the way client authentication is performed
* in any discernible manner.
*
* Certain TLS client implementations will use this list of names to aid in
* the client certificate selection process.
*
* folly::AsyncSSLSocket, which is based on OpenSSL, in particular will
* *not* use this information. folly::AsyncSSLSocket will send client
* certificates to whatever `SSLContext::loadCertificate` points to,
* regardless of what the server sends in `certificate_authorities`.
*
* @param names A vector of X509_NAMEs to send. This typically corresponds
* to the Subject of each client certificate authority used
* in the trust store.
* `OpenSSLUtil::loadNamesFromFile`.
* @throws std::exception
*/
virtual void loadClientCAList(const char* path);
void setSupportedClientCertificateAuthorityNames(
std::vector<ssl::X509NameUniquePtr> names);
/**
* setSupportedClientCertificateAuthorityNamesFromFile sets the list of
* acceptable client certificate authorities that will be sent to the client.
*
* See `SSLContext::setSupportedClientCertificateAuthorityNames`.
*
* @param path Path to a file containing PEM encoded X509 certificates.
* @throws std::exception
*/
void setSupportedClientCertificateAuthorityNamesFromFile(const char* path) {
return setSupportedClientCertificateAuthorityNames(
ssl::OpenSSLUtils::subjectNamesInPEMFile(path));
}
/*
* Override default OpenSSL password collector.
*
* @param collector Instance of user defined password collector
......
......@@ -347,6 +347,54 @@ std::string OpenSSLUtils::encodeALPNString(
return encodedALPN;
}
/**
* Deserializes PEM encoded X509 objects from the supplied source BIO, invoking
* a callback for each X509, until the BIO is exhausted or until we were unable
* to read an X509.
*/
template <class Callback>
static void forEachX509(BIO* source, Callback cb) {
while (true) {
ssl::X509UniquePtr x509(
PEM_read_bio_X509(source, nullptr, nullptr, nullptr));
if (x509 == nullptr) {
ERR_clear_error();
break;
}
cb(std::move(x509));
}
}
static std::vector<X509NameUniquePtr> getSubjectNamesFromBIO(BIO* b) {
std::vector<X509NameUniquePtr> ret;
forEachX509(b, [&](auto&& name) {
// X509_get_subject_name borrows the X509_NAME, so we must dup it.
ret.push_back(
X509NameUniquePtr(X509_NAME_dup(X509_get_subject_name(name.get()))));
});
return ret;
}
std::vector<X509NameUniquePtr> OpenSSLUtils::subjectNamesInPEMFile(
const char* filename) {
BioUniquePtr bio(BIO_new_file(filename, "r"));
if (!bio) {
throw std::runtime_error(
"OpenSSLUtils::subjectNamesInPEMFile: failed to open file");
}
return getSubjectNamesFromBIO(bio.get());
}
std::vector<X509NameUniquePtr> OpenSSLUtils::subjectNamesInPEMBuffer(
folly::ByteRange buffer) {
BioUniquePtr bio(BIO_new_mem_buf(buffer.data(), buffer.size()));
if (!bio) {
throw std::runtime_error(
"OpenSSLUtils::subjectNamesInPEMBuffer: failed to create BIO");
}
return getSubjectNamesFromBIO(bio.get());
}
} // namespace ssl
} // namespace folly
......
......@@ -112,6 +112,33 @@ class OpenSSLUtils {
*/
static std::string getCommonName(X509* x509);
/**
* Get a list of subject names corresponding to each certificate in a
* PEM encoded file.
*
* @param filename Path to a file containing PEM encoded X509 certificates.
* @return A vector of X509_NAMEs corresponding to the subject of each
* certificate.
* @throws std::exception For any errors encountered while reading in
* certificates from the file.
*/
static std::vector<X509NameUniquePtr> subjectNamesInPEMFile(
const char* filename);
/**
* Get a list of subject names corresponding to each certificate in a
* buffer containing PEM data.
*
* @param buffer A contiguous region of memory containing PEM encoded
* certificates.
* @return A vector of X509_NAMEs corresponding to the subject of each
* certificate.
* @throws std::exception For any errors encountered while reading in
* certificates from the file.
*/
static std::vector<X509NameUniquePtr> subjectNamesInPEMBuffer(
folly::ByteRange buffer);
/**
* Wrappers for BIO operations that may be different across different
* versions/flavors of OpenSSL (including forks like BoringSSL)
......@@ -129,6 +156,5 @@ class OpenSSLUtils {
static std::string encodeALPNString(
const std::vector<std::string>& supported_protocols);
};
} // namespace ssl
} // namespace folly
......@@ -1057,7 +1057,7 @@ TEST(AsyncSSLSocketTest, SSLParseClientHelloSuccess) {
serverCtx->loadPrivateKey(kTestKey);
serverCtx->loadCertificate(kTestCert);
serverCtx->loadTrustedCertificates(kTestCA);
serverCtx->loadClientCAList(kTestCA);
serverCtx->setSupportedClientCertificateAuthorityNamesFromFile(kTestCA);
clientCtx->setVerificationOption(SSLContext::SSLVerifyPeerEnum::VERIFY);
clientCtx->ciphers("AES256-SHA:AES128-SHA");
......@@ -1104,7 +1104,7 @@ TEST(AsyncSSLSocketTest, GetClientCertificate) {
serverCtx->loadPrivateKey(kTestKey);
serverCtx->loadCertificate(kTestCert);
serverCtx->loadTrustedCertificates(kClientTestCA);
serverCtx->loadClientCAList(kClientTestCA);
serverCtx->setSupportedClientCertificateAuthorityNamesFromFile(kClientTestCA);
clientCtx->setVerificationOption(SSLContext::SSLVerifyPeerEnum::VERIFY);
clientCtx->ciphers("AES256-SHA:AES128-SHA");
......@@ -1711,7 +1711,7 @@ TEST(AsyncSSLSocketTest, OverrideSSLCtxEnableVerify) {
serverCtx->loadPrivateKey(kTestKey);
serverCtx->loadCertificate(kTestCert);
serverCtx->loadTrustedCertificates(kTestCA);
serverCtx->loadClientCAList(kTestCA);
serverCtx->setSupportedClientCertificateAuthorityNamesFromFile(kTestCA);
clientCtx->setVerificationOption(SSLContext::SSLVerifyPeerEnum::NO_VERIFY);
clientCtx->ciphers("ALL:!ADH:!LOW:!EXP:!MD5:@STRENGTH");
......@@ -1828,7 +1828,7 @@ TEST(AsyncSSLSocketTest, ClientCertHandshakeSuccess) {
serverCtx->loadPrivateKey(kTestKey);
serverCtx->loadCertificate(kTestCert);
serverCtx->loadTrustedCertificates(kTestCA);
serverCtx->loadClientCAList(kTestCA);
serverCtx->setSupportedClientCertificateAuthorityNamesFromFile(kTestCA);
clientCtx->setVerificationOption(SSLContext::SSLVerifyPeerEnum::VERIFY);
clientCtx->ciphers("ALL:!ADH:!LOW:!EXP:!MD5:@STRENGTH");
......@@ -1889,7 +1889,7 @@ TEST(AsyncSSLSocketTest, NoClientCertHandshakeError) {
serverCtx->loadPrivateKey(kTestKey);
serverCtx->loadCertificate(kTestCert);
serverCtx->loadTrustedCertificates(kTestCA);
serverCtx->loadClientCAList(kTestCA);
serverCtx->setSupportedClientCertificateAuthorityNamesFromFile(kTestCA);
clientCtx->setVerificationOption(SSLContext::SSLVerifyPeerEnum::NO_VERIFY);
clientCtx->ciphers("ALL:!ADH:!LOW:!EXP:!MD5:@STRENGTH");
......@@ -2106,7 +2106,7 @@ TEST(AsyncSSLSocketTest, OpenSSL110AsyncTest) {
serverCtx->ciphers("ALL:!ADH:!LOW:!EXP:!MD5:@STRENGTH");
serverCtx->loadCertificate(kTestCert);
serverCtx->loadTrustedCertificates(kTestCA);
serverCtx->loadClientCAList(kTestCA);
serverCtx->setSupportedClientCertificateAuthorityNamesFromFile(kTestCA);
auto rsaPointers =
setupCustomRSA(kTestCert, kTestKey, jobEvbThread.getEventBase());
......@@ -2145,7 +2145,7 @@ TEST(AsyncSSLSocketTest, OpenSSL110AsyncTestFailure) {
serverCtx->ciphers("ALL:!ADH:!LOW:!EXP:!MD5:@STRENGTH");
serverCtx->loadCertificate(kTestCert);
serverCtx->loadTrustedCertificates(kTestCA);
serverCtx->loadClientCAList(kTestCA);
serverCtx->setSupportedClientCertificateAuthorityNamesFromFile(kTestCA);
// Set the wrong key for the cert
auto rsaPointers =
setupCustomRSA(kTestCert, kClientTestKey, jobEvbThread.getEventBase());
......@@ -2184,7 +2184,7 @@ TEST(AsyncSSLSocketTest, OpenSSL110AsyncTestClosedWithCallbackPending) {
serverCtx->ciphers("ALL:!ADH:!LOW:!EXP:!MD5:@STRENGTH");
serverCtx->loadCertificate(kTestCert);
serverCtx->loadTrustedCertificates(kTestCA);
serverCtx->loadClientCAList(kTestCA);
serverCtx->setSupportedClientCertificateAuthorityNamesFromFile(kTestCA);
auto rsaPointers =
setupCustomRSA(kTestCert, kTestKey, jobEvbThread->getEventBase());
......
......@@ -19,6 +19,7 @@
#include <folly/FileUtil.h>
#include <folly/io/async/test/SSLUtil.h>
#include <folly/portability/GTest.h>
#include <folly/portability/OpenSSL.h>
#include <folly/ssl/OpenSSLPtrTypes.h>
using namespace std;
......@@ -185,6 +186,29 @@ TEST_F(SSLContextTest, TestLoadCertificateChain) {
EXPECT_EQ(1, sk_X509_num(stack));
}
TEST_F(SSLContextTest, TestSetSupportedClientCAs) {
constexpr auto kCertChainPath = "folly/io/async/test/certs/client_chain.pem";
ctx.setSupportedClientCertificateAuthorityNamesFromFile(kCertChainPath);
STACK_OF(X509_NAME)* names = SSL_CTX_get_client_CA_list(ctx.getSSLCtx());
EXPECT_EQ(2, sk_X509_NAME_num(names));
static const char* kExpectedCNs[] = {"Leaf Certificate", "Intermediate CA"};
for (int i = 0; i < sk_X509_NAME_num(names); i++) {
auto name = sk_X509_NAME_value(names, i);
int indexCN = X509_NAME_get_index_by_NID(name, NID_commonName, -1);
EXPECT_NE(indexCN, -1);
auto entry = X509_NAME_get_entry(name, indexCN);
ASSERT_NE(entry, nullptr);
auto asnStringCN = X509_NAME_ENTRY_get_data(entry);
std::string commonName(
reinterpret_cast<const char*>(ASN1_STRING_get0_data(asnStringCN)),
ASN1_STRING_length(asnStringCN));
EXPECT_EQ(commonName, std::string(kExpectedCNs[i]));
}
}
TEST_F(SSLContextTest, TestGetFromSSLCtx) {
// Positive test
SSLContext* contextPtr = SSLContext::getFromSSLCtx(ctx.getSSLCtx());
......
......@@ -121,6 +121,115 @@ FOLLY_SSL_DETAIL_DEFINE_PTR_TYPE(BNCtx, BN_CTX, BN_CTX_free);
FOLLY_SSL_DETAIL_DEFINE_PTR_TYPE(SSL, SSL, SSL_free);
FOLLY_SSL_DETAIL_DEFINE_PTR_TYPE(SSLSession, SSL_SESSION, SSL_SESSION_free);
// OpenSSL STACK_OF(T) can both represent owned or borrowed values.
//
// This isn't represented in the OpenSSL "safestack" type (e.g. STACK_OF(Foo)).
// Whether or not a STACK is owning or borrowing is determined purely based on
// which destructor is called.
// * sk_T_free - This only deallocates the STACK, but does not free the
// contained items within. This is the "borrowing" version.
// * sk_T_pop_free - This deallocates both the STACK and it invokes a free
// function for each element. This is the "owning" version.
//
// The below macro,
// FOLLY_SSL_DETAIL_DEFINE_STACK_PTR_TYPE(Alias, Element)
//
// can be used to define two unique_ptr types that correspond to the "owned"
// or "borrowing" version of each.
//
// For example,
// FOLLY_SSL_DETAIL_DEFINE_STACK_PTR_TYPE(X509Name, X509_NAME)
// creates two unique ptr type aliases
// * OwningStackOfX509NameUniquePtr
// * BorrowingStackOfX509NameUniquePtr
// which corresponds to a unique_ptr of `STACK_OF(X509_NAME)` that will invoke
// the appropriate destructor:
// * OwningStackOf* -> Invokes sk_T_free
// * BorrowingStackOf* -> Invokes sk_T_pop_free
//
// IMPLEMENTATION NOTES:
// * While the _current_ implementation of OpenSSL's STACK_OF APIs utilize
// type erased OPENSSL_sk_* APIs, this is technically an implementation
// detail. It is tempting to simply create a template type that invokes
// the appropriate type casting and OPENSSL_sk_* invocations, but we must
// avoid copying implementation details for how the sk_* generated
// user-facing APIs are implemented.
// * In other words, our own macro implementation below *must not* use
// OPENSSL_sk_* APIs. They *must* only use the corresponding sk_T_free,
// sk_T_pop_free user APIs (i.e. those that can be found in OpenSSL
// documentation).
namespace detail {
template <
class StackType,
class ElementType,
void (*OwningStackDestructor)(StackType*, void (*)(ElementType*)),
void (*ElementDestructor)(ElementType*)>
struct OpenSSLOwnedStackDeleter {
void operator()(StackType* stack) const {
OwningStackDestructor(stack, ElementDestructor);
}
};
} // namespace detail
#if FOLLY_OPENSSL_PREREQ(1, 1, 0)
#define FOLLY_SSL_DETAIL_OWNING_STACK_DESTRUCTOR(T) \
::folly::ssl::detail:: \
OpenSSLOwnedStackDeleter<STACK_OF(T), T, sk_##T##_pop_free, T##_free>
#define FOLLY_SSL_DETAIL_BORROWING_STACK_DESTRUCTOR(T) \
::folly::static_function_deleter<STACK_OF(T), &sk_##T##_free>
#else
namespace detail {
template <class ElementType, void (*ElementDestructor)(ElementType*)>
struct OpenSSL102OwnedStackDestructor {
template <class T>
void operator()(T* stack) const {
sk_pop_free(
reinterpret_cast<_STACK*>(stack), ((void (*)(void*))ElementDestructor));
}
};
struct OpenSSL102BorrowedStackDestructor {
template <class T>
void operator()(T* stack) const {
sk_free(reinterpret_cast<_STACK*>(stack));
}
};
} // namespace detail
#define FOLLY_SSL_DETAIL_OWNING_STACK_DESTRUCTOR(T) \
::folly::ssl::detail::OpenSSL102OwnedStackDestructor<T, T##_free>
#define FOLLY_SSL_DETAIL_BORROWING_STACK_DESTRUCTOR(T) \
::folly::ssl::detail::OpenSSL102BorrowedStackDestructor
#endif
#define FOLLY_SSL_DETAIL_DEFINE_OWNING_STACK_PTR_TYPE( \
element_alias, element_type) \
using OwningStackOf##element_alias##UniquePtr = std::unique_ptr< \
STACK_OF(element_type), \
FOLLY_SSL_DETAIL_OWNING_STACK_DESTRUCTOR(element_type)>
#define FOLLY_SSL_DETAIL_DEFINE_BORROWING_STACK_PTR_TYPE( \
element_alias, element_type) \
using BorrowingStackOf##element_alias##UniquePtr = std::unique_ptr< \
STACK_OF(element_type), \
FOLLY_SSL_DETAIL_BORROWING_STACK_DESTRUCTOR(element_type)>
#define FOLLY_SSL_DETAIL_DEFINE_STACK_PTR_TYPE(element_alias, element_type) \
FOLLY_SSL_DETAIL_DEFINE_BORROWING_STACK_PTR_TYPE( \
element_alias, element_type); \
FOLLY_SSL_DETAIL_DEFINE_OWNING_STACK_PTR_TYPE(element_alias, element_type)
FOLLY_SSL_DETAIL_DEFINE_STACK_PTR_TYPE(X509Name, X509_NAME);
#undef FOLLY_SSL_DETAIL_BORROWING_STACK_DESTRUCTOR
#undef FOLLY_SSL_DETAIL_OWNING_STACK_DESTRUCTOR
#undef FOLLY_SSL_DETAIL_DEFINE_OWNING_STACK_PTR_TYPE
#undef FOLLY_SSL_DETAIL_DEFINE_BORROWING_STACK_PTR_TYPE
#undef FOLLY_SSL_DETAIL_DEFINE_STACK_PTR_TYPE
#undef FOLLY_SSL_DETAIL_DEFINE_PTR_TYPE
} // namespace ssl
} // namespace folly
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment