From 374ebab70bdfb3290d8a08aeeb2f3b2c5dac5d29 Mon Sep 17 00:00:00 2001 From: Hayden B Date: Fri, 18 Mar 2022 17:48:27 -0700 Subject: [PATCH] Update Username OIDC flow based on comments (#463) * Update Username OIDC flow based on comments Signed-off-by: Hayden Blauzvern * Add additional test Signed-off-by: Hayden Blauzvern --- pkg/api/api_test.go | 12 ++++--- pkg/challenges/challenges.go | 18 ++++++---- pkg/challenges/challenges_test.go | 55 ++++++++++++++++++++++++------- 3 files changed, 62 insertions(+), 23 deletions(-) diff --git a/pkg/api/api_test.go b/pkg/api/api_test.go index 5ed7ceda6..e22f3c7c4 100644 --- a/pkg/api/api_test.go +++ b/pkg/api/api_test.go @@ -160,13 +160,15 @@ func TestAPIWithEmail(t *testing.T) { usernameSubject := "foo" expectedUsernamedSubject := fmt.Sprintf("%s@%s", usernameSubject, issuerDomain.Hostname()) - for _, c := range []oidcTestContainer{ + tests := []oidcTestContainer{ { Signer: emailSigner, Issuer: emailIssuer, Subject: emailSubject, ExpectedSubject: emailSubject, }, { Signer: usernameSigner, Issuer: usernameIssuer, Subject: usernameSubject, ExpectedSubject: expectedUsernamedSubject, - }} { + }, + } + for _, c := range tests { // Create an OIDC token using this issuer's signer. tok, err := jwt.Signed(c.Signer).Claims(jwt.Claims{ Issuer: c.Issuer, @@ -241,13 +243,15 @@ func TestAPIWithUriSubject(t *testing.T) { spiffeSubject := strings.ReplaceAll(spiffeIssuer+"/foo/bar", "http", "spiffe") uriSubject := uriIssuer + "/users/1" - for _, c := range []oidcTestContainer{ + tests := []oidcTestContainer{ { Signer: spiffeSigner, Issuer: spiffeIssuer, Subject: spiffeSubject, }, { Signer: uriSigner, Issuer: uriIssuer, Subject: uriSubject, - }} { + }, + } + for _, c := range tests { // Create an OIDC token using this issuer's signer. tok, err := jwt.Signed(c.Signer).Claims(jwt.Claims{ Issuer: c.Issuer, diff --git a/pkg/challenges/challenges.go b/pkg/challenges/challenges.go index d3a0a4e3b..5ea52b2db 100644 --- a/pkg/challenges/challenges.go +++ b/pkg/challenges/challenges.go @@ -277,6 +277,10 @@ func URI(ctx context.Context, principal *oidc.IDToken, pubKey crypto.PublicKey, func Username(ctx context.Context, principal *oidc.IDToken, pubKey crypto.PublicKey, challenge []byte) (*ChallengeResult, error) { username := principal.Subject + if strings.Contains(username, "@") { + return nil, errors.New("username cannot contain @ character") + } + cfg, ok := config.FromContext(ctx).GetIssuer(principal.Issuer) if !ok { return nil, errors.New("invalid configuration for OIDC ID Token issuer") @@ -293,7 +297,7 @@ func Username(ctx context.Context, principal *oidc.IDToken, pubKey crypto.Public if err != nil { return nil, err } - if err := isDomainAllowed(cfg.SubjectDomain, uIssuer.Hostname()); err != nil { + if err := validateAllowedDomain(cfg.SubjectDomain, uIssuer.Hostname()); err != nil { return nil, err } @@ -412,12 +416,12 @@ func isURISubjectAllowed(subject, issuer *url.URL) error { return fmt.Errorf("subject (%s) and issuer (%s) URI schemes do not match", subject.Scheme, issuer.Scheme) } - return isDomainAllowed(subject.Hostname(), issuer.Hostname()) + return validateAllowedDomain(subject.Hostname(), issuer.Hostname()) } -// isDomainAllowed compares two hostnames, returning an error if the +// validateAllowedDomain compares two hostnames, returning an error if the // top-level and second-level domains do not match -func isDomainAllowed(subjectHostname, issuerHostname string) error { +func validateAllowedDomain(subjectHostname, issuerHostname string) error { // If the hostnames exactly match, return early if subjectHostname == issuerHostname { return nil @@ -427,14 +431,14 @@ func isDomainAllowed(subjectHostname, issuerHostname string) error { sHostname := strings.Split(subjectHostname, ".") iHostname := strings.Split(issuerHostname, ".") if len(sHostname) < minimumHostnameLength { - return fmt.Errorf("subject URI hostname too short: %s", subjectHostname) + return fmt.Errorf("URI hostname too short: %s", subjectHostname) } if len(iHostname) < minimumHostnameLength { - return fmt.Errorf("issuer URI hostname too short: %s", issuerHostname) + return fmt.Errorf("URI hostname too short: %s", issuerHostname) } if sHostname[len(sHostname)-1] == iHostname[len(iHostname)-1] && sHostname[len(sHostname)-2] == iHostname[len(iHostname)-2] { return nil } - return fmt.Errorf("subject and issuer hostnames do not match: %s, %s", subjectHostname, issuerHostname) + return fmt.Errorf("hostname top-level and second-level domains do not match: %s, %s", subjectHostname, issuerHostname) } diff --git a/pkg/challenges/challenges_test.go b/pkg/challenges/challenges_test.go index 244fcea62..6c617dc65 100644 --- a/pkg/challenges/challenges_test.go +++ b/pkg/challenges/challenges_test.go @@ -147,6 +147,32 @@ func TestUsername(t *testing.T) { } } +func TestUsernameInvalidChar(t *testing.T) { + cfg := &config.FulcioConfig{ + OIDCIssuers: map[string]config.OIDCIssuer{ + "https://accounts.example.com": { + IssuerURL: "https://accounts.example.com", + ClientID: "sigstore", + SubjectDomain: "example.com", + Type: config.IssuerTypeUsername, + }, + }, + } + ctx := config.With(context.Background(), cfg) + username := "foobar@example.com" + issuer := "https://accounts.example.com" + token := &oidc.IDToken{Subject: username, Issuer: issuer} + + _, err := Username(ctx, token, nil, []byte{}) + if err == nil { + t.Errorf("expected test failure, got no error") + } + msg := "username cannot contain @ character" + if err.Error() != msg { + t.Errorf("unexpected test failure message, got %s, expected %s", err.Error(), msg) + } +} + func Test_isURISubjectAllowed(t *testing.T) { tests := []struct { name string @@ -182,22 +208,22 @@ func Test_isURISubjectAllowed(t *testing.T) { name: "subject domain too short", subject: "https://example", issuer: "https://example.com", - want: fmt.Errorf("subject URI hostname too short: example"), + want: fmt.Errorf("URI hostname too short: example"), }, { name: "issuer domain too short", subject: "https://example.com", issuer: "https://issuer", - want: fmt.Errorf("issuer URI hostname too short: issuer"), + want: fmt.Errorf("URI hostname too short: issuer"), }, { name: "domain mismatch", subject: "https://example.com", issuer: "https://otherexample.com", - want: fmt.Errorf("subject and issuer hostnames do not match: example.com, otherexample.com"), + want: fmt.Errorf("hostname top-level and second-level domains do not match: example.com, otherexample.com"), }, { name: "top level domain mismatch", subject: "https://example.com", issuer: "https://example.org", - want: fmt.Errorf("subject and issuer hostnames do not match: example.com, example.org"), + want: fmt.Errorf("hostname top-level and second-level domains do not match: example.com, example.org"), }} for _, tt := range tests { subject, _ := url.Parse(tt.subject) @@ -215,7 +241,7 @@ func Test_isURISubjectAllowed(t *testing.T) { } } -func Test_isDomainAllowed(t *testing.T) { +func Test_validateAllowedDomain(t *testing.T) { tests := []struct { name string subject string // Parsed to url.URL @@ -245,32 +271,37 @@ func Test_isDomainAllowed(t *testing.T) { name: "subject domain too short", subject: "example", issuer: "example.com", - want: fmt.Errorf("subject URI hostname too short: example"), + want: fmt.Errorf("URI hostname too short: example"), }, { name: "issuer domain too short", subject: "example.com", issuer: "issuer", - want: fmt.Errorf("issuer URI hostname too short: issuer"), + want: fmt.Errorf("URI hostname too short: issuer"), }, { name: "domain mismatch", subject: "example.com", issuer: "otherexample.com", - want: fmt.Errorf("subject and issuer hostnames do not match: example.com, otherexample.com"), + want: fmt.Errorf("hostname top-level and second-level domains do not match: example.com, otherexample.com"), + }, { + name: "domain mismatch, subdomain match", + subject: "test.example.com", + issuer: "test.otherexample.com", + want: fmt.Errorf("hostname top-level and second-level domains do not match: test.example.com, test.otherexample.com"), }, { name: "top level domain mismatch", subject: "example.com", issuer: "example.org", - want: fmt.Errorf("subject and issuer hostnames do not match: example.com, example.org"), + want: fmt.Errorf("hostname top-level and second-level domains do not match: example.com, example.org"), }} for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got := isDomainAllowed(tt.subject, tt.issuer) + got := validateAllowedDomain(tt.subject, tt.issuer) if got == nil && tt.want != nil || got != nil && tt.want == nil { - t.Errorf("isURISubjectAllowed() = %v, want %v", got, tt.want) + t.Errorf("validateAllowedDomain() = %v, want %v", got, tt.want) } if got != nil && tt.want != nil && got.Error() != tt.want.Error() { - t.Errorf("isURISubjectAllowed() = %v, want %v", got, tt.want) + t.Errorf("validateAllowedDomain() = %v, want %v", got, tt.want) } }) }