Merge pull request #50875 from ericchiang/oidc-claims-prefix

Automatic merge from submit-queue (batch tested with PRs 50579, 50875, 51797, 51807, 51803)

oidc auth: make the OIDC claims prefix configurable

Add the following flags to control the prefixing of usernames and
groups authenticated using OpenID Connect tokens.

	--oidc-username-prefix
	--oidc-groups-prefix

```release-note
The OpenID Connect authenticator can now use a custom prefix, or omit the default prefix, for username and groups claims through the --oidc-username-prefix and --oidc-groups-prefix flags. For example, the authenticator can map a user with the username "jane" to "google:jane" by supplying the "google:" username prefix.
```

Closes https://github.com/kubernetes/kubernetes/issues/50408
Ref https://github.com/kubernetes/kubernetes/issues/31380

cc @grillz @kubernetes/sig-auth-pr-reviews @thomastaylor312 @gtaylor

pkg/kubeapiserver/authenticator/config.go

@@ -56,7 +56,9 @@ type AuthenticatorConfig struct {
 	OIDCClientID                string
 	OIDCCAFile                  string
 	OIDCUsernameClaim           string
+	OIDCUsernamePrefix          string
 	OIDCGroupsClaim             string
+	OIDCGroupsPrefix            string
 	ServiceAccountKeyFiles      []string
 	ServiceAccountLookup        bool
 	KeystoneURL                 string
@@ -152,7 +154,7 @@ func (config AuthenticatorConfig) New() (authenticator.Request, *spec.SecurityDe
 	// simply returns an error, the OpenID Connect plugin may query the provider to
 	// update the keys, causing performance hits.
 	if len(config.OIDCIssuerURL) > 0 && len(config.OIDCClientID) > 0 {
-		oidcAuth, err := newAuthenticatorFromOIDCIssuerURL(config.OIDCIssuerURL, config.OIDCClientID, config.OIDCCAFile, config.OIDCUsernameClaim, config.OIDCGroupsClaim)
+		oidcAuth, err := newAuthenticatorFromOIDCIssuerURL(config.OIDCIssuerURL, config.OIDCClientID, config.OIDCCAFile, config.OIDCUsernameClaim, config.OIDCUsernamePrefix, config.OIDCGroupsClaim, config.OIDCGroupsPrefix)
 		if err != nil {
 			return nil, nil, err
 		}
@@ -244,13 +246,30 @@ func newAuthenticatorFromTokenFile(tokenAuthFile string) (authenticator.Token, e
 }
 
 // newAuthenticatorFromOIDCIssuerURL returns an authenticator.Request or an error.
-func newAuthenticatorFromOIDCIssuerURL(issuerURL, clientID, caFile, usernameClaim, groupsClaim string) (authenticator.Token, error) {
+func newAuthenticatorFromOIDCIssuerURL(issuerURL, clientID, caFile, usernameClaim, usernamePrefix, groupsClaim, groupsPrefix string) (authenticator.Token, error) {
+	const noUsernamePrefix = "-"
+
+	if usernamePrefix == "" && usernameClaim != "email" {
+		// Old behavior. If a usernamePrefix isn't provided, prefix all claims other than "email"
+		// with the issuerURL.
+		//
+		// See https://github.com/kubernetes/kubernetes/issues/31380
+		usernamePrefix = issuerURL + "#"
+	}
+
+	if usernamePrefix == noUsernamePrefix {
+		// Special value indicating usernames shouldn't be prefixed.
+		usernamePrefix = ""
+	}
+
 	tokenAuthenticator, err := oidc.New(oidc.OIDCOptions{
-		IssuerURL:     issuerURL,
+		IssuerURL:      issuerURL,
-		ClientID:      clientID,
+		ClientID:       clientID,
-		CAFile:        caFile,
+		CAFile:         caFile,
-		UsernameClaim: usernameClaim,
+		UsernameClaim:  usernameClaim,
-		GroupsClaim:   groupsClaim,
+		UsernamePrefix: usernamePrefix,
+		GroupsClaim:    groupsClaim,
+		GroupsPrefix:   groupsPrefix,
 	})
 	if err != nil {
 		return nil, err

pkg/kubeapiserver/options/authentication.go

@@ -60,11 +60,13 @@ type KeystoneAuthenticationOptions struct {
 }
 
 type OIDCAuthenticationOptions struct {
-	CAFile        string
+	CAFile         string
-	ClientID      string
+	ClientID       string
-	IssuerURL     string
+	IssuerURL      string
-	UsernameClaim string
+	UsernameClaim  string
-	GroupsClaim   string
+	UsernamePrefix string
+	GroupsClaim    string
+	GroupsPrefix   string
 }
 
 type PasswordFileAuthenticationOptions struct {
@@ -217,10 +219,20 @@ func (s *BuiltInAuthenticationOptions) AddFlags(fs *pflag.FlagSet) {
 			"is not guaranteed to be unique and immutable. This flag is experimental, please see "+
 			"the authentication documentation for further details.")
 
+		fs.StringVar(&s.OIDC.UsernamePrefix, "oidc-username-prefix", "", ""+
+			"If provided, all usernames will be prefixed with this value. If not provided, "+
+			"username claims other than 'email' are prefixed by the issuer URL to avoid "+
+			"clashes. To skip any prefixing, provide the value '-'.")
+
 		fs.StringVar(&s.OIDC.GroupsClaim, "oidc-groups-claim", "", ""+
 			"If provided, the name of a custom OpenID Connect claim for specifying user groups. "+
 			"The claim value is expected to be a string or array of strings. This flag is experimental, "+
 			"please see the authentication documentation for further details.")
+
+		fs.StringVar(&s.OIDC.GroupsPrefix, "oidc-groups-prefix", "", ""+
+			"If provided, all groups will be prefixed with this value to prevent conflicts with "+
+			"other authentication strategies.")
+
 	}
 
 	if s.PasswordFile != nil {

staging/src/k8s.io/apiserver/plugin/pkg/authenticator/token/oidc/oidc.go

@@ -71,10 +71,18 @@ type OIDCOptions struct {
 	// UsernameClaim is the JWT field to use as the user's username.
 	UsernameClaim string
 
+	// UsernamePrefix, if specified, causes claims mapping to username to be prefix with
+	// the provided value. A value "oidc:" would result in usernames like "oidc:john".
+	UsernamePrefix string
+
 	// GroupsClaim, if specified, causes the OIDCAuthenticator to try to populate the user's
 	// groups with an ID Token field. If the GrouppClaim field is present in an ID Token the value
 	// must be a string or list of strings.
 	GroupsClaim string
+
+	// GroupsPrefix, if specified, causes claims mapping to group names to be prefixed with the
+	// value. A value "oidc:" would result in groups like "oidc:engineering" and "oidc:marketing".
+	GroupsPrefix string
 }
 
 type OIDCAuthenticator struct {
@@ -82,8 +90,10 @@ type OIDCAuthenticator struct {
 
 	trustedClientID string
 
-	usernameClaim string
+	usernameClaim  string
-	groupsClaim   string
+	usernamePrefix string
+	groupsClaim    string
+	groupsPrefix   string
 
 	httpClient *http.Client
 
@@ -131,7 +141,9 @@ func New(opts OIDCOptions) (*OIDCAuthenticator, error) {
 		issuerURL:       opts.IssuerURL,
 		trustedClientID: opts.ClientID,
 		usernameClaim:   opts.UsernameClaim,
+		usernamePrefix:  opts.UsernamePrefix,
 		groupsClaim:     opts.GroupsClaim,
+		groupsPrefix:    opts.GroupsPrefix,
 		httpClient:      &http.Client{Transport: tr},
 	}
 
@@ -221,13 +233,17 @@ func (a *OIDCAuthenticator) AuthenticateToken(value string) (user.Info, bool, er
 	if err := client.VerifyJWT(jwt); err != nil {
 		return nil, false, err
 	}
-
 	claims, err := jwt.Claims()
 	if err != nil {
 		return nil, false, err
 	}
+	return a.parseTokenClaims(claims)
+}
 
-	claim, ok, err := claims.StringClaim(a.usernameClaim)
+// parseTokenClaims maps a set of claims to a user. It performs basic validation such as
+// ensuring the email is verified.
+func (a *OIDCAuthenticator) parseTokenClaims(claims jose.Claims) (user.Info, bool, error) {
+	username, ok, err := claims.StringClaim(a.usernameClaim)
 	if err != nil {
 		return nil, false, err
 	}
@@ -235,9 +251,7 @@ func (a *OIDCAuthenticator) AuthenticateToken(value string) (user.Info, bool, er
 		return nil, false, fmt.Errorf("cannot find %q in JWT claims", a.usernameClaim)
 	}
 
-	var username string
+	if a.usernameClaim == "email" {
-	switch a.usernameClaim {
-	case "email":
 		verified, ok := claims["email_verified"]
 		if !ok {
 			return nil, false, errors.New("'email_verified' claim not present")
@@ -255,10 +269,10 @@ func (a *OIDCAuthenticator) AuthenticateToken(value string) (user.Info, bool, er
 		if !emailVerified {
 			return nil, false, errors.New("email not verified")
 		}
-		username = claim
+	}
-	default:
+
-		// For all other cases, use issuerURL + claim as the user name.
+	if a.usernamePrefix != "" {
-		username = fmt.Sprintf("%s#%s", a.issuerURL, claim)
+		username = a.usernamePrefix + username
 	}
 
 	// TODO(yifan): Add UID, also populate the issuer to upper layer.
@@ -278,5 +292,12 @@ func (a *OIDCAuthenticator) AuthenticateToken(value string) (user.Info, bool, er
 			info.Groups = groups
 		}
 	}
+
+	if a.groupsPrefix != "" {
+		for i, group := range info.Groups {
+			info.Groups[i] = a.groupsPrefix + group
+		}
+	}
+
 	return info, true, nil
 }

staging/src/k8s.io/apiserver/plugin/pkg/authenticator/token/oidc/oidc_test.go

@@ -17,7 +17,6 @@ limitations under the License.
 package oidc
 
 import (
-	"fmt"
 	"os"
 	"path"
 	"reflect"
@@ -213,7 +212,7 @@ func TestOIDCAuthentication(t *testing.T) {
 				"sub",
 				"",
 				generateGoodToken(t, op, srv.URL, "client-foo", "client-foo", "sub", "user-foo", "", nil),
-				&user.DefaultInfo{Name: fmt.Sprintf("%s#%s", srv.URL, "user-foo")},
+				&user.DefaultInfo{Name: "user-foo"},
 				true,
 				"",
 			},
@@ -308,7 +307,7 @@ func TestOIDCAuthentication(t *testing.T) {
 		}
 
 		for i, tt := range tests {
-			client, err := New(OIDCOptions{srv.URL, "client-foo", cert, tt.userClaim, tt.groupsClaim})
+			client, err := New(OIDCOptions{srv.URL, "client-foo", cert, tt.userClaim, "", tt.groupsClaim, ""})
 			if err != nil {
 				t.Errorf("Unexpected error: %v", err)
 				continue
@@ -334,3 +333,203 @@ func TestOIDCAuthentication(t *testing.T) {
 		}
 	}
 }
+
+func TestParseTokenClaims(t *testing.T) {
+	tests := []struct {
+		name string
+
+		// Note this is missing a lot of configuration options because
+		// parseTokenClaim doesn't handle:
+		//
+		// - 'iss' claim matching issuer URL
+		// - 'exp' claim having not expired
+		// - 'sub' claim matching a trusted client id
+		//
+		// That logic has coverage in other tests.
+
+		issuerURL      string
+		usernameClaim  string
+		usernamePrefix string
+		groupsClaim    string
+		groupsPrefix   string
+
+		claims jose.Claims
+
+		wantUser *user.DefaultInfo
+		wantErr  bool
+	}{
+		{
+			name:          "email username",
+			issuerURL:     "https://foo.com/",
+			usernameClaim: "email",
+			claims: jose.Claims{
+				"email":          "jane.doe@example.com",
+				"email_verified": true,
+			},
+			wantUser: &user.DefaultInfo{
+				Name: "jane.doe@example.com",
+			},
+		},
+		{
+			name:          "no email_verified claim",
+			issuerURL:     "https://foo.com/",
+			usernameClaim: "email",
+			claims: jose.Claims{
+				"email": "jane.doe@example.com",
+			},
+			wantErr: true,
+		},
+		{
+			name:          "email unverified",
+			issuerURL:     "https://foo.com/",
+			usernameClaim: "email",
+			claims: jose.Claims{
+				"email":          "jane.doe@example.com",
+				"email_verified": false,
+			},
+			wantErr: true,
+		},
+		{
+			name:          "non-email user claim",
+			issuerURL:     "https://foo.com/",
+			usernameClaim: "name",
+			claims: jose.Claims{
+				"name": "janedoe",
+			},
+			wantUser: &user.DefaultInfo{
+				Name: "janedoe",
+			},
+		},
+		{
+			name:          "groups claim",
+			issuerURL:     "https://foo.com/",
+			usernameClaim: "name",
+			groupsClaim:   "groups",
+			claims: jose.Claims{
+				"name":   "janedoe",
+				"groups": []string{"foo", "bar"},
+			},
+			wantUser: &user.DefaultInfo{
+				Name:   "janedoe",
+				Groups: []string{"foo", "bar"},
+			},
+		},
+		{
+			name:          "groups claim string",
+			issuerURL:     "https://foo.com/",
+			usernameClaim: "name",
+			groupsClaim:   "groups",
+			claims: jose.Claims{
+				"name":   "janedoe",
+				"groups": "foo",
+			},
+			wantUser: &user.DefaultInfo{
+				Name:   "janedoe",
+				Groups: []string{"foo"},
+			},
+		},
+		{
+			name:           "username prefix",
+			issuerURL:      "https://foo.com/",
+			usernameClaim:  "name",
+			groupsClaim:    "groups",
+			usernamePrefix: "oidc:",
+			claims: jose.Claims{
+				"name":   "janedoe",
+				"groups": []string{"foo", "bar"},
+			},
+			wantUser: &user.DefaultInfo{
+				Name:   "oidc:janedoe",
+				Groups: []string{"foo", "bar"},
+			},
+		},
+		{
+			name:           "username prefix with email",
+			issuerURL:      "https://foo.com/",
+			usernameClaim:  "email",
+			groupsClaim:    "groups",
+			usernamePrefix: "oidc:",
+			claims: jose.Claims{
+				"email":          "jane.doe@example.com",
+				"email_verified": true,
+				"groups":         []string{"foo", "bar"},
+			},
+			wantUser: &user.DefaultInfo{
+				Name:   "oidc:jane.doe@example.com",
+				Groups: []string{"foo", "bar"},
+			},
+		},
+		{
+			name:          "groups prefix",
+			issuerURL:     "https://foo.com/",
+			usernameClaim: "name",
+			groupsClaim:   "groups",
+			groupsPrefix:  "oidc:",
+			claims: jose.Claims{
+				"name":   "janedoe",
+				"groups": []string{"foo", "bar"},
+			},
+			wantUser: &user.DefaultInfo{
+				Name:   "janedoe",
+				Groups: []string{"oidc:foo", "oidc:bar"},
+			},
+		},
+		{
+			name:           "username and groups prefix",
+			issuerURL:      "https://foo.com/",
+			usernameClaim:  "name",
+			groupsClaim:    "groups",
+			usernamePrefix: "oidc-user:",
+			groupsPrefix:   "oidc:",
+			claims: jose.Claims{
+				"name":   "janedoe",
+				"groups": []string{"foo", "bar"},
+			},
+			wantUser: &user.DefaultInfo{
+				Name:   "oidc-user:janedoe",
+				Groups: []string{"oidc:foo", "oidc:bar"},
+			},
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			o := OIDCAuthenticator{
+				issuerURL:      test.issuerURL,
+				usernameClaim:  test.usernameClaim,
+				usernamePrefix: test.usernamePrefix,
+				groupsClaim:    test.groupsClaim,
+				groupsPrefix:   test.groupsPrefix,
+			}
+
+			u, ok, err := o.parseTokenClaims(test.claims)
+			if err != nil {
+				if !test.wantErr {
+					t.Errorf("failed to authenticate user: %v", err)
+				}
+				return
+			}
+			if test.wantErr {
+				t.Fatalf("expected authentication to fail")
+			}
+
+			if !ok {
+				// We don't have any cases today when the claims can return
+				// no error with an unauthenticated signal.
+				//
+				// In the future we might.
+				t.Fatalf("user wasn't authenticated")
+			}
+
+			got := &user.DefaultInfo{
+				Name:   u.GetName(),
+				UID:    u.GetUID(),
+				Groups: u.GetGroups(),
+				Extra:  u.GetExtra(),
+			}
+			if !reflect.DeepEqual(got, test.wantUser) {
+				t.Errorf("wanted user=%#v, got=%#v", test.wantUser, got)
+			}
+		})
+	}
+}