[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

easwars · 2025-03-18T21:20:08Z

testdata/spiffe/spiffebundle_match_client_spiffe.json

We need an entry for this in testdata/spiffe/README.md?

easwars · 2025-03-18T21:20:44Z

testdata/spiffe_end2end/README.md

@@ -0,0 +1,32 @@
+All of the following files in this directory except `server_spiffebundle.json` and `client_spiffebundle.json` are generated with the `generate.sh` and `generate_intermediate.sh` script in this directory.


Nit: Wrap .md files at 80-cols?

easwars · 2025-03-18T21:22:06Z

testdata/spiffe_end2end/README.md

+The SPIFFE trust bundle map files (*_spiffebundle.json) are manually created for
+end to end testing. The server_spiffebundle.json contains the "foo.bar.com"
+trust domain (only this entry is used in e2e tests) matching URI SAN of
+client_spiffe.pem, and the CA certificate is ca.pem. The client


Some file names have backticks, while some don't. Can we make them consistent please.

easwars · 2025-03-18T21:23:15Z

testdata/spiffe_end2end/README.md

+can be done in golang with the following codeblock: ``` func
+GetBase64ModulusFromPublicKey(key *rsa.PublicKey) string { return
+base64.RawURLEncoding.EncodeToString(key.N.Bytes()) }
+
+block, _ := pem.Decode(rawPemCert) cert, _ := x509.ParseCertificate(block.Bytes)
+publicKey := cert.PublicKey.(*rsa.PublicKey)
+fmt.Println(GetBase64ModulusFromPublicKey(publicKey)) ```


This code block doesn't render properly. Please ensure that the markdown file renders as you expect it to.

Looks like some auto formatting messed it up, fixed

easwars · 2025-03-18T21:37:06Z

internal/credentials/spiffe/spiffe.go

+
+// IDFromCert parses the SPIFFE ID from the x509.Certificate. If the certificate
+// does not have a valid SPIFFE ID, returns an error.
+func IDFromCert(cert *x509.Certificate) (*spiffeid.ID, error) {


Does this function have to be exported? I see it only being called from here and from tests.

Good call, changed

easwars · 2025-03-18T22:11:05Z

internal/xds/bootstrap/tlscreds/bundle_ext_test.go

+	cfg := fmt.Sprintf(`{
+		"certificate_file": "%s",
+		"private_key_file": "%s",
+		"spiffe_trust_bundle_map_file": "%s"
+	}`,
+		testdata.Path("spiffe_end2end/client_spiffe.pem"),
+		testdata.Path("spiffe_end2end/client.key"),
+		testdata.Path("spiffe_end2end/client_spiffebundle.json"))


Same here too.

Discussion in other comment

easwars · 2025-03-18T22:11:29Z

internal/xds/bootstrap/tlscreds/bundle_ext_test.go

+	if _, err = client.EmptyCall(ctx, &testpb.Empty{}); err == nil {
+		t.Errorf("EmptyCall(): got success. want failure")
+	}
+	wantErr := "spiffe: no bundle found for peer certificates"


easwars · 2025-03-18T22:11:37Z

internal/xds/bootstrap/tlscreds/bundle_ext_test.go

+		t.Errorf("EmptyCall(): got success. want failure")
+	}
+	wantErr := "spiffe: no bundle found for peer certificates"
+	if !strings.Contains(err.Error(), wantErr) {


Also check the status code?

easwars · 2025-03-18T22:11:56Z

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) {
+	wantErrContains := "spiffe: x509 certificate Verify failed"


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

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

Open

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

easwars Mar 18, 2025

gtcooke94 Mar 19, 2025

easwars Mar 18, 2025

gtcooke94 Mar 19, 2025

easwars Mar 18, 2025

gtcooke94 Mar 19, 2025

easwars Mar 18, 2025

gtcooke94 Mar 19, 2025

easwars Mar 18, 2025

gtcooke94 Mar 19, 2025

easwars Mar 18, 2025

gtcooke94 Mar 19, 2025

easwars Mar 18, 2025

gtcooke94 Mar 19, 2025

easwars Mar 18, 2025

gtcooke94 Mar 19, 2025

easwars Mar 18, 2025

gtcooke94 Mar 19, 2025

gtcooke94 Mar 21, 2025

		@@ -0,0 +1,32 @@
		All of the following files in this directory except `server_spiffebundle.json` and `client_spiffebundle.json` are generated with the `generate.sh` and `generate_intermediate.sh` script in this directory.

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

Are you sure you want to change the base?

[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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Mar 18, 2025 •

edited

Loading