Support P-384 and P-521 Server ECDSA Certificates

[JoelHenn]

Title: Support P-384 and P-521 Server ECDSA Certificates

Description:
Update Envoy to support server ECDSA certificates P-384 and P-521. Given that BoringSSL supports these curves, Envoy should allow servers to use certs with those curves to terminate TLS. The expected behavior is for Envoy to take an ECDSA cert and check to make sure it uses one of the three approved curves.

Relevant Links
Older PR for rejecting non P-256 server ECDSA certs: #5224

[ggreenway]

cc @PiotrSikora @davidben

[davidben]

(Also @agl )

The P-256 restriction is to avoid misconfigurations, reduce DoS risks, and keep the ecosystem healthy. The leaf certificate key is used to perform an signature on every TLS connection. That means that both performance and side-channel resistance are important. Performance problems translate to DoS risks for the server, and side-channel problems translate to loss of private key. (Cloud providers are especially sensitive to the DoS risks as private keys are often supplied externally.)

Good EC implementations are tuned to the specific curve, requiring error-prone optimizations. This means the ecosystem-wide cost of new curves is high. The bulk of implementation effort is spent on P-256. This is a good thing as it reduces those costs. While BoringSSL does have P-384 and P-521 APIs, they currently use a generic fallback implementation.

Additionally, even with a tuned implementation, keep in mind that EC operations scale cubicly. That means there is a significant cost to chasing bigger numbers. For TLS ECDSA leaf keys, P-256 is the right answer. (Many clients don't even allow P-521 in TLS, including BoringSSL in its default client configuration.)

[james-mathiesen]

We have existing Java applications that expect P-384 client certificates and we hit this trying to make HTTPS calls through envoy (as configured by istio). We have RSA leaf certs available and are testing with those but the root and intermediates will still be P-384.

[stale]

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or other activity occurs. Thank you for your contributions.

[stale]

This issue has been automatically closed because it has not had activity in the last 37 days. If this issue is still valid, please ping a maintainer and ask them to label it as "help wanted". Thank you for your contributions.

[cretz]

Both nist/fips and browsers support P-384 and I personally would opt-in to it even with impl perf concerns. Is it reasonable to revive this conversation? Would opt-in be acceptable? Has anything changed from a BoringSSL perspective on P-384 support since a year ago?

[ggreenway]

I would be fine with the perf concerns, but I am concerned about the side-channel resistance.

[davidben]

BoringSSL's position has not changed from #10855 (comment). While we support P-384 and do aim for side channel resistance in our generic implementation, it is still the case that P-256 is a more important curve and gets far more attention. P-256 is the right answer for TLS ECDSA leaf keys.

[cretz]

The question becomes, should non-default, opt-in support for P-384 be allowed for the discerning admin that is aware of the tradeoffs? When I say opt-in, I don't mean if it happens to be on the cert, I mean a literal curve list that defaults to P-256 only but allows a list of P-256 + P-384 akin to what is done on the connection side at https://github.com/envoyproxy/envoy/blob/v1.17.1/api/envoy/extensions/transport_sockets/tls/v3/common.proto#L92-L107

[davidben]

I think you're confusing two different (but, in TLS 1.2, related due to the mixed negotiation) things. There's the ECDH curve in the key exchange, and the ECDSA curve in the leaf certificate.

For ECDH in TLS, X25519 is the right answer, but supporting P-256 is also fine for compatibility. Though note that, only supporting P-256 ECDH in TLS 1.3 servers will cost an extra round-trip against some clients, including Chrome, due to HelloRetryRequest.
For ECDSA in TLS leaf certificates, P-256 is the right answer.

[cretz]

Sorry. For the last part, I wasn't trying to conflate the ECDH curve and the ECDSA curve choice in any way except to say "it can be configurable with a default" like other aspects of TLS.

The question is, is Envoy adamantly against even allowing P-384 ECDSA cert ability to be opted into via configuration? Many use cases for Envoy are transparent proxies where they accept the origin's cert choice even if they disagree and accept the tradeoff of them not using the "right answer". E.g. there are many right answers wrt HTTP protocol choice, cipher suite choice, etc but they are configurable.

[james-mathiesen]

When I last looked at this envoy had disabled not only handling P-384 private keys, but pretty much any operation that involved P-384 public keys. For example it could not be an HTTP client if the foreign HTTP server had a P-384 server certificate. Is that still the case? It was very awkward as many enterprise type organizations use EC for internal CAs.

[PiotrSikora]

When I last looked at this envoy had disabled not only handling P-384 private keys, but pretty much any operation that involved P-384 public keys. For example it could not be an HTTP client if the foreign HTTP server had a P-384 server certificate. Is that still the case? It was very awkward as many enterprise type organizations use EC for internal CAs.

I don't believe that was ever the case. The only place where P-384 is rejected is in Envoy's locally configured certificates.

[cretz]

I confirmed in my local fork, with an integration test, that with the following removed, P-384 works:

envoy/source/extensions/transport_sockets/tls/context_impl.cc

Lines 227 to 237 in 2eb6359

	// We only support P-256 ECDSA today.
	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) {
	throw EnvoyException(fmt::format("Failed to load certificate chain from {}, only P-256 "
	"ECDSA certificates are supported",
	ctx.cert_chain_file_path_));
	}

