Skip to content

[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

Merged
merged 12 commits into from
Apr 4, 2025
16 changes: 9 additions & 7 deletions credentials/tls/certprovider/pemfile/builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -63,19 +63,21 @@ func pluginConfigFromJSON(jd json.RawMessage) (Options, error) {
// is that the refresh_interval is represented here as a duration proto,
// while in the latter a time.Duration is used.
cfg := &struct {
CertificateFile string `json:"certificate_file,omitempty"`
PrivateKeyFile string `json:"private_key_file,omitempty"`
CACertificateFile string `json:"ca_certificate_file,omitempty"`
RefreshInterval json.RawMessage `json:"refresh_interval,omitempty"`
CertificateFile string `json:"certificate_file,omitempty"`
PrivateKeyFile string `json:"private_key_file,omitempty"`
CACertificateFile string `json:"ca_certificate_file,omitempty"`
SPIFFETrustBundleMapFile string `json:"spiffe_trust_bundle_map_file,omitempty"`
RefreshInterval json.RawMessage `json:"refresh_interval,omitempty"`
}{}
if err := json.Unmarshal(jd, cfg); err != nil {
return Options{}, fmt.Errorf("pemfile: json.Unmarshal(%s) failed: %v", string(jd), err)
}

opts := Options{
CertFile: cfg.CertificateFile,
KeyFile: cfg.PrivateKeyFile,
RootFile: cfg.CACertificateFile,
CertFile: cfg.CertificateFile,
Copy link
Contributor

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-go

Copy link
Contributor Author

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

Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done #8222

KeyFile: cfg.PrivateKeyFile,
RootFile: cfg.CACertificateFile,
SPIFFEBundleMapFile: cfg.SPIFFETrustBundleMapFile,
// Refresh interval is the only field in the configuration for which we
// support a default value. We cannot possibly have valid defaults for
// file paths to watch. Also, it is valid to specify an empty path for
Expand Down
2 changes: 1 addition & 1 deletion credentials/tls/certprovider/pemfile/watcher.go
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ func newProvider(o Options) certprovider.Provider {
if o.CertFile != "" && o.KeyFile != "" {
provider.identityDistributor = newDistributor()
}
if o.RootFile != "" {
if o.RootFile != "" || o.SPIFFEBundleMapFile != "" {
provider.rootDistributor = newDistributor()
}

Expand Down
44 changes: 44 additions & 0 deletions internal/credentials/spiffe/spiffe.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
package spiffe

import (
"crypto/x509"
"encoding/json"
"fmt"

Expand Down Expand Up @@ -61,3 +62,46 @@ func BundleMapFromBytes(bundleMapBytes []byte) (map[string]*spiffebundle.Bundle,
}
return bundleMap, nil
}

// GetRootsFromSPIFFEBundleMap returns the root trust certificates from the
// SPIFFE bundle map for the given trust domain from the leaf certificate.
func GetRootsFromSPIFFEBundleMap(bundleMap map[string]*spiffebundle.Bundle, leafCert *x509.Certificate) (*x509.CertPool, error) {
// 1. Upon receiving a peer certificate, verify that it is a well-formed SPIFFE
// leaf certificate. In particular, it must have a single URI SAN containing
// a well-formed SPIFFE ID ([SPIFFE ID format]).
spiffeID, err := idFromCert(leafCert)
if err != nil {
return nil, fmt.Errorf("spiffe: could not get spiffe ID from peer leaf cert but verification with spiffe trust map was configured: %v", err)
}

// 2. Use the trust domain in the peer certificate's SPIFFE ID to lookup
// the SPIFFE trust bundle. If the trust domain is not contained in the
// configured trust map, reject the certificate.
spiffeBundle, ok := bundleMap[spiffeID.TrustDomain().Name()]
if !ok {
return nil, fmt.Errorf("spiffe: no bundle found for peer certificates trust domain %q but verification with a SPIFFE trust map was configured", spiffeID.TrustDomain().Name())
}
roots := spiffeBundle.X509Authorities()
rootPool := x509.NewCertPool()
for _, root := range roots {
rootPool.AddCert(root)
}
return rootPool, nil
}

// 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) {
if cert == nil {
return nil, fmt.Errorf("input cert is nil")
}
// A valid SPIFFE Certificate should have exactly one URI.
if len(cert.URIs) != 1 {
return nil, fmt.Errorf("input cert has %v URIs but should have 1", len(cert.URIs))
}
id, err := spiffeid.FromURI(cert.URIs[0])
if err != nil {
return nil, fmt.Errorf("invalid spiffeid: %v", err)
}
return &id, nil
}
186 changes: 155 additions & 31 deletions internal/credentials/spiffe/spiffe_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
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.Errorf("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.Errorf("BundleMapFromBytes(%v) parsed wrong cert for test.example.com", spiffeBundleFile)
}

}
Expand All @@ -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"),
Expand All @@ -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)
}
})
}
}

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)
}
}

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) {
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")
}
})
}
}
Loading