Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

envoy: add a filter to store client cert info #4372

Merged
merged 2 commits into from Jul 19, 2023

Conversation

kenjenkins
Copy link
Contributor

Summary

Add a new Lua filter that will store client certificate info as dynamic metadata. This will allow us to configure client certificate validation at the Envoy listener level, and then pass the results of that validation into our ExtAuthz service.

This also allows us to pass the entire client certificate chain (and not just the leaf certificate, which is how the 'include_peer_certificate' ExtAuthz setting behaves). This will allow us to add support for intermediate CA certificates supplied by the client.

However, if a client certificate does not validate successfully by Envoy, we will not store the certificate chain. (This should help guard against any possibility of making policy decisions based on unvalidated client certificate data.)

Related issues

#4352

User Explanation

Checklist

  • reference any related issues
  • updated docs [n/a]
  • updated unit tests [n/a]
  • updated UPGRADING.md [n/a]
  • add appropriate tag (improvement / bug / etc)
  • ready for review

Add a new Lua filter that will store client certificate info as dynamic
metadata. This will allow us to configure client certificate validation
at the Envoy listener level, and then pass the results of that
validation into our ExtAuthz service.

This also allows us to pass the entire client certificate chain (and not
just the leaf certificate, which is how the 'include_peer_certificate'
ExtAuthz setting behaves). This will allow us to add support for
intermediate CA certificates supplied by the client.

However, if a client certificate does not validate successfully by
Envoy, we will not store the certificate chain. (This should help guard
against any possibility of making policy decisions based on unvalidated
client certificate data.)
@kenjenkins kenjenkins requested a review from a team as a code owner July 19, 2023 18:39
@coveralls
Copy link

coveralls commented Jul 19, 2023

Coverage Status

coverage: 63.363% (+0.05%) from 63.31% when pulling 8f0fe64 on kenjenkins/envoy-certificate-metadata into 1489d3a on main.

function envoy_on_request(request_handle)
local metadata = request_handle:streamInfo():dynamicMetadata()
local ssl = request_handle:streamInfo():downstreamSslConnection()
metadata:set("com.pomerium.client-certificate-info", "presented",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me know if you have any opinions on the metadata namespace to use for this data. The only guidance I've found from the Envoy documentation is this:

Key is the reverse DNS filter name, e.g. com.acme.widget. The envoy.* namespace is reserved for Envoy’s built-in filters.

(from https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/core/v3/base.proto#envoy-v3-api-field-config-core-v3-metadata-filter-metadata)

However, this metadata is for consumption by our ExtAuthz service, which can apparently ask for any metadata namespaces it likes, so I don't think this needs to correspond exactly to a filter name.

Again, happy to change this if you have an opinion about what it should be.

Copy link
Contributor

@calebdoxsey calebdoxsey left a comment

Choose a reason for hiding this comment

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

LGTM

@kenjenkins kenjenkins merged commit 8e4f728 into main Jul 19, 2023
10 checks passed
@kenjenkins kenjenkins deleted the kenjenkins/envoy-certificate-metadata branch July 19, 2023 20:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants