Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
5 changes: 5 additions & 0 deletions include/envoy/ssl/context_config.h
Original file line number Diff line number Diff line change
Expand Up @@ -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.

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.

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

*/
virtual const std::string& sigalgs() const PURE;

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.

Nit: signingAlgorithmsForTest().

};

typedef std::unique_ptr<ClientContextConfig> ClientContextConfigPtr;
Expand Down
5 changes: 3 additions & 2 deletions source/common/ssl/context_config_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -252,11 +252,12 @@ unsigned ContextConfigImpl::tlsVersionFromProto(
}

ClientContextConfigImpl::ClientContextConfigImpl(
const envoy::api::v2::auth::UpstreamTlsContext& config,
const envoy::api::v2::auth::UpstreamTlsContext& config, absl::string_view sigalgs,
Server::Configuration::TransportSocketFactoryContext& factory_context)
: ContextConfigImpl(config.common_tls_context(), factory_context),
server_name_indication_(config.sni()), allow_renegotiation_(config.allow_renegotiation()),
max_session_keys_(PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, max_session_keys, 1)) {
max_session_keys_(PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, max_session_keys, 1)),
sigalgs_(sigalgs) {
// BoringSSL treats this as a C string, so embedded NULL characters will not
// be handled correctly.
if (server_name_indication_.find('\0') != std::string::npos) {
Expand Down
16 changes: 11 additions & 5 deletions source/common/ssl/context_config_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -91,30 +91,36 @@ class ContextConfigImpl : public virtual Ssl::ContextConfig {

class ClientContextConfigImpl : public ContextConfigImpl, public ClientContextConfig {
public:
explicit ClientContextConfigImpl(
const envoy::api::v2::auth::UpstreamTlsContext& config,
ClientContextConfigImpl(
const envoy::api::v2::auth::UpstreamTlsContext& config, absl::string_view sigalgs,
Server::Configuration::TransportSocketFactoryContext& secret_provider_context);
explicit ClientContextConfigImpl(
ClientContextConfigImpl(
const envoy::api::v2::auth::UpstreamTlsContext& config,
Server::Configuration::TransportSocketFactoryContext& secret_provider_context)
: ClientContextConfigImpl(config, "", secret_provider_context) {}
ClientContextConfigImpl(
const Json::Object& config,
Server::Configuration::TransportSocketFactoryContext& secret_provider_context);

// Ssl::ClientContextConfig
const std::string& serverNameIndication() const override { return server_name_indication_; }
bool allowRenegotiation() const override { return allow_renegotiation_; }
size_t maxSessionKeys() const override { return max_session_keys_; }
const std::string& sigalgs() const override { return sigalgs_; }

private:
const std::string server_name_indication_;
const bool allow_renegotiation_;
const size_t max_session_keys_;
const std::string sigalgs_;
};

class ServerContextConfigImpl : public ContextConfigImpl, public ServerContextConfig {
public:
explicit ServerContextConfigImpl(
ServerContextConfigImpl(
const envoy::api::v2::auth::DownstreamTlsContext& config,
Server::Configuration::TransportSocketFactoryContext& secret_provider_context);
explicit ServerContextConfigImpl(
ServerContextConfigImpl(
const Json::Object& config,
Server::Configuration::TransportSocketFactoryContext& secret_provider_context);

Expand Down
141 changes: 137 additions & 4 deletions source/common/ssl/context_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,8 @@ namespace Envoy {
namespace Ssl {

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

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

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

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.

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

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.

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

See #5224

: scope_(scope), stats_(generateStats(scope)), time_source_(time_source) {
: scope_(scope), stats_(generateStats(scope)), time_source_(time_source),
tls_max_version_(config.maxProtocolVersion()) {
const auto tls_certificates = config.tlsCertificates();
tls_contexts_.resize(std::max(1UL, tls_certificates.size()));

Expand Down Expand Up @@ -527,6 +528,13 @@ ClientContextImpl::ClientContextImpl(Stats::Scope& scope, const ClientContextCon
}
}

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, "");

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.

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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

}
}

if (max_session_keys_ > 0) {
SSL_CTX_set_session_cache_mode(tls_contexts_[0].ssl_ctx_.get(), SSL_SESS_CACHE_CLIENT);
SSL_CTX_sess_set_new_cb(
Expand Down Expand Up @@ -812,11 +820,125 @@ int ServerContextImpl::sessionTicketProcess(SSL*, uint8_t* key_name, uint8_t* iv
}
}

bool ServerContextImpl::isClientEcdsaCapable(const SSL_CLIENT_HELLO* ssl_client_hello) {
CBS client_hello;
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)) {

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.

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

return false;
}

// This is the TLSv1.3 case (TLSv1.2 on the wire and the supported_versions extensions present).
// We just need to loook at signature algorithms.
if (client_version == TLS1_2_VERSION && tls_max_version_ == TLS1_3_VERSION) {
// If the supported_versions extension is found then we assume that the client is competent
// enough that just checking the signature_algorithms is sufficient.
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)) {

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.

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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

const uint8_t* signature_algorithms_data;
size_t signature_algorithms_len;
if (SSL_early_callback_ctx_extension_get(ssl_client_hello, TLSEXT_TYPE_signature_algorithms,
&signature_algorithms_data,
&signature_algorithms_len)) {
CBS signature_algorithms_ext, signature_algorithms;
CBS_init(&signature_algorithms_ext, signature_algorithms_data, signature_algorithms_len);
if (!CBS_get_u16_length_prefixed(&signature_algorithms_ext, &signature_algorithms) ||
CBS_len(&signature_algorithms_ext) != 0) {
return false;
}

while (CBS_len(&signature_algorithms) > 0) {

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.

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.

uint16_t sigalg;
if (!CBS_get_u16(&signature_algorithms, &sigalg)) {
return false;
}

if (sigalg == SSL_SIGN_ECDSA_SECP256R1_SHA256) {
return true;
}
}
}

return false;
}
}

// Otherwise we are < TLSv1.3 and need to look at both the curves in the supported_groups for
// ECDSA and also for a compatible cipher suite. https://tools.ietf.org/html/rfc4492#section-5.1.1
const uint8_t* curvelist_data;
size_t curvelist_len;
if (!SSL_early_callback_ctx_extension_get(ssl_client_hello, TLSEXT_TYPE_supported_groups,
&curvelist_data, &curvelist_len)) {
return false;
}

CBS curvelist;
CBS_init(&curvelist, curvelist_data, curvelist_len);

bool p256_ok = false;
while (CBS_len(&curvelist) > 0) {
uint16_t named_curve;
if (!CBS_get_u16(&curvelist, &named_curve)) {
return false;
}

if (named_curve == 23 /* secp256r1 */) {

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.

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

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.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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?

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.

Yeah.

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.

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.

p256_ok = true;
break;
}
}

if (!p256_ok) {
return false;
}

// The client must have offered an ECDSA ciphersuite that we like.
CBS cipher_suites;
CBS_init(&cipher_suites, ssl_client_hello->cipher_suites, ssl_client_hello->cipher_suites_len);

while (CBS_len(&cipher_suites) > 0) {
uint16_t cipher_id;
if (!CBS_get_u16(&cipher_suites, &cipher_id)) {
return false;
}

const SSL_CIPHER* c = SSL_get_cipher_by_value(cipher_id);

@PiotrSikora PiotrSikora Dec 5, 2018

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

if (c == nullptr) {
continue;
}

// Skip TLS 1.2 only ciphersuites unless the client supports it.
if (SSL_CIPHER_get_min_version(c) > client_version) {
continue;
}

if (SSL_CIPHER_get_auth_nid(c) == NID_auth_ecdsa) {
// All tls_context_ share the same set of enabled ciphers, so we can just look at the base
// context.
if (tls_contexts_[0].isCipherEnabled(c)) {
return true;
}
}
}

return false;
}

enum ssl_select_cert_result_t
ServerContextImpl::selectTlsContext(const SSL_CLIENT_HELLO* ssl_client_hello) {
// This is currently a nop, since we only have a single cert, but this is where we will implement
// the certificate selection logic in #1319.
RELEASE_ASSERT(SSL_set_SSL_CTX(ssl_client_hello->ssl, tls_contexts_[0].ssl_ctx_.get()) != nullptr,
const bool client_ecdsa_capable = isClientEcdsaCapable(ssl_client_hello);
// Fallback on first certificate.
const TlsContext* selected_ctx = &tls_contexts_[0];
for (const auto& ctx : tls_contexts_) {
if (client_ecdsa_capable == ctx.is_ecdsa_) {
selected_ctx = &ctx;
break;
}
}
RELEASE_ASSERT(SSL_set_SSL_CTX(ssl_client_hello->ssl, selected_ctx->ssl_ctx_.get()) != nullptr,
"");
return ssl_select_cert_success;
}
Expand Down Expand Up @@ -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++) {

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

const SSL_CIPHER* our_c = sk_SSL_CIPHER_value(our_ciphers, j);
if (SSL_CIPHER_get_id(our_c) == SSL_CIPHER_get_id(cipher)) {
return true;
}
}
return false;
}

} // namespace Ssl
} // namespace Envoy
3 changes: 3 additions & 0 deletions source/common/ssl/context_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,7 @@ class ContextImpl : public virtual Context {
std::string getCertChainFileName() const { return cert_chain_file_path_; };
void addClientValidationContext(const CertificateValidationContextConfig& config,
bool require_client_cert);
bool isCipherEnabled(const SSL_CIPHER* cipher);
};

// This is always non-empty, with the first context used for all new SSL
Expand All @@ -154,6 +155,7 @@ class ContextImpl : public virtual Context {
std::string ca_file_path_;
std::string cert_chain_file_path_;
TimeSource& time_source_;
const unsigned tls_max_version_;
};

typedef std::shared_ptr<ContextImpl> ContextImplSharedPtr;
Expand Down Expand Up @@ -186,6 +188,7 @@ class ServerContextImpl : public ContextImpl, public ServerContext {
unsigned int inlen);
int sessionTicketProcess(SSL* ssl, uint8_t* key_name, uint8_t* iv, EVP_CIPHER_CTX* ctx,
HMAC_CTX* hmac_ctx, int encrypt);
bool isClientEcdsaCapable(const SSL_CLIENT_HELLO* ssl_client_hello);
// Select the TLS certificate context in SSL_CTX_set_select_certificate_cb() callback with
// ClientHello details.
enum ssl_select_cert_result_t selectTlsContext(const SSL_CLIENT_HELLO* ssl_client_hello);
Expand Down
5 changes: 5 additions & 0 deletions test/config/utility.cc
Original file line number Diff line number Diff line change
Expand Up @@ -400,6 +400,11 @@ void ConfigHelper::initializeTls(bool ecdsa_cert,
TestEnvironment::runfilesPath("test/config/integration/certs/cacert.pem"));
validation_context->add_verify_certificate_hash(TEST_CLIENT_CERT_HASH);

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

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'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?


auto* tls_certificate = common_tls_context.add_tls_certificates();
if (ecdsa_cert) {
tls_certificate->mutable_certificate_chain()->set_filename(
Expand Down
Loading