[Security] Add support for SPIFFE Bundle Maps in certificate providers #8167
Conversation
| // 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| 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, | ||
| } | ||
| } |
There was a problem hiding this comment.
Similar comment as in another place.
| // 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) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
| for _, useSPIFFEBundle := range []bool{true, false} { | ||
| testName := baseName | ||
| if useSPIFFEBundle { | ||
| testName = testName + "_" + "withSPIFFEBundle" | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
| for _, useSPIFFEBundle := range []bool{true, false} { | ||
| testName := baseName | ||
| if useSPIFFEBundle { | ||
| testName = testName + "_" + "withSPIFFEBundle" | ||
| } |
There was a problem hiding this comment.
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?
| 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, | ||
| } | ||
| } |
| // 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) |
There was a problem hiding this comment.
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
|
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. |
| // // 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. |
There was a problem hiding this comment.
Nit: Please remove the double //
|
|
||
| // // 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 |
There was a problem hiding this comment.
And remove this commented out line.
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