Update Username OIDC flow based on comments (#463)

* Update Username OIDC flow based on comments Signed-off-by: Hayden Blauzvern <hblauzvern@google.com> * Add additional test Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>
sigstore · Mar 19, 2022 · 374ebab · 374ebab
1 parent b2186c0
commit 374ebab
Show file tree

Hide file tree

Showing 3 changed files with 62 additions and 23 deletions.
diff --git 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
@@ -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
@@ -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)
 			}
 		})
 	}