Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
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
3 changes: 3 additions & 0 deletions changelogs/current.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,9 @@ bug_fixes:
- area: tracing
change: |
Fixed a bug where the OpenTelemetry tracer exports the OTLP request even when no spans are present.
- area: tls
change: |
Comment thread
anitabyte marked this conversation as resolved.
Outdated
Added support for P-384 and P-521 curves for server certificates.
Comment thread
anitabyte marked this conversation as resolved.
Outdated

removed_config_or_runtime:
# *Normally occurs at the end of the* :ref:`deprecation period <deprecated>`
Expand Down
6 changes: 3 additions & 3 deletions docs/root/intro/arch_overview/security/ssl.rst
Original file line number Diff line number Diff line change
Expand Up @@ -114,15 +114,15 @@ Certificate selection
---------------------

:ref:`DownstreamTlsContexts <envoy_v3_api_msg_extensions.transport_sockets.tls.v3.DownstreamTlsContext>` support multiple TLS
certificates. These may be a mix of RSA and P-256 ECDSA certificates for multiple server name patterns.
certificates. These may be a mix of RSA and ECDSA certificates for multiple server name patterns.

Certificate config/loading rules:

* DNS SANs or Subject Common Name is extracted as server name pattern to match SNI during handshake. Subject Common Name is not used if DNS SANs are present in the certificate.
* FQDN like "test.example.com" and wildcard like "\*.example.com" are valid at the same time, which will be loaded
as two different server name patterns.
* If multiple certificates of a particular type (RSA or ECDSA) are specified for the same name or name pattern, the first one loaded is used for that name.
* Non-P-256 server ECDSA certificates are rejected.
* Non-P-256, P-384 or P-521 server ECDSA certificates are rejected.
* Static and SDS certificates may not be mixed in a given :ref:`DownstreamTlsContext
<envoy_v3_api_msg_extensions.transport_sockets.tls.v3.DownstreamTlsContext>`.

Expand All @@ -144,7 +144,7 @@ Certificate selection rules:
is false or true.
* Full scan execuates OCSP and key type checking on each cert which is the same as described above in exact SNI matching.
It falls back to the first cert in the whole list if there is no cert selected.
* Currently only two kinds of key type are supported, RSA or ECDSA. If the client supports P-256 ECDSA, the P-256 ECDSA certificate
* Currently only two kinds of key type are supported, RSA or ECDSA. If the client supports P-256, P384 or P-521 ECDSA, the P-256, P384 or P-521 ECDSA certificate
Comment thread
anitabyte marked this conversation as resolved.
Outdated
is preferred over RSA. The certificate that it falls back to might result in a failed handshake. For instance, a client only supports
RSA certificates and the certificate only support ECDSA.
* The final selected certificate must adhere to the OCSP policy. If no such certificate is found, the connection is refused.
Expand Down
5 changes: 3 additions & 2 deletions envoy/ssl/handshaker.h
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#include "envoy/server/options.h"
#include "envoy/singleton/manager.h"

#include "absl/types/optional.h"
#include "openssl/ssl.h"

Comment thread
anitabyte marked this conversation as resolved.
namespace Envoy {
Expand Down Expand Up @@ -231,8 +232,8 @@ class TlsCertificateSelector {
* @return context will have the same lifetime as ``ServerContextImpl``.
*/
virtual std::pair<const Ssl::TlsContext&, OcspStapleAction>
findTlsContext(absl::string_view sni, bool client_ecdsa_capable, bool client_ocsp_capable,
bool* cert_matched_sni) PURE;
findTlsContext(absl::string_view sni, absl::optional<std::vector<int>> client_ecdsa_capabilies,
Comment thread
anitabyte marked this conversation as resolved.
Outdated
bool client_ocsp_capable, bool* cert_matched_sni) PURE;
};

using TlsCertificateSelectorPtr = std::unique_ptr<TlsCertificateSelector>;
Expand Down
3 changes: 2 additions & 1 deletion source/common/quic/quic_server_transport_socket_factory.cc
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,8 @@ QuicServerTransportSocketFactory::getTlsCertificateAndKey(absl::string_view sni,
auto ctx =
std::dynamic_pointer_cast<Extensions::TransportSockets::Tls::ServerContextImpl>(ssl_ctx);
auto [tls_context, ocsp_staple_action] = ctx->findTlsContext(
sni, true /* TODO: ecdsa_capable */, false /* TODO: ocsp_capable */, cert_matched_sni);
sni, absl::optional<std::vector<int>>{NID_X9_62_prime256v1} /* TODO: ecdsa_capable */,
false /* TODO: ocsp_capable */, cert_matched_sni);

// Thread safety note: accessing the tls_context requires holding a shared_ptr to the ``ssl_ctx``.
// Both of these members are themselves reference counted, so it is safe to use them after
Expand Down
10 changes: 6 additions & 4 deletions source/common/tls/context_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -221,16 +221,18 @@ ContextImpl::ContextImpl(Stats::Scope& scope, const Envoy::Ssl::ContextConfig& c
ctx.is_ecdsa_ = pkey_id == EVP_PKEY_EC;
switch (pkey_id) {
case EVP_PKEY_EC: {
// We only support P-256 ECDSA today.
// We only support P-256, P384 or P-521 ECDSA today.
Comment thread
anitabyte marked this conversation as resolved.
Outdated
const EC_KEY* ecdsa_public_key = EVP_PKEY_get0_EC_KEY(public_key.get());
// Since we checked the key type above, this should be valid.
ASSERT(ecdsa_public_key != nullptr);
const EC_GROUP* ecdsa_group = EC_KEY_get0_group(ecdsa_public_key);
if (ecdsa_group == nullptr ||
EC_GROUP_get_curve_name(ecdsa_group) != NID_X9_62_prime256v1) {
(EC_GROUP_get_curve_name(ecdsa_group) != NID_X9_62_prime256v1 &&
EC_GROUP_get_curve_name(ecdsa_group) != NID_secp384r1 &&
EC_GROUP_get_curve_name(ecdsa_group) != NID_secp521r1)) {
creation_status = absl::InvalidArgumentError(
fmt::format("Failed to load certificate chain from {}, only P-256 "
"ECDSA certificates are supported",
fmt::format("Failed to load certificate chain from {}, only P-256, "
"P-384 or P-521 ECDSA certificates are supported",
ctx.cert_chain_file_path_));
return;
}
Expand Down
39 changes: 32 additions & 7 deletions source/common/tls/default_tls_certificate_selector.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

#include "source/common/tls/utility.h"

#include "openssl/evp.h"
Comment thread
anitabyte marked this conversation as resolved.
Outdated

namespace Envoy {
namespace Extensions {
namespace TransportSockets {
Expand Down Expand Up @@ -89,11 +91,12 @@ DefaultTlsCertificateSelector::selectTlsContext(const SSL_CLIENT_HELLO& ssl_clie
Ssl::CertificateSelectionCallbackPtr) {
absl::string_view sni =
absl::NullSafeStringView(SSL_get_servername(ssl_client_hello.ssl, TLSEXT_NAMETYPE_host_name));
const bool client_ecdsa_capable = server_ctx_.isClientEcdsaCapable(ssl_client_hello);
absl::optional<std::vector<int>> client_ecdsa_capabilities =
server_ctx_.getClientEcdsaCapabilities(ssl_client_hello);
const bool client_ocsp_capable = server_ctx_.isClientOcspCapable(ssl_client_hello);

auto [selected_ctx, ocsp_staple_action] =
findTlsContext(sni, client_ecdsa_capable, client_ocsp_capable, nullptr);
findTlsContext(sni, client_ecdsa_capabilities, client_ocsp_capable, nullptr);

auto stats = server_ctx_.stats();
if (client_ocsp_capable) {
Expand Down Expand Up @@ -162,8 +165,9 @@ Ssl::OcspStapleAction DefaultTlsCertificateSelector::ocspStapleAction(const Ssl:
}

std::pair<const Ssl::TlsContext&, Ssl::OcspStapleAction>
DefaultTlsCertificateSelector::findTlsContext(absl::string_view sni, bool client_ecdsa_capable,
bool client_ocsp_capable, bool* cert_matched_sni) {
DefaultTlsCertificateSelector::findTlsContext(
absl::string_view sni, absl::optional<std::vector<int>> client_ecdsa_capabilities,
bool client_ocsp_capable, bool* cert_matched_sni) {
bool unused = false;
if (cert_matched_sni == nullptr) {
// Avoid need for nullptr checks when this is set.
Expand All @@ -182,14 +186,34 @@ DefaultTlsCertificateSelector::findTlsContext(absl::string_view sni, bool client
// The selected ctx must adhere to OCSP policy
return false;
}
// if the client is ECDSA-capable and the context is ECDSA, we check if it is capable of
// handling the curves in the cert in a given TlsContext
Comment thread
anitabyte marked this conversation as resolved.
Outdated
if (client_ecdsa_capabilities.has_value() && ctx.is_ecdsa_) {
Comment thread
anitabyte marked this conversation as resolved.
Outdated
bssl::UniquePtr<EVP_PKEY> public_key(X509_get_pubkey(ctx.cert_chain_.get()));
// we're doing this with a guard that the `ctx` is ECDSA - this should be safe
Comment thread
anitabyte marked this conversation as resolved.
Outdated
const EC_KEY* ecdsa_public_key = EVP_PKEY_get0_EC_KEY(public_key.get());
ASSERT(ecdsa_public_key != nullptr);
const EC_GROUP* ecdsa_group = EC_KEY_get0_group(ecdsa_public_key);
const int ecdsa_curve_nid = EC_GROUP_get_curve_name(ecdsa_group);
// if we have a matching curve NID in our client capabilities, return `true`
Comment thread
anitabyte marked this conversation as resolved.
Outdated
const std::vector<int> cec_vec = client_ecdsa_capabilities.value();
if (std::find(cec_vec.begin(), cec_vec.end(), ecdsa_curve_nid) != cec_vec.end()) {
selected_ctx = &ctx;
ocsp_staple_action = action;
return true;
} else {
return false;
}
}

if (client_ecdsa_capable == ctx.is_ecdsa_) {
// if the client is not ECDSA-capable and the `ctx` is non-ECDSA, then select this `ctx`
Comment thread
anitabyte marked this conversation as resolved.
Outdated
if (!client_ecdsa_capabilities.has_value() && !ctx.is_ecdsa_) {
selected_ctx = &ctx;
ocsp_staple_action = action;
return true;
}

if (client_ecdsa_capable && !ctx.is_ecdsa_ && candidate_ctx == nullptr) {
if (client_ecdsa_capabilities.has_value() && !ctx.is_ecdsa_ && candidate_ctx == nullptr) {
// ECDSA cert is preferred if client is ECDSA capable, so RSA cert is marked as a candidate,
// searching will continue until exhausting all certs or find a exact match.
candidate_ctx = &ctx;
Expand Down Expand Up @@ -249,7 +273,8 @@ DefaultTlsCertificateSelector::findTlsContext(absl::string_view sni, bool client
if (selected_ctx == nullptr) {
candidate_ctx = nullptr;
// Skip loop when there is no cert compatible to key type
if (client_ecdsa_capable || (!client_ecdsa_capable && has_rsa_)) {
if (client_ecdsa_capabilities.has_value() ||
(!client_ecdsa_capabilities.has_value() && has_rsa_)) {
for (const auto& ctx : tls_contexts_) {
if (selected(ctx)) {
break;
Expand Down
4 changes: 2 additions & 2 deletions source/common/tls/default_tls_certificate_selector.h
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,8 @@ class DefaultTlsCertificateSelector : public Ssl::TlsCertificateSelector,
// Finds the best matching context. The returned context will have the same lifetime as
// ``ServerContextImpl``.
std::pair<const Ssl::TlsContext&, Ssl::OcspStapleAction>
findTlsContext(absl::string_view sni, bool client_ecdsa_capable, bool client_ocsp_capable,
bool* cert_matched_sni) override;
findTlsContext(absl::string_view sni, absl::optional<std::vector<int>> client_ecdsa_capabilities,
bool client_ocsp_capable, bool* cert_matched_sni) override;

private:
// Currently, at most one certificate of a given key type may be specified for each exact
Expand Down
54 changes: 40 additions & 14 deletions source/common/tls/server_context_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
#include "absl/container/node_hash_set.h"
#include "absl/strings/match.h"
#include "absl/strings/str_join.h"
#include "absl/types/optional.h"
#include "cert_validator/cert_validator.h"
#include "openssl/evp.h"
#include "openssl/hmac.h"
Expand Down Expand Up @@ -376,7 +377,12 @@ int ServerContextImpl::sessionTicketProcess(SSL*, uint8_t* key_name, uint8_t* iv
}
}

bool ServerContextImpl::isClientEcdsaCapable(const SSL_CLIENT_HELLO& ssl_client_hello) const {
// We want to return a list of client capabilities for ECDSA now that we support curves other
// than P-256. An empty optional is used to represent a client that is unable to handle ECDSA
// the vector inside that optional is a list of `ssl.h` constants representing ECDSA group NIDs
absl::optional<std::vector<int>>
ServerContextImpl::getClientEcdsaCapabilities(const SSL_CLIENT_HELLO& ssl_client_hello) const {
std::vector<int> client_capabilities;
CBS client_hello;
CBS_init(&client_hello, ssl_client_hello.client_hello, ssl_client_hello.client_hello_len);

Expand All @@ -399,14 +405,23 @@ bool ServerContextImpl::isClientEcdsaCapable(const SSL_CLIENT_HELLO& ssl_client_
CBS_init(&signature_algorithms_ext, signature_algorithms_data, signature_algorithms_len);
if (!CBS_get_u16_length_prefixed(&signature_algorithms_ext, &signature_algorithms) ||
CBS_len(&signature_algorithms_ext) != 0) {
return false;
return {};
}
if (cbsContainsU16(signature_algorithms, SSL_SIGN_ECDSA_SECP256R1_SHA256)) {
return true;
client_capabilities.push_back(NID_X9_62_prime256v1);
}
if (cbsContainsU16(signature_algorithms, SSL_SIGN_ECDSA_SECP384R1_SHA384)) {
client_capabilities.push_back(NID_secp384r1);
}
if (cbsContainsU16(signature_algorithms, SSL_SIGN_ECDSA_SECP521R1_SHA512)) {
client_capabilities.push_back(NID_secp521r1);
}
if (client_capabilities.size() != 0) {
return client_capabilities;
}
}
Comment thread
anitabyte marked this conversation as resolved.

return false;
return {};
}
}

Expand All @@ -416,15 +431,25 @@ bool ServerContextImpl::isClientEcdsaCapable(const SSL_CLIENT_HELLO& ssl_client_
size_t curvelist_len;
if (!SSL_early_callback_ctx_extension_get(&ssl_client_hello, TLSEXT_TYPE_supported_groups,
&curvelist_data, &curvelist_len)) {
return false;
return {};
}

CBS curvelist;
CBS_init(&curvelist, curvelist_data, curvelist_len);

// We only support P256 ECDSA curves today.
if (!cbsContainsU16(curvelist, SSL_CURVE_SECP256R1)) {
return false;
// We support P256, P384 and P521 ECDSA curves today.
if (cbsContainsU16(curvelist, SSL_CURVE_SECP256R1)) {
client_capabilities.push_back(NID_X9_62_prime256v1);
}
if (cbsContainsU16(curvelist, SSL_CURVE_SECP384R1)) {
client_capabilities.push_back(NID_secp384r1);
}
if (cbsContainsU16(curvelist, SSL_CURVE_SECP521R1)) {
client_capabilities.push_back(NID_secp521r1);
}
// if we haven't got any curves in common with the client, return empty optional
if (client_capabilities.size() == 0) {
return {};
}

// The client must have offered an ECDSA ciphersuite that we like.
Expand All @@ -434,16 +459,16 @@ bool ServerContextImpl::isClientEcdsaCapable(const SSL_CLIENT_HELLO& ssl_client_
while (CBS_len(&cipher_suites) > 0) {
uint16_t cipher_id;
if (!CBS_get_u16(&cipher_suites, &cipher_id)) {
return false;
return {};
}
// All tls_context_ share the same set of enabled ciphers, so we can just look at the base
// context.
if (tls_contexts_[0].isCipherEnabled(cipher_id, client_version)) {
return true;
return client_capabilities;
}
}

return false;
return {};
}

bool ServerContextImpl::isClientOcspCapable(const SSL_CLIENT_HELLO& ssl_client_hello) const {
Expand All @@ -458,10 +483,11 @@ bool ServerContextImpl::isClientOcspCapable(const SSL_CLIENT_HELLO& ssl_client_h
}

std::pair<const Ssl::TlsContext&, Ssl::OcspStapleAction>
ServerContextImpl::findTlsContext(absl::string_view sni, bool client_ecdsa_capable,
ServerContextImpl::findTlsContext(absl::string_view sni,
absl::optional<std::vector<int>> client_ecdsa_capabilities,
bool client_ocsp_capable, bool* cert_matched_sni) {
return tls_certificate_selector_->findTlsContext(sni, client_ecdsa_capable, client_ocsp_capable,
cert_matched_sni);
return tls_certificate_selector_->findTlsContext(sni, client_ecdsa_capabilities,
client_ocsp_capable, cert_matched_sni);
}

enum ssl_select_cert_result_t
Expand Down
10 changes: 5 additions & 5 deletions source/common/tls/server_context_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -60,11 +60,11 @@ class ServerContextImpl : public ContextImpl,

// Finds the best matching context. The returned context will have the same lifetime as
// this ``ServerContextImpl``.
std::pair<const Ssl::TlsContext&, Ssl::OcspStapleAction> findTlsContext(absl::string_view sni,
bool client_ecdsa_capable,
bool client_ocsp_capable,
bool* cert_matched_sni);
bool isClientEcdsaCapable(const SSL_CLIENT_HELLO& ssl_client_hello) const;
std::pair<const Ssl::TlsContext&, Ssl::OcspStapleAction>
findTlsContext(absl::string_view sni, absl::optional<std::vector<int>> client_ecdsa_capable,
bool client_ocsp_capable, bool* cert_matched_sni);
absl::optional<std::vector<int>>
getClientEcdsaCapabilities(const SSL_CLIENT_HELLO& ssl_client_hello) const;
bool isClientOcspCapable(const SSL_CLIENT_HELLO& ssl_client_hello) const;

private:
Expand Down
4 changes: 2 additions & 2 deletions test/common/tls/cert_selector/async_cert_selector.h
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,8 @@ class AsyncTlsCertificateSelector : public Ssl::TlsCertificateSelector,
Ssl::CertificateSelectionCallbackPtr cb) override;

// It's only for quic.
std::pair<const Ssl::TlsContext&, Ssl::OcspStapleAction> findTlsContext(absl::string_view, bool,
bool, bool*) override {
std::pair<const Ssl::TlsContext&, Ssl::OcspStapleAction>
findTlsContext(absl::string_view, absl::optional<std::vector<int>>, bool, bool*) override {
PANIC("unreachable");
};

Expand Down
38 changes: 22 additions & 16 deletions test/common/tls/context_impl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1408,8 +1408,8 @@ TEST_F(ClientContextConfigImplTest, P256EcdsaCert) {
auto cleanup = cleanUpHelper(context);
}

// Validate that non-P256 ECDSA certs are rejected.
TEST_F(ClientContextConfigImplTest, NonP256EcdsaCert) {
// Validate that P384 ECDSA certs load.
TEST_F(ClientContextConfigImplTest, P384EcdsaCert) {
envoy::extensions::transport_sockets::tls::v3::UpstreamTlsContext tls_context;
const std::string tls_certificate_yaml = R"EOF(
certificate_chain:
Expand All @@ -1421,16 +1421,12 @@ TEST_F(ClientContextConfigImplTest, NonP256EcdsaCert) {
*tls_context.mutable_common_tls_context()->add_tls_certificates());
auto client_context_config = *ClientContextConfigImpl::create(tls_context, factory_context_);
Stats::IsolatedStoreImpl store;
EXPECT_THAT(manager_.createSslClientContext(*store.rootScope(), *client_context_config)
.status()
.message(),
testing::ContainsRegex(
"Failed to load certificate chain from .*selfsigned_ecdsa_p384_cert.pem, "
"only P-256 ECDSA certificates are supported"));
auto context = *manager_.createSslClientContext(*store.rootScope(), *client_context_config);
auto cleanup = cleanUpHelper(context);
Comment thread
anitabyte marked this conversation as resolved.
Outdated
}

// Validate that non-P256 ECDSA certs are rejected loaded from `pkcs12`.
TEST_F(ClientContextConfigImplTest, NonP256EcdsaPkcs12) {
// Validate that P384 ECDSA certs are loaded from `pkcs12`.
TEST_F(ClientContextConfigImplTest, P384EcdsaPkcs12) {
envoy::extensions::transport_sockets::tls::v3::UpstreamTlsContext tls_context;
const std::string tls_certificate_yaml = R"EOF(
pkcs12:
Expand All @@ -1440,12 +1436,22 @@ TEST_F(ClientContextConfigImplTest, NonP256EcdsaPkcs12) {
*tls_context.mutable_common_tls_context()->add_tls_certificates());
auto client_context_config = *ClientContextConfigImpl::create(tls_context, factory_context_);
Stats::IsolatedStoreImpl store;
EXPECT_THAT(manager_.createSslClientContext(*store.rootScope(), *client_context_config)
.status()
.message(),
testing::ContainsRegex(
"Failed to load certificate chain from .*selfsigned_ecdsa_p384_certkey.p12, "
"only P-256 ECDSA certificates are supported"));
}

TEST_F(ClientContextConfigImplTest, P521EcdsaCert) {
envoy::extensions::transport_sockets::tls::v3::UpstreamTlsContext tls_context;
const std::string tls_certificate_yaml = R"EOF(
certificate_chain:
filename: "{{ test_rundir }}/test/common/tls/test_data/selfsigned_ecdsa_p521_cert.pem"
private_key:
filename: "{{ test_rundir }}/test/common/tls/test_data/selfsigned_ecdsa_p521_key.pem"
)EOF";
TestUtility::loadFromYaml(TestEnvironment::substitute(tls_certificate_yaml),
*tls_context.mutable_common_tls_context()->add_tls_certificates());
auto client_context_config = *ClientContextConfigImpl::create(tls_context, factory_context_);
Stats::IsolatedStoreImpl store;
auto context = *manager_.createSslClientContext(*store.rootScope(), *client_context_config);
Comment thread
anitabyte marked this conversation as resolved.
Outdated
auto cleanup = cleanUpHelper(context);
}

// Multiple TLS certificates are not yet supported.
Expand Down
Loading