-
Notifications
You must be signed in to change notification settings - Fork 4.6k
[Security] Add support for SPIFFE Bundle Maps in XDS bundles #8180
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
Changes from 9 commits
fc60a23
3b8bdcc
dc92b52
6f4df49
91e0fec
a76e2c3
509d9e4
55f605b
327a534
23ffdb3
e866627
f962395
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,51 +22,49 @@ import ( | |
| "crypto/x509" | ||
| "encoding/pem" | ||
| "io" | ||
| "net/url" | ||
| "os" | ||
| "strings" | ||
| "testing" | ||
|
|
||
| "github.com/spiffe/go-spiffe/v2/bundle/spiffebundle" | ||
| "google.golang.org/grpc/testdata" | ||
| ) | ||
|
|
||
| // loadSPIFFEBundleMap loads a SPIFFE Bundle Map from a file. See the SPIFFE | ||
| // Bundle Map spec for more detail - | ||
| // https://github.com/spiffe/spiffe/blob/main/standards/SPIFFE_Trust_Domain_and_Bundle.md#4-spiffe-bundle-format | ||
| // If duplicate keys are encountered in the JSON parsing, Go's default unmarshal | ||
| // behavior occurs which causes the last processed entry to be the entry in the | ||
| // parsed map. | ||
| func loadSPIFFEBundleMap(filePath string) (map[string]*spiffebundle.Bundle, error) { | ||
| bundleMapRaw, err := os.ReadFile(filePath) | ||
| const wantURI = "spiffe://foo.bar.com/client/workload/1" | ||
|
|
||
| func loadFileBytes(t *testing.T, filePath string) []byte { | ||
| bytes, err := os.ReadFile(filePath) | ||
| if err != nil { | ||
| return nil, err | ||
| t.Fatalf("Error reading file: %v", err) | ||
| } | ||
| return BundleMapFromBytes(bundleMapRaw) | ||
| return bytes | ||
| } | ||
|
|
||
| func TestKnownSPIFFEBundle(t *testing.T) { | ||
| spiffeBundleFile := testdata.Path("spiffe/spiffebundle.json") | ||
| bundles, err := loadSPIFFEBundleMap(spiffeBundleFile) | ||
| spiffeBundleBytes := loadFileBytes(t, spiffeBundleFile) | ||
| bundles, err := BundleMapFromBytes(spiffeBundleBytes) | ||
| if err != nil { | ||
| t.Fatalf("LoadSPIFFEBundleMap(%v) Error during parsing: %v", spiffeBundleFile, err) | ||
| t.Fatalf("BundleMapFromBytes(%v) Error during parsing: %v", spiffeBundleFile, err) | ||
easwars marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| } | ||
| wantBundleSize := 2 | ||
| 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) | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of individual comparison of fields, would you be able to construct a single See: go/go-style/decisions#compare-full-structures
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The main way of creating a 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 |
||
|
|
||
| } | ||
|
|
@@ -86,7 +84,7 @@ func loadX509Cert(t *testing.T, filePath string) *x509.Certificate { | |
| return cert | ||
| } | ||
|
|
||
| func TestLoadSPIFFEBundleMapFailures(t *testing.T) { | ||
| func TestSPIFFEBundleMapFailures(t *testing.T) { | ||
| filePaths := []string{ | ||
| testdata.Path("spiffe/spiffebundle_corrupted_cert.json"), | ||
| testdata.Path("spiffe/spiffebundle_malformed.json"), | ||
|
|
@@ -95,28 +93,154 @@ func TestLoadSPIFFEBundleMapFailures(t *testing.T) { | |
| testdata.Path("spiffe/spiffebundle_wrong_multi_certs.json"), | ||
| testdata.Path("spiffe/spiffebundle_wrong_root.json"), | ||
| testdata.Path("spiffe/spiffebundle_wrong_seq_type.json"), | ||
| testdata.Path("NOT_A_REAL_FILE"), | ||
| testdata.Path("spiffe/spiffebundle_invalid_trustdomain.json"), | ||
| testdata.Path("spiffe/spiffebundle_empty_string_key.json"), | ||
| testdata.Path("spiffe/spiffebundle_empty_keys.json"), | ||
| } | ||
| for _, path := range filePaths { | ||
| t.Run(path, func(t *testing.T) { | ||
| if _, err := loadSPIFFEBundleMap(path); err == nil { | ||
| t.Fatalf("LoadSPIFFEBundleMap(%v) did not fail but should have.", path) | ||
| bundleBytes := loadFileBytes(t, path) | ||
| if _, err := BundleMapFromBytes(bundleBytes); err == nil { | ||
| t.Fatalf("BundleMapFromBytes(%v) did not fail but should have.", path) | ||
easwars marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| } | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestLoadSPIFFEBundleMapX509Failures(t *testing.T) { | ||
| func TestSPIFFEBundleMapX509Failures(t *testing.T) { | ||
| // SPIFFE Bundles only support a use of x509-svid and jwt-svid. If a | ||
| // use other than this is specified, the parser does not fail, it | ||
| // just doesn't add an x509 authority or jwt authority to the bundle | ||
| filePath := testdata.Path("spiffe/spiffebundle_wrong_use.json") | ||
| bundle, err := loadSPIFFEBundleMap(filePath) | ||
| bundleBytes := loadFileBytes(t, filePath) | ||
| bundle, err := BundleMapFromBytes(bundleBytes) | ||
| if err != nil { | ||
| t.Fatalf("LoadSPIFFEBundleMap(%v) failed with error: %v", filePath, err) | ||
| t.Fatalf("BundleMapFromBytes(%v) failed with error: %v", filePath, err) | ||
| } | ||
| if len(bundle["example.com"].X509Authorities()) != 0 { | ||
| t.Fatalf("LoadSPIFFEBundleMap(%v) did not have empty bundle but should have.", filePath) | ||
| t.Fatalf("BundleMapFromBytes(%v) did not have empty bundle but should have.", filePath) | ||
easwars marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| } | ||
| } | ||
|
|
||
| func TestGetRootsFromSPIFFEBundleMapSuccess(t *testing.T) { | ||
| bundleMapFile := testdata.Path("spiffe/spiffebundle_match_client_spiffe.json") | ||
| bundleBytes := loadFileBytes(t, bundleMapFile) | ||
| bundle, err := BundleMapFromBytes(bundleBytes) | ||
| if err != nil { | ||
| t.Fatalf("BundleMapFromBytes(%v) failed with error: %v", bundleMapFile, err) | ||
| } | ||
|
|
||
| cert := loadX509Cert(t, testdata.Path("spiffe/client_spiffe.pem")) | ||
| gotRoots, err := GetRootsFromSPIFFEBundleMap(bundle, cert) | ||
| if err != nil { | ||
| t.Fatalf("GetRootsFromSPIFFEBundleMap() failed with err %v", err) | ||
| } | ||
| wantRoot := loadX509Cert(t, testdata.Path("spiffe/spiffe_cert.pem")) | ||
| wantRoots := x509.NewCertPool() | ||
| wantRoots.AddCert(wantRoot) | ||
| if !gotRoots.Equal(wantRoots) { | ||
| t.Fatalf("GetRootsFromSPIFFEBundleMap() got %v want %v", gotRoots, wantRoots) | ||
| } | ||
| } | ||
|
|
||
| func TestGetRootsFromSPIFFEBundleMapFailures(t *testing.T) { | ||
matthewstevenson88 marked this conversation as resolved.
Show resolved
Hide resolved
matthewstevenson88 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| bundleMapFile := testdata.Path("spiffe/spiffebundle.json") | ||
| bundleBytes := loadFileBytes(t, bundleMapFile) | ||
| bundle, err := BundleMapFromBytes(bundleBytes) | ||
| certWithTwoURIs := loadX509Cert(t, testdata.Path("spiffe/client_spiffe.pem")) | ||
| certWithTwoURIs.URIs = append(certWithTwoURIs.URIs, certWithTwoURIs.URIs[0]) | ||
| certWithNoURIs := loadX509Cert(t, testdata.Path("spiffe/client_spiffe.pem")) | ||
| certWithNoURIs.URIs = nil | ||
| if err != nil { | ||
| t.Fatalf("BundleMapFromBytes(%v) failed with error: %v", bundleMapFile, err) | ||
| } | ||
| tests := []struct { | ||
| name string | ||
| bundleMapFile string | ||
| leafCert *x509.Certificate | ||
| wantErr string | ||
| }{ | ||
| { | ||
| name: "no bundle for peer cert spiffeID", | ||
| leafCert: loadX509Cert(t, testdata.Path("spiffe/client_spiffe.pem")), | ||
| wantErr: "no bundle found for peer certificates", | ||
| }, | ||
| { | ||
| name: "cert has invalid SPIFFE id", | ||
| leafCert: loadX509Cert(t, testdata.Path("ca.pem")), | ||
| wantErr: "could not get spiffe ID from peer leaf cert", | ||
| }, | ||
| { | ||
| name: "nil cert", | ||
| leafCert: nil, | ||
| wantErr: "input cert is nil", | ||
| }, | ||
| { | ||
| name: "cert has multiple URIs", | ||
| leafCert: certWithTwoURIs, | ||
| wantErr: "input cert has 2 URIs but should have 1", | ||
| }, | ||
| { | ||
| name: "cert has no URIs", | ||
| leafCert: certWithNoURIs, | ||
| wantErr: "input cert has 0 URIs but should have 1", | ||
| }, | ||
| } | ||
| for _, tc := range tests { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| _, err = GetRootsFromSPIFFEBundleMap(bundle, tc.leafCert) | ||
| if err == nil { | ||
| t.Fatalf("GetRootsFromSPIFFEBundleMap() got no error but want error containing %v.", tc.wantErr) | ||
| } | ||
| if !strings.Contains(err.Error(), tc.wantErr) { | ||
| t.Fatalf("GetRootsFromSPIFFEBundleMap() got error: %v. want error to contain %v", err, tc.wantErr) | ||
| } | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestIDFromCert(t *testing.T) { | ||
| cert := loadX509Cert(t, testdata.Path("x509/spiffe_cert.pem")) | ||
| uri, err := idFromCert(cert) | ||
| if err != nil { | ||
| t.Fatalf("idFromCert() failed with err: %v", err) | ||
| } | ||
| if uri != nil && uri.String() != wantURI { | ||
| t.Fatalf("ID not expected, got %s, want %s", uri.String(), wantURI) | ||
| } | ||
| } | ||
|
|
||
| func TestIDFromCertFileFailures(t *testing.T) { | ||
| certWithNoURIs := loadX509Cert(t, testdata.Path("spiffe/client_spiffe.pem")) | ||
| certWithNoURIs.URIs = nil | ||
| certWithInvalidSPIFFEID := loadX509Cert(t, testdata.Path("spiffe/client_spiffe.pem")) | ||
| certWithInvalidSPIFFEID.URIs = []*url.URL{{Path: "non-spiffe.bad"}} | ||
| tests := []struct { | ||
| name string | ||
| cert *x509.Certificate | ||
| }{ | ||
| { | ||
| name: "certificate with multiple URIs", | ||
| cert: loadX509Cert(t, testdata.Path("x509/multiple_uri_cert.pem")), | ||
| }, | ||
| { | ||
| name: "certificate with invalidSPIFFE ID", | ||
| cert: certWithInvalidSPIFFEID, | ||
| }, | ||
| { | ||
| name: "nil cert", | ||
| cert: nil, | ||
| }, | ||
| { | ||
| name: "cert with no URIs", | ||
| cert: certWithNoURIs, | ||
| }, | ||
| } | ||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| if _, err := idFromCert(tt.cert); err == nil { | ||
| t.Fatalf("IDFromCert() succeeded but want error") | ||
easwars marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| } | ||
| }) | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the only reason for using the above intermediate struct is to be able to specify a default value for the refresh interval, this could be done with the
encoding/jsonpackage as follows: https://stackoverflow.com/questions/30445479/how-to-specify-default-values-when-parsing-json-in-goThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could make that a follow-up? I'd rather not refactor the existing code and make a feature change at the same time? I didn't write this originally so I don't know if that's the only reason or not without digging in a little more
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done #8222