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.

Sign up for GitHub

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

Add SSL_CTX_use_cert_and_key #2163

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Add SSL_CTX_use_cert_and_key #2163

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
807b100
added SSL_CTX_use_cert_and_key and modified internal funcs
smittals2 Feb 1, 2025
1e19892
added testing
smittals2 Feb 4, 2025
3ecfb98
store references for cleanup
smittals2 Feb 4, 2025
5ab2d28
free explicitly
smittals2 Feb 11, 2025
0805e62
use stack instead of vector
smittals2 Feb 13, 2025
9781ebe
minor fixes
smittals2 Feb 13, 2025
51b9e08
invert if condition
smittals2 Feb 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions include/openssl/ssl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1257,6 +1257,22 @@ OPENSSL_EXPORT int SSL_set_signing_algorithm_prefs(SSL *ssl,
const uint16_t *prefs,
size_t num_prefs);

// SSL_CTX_use_cert_and_key sets |x509|, |privatekey|, and |chain| on |ctx|.
// The pkey argument must be the private key of the certificate |x509|.
// If the override argument is 0, then |x509|, |privatekey|, and |chain| are
// set only if all were not previously set. If override is non-0, then the
// certificate, private key and chain certs are always set. |privatekey| and
// |x509| are not copied or duplicated, their reference count is incremented.
// In OpenSSL, a shallow copy of |chain| is stored with a reference count
// increment for all X509 objects in the chain. In AWS-LC,
// we represent X509 chains as a CRYPTO_BUFFER stack. Therefore, we create a
// an internal copy and leave the |chain| parameter untouched. This means,
// changes to |chain| after this function is called will not update in |ctx|.
//
// Returns one on success and zero on error.
OPENSSL_EXPORT int SSL_CTX_use_cert_and_key(SSL_CTX *ctx, X509 *x509,
EVP_PKEY *privatekey,
STACK_OF(X509) *chain, int override);

// Certificate and private key convenience functions.

Expand Down
110 changes: 99 additions & 11 deletions ssl/ssl_cert.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -274,8 +274,9 @@ static enum leaf_cert_and_privkey_result_t check_leaf_cert_and_privkey(
}

