Skip to content

[Security] Add verification logic using SPIFFE Bundle Maps in XDS #8229

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

Merged
merged 24 commits into from
Apr 22, 2025

Conversation

gtcooke94
Copy link
Contributor

This adds support for using SPIFFE Bundle Maps for verification in XDS clients and servers.
As part of this, setting cfg.VerifyPeerCertificate moved from xds.go to handshake_info.go because of how the layering works with accessing the SPIFFE Bundle Map of roots - note, this makes the diff a touch harder to read.

See the gRFC for more detail grpc/proposal#462

RELEASE NOTES: N/A

@easwars easwars added the Type: Feature New features or improvements in behavior label Apr 7, 2025
@easwars easwars added this to the 1.72 Release milestone Apr 7, 2025
@easwars easwars assigned matthewstevenson88 and unassigned easwars Apr 7, 2025
@easwars
Copy link
Contributor

easwars commented Apr 7, 2025

I'll wait for an LGTM from @matthewstevenson88 before making a pass.

Copy link
Contributor Author

@gtcooke94 gtcooke94 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed all comments, still working offline about adjusting the MTLS tests - fixes will come for test failures soon

Copy link
Contributor

@matthewstevenson88 matthewstevenson88 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, modulo failing tests and the one open comment thread.

Copy link

codecov bot commented Apr 16, 2025

Codecov Report

Attention: Patch coverage is 84.74576% with 18 lines in your changes missing coverage. Please review.

Project coverage is 82.04%. Comparing base (4b5505d) to head (c0ef139).
Report is 18 commits behind head on master.

Files with missing lines Patch % Lines
internal/testutils/xds/e2e/bootstrap.go 75.51% 8 Missing and 4 partials ⚠️
internal/testutils/xds/e2e/setup/setup.go 64.70% 4 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8229      +/-   ##
==========================================
+ Coverage   81.98%   82.04%   +0.06%     
==========================================
  Files         410      412       +2     
  Lines       40382    40564     +182     
==========================================
+ Hits        33107    33282     +175     
- Misses       5895     5899       +4     
- Partials     1380     1383       +3     
Files with missing lines Coverage Δ
credentials/xds/xds.go 90.00% <ø> (+2.40%) ⬆️
internal/credentials/xds/handshake_info.go 97.64% <100.00%> (+0.74%) ⬆️
internal/testutils/xds/e2e/setup/setup.go 64.70% <64.70%> (ø)
internal/testutils/xds/e2e/bootstrap.go 65.48% <75.51%> (+5.78%) ⬆️

... and 50 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

@easwars easwars left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly LGTM. Just some minor comments.

@gtcooke94 gtcooke94 assigned easwars and unassigned gtcooke94 Apr 17, 2025
@purnesh42H purnesh42H modified the milestones: 1.72 Release, 1.73 Release Apr 21, 2025
authType := pr.AuthInfo.AuthType()
switch wantSecLevel {
case e2e.SecurityLevelNone:
// if pr.AuthInfo.AuthType() != "insecure" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove the commented out line.

@easwars easwars assigned gtcooke94 and unassigned easwars Apr 22, 2025
@gtcooke94 gtcooke94 merged commit 58d1a72 into grpc:master Apr 22, 2025
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature New features or improvements in behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants