diff --git a/pkg/kubeapiserver/authenticator/config.go b/pkg/kubeapiserver/authenticator/config.go index 72261cfdac4..b1212ce67a5 100644 --- a/pkg/kubeapiserver/authenticator/config.go +++ b/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, - ClientID: clientID, - CAFile: caFile, - UsernameClaim: usernameClaim, - GroupsClaim: groupsClaim, + IssuerURL: issuerURL, + ClientID: clientID, + CAFile: caFile, + UsernameClaim: usernameClaim, + UsernamePrefix: usernamePrefix, + GroupsClaim: groupsClaim, + GroupsPrefix: groupsPrefix, }) if err != nil { return nil, err diff --git a/pkg/kubeapiserver/options/authentication.go b/pkg/kubeapiserver/options/authentication.go index 6bdc013f456..3acf11d9767 100644 --- a/pkg/kubeapiserver/options/authentication.go +++ b/pkg/kubeapiserver/options/authentication.go @@ -60,11 +60,13 @@ type KeystoneAuthenticationOptions struct { } type OIDCAuthenticationOptions struct { - CAFile string - ClientID string - IssuerURL string - UsernameClaim string - GroupsClaim string + CAFile string + ClientID string + IssuerURL string + UsernameClaim 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 { diff --git a/staging/src/k8s.io/apiserver/plugin/pkg/authenticator/token/oidc/oidc.go b/staging/src/k8s.io/apiserver/plugin/pkg/authenticator/token/oidc/oidc.go index 1b6e8f31c3e..a8c95ecb8b0 100644 --- a/staging/src/k8s.io/apiserver/plugin/pkg/authenticator/token/oidc/oidc.go +++ b/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 - groupsClaim string + usernameClaim 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 - switch a.usernameClaim { - case "email": + if a.usernameClaim == "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. - username = fmt.Sprintf("%s#%s", a.issuerURL, claim) + } + + if a.usernamePrefix != "" { + 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 } diff --git a/staging/src/k8s.io/apiserver/plugin/pkg/authenticator/token/oidc/oidc_test.go b/staging/src/k8s.io/apiserver/plugin/pkg/authenticator/token/oidc/oidc_test.go index 1ae3c6d57d5..f94d91b1a3f 100644 --- a/staging/src/k8s.io/apiserver/plugin/pkg/authenticator/token/oidc/oidc_test.go +++ b/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) + } + }) + } +}