-
Notifications
You must be signed in to change notification settings - Fork 562
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
Add SPIFFE independent multiple trust domain API. #1923
Changes from 6 commits
4228ef2
09f34c3
c5d5976
27e2de5
eeb6813
dd2a7c1
bec86c9
b6494ea
dc4e8a2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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. | ||||||
// If set, they are used for this root CA. Otherwise, this root CA is used for default trust domain | ||||||
// and its aliases. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||||||
// Note that we can have multiple certificate data for a same trust_domain. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
// In that case, certificates with a same trust domain will be merged and used together to verify peer certificates. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
repeated string trust_domains = 3; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How does this relate to trust domain defined above? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see. May be add a comment related to it? I found it confusing so asked. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 very confusing. Can we clarify this? Even in the names, such as cluster_trust_domains or mesh_trust_domains? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this is that confusing since this is a field of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why is the trust_domains plural, does that means the certificate_data can belong to multi trust domains? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes. See discussion here: #1923 (comment) for example, it can be used for aliasing trust domains. |
||||||
} | ||||||
|
||||||
// The extra root certificates for workload-to-workload communication. | ||||||
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Large diffs are not rendered by default.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.