Skip to content
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

Jump to bottom

ssl: remember stat names for configured ciphers. #14534

Merged
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 7 commits
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
50 changes: 31 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,36 @@ 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.
// 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());
Copy link
Contributor

Choose a reason for hiding this comment

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

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())) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

for (uint32_t i = 0, n = sk_SSL_CIPHER_num(ciphers); i < n; ++i) {
const SSL_CIPHER* cipher = sk_SSL_CIPHER_value(ciphers, i);
stat_name_set_->rememberBuiltin(SSL_CIPHER_get_name(cipher));
ENVOY_LOG_MISC(error, SSL_CIPHER_get_name(cipher));
jmarantz marked this conversation as resolved.
Show resolved Hide resolved
}
}

// 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");
stat_name_set_->rememberBuiltin("TLS_AES_128_GCM_SHA256");
// TLS_AES_128_GCM_SHA256 is referenced from
// IpVersionsClientVersions/SslCertficateIntegrationTest.ServerRsa/IPv4_TLSv1_3,
// possibly due to the call to
// ClientSslTransportOptions().setSigningAlgorithmsForTest
// in test/extensions/transport_sockets/tls/integration/ssl_integration_test.cc, function
// rsaOnlyClientOptions.
//
// We must also add two more that are hardcoded into the SSL 1.3 spec:
// https://tools.ietf.org/html/rfc8446
PiotrSikora marked this conversation as resolved.
Show resolved Hide resolved
stat_name_set_->rememberBuiltins(
{"TLS_AES_128_GCM_SHA256", "TLS_AES_256_GCM_SHA384", "TLS_AES_128_GCM_SHA256"});
ggreenway marked this conversation as resolved.
Show resolved Hide resolved

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

Copy link
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.

Copy link
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.

Copy link
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?

Copy link
Contributor

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.

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

Copy link
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.

Copy link
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 +649,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")
jmarantz marked this conversation as resolved.
Show resolved Hide resolved
<< std::flush;
#endif
}
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
61 changes: 61 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,67 @@ TEST_F(ServerContextConfigImplTest, PrivateKeyMethodLoadFailureBothKeyAndMethod)
"Certificate configuration can't have both private_key and private_key_provider");
}

// Subclass ContextImpl so we can instantiate directly from tests, despite the
// constructor being protected.
class TestContextImpl : public ContextImpl {
public:
TestContextImpl(Stats::Scope& scope, const Envoy::Ssl::ContextConfig& config,
TimeSource& time_source)
: ContextImpl(scope, config, time_source), pool_(scope.symbolTable()),
fallback_(pool_.add("fallback")) {}

void incCounter(absl::string_view name, absl::string_view value) {
ContextImpl::incCounter(pool_.add(name), value, fallback_);
}

Stats::StatNamePool pool_;
const Stats::StatName fallback_;
};

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_ = std::make_unique<TestContextImpl>(store_, *client_context_config_, time_system_);
}

Stats::TestUtil::TestStore store_;
envoy::extensions::transport_sockets::tls::v3::UpstreamTlsContext tls_context_;
std::unique_ptr<ClientContextConfigImpl> client_context_config_;
std::unique_ptr<TestContextImpl> 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.
context_->incCounter("ssl.ciphers", "ECDHE-ECDSA-AES256-GCM-SHA384");
Stats::CounterOptConstRef cipher =
store_.findCounterByString("ssl.ciphers.ECDHE-ECDSA-AES256-GCM-SHA384");
ASSERT_TRUE(cipher);
jmarantz marked this conversation as resolved.
Show resolved Hide resolved
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
Copy link
Contributor

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

Copy link
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.
context_->incCounter("ssl.ciphers", "unexpected");
EXPECT_FALSE(store_.findCounterByString("ssl.ciphers.unexpected"));

// We will account for the 'unexpected' cipher as "fallback".
cipher = store_.findCounterByString("ssl.ciphers.fallback");
ASSERT_TRUE(cipher);
EXPECT_EQ(1, cipher->get().value());
}
jmarantz marked this conversation as resolved.
Show resolved Hide resolved

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