Feature untrusted client certs #9172

jimini-lumox · 2019-11-29T05:59:58Z

Description: Add the ability to pass through and route 'untrusted' certificates.

This is a resurrection of PR #8248 with latest master merged, & changes to ssl-context routing merged, as requested in #8248.

The original PR covered two requirements:
Allowing certs that fail validation.
Request peer certs when there is nothing to validate against.
Support routing based on client credential validation status.

By default the client validation rules are unchanged.

Additional validation context options:
// Specify the certificate validation to be performed: VERIFY_TRUST_CHAIN(default), ACCEPT_UNTRUSTED, NOT_VERIFIED.
TrustChainVerification verify_certificate_trust_chain;

Added Routing support based on whether the client certificate was validated or not.

Risk Level: Medium
Testing: Manual and unit tests
Docs Changes: API proto changes
Release Notes: N/A

repokitteh-read-only · 2019-11-29T06:00:04Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #9172 was opened by jimini-lumox.

see: more, trace.

htuch · 2019-12-02T19:39:21Z

api/envoy/api/v2/route/route.proto

@@ -349,6 +349,9 @@ message RouteMatch {
  message TlsContextMatchOptions {
    // If specified, the route will match against whether or not a certificate is presented.
    google.protobuf.BoolValue presented = 1;
+
+    // If specified, the route will match against whether or not a certificate is validated.
+    google.protobuf.BoolValue validated = 2;


Ideally we make this an enum (and deprecate the old way of doing this).

These fields are used for route matching, and are effectively a tribool - true/false/not-defined
eg: for 'validated', by default we don't route based on validation status, unless specifically set to true / false, example snippet from previous PR #8248 :

"routes": [ { "match": { "prefix": "/", "credentials": { "validated": false } }, "route": { "cluster": "venue-edge-access-control", ... } }, { "match": { "prefix": "/", "credentials": { "validated": true }, "headers": [ { "name": ":authority", "prefix_match": "service-actual-target-service" } ] }, "route": { "cluster": "service-actual-target-service" } } ...

Ack. Is there a reason they are BoolValue other than history? What is the default value? If it's false, I suggest leaving a [#next-major-version: change type to bool] for both of these fields.

The default is that it is not specified so will not be routed against. It is only routed against if specified in the configuration. This is why we've used a BoolValue (like a tribool). Is there another recommended way of defining this?

api/envoy/api/v2/auth/cert.proto

lizan · 2019-12-04T21:34:01Z

source/extensions/transport_sockets/tls/ssl_socket.cc

@@ -301,11 +302,20 @@ void SslSocket::shutdownSsl() {
  }
 }

+SslSocketInfo::SslSocketInfo(bssl::UniquePtr<SSL> ssl, ContextImplSharedPtr ctx)
+    : ssl_(std::move(ssl)) {
+  SSL_set_ex_data(ssl_.get(), ctx->sslCustomDataIndex(), &(this->certificate_validation_status_));


Not thrilled to have more ex_data exposed into SSL library later for this. Can we consolidate this with existing SSL_set_app_data by defining a new interface for that purpose?

Currently the set_app_data is on the ssl context for ContextImpl, and there exists a set_app_data on the SSL socket in the tls_inspector, so we weren't going to change that.
Because we're using this to be able to provide extra data related to the SSL connection, we've created an SslExtendedSocketInfo class that can be used/extended to pass this information through.
Let me know what you think of this approach (included in the latest commits).

@lizan can you please give this change a re-review when you get a chance, thanks.

Currently the set_app_data is on the ssl context for ContextImpl, and there exists a set_app_data on the SSL socket in the tls_inspector, so we weren't going to change that.
Because we're using this to be able to provide extra data related to the SSL connection, we've created an SslExtendedSocketInfo class that can be used/extended to pass this information through.
Let me know what you think of this approach (included in the latest commits).

The SSL_CTX_set_app_data set ContextImpl which is not in the same scope, and TLS inspector SSL object is not leaked into here.

The only existing SSL_set_app_data is also to provide extra data related to SSL connection for transport socket options here, I meant to refactor these two into one interface and do SSL_set_app_data once.

Could we do this refactor as a subsequent PR - I'm happy to pick this up.
because this may be a bit of a tricky refactor to combine the multiple pieces of info 'cleanly'.
we've got the ContextImpl's const Network::TransportSocketOptions* options being set via SSL_set_app_data by the ContextImpl but only if (options && !options->verifySubjectAltNameListOverride().empty()) when the SslSocket is created,
and then the SslSocketInfo::SslExtendedSocketInfo member being set via SSL_set_ex_data when the SslSocket is subsequently moved to be owned by the constructed SslSocketInfo.

@lizan Can we re-open this PR please - or do we need to create another PR

Could we do this refactor as a subsequent PR - I'm happy to pick this up.
because this may be a bit of a tricky refactor to combine the multiple pieces of info 'cleanly'.
we've got the ContextImpl's const Network::TransportSocketOptions* options being set via SSL_set_app_data by the ContextImpl but only if (options && !options->verifySubjectAltNameListOverride().empty()) when the SslSocket is created,
and then the SslSocketInfo::SslExtendedSocketInfo member being set via SSL_set_ex_data when the SslSocket is subsequently moved to be owned by the constructed SslSocketInfo.

Since this PR is relatively large, so sure, can you file an issue for this and assign yourself?

Thanks, I've created issue: #9867 , but don't appear to have the power to assign myself - can you assign it to me please.

stale · 2019-12-11T22:15:40Z

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!

da77a · 2019-12-12T06:36:45Z

not stale @jimini-lumox is just offline for a little while. pinging to keep this fresh...

repokitteh-read-only · 2019-12-13T06:47:23Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #9172 was synchronize by jimini-lumox.

see: more, trace.

stale · 2019-12-25T21:00:41Z

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 · 2020-01-17T01:05:13Z

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 · 2020-01-24T02:40:37Z

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!

lizan

Can you merge master again to fix merge conflicts? Otherwise LGTM once you filed the refactor issue.

jimini-lumox · 2020-01-29T06:15:31Z

Thanks, master merged & built.
Issue: #9867 created for the refactor

lizan

Can you also add a line to version_history.rst?

lizan · 2020-01-29T06:18:00Z

include/envoy/router/router.h

@@ -535,6 +535,7 @@ class TlsContextMatchCriteria {
  virtual ~TlsContextMatchCriteria() = default;

  virtual const absl::optional<bool>& presented() const PURE;
+  virtual const absl::optional<bool>& validated() const PURE;


can you add comment to this class and methods?

lizan · 2020-01-29T09:25:08Z

docs/root/intro/version_history.rst

@@ -6,6 +6,8 @@ Version history
 * config: use type URL to select an extension whenever the config type URL (or its previous versions) uniquely identify a typed extension, see :ref:`extension configuration <config_overview_extension_configuration>`.
 * retry: added a retry predicate that :ref:`rejects hosts based on metadata. <envoy_api_field_route.RetryPolicy.retry_host_predicate>`
 * upstream: changed load distribution algorithm when all priorities enter :ref:`panic mode<arch_overview_load_balancing_panic_threshold>`. 
+* router: added the ability to match a route based on whether a downstream TLS connection certificate has been


nit: alphabetical order

lizan

Can you fix DCO? Feel free to squash all commit into one and force-push if that's needed.

lizan · 2020-01-30T07:59:39Z

Needs another merge for version_history, thanks!

mattklein123

1 quick API/docs question. Thank you!

/wait

mattklein123 · 2020-01-31T00:42:20Z

api/envoy/api/v2/route/route_components.proto

@@ -353,6 +353,9 @@ message RouteMatch {
  message TlsContextMatchOptions {
    // If specified, the route will match against whether or not a certificate is presented.
    google.protobuf.BoolValue presented = 1;
+
+    // If specified, the route will match against whether or not a certificate is validated.


What is the default here? Can you clarify?

If not specified, then the validated status (true or false) will not be considered for route matching - it's the same behaviour as the 'presented' field above.

OK can you do me a favor and clarify the docs both here and above that no value means not considered at all, a set value checks that state, etc. Feel free to phrase however you want but it was a bit unclear to me from a quick doc read. Thank you!

I've updated it, let me know if it's ok. Thanks.

jimini-lumox · 2020-01-31T05:35:52Z

I seem to have broken a bunch of things when trying to fix the DCO error - and don't think I can recover it in my branch.
Should I create a new PR, with the changes squashed?

lizan · 2020-01-31T05:46:37Z

@jimini-lumox if you can squash all into one commit and force push it to same branch it will fix. You don't need another PR but just a force push.

Signed-off-by: Michael Hargreaves <mik.hargreaves@gmail.com>

jimini-lumox · 2020-02-03T03:10:30Z

@lizan I've fixed the DCO issues with a squashed forced push. Thanks

lizan · 2020-02-03T07:11:04Z

Defer merge to @mattklein123 for doc check.

mattklein123

Thanks!

jimini-lumox requested a review from lizan as a code owner November 29, 2019 05:59

repokitteh-read-only bot added the api label Nov 29, 2019

alyssawilk assigned PiotrSikora and lizan Dec 2, 2019

htuch reviewed Dec 2, 2019

View reviewed changes

lizan reviewed Dec 4, 2019

View reviewed changes

lizan added the waiting label Dec 4, 2019

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

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Dec 12, 2019

repokitteh-read-only bot removed the waiting label Dec 13, 2019

stale bot added stale stalebot believes this issue/PR has not been touched recently and removed stale stalebot believes this issue/PR has not been touched recently labels Dec 25, 2019

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Jan 17, 2020

stale bot closed this Jan 24, 2020

mattklein123 reopened this Jan 24, 2020

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Jan 24, 2020

lizan reviewed Jan 29, 2020

View reviewed changes

jimini-lumox mentioned this pull request Jan 29, 2020

SSL Connection: combine SSL_set_ex_data and SSL_set_app_data #9867

Open

lizan reviewed Jan 29, 2020

View reviewed changes

jimini-lumox force-pushed the feature-untrusted-client-certs branch from 93632da to a0b1c3a Compare January 29, 2020 19:50

lizan reviewed Jan 29, 2020

View reviewed changes

jimini-lumox requested a review from snowp as a code owner January 29, 2020 20:07

lizan previously approved these changes Jan 30, 2020

View reviewed changes

repokitteh-read-only bot removed the api label Jan 30, 2020

jimini-lumox dismissed lizan’s stale review via b5fc9d0 January 30, 2020 19:30

repokitteh-read-only bot added the api label Jan 30, 2020

lizan previously approved these changes Jan 30, 2020

View reviewed changes

repokitteh-read-only bot removed the api label Jan 30, 2020

mattklein123 requested changes Jan 31, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Jan 31, 2020

jimini-lumox dismissed lizan’s stale review via 4e5b30f January 31, 2020 02:07

repokitteh-read-only bot added api and removed waiting labels Jan 31, 2020

jimini-lumox force-pushed the feature-untrusted-client-certs branch from 4e5b30f to 378f043 Compare January 31, 2020 04:55

jimini-lumox requested review from jmarantz and zuercher as code owners January 31, 2020 04:55

Add support to pass through and route untrusted certificates

928bd25

Signed-off-by: Michael Hargreaves <mik.hargreaves@gmail.com>

jimini-lumox force-pushed the feature-untrusted-client-certs branch from 378f043 to 928bd25 Compare January 31, 2020 06:00

lizan approved these changes Feb 3, 2020

View reviewed changes

repokitteh-read-only bot removed the api label Feb 3, 2020

mattklein123 self-assigned this Feb 3, 2020

mattklein123 approved these changes Feb 3, 2020

View reviewed changes

mattklein123 merged commit abb1f96 into envoyproxy:master Feb 3, 2020

marcosrmendezthd mentioned this pull request Feb 6, 2020

tls: request client certificate #9914

Closed

Eneminemoe mentioned this pull request Apr 1, 2020

Incoming mTLS traffic termination with non-trusted CA signed certificates istio/istio#22469

Closed

Feature untrusted client certs #9172

Feature untrusted client certs #9172

Conversation

jimini-lumox commented Nov 29, 2019

repokitteh-read-only bot commented Nov 29, 2019

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

jimini-lumox Dec 16, 2019 • 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

stale bot commented Dec 11, 2019

da77a commented Dec 12, 2019

repokitteh-read-only bot commented Dec 13, 2019

stale bot commented Dec 25, 2019

stale bot commented Jan 17, 2020

stale bot commented Jan 24, 2020

lizan left a comment

Choose a reason for hiding this comment

jimini-lumox commented Jan 29, 2020

lizan left a 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

lizan left a comment

Choose a reason for hiding this comment

lizan commented Jan 30, 2020

mattklein123 left a 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

jimini-lumox commented Jan 31, 2020

lizan commented Jan 31, 2020

jimini-lumox commented Feb 3, 2020

lizan commented Feb 3, 2020

mattklein123 left a comment

Choose a reason for hiding this comment

jimini-lumox Dec 16, 2019 •

edited

Loading