-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
xds: add use_system_root_certs to CertificateValidationContext #34235
Conversation
Signed-off-by: Mark D. Roth <roth@google.com>
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
// If true, system root certs are used only if neither of the ``trusted_ca`` | ||
// or ``ca_certificate_provider_instance`` fields are set. | ||
// [#not-implemented-hide:] | ||
bool use_system_root_certs = 17; |
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 wonder if this should be a message (empty for now) to future proof against potentially wanting some control over system cert details. For example allow or deny listing certain inbuilt certs.
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 opposed to that if you feel strongly, but I don't think it's really necessary. The real intent of this option is to tell the client to use built-in logic that knows how to find the system root certs without the control plane having to know the details. If we wanted the control plane to directly manage this, we would just use certificate providers instead.
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.
It's not just control plane but also bootstrap in some cases. I think just a SystemRoots
empty message for now. I can see folks wanting to do filtering and other stuff from orchestration logic and so on potentially.
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.
The bootstrap use-case is a good point. Okay, changed it to an empty message.
Signed-off-by: Mark D. Roth <roth@google.com>
@htuch ping |
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.
LGTM, thanks!
Commit Message: xds: add use_system_root_certs to CertificateValidationContext
Additional Description: This allows using system root certs in gRPC. For details, see grpc/proposal#436.
Risk Level: Low
Testing: N/A
Docs Changes: Included in PR
Release Notes: N/A
Platform Specific Features: N/A