Skip to content

ssl: remember stat names for configured ciphers. #14534

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

Merged
jmarantz merged 12 commits into from
Jan 6, 2021
Merged

ssl: remember stat names for configured ciphers. #14534

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
d3455ae
Remember stats for configured ciphers.
jmarantz Dec 29, 2020
a8f7b25
clean up clang-tidy error and skip exclusion.
jmarantz Dec 29, 2020
5a6e778
Merge branch 'master' into more-ssl-cipher-stats
jmarantz Dec 30, 2020
fd85180
Address review comments.
jmarantz Dec 30, 2020
af644ed
Use SSL API to enumerate the ciphers rather than parsing the config, …
jmarantz Dec 31, 2020
ec3fb68
work around clang-tidy warning about calling a virtual method from th…
jmarantz Jan 1, 2021
9fb5df3
allow stat-names from all tls contexts.
jmarantz Jan 1, 2021
9d7d0ed
review comments
jmarantz Jan 2, 2021
91dbc25
Merge branch 'master' into more-ssl-cipher-stats
jmarantz Jan 3, 2021
e5e3192
comment rewording
jmarantz Jan 3, 2021
26ca039
Merge branch 'master' into more-ssl-cipher-stats
jmarantz Jan 5, 2021
2f37980
review comments
jmarantz Jan 5, 2021
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
40 changes: 21 additions & 19 deletions source/extensions/transport_sockets/tls/context_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -463,27 +463,26 @@ ContextImpl::ContextImpl(Stats::Scope& scope, const Envoy::Ssl::ContextConfig& c

parsed_alpn_protocols_ = parseAlpnProtocols(config.alpnProtocols());

// To enumerate the required builtin ciphers, curves, algorithms, and
// versions, uncomment '#define LOG_BUILTIN_STAT_NAMES' below, and run
// bazel test //test/extensions/transport_sockets/tls/... --test_output=streamed
// | grep " Builtin ssl." | sort | uniq
// #define LOG_BUILTIN_STAT_NAMES
//
// TODO(#8035): improve tooling to find any other built-ins needed to avoid
// contention.

// Ciphers
stat_name_set_->rememberBuiltin("AEAD-AES128-GCM-SHA256");
stat_name_set_->rememberBuiltin("ECDHE-ECDSA-AES128-GCM-SHA256");
stat_name_set_->rememberBuiltin("ECDHE-RSA-AES128-GCM-SHA256");
stat_name_set_->rememberBuiltin("ECDHE-RSA-AES128-SHA");
stat_name_set_->rememberBuiltin("ECDHE-RSA-CHACHA20-POLY1305");
// Ciphers are configured as a string delimited by ":", with equivalence
// groups given as "[alt1|alt2]", and exclusions preceded by "!". For the
// purposes of collecting stats -- we want to track all these names. We don't
Comment thread
jmarantz marked this conversation as resolved.
Outdated
// need to fully parse out the structure of the cipher suites -- just extract
// out the names.
for (absl::string_view cipher_suite :

@jmarantz jmarantz Dec 29, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@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.

@PiotrSikora PiotrSikora Dec 30, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

@jmarantz jmarantz Dec 31, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Great! Done.

absl::StrSplit(config.cipherSuites(), absl::ByAnyChar(":|[]!"))) {
stat_name_set_->rememberBuiltin(cipher_suite);
}

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

@PiotrSikora PiotrSikora Dec 30, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.


// 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);
}

@PiotrSikora PiotrSikora Jan 2, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

@jmarantz jmarantz Jan 2, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

@PiotrSikora PiotrSikora Jan 3, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 jmarantz Jan 3, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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?

@ggreenway ggreenway Jan 5, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@jmarantz jmarantz Jan 5, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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).

@PiotrSikora PiotrSikora Jan 5, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

@jmarantz jmarantz Jan 6, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.


