From 5d9a4d1be4f0019d4237f0b97c959c8be72ecaab Mon Sep 17 00:00:00 2001 From: Nathan Smith Date: Mon, 16 May 2022 12:00:31 -0700 Subject: [PATCH] spiffe: correct trust domain checking 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 --- pkg/api/grpc_server_test.go | 5 +- pkg/challenges/challenges.go | 25 ++----- pkg/challenges/challenges_test.go | 113 +++++++++++++++++++----------- pkg/config/config.go | 36 ++++++++++ pkg/config/config_test.go | 86 +++++++++++++++++++++++ 5 files changed, 202 insertions(+), 63 deletions(-) diff --git a/pkg/api/grpc_server_test.go b/pkg/api/grpc_server_test.go index 2a9d184dc..fd4c05125 100644 --- a/pkg/api/grpc_server_test.go +++ b/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{ diff --git a/pkg/challenges/challenges.go b/pkg/challenges/challenges.go index ad3797c24..bb6392062 100644 --- a/pkg/challenges/challenges.go +++ b/pkg/challenges/challenges.go @@ -187,7 +187,6 @@ 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) @@ -195,15 +194,13 @@ func spiffe(ctx context.Context, principal *oidc.IDToken) (identity.Principal, e 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 { diff --git a/pkg/challenges/challenges_test.go b/pkg/challenges/challenges_test.go index 4072b273c..aa1f72c5a 100644 --- a/pkg/challenges/challenges_test.go +++ b/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 diff --git a/pkg/config/config.go b/pkg/config/config.go index 4aa15f77e..5617f0903 100644 --- a/pkg/config/config.go +++ b/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. diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go index ce7db6764..6f4fd3913 100644 --- a/pkg/config/config_test.go +++ b/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") + } + }) + } +}