ssl: remember stat names for configured ciphers. #14534

jmarantz · 2020-12-29T15:46:04Z

Commit Message: Stats are only kept for a set of known SSL ciphers, to bound memory use. That set was previously determined by running unit tests and capturing which ciphers were referenced. This PR changes it to use the configured ciphers.
Additional Description:
Risk Level: medium
Testing: //test/...
Docs Changes: n/a
Release Notes: n/a
Fixes: #14524

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz · 2020-12-29T18:41:02Z

source/extensions/transport_sockets/tls/context_impl.cc

+  // purposes of collecting stats -- we want to track all these names. We don't
+  // need to fully parse out the structure of the cipher suites -- just extract
+  // out the names.
+  for (absl::string_view cipher_suite :


@PiotrSikora is there a better way to do this? E.g. an SSL method?

I'm explicitly including the exclusion-patterns here, though it might not really do any good, because it looks like those are not whole segments from a stat-name perspective.

You could get all configured cipher suites using SSL_CTX_get_ciphers() and iterate over them using SSL_CIPHER_get_name() to get the proper names.

This will cover all the cipher suites that can be negotiated in a given context.

Great! Done.

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz · 2020-12-29T22:47:44Z

source/extensions/transport_sockets/tls/context_impl.cc

+  //
+  // TODO(jmarantz): consider whether we need this hard-coded list in addition
+  // to the ones from ecdhCurves below. Note there is no harm in remembering a
+  // curve more than once.


@PiotrSikora do you think I can remove the hardcoded set here and just use ecdhCurves()?

Ideally, we would use the configured curves from SSL_CTX, since there are a few naming conventions and users might use secp256r1 or prime256v1 instead of P-256, in which case we wouldn't be able to log the stats for them, but it doesn't look that there is a getter for that in BoringSSL.

With that in mind, we could either use the hardcoded list as before, possibly dropping the experimental post-quantum curves from the list (CECPQ2*), or we could try to map all alternative names provided in config.ecdhCurves() to NIST names (the alternatives are listed in RFC4492, appendix A). Either one is fine.

I think I'll leave it as is; a small number of preconfigured curves, combined with the ones from the config, is not a problem; we just don't want to have the space unbounded.

akonradi · 2020-12-30T18:11:09Z

source/extensions/transport_sockets/tls/context_impl.cc

+  // This cipher is referenced in a test, though it's not obvious how or why.
  stat_name_set_->rememberBuiltin("TLS_AES_128_GCM_SHA256");


Which test? Is this something we can/should fix?

I added documentation about what tests caused us to try to record values on stats that were not recorded from the cipher-suites config above. Turns out I missed one, so I added that.

It's probably worth digging into why those names showed up, but probably that should be a follow-up, and would need an SSL expert's help.

source/extensions/transport_sockets/tls/context_impl.cc

akonradi · 2020-12-30T18:12:43Z

source/extensions/transport_sockets/tls/context_impl.cc

+  // stat-name segments, so they would not match.
+  for (absl::string_view cipher_suite :
+       absl::StrSplit(config.cipherSuites(), absl::ByAnyChar(":|[]"))) {
+    if (!cipher_suite.empty() && cipher_suite[0] != '!') { // skip exclusions.


Use absl::StartsWith instead of indexing?

I like this better as it also pruned out any empty strings. Updated the inline comment as well.

Sorry, I meant keep the .empty() and replace the equality check with StartsWith.

source/extensions/transport_sockets/tls/context_impl.h

Signed-off-by: Joshua Marantz <jmarantz@google.com>

PiotrSikora · 2020-12-30T23:10:07Z

source/extensions/transport_sockets/tls/context_impl.cc

+  // purposes of collecting stats -- we want to track all these names. We don't
+  // need to fully parse out the structure of the cipher suites -- just extract
+  // out the names.
+  for (absl::string_view cipher_suite :


You could get all configured cipher suites using SSL_CTX_get_ciphers() and iterate over them using SSL_CIPHER_get_name() to get the proper names.

This will cover all the cipher suites that can be negotiated in a given context.

PiotrSikora · 2020-12-30T23:17:36Z

source/extensions/transport_sockets/tls/context_impl.cc

+  // possibly due to the call to
+  // ClientSslTransportOptions().setSigningAlgorithmsForTest
+  // in test/extensions/transport_sockets/tls/integration/ssl_integration_test.cc, function
+  // rsaOnlyClientOptions.
  stat_name_set_->rememberBuiltin("TLS_AES_128_GCM_SHA256");


This is one of the cipher suites hardcoded into TLS 1.3 spec. The other two that you should also add are: TLS_AES_256_GCM_SHA384 and TLS_CHACHA20_POLY1305_SHA256.

Also, please update the comment to reflect the source of those cipher suites.

PiotrSikora · 2020-12-30T23:17:57Z

source/extensions/transport_sockets/tls/context_impl.cc

+  // "TLS_RSA_WITH_AES_128_GCM_SHA256" in the configuration for
+  // SslSocketTest.RsaPrivateKeyProviderAsyncDecryptSuccess in
+  // test/extensions/transport_sockets/tls/ssl_socket_test.cc
+  stat_name_set_->rememberBuiltin("AES128-GCM-SHA256");


This should be covered by the "configured cipher suites" alternative suggested above.

Yup -- was able to drop this.

PiotrSikora · 2020-12-30T23:35:38Z

source/extensions/transport_sockets/tls/context_impl.cc

+  //
+  // TODO(jmarantz): consider whether we need this hard-coded list in addition
+  // to the ones from ecdhCurves below. Note there is no harm in remembering a
+  // curve more than once.


Ideally, we would use the configured curves from SSL_CTX, since there are a few naming conventions and users might use secp256r1 or prime256v1 instead of P-256, in which case we wouldn't be able to log the stats for them, but it doesn't look that there is a getter for that in BoringSSL.

With that in mind, we could either use the hardcoded list as before, possibly dropping the experimental post-quantum curves from the list (CECPQ2*), or we could try to map all alternative names provided in config.ecdhCurves() to NIST names (the alternatives are listed in RFC4492, appendix A). Either one is fine.

test/extensions/transport_sockets/tls/context_impl_test.cc

…plus other review comments. Signed-off-by: Joshua Marantz <jmarantz@google.com>

…e ctor. Signed-off-by: Joshua Marantz <jmarantz@google.com>

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz · 2021-01-01T20:36:12Z

/retest

repokitteh-read-only · 2021-01-01T20:36:16Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #14534 (comment) was created by @jmarantz.

see: more, trace.

PiotrSikora · 2021-01-02T08:09:02Z

source/extensions/transport_sockets/tls/context_impl.cc

+  // Use the SSL library to iterate over the configured ciphers.
+  for (TlsContext& tls_context : tls_contexts_) {
+    bssl::UniquePtr<SSL> ssl = bssl::UniquePtr<SSL>(SSL_new(tls_context.ssl_ctx_.get()));
+    STACK_OF(SSL_CIPHER)* ciphers = SSL_get_ciphers(ssl.get());


You can use SSL_CTX_get_ciphers() instead, and then you don't need to instantiate the SSL object.

You should be also able to use the smart iterator, i.e.:

for (const SSL_CIPHER *cipher : SSL_CTX_get_ciphers(tls_context.ssl_ctx_.get())) {

Much better; thanks. I didn't know this would just work this way as it wasn't in the docs I found.

source/extensions/transport_sockets/tls/context_impl.cc

PiotrSikora · 2021-01-02T08:16:22Z

source/extensions/transport_sockets/tls/context_impl.cc


  // Curves from
  // https://github.com/google/boringssl/blob/f4d8b969200f1ee2dd872ffb85802e6a0976afe7/ssl/ssl_key_share.cc#L384
  stat_name_set_->rememberBuiltins(
      {"P-224", "P-256", "P-384", "P-521", "X25519", "CECPQ2", "CECPQ2b"});
+  for (absl::string_view curve : absl::StrSplit(config.ecdhCurves(), ':')) {
+    stat_name_set_->rememberBuiltin(curve);
+  }


The hardcoded list is exhaustive, so iterating over configure curves is always a no-op.

Ack. I added comments acknowledging the potential redundancy but I feel like it's a bit safer to over-declare here, and there does not appear to be a downside.

Why is it "safer"? The original list is exhaustive already, and because of the alternative naming conventions mentioned before, configured list might include alternative names that can never appear in the stats, so there is nothing to gain, and it can potentially waste some memory.

What I was concerned about was that my list here would cease to become exhaustive with some future update to TLS. I'm not sure how likely/frequent that is.

If there's a list of alternative names that can't appear i stats, I could filter the configured curves so we wouldn't bother registering them. But this is really a pretty tiny amount of memory, as we are not configure stats here; just stat names. So it's just string.size() + a few extra words of overhead for the ref-counted Symbol in the SymbolTable and some map entries. So I'm not sure it's worth the added complexity unless there are going to be hundreds of them.

The flip-side is that if we don't remember builtins for configured curves, and a new curve is introduced in a future version of SSL, and is configured by users, we wouldn't get stats for it. We would get a log error for it though, so maybe that's OK and we can deal with it when it happens. I just don't know how often SSL would add new curves. Do you have a sense?

I think you should at least do a case-insensitive compare to the hard coded list and remove duplicates. Or just use the hard-coded list as @PiotrSikora recommends, and add an ENVOY_BUG if the stat for curve doesn't exist when we try to increment it.

Oh that's probably a good semantic; maybe I should use ENVOY_BUG rather than ENVOY_LOG_PERIODIC(error... for ciphers too.

It will be annoying to test though (I do have a unit-test that hits that line).

I understand the reasoning, but other than the experimental post-quantum cipher suites (CECPQ2 and CECPQ2 - the latter which was already removed), that list didn't change in years.

If you really want, we could parse kNamedGroups from BoringSSL as part of the format check, and make sure that hardcoded list matches, but I feel that this is way over-engineered for what it needs to be.

I removed the loop over the configured curves and am just using the hard-coded ones, with a switch to use ENVOY_BUG to catch unexpected curves during debug/test, and log them in release.

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz · 2021-01-03T05:08:23Z

/retest

repokitteh-read-only · 2021-01-03T05:08:28Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #14534 (comment) was created by @jmarantz.

see: more, trace.

PiotrSikora

LGTM with small nits.

source/extensions/transport_sockets/tls/context_impl.cc

PiotrSikora · 2021-01-03T08:42:43Z

source/extensions/transport_sockets/tls/context_impl.cc


  // Curves from
  // https://github.com/google/boringssl/blob/f4d8b969200f1ee2dd872ffb85802e6a0976afe7/ssl/ssl_key_share.cc#L384
  stat_name_set_->rememberBuiltins(
      {"P-224", "P-256", "P-384", "P-521", "X25519", "CECPQ2", "CECPQ2b"});
+  for (absl::string_view curve : absl::StrSplit(config.ecdhCurves(), ':')) {
+    stat_name_set_->rememberBuiltin(curve);
+  }


Why is it "safer"? The original list is exhaustive already, and because of the alternative naming conventions mentioned before, configured list might include alternative names that can never appear in the stats, so there is nothing to gain, and it can potentially waste some memory.

jmarantz

I will push the comment-change after we settle on the strategy for stats for curves.

source/extensions/transport_sockets/tls/context_impl.cc

jmarantz · 2021-01-03T15:44:59Z

source/extensions/transport_sockets/tls/context_impl.cc


  // Curves from
  // https://github.com/google/boringssl/blob/f4d8b969200f1ee2dd872ffb85802e6a0976afe7/ssl/ssl_key_share.cc#L384
  stat_name_set_->rememberBuiltins(
      {"P-224", "P-256", "P-384", "P-521", "X25519", "CECPQ2", "CECPQ2b"});
+  for (absl::string_view curve : absl::StrSplit(config.ecdhCurves(), ':')) {
+    stat_name_set_->rememberBuiltin(curve);
+  }


What I was concerned about was that my list here would cease to become exhaustive with some future update to TLS. I'm not sure how likely/frequent that is.

If there's a list of alternative names that can't appear i stats, I could filter the configured curves so we wouldn't bother registering them. But this is really a pretty tiny amount of memory, as we are not configure stats here; just stat names. So it's just string.size() + a few extra words of overhead for the ref-counted Symbol in the SymbolTable and some map entries. So I'm not sure it's worth the added complexity unless there are going to be hundreds of them.

The flip-side is that if we don't remember builtins for configured curves, and a new curve is introduced in a future version of SSL, and is configured by users, we wouldn't get stats for it. We would get a log error for it though, so maybe that's OK and we can deal with it when it happens. I just don't know how often SSL would add new curves. Do you have a sense?

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz · 2021-01-04T14:46:02Z

It probably makes sense to see @envoyproxy/senior-maintainers approval at this point while @PiotrSikora and I iterate on how exactly to future-proof the curve-name registration. We can also iterate on that in a follow-up.

jmarantz · 2021-01-04T14:46:12Z

/retest

repokitteh-read-only · 2021-01-04T14:46:17Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #14534 (comment) was created by @jmarantz.

see: more, trace.

source/extensions/transport_sockets/tls/context_impl.cc

ggreenway · 2021-01-05T19:40:48Z

source/extensions/transport_sockets/tls/context_impl.cc


  // Curves from
  // https://github.com/google/boringssl/blob/f4d8b969200f1ee2dd872ffb85802e6a0976afe7/ssl/ssl_key_share.cc#L384
  stat_name_set_->rememberBuiltins(
      {"P-224", "P-256", "P-384", "P-521", "X25519", "CECPQ2", "CECPQ2b"});
+  for (absl::string_view curve : absl::StrSplit(config.ecdhCurves(), ':')) {
+    stat_name_set_->rememberBuiltin(curve);
+  }


I think you should at least do a case-insensitive compare to the hard coded list and remove duplicates. Or just use the hard-coded list as @PiotrSikora recommends, and add an ENVOY_BUG if the stat for curve doesn't exist when we try to increment it.

source/extensions/transport_sockets/tls/context_impl.cc

test/extensions/transport_sockets/tls/context_impl_test.cc

ggreenway · 2021-01-05T19:44:56Z

test/extensions/transport_sockets/tls/context_impl_test.cc

+  EXPECT_EQ(1, cipher->get().value());
+
+  // Incrementing a stat for a random unknown cipher does not work. A
+  // rate-limited error log message will also be generated but that is hard to


As noted above, an ENVOY_BUG would make this easier to test

not super-easy, but done :) The trick with ENVOY_BUG is that (from what I saw) it won't execute the fallback logic when it aborts.

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz · 2021-01-06T00:51:34Z

/retest

repokitteh-read-only · 2021-01-06T00:51:38Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #14534 (comment) was created by @jmarantz.

see: more, trace.

ggreenway

LGTM

* master: (48 commits) Resolve 14506, avoid libidn2 for our curl dependency (envoyproxy#14601) fix new/free mismatch in Mainthread utility (envoyproxy#14596) opencensus: deprecate Zipkin configuration. (envoyproxy#14576) upstream: clean up code location (envoyproxy#14580) configuration impl: add cast for ios compilation (envoyproxy#14590) buffer impl: add cast for android compilation (envoyproxy#14589) ratelimit: add dynamic metadata to ratelimit response (envoyproxy#14508) tcp_proxy: wait for CONNECT response before start streaming data (envoyproxy#14317) stream info: cleanup address handling (envoyproxy#14432) [deps] update upb to latest commit (envoyproxy#14582) Add utility to check whether the execution is in main thread. (envoyproxy#14457) listener: undeprecate bind_to_port (envoyproxy#14480) Fix data race in overload integration test (envoyproxy#14586) deps: update PGV (envoyproxy#14571) dependencies: update cve_scan.py for some libcurl 7.74.0 false positives. (envoyproxy#14572) Network::Connection: Add L4 crash dumping support (envoyproxy#14509) ssl: remember stat names for configured ciphers. (envoyproxy#14534) formatter: add custom date formatting to downstream cert start and end dates (envoyproxy#14502) feat(lua): allow setting response body when the upstream response body is empty (envoyproxy#14486) Generalize the gRPC access logger base classes (envoyproxy#14469) ... Signed-off-by: Michael Puncel <mpuncel@squareup.com>

Remember stats for configured ciphers.

d3455ae

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz changed the title ~~Remember stats for configured ciphers.~~ ssl: Remember stats for configured ciphers. Dec 29, 2020

jmarantz changed the title ~~ssl: Remember stats for configured ciphers.~~ ssl: remember stats for configured ciphers. Dec 29, 2020

jmarantz assigned PiotrSikora Dec 29, 2020

jmarantz commented Dec 29, 2020

View reviewed changes

clean up clang-tidy error and skip exclusion.

a8f7b25

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz commented Dec 29, 2020

View reviewed changes

jmarantz marked this pull request as ready for review December 30, 2020 14:45

jmarantz requested review from asraa, ggreenway, lizan and PiotrSikora as code owners December 30, 2020 14:45

jmarantz assigned akonradi Dec 30, 2020

akonradi suggested changes Dec 30, 2020

View reviewed changes

jmarantz added 2 commits December 30, 2020 14:01

Merge branch 'master' into more-ssl-cipher-stats

5a6e778

Signed-off-by: Joshua Marantz <jmarantz@google.com>

Address review comments.

fd85180

Signed-off-by: Joshua Marantz <jmarantz@google.com>

akonradi previously approved these changes Dec 30, 2020

View reviewed changes

PiotrSikora suggested changes Dec 30, 2020

View reviewed changes

jmarantz changed the title ~~ssl: remember stats for configured ciphers.~~ ssl: remember stat names for configured ciphers. Dec 31, 2020

Use SSL API to enumerate the ciphers rather than parsing the config, …

af644ed

…plus other review comments. Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz dismissed akonradi’s stale review via af644ed December 31, 2020 15:26

jmarantz added 2 commits December 31, 2020 20:17

work around clang-tidy warning about calling a virtual method from th…

ec3fb68

…e ctor. Signed-off-by: Joshua Marantz <jmarantz@google.com>

allow stat-names from all tls contexts.

9fb5df3

Signed-off-by: Joshua Marantz <jmarantz@google.com>

PiotrSikora suggested changes Jan 2, 2021

View reviewed changes

jmarantz added 2 commits January 2, 2021 09:43

review comments

9d7d0ed

Signed-off-by: Joshua Marantz <jmarantz@google.com>

Merge branch 'master' into more-ssl-cipher-stats

91dbc25

Signed-off-by: Joshua Marantz <jmarantz@google.com>

PiotrSikora previously approved these changes Jan 3, 2021

View reviewed changes

jmarantz commented Jan 3, 2021

View reviewed changes

comment rewording

e5e3192

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz dismissed PiotrSikora’s stale review via e5e3192 January 4, 2021 04:11

ggreenway requested changes Jan 5, 2021

View reviewed changes

ggreenway self-assigned this Jan 5, 2021

jmarantz added 2 commits January 5, 2021 16:00

Merge branch 'master' into more-ssl-cipher-stats

26ca039

Signed-off-by: Joshua Marantz <jmarantz@google.com>

review comments

2f37980

Signed-off-by: Joshua Marantz <jmarantz@google.com>

ggreenway approved these changes Jan 6, 2021

View reviewed changes

jmarantz merged commit 568e6d6 into envoyproxy:master Jan 6, 2021

jmarantz deleted the more-ssl-cipher-stats branch January 6, 2021 19:41

This was referenced Jan 12, 2021

[tls] add missing built in cipher stat names #14676

Merged

tls: add missing stats for signature algorithms. #14703

Merged

Unexpected ssl.sigalgs error in version 1.17 #14708

Closed

oschaaf mentioned this pull request Jan 29, 2021

Used ciphers may not always be reflected in stats output envoyproxy/nighthawk#156

Closed

oschaaf mentioned this pull request Feb 8, 2021

MAISTRA-2124: Ssl: avoid tracking "unknown" ciphers maistra/envoy#69

Merged

		// This cipher is referenced in a test, though it's not obvious how or why.
		stat_name_set_->rememberBuiltin("TLS_AES_128_GCM_SHA256");

ssl: remember stat names for configured ciphers. #14534

ssl: remember stat names for configured ciphers. #14534

Conversation

jmarantz commented Dec 29, 2020 • edited Loading

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

jmarantz commented Jan 1, 2021

repokitteh-read-only bot commented Jan 1, 2021

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

jmarantz commented Jan 3, 2021

repokitteh-read-only bot commented Jan 3, 2021

PiotrSikora left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmarantz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmarantz commented Jan 4, 2021

jmarantz commented Jan 4, 2021

repokitteh-read-only bot commented Jan 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmarantz commented Jan 6, 2021

repokitteh-read-only bot commented Jan 6, 2021

ggreenway left a comment

Choose a reason for hiding this comment

jmarantz commented Dec 29, 2020 •

edited

Loading