// Algorithms
stat_name_set_->rememberBuiltins({"ecdsa_secp256r1_sha256", "rsa_pss_rsae_sha256"});
Expand Down Expand Up @@ -640,12 +639,15 @@ Envoy::Ssl::ClientValidationStatus ContextImpl::verifyCertificate(

void ContextImpl::incCounter(const Stats::StatName name, absl::string_view value,
const Stats::StatName fallback) const {
Stats::Counter& counter = Stats::Utility::counterFromElements(
scope_, {name, stat_name_set_->getBuiltin(value, fallback)});
counter.inc();
const Stats::StatName value_stat_name = stat_name_set_->getBuiltin(value, fallback);
if (value_stat_name == fallback) {
ENVOY_LOG_PERIODIC_MISC(error, std::chrono::minutes(1), "Unexpected {} value: {}",
scope_.symbolTable().toString(name), value);
}
Stats::Utility::counterFromElements(scope_, {name, value_stat_name}).inc();

#ifdef LOG_BUILTIN_STAT_NAMES
std::cerr << absl::StrCat("Builtin ", symbol_table.toString(name), ": ", value, "\n")
std::cerr << absl::StrCat("Builtin ", scope_.symbolTable().toString(name), ": ", value, "\n")
Comment thread
jmarantz marked this conversation as resolved.
Outdated
<< std::flush;
#endif
}
Expand Down
2 changes: 2 additions & 0 deletions source/extensions/transport_sockets/tls/context_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,8 @@ class ContextImpl : public virtual Envoy::Ssl::Context {
bool verifyCertChain(X509& leaf_cert, STACK_OF(X509) & intermediates, std::string& error_details);

protected:
friend class SslContextStatsTest;

Comment thread
jmarantz marked this conversation as resolved.
Outdated
ContextImpl(Stats::Scope& scope, const Envoy::Ssl::ContextConfig& config,
TimeSource& time_source);

Expand Down
1 change: 1 addition & 0 deletions test/extensions/transport_sockets/tls/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ envoy_cc_test(
"//test/mocks/ssl:ssl_mocks",
"//test/mocks/stats:stats_mocks",
"//test/test_common:environment_lib",
"//test/test_common:logging_lib",
"//test/test_common:network_utility_lib",
"//test/test_common:registry_lib",
"//test/test_common:simulated_time_system_lib",
Expand Down
44 changes: 44 additions & 0 deletions test/extensions/transport_sockets/tls/context_impl_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1934,6 +1934,50 @@ TEST_F(ServerContextConfigImplTest, PrivateKeyMethodLoadFailureBothKeyAndMethod)
"Certificate configuration can't have both private_key and private_key_provider");
}

class SslContextStatsTest : public SslContextImplTest {
protected:
SslContextStatsTest() {
TestUtility::loadFromYaml(TestEnvironment::substitute(yaml), tls_context_);
client_context_config_ =
std::make_unique<ClientContextConfigImpl>(tls_context_, factory_context_);
context_.reset(new ContextImpl(store_, *client_context_config_, time_system_));
}

void incCounter(absl::string_view name1, absl::string_view name2) {
Stats::StatNamePool pool(store_.symbolTable());
context_->incCounter(pool.add(name1), name2, pool.add("fallback"));
}

Stats::TestUtil::TestStore store_;
envoy::extensions::transport_sockets::tls::v3::UpstreamTlsContext tls_context_;
std::unique_ptr<ClientContextConfigImpl> client_context_config_;
std::unique_ptr<ContextImpl> context_;
const std::string yaml = R"EOF(
common_tls_context:
tls_certificates:
certificate_chain:
filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/unittest_cert.pem"
private_key:
filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/unittest_key.pem"
)EOF";
};

TEST_F(SslContextStatsTest, IncOnlyKnownCounters) {
// Incrementing a value for a cipher that is part of the configuration works, and
// we'll be able to find the value in the stats store.
incCounter("ssl.ciphers", "ECDHE-ECDSA-AES256-GCM-SHA384");
Stats::CounterOptConstRef cipher =
store_.findCounterByString("ssl.ciphers.ECDHE-ECDSA-AES256-GCM-SHA384");
EXPECT_TRUE(cipher);
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

@ggreenway ggreenway Jan 5, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

@jmarantz jmarantz Jan 5, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

// test as it is dependent on timing and test-ordering.
incCounter("ssl.ciphers", "unexpected");
EXPECT_FALSE(store_.findCounterByString("ssl.ciphers.unexpected"));
}
Comment thread
jmarantz marked this conversation as resolved.

} // namespace Tls
} // namespace TransportSockets
} // namespace Extensions
Expand Down