Feature untrusted client certs

[jimini-lumox]

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

This is a resurrection of #6760

The original PR covered two requirements:
Allowing certs that fail validation.
Request peer certs when there is nothing to validate against.

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;
// If ACCEPT_UNTRUSTED or NOT_VERIFIED selected, when a client certificate fails (or is not validated) an x-forwarded-untrusted-client-cert HTTP header hoisted.

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

[zuercher]

@jimini-lumox Thanks for taking this up. In addition to the test failures and formatting, you'll need to rewrite those commits (including the merge) with DCO sign-offs (see the DCO section in CONTRIBUTING.md).

[zuercher]

/wait

[repokitteh-read-only]

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

🐱

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

see: more, trace.

[lizan]

Do we really want a new header field for untrusted client certificates? Can we just add Trusted=False in XFCC header to indicate that?

[jimini-lumox]

We had initially implemented this way, however had concerns over the potential for existing code to treat ForwardedClientCert as having passed validation.
An example of the routing we perform is as follows: - route to the intended service, iff have x-forwarded-client-cert header. (I guess a 'safe-regex_match' could alternatively be used to route based on 'Trusted=False/True'

"routes": [
{
"match": { "prefix": "/", "headers": [
{ "name": "x-forwarded-untrusted-client-cert", "present_match": true }
] },
"route": {
"cluster": "venue-edge-access-control",
"prefix_rewrite": "/api/cert/untrusted",
"host_rewrite": "venue-edge-access-control"
}
},
{
"match": { "prefix": "/", "headers": [
{ "name": "x-forwarded-client-cert", "present_match": true, "invert_match" : true }
] },
"route": {
"cluster": "venue-edge-access-control",
"prefix_rewrite": "/api/cert/untrusted",
"host_rewrite": "venue-edge-access-control"
}
},
{
"match": { "prefix": "/", "headers": [
{ "name": "x-forwarded-client-cert", "present_match": true },
{ "name": ":authority", "prefix_match": "service-actual-target-service" }
] },
"route": {
"cluster": "service-actual-target-service"
}
},
....

[htuch]

@PiotrSikora thoughts on these headers and what other proxies might do.

[lizan]

@jimini-lumox I see what your example try to address. Does it make more sense to have a route matcher based on presented client cert semantically rather than relying on XFCC/XFUCC header? Adding header and route matching can be orthogonal.

[jimini-lumox]

I'm not sure I understand exactly, or how to achieve it using the route matcher. Below is a comment on our usage from the first cut of the PR (#5207). Do you know how we would use route matching to handle the scenarios (maybe it's easy, I'm not sure)

We use Envoy as an Edge Proxy to host services, with thousands of devices in venues on a private network.
We're using the Hash List as validation of clients, which will initially be empty ie - nothing to validate against.
Client Certs are currently self-signed (no PKI infrastructure available) - so there is nothing within the certificate to validate against.
If a client connects and its Cert Hash is in the list, it will be routed to its intended target service.
(eg: x-forwarded-client-cert header present & :authority header match)
If a client connects and its Cert Hash is not in the list (or there is no validation context yet), then it may be routed to an access-control-service.
(eg: x-forwarded-untrusted-client-cert header present & :authority header match)
Then the access-control-service controls whether a client is to be permitted, in which case the client hash will be added to the verify_certificate_hash list (using dynamic_resources from the Edge Proxy, so subsequent request is routed to the intended service).
Additionally we may not care if a client needs to be validated to reach certain other host services.

[jimini-lumox]

RouteMatch only uses http headers, so if I'm following you correctly:

• always hoist headers into x-forwarded-client-cert (instead of using x-forwarded-untrusted-client-cert for either failed or unvalidated)
• and,
  either add a field to the x-forwarded-client-cert to indicated that it passed/failed validation (eg Trusted=false)
  or add another http header that simply indicates whether the client was validated, so we can route based on this.
• and then add a new RouteMatch field that can simplify the route matching rules for validation, eg something like:

  message RouteMatch {
    message CredentialMatchOptions {
        google.protobuf.BoolValue validated = 1;
        // can add further matches here - eg Issuer,Hash etc.
    }
    ...
    CredentialMatchOptions credentials = 11;
  }

then the matching would simplify to:

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

[lizan]

The config example looks right, I think we should open another issue for this API proposal.

RouteMatch only uses http headers

This is how current implementation does but not a hard limitation. You can use the cert information directly from downstream connection and plumb it into RouteMatcher.

[jimini-lumox]

Ok, so should I create a new PR that has this API proposal including the changes to route the required connection info through to the Route matching.
I was thinking that we could provide the StreamInfo through the route matching from which we can get the downstreamSslConnection info etc.

[lizan]

I was thinking that we could provide the StreamInfo through the route matching from which we can get the downstreamSslConnection info etc.

Exactly.

[jimini-lumox]

I've raised PR #8453 with an initial cut of passing the connection stream_info through to the route matching.

[htuch]

@PiotrSikora thoughts on these headers and what other proxies might do.

[stale]

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!

[jimini-lumox]

PR #8453 still in progress which will impact the implementation of this PR
PR #8453 now accepted, so will merge into this PR, updating configuration, tests etc. WIP

[stale]

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]

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]

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!

[jimini-lumox]

I think this is ready for a review with PR #8453 merged and appropriate unit tests.
@lizan How do I re-open this PR? Thanks.