(granted I had to SSL_set1_curves on the client side of the http integration test inside of envoy to have it work)

[ggreenway]

That isn't a sufficient fix to fully support it. There is also some code used to support dual-cert (RSA and EC) that is looking through the ClientHello to figure out which cert to offer.

[cretz]

This still sets the ctx.is_ecdsa_ value so it'll be chosen (default to) on client ECDSA support. But yes, if there are additional checks we have to do to determine client P-384 support beyond the is-ecdsa, then changes would be needed there.

[wez470]

I wanted to ask a question here. Would it be acceptable if opt-in compile time support were added to allow ECDSA P-384 and P-521 certificates?

[jdubs11]

P256 is not even recommended by NSA anymore, when will Envoy support P384? The reason given in this thread 2 years ago seems invalid now, and was invalid then.

[ldemailly]

@cretz so can we remove that section you mentioned ? what is its purpose vs

  ecdh_curves: 
                - P-384

being allowed in the config?

Or am I reading it wrong (still seems here in latest tag:)
https://github.com/envoyproxy/envoy/blob/v1.25.2/source/extensions/transport_sockets/tls/context_impl.cc#L220-L230

[ggreenway]

Re-opening. @jdubs11 and I were discussing this is another issue.

[ggreenway]

To implement this correctly, consider and address the comments left when the original code was added: #5200 (comment)

[anitgandhi]

👋 if it helps at all, we were able to implement this in a fork with the following patch. note that it also includes RSA-1024 support, which is something we also needed. also, it's probably a bit too relaxed on the ECDSA curve types. but hopefully it's a starting point

diff --git a/source/extensions/transport_sockets/tls/context_impl.cc b/source/extensions/transport_sockets/tls/context_impl.cc
index 38ff08d9f5..9bd4323cfc 100644
--- a/source/extensions/transport_sockets/tls/context_impl.cc
+++ b/source/extensions/transport_sockets/tls/context_impl.cc
@@ -224,17 +224,6 @@ 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.
-        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) {
-          throw EnvoyException(fmt::format("Failed to load certificate chain from {}, only P-256 "
-                                           "ECDSA certificates are supported",
-                                           ctx.cert_chain_file_path_));
-        }
         ctx.is_ecdsa_ = true;
       } break;
       case EVP_PKEY_RSA: {
@@ -252,10 +241,10 @@ ContextImpl::ContextImpl(Stats::Scope& scope, const Envoy::Ssl::ContextConfig& c
                           ctx.cert_chain_file_path_));
         }
 #else
-        if (rsa_key_length < 2048 / 8) {
+        if (rsa_key_length < 1024 / 8) {
           throw EnvoyException(
               fmt::format("Failed to load certificate chain from {}, only RSA "
-                          "certificates with 2048-bit or larger keys are supported",
+                          "certificates with 1024-bit or larger keys are supported",
                           ctx.cert_chain_file_path_));
         }
 #endif
@@ -982,7 +971,9 @@ bool ServerContextImpl::isClientEcdsaCapable(const SSL_CLIENT_HELLO* ssl_client_
             CBS_len(&signature_algorithms_ext) != 0) {
           return false;
         }
-        if (cbsContainsU16(signature_algorithms, SSL_SIGN_ECDSA_SECP256R1_SHA256)) {
+        if (cbsContainsU16(signature_algorithms, SSL_SIGN_ECDSA_SECP256R1_SHA256) ||
+            cbsContainsU16(signature_algorithms, SSL_SIGN_ECDSA_SECP384R1_SHA384) ||
+            cbsContainsU16(signature_algorithms, SSL_SIGN_ECDSA_SECP521R1_SHA512)) {
           return true;
         }
       }

[github-actions]

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or "no stalebot" or other activity occurs. Thank you for your contributions.

[github-actions]

This issue has been automatically closed because it has not had activity in the last 37 days. If this issue is still valid, please ping a maintainer and ask them to label it as "help wanted" or "no stalebot". Thank you for your contributions.

[guthrey-coy]

@ggreenway is there any timeline for when this behavior might be made available?

[ggreenway]

Nobody is currently working on it, so there is no timeline currently.