spiffe: correct trust domain checking (#588)

SPIFFE issuers must configure a trust domain. We no longer assume that
the the trust domain has some implicit relationship with the OIDC issuer
domain. Tokens with a mismatch in trust domain are rejected.

Signed-off-by: Nathan Smith <nathan@chainguard.dev>

pkg/api/grpc_server_test.go

@@ -293,7 +293,8 @@ func TestAPIWithUriSubject(t *testing.T) {
 			%q: {
 				"IssuerURL": %q,
 				"ClientID": "sigstore",
-				"Type": "spiffe"
+				"Type": "spiffe",
+				"SPIFFETrustDomain": "foo.com"
 			},
 			%q: {
 				"IssuerURL": %q,
@@ -307,7 +308,7 @@ func TestAPIWithUriSubject(t *testing.T) {
 		t.Fatalf("config.Read() = %v", err)
 	}
 
-	spiffeSubject := strings.ReplaceAll(spiffeIssuer+"/foo/bar", "http", "spiffe")
+	spiffeSubject := "spiffe://foo.com/bar"
 	uriSubject := uriIssuer + "/users/1"
 
 	tests := []oidcTestContainer{

pkg/challenges/challenges.go

@@ -187,23 +187,20 @@ func email(ctx context.Context, principal *oidc.IDToken) (identity.Principal, er
 }
 
 func spiffe(ctx context.Context, principal *oidc.IDToken) (identity.Principal, error) {
-
 	spiffeID := principal.Subject
 
 	cfg, ok := config.FromContext(ctx).GetIssuer(principal.Issuer)
 	if !ok {
 		return nil, errors.New("invalid configuration for OIDC ID Token issuer")
 	}
 
-	// The Spiffe ID must be a subdomain of the issuer (spiffe://foo.example.com -> example.com/...)
-	u, err := url.Parse(cfg.IssuerURL)
+	parsed, err := url.Parse(spiffeID)
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("failed to parse spiffeID: %w", err)
 	}
 
-	issuerHostname := u.Hostname()
-	if !isSpiffeIDAllowed(u.Hostname(), spiffeID) {
-		return nil, fmt.Errorf("%s is not allowed for %s", spiffeID, issuerHostname)
+	if parsed.Hostname() != cfg.SPIFFETrustDomain {
+		return nil, fmt.Errorf("spiffe id %s doesn't match configured trust domain %s", spiffeID, cfg.SPIFFETrustDomain)
 	}
 
 	issuer, err := oauthflow.IssuerFromIDToken(principal, cfg.IssuerClaim)
@@ -445,20 +442,6 @@ func workflowInfoFromIDToken(token *oidc.IDToken) (map[AdditionalInfo]string, er
 		GithubWorkflowRef:        claims.Ref}, nil
 }
 
-func isSpiffeIDAllowed(host, spiffeID string) bool {
-	u, err := url.Parse(spiffeID)
-	if err != nil {
-		return false
-	}
-	if u.Scheme != "spiffe" {
-		return false
-	}
-	if u.Hostname() == host {
-		return true
-	}
-	return strings.Contains(u.Hostname(), "."+host)
-}
-
 // isURISubjectAllowed compares the subject and issuer URIs,
 // returning an error if the scheme or the hostnames do not match
 func isURISubjectAllowed(subject, issuer *url.URL) error {

pkg/challenges/challenges_test.go

@@ -269,46 +269,6 @@ func factExtensionIs(oid asn1.ObjectIdentifier, value string) func(x509.Certific
 		return errors.New("extension not set")
 	}
 }
-func Test_isSpiffeIDAllowed(t *testing.T) {
-	tests := []struct {
-		name     string
-		host     string
-		spiffeID string
-		want     bool
-	}{{
-		name:     "match",
-		host:     "foobar.com",
-		spiffeID: "spiffe://foobar.com/stuff",
-		want:     true,
-	}, {
-		name:     "subdomain match",
-		host:     "foobar.com",
-		spiffeID: "spiffe://spife.foobar.com/stuff",
-		want:     true,
-	}, {
-		name:     "subdomain mismatch",
-		host:     "foo.foobar.com",
-		spiffeID: "spiffe://spife.foobar.com/stuff",
-		want:     false,
-	}, {
-		name:     "inverted mismatch",
-		host:     "foo.foobar.com",
-		spiffeID: "spiffe://foobar.com/stuff",
-		want:     false,
-	}, {
-		name:     "no dot mismatch",
-		host:     "foobar.com",
-		spiffeID: "spiffe://foofoobar.com/stuff",
-		want:     false,
-	}}
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			if got := isSpiffeIDAllowed(tt.host, tt.spiffeID); got != tt.want {
-				t.Errorf("isSpiffeIDAllowed() = %v, want %v", got, tt.want)
-			}
-		})
-	}
-}
 
 func TestURI(t *testing.T) {
 	cfg := &config.FulcioConfig{
@@ -544,6 +504,79 @@ func TestEmailWithClaims(t *testing.T) {
 	}
 }
 
+func TestSpiffe(t *testing.T) {
+	tests := map[string]struct {
+		Token   *oidc.IDToken
+		Config  *config.FulcioConfig
+		WantErr bool
+	}{
+		"good token": {
+			Token: &oidc.IDToken{
+				Subject: "spiffe://foo.com/bar",
+				Issuer:  "id.foo.com",
+			},
+			Config: &config.FulcioConfig{
+				OIDCIssuers: map[string]config.OIDCIssuer{
+					"id.foo.com": {
+						IssuerURL:         "id.foo.com",
+						ClientID:          "sigstore",
+						Type:              config.IssuerTypeSpiffe,
+						SPIFFETrustDomain: "foo.com",
+					},
+				},
+			},
+			WantErr: false,
+		},
+		"spiffe id wrong trust domain": {
+			Token: &oidc.IDToken{
+				Subject: "spiffe://baz.com/bar",
+				Issuer:  "id.foo.com",
+			},
+			Config: &config.FulcioConfig{
+				OIDCIssuers: map[string]config.OIDCIssuer{
+					"id.foo.com": {
+						IssuerURL:         "id.foo.com",
+						ClientID:          "sigstore",
+						Type:              config.IssuerTypeSpiffe,
+						SPIFFETrustDomain: "foo.com",
+					},
+				},
+			},
+			WantErr: true,
+		},
+		"spiffe id no issuer configured": {
+			Token: &oidc.IDToken{
+				Subject: "spiffe://foo.com/bar",
+				Issuer:  "id.foo.com",
+			},
+			Config: &config.FulcioConfig{
+				OIDCIssuers: map[string]config.OIDCIssuer{
+					"id.bar.com": {
+						IssuerURL:         "id.bar.com",
+						ClientID:          "sigstore",
+						Type:              config.IssuerTypeSpiffe,
+						SPIFFETrustDomain: "foo.com",
+					},
+				},
+			},
+			WantErr: true,
+		},
+	}
+
+	for name, test := range tests {
+		t.Run(name, func(t *testing.T) {
+			ctx := config.With(context.Background(), test.Config)
+			_, err := spiffe(ctx, test.Token)
+			if err != nil && !test.WantErr {
+				t.Error(err)
+			}
+			if err == nil && test.WantErr {
+				t.Error("expected error")
+			}
+		})
+	}
+}
+
 func Test_validateAllowedDomain(t *testing.T) {
 	tests := []struct {
 		name    string

pkg/config/config.go

@@ -19,6 +19,7 @@ import (
 	"context"
 	"crypto/x509"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"io/ioutil"
 	"net/http"
@@ -65,6 +66,10 @@ type OIDCIssuer struct {
 	// The domain that must be present in the subject for 'uri' issuer types
 	// Also used to create an email for 'username' issuer types
 	SubjectDomain string `json:"SubjectDomain,omitempty"`
+	// SPIFFETrustDomain specifies the trust domain that 'spiffe' issuer types
+	// issue ID tokens for. Tokens with a different trust domain will be
+	// rejected.
+	SPIFFETrustDomain string `json:"SPIFFETrustDomain,omitempty"`
 }
 
 func metaRegex(issuer string) (*regexp.Regexp, error) {
@@ -183,9 +188,35 @@ func parseConfig(b []byte) (cfg *FulcioConfig, err error) {
 	if err := json.Unmarshal(b, cfg); err != nil {
 		return nil, fmt.Errorf("unmarshal: %w", err)
 	}
+
 	return cfg, nil
 }
 
+func validateConfig(conf *FulcioConfig) error {
+	if conf == nil {
+		return errors.New("nil config")
+	}
+
+	for _, issuer := range conf.OIDCIssuers {
+		if issuer.Type == IssuerTypeSpiffe && issuer.SPIFFETrustDomain == "" {
+			return errors.New("spiffe issuer must have SPIFFETrustDomain set")
+		}
+		if issuer.Type == IssuerTypeURI && issuer.SubjectDomain == "" {
+			return errors.New("uri issuer must have SubjectDomain set")
+		}
+	}
+
+	for _, metaIssuer := range conf.MetaIssuers {
+		if metaIssuer.Type == IssuerTypeSpiffe {
+			// This would establish a many to one relationship for OIDC issuers
+			// to trust domains so we fail early and reject this configuration.
+			return errors.New("SPIFFE meta issuers not supported")
+		}
+	}
+
+	return nil
+}
+
 var DefaultConfig = &FulcioConfig{
 	OIDCIssuers: map[string]OIDCIssuer{
 		"https://oauth2.sigstore.dev/auth": {
@@ -248,6 +279,11 @@ func Read(b []byte) (*FulcioConfig, error) {
 		return nil, fmt.Errorf("parse: %w", err)
 	}
 
+	err = validateConfig(config)
+	if err != nil {
+		return nil, fmt.Errorf("validate: %w", err)
+	}
+
 	if _, ok := config.GetIssuer("https://kubernetes.default.svc"); ok {
 		// Add the Kubernetes cluster's CA to the system CA pool, and to
 		// the default transport.

pkg/config/config_test.go

@@ -86,3 +86,89 @@ func TestMetaURLs(t *testing.T) {
 		})
 	}
 }
+
+func TestValidateConfig(t *testing.T) {
+	tests := map[string]struct {
+		Config    *FulcioConfig
+		WantError bool
+	}{
+		"good spiffe config": {
+			Config: &FulcioConfig{
+				OIDCIssuers: map[string]OIDCIssuer{
+					"issuer.example.com": {
+						IssuerURL:         "issuer.example.com",
+						ClientID:          "foo",
+						Type:              IssuerTypeSpiffe,
+						SPIFFETrustDomain: "example.com",
+					},
+				},
+			},
+			WantError: false,
+		},
+		"spiffe issuer requires a trust domain": {
+			Config: &FulcioConfig{
+				OIDCIssuers: map[string]OIDCIssuer{
+					"issuer.example.com": {
+						IssuerURL: "issuer.example.com",
+						ClientID:  "foo",
+						Type:      IssuerTypeSpiffe,
+					},
+				},
+			},
+			WantError: true,
+		},
+		"spiffe issuer cannot be a meta issuer": {
+			Config: &FulcioConfig{
+				MetaIssuers: map[string]OIDCIssuer{
+					"*.example.com": {
+						ClientID:          "foo",
+						Type:              IssuerTypeSpiffe,
+						SPIFFETrustDomain: "example.com",
+					},
+				},
+			},
+			WantError: true,
+		},
+		"good uri config": {
+			Config: &FulcioConfig{
+				OIDCIssuers: map[string]OIDCIssuer{
+					"issuer.example.com": {
+						IssuerURL:     "issuer.example.com",
+						ClientID:      "foo",
+						Type:          IssuerTypeURI,
+						SubjectDomain: "other.example.com",
+					},
+				},
+			},
+			WantError: false,
+		},
+		"uri issuer requires a subject domain": {
+			Config: &FulcioConfig{
+				OIDCIssuers: map[string]OIDCIssuer{
+					"issuer.example.com": {
+						IssuerURL: "issuer.example.com",
+						ClientID:  "foo",
+						Type:      IssuerTypeURI,
+					},
+				},
+			},
+			WantError: true,
+		},
+		"nil config isn't valid": {
+			Config:    nil,
+			WantError: true,
+		},
+	}
+
+	for name, test := range tests {
+		t.Run(name, func(t *testing.T) {
+			err := validateConfig(test.Config)
+			if err != nil && !test.WantError {
+				t.Error(err)
+			}
+			if err == nil && test.WantError {
+				t.Error("expected error")
+			}
+		})
+	}
+}