[Security] Add support for SPIFFE Bundle Maps in certificate providers by gtcooke94 · Pull Request #8167 · grpc/grpc-go

gtcooke94 · 2025-03-12T18:03:33Z

This adds support for providers, (and specifically root file watcher providers) to return SPIFFE Bundle Maps in the KeyMaterial.

See the gRFC for more detail grpc/proposal#462

RELEASE NOTES: N/A

easwars · 2025-03-14T22:35:10Z

+	// SPIFFEBundleMap is an in-memory representation of a spiffe trust bundle
+	// map. If this value exists, it will be used to find the roots for a given
+	// trust domain rather than the Roots in this struct.
+	SPIFFEBundleMap spiffe.BundleMap


This type spiffe.BundleMap, is defined in an internal package. But the KeyMaterial struct is exported to our users. This will be unfortunate if we add a new field in a publicly exported type, but our users can't have access to that type.

Would it be possible instead of the pemfile certificate provider to convert the SPIFFE bundle into an x509.CertPool and return it using the Roots field instead? I see that there is a way to extract the certificates from a SPIFFE bundle. So, you might be able to extract them and create an x509.CertPool and add those certificates to that pool and return it. But I don't know if functionally, that will still do what you want it to do.

I think the right thing to do here would be to make spiffe.BundleMap publicly exported

Tne SPIFFE bundle map can't be directly set as x509.CertPool roots. It's essentially a map of string->root certs, where the string is a SPIFFE ID, and at handshake time you lookup the roots for the peer's SPIFFE ID, and after this map lookup we set the roots. But the KeyMaterial call doesn't necessarily happen during the handshake.

We could make it a map of strings to x509.CertPool (which is essentially what it is), but I think it would be okay to publicly export the type - would you rather see a public module that has the spiffe.BundleMap, or I think we can typealias and say pemfile.spiffeBundleMap = spiffe.BundleMap?

I think it would be easaier/nicer if we make SPIFFEBundleMap to be of type map[string]*spiffebundle.Bundle here and get rid of the BundleMap type in the internal spiffe package.

We also need to document the Roots field that it has lower priority over the SPIFFEBundleMap field.

Also, maybe SPIFFEBundles is a better name as opposed to SPIFFEBundleMap?

Removed the type in preference of map[string]*spiffebundle.Bundle and notated the preferences in the Roots field

The SPIFFEBundleMap name here comes from the spec, rather than because it is a Map in Go - I think we should keep the name.

easwars · 2025-03-14T22:51:20Z

+			var opts Options
+			if useSPIFFEBundle {
+				opts = Options{
+					CertFile:            path.Join(symLinkName, certFile),
+					KeyFile:             path.Join(symLinkName, keyFile),
+					RootFile:            path.Join(symLinkName, rootFile),
+					SPIFFEBundleMapFile: path.Join(symLinkName, spiffeBundleFile),
+					RefreshDuration:     defaultTestRefreshDuration,
+				}
+			} else {
+				opts = Options{
+					CertFile:        path.Join(symLinkName, certFile),
+					KeyFile:         path.Join(symLinkName, keyFile),
+					RootFile:        path.Join(symLinkName, rootFile),
+					RefreshDuration: defaultTestRefreshDuration,
+				}
+			}


Similar comment as in another place.

easwars · 2025-03-14T22:52:06Z

