Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
49 changes: 49 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,51 @@ 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 := credinternal.SPIFFEIDFromCert(leafCert)
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 %v 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 x509.Certificate. If the SPIFFE
// ID format is invalid, return nil with warning.
func IDFromCert(cert *x509.Certificate) (*spiffeid.ID, error) {
// return spiffeid.FromURI(cert.URIs)
if cert == nil {
return nil, fmt.Errorf("spiffe: IDFromCert() failed because input cert is nil")
}
// A valid SPIFFE Certificate should have exactly one URI
if cert.URIs == nil {
return nil, fmt.Errorf("spiffe: IDFromCert() failed because input cert has no URIs")
}
if len(cert.URIs) > 1 {
return nil, fmt.Errorf("spiffe: IDFromCert() failed because input cert has more than 1 URI")
}
ID, err := spiffeid.FromURI(cert.URIs[0])
if err != nil {
return nil, fmt.Errorf("spiffe: IDFromCert() failed with invalid spiffeid: %v", err)
}
return &ID, nil
}
191 changes: 180 additions & 11 deletions internal/credentials/spiffe/spiffe_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -47,26 +51,26 @@ func TestKnownSPIFFEBundle(t *testing.T) {
spiffeBundleFile := testdata.Path("spiffe/spiffebundle.json")
bundles, err := loadSPIFFEBundleMap(spiffeBundleFile)
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
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)
Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably be a t.Errorf. See: go/go-style/decisions#keep-going

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually made another suggestion regarding these checks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replied in other thread, left the ones checking for nil as Fatalf because we call functions on them later. Changed the later ones to Errorf.

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

}
Expand All @@ -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"),
Expand All @@ -101,22 +105,187 @@ func TestLoadSPIFFEBundleMapFailures(t *testing.T) {
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)
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)
if err != nil {
t.Fatalf("GetRootsFromSPIFFEBundleMap() failed with err %v", err)
}
wantRoot := loadX509Cert(t, testdata.Path("spiffe/spiffe_cert.pem"))
wantPool := x509.NewCertPool()
wantPool.AddCert(wantRoot)
if !rootCerts.Equal(wantPool) {
t.Fatalf("GetRootsFromSPIFFEBundleMap() got %v want %v", rootCerts, wantPool)
}
}

func TestGetRootsFromSPIFFEBundleMapFailures(t *testing.T) {
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)
}
})
}
}

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)
}
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 {
t.Fatalf("IDFromCert() succeeded but want error")
}
})
}
}

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