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

tls: implement SPIFFE Certificate Validator for independent multiple trust domain support #14884

Merged
merged 46 commits into from
Mar 3, 2021

Conversation

mathetake
Copy link
Member

@mathetake mathetake commented Jan 31, 2021

Signed-off-by: Takeshi Yoneda takeshi@tetrate.io

This is the sequel to the previous refactoring PR: #14757

Commit Message: tls: implement SPIFFE Certificate Validator for independent multiple trust domain support.
Additional Description: Adds the extension point for certificate validations, and its first implementation for SPIFFE multi trust domain support in a single listener or cluster. Resolves #14614 and #9284.
Risk Level: low (only adding the new extension point and one implementation for it)
Testing: unit tests and integration tests.
Docs Changes:
Release Notes: tls: implement SPIFFE Certificate Validator for independent multiple trust domain support.

cc @lizan

Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
@repokitteh-read-only
Copy link

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
API shepherd assignee is @mattklein123
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #14884 was opened by mathetake.

see: more, trace.

Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
@lizan lizan added the waiting label Feb 2, 2021
Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
@mathetake mathetake changed the title tls: implement SPIFFE Certificate Validator for multiple trust domain support tls: implement SPIFFE Certificate Validator for independent multiple trust domain support Feb 3, 2021
Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
@@ -374,4 +375,11 @@ message CertificateValidationContext {
// Certificate trust chain verification mode.
TrustChainVerification trust_chain_verification = 10
[(validate.rules).enum = {defined_only: true}];

// The configuration of an extension specific certificate validator.
// If specified, all the values above are *ignored*.
Copy link

Choose a reason for hiding this comment

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

Sorry to wander in here, I have a quick question. Does this comment imply that I can't use match_subject_alt_names at the same time as custom_validator_config? Will the SPIFFE validator extension support name matching?

Wondering the same about require_signed_certificate_timestamp and allow_expired_certificate, both of which also seem like they'd still be desirable when using SPIFFE validation

Copy link

Choose a reason for hiding this comment

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

Related question: will a combined validation context work as expected if a custom validator config is set in one of them? i.e. can I set the name matching in a default context and then additionally configure the SPIFFE validator?

Copy link
Member Author

@mathetake mathetake Feb 4, 2021

Choose a reason for hiding this comment

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

Hi @evan2645, thanks for dropping by!

In the current implementation, Envoy does not use any of existing fields in CertificateValidationContext.* when CertificateValidationContext.custom_validator_config is set. So setting match_subject_alt_names, require_signed_certificate_timestamp and allow_expired_certificate would have no effect at all.

As for require_signed_certificate_timestamp and allow_expired_certificate, it's easy to implement, but I would like to limit the scope of this initial PR. So I would love to work on adding support for these fields in other PRs. (So I would change this description here to, like: If specified, the usage of all the values above depends on the custom validator.)

For match_subject_alt_names, I would like to ask you about what the expected behavior looks like? When the name match, then how should we choose the trust domain? I got it. Choose the trust bundle based on the given trust domain first, and then verify the SAN name matching, and so maybe you want to have SAN matching conditions per trust bundle, right?

Copy link

Choose a reason for hiding this comment

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

Hi @evan2645, thanks for dropping by!

Thanks for the warm welcome :)

it's easy to implement, but I would like to limit the scope of this initial PR.

Oh sure! Sorry, I didn't mean to imply that these things must be addressed right now. I just wanted to call attention to it, and make sure that it is at least being considered in the grand scheme of things. I'd probably call support for require_signed_certificate_timestamp and allow_expired_certificate as nice to have. The feature provided by match_subject_alt_names configurable is much more important.

So I would change this description here to, like: If specified, the usage of all the values above depends on the custom validator.

That sounds pretty good, especially if it means we'll be able to use the existing fields/values rather than having to redefine them on the custom validator portion of the proto

For match_subject_alt_names, I would like to ask you about what the expected behavior looks like? When the name match, then how should we choose the trust domain? I got it. Choose the trust bundle based on the given trust domain first, and then verify the SAN name matching, and so maybe you want to have SAN matching conditions per trust bundle, right?

