Add SPIFFE independent multiple trust domain API. #1923

mathetake · 2021-03-23T02:01:58Z

Now that Envoy side implementations (envoyproxy/envoy#14884, envoyproxy/envoy#15509) have landed upstream, and "independent multiple trust domain authN/Z" can be achieved in Istio. So this PR adds API for that.

References:

Support multiple trust bundles in Istio istio#30454
Design doc (shared with istio-team-drive-access group.)
SPIFFE and Envoy to achieve cross-cluster mTLS where every cluster utilizes an independent SPIFFE trust domain envoyproxy/envoy#14614

Signed-off-by: Takeshi Yoneda [email protected]

istio-policy-bot · 2021-03-23T02:02:00Z

😊 Welcome @mathetake! This is either your first contribution to the Istio api repo, or it's been
awhile since you've been here.

You can learn more about the Istio working groups, code of conduct, and contributing guidelines
by referring to Contributing to Istio.

Thanks for contributing!

Courtesy of your friendly welcome wagon.

istio-testing · 2021-03-23T02:02:14Z

Hi @mathetake. Thanks for your PR.

I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Signed-off-by: Takeshi Yoneda <[email protected]>

mathetake · 2021-03-23T02:18:23Z

cc @lizan

lizan · 2021-03-23T02:22:30Z

@myidpt @howardjohn PTAL

mesh/v1alpha1/config.proto

lizan · 2021-03-24T06:33:30Z

/ok-to-test

mesh/v1alpha1/config.proto

security/v1beta1/authorization_policy.proto

mesh/v1alpha1/config.proto

…st-domain

Signed-off-by: Takeshi Yoneda <[email protected]>

nrjpoddar · 2021-03-30T03:28:43Z

mesh/v1alpha1/config.proto

+    // and its aliases.
+    // Note that we can have multiple certificate data for a same trust_domain.
+    // In that case, certificates with a same trust domain will be merged and used together to verify peer certificates.
+    repeated string trust_domains = 3;


How does this relate to trust domain defined above?

this is associated with the "additional" root ca (CertificateData) to be used to validate incoming peer certs. In contrast, the trust domain above is used for singing certs with this cluster's root ca (e.g. istio-ca-secret).

I see. May be add a comment related to it? I found it confusing so asked.

+1 very confusing. Can we clarify this? Even in the names, such as cluster_trust_domains or mesh_trust_domains?

I don't think this is that confusing since this is a field of CertificateData which is not in the top-level MeshConfig. But I agree that this is a bit confusing. @costinm @howardjohn thoughts?

Is there a corresponding PR in the Istio Repo yet to handle Certificates for multiple TrustDomains? As of now, it is assumed that all these trustAnchors are intended for the configured mesh TrustDomain

…st-domain

Signed-off-by: Takeshi Yoneda <[email protected]>

…st-domain

istio-testing · 2021-04-19T05:49:05Z

@mathetake: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
release-notes_api	`dc4e8a2`	link	`/test release-notes_api`

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

hzxuzhonghu · 2021-04-20T06:26:31Z

mesh/v1alpha1/config.proto

+    // and its aliases.
+    // Note that we can have multiple certificate data for a same trust_domain.
+    // In that case, certificates with a same trust domain will be merged and used together to verify peer certificates.
+    repeated string trust_domains = 3;


why is the trust_domains plural, does that means the certificate_data can belong to multi trust domains?

yes. See discussion here: #1923 (comment) for example, it can be used for aliasing trust domains.

mathetake · 2021-04-21T04:43:53Z

@costinm @howardjohn could you take another look? Thanks!

howardjohn · 2021-04-28T18:42:37Z

security/v1beta1/authorization_policy.proto

@@ -410,6 +410,10 @@ message Source {

  // Optional. A list of negative match of remote IP blocks.
  repeated string not_remote_ip_blocks = 10;
+
+  // Optional. A list of trust domains of peer certificates.


If I have mesh config:

trustDomain: foo trustDomainAliases: [bar, baz]

and I set trustDomains=foo, do bar and baz apply?
what if I set trustDomains=baz?

Basically are aliases applied here?

Thanks for putting another insight. I think we should implicitly apply foo and bar as well when only setting trustDomains=baz in line with the doc of trustDomainAliases saying "aliases are treated as the same trust domain". And this is how we can treat foo, bar and baz as the same trust domain.

so maybe should i put the comment about this behavior here?

Yes, please. In this case, we should always treat foo, bar, baz as identical.
Normally, users should not specify the trust_domains when they apply the authz policy to the local trust domain. But if they did, trust domain aliasing also applies here.

howardjohn

LGTM, one small clarification

costinm

I am a bit concerned with the 3 names - trust_domain, trust_domain_aliasses and trust_domains. Note that trust_domain is used to indicate what we'll put in the certs we generate, and the aliases was introduced mainly for migrations - and doesn't make sense once
we have the more explicit trust bundles.

From the Spiffee spec: "implementations MUST securely associate bundle endpoints with their respective trust domain, nominally through explicit configuration of a <trust_domain_name, endpoint> tuple" - no mention of aliases, which can be represented by having more tuples ( except that it is discouraged to have same keys associated with different names !)

https://tools.ietf.org/html/rfc5280#section-4.2.1.10 has the concept of 'name constraints' and
'permittedSubtrees' - which can apply to both Spiffee URL SANs but also domain SANs and even IP certs. However those are built into the cert PEM - so wouldn't need any configuration on our side.

Same for OIDC - the 'trust domain' is built into the discovery URL.

In other words: I'm worried we're again reinventing the wheel and confusing people, like we did with few other things in security.

I am also worried about the 'mesh wide' / mesh operator applicability of the config - instead of using ProxyConfig which allows more specific targeting. Since DestinationRule, Policy, etc are driven by the mesh user - I don't see why the trust endpoints would be mesh-wide and controlled by operator.

So my alternate proposals:

add this to ProxyConfig instead ( or maybe even DestinationRule - combined with the proposal we just heard in Env/networking for improving verification of SANs )
either use a 'trustDomain' ( without aliases - see spiffee spec/recommendation ), or use the RFC5280 naming, mentioning that the constraints can also be loaded from the cert itself. Since this kind of constraining of a root cert is in no way specific to spiffee URL SANs - I would very much prefer the 'constraints' approach.
If the URL form is specified I would just document that for Istio we recommend using the OIDC format for the URL, which includes the 'trust domain' in the URL in a well-known form, and validate that.

costinm · 2021-05-03T13:51:10Z

I would also request having this reviewed by Env WG ( MeshConfig vs ProxyConfig ) and Networking ( vs DestinationRule, and how it can fit/interact with the new proposal for validation in DR ). In particular we have a pretty big disconnect between networking - defining SAN validations in DR and client SAN validation in Gateway - and mesh config terms that are only used in Spiffee ( like the fact we validate ones by default and others not, ownership of configs, etc ).

I think this is a very important and useful PR - I would love to see this replace the 'trustDomainAliases' with a more standards-based and consistrent/integrated solution.

myidpt · 2021-05-03T16:49:37Z

mesh/v1alpha1/config.proto

@@ -222,6 +222,13 @@ message MeshConfig {
      // The certificate is retrieved from the endpoint.
      string spiffe_bundle_url = 2;
    }
+
+    // Optional. Specify the list of trust domains to which this certificate data belongs.


Suggested change

// Optional. Specify the list of trust domains to which this certificate data belongs.

// Optional. Specify the list of trust domains to which this root of trust belongs.

myidpt · 2021-05-03T16:51:47Z

mesh/v1alpha1/config.proto

+
+    // Optional. Specify the list of trust domains to which this certificate data belongs.
+    // If set, they are used for this root CA. Otherwise, this root CA is used for default trust domain
+    // and its aliases.


This becomes error prone now. If the user forgets to specify trust_domains, the external CAs will be able to impersonate the local CA. I think in the long term, we should mandate this field to be non-empty (ie, if it's empty, the CertificateData is invalid). How about if users want to use this to configure extra roots for the local trust domains. they should set "LOCAL" as the trust domain? We need to think about the backward compatibility, though. Istiod can log warnings in 1.11 when trust_domains is unset, and we mandate it in 1.12.

myidpt · 2021-05-03T16:56:32Z

mesh/v1alpha1/config.proto

+    // Optional. Specify the list of trust domains to which this certificate data belongs.
+    // If set, they are used for this root CA. Otherwise, this root CA is used for default trust domain
+    // and its aliases.
+    // Note that we can have multiple certificate data for a same trust_domain.


Suggested change

// Note that we can have multiple certificate data for a same trust_domain.

// Note that we can have multiple roots of trust for a same trust domain.

myidpt · 2021-05-03T16:57:52Z

mesh/v1alpha1/config.proto

+    // If set, they are used for this root CA. Otherwise, this root CA is used for default trust domain
+    // and its aliases.
+    // Note that we can have multiple certificate data for a same trust_domain.
+    // In that case, certificates with a same trust domain will be merged and used together to verify peer certificates.


Suggested change

// In that case, certificates with a same trust domain will be merged and used together to verify peer certificates.

// In that case, root certificates associated with the same trust domain will be merged and used together to verify peer certificates from that trust domain.

myidpt · 2021-05-03T17:09:51Z

security/v1beta1/authorization_policy.proto

@@ -410,6 +410,10 @@ message Source {

  // Optional. A list of negative match of remote IP blocks.
  repeated string not_remote_ip_blocks = 10;
+
+  // Optional. A list of trust domains of peer certificates.


Yes, please. In this case, we should always treat foo, bar, baz as identical.
Normally, users should not specify the trust_domains when they apply the authz policy to the local trust domain. But if they did, trust domain aliasing also applies here.

mathetake · 2021-05-10T01:02:22Z

@costinm Thank you very much for putting your thoughts in detail.

add this to ProxyConfig instead ( or maybe even DestinationRule - combined with the proposal we just heard in Env/networking for improving verification of SANs )

Could you add reviewers from these WG to this PR? And I would appreciate it if you could put the link to the proposal.

either use a 'trustDomain' ( without aliases - see spiffee spec/recommendation ), or use the RFC5280 naming, mentioning that the constraints can also be loaded from the cert itself. Since this kind of constraining of a root cert is in no way specific to spiffee URL SANs - I would very much prefer the 'constraints' approach.

Agreed and now I would love to go with 'naming constraints' and I'm even thinking of removing 'trust_domain(s)' field, which seems much clearer and standardized. Does it make sense to you to remove the field and only support loading domains from naming constrains?

If the URL form is specified I would just document that for Istio we recommend using the OIDC format for the URL, which includes the 'trust domain' in the URL in a well-known form, and validate that.

sg but if a user wants to load a "inline" cert (via "pem" field or equivalent in the future), then this cannot be available. So I guess this would introduce another confusion on users. I would prefer we stick to either "naming constrians" approach or "trust_domain" (without "s") approach as you proposed above. WDYT?

istio-testing · 2021-05-12T18:36:15Z

@mathetake: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

mathetake · 2021-10-19T04:02:04Z

superseded by #2090

istio-testing added the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Mar 23, 2021

google-cla bot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Mar 23, 2021

istio-testing added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 23, 2021

istio-testing added the needs-ok-to-test label Mar 23, 2021

Add SPIFFE independent multiple trust domain API.

4228ef2

Signed-off-by: Takeshi Yoneda <[email protected]>

mathetake marked this pull request as ready for review March 23, 2021 02:16

mathetake requested review from dcberg, duderino, linsun, louiscryan, nrjpoddar and smawson as code owners March 23, 2021 02:16

istio-testing removed the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Mar 23, 2021

howardjohn reviewed Mar 23, 2021

View reviewed changes

mesh/v1alpha1/config.proto Outdated Show resolved Hide resolved

mesh/v1alpha1/config.proto Outdated Show resolved Hide resolved

istio-testing added ok-to-test Set this label allow normal testing to take place for a PR not submitted by an Istio org member. needs-rebase Indicates a PR needs to be rebased before being merged and removed needs-ok-to-test labels Mar 24, 2021

costinm reviewed Mar 28, 2021

View reviewed changes

mesh/v1alpha1/config.proto Outdated Show resolved Hide resolved

costinm reviewed Mar 28, 2021

View reviewed changes

security/v1beta1/authorization_policy.proto Outdated Show resolved Hide resolved

howardjohn reviewed Mar 29, 2021

View reviewed changes

mesh/v1alpha1/config.proto Outdated Show resolved Hide resolved

mathetake added 2 commits March 30, 2021 09:46

Merge remote-tracking branch 'origin/master' into spiffe-multiple-tru…

09f34c3

…st-domain

have multiple trust_domains in CertificateData, and fixed comment

c5d5976

Signed-off-by: Takeshi Yoneda <[email protected]>

istio-testing added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed needs-rebase Indicates a PR needs to be rebased before being merged size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 30, 2021

Add comments about multiple cert data with a same trust_domain

eeb6813

Signed-off-by: Takeshi Yoneda <[email protected]>

istio-testing added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 30, 2021

fix comment: client -> peer

dd2a7c1

Signed-off-by: Takeshi Yoneda <[email protected]>

mathetake requested review from costinm and howardjohn March 30, 2021 01:34

nrjpoddar reviewed Mar 30, 2021

View reviewed changes

istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Apr 2, 2021

mathetake added 2 commits April 5, 2021 09:10

Merge remote-tracking branch 'origin/master' into spiffe-multiple-tru…

bec86c9

…st-domain

re make

b6494ea

Signed-off-by: Takeshi Yoneda <[email protected]>

istio-testing removed the needs-rebase Indicates a PR needs to be rebased before being merged label Apr 5, 2021

caleygoff-invitae mentioned this pull request Apr 12, 2021

Please consider adding a walkthrough for cross-cluster mTLS support with unique trust_domains for each cluster aws/aws-app-mesh-examples#407

Open

istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Apr 18, 2021

Merge remote-tracking branch 'origin/master' into spiffe-multiple-tru…

dc4e8a2

…st-domain

istio-testing removed the needs-rebase Indicates a PR needs to be rebased before being merged label Apr 19, 2021

hzxuzhonghu reviewed Apr 20, 2021

View reviewed changes

howardjohn reviewed Apr 28, 2021

View reviewed changes

costinm reviewed May 3, 2021

View reviewed changes

myidpt reviewed May 3, 2021

View reviewed changes

istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label May 12, 2021

howardjohn mentioned this pull request Sep 22, 2021

add signer:rootcert map into mesh config #2090

Merged

mathetake closed this Oct 19, 2021

mathetake deleted the spiffe-multiple-trust-domain branch October 19, 2021 04:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add SPIFFE independent multiple trust domain API. #1923

Add SPIFFE independent multiple trust domain API. #1923

mathetake commented Mar 23, 2021 •

edited

Loading

istio-policy-bot commented Mar 23, 2021

istio-testing commented Mar 23, 2021

mathetake commented Mar 23, 2021

lizan commented Mar 23, 2021

lizan commented Mar 24, 2021

nrjpoddar Mar 30, 2021

mathetake Apr 1, 2021

nrjpoddar Apr 1, 2021

linsun Apr 2, 2021 •

edited

Loading

mathetake Apr 4, 2021

shankgan Apr 30, 2021

istio-testing commented Apr 19, 2021

hzxuzhonghu Apr 20, 2021

mathetake Apr 20, 2021 •

edited

Loading

mathetake commented Apr 21, 2021

howardjohn Apr 28, 2021

mathetake Apr 29, 2021

myidpt May 3, 2021

howardjohn left a comment

costinm left a comment

costinm commented May 3, 2021 •

edited

Loading

myidpt May 3, 2021

myidpt May 3, 2021

myidpt May 3, 2021

myidpt May 3, 2021

myidpt May 3, 2021

mathetake commented May 10, 2021 •

edited

Loading

istio-testing commented May 12, 2021

mathetake commented Oct 19, 2021

	// Optional. Specify the list of trust domains to which this certificate data belongs.
	// Optional. Specify the list of trust domains to which this root of trust belongs.

	// Note that we can have multiple certificate data for a same trust_domain.
	// Note that we can have multiple roots of trust for a same trust domain.

	// In that case, certificates with a same trust domain will be merged and used together to verify peer certificates.
	// In that case, root certificates associated with the same trust domain will be merged and used together to verify peer certificates from that trust domain.

Add SPIFFE independent multiple trust domain API. #1923

Add SPIFFE independent multiple trust domain API. #1923

Conversation

mathetake commented Mar 23, 2021 • edited Loading

istio-policy-bot commented Mar 23, 2021

istio-testing commented Mar 23, 2021

mathetake commented Mar 23, 2021

lizan commented Mar 23, 2021

lizan commented Mar 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linsun Apr 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

istio-testing commented Apr 19, 2021

Choose a reason for hiding this comment

mathetake Apr 20, 2021 • edited Loading

Choose a reason for hiding this comment

mathetake commented Apr 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

howardjohn left a comment

Choose a reason for hiding this comment

costinm left a comment

Choose a reason for hiding this comment

costinm commented May 3, 2021 • 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

mathetake commented May 10, 2021 • edited Loading

istio-testing commented May 12, 2021

mathetake commented Oct 19, 2021

mathetake commented Mar 23, 2021 •

edited

Loading

linsun Apr 2, 2021 •

edited

Loading

mathetake Apr 20, 2021 •

edited

Loading

costinm commented May 3, 2021 •

edited

Loading

mathetake commented May 10, 2021 •

edited

Loading