From bc3dc12203624a18c18abd3ceb279ebf053751c3 Mon Sep 17 00:00:00 2001 From: Eric Chiang Date: Mon, 1 Aug 2016 10:14:20 -0700 Subject: [PATCH] oidc authentication plugin: don't trim issuer URLs with trailing slashes The issuer URL passed to the plugin must identically match the issuer URL returned by OpenID Connect discovery. However, the plugin currently trims all trailing slashes from issuer URLs, causing a mismatch. Since the go-oidc client already handles this case correctly, don't trim the path. --- .../pkg/auth/authenticator/token/oidc/oidc.go | 3 +- .../authenticator/token/oidc/oidc_test.go | 220 +++++++++--------- .../token/oidc/testing/provider.go | 54 +++-- plugin/pkg/client/auth/oidc/oidc_test.go | 3 +- 4 files changed, 141 insertions(+), 139 deletions(-) diff --git a/plugin/pkg/auth/authenticator/token/oidc/oidc.go b/plugin/pkg/auth/authenticator/token/oidc/oidc.go index 4a0e191ca9b..4a6728f86a2 100644 --- a/plugin/pkg/auth/authenticator/token/oidc/oidc.go +++ b/plugin/pkg/auth/authenticator/token/oidc/oidc.go @@ -33,7 +33,6 @@ import ( "fmt" "net/http" "net/url" - "strings" "sync" "sync/atomic" @@ -174,7 +173,7 @@ func (a *OIDCAuthenticator) client() (*oidc.Client, error) { } // Try to initialize client. - providerConfig, err := oidc.FetchProviderConfig(a.httpClient, strings.TrimSuffix(a.issuerURL, "/")) + providerConfig, err := oidc.FetchProviderConfig(a.httpClient, a.issuerURL) if err != nil { glog.Errorf("oidc authenticator: failed to fetch provider discovery data: %v", err) return nil, fmt.Errorf("fetch provider config: %v", err) diff --git a/plugin/pkg/auth/authenticator/token/oidc/oidc_test.go b/plugin/pkg/auth/authenticator/token/oidc/oidc_test.go index 4956e5da338..9195d2420f1 100644 --- a/plugin/pkg/auth/authenticator/token/oidc/oidc_test.go +++ b/plugin/pkg/auth/authenticator/token/oidc/oidc_test.go @@ -110,14 +110,13 @@ func TestTLSConfig(t *testing.T) { for _, tc := range tests { func() { - op := oidctesting.NewOIDCProvider(t) + op := oidctesting.NewOIDCProvider(t, "") srv, err := op.ServeTLSWithKeyPair(tc.serverCertFile, tc.serverKeyFile) if err != nil { t.Errorf("%s: %v", tc.testCase, err) return } defer srv.Close() - op.AddMinimalProviderConfig(srv) issuer := srv.URL clientID := "client-foo" @@ -178,8 +177,6 @@ func TestTLSConfig(t *testing.T) { } func TestOIDCAuthentication(t *testing.T) { - var err error - cert := path.Join(os.TempDir(), "oidc-cert") key := path.Join(os.TempDir(), "oidc-key") @@ -188,119 +185,120 @@ func TestOIDCAuthentication(t *testing.T) { oidctesting.GenerateSelfSignedCert(t, "127.0.0.1", cert, key) - // Create a TLS server and a client. - op := oidctesting.NewOIDCProvider(t) - srv, err := op.ServeTLSWithKeyPair(cert, key) - if err != nil { - t.Fatalf("Cannot start server: %v", err) - } - defer srv.Close() + // Ensure all tests pass when the issuer is not at a base URL. + for _, path := range []string{"", "/path/with/trailing/slash/"} { - // A provider config with all required fields. - op.AddMinimalProviderConfig(srv) - - tests := []struct { - userClaim string - groupsClaim string - token string - userInfo user.Info - verified bool - err string - }{ - { - "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")}, - true, - "", - }, - { - // Use user defined claim (email here). - "email", - "", - generateGoodToken(t, op, srv.URL, "client-foo", "client-foo", "email", "foo@example.com", "", nil), - &user.DefaultInfo{Name: "foo@example.com"}, - true, - "", - }, - { - // Use user defined claim (email here). - "email", - "", - generateGoodToken(t, op, srv.URL, "client-foo", "client-foo", "email", "foo@example.com", "groups", []string{"group1", "group2"}), - &user.DefaultInfo{Name: "foo@example.com"}, - true, - "", - }, - { - // Use user defined claim (email here). - "email", - "groups", - generateGoodToken(t, op, srv.URL, "client-foo", "client-foo", "email", "foo@example.com", "groups", []string{"group1", "group2"}), - &user.DefaultInfo{Name: "foo@example.com", Groups: []string{"group1", "group2"}}, - true, - "", - }, - { - "sub", - "", - generateMalformedToken(t, op, srv.URL, "client-foo", "client-foo", "sub", "user-foo", "", nil), - nil, - false, - "oidc: unable to verify JWT signature: no matching keys", - }, - { - // Invalid 'aud'. - "sub", - "", - generateGoodToken(t, op, srv.URL, "client-foo", "client-bar", "sub", "user-foo", "", nil), - nil, - false, - "oidc: JWT claims invalid: invalid claims, 'aud' claim and 'client_id' do not match", - }, - { - // Invalid issuer. - "sub", - "", - generateGoodToken(t, op, "http://foo-bar.com", "client-foo", "client-foo", "sub", "user-foo", "", nil), - nil, - false, - "oidc: JWT claims invalid: invalid claim value: 'iss'.", - }, - { - "sub", - "", - generateExpiredToken(t, op, srv.URL, "client-foo", "client-foo", "sub", "user-foo", "", nil), - nil, - false, - "oidc: JWT claims invalid: token is expired", - }, - } - - for i, tt := range tests { - client, err := New(OIDCOptions{srv.URL, "client-foo", cert, tt.userClaim, tt.groupsClaim}) + // Create a TLS server and a client. + op := oidctesting.NewOIDCProvider(t, path) + srv, err := op.ServeTLSWithKeyPair(cert, key) if err != nil { - t.Errorf("Unexpected error: %v", err) - continue + t.Fatalf("Cannot start server: %v", err) + } + defer srv.Close() + + tests := []struct { + userClaim string + groupsClaim string + token string + userInfo user.Info + verified bool + err string + }{ + { + "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")}, + true, + "", + }, + { + // Use user defined claim (email here). + "email", + "", + generateGoodToken(t, op, srv.URL, "client-foo", "client-foo", "email", "foo@example.com", "", nil), + &user.DefaultInfo{Name: "foo@example.com"}, + true, + "", + }, + { + // Use user defined claim (email here). + "email", + "", + generateGoodToken(t, op, srv.URL, "client-foo", "client-foo", "email", "foo@example.com", "groups", []string{"group1", "group2"}), + &user.DefaultInfo{Name: "foo@example.com"}, + true, + "", + }, + { + // Use user defined claim (email here). + "email", + "groups", + generateGoodToken(t, op, srv.URL, "client-foo", "client-foo", "email", "foo@example.com", "groups", []string{"group1", "group2"}), + &user.DefaultInfo{Name: "foo@example.com", Groups: []string{"group1", "group2"}}, + true, + "", + }, + { + "sub", + "", + generateMalformedToken(t, op, srv.URL, "client-foo", "client-foo", "sub", "user-foo", "", nil), + nil, + false, + "oidc: unable to verify JWT signature: no matching keys", + }, + { + // Invalid 'aud'. + "sub", + "", + generateGoodToken(t, op, srv.URL, "client-foo", "client-bar", "sub", "user-foo", "", nil), + nil, + false, + "oidc: JWT claims invalid: invalid claims, 'aud' claim and 'client_id' do not match", + }, + { + // Invalid issuer. + "sub", + "", + generateGoodToken(t, op, "http://foo-bar.com", "client-foo", "client-foo", "sub", "user-foo", "", nil), + nil, + false, + "oidc: JWT claims invalid: invalid claim value: 'iss'.", + }, + { + "sub", + "", + generateExpiredToken(t, op, srv.URL, "client-foo", "client-foo", "sub", "user-foo", "", nil), + nil, + false, + "oidc: JWT claims invalid: token is expired", + }, } - user, result, err := client.AuthenticateToken(tt.token) - if tt.err != "" { - if !strings.HasPrefix(err.Error(), tt.err) { - t.Errorf("#%d: Expecting: %v..., but got: %v", i, tt.err, err) - } - } else { + for i, tt := range tests { + client, err := New(OIDCOptions{srv.URL, "client-foo", cert, tt.userClaim, tt.groupsClaim}) if err != nil { - t.Errorf("#%d: Unexpected error: %v", i, err) + t.Errorf("Unexpected error: %v", err) + continue } + + user, result, err := client.AuthenticateToken(tt.token) + if tt.err != "" { + if !strings.HasPrefix(err.Error(), tt.err) { + t.Errorf("#%d: Expecting: %v..., but got: %v", i, tt.err, err) + } + } else { + if err != nil { + t.Errorf("#%d: Unexpected error: %v", i, err) + } + } + if !reflect.DeepEqual(tt.verified, result) { + t.Errorf("#%d: Expecting: %v, but got: %v", i, tt.verified, result) + } + if !reflect.DeepEqual(tt.userInfo, user) { + t.Errorf("#%d: Expecting: %v, but got: %v", i, tt.userInfo, user) + } + client.Close() } - if !reflect.DeepEqual(tt.verified, result) { - t.Errorf("#%d: Expecting: %v, but got: %v", i, tt.verified, result) - } - if !reflect.DeepEqual(tt.userInfo, user) { - t.Errorf("#%d: Expecting: %v, but got: %v", i, tt.userInfo, user) - } - client.Close() } } diff --git a/plugin/pkg/auth/authenticator/token/oidc/testing/provider.go b/plugin/pkg/auth/authenticator/token/oidc/testing/provider.go index b3cbdc7a3a5..ae7353ff276 100644 --- a/plugin/pkg/auth/authenticator/token/oidc/testing/provider.go +++ b/plugin/pkg/auth/authenticator/token/oidc/testing/provider.go @@ -33,6 +33,7 @@ import ( "net/http/httptest" "net/url" "os" + "path" "path/filepath" "testing" "time" @@ -43,7 +44,7 @@ import ( ) // NewOIDCProvider provides a bare minimum OIDC IdP Server useful for testing. -func NewOIDCProvider(t *testing.T) *OIDCProvider { +func NewOIDCProvider(t *testing.T, issuerPath string) *OIDCProvider { privKey, err := key.GeneratePrivateKey() if err != nil { t.Fatalf("Cannot create OIDC Provider: %v", err) @@ -51,20 +52,22 @@ func NewOIDCProvider(t *testing.T) *OIDCProvider { } op := &OIDCProvider{ - Mux: http.NewServeMux(), - PrivKey: privKey, + Mux: http.NewServeMux(), + PrivKey: privKey, + issuerPath: issuerPath, } - op.Mux.HandleFunc("/.well-known/openid-configuration", op.handleConfig) - op.Mux.HandleFunc("/keys", op.handleKeys) + op.Mux.HandleFunc(path.Join(issuerPath, "/.well-known/openid-configuration"), op.handleConfig) + op.Mux.HandleFunc(path.Join(issuerPath, "/keys"), op.handleKeys) return op } type OIDCProvider struct { - Mux *http.ServeMux - PCFG oidc.ProviderConfig - PrivKey *key.PrivateKey + Mux *http.ServeMux + PCFG oidc.ProviderConfig + PrivKey *key.PrivateKey + issuerPath string } func (op *OIDCProvider) ServeTLSWithKeyPair(cert, key string) (*httptest.Server, error) { @@ -77,20 +80,31 @@ func (op *OIDCProvider) ServeTLSWithKeyPair(cert, key string) (*httptest.Server, return nil, fmt.Errorf("Cannot load cert/key pair: %v", err) } srv.StartTLS() - return srv, nil -} -func (op *OIDCProvider) AddMinimalProviderConfig(srv *httptest.Server) { + // The issuer's URL is extended by an optional path. This ensures that the plugin can + // handle issuers that use a non-root path for discovery (see kubernetes/kubernetes#29749). + srv.URL = srv.URL + op.issuerPath + + u, err := url.Parse(srv.URL) + if err != nil { + return nil, err + } + pathFor := func(p string) *url.URL { + u2 := *u // Shallow copy. + u2.Path = path.Join(u2.Path, p) + return &u2 + } + op.PCFG = oidc.ProviderConfig{ - Issuer: MustParseURL(srv.URL), - AuthEndpoint: MustParseURL(srv.URL + "/auth"), - TokenEndpoint: MustParseURL(srv.URL + "/token"), - KeysEndpoint: MustParseURL(srv.URL + "/keys"), + Issuer: u, + AuthEndpoint: pathFor("/auth"), + TokenEndpoint: pathFor("/token"), + KeysEndpoint: pathFor("/keys"), ResponseTypesSupported: []string{"code"}, SubjectTypesSupported: []string{"public"}, IDTokenSigningAlgValues: []string{"RS256"}, } - + return srv, nil } func (op *OIDCProvider) handleConfig(w http.ResponseWriter, req *http.Request) { @@ -122,14 +136,6 @@ func (op *OIDCProvider) handleKeys(w http.ResponseWriter, req *http.Request) { w.Write(b) } -func MustParseURL(s string) *url.URL { - u, err := url.Parse(s) - if err != nil { - panic(fmt.Errorf("Failed to parse url: %v", err)) - } - return u -} - // generateSelfSignedCert generates a self-signed cert/key pairs and writes to the certPath/keyPath. // This method is mostly identical to crypto.GenerateSelfSignedCert except for the 'IsCA' and 'KeyUsage' // in the certificate template. (Maybe we can merge these two methods). diff --git a/plugin/pkg/client/auth/oidc/oidc_test.go b/plugin/pkg/client/auth/oidc/oidc_test.go index 4e15113b957..e3dca0f748e 100644 --- a/plugin/pkg/client/auth/oidc/oidc_test.go +++ b/plugin/pkg/client/auth/oidc/oidc_test.go @@ -51,13 +51,12 @@ func TestNewOIDCAuthProvider(t *testing.T) { defer os.Remove(tempDir) oidctesting.GenerateSelfSignedCert(t, "127.0.0.1", cert, key) - op := oidctesting.NewOIDCProvider(t) + op := oidctesting.NewOIDCProvider(t, "") srv, err := op.ServeTLSWithKeyPair(cert, key) if err != nil { t.Fatalf("Cannot start server %v", err) } defer srv.Close() - op.AddMinimalProviderConfig(srv) certData, err := ioutil.ReadFile(cert) if err != nil {