static int cert_set_chain_and_key(
CERT *cert, CRYPTO_BUFFER *const *certs, size_t num_certs,
EVP_PKEY *privkey, const SSL_PRIVATE_KEY_METHOD *privkey_method) {
CERT *cert, UniquePtr<STACK_OF(CRYPTO_BUFFER)> *certs, size_t num_certs,
EVP_PKEY *privkey, const SSL_PRIVATE_KEY_METHOD *privkey_method,
int override) {
if (num_certs == 0 || (privkey == NULL && privkey_method == NULL)) {
OPENSSL_PUT_ERROR(SSL, ERR_R_PASSED_NULL_PARAMETER);
return 0;
Expand All @@ -286,10 +287,11 @@ static int cert_set_chain_and_key(
return 0;
}

switch (check_leaf_cert_and_privkey(certs[0], privkey)) {
switch (check_leaf_cert_and_privkey(sk_CRYPTO_BUFFER_value(certs->get(), 0), privkey)) {
case leaf_cert_and_privkey_error:
return 0;
case leaf_cert_and_privkey_mismatch:
OPENSSL_PUT_ERROR(SSL, SSL_R_CERTIFICATE_AND_PRIVATE_KEY_MISMATCH);
OPENSSL_PUT_ERROR(SSL, SSL_R_CERTIFICATE_AND_PRIVATE_KEY_MISMATCH);
Comment on lines +294 to 295
Copy link
Contributor

Choose a reason for hiding this comment

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

Duplicate line.

return 0;
case leaf_cert_and_privkey_ok:
Expand All @@ -302,7 +304,7 @@ static int cert_set_chain_and_key(
}

for (size_t i = 0; i < num_certs; i++) {
if (!PushToStack(certs_sk.get(), UpRef(certs[i]))) {
if (!PushToStack(certs_sk.get(), UpRef(sk_CRYPTO_BUFFER_value(certs->get(), i)))) {
return 0;
}
}
Comment on lines 306 to 310
Copy link
Contributor

Choose a reason for hiding this comment

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

We could use sk_CRYPTO_BUFFER_deep_copy here:

  UniquePtr<STACK_OF(CRYPTO_BUFFER)> certs_sk(sk_CRYPTO_BUFFER_deep_copy(
      certs->get(), buffer_up_ref, CRYPTO_BUFFER_free));

The copy function buffer_up_ref is defined above. It's also needed here. Perhaps the copy function should be defined somewhere they could share?

Expand All @@ -311,12 +313,19 @@ static int cert_set_chain_and_key(
return 0;
}

// Update certificate slot index once all checks have passed.
// Certificate slot validity already checked in |check_leaf_cert_and_privkey|.
int idx = ssl_get_certificate_slot_index(privkey);
cert->cert_private_keys[idx].privatekey = UpRef(privkey);
CERT_PKEY *cert_pkey = &cert->cert_private_keys[idx];
Comment on lines 316 to +318
Copy link
Contributor

Choose a reason for hiding this comment

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

NP: I'm concerned about there not being a bounds check on the idx value returned. Perhaps add an assert(idx >= 0); here to improve detection/diagnosability were this ever to fail?

  • I am being a bit paranoid. There is a call to ssl_is_key_type_supported in do_leaf_cert_and_privkey_checks that should prevent failure here. But some of this logic might soon be refactored for PQ?

if (!override && (cert_pkey->privatekey != NULL ||
cert_pkey->x509_leaf != NULL ||
cert_pkey->chain != NULL)) {
return 0;
}
Comment on lines +319 to +323
Copy link
Contributor

Choose a reason for hiding this comment

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

  • Couldn't this check be at the start of the function (line ~280) and avoid the potentially unnecessary work/allocations above?
  • Even better the check could be at the top of SSL_CTX_use_cert_and_key (the only caller that doesn't override).
  int idx = ssl_get_certificate_slot_index(privatekey);
  if (idx < 0) {
    OPENSSL_PUT_ERROR(SSL, SSL_R_UNKNOWN_CERTIFICATE_TYPE);
    return 0;
  }
  CERT_PKEY *cert_pkey = &ctx->cert->cert_private_keys[idx];
  if (!override && (cert_pkey->privatekey != NULL ||
      cert_pkey->x509_leaf != NULL ||
      cert_pkey->chain != NULL)) {
    return 0;
  }


// Update certificate slot index once all checks have passed.
cert_pkey->privatekey = UpRef(privkey);
cert->key_method = privkey_method;
cert->cert_private_keys[idx].chain = std::move(certs_sk);
cert_pkey->chain = std::move(certs_sk);
cert->cert_private_key_idx = idx;
return 1;
}
Expand Down Expand Up @@ -952,21 +961,100 @@ BSSL_NAMESPACE_END

using namespace bssl;

static int cert_array_to_stack(CRYPTO_BUFFER *const *from,
UniquePtr<STACK_OF(CRYPTO_BUFFER)> *to,
size_t num_certs) {
for (size_t i = 0; i < num_certs; i++) {
if (!PushToStack(to->get(), UpRef(from[i]))) {
return 0;
}
}
return 1;
}

int SSL_set_chain_and_key(SSL *ssl, CRYPTO_BUFFER *const *certs,
size_t num_certs, EVP_PKEY *privkey,
const SSL_PRIVATE_KEY_METHOD *privkey_method) {
if (!ssl->config) {
return 0;
}
return cert_set_chain_and_key(ssl->config->cert.get(), certs, num_certs,
privkey, privkey_method);
UniquePtr<STACK_OF(CRYPTO_BUFFER)> crypto_certs(sk_CRYPTO_BUFFER_new_null());
if (!cert_array_to_stack(certs, &crypto_certs, num_certs)) {
return 0;
}
return cert_set_chain_and_key(ssl->config->cert.get(), &crypto_certs, num_certs,
privkey, privkey_method, 1);
}

int SSL_CTX_set_chain_and_key(SSL_CTX *ctx, CRYPTO_BUFFER *const *certs,
size_t num_certs, EVP_PKEY *privkey,
const SSL_PRIVATE_KEY_METHOD *privkey_method) {
return cert_set_chain_and_key(ctx->cert.get(), certs, num_certs, privkey,
privkey_method);
UniquePtr<STACK_OF(CRYPTO_BUFFER)> crypto_certs(sk_CRYPTO_BUFFER_new_null());
if (!cert_array_to_stack(certs, &crypto_certs, num_certs)) {
return 0;
}
return cert_set_chain_and_key(ctx->cert.get(), &crypto_certs, num_certs, privkey,
privkey_method, 1);
}


int SSL_CTX_use_cert_and_key(SSL_CTX *ctx, X509 *x509, EVP_PKEY *privatekey,
STACK_OF(X509) *chain, int override) {

if (privatekey == nullptr || x509 == nullptr) {
OPENSSL_PUT_ERROR(SSL, ERR_R_PASSED_NULL_PARAMETER);
return 0;
}
UniquePtr<STACK_OF(CRYPTO_BUFFER)> leaf_and_chain(sk_CRYPTO_BUFFER_new_null());

// Convert leaf certificate (x509) to CRYPTO_BUFFER
uint8_t *buf = nullptr;
int cert_len = i2d_X509(x509, &buf);
if (cert_len <= 0) {
OPENSSL_free(buf);
return 0;
}

// Add leaf cert first to the chain
UniquePtr<CRYPTO_BUFFER> leaf_buf(CRYPTO_BUFFER_new(buf, cert_len, nullptr));
OPENSSL_free(buf);
if (!leaf_buf ||
!PushToStack(leaf_and_chain.get(), std::move(leaf_buf))) {
return 0;
}

// Convert other chain certificates to CRYPTO_BUFFER objects
if (chain != nullptr) {
for (size_t i = 0; i < sk_X509_num(chain); i++) {
buf = nullptr;
cert_len = i2d_X509(sk_X509_value(chain, i), &buf);
if (cert_len <= 0) {
OPENSSL_free(buf);
return 0;
}

UniquePtr<CRYPTO_BUFFER> chain_buf(CRYPTO_BUFFER_new(buf, cert_len, nullptr));
OPENSSL_free(buf);
if (!chain_buf ||
!PushToStack(leaf_and_chain.get(), std::move(chain_buf))) {
return 0;
}
}
}

// Call SSL_CTX_set_chain_and_key to set the chain and key
if (!cert_set_chain_and_key(ctx->cert.get(), &leaf_and_chain,
sk_CRYPTO_BUFFER_num(leaf_and_chain.get()),
privatekey, nullptr, override)) {
return 0;
}

// Store a reference to the passed in |x509| leaf object
int idx = ssl_get_certificate_slot_index(privatekey);
X509_up_ref(x509);
ctx->cert->cert_private_keys[idx].x509_leaf = x509;

return 1;
}

void SSL_certs_clear(SSL *ssl) {
Expand Down
43 changes: 43 additions & 0 deletions ssl/ssl_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6226,6 +6226,12 @@ TEST(SSLTest, SetChainAndKeyMismatch) {
ASSERT_FALSE(SSL_CTX_set_chain_and_key(ctx.get(), &chain[0], chain.size(),
key.get(), nullptr));
ERR_clear_error();

// Ensure |SSL_CTX_use_cert_and_key| also fails
bssl::UniquePtr<X509> x509_leaf = X509FromBuffer(GetChainTestCertificateBuffer());
ASSERT_FALSE(SSL_CTX_use_cert_and_key(ctx.get(), x509_leaf.get(),
key.get(), NULL, 1));
ERR_clear_error();
}

TEST(SSLTest, SetChainAndKey) {
Expand Down Expand Up @@ -6264,6 +6270,43 @@ TEST(SSLTest, SetChainAndKey) {
server_ctx.get()));
}

TEST(SSLTest, SetLeafChainAndKey) {
bssl::UniquePtr<SSL_CTX> client_ctx(SSL_CTX_new(TLS_with_buffers_method()));
ASSERT_TRUE(client_ctx);
bssl::UniquePtr<SSL_CTX> server_ctx(SSL_CTX_new(TLS_with_buffers_method()));
ASSERT_TRUE(server_ctx);

ASSERT_EQ(nullptr, SSL_CTX_get0_chain(server_ctx.get()));

bssl::UniquePtr<EVP_PKEY> key = GetChainTestKey();
ASSERT_TRUE(key);
bssl::UniquePtr<X509> leaf = X509FromBuffer(GetChainTestCertificateBuffer());
ASSERT_TRUE(leaf);
bssl::UniquePtr<X509> intermediate =
X509FromBuffer(GetChainTestIntermediateBuffer());
bssl::UniquePtr<STACK_OF(X509)> chain(sk_X509_new_null());
ASSERT_TRUE(chain);
ASSERT_TRUE(PushToStack(chain.get(), std::move(intermediate)));

ASSERT_TRUE(SSL_CTX_use_cert_and_key(server_ctx.get(), leaf.get(),
key.get(), chain.get(), 1));

SSL_CTX_set_custom_verify(
client_ctx.get(), SSL_VERIFY_PEER,
[](SSL *ssl, uint8_t *out_alert) -> ssl_verify_result_t {
return ssl_verify_ok;
});

bssl::UniquePtr<SSL> client, server;
ASSERT_TRUE(ConnectClientAndServer(&client, &server, client_ctx.get(),
server_ctx.get()));

// Try setting on previously populated fields without an override
ASSERT_FALSE(SSL_CTX_use_cert_and_key(server_ctx.get(), leaf.get(),
key.get(), chain.get(), 0));
ERR_clear_error();
}

TEST(SSLTest, BuffersFailWithoutCustomVerify) {
bssl::UniquePtr<SSL_CTX> client_ctx(SSL_CTX_new(TLS_with_buffers_method()));
ASSERT_TRUE(client_ctx);
Expand Down
Loading