It could be that I want to allow multiple SPIFFE IDs that are in different trust domains. To support this, what we'll probably want to do is:

  1. Extract peer SPIFFE ID
  2. Choose correct bundle based on trust domain
  3. Validate peer SVID using the selected bundle
  4. Compare the validated SPIFFE ID to the allowed names from match_subject_alt_names

I don't think that we want to tie the matching to a particular bundle.

Related question: will a combined validation context work as expected if a custom validator config is set in one of them? i.e. can I set the name matching in a default context and then additionally configure the SPIFFE validator?

I think this would work as expected if the custom validator is able to consume the existing fields/values from the validation context proto?

The important part here is that the SPIFFE identity and bundles may be supplied by one system, and the authorization policy (e.g. match_subject_alt_names) from a different system. Since the config for authentication and authorization are mixed into the same proto, the only way we can (currently) solve for this is through the use of a combined validation context in which match_subject_alt_names is provided by one and custom_validator_config is provided by the other. If there is a better way to accomplish this, I'd love to hear about it

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this would work as expected if the custom validator is able to consume the existing fields/values from the validation context proto?

The important part here is that the SPIFFE identity and bundles may be supplied by one system, and the authorization policy (e.g. match_subject_alt_names) from a different system. Since the config for authentication and authorization are mixed into the same proto, the only way we can (currently) solve for this is through the use of a combined validation context in which match_subject_alt_names is provided by one and custom_validator_config is provided by the other. If there is a better way to accomplish this, I'd love to hear about it

OK, that makes sense. Thanks for the detailed description 👍

Copy link

Choose a reason for hiding this comment

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

Thanks for your work on this @mathetake please let me know if there's anything else I can do to help ❤️

dgn added a commit to dgn/envoy-maistra that referenced this pull request Apr 29, 2021
dgn added a commit to dgn/envoy-maistra that referenced this pull request Apr 29, 2021
dgn added a commit to dgn/envoy-maistra that referenced this pull request Apr 29, 2021
dgn added a commit to dgn/envoy-maistra that referenced this pull request Apr 29, 2021
dgn added a commit to dgn/envoy-maistra that referenced this pull request Apr 29, 2021
dgn added a commit to dgn/envoy-maistra that referenced this pull request Apr 29, 2021
dgn added a commit to dgn/envoy-maistra that referenced this pull request Apr 29, 2021
dgn added a commit to dgn/envoy-maistra that referenced this pull request Apr 29, 2021
dgn added a commit to dgn/envoy-maistra that referenced this pull request Apr 29, 2021
dgn added a commit to dgn/envoy-maistra that referenced this pull request Apr 29, 2021
dgn added a commit to dgn/envoy-maistra that referenced this pull request Apr 29, 2021
dgn added a commit to dgn/envoy-maistra that referenced this pull request Apr 29, 2021
dgn added a commit to dgn/envoy-maistra that referenced this pull request Apr 29, 2021
dgn added a commit to dgn/envoy-maistra that referenced this pull request Apr 29, 2021
dgn added a commit to dgn/envoy-maistra that referenced this pull request Apr 30, 2021
dgn added a commit to dgn/envoy-maistra that referenced this pull request Apr 30, 2021
dgn added a commit to dgn/envoy-maistra that referenced this pull request Apr 30, 2021
dgn added a commit to dgn/envoy-maistra that referenced this pull request Apr 30, 2021
dgn added a commit to dgn/envoy-maistra that referenced this pull request Apr 30, 2021
dgn added a commit to dgn/envoy-maistra that referenced this pull request Apr 30, 2021
dgn added a commit to dgn/envoy-maistra that referenced this pull request Apr 30, 2021
ipuustin pushed a commit to ipuustin/envoy that referenced this pull request Sep 23, 2021
…trust domain support (envoyproxy#14884)

Adds the extension point for certificate validations, and its first implementation for SPIFFE multi trust domain support in a single listener or cluster. Resolves envoyproxy#14614 and envoyproxy#9284.
Risk Level: low (only adding the new extension point and one implementation for it)
Testing: unit tests and integration tests.
Docs Changes:
Release Notes: tls: implement SPIFFE Certificate Validator for independent multiple trust domain support.

Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
ipuustin pushed a commit to ipuustin/envoy that referenced this pull request Sep 23, 2021
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.

SPIFFE and Envoy to achieve cross-cluster mTLS where every cluster utilizes an independent SPIFFE trust domain
9 participants