Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

oidc auth: make the OIDC claims prefix configurable #50875

Merged
merged 1 commit into from
Sep 3, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
33 changes: 26 additions & 7 deletions pkg/kubeapiserver/authenticator/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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
Expand Down
22 changes: 17 additions & 5 deletions pkg/kubeapiserver/options/authentication.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -213,10 +215,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.")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we just less concerned about collisions for the group claim vs the username claim?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not really, the prefixing wasn't done when group support was added, and this is preserving compatibility with existing behavior


}

if s.PasswordFile != nil {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,19 +71,29 @@ 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 {
issuerURL string

trustedClientID string

usernameClaim string
groupsClaim string
usernameClaim string
usernamePrefix string
groupsClaim string
groupsPrefix string

httpClient *http.Client

Expand Down Expand Up @@ -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},
}

Expand Down Expand Up @@ -221,23 +233,25 @@ 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
}
if !ok {
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")
Expand All @@ -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.
Expand All @@ -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
}