-
Notifications
You must be signed in to change notification settings - Fork 233
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
A82: xDS System Root Certificates #436
Merged
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
4127707
A82: xDS System Root Certificates
markdroth e2df54f
add link to xDS PR
markdroth c381eda
clarify language and add link to new xDS field
markdroth 5e99ab1
add mailing list link
markdroth 6727148
support this field on client side only
markdroth 2220a1b
clarify wording
markdroth 2cf3687
add link to C-core impl
markdroth File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,83 @@ | ||
A82: xDS System Root Certificates | ||
---- | ||
* Author(s): @markdroth | ||
* Approver: @ejona86, @dfawley | ||
* Status: {Draft, In Review, Ready for Implementation, Implemented} | ||
* Implemented in: <language, ...> | ||
* Last updated: 2024-07-08 | ||
* Discussion at: https://groups.google.com/g/grpc-io/c/BgqeUU0q4fU | ||
|
||
## Abstract | ||
|
||
We will add a new xDS option to use the system's default root | ||
certificates for TLS certificate validation. | ||
|
||
## Background | ||
|
||
Most service mesh workloads use mTLS, as described in [gRFC A29][A29]. | ||
However, there are cases where it is useful for applications to use | ||
normal TLS rather than using certificates for workload identity, such as | ||
when a mesh wants to move some workloads behind a reverse proxy. | ||
|
||
gRPC already has code to find the system root certificates on various | ||
platforms. However, there is currently no way for the xDS control plane | ||
to tell the client to use that functionality, at least not without the | ||
cumbersome setup of duplicating that functionality in a certificate | ||
provider config in the xDS bootstrap file. | ||
|
||
### Related Proposals: | ||
* [gRFC A29: xDS mTLS Security][A29] | ||
|
||
[A29]: A29-xds-tls-security.md | ||
|
||
## Proposal | ||
|
||
We have added a [`system_root_certs` | ||
field](https://github.com/envoyproxy/envoy/blob/84d8fdd11e78013cd50596fa3b704e152512455e/api/envoy/extensions/transport_sockets/tls/v3/common.proto#L399) | ||
to the xDS `CertificateValidationContext` message (see | ||
envoyproxy/envoy#34235). In the gRPC client, if this field is present | ||
and the `ca_certificate_provider_instance` field is unset, system root | ||
certificates will be used for validation. | ||
|
||
### xDS Resource Validation | ||
|
||
When processing a CDS resource, we will look at this new field if | ||
`ca_certificate_provider_instance` is unset. The parsed CDS resource | ||
delivered to the XdsClient watcher will indicate if system root certs | ||
should be used. If feasible, the parsed representation should be | ||
structured such that it is not possible to indicate both a certificate | ||
provider instance and using system root certs, since those options are | ||
mutually exclusive. | ||
|
||
Note that LDS validation will be unchanged. The new `system_root_certs` | ||
field will be ignored on the gRPC server side. | ||
|
||
### xds_cluster_impl LB Policy Changes | ||
|
||
The xds_cluster_impl LB policy sets the configuration for the XdsCreds | ||
functionality based on the CDS resource. We will modify it such that if | ||
the CDS resource indicates that system root certs are to be used, it | ||
will configure XdsCreds to use system root certs. | ||
|
||
### XdsCredentials Changes | ||
|
||
The XdsCredentials code will be modified such that if it is configured | ||
to use system root certs, it will configure the TlsCreds code to do that. | ||
|
||
### Temporary environment variable protection | ||
|
||
Use of the `use_system_root_certs` field in CDS and LDS will be guarded | ||
by the `GRPC_EXPERIMENTAL_XDS_SYSTEM_ROOT_CERTS` env var. The env var | ||
guard will be removed once the feature passes interop tests. | ||
|
||
## Rationale | ||
|
||
We already have code in gRPC to find the system root certs for various | ||
platforms. We don't want to have to reproduce that functionality in a | ||
cert provider impl. | ||
|
||
## Implementation | ||
|
||
C-core implementation in https://github.com/grpc/grpc/pull/37185. | ||
|
||
Will also be implemented in Java, Go, and Node. |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
I'm not sure I noticed that we were going to have different validation for client-side even though this is in a shared message with server-side. Up above it says "Note that LDS validation will be unchanged" which appears to disagree with this line. Only supporting it on client-side might be a bit harder to support, since this is in CommonTlsContext. I agree we don't need it on server-side, but can we add support for it anyway when there is shared code?
Also: s/use_system_root_certs/system_root_certs/
CC @kannanjgithub
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.
We did talk about this question specifically before finalizing this gRFC. I had originally intended to support this option on both the client side and server side, just for consistency, but when I went to implement this in C-core, it turned out to be non-trivial, because we don't already have server-side code for using system root certs, so we decided to exclude it.
You're right about the typo. I'll send a separate PR to fix that.
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.
Sent #451 to fix this.