+				// Since we have root and identity certs, we need to make sure the
+				// update is pushed on both of them.
+				if _, err := distCh.Receive(ctx); err != nil {
+					t.Fatalf("timeout waiting for provider to read files and push key material to distributor: %v", err)


Here and in other places in this PR, test errors need not follow the error string capitalization guideline. Please see: go/go-style/decisions#error-strings

I apologize, a lot of this is just code that is in the diff because it was tabbed over, I didn't closely read all of the existing errors. I'll do my best to clean up

easwars · 2025-03-14T22:53:57Z

+	for _, useSPIFFEBundle := range []bool{true, false} {
+		testName := baseName
+		if useSPIFFEBundle {
+			testName = testName + "_" + "withSPIFFEBundle"
+		}


Making tests easier to read (even if that means they are more verbose) should be an ideal to strive for. Would it be possible to refactor this such that you have separate tests for the SPIFFE case and the non-spiffe case, but they both call into a common function to do the common parts.

I think they are all fundamentally testing the same thing, except setting one option value or not (i.e. are the configured files reloaded). And the test setup for creating temp files and a distributor who's channel we control and block on is relatively complicated - in my opinion it's easiest to do this setup with these if statements for configuration rather than duplicating the complex code.

Or, do you mean just separate this for loop into two separate tests that call into a func that is the content of t.Run?

gtcooke94

Sorry, missed a few comments

gtcooke94 · 2025-03-17T15:26:04Z

+	for _, useSPIFFEBundle := range []bool{true, false} {
+		testName := baseName
+		if useSPIFFEBundle {
+			testName = testName + "_" + "withSPIFFEBundle"
+		}


I think they are all fundamentally testing the same thing, except setting one option value or not (i.e. are the configured files reloaded). And the test setup for creating temp files and a distributor who's channel we control and block on is relatively complicated - in my opinion it's easiest to do this setup with these if statements for configuration rather than duplicating the complex code.

Or, do you mean just separate this for loop into two separate tests that call into a func that is the content of t.Run?

gtcooke94 · 2025-03-17T15:27:03Z

+			var opts Options
+			if useSPIFFEBundle {
+				opts = Options{
+					CertFile:            path.Join(symLinkName, certFile),
+					KeyFile:             path.Join(symLinkName, keyFile),
+					RootFile:            path.Join(symLinkName, rootFile),
+					SPIFFEBundleMapFile: path.Join(symLinkName, spiffeBundleFile),
+					RefreshDuration:     defaultTestRefreshDuration,
+				}
+			} else {
+				opts = Options{
+					CertFile:        path.Join(symLinkName, certFile),
+					KeyFile:         path.Join(symLinkName, keyFile),
+					RootFile:        path.Join(symLinkName, rootFile),
+					RefreshDuration: defaultTestRefreshDuration,
+				}
+			}


gtcooke94 · 2025-03-17T15:28:28Z

+				// Since we have root and identity certs, we need to make sure the
+				// update is pushed on both of them.
+				if _, err := distCh.Receive(ctx); err != nil {
+					t.Fatalf("timeout waiting for provider to read files and push key material to distributor: %v", err)


I apologize, a lot of this is just code that is in the diff because it was tabbed over, I didn't closely read all of the existing errors. I'll do my best to clean up

easwars · 2025-03-17T16:52:10Z

Please don't mark conversations as resolved. It should be the reviewer who marks them as resolved, when they have been satisfactorily handled. Now I have to click unresolve on every comment and verify that it has been handled satisfactorily.

…e directly

easwars · 2025-03-17T18:02:50Z

+// // BundleMap represents a SPIFFE Bundle Map per the spec
+// // https://github.com/spiffe/spiffe/blob/main/standards/SPIFFE_Trust_Domain_and_Bundle.md#4-spiffe-bundle-format.


Nit: Please remove the double //

easwars · 2025-03-17T18:03:02Z


+// // BundleMap represents a SPIFFE Bundle Map per the spec
+// // https://github.com/spiffe/spiffe/blob/main/standards/SPIFFE_Trust_Domain_and_Bundle.md#4-spiffe-bundle-format.
+// type BundleMap map[string]*spiffebundle.Bundle


And remove this commented out line.

grpc#8167)

gtcooke94 added 30 commits February 19, 2025 20:04

add spiffe test files

4bb0db3

Add go spiffe dependency

1afc0d4

add spiffe bundle dep

c9109bc

move spiffe test files

c92682d

progress

c84bbea

some progress

dfb8d5a

spiffe bundle map parsing

1ca1fa1

.

3aeff78

.

455d2a1

Add some documentation

d323023

mod tidy

78622b3

Address PR comments, add test for not a real file

564e562

add comment for duplicate handling

7bd94aa

Merge branch 'master' into spiffe

809f1c4

mod tidy after master

d8ff305

mod tidy examples

654a042

trailing newline in spiffe cnf

338ae2c

add copyright

c3231b6

trailing newline

2ea2059

newlines at end of json

5c5c530

remove spiffebundle1.json

2e11bd9

update multi certs issue

7028fe5

go mod tidy elsewhere

caf8539

fix formatting

1c1b767

use TrustDomainFromString

4c25f23

fixes for CI from arjan-bal@17e8bbf

bb249a0

.

02b6d9c

rebase conflict

571af50

more watcher tests

6fdee4a

add another test file

dcfb6b5

gtcooke94 assigned easwars and unassigned easwars Mar 13, 2025

easwars reviewed Mar 14, 2025

View reviewed changes

easwars assigned gtcooke94 and unassigned easwars Mar 14, 2025

gtcooke94 added 4 commits March 17, 2025 14:35

pr comments

2a8219c

test bad updates

e39a56d

root file coverage

e6a7f8a

pr comments

d205855

gtcooke94 commented Mar 17, 2025

View reviewed changes

gtcooke94 requested a review from easwars March 17, 2025 15:48

gtcooke94 assigned easwars and unassigned gtcooke94 Mar 17, 2025

easwars assigned gtcooke94 and unassigned easwars Mar 17, 2025

swap spiffe.BundleMap type to just use map[string]*spiffebundle.bundl…

1083657

…e directly

gtcooke94 assigned easwars and unassigned gtcooke94 Mar 17, 2025

lowercase beginning of loadSpiffeBundleMap

f4f0e8e

easwars approved these changes Mar 17, 2025

View reviewed changes

easwars assigned gtcooke94 and unassigned easwars Mar 17, 2025

remove commented code

6a54ef8

gtcooke94 merged commit 1f6b0cf into grpc:master Mar 17, 2025
14 of 15 checks passed

gtcooke94 added a commit to gtcooke94/grpc-go that referenced this pull request Mar 17, 2025

[Security] Add support for SPIFFE Bundle Maps in certificate providers (

74934a1

grpc#8167)

gtcooke94 added a commit to gtcooke94/grpc-go that referenced this pull request Mar 17, 2025

[Security] Add support for SPIFFE Bundle Maps in certificate providers (

6b0c325

grpc#8167)

gtcooke94 added a commit to gtcooke94/grpc-go that referenced this pull request Mar 17, 2025

[Security] Add support for SPIFFE Bundle Maps in certificate providers (

1b7b914

grpc#8167)

janardhanvissa pushed a commit to janardhanvissa/grpc-go that referenced this pull request Mar 24, 2025

[Security] Add support for SPIFFE Bundle Maps in certificate providers (

5dae129

grpc#8167)

github-actions Bot locked as resolved and limited conversation to collaborators Sep 14, 2025

		// // BundleMap represents a SPIFFE Bundle Map per the spec
		// // https://github.com/spiffe/spiffe/blob/main/standards/SPIFFE_Trust_Domain_and_Bundle.md#4-spiffe-bundle-format.

Conversation

gtcooke94 commented Mar 12, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gtcooke94 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

easwars commented Mar 17, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants