tls: add local certificate presented flag for mTLS detection by kyessenov · Pull Request #8464 · envoyproxy/envoy

kyessenov · 2019-10-02T18:50:32Z

Signed-off-by: Kuat Yessenov kuat@google.com

Description: add a boolean flag to report whether a local cert is presented. This is useful for upstream mTLS detection.
Risk Level: low
Testing: unit tests updated
Docs Changes: none
Release Notes: none

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov · 2019-10-02T18:50:54Z

cc @lizan @nrjpoddar in ref to istio/proxy#2434

Signed-off-by: Kuat Yessenov <kuat@google.com>

lizan · 2019-10-02T18:55:08Z

source/extensions/transport_sockets/tls/ssl_socket.cc

 }

+bool SslSocketInfo::localCertificatePresented() const {
+  bssl::UniquePtr<X509> cert(SSL_get_certificate(ssl_.get()));


IIRC SSL_get_certificate doesn't increment the refcount of X509 so this shouldn't be UniquePtr, ASAN will catch if that's the case likely.

Signed-off-by: Kuat Yessenov <kuat@google.com>

nrjpoddar · 2019-10-02T19:13:27Z

source/extensions/transport_sockets/tls/ssl_socket.cc

 }

+bool SslSocketInfo::localCertificatePresented() const {
+  X509* cert = SSL_get_certificate(ssl_.get());


IIUC this just verifies existence of a client cert and not an acknowledgement of it's verification by the server?

I don't think there's a way to know whether server verified, you know whether the handshake is successful or not, that doesn't indicate server verified though. I might be wrong, cc @PiotrSikora

@nrjpoddar is correct, the code in this PR (as of right now) only verifies that the TLS client certificate is configured, and not that the established connection is mutually authenticated.

We could potentially install SSL_CTX_set_cert_cb and call SSL_get_client_CA_list to check if peer requested local certificate, but I'm not aware of any API that allows us to check this directly.

That actually means that this is not a new problem. When we report mTLS using peerCertificatePresented on the server, that does not mean that the certificate was validated by the server. So at least in istio context, we should make it clear that mTLS attributes are necessary but not sufficient for true mTLS. For telemetry purposes, this is fine, but for policy, it's not safe to rely on peerCertificatePresented or localCertificatePresented. It's entirely possible that both parties present certs but do no validation.

Anyways, I think this PR is still useful for upstream mTLS bit.

Totally agree, minor nit the name for this API should be localCertificateConfigured for correctness.

It has been great learning just watching the thread. We are mixing few different things here, that I hope to clarify:

First, how can I tell a particular connection is mTLS reliably from a perspective of one peer i.e. client or server. I don't think this is always possible but can be closely approximated by this logic.

client reports mTLS true if it verified server certificate, presented local certificate & handshake was successful

server reports mTLS true if it presented local certificate, verified client certificate & handshake was successful

If both side reports mTLS false it is definitely plain text

If one side reports false and other true, we cannot definitely say if the connection in mTLS, TLS or plaintext

If both sides report mTLS true, we can for 99% of cases say that the connection was mTLS.

The second issue we are trying to solve here is get enough information via Envoy to figure out a config distribution problem i.e. control plane says use mTLS but the client/server never presents a certificate. In this scenario exposing the individual bits of information on a peer like requested peer certificate, presented local certificate & verified peer certificate helps.

The SSL session resumption afaik should just carry forward the mTLS boolean and other information bits that is stored the first time the handshake was finished.

Neeraj, agree with the mTLS detection assessment.

@davidben I suggested to install callback using SSL_CTX_set_cert_cb AND use SSL_get_client_CA_list inside the callback to detect if server requested client certificate that matches issuer of the configured client certificate (although, we don't do that currently, so it might be an overkill for this PR), i.e. it was a single suggestion, not two. Sorry if that wasn't clear enough.

Regarding session resumption, you are, of course, right (and thank you for the reminder, I keep forgetting about this more often that I should have)...

For clients, we could store this bit (and possibly others) when adding session keys to the client-side cache in Envoy, and then report value used to establish the original connection.

For servers, we rely on BoringSSL for caching, and we don't store anything in Envoy, so that would be tricky... but we include all configuration related to the client certificate verification in SSL_CTX_set_session_id_context, so sessions cannot be resumed across different configurations, which means that we can infer mTLS bit from the successful handshake and the server-side configuration alone.

What would be the benefit of checking SSL_get_client_CA_list? BoringSSL sends the client certificate, if configured + requested, independent of SSL_get_client_CA_list. The CA list is purely a hint. On mismatch, likely the handshake will fail (in which case you don't care). If it succeeds anyway, then the server accepted your client certificate. (Or maybe it silently ignored it, but it's always possible for the server to silently ignore it, which gets to the question of what you're even measuring.)

For servers, we rely on BoringSSL for caching, and we don't store anything in Envoy, so that would be tricky

This is easy for servers. Just check if SSL_get_peer_certificate returns anything. We store the peer certificate in the session because that's a critical part of authentication. We don't store the local certificate because that's a waste of memory and ticket size. But, as a server, you always send a local certificate. It's the client certificate that's optional.

Let's not check for SSL_get_client_CA_list. That does not handle hash verification on the server-side. I think checking that a client certificate was requested is sufficient for now.

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov · 2019-10-04T18:16:09Z

@PiotrSikora It looks like envoy only support server session resumption. That means the bit is still correct since we always set it to true on the server side.

PiotrSikora · 2019-10-05T21:46:13Z

source/extensions/transport_sockets/tls/ssl_socket.cc

+        ssl_.get(),
+        [](SSL*, void* arg) -> int {
+          auto info = static_cast<SslSocketInfo*>(arg);
+          info->local_cert_presented = true;


I don't think this is sufficient, since it's not verifying that the client certificate is configured, and I think that this callback will be called even if it isn't (I might be wrong, so it's worth double-checking that).

This should work better:
info->local_cert_presented = (SSL_get_certificate(ssl) != nullptr);

Ideally, local_cert_presented should be set to true only if the issuer of the client certificate matches one of the CAs that server requested a certificate for (using SSL_get_client_CA_list and SSL_get0_certificate_types), but we're not checking for that before sending the certificate, and server is going to close the connection if they don't match anyway, so it might be an overkill if we want to use this only for telemetry.

We assume that handshake completes successfully. That means all those checks (there is a client cert, and CA trust chain, and other things) are already done. Why do we need another set of checks that duplicate the handshake?

PiotrSikora · 2019-10-05T21:49:39Z

source/extensions/transport_sockets/tls/ssl_socket.cc


+SslSocketInfo::SslSocketInfo(bssl::UniquePtr<SSL> ssl, InitialState state) : ssl_(std::move(ssl)) {
+  if (state == InitialState::Client) {
+    SSL_set_cert_cb(


It would be more efficient to configure this once in ClientContextImpl using SSL_CTX_set_cert_cb instead of here, when it's called for each connection.

Also, installing it in ClientContextImpl means that there is access to the configuration values, so it might make sense to install this callback only when the client certificate is configured.

I'm not sure how to pass SSL info via context. Can you be more specific about your suggestion?

PiotrSikora · 2019-10-05T22:32:18Z

source/extensions/transport_sockets/tls/ssl_socket.cc

 }

+bool SslSocketInfo::localCertificatePresented() const {
+  X509* cert = SSL_get_certificate(ssl_.get());


@davidben I suggested to install callback using SSL_CTX_set_cert_cb AND use SSL_get_client_CA_list inside the callback to detect if server requested client certificate that matches issuer of the configured client certificate (although, we don't do that currently, so it might be an overkill for this PR), i.e. it was a single suggestion, not two. Sorry if that wasn't clear enough.

Regarding session resumption, you are, of course, right (and thank you for the reminder, I keep forgetting about this more often that I should have)...

For clients, we could store this bit (and possibly others) when adding session keys to the client-side cache in Envoy, and then report value used to establish the original connection.

For servers, we rely on BoringSSL for caching, and we don't store anything in Envoy, so that would be tricky... but we include all configuration related to the client certificate verification in SSL_CTX_set_session_id_context, so sessions cannot be resumed across different configurations, which means that we can infer mTLS bit from the successful handshake and the server-side configuration alone.

PiotrSikora

@PiotrSikora It looks like envoy only support server session resumption. That means the bit is still correct since we always set it to true on the server side.

Envoy supports both: server- and client-side session resumption (see: #4791).

stale · 2019-10-15T21:32:58Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

stale · 2019-10-22T22:09:22Z

This pull request has been automatically closed because it has not had activity in the last 14 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

Description: update CEL runtime to tighten the complexity bounds: - remove comprehension operators (forall, exists) - remove list and string concat to avoid memory allocation - limit RE2 regex max program size - remove string conversion to avoid string allocation Risk Level: low Testing: unit tests Docs Changes: remove upstream.mtls attribute due to ongoing #8464 Release Notes: Signed-off-by: Kuat Yessenov <kuat@google.com>

tls: add local certificate presented flag for mTLS detection

7500c1e

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov requested a review from lizan as a code owner October 2, 2019 18:50

typo fix

a36b321

Signed-off-by: Kuat Yessenov <kuat@google.com>

lizan reviewed Oct 2, 2019

View reviewed changes

fix ASAN

193c6c1

Signed-off-by: Kuat Yessenov <kuat@google.com>

lizan mentioned this pull request Oct 2, 2019

add upstream info istio/proxy#2434

Closed

nrjpoddar reviewed Oct 2, 2019

View reviewed changes

lizan assigned PiotrSikora Oct 4, 2019

kyessenov added 2 commits October 3, 2019 22:37

update to use cert callbacks

25a1057

Signed-off-by: Kuat Yessenov <kuat@google.com>

update tests

51faa62

Signed-off-by: Kuat Yessenov <kuat@google.com>

PiotrSikora reviewed Oct 5, 2019

View reviewed changes

PiotrSikora suggested changes Oct 5, 2019

View reviewed changes

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Oct 15, 2019

stale bot closed this Oct 22, 2019

kyessenov mentioned this pull request Oct 24, 2019

update cel-cpp #8746

Merged

nrjpoddar mentioned this pull request Apr 21, 2021

Update workload outbound dashboard to distinguish mTLS or not. istio/istio#32347

Merged

Conversation

kyessenov commented Oct 2, 2019

Uh oh!

kyessenov commented Oct 2, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nrjpoddar Oct 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kyessenov commented Oct 4, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PiotrSikora left a comment

Choose a reason for hiding this comment

Uh oh!

stale bot commented Oct 15, 2019

Uh oh!

stale bot commented Oct 22, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

nrjpoddar Oct 4, 2019 •

edited

Loading