tls: client ECDSA detection during ClientHello. #5200

htuch · 2018-12-03T22:26:14Z

This PR adds support to ServerContextImpl::selectTlsContext(), allowing
for the selection of a certificate based on whether the client is RSA
and/or ECDSA capable from a list.

Since the list is still a singleton due to config ingest limitations,
we're still effectively single certificate, but this PR exercises the
logic that will be used in the multi-certificate selection case.
Ideally, there should be no observable behavioral changes.

Risk Level: Low (this should continue to fallback to the default
certificate, even if the ECDSA code is exercised).
Testing: Some new parameterized integration tests.

Signed-off-by: Harvey Tuch htuch@google.com

This PR adds support to ServerContextImpl::selectTlsContext(), allowing for the selection of a certificate based on whether the client is RSA and/or ECDSA capable from a list. Since the list is still a singleton due to config ingest limitations, we're still effectively single certificate, but this PR exercises the logic that will be used in the multi-certificate selection case. Ideally, there should be no observable behavioral changes. Risk Level: Low (this should continue to fallback to the default certificate, even if the ECDSA code is exercised). Testing: Some new paramterized integration tests. Signed-off-by: Harvey Tuch <htuch@google.com>

htuch · 2018-12-03T22:27:18Z

@davidben @PiotrSikora can you folks take a look at the detection algorithm and compare with the reference algorithm we discussed? I can add more tests as well, I just want to make sure we're in agreement first on what to test. Thanks.

PiotrSikora · 2018-12-04T21:16:47Z

include/envoy/ssl/context_config.h

+  /**
+   * @return const std::string& with the signature algorithms for the context.
+   */
+  virtual const std::string& sigalgs() const PURE;


Nit: signingAlgorithmsForTest().

PiotrSikora · 2018-12-04T21:25:24Z

source/common/ssl/context_impl.cc

+      return false;
+    }
+
+    if (named_curve == 23 /* secp256r1 */) {


Please check for stronger curves as well, i.e.

if (named_curve == SSL_CURVE_SECP256R1 || named_curve == SSL_CURVE_SECP384R1 || named_curve == SSL_CURVE_SECP521R1) {

That's not quite right either. You need to specifically check for the curve that your certificate uses. The code this was based on assumes server ECDSA certificates are always P-256. (P-256 receives by far the bulk of security hardening and performance work. Until earlier this year, our P-384 and P-521 implementations were not even constant-time, and we are unlikely to pay the binary size to optimize either with pre-computed base point tables.)

If Envoy also only cares about P-256 (good idea) this code should stay as-is (but do replace 23 with SSL_CURVE_SECP256R1). If it wants to support others, it should extract the curve type and look for just that one.

I'm going to follow the logic that will arrive with SSL_IDENTITY. I'll find the first ECDSA certificate in the list, extract its curve and use this in the check. Does this make sense to you and @davidben?

If you extract just the first ECDSA certificate's curve, then you should only consider the first certificate because the check won't be valid for the others.

But I would recommend just doing P-256. Again, the other curves do not see security or performance work.

PiotrSikora · 2018-12-04T21:28:18Z

source/common/ssl/context_impl.cc

+    const uint8_t* supported_versions_data;
+    size_t supported_versions_len;
+    if (SSL_early_callback_ctx_extension_get(ssl_client_hello, TLSEXT_TYPE_supported_versions,
+                                             &supported_versions_data, &supported_versions_len)) {


In theory, we should verify that client announced TLS 1.3 in supported_versions.

I'm going to follow the reference implementation here and leave as is, we can discuss offline.

PiotrSikora · 2018-12-04T21:39:26Z

test/config/utility.cc

+  // We'll negotiate up to TLSv1.3, but it really depends on what the client
+  // sets.
+  common_tls_context.mutable_tls_params()->set_tls_maximum_protocol_version(
+      envoy::api::v2::auth::TlsParameters::TLSv1_3);


I'm not in love with this change, since we don't have TLS 1.3 enabled by default yet. Could we get away with setting this only when needed?

PiotrSikora · 2018-12-04T21:42:24Z

source/common/ssl/context_impl.cc

@@ -25,7 +25,8 @@ namespace Envoy {
 namespace Ssl {

 ContextImpl::ContextImpl(Stats::Scope& scope, const ContextConfig& config, TimeSource& time_source)


I think it would make sense to verify that you're adding at most one RSA and one ECDSA certificate, since SSL_IDENTITY might reject configs like that. cc @davidben

I didn't have particular plans to reject it though, at least with the PR as-is, I don't think anything but the first ECDSA certificate and the first RSA certificate will ever be used.

Based on what @davidben writes, I think we should be more permissive today. In general, considering that this code is throw away when SSL_IDENTITY arrives, we should aim to minimize the number of additional branches and testing complexity.

@davidben yeah, that's my point... I'd like to prevent people from accidentally configuring two ECDSA certificates, where the second one will be always ignored, etc.

@htuch fair enough.

@htuch I think you are confusing two related decisions.

If you only look at one curve, then obviously only one ECDSA certificate will be used and multiple are pointless. If you look at multiple curves, having both a P-256 and a P-384 certificate is potentially useful. However, P-384 and P-521 are themselves pointless, so that informs the first decision.

I think I'm going to stick to P-256 only for simplicity and validate the certificate at config ingestion time to ensure it meets this requirement.

davidben · 2018-12-04T22:19:20Z

include/envoy/ssl/context_config.h

@@ -88,6 +88,11 @@ class ClientContextConfig : public virtual ContextConfig {
   * @return The maximum number of session keys to store.
   */
  virtual size_t maxSessionKeys() const PURE;
+
+  /**
+   * @return const std::string& with the signature algorithms for the context.


There is no standard string representation of a signature algorithm list. This should document what format is being used.

davidben · 2018-12-04T22:19:47Z

source/common/ssl/context_impl.cc

@@ -25,7 +25,8 @@ namespace Envoy {
 namespace Ssl {

 ContextImpl::ContextImpl(Stats::Scope& scope, const ContextConfig& config, TimeSource& time_source)


I didn't have particular plans to reject it though, at least with the PR as-is, I don't think anything but the first ECDSA certificate and the first RSA certificate will ever be used.

davidben · 2018-12-04T22:20:55Z

source/common/ssl/context_impl.cc

+  if (!config.sigalgs().empty()) {
+    for (auto& ctx : tls_contexts_) {
+      int rc = SSL_CTX_set1_sigalgs_list(ctx.ssl_ctx_.get(), config.sigalgs().c_str());
+      RELEASE_ASSERT(rc == 1, "");


(Note this will fail on syntax error. Is that okay for your purposes here?)

Yeah, this is just for tests; I've done the rename suggested by @PiotrSikora, so we are good here.

davidben · 2018-12-04T22:21:16Z

source/common/ssl/context_impl.cc

+  CBS_init(&client_hello, ssl_client_hello->client_hello, ssl_client_hello->client_hello_len);
+
+  uint16_t client_version;
+  if (!CBS_get_u16(&client_hello, &client_version)) {


There is no need to parse this value out. It's already in ssl_client_hello->version.

davidben · 2018-12-04T22:24:14Z

source/common/ssl/context_impl.cc

+      return false;
+    }
+
+    if (named_curve == 23 /* secp256r1 */) {


That's not quite right either. You need to specifically check for the curve that your certificate uses. The code this was based on assumes server ECDSA certificates are always P-256. (P-256 receives by far the bulk of security hardening and performance work. Until earlier this year, our P-384 and P-521 implementations were not even constant-time, and we are unlikely to pay the binary size to optimize either with pre-computed base point tables.)

If Envoy also only cares about P-256 (good idea) this code should stay as-is (but do replace 23 with SSL_CURVE_SECP256R1). If it wants to support others, it should extract the curve type and look for just that one.

davidben · 2018-12-04T22:25:41Z

source/common/ssl/context_impl.cc

@@ -866,5 +988,16 @@ void ServerContextImpl::TlsContext::addClientValidationContext(
  }
 }

+bool ServerContextImpl::TlsContext::isCipherEnabled(const SSL_CIPHER* cipher) {
+  STACK_OF(SSL_CIPHER)* our_ciphers = SSL_CTX_get_ciphers(ssl_ctx_.get());
+  for (size_t j = 0; j < sk_SSL_CIPHER_num(our_ciphers); j++) {


You can just write:

for (const SSL_CIPHER* our_c : SSL_CTX_get_ciphers(ssl_ctx_.get())) { if (SSL_CIPHER_get_id(our_c) == SSL_CIPHER_get_id(cipher)) { return true; } } return false;

I was bored one day and made range-for loops work on STACK_OF(T). :-)

davidben · 2018-12-04T22:27:59Z

source/common/ssl/context_impl.cc

+          return false;
+        }
+
+        while (CBS_len(&signature_algorithms) > 0) {


Optional: you could probably abstract this and the curves loop into a cbsContainsU16(&signature_algorithms, SSL_SIGN_ECDSA_SECP256R1_SHA256) and similar for the curve.

Signed-off-by: Harvey Tuch <htuch@google.com>

In support of envoyproxy#5200. There are no performance or security advantages today of non-P256 ECDSA certs. Risk Level: Low (unlikely anyone is using these). Testing: Pending; will add when we have agreement this is where we want to go. Signed-off-by: Harvey Tuch <htuch@google.com>

PiotrSikora · 2018-12-05T21:20:50Z

source/common/ssl/context_impl.cc

+      return false;
+    }
+
+    const SSL_CIPHER* c = SSL_get_cipher_by_value(cipher_id);


You could probably push SSL_get_cipher_by_value(), SSL_CIPHER_get_min_version() and SSL_CIPHER_get_auth_nid() into isCipherEnabled() to make this loop more readable, i.e.:

while (CBS_len(&cipher_suites) > 0) { uint16_t cipher_id; if (!CBS_get_u16(&cipher_suites, &cipher_id)) { return false; } // 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)) { return true; } }

Signed-off-by: Harvey Tuch <htuch@google.com>

In support of #5200. There are no performance or security advantages today of non-P256 ECDSA certs. Risk Level: Low (unlikely anyone is using these). Testing: Unit tests added to ssl:context_impl_test for happy/sad paths. Signed-off-by: Harvey Tuch <htuch@google.com>

PiotrSikora

Oops, sorry for the delay! I was sure I that approved this yesterday along with the P-256 PR.

PiotrSikora · 2018-12-06T23:49:58Z

source/common/ssl/context_impl.cc

@@ -812,11 +839,88 @@ int ServerContextImpl::sessionTicketProcess(SSL*, uint8_t* key_name, uint8_t* iv
  }
 }

+bool ServerContextImpl::isClientEcdsaCapable(const SSL_CLIENT_HELLO* ssl_client_hello) {
+  const uint16_t client_version = ssl_client_hello->version;


Nit: not that it matters, but I would move it just before the first use, i.e. a line before the if statement.

PiotrSikora · 2018-12-07T21:41:57Z

test/integration/ssl_integration_test.cc

+}
+
+// Validate certificate selection across different certificate types and client
+// TLS versions.


Nit: comment wrapped at 80 chars again?

Signed-off-by: Harvey Tuch <htuch@google.com>

In support of envoyproxy#5200. There are no performance or security advantages today of non-P256 ECDSA certs. Risk Level: Low (unlikely anyone is using these). Testing: Unit tests added to ssl:context_impl_test for happy/sad paths. Signed-off-by: Harvey Tuch <htuch@google.com> Signed-off-by: Fred Douglas <fredlas@google.com>

This PR adds support to ServerContextImpl::selectTlsContext(), allowing for the selection of a certificate based on whether the client is RSA and/or ECDSA capable from a list. Since the list is still a singleton due to config ingest limitations, we're still effectively single certificate, but this PR exercises the logic that will be used in the multi-certificate selection case. Ideally, there should be no observable behavioral changes. Risk Level: Low (this should continue to fallback to the default certificate, even if the ECDSA code is exercised). Testing: Some new parameterized integration tests. Part of envoyproxy#1319 Signed-off-by: Harvey Tuch <htuch@google.com> Signed-off-by: Fred Douglas <fredlas@google.com>

varsraja · 2019-07-11T06:10:09Z

Hi,
Can some one help us on why non P256 ECDSA certs are not allowed in envoy proxy.
We have our server certs generated with P512 ECDSA and would like to use them in the envoy proxy.
Are there plans to support the other curves of ECDSA ?
We cant afford to change our algo in certs, so can you suggest an alertnate way if envoy proxy is not going to support the other curves of ECDSA.

htuch requested review from mkbehr, davidben and PiotrSikora and removed request for mkbehr December 3, 2018 22:26

htuch assigned davidben and PiotrSikora Dec 3, 2018

PiotrSikora suggested changes Dec 4, 2018

View reviewed changes

Merge remote-tracking branch 'upstream/master' into ecdsa-detection

3a7c08a

davidben reviewed Dec 4, 2018

View reviewed changes

htuch added 2 commits December 4, 2018 17:34

s/sigalgs/signingAlgorithmsForTest/

cd2dfff

Signed-off-by: Harvey Tuch <htuch@google.com>

Rest of review feedback.

b480487

Signed-off-by: Harvey Tuch <htuch@google.com>

htuch mentioned this pull request Dec 5, 2018

tls: reject non-P256 TLS certificates. #5224

Merged

PiotrSikora reviewed Dec 5, 2018

View reviewed changes

htuch added 2 commits December 6, 2018 13:10

Merge remote-tracking branch 'upstream/master' into ecdsa-detection

86ed9fb

Refactor isCipherEnabled().

36c0664

Signed-off-by: Harvey Tuch <htuch@google.com>

PiotrSikora previously approved these changes Dec 7, 2018

View reviewed changes

htuch added 2 commits December 9, 2018 10:39

Merge remote-tracking branch 'upstream/master' into ecdsa-detection

5e991d1

Nits.

c1ef2a2

Signed-off-by: Harvey Tuch <htuch@google.com>

htuch dismissed PiotrSikora’s stale review via c1ef2a2 December 9, 2018 15:51

htuch merged commit eb0cdd7 into envoyproxy:master Dec 9, 2018

htuch deleted the ecdsa-detection branch December 9, 2018 17:08

ggreenway mentioned this pull request Jul 21, 2023

Support P-384 and P-521 Server ECDSA Certificates #10855

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tls: client ECDSA detection during ClientHello. #5200

tls: client ECDSA detection during ClientHello. #5200

htuch commented Dec 3, 2018 •

edited

htuch commented Dec 3, 2018

PiotrSikora Dec 4, 2018

PiotrSikora Dec 4, 2018

davidben Dec 4, 2018

htuch Dec 4, 2018

PiotrSikora Dec 4, 2018

davidben Dec 4, 2018

PiotrSikora Dec 4, 2018

htuch Dec 4, 2018

PiotrSikora Dec 4, 2018

PiotrSikora Dec 4, 2018

davidben Dec 4, 2018

htuch Dec 4, 2018

PiotrSikora Dec 4, 2018

davidben Dec 4, 2018

htuch Dec 5, 2018

htuch Dec 5, 2018

davidben Dec 4, 2018

davidben Dec 4, 2018

davidben Dec 4, 2018

htuch Dec 4, 2018

davidben Dec 4, 2018

davidben Dec 4, 2018

davidben Dec 4, 2018

davidben Dec 4, 2018

PiotrSikora Dec 5, 2018 •

edited

PiotrSikora left a comment

PiotrSikora Dec 6, 2018

PiotrSikora Dec 7, 2018

varsraja commented Jul 11, 2019

		@@ -25,7 +25,8 @@ namespace Envoy {
		namespace Ssl {

		ContextImpl::ContextImpl(Stats::Scope& scope, const ContextConfig& config, TimeSource& time_source)

tls: client ECDSA detection during ClientHello. #5200

tls: client ECDSA detection during ClientHello. #5200

Conversation

htuch commented Dec 3, 2018 • edited

htuch commented Dec 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PiotrSikora Dec 5, 2018 • edited

Choose a reason for hiding this comment

PiotrSikora left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

varsraja commented Jul 11, 2019

htuch commented Dec 3, 2018 •

edited

PiotrSikora Dec 5, 2018 •

edited