Skip to content

Support 'untrusted' client certificate routing#5207

Closed
jimini-lumox wants to merge 2 commits into
envoyproxy:masterfrom
jimini-lumox:support-untrusted-client-cert-routing
Closed

Support 'untrusted' client certificate routing#5207
jimini-lumox wants to merge 2 commits into
envoyproxy:masterfrom
jimini-lumox:support-untrusted-client-cert-routing

Conversation

@jimini-lumox

Copy link
Copy Markdown
Contributor

Description: Add the ability to pass through and route 'untrusted' clients.
Unit tests have not been added yet - want to get an early idea whether this PR would be rejected due to possible security concerns etc..
By default the client validation rules are unchanged.

Additional validation context options:

  • request_client_certificate : // If specified, Envoy will always request for clients to provide their certificate (without requiring it).
  • permit_untrusted_client_certificate : // If specified, Envoy will NOT reject connections using client certificates that have failed validation.
  • validation_permits_no_client_certificate : // If specified, Envoy will NOT require a client certificate if a validation context exists.
    'untrusted' (either not validated, or failed validation) client certificate details added to 'x-forwarded-untrusted-client-cert'

Risk Level: Medium
Testing: Manual so far - unit tests will be added
Docs Changes: API proto changes
Release Notes: N/A

@htuch

htuch commented Dec 4, 2018

Copy link
Copy Markdown
Member

@jimini-lumox what's the intended use case? Unvalidated TLS is not really any more secure than plain text..

@PiotrSikora

Copy link
Copy Markdown
Contributor

@htuch unverified TLS provides protection against passive observers, so it's "slightly" better than plain text :) But this PR is about unauthenticated clients in mutual TLS, and not about server certificates.

@jimini-lumox what exactly is your use case? request_client_certificate and validation_permits_no_client_certificate are already implemented by the means of trusted_ca and require_client_certificate. Currently, there is no way to accept client certificates that FAILED verification, but we have allow_expired_certificate, which might work for your use case.

@jimini-lumox

Copy link
Copy Markdown
Contributor Author

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

Example Listener Config
"tls_context": {
"require_client_certificate": false,
"common_tls_context": {
"tls_certificates": {
"certificate_chain": { "filename": "/certs/server.crt" },
"private_key": { "filename": "/certs/server.key" }
},
"validation_context": {
"request_client_certificate": true,
"permit_untrusted_client_certificate": true,
"verify_certificate_hash": [
"F9:17:93:CD:CD....."
]
}
}
And Example Listener Route Config
"routes": [
{
"match": { "prefix": "/", "headers": [
{ "name": "x-forwarded-untrusted-client-cert", "present_match": true },
{ "name": ":authority", "prefix_match": "intended-target-service" }
] },
"route": {
"cluster": "edge-access-control",
"prefix_rewrite": "/api/cert/untrusted"
}
},
{
"match": { "prefix": "/", "headers": [
{ "name": "x-forwarded-client-cert", "present_match": true },
{ "name": ":authority", "prefix_match": "intended-target-service" }
] },
"route": { "cluster": "intended-target-service", "use_websocket": true }
}
Does this help to understand our use case?

@stale

stale Bot commented Dec 11, 2018

Copy link
Copy Markdown

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 stale Bot added the stale stalebot believes this issue/PR has not been touched recently label Dec 11, 2018
@htuch htuch removed the stale stalebot believes this issue/PR has not been touched recently label Dec 13, 2018
@htuch

htuch commented Dec 13, 2018

Copy link
Copy Markdown
Member

Removed stale tag, this PR should be reviewed. @PiotrSikora WDYT? I'll give this another pass in the next day.

@htuch htuch left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The overall use case looks valid. Let's settle on the API/design before digging into the code.

I'm wondering if there are other approaches as well that might work. For example, what if we had a filter chain match fall-thru for when client certificates don't match, which could do the diversion. This would require us to make changes to the filter chain match criteria I think.

Also, there is quite a bit of similarity here between on-demand LDS or filter chain discovery and what you want to do. If we can hold a new connection in-flight, do the required access control lookup and then resume (and update the filter chain TLS context) then we would not need to recreate the connection after an xDS update.

WDYT? @PiotrSikora?


// If specified, Envoy will not reject expired certificates.
bool allow_expired_certificate = 8;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you update the description at

// verified. By default, a client certificate is optional, unless one of the additional
to describe how all these options interact? I share @PiotrSikora that we might have some redundancy here, in particular I think we could collapse the request/require client certificate fields with the right combination of options.

@PiotrSikora

Copy link
Copy Markdown
Contributor

Oops, sorry for the delay, this felt off my radar.

I think that we could simply add verify_client_certificate: true|check|false, where:

  • true would verify the client certificate and reject connection if the verification failed (what we have today),
  • check would verify the client certificate, but forward it and the verification status via x-forwarded-client-cert or another header (for easier matching), regardless of the verification status,
  • false would forward the client certificate to the backend via x-forwarded-client-cert without attempting to verify it (to avoid crypto operations, do access control at the backend, etc.).

I believe that this, combined with the existing trusted_ca and require_client_certificate would work for @jimini-lumox, wouldn't it?

Also, we already have the External Authorization filter, which should work even better, shouldn't it?

wrt filter chain matching, we do TLS after the filter chain and its TLS context are already selected, so doing verification, and then re-picking filter chain with a different TLS context in case of success/failure would be rather challenging and quite messy, IMHO.

@ggreenway

Copy link
Copy Markdown
Member

+1 to what @PiotrSikora said. That's what I was thinking for this as well.

@alyssawilk

Copy link
Copy Markdown
Contributor

Nice addition - probably worth including explicit release notes about it :-)

@stale

stale Bot commented Dec 29, 2018

Copy link
Copy Markdown

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 stale Bot added the stale stalebot believes this issue/PR has not been touched recently label Dec 29, 2018
@stale

stale Bot commented Jan 6, 2019

Copy link
Copy Markdown

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!

@PiotrSikora

Copy link
Copy Markdown
Contributor

Moved to #5501, so that we don't forget about this feature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stale stalebot believes this issue/PR has not been touched recently

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants