[Security] Add support for SPIFFE Bundle Maps in XDS bundles #8180

gtcooke94 · 2025-03-18T17:43:16Z

This adds support for configuring SPIFFE Bundle Maps inside of credentials via xds bundles.

See the gRFC for more detail grpc/proposal#462

RELEASE NOTES: N/A

codecov · 2025-03-18T17:54:32Z

Codecov Report

Attention: Patch coverage is 79.86577% with 30 lines in your changes missing coverage. Please review.

Project coverage is 82.12%. Comparing base (1f6b0cf) to head (327a534).
Report is 8 commits behind head on master.

Files with missing lines	Patch %	Lines
internal/testutils/tls_creds.go	55.00%	18 Missing and 9 partials ⚠️
internal/xds/bootstrap/tlscreds/bundle.go	93.87%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8180      +/-   ##
==========================================
- Coverage   82.42%   82.12%   -0.31%     
==========================================
  Files         393      410      +17     
  Lines       39162    40371    +1209     
==========================================
+ Hits        32280    33154     +874     
- Misses       5571     5855     +284     
- Partials     1311     1362      +51

Files with missing lines	Coverage Δ
credentials/tls/certprovider/pemfile/builder.go	`100.00% <100.00%> (ø)`
credentials/tls/certprovider/pemfile/watcher.go	`86.56% <100.00%> (-2.99%)`	⬇️
internal/credentials/spiffe/spiffe.go	`100.00% <100.00%> (ø)`
internal/xds/bootstrap/tlscreds/bundle.go	`86.86% <93.87%> (+4.41%)`	⬆️
internal/testutils/tls_creds.go	`55.00% <55.00%> (ø)`

... and 45 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

internal/credentials/spiffe/spiffe.go

internal/credentials/spiffe/spiffe_test.go

internal/xds/bootstrap/tlscreds/bundle.go

testdata/spiffe/spiffebundle_match_client_spiffe.json

testdata/spiffe_end2end/README.md

internal/credentials/spiffe/spiffe.go

internal/xds/bootstrap/tlscreds/bundle_ext_test.go

internal/xds/bootstrap/tlscreds/bundle_test.go

easwars · 2025-03-18T22:13:26Z

internal/xds/bootstrap/tlscreds/bundle_test.go

@@ -96,3 +102,79 @@ func (s) TestFailingProvider(t *testing.T) {
 		t.Errorf("EmptyCall() got err: %s, want err to contain: %s", err, wantErr)
 	}
 }
+
+func (s) TestSPIFFEVerifyFuncMismatchedCert(t *testing.T) {


Can/Should these tests be e2e style as well? That way, we can actually verify that the error returned to the user (as part of the RPC) has enough details in it for the user to take meaningful action.

You mean should we replicate these tests in bundle_ext_test.go and actually do the client/server call rather than just this inner call? We have one failure case test where there is no bundle for the peer certificates trust domain, I can expand and add these cases?

I went on and expanded the end2end tests, PTAL

easwars

Mostly LGTM. Just minor comments this time around.

internal/credentials/spiffe/spiffe_test.go

easwars · 2025-03-31T22:22:37Z

internal/credentials/spiffe/spiffe_test.go

+	const wantBundleSize = 2
 	if len(bundles) != wantBundleSize {
-		t.Fatalf("LoadSPIFFEBundleMap(%v) did not parse correct bundle length. got %v want %v", spiffeBundleFile, len(bundles), wantBundleSize)
+		t.Fatalf("BundleMapFromBytes(%v) did not parse correct bundle length. got %v want %v", spiffeBundleFile, len(bundles), wantBundleSize)
 	}
 	if bundles["example.com"] == nil {
-		t.Fatalf("LoadSPIFFEBundleMap(%v) got no bundle for example.com", spiffeBundleFile)
+		t.Fatalf("BundleMapFromBytes(%v) got no bundle for example.com", spiffeBundleFile)
 	}
 	if bundles["test.example.com"] == nil {
-		t.Fatalf("LoadSPIFFEBundleMap(%v) got no bundle for test.example.com", spiffeBundleFile)
+		t.Fatalf("BundleMapFromBytes(%v) got no bundle for test.example.com", spiffeBundleFile)
 	}

-	expectedExampleComCert := loadX509Cert(t, testdata.Path("spiffe/spiffe_cert.pem"))
-	expectedTestExampleComCert := loadX509Cert(t, testdata.Path("spiffe/server1_spiffe.pem"))
-	if !bundles["example.com"].X509Authorities()[0].Equal(expectedExampleComCert) {
-		t.Fatalf("LoadSPIFFEBundleMap(%v) parsed wrong cert for example.com.", spiffeBundleFile)
+	wantExampleComCert := loadX509Cert(t, testdata.Path("spiffe/spiffe_cert.pem"))
+	wantTestExampleComCert := loadX509Cert(t, testdata.Path("spiffe/server1_spiffe.pem"))
+	if !bundles["example.com"].X509Authorities()[0].Equal(wantExampleComCert) {
+		t.Fatalf("BundleMapFromBytes(%v) parsed wrong cert for example.com.", spiffeBundleFile)
 	}
-	if !bundles["test.example.com"].X509Authorities()[0].Equal(expectedTestExampleComCert) {
-		t.Fatalf("LoadSPIFFEBundleMap(%v) parsed wrong cert for test.example.com", spiffeBundleFile)
+	if !bundles["test.example.com"].X509Authorities()[0].Equal(wantTestExampleComCert) {
+		t.Fatalf("BundleMapFromBytes(%v) parsed wrong cert for test.example.com", spiffeBundleFile)
 	}


Instead of individual comparison of fields, would you be able to construct a single wantBundles of type map[string]*spiffebundle.Bundle and directly compare it with the result from BundleMapFromBytes?

See: go/go-style/decisions#compare-full-structures

The main way of creating a spiffebundle.Bundle is loading it from the bytes/file, which is largely what we are wrapping/testing. I could create one manually using the spiffebundle APIs in the test, but I think the readability would suffer significantly.
I could also manually craft more spiffebundle files that are bundles and not a larger bundle map as well, then load those using the spiffebundle library, but I think that also suffers from more confusion, readability issues, and bloat. In addition, that's more files that would need to be manually created and kept in sync with an update.

I'm happy to make it work if you feel very strongly, but I think this is a case where creating a full known struct to compare to is quite a bit more complex than comparing the fields that we care about. Let me know

internal/credentials/spiffe/spiffe_test.go

easwars · 2025-03-31T22:29:20Z

credentials/tls/certprovider/pemfile/builder.go

-		CertFile: cfg.CertificateFile,
-		KeyFile:  cfg.PrivateKeyFile,
-		RootFile: cfg.CACertificateFile,
+		CertFile:            cfg.CertificateFile,


It would be great if you could file an issue to track this. Otherwise, we will definitely forget about it. Thanks. We could even get someone from BLR to work on it if we have the it described in detail in an issue.

internal/xds/bootstrap/tlscreds/bundle_ext_test.go

easwars · 2025-04-03T00:11:55Z

internal/xds/bootstrap/tlscreds/bundle_ext_test.go

+		ss := stubserver.StubServer{
+			Address:    server.Address,
+			EmptyCallF: func(context.Context, *testpb.Empty) (*testpb.Empty, error) { return &testpb.Empty{}, nil },
+		}
+		server = stubserver.StartTestService(t, &ss, serverCredentials)


The restartable listener type provides a Stop and Restart method. When Stop is called, existing connections are closed and new connections are not accepted, and when Restart is called, new connections are accepted. Therefore, once Restart is called, we should see another handshake.

internal/xds/bootstrap/tlscreds/bundle_ext_test.go

Add support for SPIFFE Bundle Maps in xds bundle maps

fc60a23

gtcooke94 added Type: Security A bug or other problem affecting security Area: xDS Includes everything xDS related, including LB policies used with xDS. Area: Auth Includes regular credentials API and implementation. Also includes advancedtls, authz, rbac etc. labels Mar 18, 2025

gtcooke94 added this to the 1.72 Release milestone Mar 18, 2025

remove type that accidentally made it through merge

3b8bdcc

gtcooke94 requested a review from matthewstevenson88 March 18, 2025 17:53

gtcooke94 requested a review from easwars March 18, 2025 18:32

gtcooke94 assigned easwars and matthewstevenson88 Mar 18, 2025

matthewstevenson88 reviewed Mar 18, 2025

View reviewed changes

Address PR comments - expand test cases, nits

dc92b52

gtcooke94 requested a review from matthewstevenson88 March 18, 2025 20:24

matthewstevenson88 approved these changes Mar 18, 2025

View reviewed changes

internal/xds/bootstrap/tlscreds/bundle.go Outdated Show resolved Hide resolved

internal/xds/bootstrap/tlscreds/bundle.go Show resolved Hide resolved

internal/xds/bootstrap/tlscreds/bundle.go Show resolved Hide resolved

gtcooke94 added 2 commits March 18, 2025 20:43

cleaner length check

6f4df49

oops bad parenthesis

91e0fec

easwars reviewed Mar 18, 2025

View reviewed changes

easwars assigned gtcooke94 and unassigned easwars and matthewstevenson88 Mar 18, 2025

gtcooke94 added 4 commits March 19, 2025 16:32

address PR comments

a76e2c3

more end2end style tests

509d9e4

add and cleanup end2end tests

55f605b

clean up bundle_test.go

327a534

gtcooke94 requested a review from easwars March 21, 2025 16:06

gtcooke94 assigned easwars Mar 21, 2025

arjan-bal mentioned this pull request Mar 24, 2025

[Security] Add support for SPIFFE Bundle Maps in XDS bundles #8177

Closed

easwars reviewed Apr 3, 2025

View reviewed changes

easwars removed their assignment Apr 3, 2025

easwars assigned gtcooke94 and unassigned gtcooke94 Apr 3, 2025

eshitachandwani modified the milestones: 1.72 Release, 1.73 Release Apr 3, 2025

gtcooke94 added 3 commits April 3, 2025 14:17

style and informational PR comments

23ffdb3

use restartable listener

e866627

fatal to error where appropriate

f962395

gtcooke94 assigned easwars and unassigned gtcooke94 Apr 4, 2025

easwars approved these changes Apr 4, 2025

View reviewed changes

easwars assigned gtcooke94 and unassigned easwars Apr 4, 2025

gtcooke94 merged commit 4b5505d into grpc:master Apr 4, 2025
13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security] Add support for SPIFFE Bundle Maps in XDS bundles #8180

[Security] Add support for SPIFFE Bundle Maps in XDS bundles #8180

gtcooke94 commented Mar 18, 2025

codecov bot commented Mar 18, 2025 •

edited

Loading

easwars Mar 18, 2025

gtcooke94 Mar 19, 2025

gtcooke94 Mar 21, 2025

easwars left a comment

easwars Mar 31, 2025

gtcooke94 Apr 3, 2025

easwars Mar 31, 2025

easwars Apr 3, 2025

[Security] Add support for SPIFFE Bundle Maps in XDS bundles #8180

[Security] Add support for SPIFFE Bundle Maps in XDS bundles #8180

Conversation

gtcooke94 commented Mar 18, 2025

codecov bot commented Mar 18, 2025 • edited Loading

Codecov Report

easwars Mar 18, 2025

Choose a reason for hiding this comment

gtcooke94 Mar 19, 2025

Choose a reason for hiding this comment

gtcooke94 Mar 21, 2025

Choose a reason for hiding this comment

easwars left a comment

Choose a reason for hiding this comment

easwars Mar 31, 2025

Choose a reason for hiding this comment

gtcooke94 Apr 3, 2025

Choose a reason for hiding this comment

easwars Mar 31, 2025

Choose a reason for hiding this comment

easwars Apr 3, 2025

Choose a reason for hiding this comment

codecov bot commented Mar 18, 2025 •

edited

Loading