-
Notifications
You must be signed in to change notification settings - Fork 4.5k
[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 3 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,13 +22,17 @@ import ( | |
"crypto/x509" | ||
"encoding/pem" | ||
"io" | ||
"net/url" | ||
"os" | ||
"strings" | ||
"testing" | ||
|
||
"github.com/spiffe/go-spiffe/v2/bundle/spiffebundle" | ||
"google.golang.org/grpc/testdata" | ||
) | ||
|
||
const wantURI = "spiffe://foo.bar.com/client/workload/1" | ||
|
||
// 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 | ||
|
@@ -47,26 +51,26 @@ func TestKnownSPIFFEBundle(t *testing.T) { | |
spiffeBundleFile := testdata.Path("spiffe/spiffebundle.json") | ||
bundles, err := loadSPIFFEBundleMap(spiffeBundleFile) | ||
easwars marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if err != nil { | ||
t.Fatalf("LoadSPIFFEBundleMap(%v) Error during parsing: %v", spiffeBundleFile, err) | ||
t.Fatalf("loadSPIFFEBundleMap(%v) Error during parsing: %v", spiffeBundleFile, err) | ||
} | ||
wantBundleSize := 2 | ||
easwars marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if len(bundles) != wantBundleSize { | ||
t.Fatalf("LoadSPIFFEBundleMap(%v) did not parse correct bundle length. got %v want %v", spiffeBundleFile, len(bundles), wantBundleSize) | ||
t.Fatalf("loadSPIFFEBundleMap(%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("loadSPIFFEBundleMap(%v) got no bundle for example.com", spiffeBundleFile) | ||
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. This should probably be a 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. Actually made another suggestion regarding these checks. 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. Replied in other thread, left the ones checking for |
||
} | ||
if bundles["test.example.com"] == nil { | ||
t.Fatalf("LoadSPIFFEBundleMap(%v) got no bundle for test.example.com", spiffeBundleFile) | ||
t.Fatalf("loadSPIFFEBundleMap(%v) got no bundle for test.example.com", spiffeBundleFile) | ||
} | ||
|
||
expectedExampleComCert := loadX509Cert(t, testdata.Path("spiffe/spiffe_cert.pem")) | ||
easwars marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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) | ||
t.Fatalf("loadSPIFFEBundleMap(%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) | ||
t.Fatalf("loadSPIFFEBundleMap(%v) parsed wrong cert for test.example.com", spiffeBundleFile) | ||
} | ||
|
||
} | ||
|
@@ -86,7 +90,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"), | ||
|
@@ -97,26 +101,229 @@ func TestLoadSPIFFEBundleMapFailures(t *testing.T) { | |
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) | ||
t.Fatalf("loadSPIFFEBundleMap(%v) did not fail but should have.", path) | ||
} | ||
}) | ||
} | ||
} | ||
|
||
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) | ||
if err != nil { | ||
t.Fatalf("LoadSPIFFEBundleMap(%v) failed with error: %v", filePath, err) | ||
t.Fatalf("loadSPIFFEBundleMap(%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("loadSPIFFEBundleMap(%v) did not have empty bundle but should have.", filePath) | ||
} | ||
} | ||
|
||
func TestGetRootsFromSPIFFEBundleMapSuccess(t *testing.T) { | ||
bundleMapFile := testdata.Path("spiffe/spiffebundle_match_client_spiffe.json") | ||
bundle, err := loadSPIFFEBundleMap(bundleMapFile) | ||
matthewstevenson88 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if err != nil { | ||
t.Fatalf("loadSPIFFEBundleMap(%v) failed with error: %v", bundleMapFile, err) | ||
} | ||
|
||
cert := loadX509Cert(t, testdata.Path("spiffe/client_spiffe.pem")) | ||
rootCerts, err := GetRootsFromSPIFFEBundleMap(bundle, cert) | ||
easwars marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if err != nil { | ||
t.Fatalf("GetRootsFromSPIFFEBundleMap() failed with err %v", err) | ||
} | ||
wantRoot := loadX509Cert(t, testdata.Path("spiffe/spiffe_cert.pem")) | ||
wantPool := x509.NewCertPool() | ||
easwars marked this conversation as resolved.
Show resolved
Hide resolved
|
||
wantPool.AddCert(wantRoot) | ||
if !rootCerts.Equal(wantPool) { | ||
t.Fatalf("GetRootsFromSPIFFEBundleMap() got %v want %v", rootCerts, wantPool) | ||
} | ||
} | ||
|
||
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
|
||
tests := []struct { | ||
name string | ||
bundleMapFile string | ||
leafCertFile string | ||
wantErr string | ||
}{ | ||
{ | ||
name: "no bundle for peer cert spiffeID", | ||
bundleMapFile: testdata.Path("spiffe/spiffebundle.json"), | ||
leafCertFile: testdata.Path("spiffe/client_spiffe.pem"), | ||
wantErr: "no bundle found for peer certificates", | ||
}, | ||
{ | ||
name: "cert has invalid SPIFFE id", | ||
bundleMapFile: testdata.Path("spiffe/spiffebundle.json"), | ||
leafCertFile: testdata.Path("ca.pem"), | ||
wantErr: "could not get spiffe ID from peer leaf cert", | ||
}, | ||
} | ||
for _, tc := range tests { | ||
t.Run(tc.name, func(t *testing.T) { | ||
bundle, err := loadSPIFFEBundleMap(tc.bundleMapFile) | ||
if err != nil { | ||
t.Fatalf("loadSPIFFEBundleMap(%v) failed with error: %v", tc.bundleMapFile, err) | ||
} | ||
cert := loadX509Cert(t, tc.leafCertFile) | ||
_, err = GetRootsFromSPIFFEBundleMap(bundle, cert) | ||
if err == nil { | ||
t.Fatalf("GetRootsFromSPIFFEBundleMap() want err got none") | ||
} | ||
if !strings.Contains(err.Error(), tc.wantErr) { | ||
t.Fatalf("GetRootsFromSPIFFEBundleMap() want err to contain %v. got %v", tc.wantErr, err) | ||
easwars marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
}) | ||
} | ||
} | ||
|
||
func TestGetRootsFromSPIFFEBundleMapNilCert(t *testing.T) { | ||
wantErr := "input cert is nil" | ||
easwars marked this conversation as resolved.
Show resolved
Hide resolved
|
||
bundleMapFile := testdata.Path("spiffe/spiffebundle.json") | ||
bundle, err := loadSPIFFEBundleMap(bundleMapFile) | ||
if err != nil { | ||
t.Fatalf("loadSPIFFEBundleMap(%v) failed with error: %v", bundleMapFile, err) | ||
} | ||
_, err = GetRootsFromSPIFFEBundleMap(bundle, nil) | ||
if err == nil { | ||
t.Fatalf("GetRootsFromSPIFFEBundleMap() want err got none") | ||
} | ||
if !strings.Contains(err.Error(), wantErr) { | ||
t.Fatalf("GetRootsFromSPIFFEBundleMap() want err to contain %v. got %v", wantErr, err) | ||
easwars marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
} | ||
|
||
func TestGetRootsFromSPIFFEBundleMapMultipleURIs(t *testing.T) { | ||
easwars marked this conversation as resolved.
Show resolved
Hide resolved
|
||
wantErr := "input cert has more than 1 URI" | ||
easwars marked this conversation as resolved.
Show resolved
Hide resolved
|
||
bundleMapFile := testdata.Path("spiffe/spiffebundle.json") | ||
leafCertFile := testdata.Path("spiffe/client_spiffe.pem") | ||
bundle, err := loadSPIFFEBundleMap(bundleMapFile) | ||
if err != nil { | ||
t.Fatalf("loadSPIFFEBundleMap(%v) failed with error: %v", bundleMapFile, err) | ||
} | ||
cert := loadX509Cert(t, leafCertFile) | ||
// Add a duplicate URI of the first | ||
cert.URIs = append(cert.URIs, cert.URIs[0]) | ||
_, err = GetRootsFromSPIFFEBundleMap(bundle, cert) | ||
if err == nil { | ||
t.Fatalf("GetRootsFromSPIFFEBundleMap() want err got none") | ||
} | ||
if !strings.Contains(err.Error(), wantErr) { | ||
t.Fatalf("GetRootsFromSPIFFEBundleMap() want err to contain %v. got %v", wantErr, err) | ||
easwars marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
} | ||
|
||
func TestIDFromCert(t *testing.T) { | ||
data, err := os.ReadFile(testdata.Path("x509/spiffe_cert.pem")) | ||
if err != nil { | ||
t.Fatalf("os.ReadFile(%s) failed: %v", "x509/spiffe_cert.pem", err) | ||
} | ||
block, _ := pem.Decode(data) | ||
if block == nil { | ||
t.Fatalf("Failed to parse the certificate: byte block is nil") | ||
} | ||
cert, err := x509.ParseCertificate(block.Bytes) | ||
if err != nil { | ||
t.Fatalf("x509.ParseCertificate(%b) failed: %v", block.Bytes, err) | ||
} | ||
easwars marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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) { | ||
tests := []struct { | ||
name string | ||
dataPath string | ||
}{ | ||
{ | ||
name: "certificate with multiple URIs", | ||
dataPath: "x509/multiple_uri_cert.pem", | ||
}, | ||
{ | ||
name: "certificate without SPIFFE ID", | ||
dataPath: "x509/client1_cert.pem", | ||
}, | ||
} | ||
for _, tt := range tests { | ||
t.Run(tt.name, func(t *testing.T) { | ||
data, err := os.ReadFile(testdata.Path(tt.dataPath)) | ||
if err != nil { | ||
t.Fatalf("os.ReadFile(%s) failed: %v", testdata.Path(tt.dataPath), err) | ||
} | ||
block, _ := pem.Decode(data) | ||
if block == nil { | ||
t.Fatalf("Failed to parse the certificate: byte block is nil") | ||
} | ||
cert, err := x509.ParseCertificate(block.Bytes) | ||
if err != nil { | ||
t.Fatalf("x509.ParseCertificate(%b) failed: %v", block.Bytes, err) | ||
} | ||
_, err = IDFromCert(cert) | ||
if err == nil { | ||
easwars marked this conversation as resolved.
Show resolved
Hide resolved
|
||
t.Fatalf("IDFromCert() succeeded but want error") | ||
easwars marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
}) | ||
} | ||
} | ||
|
||
func TestIDFromCertNoURIs(t *testing.T) { | ||
data, err := os.ReadFile(testdata.Path("x509/spiffe_cert.pem")) | ||
if err != nil { | ||
t.Fatalf("os.ReadFile(%s) failed: %v", testdata.Path("x509/spiffe_cert.pem"), err) | ||
} | ||
block, _ := pem.Decode(data) | ||
if block == nil { | ||
t.Fatalf("Failed to parse the certificate: byte block is nil") | ||
} | ||
cert, err := x509.ParseCertificate(block.Bytes) | ||
if err != nil { | ||
t.Fatalf("x509.ParseCertificate(%b) failed: %v", block.Bytes, err) | ||
} | ||
// This cert has valid URIs. Set to nil for this test | ||
cert.URIs = nil | ||
_, err = IDFromCert(cert) | ||
if err == nil { | ||
t.Fatalf("IDFromCert() succeeded but want error") | ||
} | ||
} | ||
|
||
func TestIDFromCertNonSPIFFEURI(t *testing.T) { | ||
easwars marked this conversation as resolved.
Show resolved
Hide resolved
|
||
data, err := os.ReadFile(testdata.Path("x509/spiffe_cert.pem")) | ||
if err != nil { | ||
t.Fatalf("os.ReadFile(%s) failed: %v", testdata.Path("x509/spiffe_cert.pem"), err) | ||
} | ||
block, _ := pem.Decode(data) | ||
if block == nil { | ||
t.Fatalf("Failed to parse the certificate: byte block is nil") | ||
} | ||
cert, err := x509.ParseCertificate(block.Bytes) | ||
if err != nil { | ||
t.Fatalf("x509.ParseCertificate(%b) failed: %v", block.Bytes, err) | ||
} | ||
// This cert has valid URIs. Set to nil for this test | ||
cert.URIs = []*url.URL{{Path: "non-spiffe.bad"}} | ||
_, err = IDFromCert(cert) | ||
if err == nil { | ||
t.Fatal("IDFromCert() succeeded but want error") | ||
} | ||
} | ||
|
||
func TestIDFromCertNil(t *testing.T) { | ||
_, err := IDFromCert(nil) | ||
if err == nil { | ||
t.Fatalf("IDFromCert() succeeded but want error") | ||
} | ||
} |
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/json
package 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