From 9b23f22472ebba899ea4c3111cdeee3cebdbe478 Mon Sep 17 00:00:00 2001 From: Monis Khan Date: Tue, 28 Jan 2020 10:27:12 -0500 Subject: [PATCH] Make oidc authenticator audience agnostic This change removes the audience logic from the oidc authenticator and collapses it onto the same logic used by other audience unaware authenticators. oidc is audience unaware in the sense that it does not know or understand the API server's audience. As before, the authenticator will continue to check that the token audience matches the configured client ID. The reasoning for this simplification is: 1. The previous code tries to make the client ID on the oidc token a valid audience. But by not returning any audience, the token is not valid when used via token review on a server that is configured to honor audiences (the token works against the Kube API because the audience check is skipped). 2. It is unclear what functionality would be gained by allowing token review to check the client ID as a valid audience. It could serve as a proxy to know that the token was honored by the oidc authenticator, but that does not seem like a valid use case. 3. It has never been possible to use the client ID as an audience with token review as it would have always failed the audience intersection check. Thus this change is backwards compatible. It is strange that the oidc authenticator would be considered audience unaware when oidc tokens have an audience claim, but from the perspective of the Kube API (and for backwards compatibility), these tokens are only valid for the API server's audience. This change seems to be the least magical and most consistent way to honor backwards compatibility and to allow oidc tokens to be used via token review when audience support in enabled. Signed-off-by: Monis Khan --- pkg/kubeapiserver/authenticator/config.go | 3 +- .../plugin/pkg/authenticator/token/oidc/BUILD | 1 - .../pkg/authenticator/token/oidc/oidc.go | 15 -- .../pkg/authenticator/token/oidc/oidc_test.go | 138 +----------------- 4 files changed, 5 insertions(+), 152 deletions(-) diff --git a/pkg/kubeapiserver/authenticator/config.go b/pkg/kubeapiserver/authenticator/config.go index 0e27620d603..ad541fc8a11 100644 --- a/pkg/kubeapiserver/authenticator/config.go +++ b/pkg/kubeapiserver/authenticator/config.go @@ -165,7 +165,6 @@ func (config Config) New() (authenticator.Request, *spec.SecurityDefinitions, er oidcAuth, err := newAuthenticatorFromOIDCIssuerURL(oidc.Options{ IssuerURL: config.OIDCIssuerURL, ClientID: config.OIDCClientID, - APIAudiences: config.APIAudiences, CAFile: config.OIDCCAFile, UsernameClaim: config.OIDCUsernameClaim, UsernamePrefix: config.OIDCUsernamePrefix, @@ -177,7 +176,7 @@ func (config Config) New() (authenticator.Request, *spec.SecurityDefinitions, er if err != nil { return nil, nil, err } - tokenAuthenticators = append(tokenAuthenticators, oidcAuth) + tokenAuthenticators = append(tokenAuthenticators, authenticator.WrapAudienceAgnosticToken(config.APIAudiences, oidcAuth)) } if len(config.WebhookTokenAuthnConfigFile) > 0 { webhookTokenAuth, err := newWebhookTokenAuthenticator(config.WebhookTokenAuthnConfigFile, config.WebhookTokenAuthnVersion, config.WebhookTokenAuthnCacheTTL, config.APIAudiences) diff --git a/staging/src/k8s.io/apiserver/plugin/pkg/authenticator/token/oidc/BUILD b/staging/src/k8s.io/apiserver/plugin/pkg/authenticator/token/oidc/BUILD index df9f34382a9..e0cba0dd20e 100644 --- a/staging/src/k8s.io/apiserver/plugin/pkg/authenticator/token/oidc/BUILD +++ b/staging/src/k8s.io/apiserver/plugin/pkg/authenticator/token/oidc/BUILD @@ -13,7 +13,6 @@ go_test( data = glob(["testdata/**"]), embed = [":go_default_library"], deps = [ - "//staging/src/k8s.io/apiserver/pkg/authentication/authenticator:go_default_library", "//staging/src/k8s.io/apiserver/pkg/authentication/user:go_default_library", "//vendor/github.com/coreos/go-oidc:go_default_library", "//vendor/gopkg.in/square/go-jose.v2:go_default_library", 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 1888fa91d6d..8442e4256d1 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 @@ -78,12 +78,6 @@ type Options struct { // See: https://openid.net/specs/openid-connect-core-1_0.html#IDToken ClientID string - // APIAudiences are the audiences that the API server identitifes as. The - // (API audiences unioned with the ClientIDs) should have a non-empty - // intersection with the request's target audience. This preserves the - // behavior of the OIDC authenticator pre-introduction of API audiences. - APIAudiences authenticator.Audiences - // Path to a PEM encoded root certificate of the provider. CAFile string @@ -194,8 +188,6 @@ type Authenticator struct { groupsClaim string groupsPrefix string requiredClaims map[string]string - clientIDs authenticator.Audiences - apiAudiences authenticator.Audiences // Contains an *oidc.IDTokenVerifier. Do not access directly use the // idTokenVerifier method. @@ -325,8 +317,6 @@ func newAuthenticator(opts Options, initVerifier func(ctx context.Context, a *Au groupsClaim: opts.GroupsClaim, groupsPrefix: opts.GroupsPrefix, requiredClaims: opts.RequiredClaims, - clientIDs: authenticator.Audiences{opts.ClientID}, - apiAudiences: opts.APIAudiences, cancel: cancel, resolver: resolver, } @@ -542,11 +532,6 @@ func (r *claimResolver) resolve(endpoint endpoint, allClaims claims) error { } func (a *Authenticator) AuthenticateToken(ctx context.Context, token string) (*authenticator.Response, bool, error) { - if reqAuds, ok := authenticator.AudiencesFrom(ctx); ok { - if len(reqAuds.Intersect(a.clientIDs)) == 0 && len(reqAuds.Intersect(a.apiAudiences)) == 0 { - return nil, false, nil - } - } if !hasCorrectIssuer(a.issuerURL, token) { return nil, false, 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 3f5db497e4d..aeebc87566a 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 @@ -37,8 +37,6 @@ import ( oidc "github.com/coreos/go-oidc" jose "gopkg.in/square/go-jose.v2" - - "k8s.io/apiserver/pkg/authentication/authenticator" "k8s.io/apiserver/pkg/authentication/user" "k8s.io/klog" ) @@ -142,7 +140,6 @@ type claimsTest struct { wantInitErr bool claimToResponseMap map[string]string openIDConfig string - reqAudiences authenticator.Audiences } // Replace formats the contents of v into the provided template. @@ -299,12 +296,7 @@ func (c *claimsTest) run(t *testing.T) { t.Fatalf("serialize token: %v", err) } - ctx := context.Background() - if c.reqAudiences != nil { - ctx = authenticator.WithAudiences(ctx, c.reqAudiences) - } - - got, ok, err := a.AuthenticateToken(ctx, token) + got, ok, err := a.AuthenticateToken(context.Background(), token) if err != nil { if !c.wantErr { @@ -1397,11 +1389,10 @@ func TestToken(t *testing.T) { }, }, { - name: "good token with api req audience", + name: "good token with bad client id", options: Options{ IssuerURL: "https://auth.example.com", ClientID: "my-client", - APIAudiences: authenticator.Audiences{"api"}, UsernameClaim: "username", now: func() time.Time { return now }, }, @@ -1411,132 +1402,11 @@ func TestToken(t *testing.T) { }, claims: fmt.Sprintf(`{ "iss": "https://auth.example.com", - "aud": "my-client", + "aud": "my-wrong-client", "username": "jane", "exp": %d }`, valid.Unix()), - reqAudiences: authenticator.Audiences{"api"}, - want: &user.DefaultInfo{ - Name: "jane", - }, - }, - { - name: "good token with multiple api req audience", - options: Options{ - IssuerURL: "https://auth.example.com", - ClientID: "my-client", - APIAudiences: authenticator.Audiences{"api", "other"}, - UsernameClaim: "username", - now: func() time.Time { return now }, - }, - signingKey: loadRSAPrivKey(t, "testdata/rsa_1.pem", jose.RS256), - pubKeys: []*jose.JSONWebKey{ - loadRSAKey(t, "testdata/rsa_1.pem", jose.RS256), - }, - claims: fmt.Sprintf(`{ - "iss": "https://auth.example.com", - "aud": "my-client", - "username": "jane", - "exp": %d - }`, valid.Unix()), - reqAudiences: authenticator.Audiences{"api"}, - want: &user.DefaultInfo{ - Name: "jane", - }, - }, - { - name: "good token with client_id req audience", - options: Options{ - IssuerURL: "https://auth.example.com", - ClientID: "my-client", - APIAudiences: authenticator.Audiences{"api"}, - UsernameClaim: "username", - now: func() time.Time { return now }, - }, - signingKey: loadRSAPrivKey(t, "testdata/rsa_1.pem", jose.RS256), - pubKeys: []*jose.JSONWebKey{ - loadRSAKey(t, "testdata/rsa_1.pem", jose.RS256), - }, - claims: fmt.Sprintf(`{ - "iss": "https://auth.example.com", - "aud": "my-client", - "username": "jane", - "exp": %d - }`, valid.Unix()), - reqAudiences: authenticator.Audiences{"my-client"}, - want: &user.DefaultInfo{ - Name: "jane", - }, - }, - { - name: "good token with client_id and api req audience", - options: Options{ - IssuerURL: "https://auth.example.com", - ClientID: "my-client", - APIAudiences: authenticator.Audiences{"api"}, - UsernameClaim: "username", - now: func() time.Time { return now }, - }, - signingKey: loadRSAPrivKey(t, "testdata/rsa_1.pem", jose.RS256), - pubKeys: []*jose.JSONWebKey{ - loadRSAKey(t, "testdata/rsa_1.pem", jose.RS256), - }, - claims: fmt.Sprintf(`{ - "iss": "https://auth.example.com", - "aud": "my-client", - "username": "jane", - "exp": %d - }`, valid.Unix()), - reqAudiences: authenticator.Audiences{"my-client", "api"}, - want: &user.DefaultInfo{ - Name: "jane", - }, - }, - { - name: "good token with client_id and api req audience", - options: Options{ - IssuerURL: "https://auth.example.com", - ClientID: "my-client", - APIAudiences: authenticator.Audiences{"api"}, - UsernameClaim: "username", - now: func() time.Time { return now }, - }, - signingKey: loadRSAPrivKey(t, "testdata/rsa_1.pem", jose.RS256), - pubKeys: []*jose.JSONWebKey{ - loadRSAKey(t, "testdata/rsa_1.pem", jose.RS256), - }, - claims: fmt.Sprintf(`{ - "iss": "https://auth.example.com", - "aud": "my-client", - "username": "jane", - "exp": %d - }`, valid.Unix()), - reqAudiences: authenticator.Audiences{"my-client", "api"}, - want: &user.DefaultInfo{ - Name: "jane", - }, - }, - { - name: "good token with client_id and bad req audience", - options: Options{ - IssuerURL: "https://auth.example.com", - ClientID: "my-client", - APIAudiences: authenticator.Audiences{"api"}, - UsernameClaim: "username", - now: func() time.Time { return now }, - }, - signingKey: loadRSAPrivKey(t, "testdata/rsa_1.pem", jose.RS256), - pubKeys: []*jose.JSONWebKey{ - loadRSAKey(t, "testdata/rsa_1.pem", jose.RS256), - }, - claims: fmt.Sprintf(`{ - "iss": "https://auth.example.com", - "aud": "my-client", - "username": "jane", - "exp": %d - }`, valid.Unix()), - reqAudiences: authenticator.Audiences{"other"}, - wantSkip: true, + wantErr: true, }, } for _, test := range tests {