-
Notifications
You must be signed in to change notification settings - Fork 4.6k
xds: Implement system root certs support #8013
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
Conversation
67db837 to
07c8a31
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #8013 +/- ##
==========================================
+ Coverage 81.93% 82.32% +0.38%
==========================================
Files 381 383 +2
Lines 38581 38824 +243
==========================================
+ Hits 31613 31963 +350
+ Misses 5640 5539 -101
+ Partials 1328 1322 -6
|
easwars
left a comment
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.
Mostly LGTM. Mainly minor comments.
| systemRootCertsFunc = origSRCF | ||
| }() | ||
| envconfig.XDSSystemRootCertsEnabled = true | ||
| systemRootCertsFunc = origSRCF |
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 think this line is redundant, right, because we override systemRootCertsFunc right below.
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.
You're right, it's a copy/paste error. Removed it.
|
|
||
| func (systemRootCertsProvider) Close() {} | ||
|
|
||
| func (systemRootCertsProvider) KeyMaterial(_ context.Context) (*certprovider.KeyMaterial, error) { |
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.
Nit: since the context is the only parameter, can't we omit the _ parameter name?
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.
Done.
As per gRFC A82, xDS client should be able to use system root certificates to validate server identity if the use of system root certs is configured by the xDS management server. This is useful when the gRPC server is behind a reverse proxy. As per the gRFC, the feature is behind a feature flag till it is proven stable.
RELEASE NOTES:
GRPC_EXPERIMENTAL_XDS_SYSTEM_ROOT_CERTStotrue(case insensitive). See gRFC A82 (A82: xDS System Root Certificates proposal#436) for details.