From c34e5a727dc80e14ebe0aa81bf4ed1cf9886c764 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Tue, 3 Nov 2020 16:17:38 -0800 Subject: [PATCH 01/21] Starting the implementation of an OIDC authorization endpoint handler Does not validate incoming request parameters yet. Also is not served on the http/https ports yet. Those will come in future commits. Signed-off-by: Andrew Keesler --- internal/oidc/auth/auth_handler.go | 111 +++++++++ internal/oidc/auth/auth_handler_test.go | 303 ++++++++++++++++++++++++ 2 files changed, 414 insertions(+) create mode 100644 internal/oidc/auth/auth_handler.go create mode 100644 internal/oidc/auth/auth_handler_test.go diff --git a/internal/oidc/auth/auth_handler.go b/internal/oidc/auth/auth_handler.go new file mode 100644 index 000000000..247be1fb9 --- /dev/null +++ b/internal/oidc/auth/auth_handler.go @@ -0,0 +1,111 @@ +// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +// Package auth provides a handler for the OIDC authorization endpoint. +package auth + +import ( + "fmt" + "net/http" + + "golang.org/x/oauth2" + "k8s.io/klog/v2" + + "go.pinniped.dev/internal/httputil/httperr" + "go.pinniped.dev/internal/oidc/provider" + "go.pinniped.dev/internal/oidcclient/nonce" + "go.pinniped.dev/internal/oidcclient/pkce" + "go.pinniped.dev/internal/oidcclient/state" +) + +type IDPListGetter interface { + GetIDPList() []provider.UpstreamOIDCIdentityProvider +} + +func NewHandler( + issuer string, + idpListGetter IDPListGetter, + generateState func() (state.State, error), + generatePKCE func() (pkce.Code, error), + generateNonce func() (nonce.Nonce, error), +) http.Handler { + return httperr.HandlerFunc(func(w http.ResponseWriter, r *http.Request) error { + if r.Method != http.MethodPost && r.Method != http.MethodGet { + // https://openid.net/specs/openid-connect-core-1_0.html#AuthRequest + // Authorization Servers MUST support the use of the HTTP GET and POST methods defined in + // RFC 2616 [RFC2616] at the Authorization Endpoint. + return httperr.Newf(http.StatusMethodNotAllowed, "%s (try GET or POST)", r.Method) + } + + upstreamIDP, err := chooseUpstreamIDP(idpListGetter) + if err != nil { + return err + } + + stateValue, nonceValue, pkceValue, err := generateParams(generateState, generateNonce, generatePKCE) + if err != nil { + return err + } + + upstreamOAuthConfig := oauth2.Config{ + ClientID: upstreamIDP.ClientID, + Endpoint: oauth2.Endpoint{ + AuthURL: upstreamIDP.AuthorizationURL.String(), + }, + RedirectURL: fmt.Sprintf("%s/callback/%s", issuer, upstreamIDP.Name), + Scopes: upstreamIDP.Scopes, + } + + http.Redirect(w, r, + upstreamOAuthConfig.AuthCodeURL( + stateValue.String(), + oauth2.AccessTypeOffline, + nonceValue.Param(), + pkceValue.Challenge(), + pkceValue.Method(), + ), + 302, + ) + + return nil + }) +} + +func chooseUpstreamIDP(idpListGetter IDPListGetter) (*provider.UpstreamOIDCIdentityProvider, error) { + allUpstreamIDPs := idpListGetter.GetIDPList() + if len(allUpstreamIDPs) == 0 { + return nil, httperr.New( + http.StatusUnprocessableEntity, + "No upstream providers are configured", + ) + } else if len(allUpstreamIDPs) > 1 { + return nil, httperr.New( + http.StatusUnprocessableEntity, + "Too many upstream providers are configured (support for multiple upstreams is not yet implemented)", + ) + } + return &allUpstreamIDPs[0], nil +} + +func generateParams( + generateState func() (state.State, error), + generateNonce func() (nonce.Nonce, error), + generatePKCE func() (pkce.Code, error), +) (state.State, nonce.Nonce, pkce.Code, error) { + stateValue, err := generateState() + if err != nil { + klog.InfoS("error generating state param", "err", err) + return "", "", "", httperr.Wrap(http.StatusInternalServerError, "error generating state param", err) + } + nonceValue, err := generateNonce() + if err != nil { + klog.InfoS("error generating nonce param", "err", err) + return "", "", "", httperr.Wrap(http.StatusInternalServerError, "error generating nonce param", err) + } + pkceValue, err := generatePKCE() + if err != nil { + klog.InfoS("error generating PKCE param", "err", err) + return "", "", "", httperr.Wrap(http.StatusInternalServerError, "error generating PKCE param", err) + } + return stateValue, nonceValue, pkceValue, nil +} diff --git a/internal/oidc/auth/auth_handler_test.go b/internal/oidc/auth/auth_handler_test.go new file mode 100644 index 000000000..36c607d54 --- /dev/null +++ b/internal/oidc/auth/auth_handler_test.go @@ -0,0 +1,303 @@ +// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package auth + +import ( + "fmt" + "net/http" + "net/http/httptest" + "net/url" + "strings" + "testing" + + "github.com/stretchr/testify/require" + + "go.pinniped.dev/internal/oidc/provider" + "go.pinniped.dev/internal/oidcclient/nonce" + "go.pinniped.dev/internal/oidcclient/pkce" + "go.pinniped.dev/internal/oidcclient/state" +) + +func TestAuthorizationEndpoint(t *testing.T) { + upstreamAuthURL, err := url.Parse("https://some-upstream-idp:8443/auth") + require.NoError(t, err) + + upstreamOIDCIdentityProvider := provider.UpstreamOIDCIdentityProvider{ + Name: "some-idp", + ClientID: "some-client-id", + AuthorizationURL: *upstreamAuthURL, + Scopes: []string{"scope1", "scope2"}, + } + + issuer := "https://my-issuer.com/some-path" + + happyStateGenerator := func() (state.State, error) { return "test-state", nil } + happyPKCEGenerator := func() (pkce.Code, error) { return "test-pkce", nil } + happyNonceGenerator := func() (nonce.Nonce, error) { return "test-nonce", nil } + + // This is the PKCE challenge which is calculated as base64(sha256("test-pkce")). For example: + // $ echo -n test-pkce | shasum -a 256 | cut -d" " -f1 | xxd -r -p | base64 | cut -d"=" -f1 + expectedCodeChallenge := "VVaezYqum7reIhoavCHD1n2d-piN3r_mywoYj7fCR7g" + + happyGetRequestPath := fmt.Sprintf( + "/some/path?response_type=code&scope=%s&client_id=pinniped-cli&state=some-state-value&redirect_uri=%s", + url.QueryEscape("openid profile email"), + url.QueryEscape("http://127.0.0.1/callback"), + ) + + type testCase struct { + name string + + issuer string + idpListGetter provider.DynamicUpstreamIDPProvider + generateState func() (state.State, error) + generatePKCE func() (pkce.Code, error) + generateNonce func() (nonce.Nonce, error) + method string + path string + body string + + wantStatus int + wantContentType string + wantBodyString string + wantLocationHeader string + } + + tests := []testCase{ + { + name: "happy path using GET", + issuer: issuer, + idpListGetter: newIDPListGetter(upstreamOIDCIdentityProvider), + generateState: happyStateGenerator, + generatePKCE: happyPKCEGenerator, + generateNonce: happyNonceGenerator, + method: http.MethodGet, + path: happyGetRequestPath, + wantStatus: http.StatusFound, + wantContentType: "text/html; charset=utf-8", + wantBodyString: "", + wantLocationHeader: fmt.Sprintf("%s?%s", + upstreamAuthURL.String(), + url.Values{ + "response_type": []string{"code"}, + "access_type": []string{"offline"}, + "scope": []string{"scope1 scope2"}, + "client_id": []string{"some-client-id"}, + "state": []string{"test-state"}, + "nonce": []string{"test-nonce"}, + "code_challenge": []string{expectedCodeChallenge}, + "code_challenge_method": []string{"S256"}, + "redirect_uri": []string{issuer + "/callback/some-idp"}, + }.Encode(), + ), + }, + { + name: "happy path using POST", + issuer: issuer, + idpListGetter: newIDPListGetter(upstreamOIDCIdentityProvider), + generateState: happyStateGenerator, + generatePKCE: happyPKCEGenerator, + generateNonce: happyNonceGenerator, + method: http.MethodPost, + path: "/some/path", + body: url.Values{ + "response_type": []string{"code"}, + "scope": []string{"openid profile email"}, + "client_id": []string{"pinniped-cli"}, + "state": []string{"some-state-value"}, + "redirect_uri": []string{"http://127.0.0.1/callback"}, + }.Encode(), + wantStatus: http.StatusFound, + wantContentType: "", + wantBodyString: "", + wantLocationHeader: fmt.Sprintf("%s?%s", + upstreamAuthURL.String(), + url.Values{ + "response_type": []string{"code"}, + "access_type": []string{"offline"}, + "scope": []string{"scope1 scope2"}, + "client_id": []string{"some-client-id"}, + "state": []string{"test-state"}, + "nonce": []string{"test-nonce"}, + "code_challenge": []string{expectedCodeChallenge}, + "code_challenge_method": []string{"S256"}, + "redirect_uri": []string{issuer + "/callback/some-idp"}, + }.Encode(), + ), + }, + { + name: "error while generating state", + issuer: issuer, + idpListGetter: newIDPListGetter(upstreamOIDCIdentityProvider), + generateState: func() (state.State, error) { return "", fmt.Errorf("some state generator error") }, + generatePKCE: happyPKCEGenerator, + generateNonce: happyNonceGenerator, + method: http.MethodGet, + path: happyGetRequestPath, + wantStatus: http.StatusInternalServerError, + wantContentType: "text/plain; charset=utf-8", + wantBodyString: "Internal Server Error: error generating state param\n", + }, + { + name: "error while generating nonce", + issuer: issuer, + idpListGetter: newIDPListGetter(upstreamOIDCIdentityProvider), + generateState: happyStateGenerator, + generatePKCE: happyPKCEGenerator, + generateNonce: func() (nonce.Nonce, error) { return "", fmt.Errorf("some nonce generator error") }, + method: http.MethodGet, + path: happyGetRequestPath, + wantStatus: http.StatusInternalServerError, + wantContentType: "text/plain; charset=utf-8", + wantBodyString: "Internal Server Error: error generating nonce param\n", + }, + { + name: "error while generating PKCE", + issuer: issuer, + idpListGetter: newIDPListGetter(upstreamOIDCIdentityProvider), + generateState: happyStateGenerator, + generatePKCE: func() (pkce.Code, error) { return "", fmt.Errorf("some PKCE generator error") }, + generateNonce: happyNonceGenerator, + method: http.MethodGet, + path: happyGetRequestPath, + wantStatus: http.StatusInternalServerError, + wantContentType: "text/plain; charset=utf-8", + wantBodyString: "Internal Server Error: error generating PKCE param\n", + }, + { + name: "no upstream providers are configured", + issuer: issuer, + idpListGetter: newIDPListGetter(), + method: http.MethodGet, + path: happyGetRequestPath, + wantStatus: http.StatusUnprocessableEntity, + wantContentType: "text/plain; charset=utf-8", + wantBodyString: "Unprocessable Entity: No upstream providers are configured\n", + }, + { + name: "too many upstream providers are configured", + issuer: issuer, + idpListGetter: newIDPListGetter(upstreamOIDCIdentityProvider, upstreamOIDCIdentityProvider), + method: http.MethodGet, + path: happyGetRequestPath, + wantStatus: http.StatusUnprocessableEntity, + wantContentType: "text/plain; charset=utf-8", + wantBodyString: "Unprocessable Entity: Too many upstream providers are configured (support for multiple upstreams is not yet implemented)\n", + }, + { + name: "PUT is a bad method", + issuer: issuer, + idpListGetter: newIDPListGetter(upstreamOIDCIdentityProvider), + method: http.MethodPut, + path: "/some/path", + wantStatus: http.StatusMethodNotAllowed, + wantContentType: "text/plain; charset=utf-8", + wantBodyString: "Method Not Allowed: PUT (try GET or POST)\n", + }, + { + name: "PATCH is a bad method", + issuer: issuer, + idpListGetter: newIDPListGetter(upstreamOIDCIdentityProvider), + method: http.MethodPatch, + path: "/some/path", + wantStatus: http.StatusMethodNotAllowed, + wantContentType: "text/plain; charset=utf-8", + wantBodyString: "Method Not Allowed: PATCH (try GET or POST)\n", + }, + { + name: "DELETE is a bad method", + issuer: issuer, + idpListGetter: newIDPListGetter(upstreamOIDCIdentityProvider), + method: http.MethodDelete, + path: "/some/path", + wantStatus: http.StatusMethodNotAllowed, + wantContentType: "text/plain; charset=utf-8", + wantBodyString: "Method Not Allowed: DELETE (try GET or POST)\n", + }, + } + + runOneTestCase := func(t *testing.T, test testCase, subject http.Handler) { + req := httptest.NewRequest(test.method, test.path, strings.NewReader(test.body)) + rsp := httptest.NewRecorder() + subject.ServeHTTP(rsp, req) + + require.Equal(t, test.wantStatus, rsp.Code) + require.Equal(t, test.wantContentType, rsp.Header().Get("Content-Type")) + + if test.wantBodyString != "" { + require.Equal(t, test.wantBodyString, rsp.Body.String()) + } + + if test.wantLocationHeader != "" { + actualLocation := rsp.Header().Get("Location") + requireEqualURLs(t, actualLocation, test.wantLocationHeader) + } + } + + for _, test := range tests { + test := test + t.Run(test.name, func(t *testing.T) { + subject := NewHandler(test.issuer, test.idpListGetter, test.generateState, test.generatePKCE, test.generateNonce) + runOneTestCase(t, test, subject) + }) + } + + t.Run("allows upstream provider configuration to change between requests", func(t *testing.T) { + test := tests[0] + require.Equal(t, "happy path using GET", test.name) // re-use the happy path test case + + subject := NewHandler(test.issuer, test.idpListGetter, test.generateState, test.generatePKCE, test.generateNonce) + + runOneTestCase(t, test, subject) + + // Call the setter to change the upstream IDP settings. + newProviderSettings := provider.UpstreamOIDCIdentityProvider{ + Name: "some-other-idp", + ClientID: "some-other-client-id", + AuthorizationURL: *upstreamAuthURL, + Scopes: []string{"other-scope1", "other-scope2"}, + } + test.idpListGetter.SetIDPList([]provider.UpstreamOIDCIdentityProvider{newProviderSettings}) + + // Update the expectations of the test case to match the new upstream IDP settings. + test.wantLocationHeader = fmt.Sprintf("%s?%s", + upstreamAuthURL.String(), + url.Values{ + "response_type": []string{"code"}, + "access_type": []string{"offline"}, + "scope": []string{"other-scope1 other-scope2"}, + "client_id": []string{"some-other-client-id"}, + "state": []string{"test-state"}, + "nonce": []string{"test-nonce"}, + "code_challenge": []string{expectedCodeChallenge}, + "code_challenge_method": []string{"S256"}, + "redirect_uri": []string{issuer + "/callback/some-other-idp"}, + }.Encode(), + ) + + // Run again on the same instance of the subject with the modified upstream IDP settings and the + // modified expectations. This should ensure that the implementation is using the in-memory cache + // of upstream IDP settings appropriately in terms of always getting the values from the cache + // on every request. + runOneTestCase(t, test, subject) + }) +} + +func requireEqualURLs(t *testing.T, actualURL string, expectedURL string) { + actualLocationURL, err := url.Parse(actualURL) + require.NoError(t, err) + expectedLocationURL, err := url.Parse(expectedURL) + require.NoError(t, err) + require.Equal(t, expectedLocationURL.Scheme, actualLocationURL.Scheme) + require.Equal(t, expectedLocationURL.User, actualLocationURL.User) + require.Equal(t, expectedLocationURL.Host, actualLocationURL.Host) + require.Equal(t, expectedLocationURL.Path, actualLocationURL.Path) + require.Equal(t, expectedLocationURL.Query(), actualLocationURL.Query()) +} + +func newIDPListGetter(upstreamOIDCIdentityProviders ...provider.UpstreamOIDCIdentityProvider) provider.DynamicUpstreamIDPProvider { + idpProvider := provider.NewDynamicUpstreamIDPProvider() + idpProvider.SetIDPList(upstreamOIDCIdentityProviders) + return idpProvider +} From 259ffb52677ead73285f3179493e045f028b17bf Mon Sep 17 00:00:00 2001 From: Andrew Keesler Date: Wed, 4 Nov 2020 10:15:19 -0500 Subject: [PATCH 02/21] Checkpoint: write a single negative test using fosite Bringing in fosite to our go.mod introduced those other go.mod changes. Signed-off-by: Andrew Keesler --- go.mod | 5 +- go.sum | 59 ++++++++++++++ internal/oidc/auth/auth_handler.go | 11 +++ internal/oidc/auth/auth_handler_test.go | 102 ++++++++++++++++++++++-- 4 files changed, 170 insertions(+), 7 deletions(-) diff --git a/go.mod b/go.mod index f85984f89..dcec2960c 100644 --- a/go.mod +++ b/go.mod @@ -14,6 +14,7 @@ require ( github.com/golang/mock v1.4.4 github.com/golangci/golangci-lint v1.31.0 github.com/google/go-cmp v0.5.2 + github.com/ory/fosite v0.35.1 github.com/pkg/browser v0.0.0-20180916011732-0a3d74bf9ce4 github.com/sclevine/agouti v3.0.0+incompatible github.com/sclevine/spec v1.4.0 @@ -22,8 +23,8 @@ require ( github.com/stretchr/testify v1.6.1 go.pinniped.dev/generated/1.19/apis v0.0.0-00010101000000-000000000000 go.pinniped.dev/generated/1.19/client v0.0.0-00010101000000-000000000000 - golang.org/x/crypto v0.0.0-20200622213623-75b288015ac9 - golang.org/x/oauth2 v0.0.0-20191202225959-858c2ad4c8b6 + golang.org/x/crypto v0.0.0-20200709230013-948cd5f35899 + golang.org/x/oauth2 v0.0.0-20200107190931-bf48bf16ab8d golang.org/x/sync v0.0.0-20200625203802-6e8e738ad208 gopkg.in/square/go-jose.v2 v2.5.1 k8s.io/api v0.19.2 diff --git a/go.sum b/go.sum index 7006b2c37..aa2e942e7 100644 --- a/go.sum +++ b/go.sum @@ -42,6 +42,7 @@ github.com/Masterminds/semver v1.5.0 h1:H65muMkzWKEuNDnfl9d70GUjFniHKHRbFPGBuZ3Q github.com/Masterminds/semver v1.5.0/go.mod h1:MB6lktGJrhw8PrUyiEoblNEGEQ+RzHPF078ddwwvV3Y= github.com/NYTimes/gziphandler v0.0.0-20170623195520-56545f4a5d46 h1:lsxEuwrXEAokXB9qhlbKWPpo3KMLZQ5WB5WLQRW1uq0= github.com/NYTimes/gziphandler v0.0.0-20170623195520-56545f4a5d46/go.mod h1:3wb06e3pkSAbeQ52E9H9iFoQsEEwGN64994WTCIhntQ= +github.com/OneOfOne/xxhash v1.2.2 h1:KMrpdQIwFcEqXDklaen+P1axHaj9BSKzvpUUfnHldSE= github.com/OneOfOne/xxhash v1.2.2/go.mod h1:HSdplMjZKSmBqAxg5vPj2TmRDmfkzw+cTzAElWljhcU= github.com/OpenPeeDeeP/depguard v1.0.1 h1:VlW4R6jmBIv3/u1JNlawEvJMM4J+dPORPaZasQee8Us= github.com/OpenPeeDeeP/depguard v1.0.1/go.mod h1:xsIw86fROiiwelg+jB2uM9PiKihMMmUx/1V+TNhjQvM= @@ -63,6 +64,8 @@ github.com/armon/circbuf v0.0.0-20150827004946-bbbad097214e/go.mod h1:3U/XgcO3hC github.com/armon/consul-api v0.0.0-20180202201655-eb2c6b5be1b6/go.mod h1:grANhF5doyWs3UAsr3K4I6qtAmlQcZDesFNEHPZAzj8= github.com/armon/go-metrics v0.0.0-20180917152333-f0300d1749da/go.mod h1:Q73ZrmVTwzkszR9V5SSuryQ31EELlFMUz1kKyl939pY= github.com/armon/go-radix v0.0.0-20180808171621-7fddfc383310/go.mod h1:ufUuZ+zHj4x4TnLV4JWEpy2hxWSpsRywHrMgIH9cCH8= +github.com/asaskevich/govalidator v0.0.0-20200428143746-21a406dcc535 h1:4daAzAu0S6Vi7/lbWECcX0j45yZReDZ56BQsrVBOEEY= +github.com/asaskevich/govalidator v0.0.0-20200428143746-21a406dcc535/go.mod h1:oGkLhpf+kjZl6xBf758TQhh5XrAeiJv/7FRz/2spLIg= github.com/beorn7/perks v0.0.0-20180321164747-3a771d992973/go.mod h1:Dwedo/Wpr24TaqPxmxbtue+5NUziq4I4S80YR8gNf3Q= github.com/beorn7/perks v1.0.0/go.mod h1:KWe93zE9D1o94FZ5RNwFwVgaQK1VOXiVxmqh+CedLV8= github.com/beorn7/perks v1.0.1 h1:VlbKKnNfV8bJzeqoa4cOKqO6bYr3WgKZxO8Z16+hsOM= @@ -111,8 +114,14 @@ github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/denis-tingajkin/go-header v0.3.1 h1:ymEpSiFjeItCy1FOP+x0M2KdCELdEAHUsNa8F+hHc6w= github.com/denis-tingajkin/go-header v0.3.1/go.mod h1:sq/2IxMhaZX+RRcgHfCRx/m0M5na0fBt4/CRe7Lrji0= +github.com/dgraph-io/ristretto v0.0.1/go.mod h1:T40EBc7CJke8TkpiYfGGKAeFjSaxuFXhuXRyumBd6RE= +github.com/dgraph-io/ristretto v0.0.2/go.mod h1:KPxhHT9ZxKefz+PCeOGsrHpl1qZ7i70dGTu2u+Ahh6E= +github.com/dgraph-io/ristretto v0.0.3 h1:jh22xisGBjrEVnRZ1DVTpBVQm0Xndu8sMl0CWDzSIBI= +github.com/dgraph-io/ristretto v0.0.3/go.mod h1:KPxhHT9ZxKefz+PCeOGsrHpl1qZ7i70dGTu2u+Ahh6E= github.com/dgrijalva/jwt-go v3.2.0+incompatible h1:7qlOGliEKZXTDg6OTjfoBKDXWrumCAMpl/TFQ4/5kLM= github.com/dgrijalva/jwt-go v3.2.0+incompatible/go.mod h1:E3ru+11k8xSBh+hMPgOLZmtrrCbhqsmaPHjLKYnJCaQ= +github.com/dgryski/go-farm v0.0.0-20190423205320-6a90982ecee2 h1:tdlZCpZ/P9DhczCTSixgIKmwPv6+wP5DGjqLYw5SUiA= +github.com/dgryski/go-farm v0.0.0-20190423205320-6a90982ecee2/go.mod h1:SqUrOPUnsFjfmXRMNPybcSiG0BgUW2AuFH8PAnS2iTw= github.com/dgryski/go-sip13 v0.0.0-20181026042036-e10d5fee7954/go.mod h1:vAd38F8PWV+bWy6jNmig1y/TA+kYO4g3RSRF0IAv0no= github.com/docker/spdystream v0.0.0-20160310174837-449fdfce4d96 h1:cenwrSVm+Z7QLSV/BsnenAOcDXdX4cMv4wP0B/5QbPg= github.com/docker/spdystream v0.0.0-20160310174837-449fdfce4d96/go.mod h1:Qh8CwZgvJUkLughtfhJv5dyTYa91l1fOUCrgjqmcifM= @@ -122,6 +131,8 @@ github.com/dustin/go-humanize v1.0.0 h1:VSnTsYCnlFHaM2/igO1h6X3HA71jcobQuxemgkq4 github.com/dustin/go-humanize v1.0.0/go.mod h1:HtrtbFcZ19U5GC7JDqmcUSB87Iq5E25KnS6fMYU6eOk= github.com/elazarl/goproxy v0.0.0-20180725130230-947c36da3153 h1:yUdfgN0XgIJw7foRItutHYUIhlcKzcSf5vDpdhQAKTc= github.com/elazarl/goproxy v0.0.0-20180725130230-947c36da3153/go.mod h1:/Zj4wYkgs4iZTTu3o/KG3Itv/qCCa8VVMlb3i9OVuzc= +github.com/elazarl/goproxy v0.0.0-20181003060214-f58a169a71a5 h1:LCoguo7Zd0MByKMbQbTvcZw7HiBcbvew+MOcwsJVwrY= +github.com/elazarl/goproxy v0.0.0-20181003060214-f58a169a71a5/go.mod h1:/Zj4wYkgs4iZTTu3o/KG3Itv/qCCa8VVMlb3i9OVuzc= github.com/emicklei/go-restful v0.0.0-20170410110728-ff4f55a20633/go.mod h1:otzb+WCGbkyDHkqmQmT5YD2WR4BBwUdeQoFo8l/7tVs= github.com/emicklei/go-restful v2.9.5+incompatible h1:spTtZBk5DYEvbxMVutUuTyh1Ao2r4iyvLdACqsl/Ljk= github.com/emicklei/go-restful v2.9.5+incompatible/go.mod h1:otzb+WCGbkyDHkqmQmT5YD2WR4BBwUdeQoFo8l/7tVs= @@ -214,6 +225,7 @@ github.com/golang/mock v1.1.1/go.mod h1:oTYuIxOrZwtPieC+H1uAHpcLFnEyAGVDL/k47Jfb github.com/golang/mock v1.2.0/go.mod h1:oTYuIxOrZwtPieC+H1uAHpcLFnEyAGVDL/k47Jfbm0A= github.com/golang/mock v1.3.1 h1:qGJ6qTW+x6xX/my+8YUVl4WNpX9B7+/l2tRsHGZ7f2s= github.com/golang/mock v1.3.1/go.mod h1:sBzyDLLjw3U8JLTeZvSv8jJB+tU5PVekmnlKIyFUx0Y= +github.com/golang/mock v1.4.3/go.mod h1:UOMv5ysSaYNkG+OFQykRIcU/QvvxJf3p21QfJ2Bt3cw= github.com/golang/mock v1.4.4 h1:l75CXGRSwbaYNpl/Z2X1XIIAMSCquvXgpVZDhwEIJsc= github.com/golang/mock v1.4.4/go.mod h1:l3mdAwkq5BuhzHwde/uurv3sEJeZMXNpwsxVWU71h+4= github.com/golang/protobuf v1.2.0/go.mod h1:6lQm79b+lXiMfvg/cZm0SGofjICqVBUtrP5yJMmIC1U= @@ -288,6 +300,7 @@ github.com/googleapis/gnostic v0.4.1/go.mod h1:LRhVm6pbyptWbWbuZ38d1eyptfvIytN3i github.com/gookit/color v1.2.5/go.mod h1:AhIE+pS6D4Ql0SQWbBeXPHw7gY0/sjHoA4s/n1KB7xg= github.com/gopherjs/gopherjs v0.0.0-20181017120253-0766667cb4d1 h1:EGx4pi6eqNxGaHF6qqu48+N2wcFQ5qg5FXgOdqsJ5d8= github.com/gopherjs/gopherjs v0.0.0-20181017120253-0766667cb4d1/go.mod h1:wJfORRmW1u3UXTncJ5qlYoELFm8eSnnEO6hX4iZ3EWY= +github.com/gorilla/mux v1.7.3/go.mod h1:1lud6UwP+6orDFRuTfBEV8e9/aOM/c4fVVCaMa2zaAs= github.com/gorilla/websocket v0.0.0-20170926233335-4201258b820c/go.mod h1:E7qHFY5m1UJ88s3WnNqhKjPHQ0heANvMoAMk2YaljkQ= github.com/gorilla/websocket v1.4.0/go.mod h1:E7qHFY5m1UJ88s3WnNqhKjPHQ0heANvMoAMk2YaljkQ= github.com/gorilla/websocket v1.4.2 h1:+/TMaTYc4QFitKJxsQ7Yye35DkWvkdLcvGKqM+x0Ufc= @@ -359,6 +372,7 @@ github.com/klauspost/compress v1.10.10/go.mod h1:aoV0uJVorq1K+umq18yTdKaF57EivdY github.com/konsorten/go-windows-terminal-sequences v1.0.1/go.mod h1:T0+1ngSBFLxvqU3pZ+m/2kptfBszLMUkC4ZK/EgS/cQ= github.com/konsorten/go-windows-terminal-sequences v1.0.3 h1:CE8S1cTafDpPvMhIxNJKvHsGVBgn1xWYf1NbHQhywc8= github.com/konsorten/go-windows-terminal-sequences v1.0.3/go.mod h1:T0+1ngSBFLxvqU3pZ+m/2kptfBszLMUkC4ZK/EgS/cQ= +github.com/kr/fs v0.1.0/go.mod h1:FFnZGqtBN9Gxj7eW1uZ42v5BccTP0vu6NEaFoC2HwRg= github.com/kr/logfmt v0.0.0-20140226030751-b84e30acd515/go.mod h1:+0opPa2QZZtGFBFZlji/RkVcI2GknAs/DXo4wKdlNEc= github.com/kr/pretty v0.1.0/go.mod h1:dAy3ld7l9f0ibDNOQOHHMYYIIbhfbHSm3C4ZsoJORNo= github.com/kr/pretty v0.2.0/go.mod h1:ipq/a2n7PKx3OHsz4KJII5eveXtPO4qwEXGdVfWzfnI= @@ -396,6 +410,8 @@ github.com/mattn/go-isatty v0.0.12/go.mod h1:cbi8OIDigv2wuxKPP5vlRcQ1OAZbq2CE4Ky github.com/mattn/go-runewidth v0.0.2/go.mod h1:LwmH8dsx7+W8Uxz3IHJYH5QSwggIsqBzpuz5H//U1FU= github.com/mattn/go-sqlite3 v1.9.0/go.mod h1:FPy6KqzDD04eiIsT53CuJW3U88zkxoIYsOqkbpncsNc= github.com/mattn/goveralls v0.0.2/go.mod h1:8d1ZMHsd7fW6IRPKQh46F2WRpyib5/X4FOpevwGNQEw= +github.com/mattn/goveralls v0.0.6 h1:cr8Y0VMo/MnEZBjxNN/vh6G90SZ7IMb6lms1dzMoO+Y= +github.com/mattn/goveralls v0.0.6/go.mod h1:h8b4ow6FxSPMQHF6o2ve3qsclnffZjYTNEKmLesRwqw= github.com/matttproud/golang_protobuf_extensions v1.0.1/go.mod h1:D8He9yQNgCq6Z5Ld7szi9bcBfOoFv/3dc6xSMkL2PC0= github.com/matttproud/golang_protobuf_extensions v1.0.2-0.20181231171920-c182affec369 h1:I0XW9+e1XWDxdcEniV4rQAIOPUGDq67JSCiRCgGCZLI= github.com/matttproud/golang_protobuf_extensions v1.0.2-0.20181231171920-c182affec369/go.mod h1:BSXmuO+STAnVfrANrmjBb36TMTDstsz7MSK+HVaYKv4= @@ -411,6 +427,8 @@ github.com/mitchellh/iochan v1.0.0/go.mod h1:JwYml1nuB7xOzsp52dPpHFffvOCDupsG0Qu github.com/mitchellh/mapstructure v0.0.0-20160808181253-ca63d7c062ee/go.mod h1:FVVH3fgwuzCH5S8UJGiWEs2h04kUh9fWfEaFds41c1Y= github.com/mitchellh/mapstructure v1.1.2 h1:fmNYVwqnSfB9mZU6OS2O6GsXM+wcskZDuKQzvN1EDeE= github.com/mitchellh/mapstructure v1.1.2/go.mod h1:FVVH3fgwuzCH5S8UJGiWEs2h04kUh9fWfEaFds41c1Y= +github.com/mitchellh/mapstructure v1.3.2 h1:mRS76wmkOn3KkKAyXDu42V+6ebnXWIztFSYGN7GeoRg= +github.com/mitchellh/mapstructure v1.3.2/go.mod h1:bFUtVrKA4DC2yAKiSyO/QUcy7e+RRV2QTWOzhPopBRo= github.com/moby/term v0.0.0-20200312100748-672ec06f55cd/go.mod h1:DdlQx2hp0Ss5/fLikoLlEeIYiATotOjgB//nb973jeo= github.com/modern-go/concurrent v0.0.0-20180228061459-e0a39a4cb421/go.mod h1:6dJC0mAP4ikYIbvyc7fijjWJddQyLn8Ig3JB5CqoB9Q= github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd h1:TRLaZ9cD/w8PVh93nsPXa1VrQ6jlwL5oN8l14QlcNfg= @@ -418,6 +436,9 @@ github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd/go.mod h1:6dJ github.com/modern-go/reflect2 v0.0.0-20180701023420-4b7aa43c6742/go.mod h1:bx2lNnkwVCuqBIxFjflWJWanXIb3RllmbCylyMrvgv0= github.com/modern-go/reflect2 v1.0.1 h1:9f412s+6RmYXLWZSEzVVgPGK7C2PphHj5RJrvfx9AWI= github.com/modern-go/reflect2 v1.0.1/go.mod h1:bx2lNnkwVCuqBIxFjflWJWanXIb3RllmbCylyMrvgv0= +github.com/mohae/deepcopy v0.0.0-20170929034955-c48cc78d4826 h1:RWengNIwukTxcDr9M+97sNutRR1RKhG96O6jWumTTnw= +github.com/mohae/deepcopy v0.0.0-20170929034955-c48cc78d4826/go.mod h1:TaXosZuwdSHYgviHp1DAtfrULt5eUgsSMsZf+YrPgl8= +github.com/moul/http2curl v0.0.0-20170919181001-9ac6cf4d929b/go.mod h1:8UbvGypXm98wA/IqH45anm5Y2Z6ep6O31QGOAZ3H0fQ= github.com/mozilla/tls-observatory v0.0.0-20200317151703-4fa42e1c2dee/go.mod h1:SrKMQvPiws7F7iqYp8/TX+IhxCYhzr6N/1yb8cwHsGk= github.com/munnerz/goautoneg v0.0.0-20120707110453-a547fc61f48d/go.mod h1:+n7T8mK8HuQTcFwEeznm/DIxMOiR9yIdICNftLE1DvQ= github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 h1:C3w9PqII01/Oq1c1nUAm88MOHcQC9l5mIlSMApZMrHA= @@ -435,6 +456,8 @@ github.com/nishanths/exhaustive v0.0.0-20200811152831-6cf413ae40e0/go.mod h1:wBE github.com/nxadm/tail v1.4.4 h1:DQuhQpB1tVlglWS2hLQ5OV6B5r8aGxSrPc5Qo6uTN78= github.com/nxadm/tail v1.4.4/go.mod h1:kenIhsEOeOJmVchQTgglprH7qJGnHDVpk1VPCcaMI8A= github.com/oklog/ulid v1.3.1/go.mod h1:CirwcVhetQ6Lv90oh/F+FBtV6XMibvdAFo93nm5qn4U= +github.com/oleiade/reflections v1.0.0 h1:0ir4pc6v8/PJ0yw5AEtMddfXpWBXg9cnG7SgSoJuCgY= +github.com/oleiade/reflections v1.0.0/go.mod h1:RbATFBbKYkVdqmSFtx13Bb/tVhR0lgOBXunWTZKeL4w= github.com/olekukonko/tablewriter v0.0.0-20170122224234-a0225b3f23b5/go.mod h1:vsDQFd/mU46D+Z4whnwzcISnGGzXWMclvtLoiIKAKIo= github.com/onsi/ginkgo v0.0.0-20170829012221-11459a886d9c/go.mod h1:lLunBs/Ym6LB5Z9jYTR76FiuTmxDTDusOGeTQH+WWjE= github.com/onsi/ginkgo v1.6.0/go.mod h1:lLunBs/Ym6LB5Z9jYTR76FiuTmxDTDusOGeTQH+WWjE= @@ -447,10 +470,22 @@ github.com/onsi/gomega v1.7.0/go.mod h1:ex+gbHU/CVuBBDIJjb2X0qEXbFg53c61hWP/1Cpa github.com/onsi/gomega v1.7.1/go.mod h1:XdKZgCCFLUoM/7CFJVPcG8C1xQ1AJ0vpAezJrB7JYyY= github.com/onsi/gomega v1.10.1 h1:o0+MgICZLuZ7xjH7Vx6zS/zcu93/BEp1VwkIW1mEXCE= github.com/onsi/gomega v1.10.1/go.mod h1:iN09h71vgCQne3DLsj+A5owkum+a2tYe+TOCB1ybHNo= +github.com/ory/fosite v0.35.1 h1:mGPcwVGwHA7Yy9wr/7LDps6BEXyavL32NxizL9eH53Q= +github.com/ory/fosite v0.35.1/go.mod h1:h+ize9gk0GvRyGjabriqSEmTkMhny+O95cijb8DVqPE= +github.com/ory/go-acc v0.2.5 h1:31irXHzG2vnKQSE4weJm7AdfrnpaVjVCq3nD7viXCJE= +github.com/ory/go-acc v0.2.5/go.mod h1:4Kb/UnPcT8qRAk3IAxta+hvVapdxTLWtrr7bFLlEgpw= +github.com/ory/go-convenience v0.1.0 h1:zouLKfF2GoSGnJwGq+PE/nJAE6dj2Zj5QlTgmMTsTS8= +github.com/ory/go-convenience v0.1.0/go.mod h1:uEY/a60PL5c12nYz4V5cHY03IBmwIAEm8TWB0yn9KNs= +github.com/ory/viper v1.7.5 h1:+xVdq7SU3e1vNaCsk/ixsfxE4zylk1TJUiJrY647jUE= +github.com/ory/viper v1.7.5/go.mod h1:ypOuyJmEUb3oENywQZRgeAMwqgOyDqwboO1tj3DjTaM= +github.com/parnurzeal/gorequest v0.2.15/go.mod h1:3Kh2QUMJoqw3icWAecsyzkpY7UzRfDhbRdTjtNwNiUE= github.com/pascaldekloe/goe v0.0.0-20180627143212-57f6aae5913c/go.mod h1:lzWF7FIEvWOWxwDKqyGYQf6ZUaNfKdP144TG7ZOy1lc= +github.com/pborman/uuid v1.2.0 h1:J7Q5mO4ysT1dv8hyrUGHb9+ooztCXu1D8MY8DZYsu3g= github.com/pborman/uuid v1.2.0/go.mod h1:X/NO0urCmaxf9VXbdlT7C2Yzkj2IKimNn4k+gtPdI/k= github.com/pelletier/go-toml v1.2.0 h1:T5zMGML61Wp+FlcbWjRDT7yAxhJNAiPPLOFECq181zc= github.com/pelletier/go-toml v1.2.0/go.mod h1:5z9KED0ma1S8pY6P1sdut58dfprrGBbd/94hg7ilaic= +github.com/pelletier/go-toml v1.8.0 h1:Keo9qb7iRJs2voHvunFtuuYFsbWeOBh8/P9v/kVMFtw= +github.com/pelletier/go-toml v1.8.0/go.mod h1:D6yutnOGMveHEPV7VQOuvI/gXY61bv+9bAOTRnLElKs= github.com/peterbourgon/diskv v2.0.1+incompatible/go.mod h1:uqqh8zWWbv1HBMNONnaR/tNboyR3/BZd58JJSHlUSCU= github.com/phayes/checkstyle v0.0.0-20170904204023-bfd46e6a821d h1:CdDQnGF8Nq9ocOS/xlSptM1N3BbrA6/kmaep5ggwaIA= github.com/phayes/checkstyle v0.0.0-20170904204023-bfd46e6a821d/go.mod h1:3OzsM7FXDQlpCiw2j81fOmAwQLnZnLGXVKUzeKQXIAw= @@ -460,6 +495,7 @@ github.com/pkg/errors v0.8.0/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINE github.com/pkg/errors v0.8.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0= github.com/pkg/errors v0.9.1 h1:FEBLx1zS214owpjy7qsBeixbURkuhQAwrK5UwLGTwt4= github.com/pkg/errors v0.9.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0= +github.com/pkg/sftp v1.10.1/go.mod h1:lYOWFsE0bwd1+KfKJaKeuokY15vzFx25BLbzYYoAxZI= github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= github.com/posener/complete v1.1.1/go.mod h1:em0nMJCgc9GFtwrmVmEMR/ZL6WyhyjMBndrE9hABlRI= @@ -530,17 +566,24 @@ github.com/sonatard/noctx v0.0.1 h1:VC1Qhl6Oxx9vvWo3UDgrGXYCeKCe3Wbw7qAWL6FrmTY= github.com/sonatard/noctx v0.0.1/go.mod h1:9D2D/EoULe8Yy2joDHJj7bv3sZoq9AaSb8B4lqBjiZI= github.com/sourcegraph/go-diff v0.6.0 h1:WbN9e/jD8ujU+o0vd9IFN5AEwtfB0rn/zM/AANaClqQ= github.com/sourcegraph/go-diff v0.6.0/go.mod h1:iBszgVvyxdc8SFZ7gm69go2KDdt3ag071iBaWPF6cjs= +github.com/spaolacci/murmur3 v0.0.0-20180118202830-f09979ecbc72 h1:qLC7fQah7D6K1B0ujays3HV9gkFtllcxhzImRR7ArPQ= github.com/spaolacci/murmur3 v0.0.0-20180118202830-f09979ecbc72/go.mod h1:JwIasOWyU6f++ZhiEuf87xNszmSA2myDM2Kzu9HwQUA= github.com/spf13/afero v1.1.2/go.mod h1:j4pytiNVoe2o6bmDsKpLACNPDBIoEAkihy7loJ1B0CQ= github.com/spf13/afero v1.2.2 h1:5jhuqJyZCZf2JRofRvN/nIFgIWNzPa3/Vz8mYylgbWc= github.com/spf13/afero v1.2.2/go.mod h1:9ZxEEn6pIJ8Rxe320qSDBk6AsU0r9pR7Q4OcevTdifk= +github.com/spf13/afero v1.3.2 h1:GDarE4TJQI52kYSbSAmLiId1Elfj+xgSDqrUZxFhxlU= +github.com/spf13/afero v1.3.2/go.mod h1:5KUK8ByomD5Ti5Artl0RtHeI5pTF7MIDuXL3yY520V4= github.com/spf13/cast v1.3.0 h1:oget//CVOEoFewqQxwr0Ej5yjygnqGkvggSE/gB35Q8= github.com/spf13/cast v1.3.0/go.mod h1:Qx5cxh0v+4UWYiBimWS+eyWzqEqokIECu5etghLkUJE= +github.com/spf13/cast v1.3.1 h1:nFm6S0SMdyzrzcmThSipiEubIDy8WEXKNZ0UOgiRpng= +github.com/spf13/cast v1.3.1/go.mod h1:Qx5cxh0v+4UWYiBimWS+eyWzqEqokIECu5etghLkUJE= github.com/spf13/cobra v0.0.3/go.mod h1:1l0Ry5zgKvJasoi3XT1TypsSe7PqH0Sj9dhYf7v3XqQ= github.com/spf13/cobra v1.0.0 h1:6m/oheQuQ13N9ks4hubMG6BnvwOeaJrqSPLahSnczz8= github.com/spf13/cobra v1.0.0/go.mod h1:/6GTrnGXV9HjY+aR4k0oJ5tcvakLuG6EuKReYlHNrgE= github.com/spf13/jwalterweatherman v1.0.0 h1:XHEdyB+EcvlqZamSM4ZOMGlc93t6AcsBEu9Gc1vn7yk= github.com/spf13/jwalterweatherman v1.0.0/go.mod h1:cQK4TGJAtQXfYWX+Ddv3mKDzgVb68N+wFjFa4jdeBTo= +github.com/spf13/jwalterweatherman v1.1.0 h1:ue6voC5bR5F8YxI5S67j9i582FU4Qvo2bmqnqMYADFk= +github.com/spf13/jwalterweatherman v1.1.0/go.mod h1:aNWZUN0dPAAO/Ljvb5BEdw96iTZ0EXowPYD95IqWIGo= github.com/spf13/pflag v0.0.0-20170130214245-9ff6c6923cff/go.mod h1:DYY7MBk1bdzusC3SYhjObp+wFpr4gzcvqqNjLnInEg4= github.com/spf13/pflag v1.0.1/go.mod h1:DYY7MBk1bdzusC3SYhjObp+wFpr4gzcvqqNjLnInEg4= github.com/spf13/pflag v1.0.3/go.mod h1:DYY7MBk1bdzusC3SYhjObp+wFpr4gzcvqqNjLnInEg4= @@ -614,10 +657,14 @@ golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACk golang.org/x/crypto v0.0.0-20190510104115-cbcb75029529/go.mod h1:yigFU9vqHzYiE8UmvKecakEJjdnWj3jj499lnFckfCI= golang.org/x/crypto v0.0.0-20190605123033-f99c8df09eb5/go.mod h1:yigFU9vqHzYiE8UmvKecakEJjdnWj3jj499lnFckfCI= golang.org/x/crypto v0.0.0-20190611184440-5c40567a22f8/go.mod h1:yigFU9vqHzYiE8UmvKecakEJjdnWj3jj499lnFckfCI= +golang.org/x/crypto v0.0.0-20190820162420-60c769a6c586/go.mod h1:yigFU9vqHzYiE8UmvKecakEJjdnWj3jj499lnFckfCI= golang.org/x/crypto v0.0.0-20191011191535-87dc89f01550/go.mod h1:yigFU9vqHzYiE8UmvKecakEJjdnWj3jj499lnFckfCI= golang.org/x/crypto v0.0.0-20191206172530-e9b2fee46413/go.mod h1:LzIPMQfyMNhhGPhUkYOs5KpL4U8rLKemX1yGLhDgUto= +golang.org/x/crypto v0.0.0-20200510223506-06a226fb4e37/go.mod h1:LzIPMQfyMNhhGPhUkYOs5KpL4U8rLKemX1yGLhDgUto= golang.org/x/crypto v0.0.0-20200622213623-75b288015ac9 h1:psW17arqaxU48Z5kZ0CQnkZWQJsqcURM6tKiBApRjXI= golang.org/x/crypto v0.0.0-20200622213623-75b288015ac9/go.mod h1:LzIPMQfyMNhhGPhUkYOs5KpL4U8rLKemX1yGLhDgUto= +golang.org/x/crypto v0.0.0-20200709230013-948cd5f35899 h1:DZhuSZLsGlFL4CmhA8BcRA0mnthyA/nZ00AqCUo7vHg= +golang.org/x/crypto v0.0.0-20200709230013-948cd5f35899/go.mod h1:LzIPMQfyMNhhGPhUkYOs5KpL4U8rLKemX1yGLhDgUto= golang.org/x/exp v0.0.0-20190121172915-509febef88a4/go.mod h1:CJ0aWSM057203Lf6IL+f9T1iT9GByDxfZKAQTCR3kQA= golang.org/x/exp v0.0.0-20190306152737-a1d7652674e8/go.mod h1:CJ0aWSM057203Lf6IL+f9T1iT9GByDxfZKAQTCR3kQA= golang.org/x/exp v0.0.0-20190510132918-efd6b22b2522/go.mod h1:ZjyILWgesfNpC6sMxTJOJm9Kp84zZh5NQWvqDGG3Qr8= @@ -675,6 +722,8 @@ golang.org/x/oauth2 v0.0.0-20190226205417-e64efc72b421/go.mod h1:gOpvHmFTYa4Iltr golang.org/x/oauth2 v0.0.0-20190604053449-0f29369cfe45/go.mod h1:gOpvHmFTYa4IltrdGE7lF6nIHvwfUNPOp7c8zoXwtLw= golang.org/x/oauth2 v0.0.0-20191202225959-858c2ad4c8b6 h1:pE8b58s1HRDMi8RDc79m0HISf9D4TzseP40cEA6IGfs= golang.org/x/oauth2 v0.0.0-20191202225959-858c2ad4c8b6/go.mod h1:gOpvHmFTYa4IltrdGE7lF6nIHvwfUNPOp7c8zoXwtLw= +golang.org/x/oauth2 v0.0.0-20200107190931-bf48bf16ab8d h1:TzXSXBo42m9gQenoE3b9BGiEpg5IG2JkU5FkPIawgtw= +golang.org/x/oauth2 v0.0.0-20200107190931-bf48bf16ab8d/go.mod h1:gOpvHmFTYa4IltrdGE7lF6nIHvwfUNPOp7c8zoXwtLw= golang.org/x/sync v0.0.0-20180314180146-1d60e4601c6f/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sync v0.0.0-20181108010431-42b317875d0f/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sync v0.0.0-20181221193216-37e7f081c4d4/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= @@ -709,6 +758,7 @@ golang.org/x/sys v0.0.0-20191204072324-ce4227a45e2e/go.mod h1:h1NjWce9XRLGQEsW7w golang.org/x/sys v0.0.0-20191228213918-04cbcbbfeed8/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/sys v0.0.0-20200106162015-b016eb3dc98e/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/sys v0.0.0-20200116001909-b77594299b42/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= +golang.org/x/sys v0.0.0-20200124204421-9fbb57f87de9/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/sys v0.0.0-20200202164722-d101bd2416d5/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/sys v0.0.0-20200223170610-d5e6a3e2c0ae/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/sys v0.0.0-20200302150141-5c8b2ff67527/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= @@ -718,6 +768,9 @@ golang.org/x/sys v0.0.0-20200602225109-6fdc65e7d980/go.mod h1:h1NjWce9XRLGQEsW7w golang.org/x/sys v0.0.0-20200615200032-f1bc736245b1/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/sys v0.0.0-20200622214017-ed371f2e16b4 h1:5/PjkGUjvEU5Gl6BxmvKRPpqo2uNMv4rcHBMwzk/st8= golang.org/x/sys v0.0.0-20200622214017-ed371f2e16b4/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= +golang.org/x/sys v0.0.0-20200720211630-cb9d2d5c5666 h1:gVCS+QOncANNPlmlO1AhlU3oxs4V9z+gTtPwIk3p2N8= +golang.org/x/sys v0.0.0-20200720211630-cb9d2d5c5666/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= +golang.org/x/text v0.0.0-20170915032832-14c0d48ead0c/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= golang.org/x/text v0.3.1-0.20180807135948-17ff2d5776d2/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= golang.org/x/text v0.3.2/go.mod h1:bEr9sfX3Q8Zfm5fL9x+3itogRgK3+ptLWKqgva+5dAk= @@ -768,11 +821,13 @@ golang.org/x/tools v0.0.0-20200324003944-a576cf524670/go.mod h1:Sl4aGygMT6LrqrWc golang.org/x/tools v0.0.0-20200414032229-332987a829c3/go.mod h1:EkVYQZoAsY45+roYkvgYkIh4xh/qjgUK9TdY2XT94GE= golang.org/x/tools v0.0.0-20200422022333-3d57cf2e726e/go.mod h1:EkVYQZoAsY45+roYkvgYkIh4xh/qjgUK9TdY2XT94GE= golang.org/x/tools v0.0.0-20200519015757-0d0afa43d58a/go.mod h1:EkVYQZoAsY45+roYkvgYkIh4xh/qjgUK9TdY2XT94GE= +golang.org/x/tools v0.0.0-20200522201501-cb1345f3a375/go.mod h1:EkVYQZoAsY45+roYkvgYkIh4xh/qjgUK9TdY2XT94GE= golang.org/x/tools v0.0.0-20200616133436-c1934b75d054/go.mod h1:EkVYQZoAsY45+roYkvgYkIh4xh/qjgUK9TdY2XT94GE= golang.org/x/tools v0.0.0-20200625211823-6506e20df31f/go.mod h1:EkVYQZoAsY45+roYkvgYkIh4xh/qjgUK9TdY2XT94GE= golang.org/x/tools v0.0.0-20200626171337-aa94e735be7f/go.mod h1:EkVYQZoAsY45+roYkvgYkIh4xh/qjgUK9TdY2XT94GE= golang.org/x/tools v0.0.0-20200626171337-aa94e735be7f/go.mod h1:EkVYQZoAsY45+roYkvgYkIh4xh/qjgUK9TdY2XT94GE= golang.org/x/tools v0.0.0-20200701041122-1837592efa10/go.mod h1:EkVYQZoAsY45+roYkvgYkIh4xh/qjgUK9TdY2XT94GE= +golang.org/x/tools v0.0.0-20200721223218-6123e77877b2/go.mod h1:njjCfa9FT2d7l9Bc6FUM5FLjQPp3cFF28FI3qnDFljA= golang.org/x/tools v0.0.0-20200724022722-7017fd6b1305 h1:yaM5S0KcY0lIoZo7Fl+oi91b/DdlU2zuWpfHrpWbCS0= golang.org/x/tools v0.0.0-20200724022722-7017fd6b1305/go.mod h1:njjCfa9FT2d7l9Bc6FUM5FLjQPp3cFF28FI3qnDFljA= golang.org/x/tools v0.0.0-20200812195022-5ae4c3c160a0 h1:SQvH+DjrwqD1hyyQU+K7JegHz1KEZgEwt17p9d6R2eg= @@ -840,6 +895,8 @@ gopkg.in/inf.v0 v0.9.1 h1:73M5CoZyi3ZLMOyDlQh031Cx6N9NDJ2Vvfl76EDAgDc= gopkg.in/inf.v0 v0.9.1/go.mod h1:cWUDdTG/fYaXco+Dcufb5Vnc6Gp2YChqWtbxRZE0mXw= gopkg.in/ini.v1 v1.51.0 h1:AQvPpx3LzTDM0AjnIRlVFwFFGC+npRopjZxLJj6gdno= gopkg.in/ini.v1 v1.51.0/go.mod h1:pNLf8WUiyNEtQjuu5G5vTm06TEv9tsIgeAvK8hOrP4k= +gopkg.in/ini.v1 v1.57.0 h1:9unxIsFcTt4I55uWluz+UmL95q4kdJ0buvQ1ZIqVQww= +gopkg.in/ini.v1 v1.57.0/go.mod h1:pNLf8WUiyNEtQjuu5G5vTm06TEv9tsIgeAvK8hOrP4k= gopkg.in/natefinch/lumberjack.v2 v2.0.0 h1:1Lc07Kr7qY4U2YPouBjpCLxpiyxIVoxqXgkXLknAOE8= gopkg.in/natefinch/lumberjack.v2 v2.0.0/go.mod h1:l0ndWWf7gzL7RNwBG7wST/UCcT4T24xpD6X8LsfU/+k= gopkg.in/resty.v1 v1.12.0/go.mod h1:mDo4pnntr5jdWRML875a/NmxYqAlA73dVijT2AXvQQo= @@ -901,6 +958,8 @@ mvdan.cc/lint v0.0.0-20170908181259-adc824a0674b/go.mod h1:2odslEg/xrtNQqCYg2/jC mvdan.cc/unparam v0.0.0-20190720180237-d51796306d8f h1:Cq7MalBHYACRd6EesksG1Q8EoIAKOsiZviGKbOLIej4= mvdan.cc/unparam v0.0.0-20190720180237-d51796306d8f/go.mod h1:4G1h5nDURzA3bwVMZIVpwbkw+04kSxk3rAtzlimaUJw= rsc.io/binaryregexp v0.2.0/go.mod h1:qTv7/COck+e2FymRvadv62gMdZztPaShugOCi3I+8D8= +rsc.io/quote/v3 v3.1.0/go.mod h1:yEA65RcK8LyAZtP9Kv3t0HmxON59tX3rD+tICJqUlj0= +rsc.io/sampler v1.3.0/go.mod h1:T1hPZKmBbMNahiBKFy5HrXp6adAjACjK9JXDnKaTXpA= sigs.k8s.io/apiserver-network-proxy/konnectivity-client v0.0.9 h1:rusRLrDhjBp6aYtl9sGEvQJr6faoHoDLd0YcUBTZguI= sigs.k8s.io/apiserver-network-proxy/konnectivity-client v0.0.9/go.mod h1:dzAXnQbTRyDlZPJX2SUPEqvnB+j7AJjtlox7PEwigU0= sigs.k8s.io/structured-merge-diff/v4 v4.0.1 h1:YXTMot5Qz/X1iBRJhAt+vI+HVttY0WkSqqhKxQ0xVbA= diff --git a/internal/oidc/auth/auth_handler.go b/internal/oidc/auth/auth_handler.go index 247be1fb9..d5016c454 100644 --- a/internal/oidc/auth/auth_handler.go +++ b/internal/oidc/auth/auth_handler.go @@ -8,6 +8,7 @@ import ( "fmt" "net/http" + "github.com/ory/fosite" "golang.org/x/oauth2" "k8s.io/klog/v2" @@ -25,6 +26,7 @@ type IDPListGetter interface { func NewHandler( issuer string, idpListGetter IDPListGetter, + oauthHelper fosite.OAuth2Provider, generateState func() (state.State, error), generatePKCE func() (pkce.Code, error), generateNonce func() (nonce.Nonce, error), @@ -37,6 +39,15 @@ func NewHandler( return httperr.Newf(http.StatusMethodNotAllowed, "%s (try GET or POST)", r.Method) } + authorizeRequester, err := oauthHelper.NewAuthorizeRequest( + r.Context(), // TODO: maybe another context here since this one will expire? + r, + ) + if err != nil { + oauthHelper.WriteAuthorizeError(w, authorizeRequester, err) + return nil + } + upstreamIDP, err := chooseUpstreamIDP(idpListGetter) if err != nil { return err diff --git a/internal/oidc/auth/auth_handler_test.go b/internal/oidc/auth/auth_handler_test.go index 36c607d54..72acbe090 100644 --- a/internal/oidc/auth/auth_handler_test.go +++ b/internal/oidc/auth/auth_handler_test.go @@ -5,14 +5,19 @@ package auth import ( "fmt" + "mime" "net/http" "net/http/httptest" "net/url" "strings" "testing" + "github.com/ory/fosite" + "github.com/ory/fosite/compose" + "github.com/ory/fosite/storage" "github.com/stretchr/testify/require" + "go.pinniped.dev/internal/here" "go.pinniped.dev/internal/oidc/provider" "go.pinniped.dev/internal/oidcclient/nonce" "go.pinniped.dev/internal/oidcclient/pkce" @@ -20,6 +25,22 @@ import ( ) func TestAuthorizationEndpoint(t *testing.T) { + const ( + downstreamRedirectURI = "http://127.0.0.1/callback" + ) + + var ( + fositeInvalidClientErrorBody = here.Doc(` + { + "error": "invalid_client", + "error_verbose": "Client authentication failed (e.g., unknown client, no client authentication included, or unsupported authentication method)", + "error_description": "Client authentication failed (e.g., unknown client, no client authentication included, or unsupported authentication method)\n\nThe requested OAuth 2.0 Client does not exist.", + "error_hint": "The requested OAuth 2.0 Client does not exist.", + "status_code": 401 + } + `) + ) + upstreamAuthURL, err := url.Parse("https://some-upstream-idp:8443/auth") require.NoError(t, err) @@ -32,6 +53,33 @@ func TestAuthorizationEndpoint(t *testing.T) { issuer := "https://my-issuer.com/some-path" + oauthStore := &storage.MemoryStore{ + Clients: map[string]fosite.Client{ + "pinniped-cli": &fosite.DefaultOpenIDConnectClient{ + DefaultClient: &fosite.DefaultClient{ + ID: "pinniped-cli", + Public: true, + RedirectURIs: []string{downstreamRedirectURI}, + ResponseTypes: []string{"code"}, + GrantTypes: []string{"authorization_code"}, + Scopes: []string{"openid", "profile", "email"}, + }, + }, + }, + } + oauthHelper := compose.Compose( + &compose.Config{}, + oauthStore, + &compose.CommonStrategy{ + // Shouldn't need any of this - we aren't doing auth code stuff, issuing ID tokens, or signing + // anything yet. + }, + nil, // hasher, shouldn't need this - we aren't doing any client auth + compose.OAuth2AuthorizeExplicitFactory, // need this to validate generic OAuth2 stuff + compose.OpenIDConnectExplicitFactory, // need this to validate OIDC stuff + compose.OAuth2PKCEFactory, // need this to validate PKCE stuff + ) + happyStateGenerator := func() (state.State, error) { return "test-state", nil } happyPKCEGenerator := func() (pkce.Code, error) { return "test-pkce", nil } happyNonceGenerator := func() (nonce.Nonce, error) { return "test-nonce", nil } @@ -43,7 +91,7 @@ func TestAuthorizationEndpoint(t *testing.T) { happyGetRequestPath := fmt.Sprintf( "/some/path?response_type=code&scope=%s&client_id=pinniped-cli&state=some-state-value&redirect_uri=%s", url.QueryEscape("openid profile email"), - url.QueryEscape("http://127.0.0.1/callback"), + url.QueryEscape(downstreamRedirectURI), ) type testCase struct { @@ -56,11 +104,13 @@ func TestAuthorizationEndpoint(t *testing.T) { generateNonce func() (nonce.Nonce, error) method string path string + contentType string body string wantStatus int wantContentType string wantBodyString string + wantBodyJSON string wantLocationHeader string } @@ -101,12 +151,13 @@ func TestAuthorizationEndpoint(t *testing.T) { generateNonce: happyNonceGenerator, method: http.MethodPost, path: "/some/path", + contentType: "application/x-www-form-urlencoded", body: url.Values{ "response_type": []string{"code"}, "scope": []string{"openid profile email"}, "client_id": []string{"pinniped-cli"}, "state": []string{"some-state-value"}, - "redirect_uri": []string{"http://127.0.0.1/callback"}, + "redirect_uri": []string{downstreamRedirectURI}, }.Encode(), wantStatus: http.StatusFound, wantContentType: "", @@ -126,6 +177,23 @@ func TestAuthorizationEndpoint(t *testing.T) { }.Encode(), ), }, + { + name: "downstream client does not exist", + issuer: issuer, + idpListGetter: newIDPListGetter(upstreamOIDCIdentityProvider), + generateState: happyStateGenerator, + generatePKCE: happyPKCEGenerator, + generateNonce: happyNonceGenerator, + method: http.MethodGet, + path: fmt.Sprintf( + "/some/path?response_type=code&scope=%s&client_id=invalid-client&state=some-state-value&redirect_uri=%s", + url.QueryEscape("openid profile email"), + url.QueryEscape(downstreamRedirectURI), + ), + wantStatus: http.StatusUnauthorized, + wantContentType: "application/json; charset=utf-8", + wantBodyJSON: fositeInvalidClientErrorBody, + }, { name: "error while generating state", issuer: issuer, @@ -219,15 +287,22 @@ func TestAuthorizationEndpoint(t *testing.T) { runOneTestCase := func(t *testing.T, test testCase, subject http.Handler) { req := httptest.NewRequest(test.method, test.path, strings.NewReader(test.body)) + req.Header.Set("Content-Type", test.contentType) rsp := httptest.NewRecorder() subject.ServeHTTP(rsp, req) + t.Logf("response: %#v", rsp) + t.Logf("body: %q", rsp.Body.String()) + require.Equal(t, test.wantStatus, rsp.Code) - require.Equal(t, test.wantContentType, rsp.Header().Get("Content-Type")) + requireEqualContentType(t, rsp.Header().Get("Content-Type"), test.wantContentType) if test.wantBodyString != "" { require.Equal(t, test.wantBodyString, rsp.Body.String()) } + if test.wantBodyJSON != "" { + require.JSONEq(t, test.wantBodyJSON, rsp.Body.String()) + } if test.wantLocationHeader != "" { actualLocation := rsp.Header().Get("Location") @@ -238,7 +313,7 @@ func TestAuthorizationEndpoint(t *testing.T) { for _, test := range tests { test := test t.Run(test.name, func(t *testing.T) { - subject := NewHandler(test.issuer, test.idpListGetter, test.generateState, test.generatePKCE, test.generateNonce) + subject := NewHandler(test.issuer, test.idpListGetter, oauthHelper, test.generateState, test.generatePKCE, test.generateNonce) runOneTestCase(t, test, subject) }) } @@ -247,7 +322,7 @@ func TestAuthorizationEndpoint(t *testing.T) { test := tests[0] require.Equal(t, "happy path using GET", test.name) // re-use the happy path test case - subject := NewHandler(test.issuer, test.idpListGetter, test.generateState, test.generatePKCE, test.generateNonce) + subject := NewHandler(test.issuer, test.idpListGetter, oauthHelper, test.generateState, test.generatePKCE, test.generateNonce) runOneTestCase(t, test, subject) @@ -284,7 +359,24 @@ func TestAuthorizationEndpoint(t *testing.T) { }) } +func requireEqualContentType(t *testing.T, actual string, expected string) { + t.Helper() + + if expected == "" { + require.Empty(t, actual) + return + } + + actualContentType, actualContentTypeParams, err := mime.ParseMediaType(expected) + require.NoError(t, err) + expectedContentType, expectedContentTypeParams, err := mime.ParseMediaType(expected) + require.NoError(t, err) + require.Equal(t, actualContentType, expectedContentType) + require.Equal(t, actualContentTypeParams, expectedContentTypeParams) +} + func requireEqualURLs(t *testing.T, actualURL string, expectedURL string) { + t.Helper() actualLocationURL, err := url.Parse(actualURL) require.NoError(t, err) expectedLocationURL, err := url.Parse(expectedURL) From 4f95e6a372db1b43f991fdb8564a0332a06cab77 Mon Sep 17 00:00:00 2001 From: Andrew Keesler Date: Wed, 4 Nov 2020 10:30:53 -0500 Subject: [PATCH 03/21] auth_handler.go: add test for invalid downstream redirect uri Signed-off-by: Andrew Keesler --- internal/oidc/auth/auth_handler_test.go | 27 +++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/internal/oidc/auth/auth_handler_test.go b/internal/oidc/auth/auth_handler_test.go index 72acbe090..09bb192b8 100644 --- a/internal/oidc/auth/auth_handler_test.go +++ b/internal/oidc/auth/auth_handler_test.go @@ -39,6 +39,16 @@ func TestAuthorizationEndpoint(t *testing.T) { "status_code": 401 } `) + + fositeInvalidRedirectURIErrorBody = here.Doc(` + { + "error": "invalid_request", + "error_verbose": "The request is missing a required parameter, includes an invalid parameter value, includes a parameter more than once, or is otherwise malformed", + "error_description": "The request is missing a required parameter, includes an invalid parameter value, includes a parameter more than once, or is otherwise malformed\n\nThe \"redirect_uri\" parameter does not match any of the OAuth 2.0 Client's pre-registered redirect urls.", + "error_hint": "The \"redirect_uri\" parameter does not match any of the OAuth 2.0 Client's pre-registered redirect urls.", + "status_code": 400 + } + `) ) upstreamAuthURL, err := url.Parse("https://some-upstream-idp:8443/auth") @@ -194,6 +204,23 @@ func TestAuthorizationEndpoint(t *testing.T) { wantContentType: "application/json; charset=utf-8", wantBodyJSON: fositeInvalidClientErrorBody, }, + { + name: "downstream redirect uri does not match what is configured for client", + issuer: issuer, + idpListGetter: newIDPListGetter(upstreamOIDCIdentityProvider), + generateState: happyStateGenerator, + generatePKCE: happyPKCEGenerator, + generateNonce: happyNonceGenerator, + method: http.MethodGet, + path: fmt.Sprintf( + "/some/path?response_type=code&scope=%s&client_id=pinniped-cli&state=some-state-value&redirect_uri=%s", + url.QueryEscape("openid profile email"), + url.QueryEscape("http://127.0.0.1/does-not-match-what-is-configured-for-pinniped-cli-client"), + ), + wantStatus: http.StatusBadRequest, + wantContentType: "application/json; charset=utf-8", + wantBodyJSON: fositeInvalidRedirectURIErrorBody, + }, { name: "error while generating state", issuer: issuer, From e8f433643fa023d6d9375a600db436eb4d3ffc4e Mon Sep 17 00:00:00 2001 From: Andrew Keesler Date: Wed, 4 Nov 2020 10:35:26 -0500 Subject: [PATCH 04/21] auth_handler.go: only inject oauth store into handler Previously we were injecting the whole oauth handler chain into this function, which meant we were essentially writing unit tests to test our tests. Let's push some of this logic into the source code. Signed-off-by: Andrew Keesler --- internal/oidc/auth/auth_handler.go | 16 ++++++++++++++-- internal/oidc/auth/auth_handler_test.go | 17 ++--------------- 2 files changed, 16 insertions(+), 17 deletions(-) diff --git a/internal/oidc/auth/auth_handler.go b/internal/oidc/auth/auth_handler.go index d5016c454..144a8fc19 100644 --- a/internal/oidc/auth/auth_handler.go +++ b/internal/oidc/auth/auth_handler.go @@ -8,7 +8,7 @@ import ( "fmt" "net/http" - "github.com/ory/fosite" + "github.com/ory/fosite/compose" "golang.org/x/oauth2" "k8s.io/klog/v2" @@ -26,11 +26,23 @@ type IDPListGetter interface { func NewHandler( issuer string, idpListGetter IDPListGetter, - oauthHelper fosite.OAuth2Provider, + oauthStore interface{}, generateState func() (state.State, error), generatePKCE func() (pkce.Code, error), generateNonce func() (nonce.Nonce, error), ) http.Handler { + oauthHelper := compose.Compose( + &compose.Config{}, + oauthStore, + &compose.CommonStrategy{ + // Shouldn't need any of this - we aren't doing auth code stuff, issuing ID tokens, or signing + // anything yet. + }, + nil, // hasher, shouldn't need this - we aren't doing any client auth...yet? + compose.OAuth2AuthorizeExplicitFactory, + compose.OpenIDConnectExplicitFactory, + compose.OAuth2PKCEFactory, + ) return httperr.HandlerFunc(func(w http.ResponseWriter, r *http.Request) error { if r.Method != http.MethodPost && r.Method != http.MethodGet { // https://openid.net/specs/openid-connect-core-1_0.html#AuthRequest diff --git a/internal/oidc/auth/auth_handler_test.go b/internal/oidc/auth/auth_handler_test.go index 09bb192b8..1b6282da6 100644 --- a/internal/oidc/auth/auth_handler_test.go +++ b/internal/oidc/auth/auth_handler_test.go @@ -13,7 +13,6 @@ import ( "testing" "github.com/ory/fosite" - "github.com/ory/fosite/compose" "github.com/ory/fosite/storage" "github.com/stretchr/testify/require" @@ -77,18 +76,6 @@ func TestAuthorizationEndpoint(t *testing.T) { }, }, } - oauthHelper := compose.Compose( - &compose.Config{}, - oauthStore, - &compose.CommonStrategy{ - // Shouldn't need any of this - we aren't doing auth code stuff, issuing ID tokens, or signing - // anything yet. - }, - nil, // hasher, shouldn't need this - we aren't doing any client auth - compose.OAuth2AuthorizeExplicitFactory, // need this to validate generic OAuth2 stuff - compose.OpenIDConnectExplicitFactory, // need this to validate OIDC stuff - compose.OAuth2PKCEFactory, // need this to validate PKCE stuff - ) happyStateGenerator := func() (state.State, error) { return "test-state", nil } happyPKCEGenerator := func() (pkce.Code, error) { return "test-pkce", nil } @@ -340,7 +327,7 @@ func TestAuthorizationEndpoint(t *testing.T) { for _, test := range tests { test := test t.Run(test.name, func(t *testing.T) { - subject := NewHandler(test.issuer, test.idpListGetter, oauthHelper, test.generateState, test.generatePKCE, test.generateNonce) + subject := NewHandler(test.issuer, test.idpListGetter, oauthStore, test.generateState, test.generatePKCE, test.generateNonce) runOneTestCase(t, test, subject) }) } @@ -349,7 +336,7 @@ func TestAuthorizationEndpoint(t *testing.T) { test := tests[0] require.Equal(t, "happy path using GET", test.name) // re-use the happy path test case - subject := NewHandler(test.issuer, test.idpListGetter, oauthHelper, test.generateState, test.generatePKCE, test.generateNonce) + subject := NewHandler(test.issuer, test.idpListGetter, oauthStore, test.generateState, test.generatePKCE, test.generateNonce) runOneTestCase(t, test, subject) From d8c8f0486049ee4f4f1d5ef01b00793af38bd374 Mon Sep 17 00:00:00 2001 From: Andrew Keesler Date: Wed, 4 Nov 2020 11:12:26 -0500 Subject: [PATCH 05/21] auth_handler.go: write some more negative tests Signed-off-by: Andrew Keesler --- internal/oidc/auth/auth_handler_test.go | 113 ++++++++++++++++++++++++ 1 file changed, 113 insertions(+) diff --git a/internal/oidc/auth/auth_handler_test.go b/internal/oidc/auth/auth_handler_test.go index 1b6282da6..08f1c99ba 100644 --- a/internal/oidc/auth/auth_handler_test.go +++ b/internal/oidc/auth/auth_handler_test.go @@ -48,6 +48,34 @@ func TestAuthorizationEndpoint(t *testing.T) { "status_code": 400 } `) + + fositeUnsupportedResponseTypeErrorQuery = url.Values{ + "error": []string{"unsupported_response_type"}, + "error_description": []string{"The authorization server does not support obtaining a token using this method\n\nThe client is not allowed to request response_type \"unsupported\"."}, + "error_hint": []string{`The client is not allowed to request response_type "unsupported".`}, + "state": []string{"some-state-value"}, + }.Encode() + + fositeInvalidScopeErrorQuery = url.Values{ + "error": []string{"invalid_scope"}, + "error_description": []string{"The requested scope is invalid, unknown, or malformed\n\nThe OAuth 2.0 Client is not allowed to request scope \"tuna\"."}, + "error_hint": []string{`The OAuth 2.0 Client is not allowed to request scope "tuna".`}, + "state": []string{"some-state-value"}, + }.Encode() + + fositeInvalidStateErrorQuery = url.Values{ + "error": []string{"invalid_state"}, + "error_description": []string{"The state is missing or does not have enough characters and is therefore considered too weak\n\nRequest parameter \"state\" must be at least be 8 characters long to ensure sufficient entropy."}, + "error_hint": []string{`Request parameter "state" must be at least be 8 characters long to ensure sufficient entropy.`}, + "state": []string{"short"}, + }.Encode() + + fositeMissingResponseTypeErrorQuery = url.Values{ + "error": []string{"unsupported_response_type"}, + "error_description": []string{"The authorization server does not support obtaining a token using this method\n\nThe request is missing the \"response_type\"\" parameter."}, + "error_hint": []string{`The request is missing the "response_type"" parameter.`}, + "state": []string{"some-state-value"}, + }.Encode() ) upstreamAuthURL, err := url.Parse("https://some-upstream-idp:8443/auth") @@ -208,6 +236,91 @@ func TestAuthorizationEndpoint(t *testing.T) { wantContentType: "application/json; charset=utf-8", wantBodyJSON: fositeInvalidRedirectURIErrorBody, }, + { + name: "response type is unsupported", + issuer: issuer, + idpListGetter: newIDPListGetter(upstreamOIDCIdentityProvider), + generateState: happyStateGenerator, + generatePKCE: happyPKCEGenerator, + generateNonce: happyNonceGenerator, + method: http.MethodGet, + path: fmt.Sprintf( + "/some/path?response_type=unsupported&scope=%s&client_id=pinniped-cli&state=some-state-value&redirect_uri=%s", + url.QueryEscape("openid profile email"), + url.QueryEscape(downstreamRedirectURI), + ), + wantStatus: http.StatusFound, + wantContentType: "application/json; charset=utf-8", + wantLocationHeader: fmt.Sprintf("%s?%s", downstreamRedirectURI, fositeUnsupportedResponseTypeErrorQuery), + }, + { + name: "downstream scopes do not match what is configured for client", + issuer: issuer, + idpListGetter: newIDPListGetter(upstreamOIDCIdentityProvider), + generateState: happyStateGenerator, + generatePKCE: happyPKCEGenerator, + generateNonce: happyNonceGenerator, + method: http.MethodGet, + path: fmt.Sprintf( + "/some/path?response_type=code&scope=%s&client_id=pinniped-cli&state=some-state-value&redirect_uri=%s", + url.QueryEscape("openid profile email tuna"), + url.QueryEscape(downstreamRedirectURI), + ), + wantStatus: http.StatusFound, + wantContentType: "application/json; charset=utf-8", + wantLocationHeader: fmt.Sprintf("%s?%s", downstreamRedirectURI, fositeInvalidScopeErrorQuery), + }, + { + name: "missing response type in request", + issuer: issuer, + idpListGetter: newIDPListGetter(upstreamOIDCIdentityProvider), + generateState: happyStateGenerator, + generatePKCE: happyPKCEGenerator, + generateNonce: happyNonceGenerator, + method: http.MethodGet, + path: fmt.Sprintf( + "/some/path?scope=%s&client_id=pinniped-cli&state=some-state-value&redirect_uri=%s", + url.QueryEscape("openid profile email"), + url.QueryEscape(downstreamRedirectURI), + ), + wantStatus: http.StatusFound, + wantContentType: "application/json; charset=utf-8", + wantLocationHeader: fmt.Sprintf("%s?%s", downstreamRedirectURI, fositeMissingResponseTypeErrorQuery), + }, + { + name: "missing client id in request", + issuer: issuer, + idpListGetter: newIDPListGetter(upstreamOIDCIdentityProvider), + generateState: happyStateGenerator, + generatePKCE: happyPKCEGenerator, + generateNonce: happyNonceGenerator, + method: http.MethodGet, + path: fmt.Sprintf( + "/some/path?response_type=code&scope=%s&state=some-state-value&redirect_uri=%s", + url.QueryEscape("openid profile email"), + url.QueryEscape(downstreamRedirectURI), + ), + wantStatus: http.StatusUnauthorized, + wantContentType: "application/json; charset=utf-8", + wantBodyJSON: fositeInvalidClientErrorBody, + }, + { + name: "state does not have enough entropy", + issuer: issuer, + idpListGetter: newIDPListGetter(upstreamOIDCIdentityProvider), + generateState: happyStateGenerator, + generatePKCE: happyPKCEGenerator, + generateNonce: happyNonceGenerator, + method: http.MethodGet, + path: fmt.Sprintf( + "/some/path?response_type=code&scope=%s&client_id=pinniped-cli&state=short&redirect_uri=%s", + url.QueryEscape("openid profile email"), + url.QueryEscape(downstreamRedirectURI), + ), + wantStatus: http.StatusFound, + wantContentType: "application/json; charset=utf-8", + wantLocationHeader: fmt.Sprintf("%s?%s", downstreamRedirectURI, fositeInvalidStateErrorQuery), + }, { name: "error while generating state", issuer: issuer, From 6fe455c68749ef3302c9959619d5993ffce8454e Mon Sep 17 00:00:00 2001 From: Andrew Keesler Date: Wed, 4 Nov 2020 11:20:03 -0500 Subject: [PATCH 06/21] auth_handler.go: comment out currently unused fosite wiring See e8f4336 for why this is here in the first place. Signed-off-by: Andrew Keesler --- internal/oidc/auth/auth_handler.go | 26 ++++++++++++++++++-------- 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/internal/oidc/auth/auth_handler.go b/internal/oidc/auth/auth_handler.go index 144a8fc19..d46384af9 100644 --- a/internal/oidc/auth/auth_handler.go +++ b/internal/oidc/auth/auth_handler.go @@ -32,16 +32,26 @@ func NewHandler( generateNonce func() (nonce.Nonce, error), ) http.Handler { oauthHelper := compose.Compose( + // Empty Config for right now since we aren't using anything in it. We may want to inject this + // in the future since it has some really nice configuration knobs like token lifetime. &compose.Config{}, + + // This is the thing that matters right now - the store is used to get information about the + // client in the authorization request. oauthStore, - &compose.CommonStrategy{ - // Shouldn't need any of this - we aren't doing auth code stuff, issuing ID tokens, or signing - // anything yet. - }, - nil, // hasher, shouldn't need this - we aren't doing any client auth...yet? - compose.OAuth2AuthorizeExplicitFactory, - compose.OpenIDConnectExplicitFactory, - compose.OAuth2PKCEFactory, + + // Shouldn't need any of this filled in as of right now - we aren't doing auth code stuff, + // issuing ID tokens, or signing anything yet. + &compose.CommonStrategy{}, + + // hasher, shouldn't need this right now - we aren't doing any client auth...yet? + nil, + + // We will _probably_ want the below handlers somewhere in the code, but I'm not sure where yet, + // and we don't need them for the tests to pass currently, so they are commented out. + // compose.OAuth2AuthorizeExplicitFactory, + // compose.OpenIDConnectExplicitFactory, + // compose.OAuth2PKCEFactory, ) return httperr.HandlerFunc(func(w http.ResponseWriter, r *http.Request) error { if r.Method != http.MethodPost && r.Method != http.MethodGet { From 9e4ffd1cce16597b9484ed1dba96bc757c55932e Mon Sep 17 00:00:00 2001 From: Andrew Keesler Date: Wed, 4 Nov 2020 11:29:33 -0500 Subject: [PATCH 07/21] One of these days I will get here.Doc() spacing correct Signed-off-by: Andrew Keesler --- internal/oidc/auth/auth_handler_test.go | 28 ++++++++++++------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/internal/oidc/auth/auth_handler_test.go b/internal/oidc/auth/auth_handler_test.go index 08f1c99ba..58a8a221a 100644 --- a/internal/oidc/auth/auth_handler_test.go +++ b/internal/oidc/auth/auth_handler_test.go @@ -30,23 +30,23 @@ func TestAuthorizationEndpoint(t *testing.T) { var ( fositeInvalidClientErrorBody = here.Doc(` - { - "error": "invalid_client", - "error_verbose": "Client authentication failed (e.g., unknown client, no client authentication included, or unsupported authentication method)", - "error_description": "Client authentication failed (e.g., unknown client, no client authentication included, or unsupported authentication method)\n\nThe requested OAuth 2.0 Client does not exist.", - "error_hint": "The requested OAuth 2.0 Client does not exist.", - "status_code": 401 - } + { + "error": "invalid_client", + "error_verbose": "Client authentication failed (e.g., unknown client, no client authentication included, or unsupported authentication method)", + "error_description": "Client authentication failed (e.g., unknown client, no client authentication included, or unsupported authentication method)\n\nThe requested OAuth 2.0 Client does not exist.", + "error_hint": "The requested OAuth 2.0 Client does not exist.", + "status_code": 401 + } `) fositeInvalidRedirectURIErrorBody = here.Doc(` - { - "error": "invalid_request", - "error_verbose": "The request is missing a required parameter, includes an invalid parameter value, includes a parameter more than once, or is otherwise malformed", - "error_description": "The request is missing a required parameter, includes an invalid parameter value, includes a parameter more than once, or is otherwise malformed\n\nThe \"redirect_uri\" parameter does not match any of the OAuth 2.0 Client's pre-registered redirect urls.", - "error_hint": "The \"redirect_uri\" parameter does not match any of the OAuth 2.0 Client's pre-registered redirect urls.", - "status_code": 400 - } + { + "error": "invalid_request", + "error_verbose": "The request is missing a required parameter, includes an invalid parameter value, includes a parameter more than once, or is otherwise malformed", + "error_description": "The request is missing a required parameter, includes an invalid parameter value, includes a parameter more than once, or is otherwise malformed\n\nThe \"redirect_uri\" parameter does not match any of the OAuth 2.0 Client's pre-registered redirect urls.", + "error_hint": "The \"redirect_uri\" parameter does not match any of the OAuth 2.0 Client's pre-registered redirect urls.", + "status_code": 400 + } `) fositeUnsupportedResponseTypeErrorQuery = url.Values{ From 8a7e22e63e93c231dc094c2bc64627825bd0f28d Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Wed, 4 Nov 2020 08:43:45 -0800 Subject: [PATCH 08/21] @ankeesler: Maybe, but not this time ;) --- internal/oidc/auth/auth_handler_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/oidc/auth/auth_handler_test.go b/internal/oidc/auth/auth_handler_test.go index 58a8a221a..f43d23b81 100644 --- a/internal/oidc/auth/auth_handler_test.go +++ b/internal/oidc/auth/auth_handler_test.go @@ -37,7 +37,7 @@ func TestAuthorizationEndpoint(t *testing.T) { "error_hint": "The requested OAuth 2.0 Client does not exist.", "status_code": 401 } - `) + `) fositeInvalidRedirectURIErrorBody = here.Doc(` { @@ -47,7 +47,7 @@ func TestAuthorizationEndpoint(t *testing.T) { "error_hint": "The \"redirect_uri\" parameter does not match any of the OAuth 2.0 Client's pre-registered redirect urls.", "status_code": 400 } - `) + `) fositeUnsupportedResponseTypeErrorQuery = url.Values{ "error": []string{"unsupported_response_type"}, From 0045ce42866d1bbb37f2ce59b591dcb35e85df99 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Wed, 4 Nov 2020 09:58:40 -0800 Subject: [PATCH 09/21] Refactor auth_handler_test.go's creation of paths and urls to use helpers --- internal/oidc/auth/auth_handler_test.go | 335 ++++++++++++------------ 1 file changed, 169 insertions(+), 166 deletions(-) diff --git a/internal/oidc/auth/auth_handler_test.go b/internal/oidc/auth/auth_handler_test.go index f43d23b81..d4bee4835 100644 --- a/internal/oidc/auth/auth_handler_test.go +++ b/internal/oidc/auth/auth_handler_test.go @@ -49,33 +49,33 @@ func TestAuthorizationEndpoint(t *testing.T) { } `) - fositeUnsupportedResponseTypeErrorQuery = url.Values{ - "error": []string{"unsupported_response_type"}, - "error_description": []string{"The authorization server does not support obtaining a token using this method\n\nThe client is not allowed to request response_type \"unsupported\"."}, - "error_hint": []string{`The client is not allowed to request response_type "unsupported".`}, - "state": []string{"some-state-value"}, - }.Encode() + fositeUnsupportedResponseTypeErrorQuery = map[string]string{ + "error": "unsupported_response_type", + "error_description": "The authorization server does not support obtaining a token using this method\n\nThe client is not allowed to request response_type \"unsupported\".", + "error_hint": `The client is not allowed to request response_type "unsupported".`, + "state": "some-state-value", + } - fositeInvalidScopeErrorQuery = url.Values{ - "error": []string{"invalid_scope"}, - "error_description": []string{"The requested scope is invalid, unknown, or malformed\n\nThe OAuth 2.0 Client is not allowed to request scope \"tuna\"."}, - "error_hint": []string{`The OAuth 2.0 Client is not allowed to request scope "tuna".`}, - "state": []string{"some-state-value"}, - }.Encode() + fositeInvalidScopeErrorQuery = map[string]string{ + "error": "invalid_scope", + "error_description": "The requested scope is invalid, unknown, or malformed\n\nThe OAuth 2.0 Client is not allowed to request scope \"tuna\".", + "error_hint": `The OAuth 2.0 Client is not allowed to request scope "tuna".`, + "state": "some-state-value", + } - fositeInvalidStateErrorQuery = url.Values{ - "error": []string{"invalid_state"}, - "error_description": []string{"The state is missing or does not have enough characters and is therefore considered too weak\n\nRequest parameter \"state\" must be at least be 8 characters long to ensure sufficient entropy."}, - "error_hint": []string{`Request parameter "state" must be at least be 8 characters long to ensure sufficient entropy.`}, - "state": []string{"short"}, - }.Encode() + fositeInvalidStateErrorQuery = map[string]string{ + "error": "invalid_state", + "error_description": "The state is missing or does not have enough characters and is therefore considered too weak\n\nRequest parameter \"state\" must be at least be 8 characters long to ensure sufficient entropy.", + "error_hint": `Request parameter "state" must be at least be 8 characters long to ensure sufficient entropy.`, + "state": "short", + } - fositeMissingResponseTypeErrorQuery = url.Values{ - "error": []string{"unsupported_response_type"}, - "error_description": []string{"The authorization server does not support obtaining a token using this method\n\nThe request is missing the \"response_type\"\" parameter."}, - "error_hint": []string{`The request is missing the "response_type"" parameter.`}, - "state": []string{"some-state-value"}, - }.Encode() + fositeMissingResponseTypeErrorQuery = map[string]string{ + "error": "unsupported_response_type", + "error_description": "The authorization server does not support obtaining a token using this method\n\nThe request is missing the \"response_type\"\" parameter.", + "error_hint": `The request is missing the "response_type"" parameter.`, + "state": "some-state-value", + } ) upstreamAuthURL, err := url.Parse("https://some-upstream-idp:8443/auth") @@ -113,10 +113,66 @@ func TestAuthorizationEndpoint(t *testing.T) { // $ echo -n test-pkce | shasum -a 256 | cut -d" " -f1 | xxd -r -p | base64 | cut -d"=" -f1 expectedCodeChallenge := "VVaezYqum7reIhoavCHD1n2d-piN3r_mywoYj7fCR7g" - happyGetRequestPath := fmt.Sprintf( - "/some/path?response_type=code&scope=%s&client_id=pinniped-cli&state=some-state-value&redirect_uri=%s", - url.QueryEscape("openid profile email"), - url.QueryEscape(downstreamRedirectURI), + pathWithQuery := func(path string, query map[string]string) string { + values := url.Values{} + for k, v := range query { + values[k] = []string{v} + } + pathToReturn := fmt.Sprintf("%s?%s", path, values.Encode()) + require.NotRegexp(t, "^http", pathToReturn, "pathWithQuery helper was used to create a URL") + return pathToReturn + } + + urlWithQuery := func(baseURL string, query map[string]string) string { + values := url.Values{} + for k, v := range query { + values[k] = []string{v} + } + urlToReturn := fmt.Sprintf("%s?%s", baseURL, values.Encode()) + _, err := url.Parse(urlToReturn) + require.NoError(t, err, "urlWithQuery helper was used to create an illegal URL") + return urlToReturn + } + + happyGetRequestQueryMap := map[string]string{ + "response_type": "code", + "scope": "openid profile email", + "client_id": "pinniped-cli", + "state": "some-state-value", + "nonce": "some-nonce-value", + "redirect_uri": downstreamRedirectURI, + } + + happyGetRequestPath := pathWithQuery("/some/path", happyGetRequestQueryMap) + + modifiedHappyGetRequestPath := func(queryOverrides map[string]string) string { + copyOfHappyGetRequestQueryMap := map[string]string{} + for k, v := range happyGetRequestQueryMap { + copyOfHappyGetRequestQueryMap[k] = v + } + for k, v := range queryOverrides { + _, hasKey := copyOfHappyGetRequestQueryMap[k] + if v == "" && hasKey { + delete(copyOfHappyGetRequestQueryMap, k) + } else { + copyOfHappyGetRequestQueryMap[k] = v + } + } + return pathWithQuery("/some/path", copyOfHappyGetRequestQueryMap) + } + + happyGetRequestExpectedRedirectLocation := urlWithQuery(upstreamAuthURL.String(), + map[string]string{ + "response_type": "code", + "access_type": "offline", + "scope": "scope1 scope2", + "client_id": "some-client-id", + "state": "test-state", + "nonce": "test-nonce", + "code_challenge": expectedCodeChallenge, + "code_challenge_method": "S256", + "redirect_uri": issuer + "/callback/some-idp", + }, ) type testCase struct { @@ -141,31 +197,18 @@ func TestAuthorizationEndpoint(t *testing.T) { tests := []testCase{ { - name: "happy path using GET", - issuer: issuer, - idpListGetter: newIDPListGetter(upstreamOIDCIdentityProvider), - generateState: happyStateGenerator, - generatePKCE: happyPKCEGenerator, - generateNonce: happyNonceGenerator, - method: http.MethodGet, - path: happyGetRequestPath, - wantStatus: http.StatusFound, - wantContentType: "text/html; charset=utf-8", - wantBodyString: "", - wantLocationHeader: fmt.Sprintf("%s?%s", - upstreamAuthURL.String(), - url.Values{ - "response_type": []string{"code"}, - "access_type": []string{"offline"}, - "scope": []string{"scope1 scope2"}, - "client_id": []string{"some-client-id"}, - "state": []string{"test-state"}, - "nonce": []string{"test-nonce"}, - "code_challenge": []string{expectedCodeChallenge}, - "code_challenge_method": []string{"S256"}, - "redirect_uri": []string{issuer + "/callback/some-idp"}, - }.Encode(), - ), + name: "happy path using GET", + issuer: issuer, + idpListGetter: newIDPListGetter(upstreamOIDCIdentityProvider), + generateState: happyStateGenerator, + generatePKCE: happyPKCEGenerator, + generateNonce: happyNonceGenerator, + method: http.MethodGet, + path: happyGetRequestPath, + wantStatus: http.StatusFound, + wantContentType: "text/html; charset=utf-8", + wantBodyString: "", + wantLocationHeader: happyGetRequestExpectedRedirectLocation, }, { name: "happy path using POST", @@ -184,37 +227,20 @@ func TestAuthorizationEndpoint(t *testing.T) { "state": []string{"some-state-value"}, "redirect_uri": []string{downstreamRedirectURI}, }.Encode(), - wantStatus: http.StatusFound, - wantContentType: "", - wantBodyString: "", - wantLocationHeader: fmt.Sprintf("%s?%s", - upstreamAuthURL.String(), - url.Values{ - "response_type": []string{"code"}, - "access_type": []string{"offline"}, - "scope": []string{"scope1 scope2"}, - "client_id": []string{"some-client-id"}, - "state": []string{"test-state"}, - "nonce": []string{"test-nonce"}, - "code_challenge": []string{expectedCodeChallenge}, - "code_challenge_method": []string{"S256"}, - "redirect_uri": []string{issuer + "/callback/some-idp"}, - }.Encode(), - ), + wantStatus: http.StatusFound, + wantContentType: "", + wantBodyString: "", + wantLocationHeader: happyGetRequestExpectedRedirectLocation, }, { - name: "downstream client does not exist", - issuer: issuer, - idpListGetter: newIDPListGetter(upstreamOIDCIdentityProvider), - generateState: happyStateGenerator, - generatePKCE: happyPKCEGenerator, - generateNonce: happyNonceGenerator, - method: http.MethodGet, - path: fmt.Sprintf( - "/some/path?response_type=code&scope=%s&client_id=invalid-client&state=some-state-value&redirect_uri=%s", - url.QueryEscape("openid profile email"), - url.QueryEscape(downstreamRedirectURI), - ), + name: "downstream client does not exist", + issuer: issuer, + idpListGetter: newIDPListGetter(upstreamOIDCIdentityProvider), + generateState: happyStateGenerator, + generatePKCE: happyPKCEGenerator, + generateNonce: happyNonceGenerator, + method: http.MethodGet, + path: modifiedHappyGetRequestPath(map[string]string{"client_id": "invalid-client"}), wantStatus: http.StatusUnauthorized, wantContentType: "application/json; charset=utf-8", wantBodyJSON: fositeInvalidClientErrorBody, @@ -227,99 +253,77 @@ func TestAuthorizationEndpoint(t *testing.T) { generatePKCE: happyPKCEGenerator, generateNonce: happyNonceGenerator, method: http.MethodGet, - path: fmt.Sprintf( - "/some/path?response_type=code&scope=%s&client_id=pinniped-cli&state=some-state-value&redirect_uri=%s", - url.QueryEscape("openid profile email"), - url.QueryEscape("http://127.0.0.1/does-not-match-what-is-configured-for-pinniped-cli-client"), - ), + path: modifiedHappyGetRequestPath(map[string]string{ + "redirect_uri": "http://127.0.0.1/does-not-match-what-is-configured-for-pinniped-cli-client", + }), wantStatus: http.StatusBadRequest, wantContentType: "application/json; charset=utf-8", wantBodyJSON: fositeInvalidRedirectURIErrorBody, }, { - name: "response type is unsupported", - issuer: issuer, - idpListGetter: newIDPListGetter(upstreamOIDCIdentityProvider), - generateState: happyStateGenerator, - generatePKCE: happyPKCEGenerator, - generateNonce: happyNonceGenerator, - method: http.MethodGet, - path: fmt.Sprintf( - "/some/path?response_type=unsupported&scope=%s&client_id=pinniped-cli&state=some-state-value&redirect_uri=%s", - url.QueryEscape("openid profile email"), - url.QueryEscape(downstreamRedirectURI), - ), + name: "response type is unsupported", + issuer: issuer, + idpListGetter: newIDPListGetter(upstreamOIDCIdentityProvider), + generateState: happyStateGenerator, + generatePKCE: happyPKCEGenerator, + generateNonce: happyNonceGenerator, + method: http.MethodGet, + path: modifiedHappyGetRequestPath(map[string]string{"response_type": "unsupported"}), wantStatus: http.StatusFound, wantContentType: "application/json; charset=utf-8", - wantLocationHeader: fmt.Sprintf("%s?%s", downstreamRedirectURI, fositeUnsupportedResponseTypeErrorQuery), + wantLocationHeader: urlWithQuery(downstreamRedirectURI, fositeUnsupportedResponseTypeErrorQuery), }, { - name: "downstream scopes do not match what is configured for client", - issuer: issuer, - idpListGetter: newIDPListGetter(upstreamOIDCIdentityProvider), - generateState: happyStateGenerator, - generatePKCE: happyPKCEGenerator, - generateNonce: happyNonceGenerator, - method: http.MethodGet, - path: fmt.Sprintf( - "/some/path?response_type=code&scope=%s&client_id=pinniped-cli&state=some-state-value&redirect_uri=%s", - url.QueryEscape("openid profile email tuna"), - url.QueryEscape(downstreamRedirectURI), - ), + name: "downstream scopes do not match what is configured for client", + issuer: issuer, + idpListGetter: newIDPListGetter(upstreamOIDCIdentityProvider), + generateState: happyStateGenerator, + generatePKCE: happyPKCEGenerator, + generateNonce: happyNonceGenerator, + method: http.MethodGet, + path: modifiedHappyGetRequestPath(map[string]string{"scope": "openid profile email tuna"}), wantStatus: http.StatusFound, wantContentType: "application/json; charset=utf-8", - wantLocationHeader: fmt.Sprintf("%s?%s", downstreamRedirectURI, fositeInvalidScopeErrorQuery), + wantLocationHeader: urlWithQuery(downstreamRedirectURI, fositeInvalidScopeErrorQuery), }, { - name: "missing response type in request", - issuer: issuer, - idpListGetter: newIDPListGetter(upstreamOIDCIdentityProvider), - generateState: happyStateGenerator, - generatePKCE: happyPKCEGenerator, - generateNonce: happyNonceGenerator, - method: http.MethodGet, - path: fmt.Sprintf( - "/some/path?scope=%s&client_id=pinniped-cli&state=some-state-value&redirect_uri=%s", - url.QueryEscape("openid profile email"), - url.QueryEscape(downstreamRedirectURI), - ), + name: "missing response type in request", + issuer: issuer, + idpListGetter: newIDPListGetter(upstreamOIDCIdentityProvider), + generateState: happyStateGenerator, + generatePKCE: happyPKCEGenerator, + generateNonce: happyNonceGenerator, + method: http.MethodGet, + path: modifiedHappyGetRequestPath(map[string]string{"response_type": ""}), wantStatus: http.StatusFound, wantContentType: "application/json; charset=utf-8", - wantLocationHeader: fmt.Sprintf("%s?%s", downstreamRedirectURI, fositeMissingResponseTypeErrorQuery), + wantLocationHeader: urlWithQuery(downstreamRedirectURI, fositeMissingResponseTypeErrorQuery), }, { - name: "missing client id in request", - issuer: issuer, - idpListGetter: newIDPListGetter(upstreamOIDCIdentityProvider), - generateState: happyStateGenerator, - generatePKCE: happyPKCEGenerator, - generateNonce: happyNonceGenerator, - method: http.MethodGet, - path: fmt.Sprintf( - "/some/path?response_type=code&scope=%s&state=some-state-value&redirect_uri=%s", - url.QueryEscape("openid profile email"), - url.QueryEscape(downstreamRedirectURI), - ), + name: "missing client id in request", + issuer: issuer, + idpListGetter: newIDPListGetter(upstreamOIDCIdentityProvider), + generateState: happyStateGenerator, + generatePKCE: happyPKCEGenerator, + generateNonce: happyNonceGenerator, + method: http.MethodGet, + path: modifiedHappyGetRequestPath(map[string]string{"client_id": ""}), wantStatus: http.StatusUnauthorized, wantContentType: "application/json; charset=utf-8", wantBodyJSON: fositeInvalidClientErrorBody, }, { - name: "state does not have enough entropy", - issuer: issuer, - idpListGetter: newIDPListGetter(upstreamOIDCIdentityProvider), - generateState: happyStateGenerator, - generatePKCE: happyPKCEGenerator, - generateNonce: happyNonceGenerator, - method: http.MethodGet, - path: fmt.Sprintf( - "/some/path?response_type=code&scope=%s&client_id=pinniped-cli&state=short&redirect_uri=%s", - url.QueryEscape("openid profile email"), - url.QueryEscape(downstreamRedirectURI), - ), + name: "state does not have enough entropy", + issuer: issuer, + idpListGetter: newIDPListGetter(upstreamOIDCIdentityProvider), + generateState: happyStateGenerator, + generatePKCE: happyPKCEGenerator, + generateNonce: happyNonceGenerator, + method: http.MethodGet, + path: modifiedHappyGetRequestPath(map[string]string{"state": "short"}), wantStatus: http.StatusFound, wantContentType: "application/json; charset=utf-8", - wantLocationHeader: fmt.Sprintf("%s?%s", downstreamRedirectURI, fositeInvalidStateErrorQuery), + wantLocationHeader: urlWithQuery(downstreamRedirectURI, fositeInvalidStateErrorQuery), }, { name: "error while generating state", @@ -363,7 +367,7 @@ func TestAuthorizationEndpoint(t *testing.T) { { name: "no upstream providers are configured", issuer: issuer, - idpListGetter: newIDPListGetter(), + idpListGetter: newIDPListGetter(), // empty method: http.MethodGet, path: happyGetRequestPath, wantStatus: http.StatusUnprocessableEntity, @@ -373,7 +377,7 @@ func TestAuthorizationEndpoint(t *testing.T) { { name: "too many upstream providers are configured", issuer: issuer, - idpListGetter: newIDPListGetter(upstreamOIDCIdentityProvider, upstreamOIDCIdentityProvider), + idpListGetter: newIDPListGetter(upstreamOIDCIdentityProvider, upstreamOIDCIdentityProvider), // more than one not allowed method: http.MethodGet, path: happyGetRequestPath, wantStatus: http.StatusUnprocessableEntity, @@ -463,19 +467,18 @@ func TestAuthorizationEndpoint(t *testing.T) { test.idpListGetter.SetIDPList([]provider.UpstreamOIDCIdentityProvider{newProviderSettings}) // Update the expectations of the test case to match the new upstream IDP settings. - test.wantLocationHeader = fmt.Sprintf("%s?%s", - upstreamAuthURL.String(), - url.Values{ - "response_type": []string{"code"}, - "access_type": []string{"offline"}, - "scope": []string{"other-scope1 other-scope2"}, - "client_id": []string{"some-other-client-id"}, - "state": []string{"test-state"}, - "nonce": []string{"test-nonce"}, - "code_challenge": []string{expectedCodeChallenge}, - "code_challenge_method": []string{"S256"}, - "redirect_uri": []string{issuer + "/callback/some-other-idp"}, - }.Encode(), + test.wantLocationHeader = urlWithQuery(upstreamAuthURL.String(), + map[string]string{ + "response_type": "code", + "access_type": "offline", + "scope": "other-scope1 other-scope2", + "client_id": "some-other-client-id", + "state": "test-state", + "nonce": "test-nonce", + "code_challenge": expectedCodeChallenge, + "code_challenge_method": "S256", + "redirect_uri": issuer + "/callback/some-other-idp", + }, ) // Run again on the same instance of the subject with the modified upstream IDP settings and the From 2564d1be42f166419a09e5e0ae0dd66f261c8524 Mon Sep 17 00:00:00 2001 From: Andrew Keesler Date: Wed, 4 Nov 2020 12:19:07 -0800 Subject: [PATCH 10/21] Supervisor authorize endpoint errors when missing PKCE params Signed-off-by: Ryan Richard --- internal/oidc/auth/auth_handler.go | 27 +++++- internal/oidc/auth/auth_handler_test.go | 115 ++++++++++++++++++------ 2 files changed, 110 insertions(+), 32 deletions(-) diff --git a/internal/oidc/auth/auth_handler.go b/internal/oidc/auth/auth_handler.go index d46384af9..8142c25c3 100644 --- a/internal/oidc/auth/auth_handler.go +++ b/internal/oidc/auth/auth_handler.go @@ -8,6 +8,8 @@ import ( "fmt" "net/http" + "github.com/ory/fosite/handler/openid" + "github.com/ory/fosite/compose" "golang.org/x/oauth2" "k8s.io/klog/v2" @@ -31,10 +33,14 @@ func NewHandler( generatePKCE func() (pkce.Code, error), generateNonce func() (nonce.Nonce, error), ) http.Handler { + oauthConfig := &compose.Config{ + EnforcePKCEForPublicClients: true, + } + secret := []byte("some-cool-secret-that-is-32bytes") // TODO use a real secret once we care about real authorization codes oauthHelper := compose.Compose( // Empty Config for right now since we aren't using anything in it. We may want to inject this // in the future since it has some really nice configuration knobs like token lifetime. - &compose.Config{}, + oauthConfig, // This is the thing that matters right now - the store is used to get information about the // client in the authorization request. @@ -42,16 +48,18 @@ func NewHandler( // Shouldn't need any of this filled in as of right now - we aren't doing auth code stuff, // issuing ID tokens, or signing anything yet. - &compose.CommonStrategy{}, + &compose.CommonStrategy{ + CoreStrategy: compose.NewOAuth2HMACStrategy(oauthConfig, secret, nil), + }, // hasher, shouldn't need this right now - we aren't doing any client auth...yet? nil, // We will _probably_ want the below handlers somewhere in the code, but I'm not sure where yet, // and we don't need them for the tests to pass currently, so they are commented out. - // compose.OAuth2AuthorizeExplicitFactory, + compose.OAuth2AuthorizeExplicitFactory, // compose.OpenIDConnectExplicitFactory, - // compose.OAuth2PKCEFactory, + compose.OAuth2PKCEFactory, ) return httperr.HandlerFunc(func(w http.ResponseWriter, r *http.Request) error { if r.Method != http.MethodPost && r.Method != http.MethodGet { @@ -70,6 +78,17 @@ func NewHandler( return nil } + // Perform validations + _, err = oauthHelper.NewAuthorizeResponse( + r.Context(), // TODO: maybe another context here since this one will expire? + authorizeRequester, + &openid.DefaultSession{}, + ) + if err != nil { + oauthHelper.WriteAuthorizeError(w, authorizeRequester, err) + return nil + } + upstreamIDP, err := chooseUpstreamIDP(idpListGetter) if err != nil { return err diff --git a/internal/oidc/auth/auth_handler_test.go b/internal/oidc/auth/auth_handler_test.go index d4bee4835..d84541379 100644 --- a/internal/oidc/auth/auth_handler_test.go +++ b/internal/oidc/auth/auth_handler_test.go @@ -5,6 +5,7 @@ package auth import ( "fmt" + "html" "mime" "net/http" "net/http/httptest" @@ -49,6 +50,20 @@ func TestAuthorizationEndpoint(t *testing.T) { } `) + fositeMissingCodeChallengeErrorQuery = map[string]string{ + "error": "invalid_request", + "error_description": "The request is missing a required parameter, includes an invalid parameter value, includes a parameter more than once, or is otherwise malformed\n\nThis client must include a code_challenge when performing the authorize code flow, but it is missing.", + "error_hint": "This client must include a code_challenge when performing the authorize code flow, but it is missing.", + "state": "some-state-value", + } + + fositeMissingCodeChallengeMethodErrorQuery = map[string]string{ + "error": "invalid_request", + "error_description": "The request is missing a required parameter, includes an invalid parameter value, includes a parameter more than once, or is otherwise malformed\n\nClients must use code_challenge_method=S256, plain is not allowed.", + "error_hint": "Clients must use code_challenge_method=S256, plain is not allowed.", + "state": "some-state-value", + } + fositeUnsupportedResponseTypeErrorQuery = map[string]string{ "error": "unsupported_response_type", "error_description": "The authorization server does not support obtaining a token using this method\n\nThe client is not allowed to request response_type \"unsupported\".", @@ -103,6 +118,8 @@ func TestAuthorizationEndpoint(t *testing.T) { }, }, }, + AuthorizeCodes: map[string]storage.StoreAuthorizeCode{}, + PKCES: map[string]fosite.Requester{}, } happyStateGenerator := func() (state.State, error) { return "test-state", nil } @@ -111,7 +128,7 @@ func TestAuthorizationEndpoint(t *testing.T) { // This is the PKCE challenge which is calculated as base64(sha256("test-pkce")). For example: // $ echo -n test-pkce | shasum -a 256 | cut -d" " -f1 | xxd -r -p | base64 | cut -d"=" -f1 - expectedCodeChallenge := "VVaezYqum7reIhoavCHD1n2d-piN3r_mywoYj7fCR7g" + expectedUpstreamCodeChallenge := "VVaezYqum7reIhoavCHD1n2d-piN3r_mywoYj7fCR7g" pathWithQuery := func(path string, query map[string]string) string { values := url.Values{} @@ -135,12 +152,14 @@ func TestAuthorizationEndpoint(t *testing.T) { } happyGetRequestQueryMap := map[string]string{ - "response_type": "code", - "scope": "openid profile email", - "client_id": "pinniped-cli", - "state": "some-state-value", - "nonce": "some-nonce-value", - "redirect_uri": downstreamRedirectURI, + "response_type": "code", + "scope": "openid profile email", + "client_id": "pinniped-cli", + "state": "some-state-value", + "nonce": "some-nonce-value", + "code_challenge": "some-challenge", + "code_challenge_method": "S256", + "redirect_uri": downstreamRedirectURI, } happyGetRequestPath := pathWithQuery("/some/path", happyGetRequestQueryMap) @@ -169,7 +188,7 @@ func TestAuthorizationEndpoint(t *testing.T) { "client_id": "some-client-id", "state": "test-state", "nonce": "test-nonce", - "code_challenge": expectedCodeChallenge, + "code_challenge": expectedUpstreamCodeChallenge, "code_challenge_method": "S256", "redirect_uri": issuer + "/callback/some-idp", }, @@ -197,17 +216,20 @@ func TestAuthorizationEndpoint(t *testing.T) { tests := []testCase{ { - name: "happy path using GET", - issuer: issuer, - idpListGetter: newIDPListGetter(upstreamOIDCIdentityProvider), - generateState: happyStateGenerator, - generatePKCE: happyPKCEGenerator, - generateNonce: happyNonceGenerator, - method: http.MethodGet, - path: happyGetRequestPath, - wantStatus: http.StatusFound, - wantContentType: "text/html; charset=utf-8", - wantBodyString: "", + name: "happy path using GET", + issuer: issuer, + idpListGetter: newIDPListGetter(upstreamOIDCIdentityProvider), + generateState: happyStateGenerator, + generatePKCE: happyPKCEGenerator, + generateNonce: happyNonceGenerator, + method: http.MethodGet, + path: happyGetRequestPath, + wantStatus: http.StatusFound, + wantContentType: "text/html; charset=utf-8", + wantBodyString: fmt.Sprintf(`Found.%s`, + html.EscapeString(happyGetRequestExpectedRedirectLocation), + "\n\n", + ), wantLocationHeader: happyGetRequestExpectedRedirectLocation, }, { @@ -221,11 +243,13 @@ func TestAuthorizationEndpoint(t *testing.T) { path: "/some/path", contentType: "application/x-www-form-urlencoded", body: url.Values{ - "response_type": []string{"code"}, - "scope": []string{"openid profile email"}, - "client_id": []string{"pinniped-cli"}, - "state": []string{"some-state-value"}, - "redirect_uri": []string{downstreamRedirectURI}, + "response_type": []string{"code"}, + "scope": []string{"openid profile email"}, + "client_id": []string{"pinniped-cli"}, + "state": []string{"some-state-value"}, + "code_challenge": []string{"some-challenge"}, + "code_challenge_method": []string{"S256"}, + "redirect_uri": []string{downstreamRedirectURI}, }.Encode(), wantStatus: http.StatusFound, wantContentType: "", @@ -272,6 +296,7 @@ func TestAuthorizationEndpoint(t *testing.T) { wantStatus: http.StatusFound, wantContentType: "application/json; charset=utf-8", wantLocationHeader: urlWithQuery(downstreamRedirectURI, fositeUnsupportedResponseTypeErrorQuery), + wantBodyString: "", }, { name: "downstream scopes do not match what is configured for client", @@ -285,6 +310,7 @@ func TestAuthorizationEndpoint(t *testing.T) { wantStatus: http.StatusFound, wantContentType: "application/json; charset=utf-8", wantLocationHeader: urlWithQuery(downstreamRedirectURI, fositeInvalidScopeErrorQuery), + wantBodyString: "", }, { name: "missing response type in request", @@ -298,6 +324,7 @@ func TestAuthorizationEndpoint(t *testing.T) { wantStatus: http.StatusFound, wantContentType: "application/json; charset=utf-8", wantLocationHeader: urlWithQuery(downstreamRedirectURI, fositeMissingResponseTypeErrorQuery), + wantBodyString: "", }, { name: "missing client id in request", @@ -312,6 +339,34 @@ func TestAuthorizationEndpoint(t *testing.T) { wantContentType: "application/json; charset=utf-8", wantBodyJSON: fositeInvalidClientErrorBody, }, + { + name: "missing PKCE code_challenge in request", // See https://tools.ietf.org/html/rfc7636#section-4.4.1 + issuer: issuer, + idpListGetter: newIDPListGetter(upstreamOIDCIdentityProvider), + generateState: happyStateGenerator, + generatePKCE: happyPKCEGenerator, + generateNonce: happyNonceGenerator, + method: http.MethodGet, + path: modifiedHappyGetRequestPath(map[string]string{"code_challenge": ""}), + wantStatus: http.StatusFound, + wantContentType: "application/json; charset=utf-8", + wantLocationHeader: urlWithQuery(downstreamRedirectURI, fositeMissingCodeChallengeErrorQuery), + wantBodyString: "", + }, + { + name: "missing PKCE code_challenge_method in request", // See https://tools.ietf.org/html/rfc7636#section-4.4.1 + issuer: issuer, + idpListGetter: newIDPListGetter(upstreamOIDCIdentityProvider), + generateState: happyStateGenerator, + generatePKCE: happyPKCEGenerator, + generateNonce: happyNonceGenerator, + method: http.MethodGet, + path: modifiedHappyGetRequestPath(map[string]string{"code_challenge_method": ""}), + wantStatus: http.StatusFound, + wantContentType: "application/json; charset=utf-8", + wantLocationHeader: urlWithQuery(downstreamRedirectURI, fositeMissingCodeChallengeMethodErrorQuery), + wantBodyString: "", + }, { name: "state does not have enough entropy", issuer: issuer, @@ -324,6 +379,7 @@ func TestAuthorizationEndpoint(t *testing.T) { wantStatus: http.StatusFound, wantContentType: "application/json; charset=utf-8", wantLocationHeader: urlWithQuery(downstreamRedirectURI, fositeInvalidStateErrorQuery), + wantBodyString: "", }, { name: "error while generating state", @@ -428,11 +484,10 @@ func TestAuthorizationEndpoint(t *testing.T) { require.Equal(t, test.wantStatus, rsp.Code) requireEqualContentType(t, rsp.Header().Get("Content-Type"), test.wantContentType) - if test.wantBodyString != "" { - require.Equal(t, test.wantBodyString, rsp.Body.String()) - } if test.wantBodyJSON != "" { require.JSONEq(t, test.wantBodyJSON, rsp.Body.String()) + } else { + require.Equal(t, test.wantBodyString, rsp.Body.String()) } if test.wantLocationHeader != "" { @@ -475,11 +530,15 @@ func TestAuthorizationEndpoint(t *testing.T) { "client_id": "some-other-client-id", "state": "test-state", "nonce": "test-nonce", - "code_challenge": expectedCodeChallenge, + "code_challenge": expectedUpstreamCodeChallenge, "code_challenge_method": "S256", "redirect_uri": issuer + "/callback/some-other-idp", }, ) + test.wantBodyString = fmt.Sprintf(`Found.%s`, + html.EscapeString(test.wantLocationHeader), + "\n\n", + ) // Run again on the same instance of the subject with the modified upstream IDP settings and the // modified expectations. This should ensure that the implementation is using the in-memory cache From ba688f56aa81ef415b50632d16fa3178d5dc9d51 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Wed, 4 Nov 2020 12:29:43 -0800 Subject: [PATCH 11/21] Supervisor authorize endpoint errors when PKCE code_challenge_method is invalid Signed-off-by: Andrew Keesler --- internal/oidc/auth/auth_handler_test.go | 35 +++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/internal/oidc/auth/auth_handler_test.go b/internal/oidc/auth/auth_handler_test.go index d84541379..11b2a6f8c 100644 --- a/internal/oidc/auth/auth_handler_test.go +++ b/internal/oidc/auth/auth_handler_test.go @@ -64,6 +64,13 @@ func TestAuthorizationEndpoint(t *testing.T) { "state": "some-state-value", } + fositeInvalidCodeChallengeErrorQuery = map[string]string{ + "error": "invalid_request", + "error_description": "The request is missing a required parameter, includes an invalid parameter value, includes a parameter more than once, or is otherwise malformed\n\nThe code_challenge_method is not supported, use S256 instead.", + "error_hint": "The code_challenge_method is not supported, use S256 instead.", + "state": "some-state-value", + } + fositeUnsupportedResponseTypeErrorQuery = map[string]string{ "error": "unsupported_response_type", "error_description": "The authorization server does not support obtaining a token using this method\n\nThe client is not allowed to request response_type \"unsupported\".", @@ -353,6 +360,34 @@ func TestAuthorizationEndpoint(t *testing.T) { wantLocationHeader: urlWithQuery(downstreamRedirectURI, fositeMissingCodeChallengeErrorQuery), wantBodyString: "", }, + { + name: "invalid value for PKCE code_challenge_method in request", // https://tools.ietf.org/html/rfc7636#section-4.3 + issuer: issuer, + idpListGetter: newIDPListGetter(upstreamOIDCIdentityProvider), + generateState: happyStateGenerator, + generatePKCE: happyPKCEGenerator, + generateNonce: happyNonceGenerator, + method: http.MethodGet, + path: modifiedHappyGetRequestPath(map[string]string{"code_challenge_method": "this-is-not-a-valid-pkce-alg"}), + wantStatus: http.StatusFound, + wantContentType: "application/json; charset=utf-8", + wantLocationHeader: urlWithQuery(downstreamRedirectURI, fositeInvalidCodeChallengeErrorQuery), + wantBodyString: "", + }, + { + name: "when PKCE code_challenge_method in request is `plain`", // https://tools.ietf.org/html/rfc7636#section-4.3 + issuer: issuer, + idpListGetter: newIDPListGetter(upstreamOIDCIdentityProvider), + generateState: happyStateGenerator, + generatePKCE: happyPKCEGenerator, + generateNonce: happyNonceGenerator, + method: http.MethodGet, + path: modifiedHappyGetRequestPath(map[string]string{"code_challenge_method": "plain"}), + wantStatus: http.StatusFound, + wantContentType: "application/json; charset=utf-8", + wantLocationHeader: urlWithQuery(downstreamRedirectURI, fositeMissingCodeChallengeMethodErrorQuery), + wantBodyString: "", + }, { name: "missing PKCE code_challenge_method in request", // See https://tools.ietf.org/html/rfc7636#section-4.4.1 issuer: issuer, From a36f7c6c07b8b39532955e05b6ccab2aca3eed4c Mon Sep 17 00:00:00 2001 From: Andrew Keesler Date: Wed, 4 Nov 2020 15:04:50 -0800 Subject: [PATCH 12/21] Test that the port of localhost redirect URI is ignored during validation Also move definition of our oauth client and the general fosite configuration to a helper so we can use the same config to construct the handler for both test and production code. Signed-off-by: Ryan Richard --- internal/oidc/auth/auth_handler.go | 55 +++++-------------------- internal/oidc/auth/auth_handler_test.go | 41 +++++++++++------- internal/oidc/oidc.go | 39 ++++++++++++++++++ 3 files changed, 76 insertions(+), 59 deletions(-) diff --git a/internal/oidc/auth/auth_handler.go b/internal/oidc/auth/auth_handler.go index 8142c25c3..7f89eaee0 100644 --- a/internal/oidc/auth/auth_handler.go +++ b/internal/oidc/auth/auth_handler.go @@ -10,7 +10,8 @@ import ( "github.com/ory/fosite/handler/openid" - "github.com/ory/fosite/compose" + "github.com/ory/fosite" + "golang.org/x/oauth2" "k8s.io/klog/v2" @@ -28,39 +29,11 @@ type IDPListGetter interface { func NewHandler( issuer string, idpListGetter IDPListGetter, - oauthStore interface{}, + oauthHelper fosite.OAuth2Provider, generateState func() (state.State, error), generatePKCE func() (pkce.Code, error), generateNonce func() (nonce.Nonce, error), ) http.Handler { - oauthConfig := &compose.Config{ - EnforcePKCEForPublicClients: true, - } - secret := []byte("some-cool-secret-that-is-32bytes") // TODO use a real secret once we care about real authorization codes - oauthHelper := compose.Compose( - // Empty Config for right now since we aren't using anything in it. We may want to inject this - // in the future since it has some really nice configuration knobs like token lifetime. - oauthConfig, - - // This is the thing that matters right now - the store is used to get information about the - // client in the authorization request. - oauthStore, - - // Shouldn't need any of this filled in as of right now - we aren't doing auth code stuff, - // issuing ID tokens, or signing anything yet. - &compose.CommonStrategy{ - CoreStrategy: compose.NewOAuth2HMACStrategy(oauthConfig, secret, nil), - }, - - // hasher, shouldn't need this right now - we aren't doing any client auth...yet? - nil, - - // We will _probably_ want the below handlers somewhere in the code, but I'm not sure where yet, - // and we don't need them for the tests to pass currently, so they are commented out. - compose.OAuth2AuthorizeExplicitFactory, - // compose.OpenIDConnectExplicitFactory, - compose.OAuth2PKCEFactory, - ) return httperr.HandlerFunc(func(w http.ResponseWriter, r *http.Request) error { if r.Method != http.MethodPost && r.Method != http.MethodGet { // https://openid.net/specs/openid-connect-core-1_0.html#AuthRequest @@ -69,21 +42,7 @@ func NewHandler( return httperr.Newf(http.StatusMethodNotAllowed, "%s (try GET or POST)", r.Method) } - authorizeRequester, err := oauthHelper.NewAuthorizeRequest( - r.Context(), // TODO: maybe another context here since this one will expire? - r, - ) - if err != nil { - oauthHelper.WriteAuthorizeError(w, authorizeRequester, err) - return nil - } - - // Perform validations - _, err = oauthHelper.NewAuthorizeResponse( - r.Context(), // TODO: maybe another context here since this one will expire? - authorizeRequester, - &openid.DefaultSession{}, - ) + authorizeRequester, err := oauthHelper.NewAuthorizeRequest(r.Context(), r) if err != nil { oauthHelper.WriteAuthorizeError(w, authorizeRequester, err) return nil @@ -94,6 +53,12 @@ func NewHandler( return err } + _, err = oauthHelper.NewAuthorizeResponse(r.Context(), authorizeRequester, &openid.DefaultSession{}) + if err != nil { + oauthHelper.WriteAuthorizeError(w, authorizeRequester, err) + return nil + } + stateValue, nonceValue, pkceValue, err := generateParams(generateState, generateNonce, generatePKCE) if err != nil { return err diff --git a/internal/oidc/auth/auth_handler_test.go b/internal/oidc/auth/auth_handler_test.go index 11b2a6f8c..1da217a96 100644 --- a/internal/oidc/auth/auth_handler_test.go +++ b/internal/oidc/auth/auth_handler_test.go @@ -18,6 +18,7 @@ import ( "github.com/stretchr/testify/require" "go.pinniped.dev/internal/here" + "go.pinniped.dev/internal/oidc" "go.pinniped.dev/internal/oidc/provider" "go.pinniped.dev/internal/oidcclient/nonce" "go.pinniped.dev/internal/oidcclient/pkce" @@ -112,22 +113,15 @@ func TestAuthorizationEndpoint(t *testing.T) { issuer := "https://my-issuer.com/some-path" + // Configure fosite the same way that the production code would, except use in-memory storage. oauthStore := &storage.MemoryStore{ - Clients: map[string]fosite.Client{ - "pinniped-cli": &fosite.DefaultOpenIDConnectClient{ - DefaultClient: &fosite.DefaultClient{ - ID: "pinniped-cli", - Public: true, - RedirectURIs: []string{downstreamRedirectURI}, - ResponseTypes: []string{"code"}, - GrantTypes: []string{"authorization_code"}, - Scopes: []string{"openid", "profile", "email"}, - }, - }, - }, + Clients: map[string]fosite.Client{oidc.PinnipedCLIOIDCClient().ID: oidc.PinnipedCLIOIDCClient()}, AuthorizeCodes: map[string]storage.StoreAuthorizeCode{}, PKCES: map[string]fosite.Requester{}, } + hmacSecret := []byte("some secret - must have at least 32 bytes") + require.GreaterOrEqual(t, len(hmacSecret), 32, "fosite requires that hmac secrets have at least 32 bytes") + oauthHelper := oidc.FositeOauth2Helper(oauthStore, hmacSecret) happyStateGenerator := func() (state.State, error) { return "test-state", nil } happyPKCEGenerator := func() (pkce.Code, error) { return "test-pkce", nil } @@ -291,6 +285,25 @@ func TestAuthorizationEndpoint(t *testing.T) { wantContentType: "application/json; charset=utf-8", wantBodyJSON: fositeInvalidRedirectURIErrorBody, }, + { + name: "downstream redirect uri matches what is configured for client except for the port number", + issuer: issuer, + idpListGetter: newIDPListGetter(upstreamOIDCIdentityProvider), + generateState: happyStateGenerator, + generatePKCE: happyPKCEGenerator, + generateNonce: happyNonceGenerator, + method: http.MethodGet, + path: modifiedHappyGetRequestPath(map[string]string{ + "redirect_uri": "http://127.0.0.1:42/callback", + }), + wantStatus: http.StatusFound, + wantContentType: "text/html; charset=utf-8", + wantBodyString: fmt.Sprintf(`Found.%s`, + html.EscapeString(happyGetRequestExpectedRedirectLocation), + "\n\n", + ), + wantLocationHeader: happyGetRequestExpectedRedirectLocation, + }, { name: "response type is unsupported", issuer: issuer, @@ -534,7 +547,7 @@ func TestAuthorizationEndpoint(t *testing.T) { for _, test := range tests { test := test t.Run(test.name, func(t *testing.T) { - subject := NewHandler(test.issuer, test.idpListGetter, oauthStore, test.generateState, test.generatePKCE, test.generateNonce) + subject := NewHandler(test.issuer, test.idpListGetter, oauthHelper, test.generateState, test.generatePKCE, test.generateNonce) runOneTestCase(t, test, subject) }) } @@ -543,7 +556,7 @@ func TestAuthorizationEndpoint(t *testing.T) { test := tests[0] require.Equal(t, "happy path using GET", test.name) // re-use the happy path test case - subject := NewHandler(test.issuer, test.idpListGetter, oauthStore, test.generateState, test.generatePKCE, test.generateNonce) + subject := NewHandler(test.issuer, test.idpListGetter, oauthHelper, test.generateState, test.generatePKCE, test.generateNonce) runOneTestCase(t, test, subject) diff --git a/internal/oidc/oidc.go b/internal/oidc/oidc.go index d78f199c6..2fd61d3d1 100644 --- a/internal/oidc/oidc.go +++ b/internal/oidc/oidc.go @@ -4,9 +4,48 @@ // Package oidc contains common OIDC functionality needed by Pinniped. package oidc +import ( + "github.com/ory/fosite" + "github.com/ory/fosite/compose" +) + const ( WellKnownEndpointPath = "/.well-known/openid-configuration" AuthorizationEndpointPath = "/oauth2/authorize" TokenEndpointPath = "/oauth2/token" //nolint:gosec // ignore lint warning that this is a credential JWKSEndpointPath = "/jwks.json" ) + +func PinnipedCLIOIDCClient() *fosite.DefaultOpenIDConnectClient { + return &fosite.DefaultOpenIDConnectClient{ + DefaultClient: &fosite.DefaultClient{ + ID: "pinniped-cli", + Public: true, + RedirectURIs: []string{"http://127.0.0.1/callback"}, + ResponseTypes: []string{"code"}, + GrantTypes: []string{"authorization_code"}, + Scopes: []string{"openid", "profile", "email"}, + }, + } +} + +// Note that Fosite requires the HMAC secret to be 32 bytes. +func FositeOauth2Helper(oauthStore interface{}, hmacSecretOfLength32 []byte) fosite.OAuth2Provider { + oauthConfig := &compose.Config{ + EnforcePKCEForPublicClients: true, + } + + return compose.Compose( + oauthConfig, + oauthStore, + &compose.CommonStrategy{ + CoreStrategy: compose.NewOAuth2HMACStrategy(oauthConfig, hmacSecretOfLength32, nil), + }, + nil, // hasher, defaults to using BCrypt when nil. Used for hashing client secrets. + compose.OAuth2AuthorizeExplicitFactory, + // compose.OAuth2RefreshTokenGrantFactory, + // compose.OpenIDConnectExplicitFactory, + // compose.OpenIDConnectRefreshFactory, + compose.OAuth2PKCEFactory, + ) +} From 33ce79f89dc7b30627cd4adc235dda39c314edba Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Wed, 4 Nov 2020 17:06:47 -0800 Subject: [PATCH 13/21] Expose the Supervisor OIDC authorization endpoint to the public --- cmd/pinniped-supervisor/main.go | 3 +- internal/oidc/auth/auth_handler.go | 4 + internal/oidc/oidc.go | 6 +- internal/oidc/provider/manager/manager.go | 22 ++- .../oidc/provider/manager/manager_test.go | 133 +++++++++++------- 5 files changed, 116 insertions(+), 52 deletions(-) diff --git a/cmd/pinniped-supervisor/main.go b/cmd/pinniped-supervisor/main.go index dd89c1e07..302d1d209 100644 --- a/cmd/pinniped-supervisor/main.go +++ b/cmd/pinniped-supervisor/main.go @@ -181,9 +181,10 @@ func run(serverInstallationNamespace string, cfg *supervisor.Config) error { dynamicJWKSProvider := jwks.NewDynamicJWKSProvider() dynamicTLSCertProvider := provider.NewDynamicTLSCertProvider() + dynamicUpstreamIDPProvider := provider.NewDynamicUpstreamIDPProvider() // OIDC endpoints will be served by the oidProvidersManager, and any non-OIDC paths will fallback to the healthMux. - oidProvidersManager := manager.NewManager(healthMux, dynamicJWKSProvider) + oidProvidersManager := manager.NewManager(healthMux, dynamicJWKSProvider, dynamicUpstreamIDPProvider) startControllers( ctx, diff --git a/internal/oidc/auth/auth_handler.go b/internal/oidc/auth/auth_handler.go index 7f89eaee0..8fbcf5751 100644 --- a/internal/oidc/auth/auth_handler.go +++ b/internal/oidc/auth/auth_handler.go @@ -44,23 +44,27 @@ func NewHandler( authorizeRequester, err := oauthHelper.NewAuthorizeRequest(r.Context(), r) if err != nil { + klog.InfoS("authorize request error", "err", err, "details", fosite.ErrorToRFC6749Error(err).ToValues()) oauthHelper.WriteAuthorizeError(w, authorizeRequester, err) return nil } upstreamIDP, err := chooseUpstreamIDP(idpListGetter) if err != nil { + klog.InfoS("authorize request upstream selection error", "err", err) return err } _, err = oauthHelper.NewAuthorizeResponse(r.Context(), authorizeRequester, &openid.DefaultSession{}) if err != nil { + klog.InfoS("authorize response error", "err", err, "details", fosite.ErrorToRFC6749Error(err).ToValues()) oauthHelper.WriteAuthorizeError(w, authorizeRequester, err) return nil } stateValue, nonceValue, pkceValue, err := generateParams(generateState, generateNonce, generatePKCE) if err != nil { + klog.InfoS("authorize generate error", "err", err) return err } diff --git a/internal/oidc/oidc.go b/internal/oidc/oidc.go index 2fd61d3d1..a03c86d04 100644 --- a/internal/oidc/oidc.go +++ b/internal/oidc/oidc.go @@ -29,8 +29,7 @@ func PinnipedCLIOIDCClient() *fosite.DefaultOpenIDConnectClient { } } -// Note that Fosite requires the HMAC secret to be 32 bytes. -func FositeOauth2Helper(oauthStore interface{}, hmacSecretOfLength32 []byte) fosite.OAuth2Provider { +func FositeOauth2Helper(oauthStore interface{}, hmacSecretOfLengthAtLeast32 []byte) fosite.OAuth2Provider { oauthConfig := &compose.Config{ EnforcePKCEForPublicClients: true, } @@ -39,7 +38,8 @@ func FositeOauth2Helper(oauthStore interface{}, hmacSecretOfLength32 []byte) fos oauthConfig, oauthStore, &compose.CommonStrategy{ - CoreStrategy: compose.NewOAuth2HMACStrategy(oauthConfig, hmacSecretOfLength32, nil), + // Note that Fosite requires the HMAC secret to be at least 32 bytes. + CoreStrategy: compose.NewOAuth2HMACStrategy(oauthConfig, hmacSecretOfLengthAtLeast32, nil), }, nil, // hasher, defaults to using BCrypt when nil. Used for hashing client secrets. compose.OAuth2AuthorizeExplicitFactory, diff --git a/internal/oidc/provider/manager/manager.go b/internal/oidc/provider/manager/manager.go index 46a5f071d..fbee5db41 100644 --- a/internal/oidc/provider/manager/manager.go +++ b/internal/oidc/provider/manager/manager.go @@ -8,12 +8,18 @@ import ( "strings" "sync" + "github.com/ory/fosite" + "github.com/ory/fosite/storage" "k8s.io/klog/v2" "go.pinniped.dev/internal/oidc" + "go.pinniped.dev/internal/oidc/auth" "go.pinniped.dev/internal/oidc/discovery" "go.pinniped.dev/internal/oidc/jwks" "go.pinniped.dev/internal/oidc/provider" + "go.pinniped.dev/internal/oidcclient/nonce" + "go.pinniped.dev/internal/oidcclient/pkce" + "go.pinniped.dev/internal/oidcclient/state" ) // Manager can manage multiple active OIDC providers. It acts as a request router for them. @@ -25,16 +31,19 @@ type Manager struct { providerHandlers map[string]http.Handler // map of all routes for all providers nextHandler http.Handler // the next handler in a chain, called when this manager didn't know how to handle a request dynamicJWKSProvider jwks.DynamicJWKSProvider // in-memory cache of per-issuer JWKS data + idpListGetter auth.IDPListGetter // in-memory cache of upstream IDPs } // NewManager returns an empty Manager. // nextHandler will be invoked for any requests that could not be handled by this manager's providers. // dynamicJWKSProvider will be used as an in-memory cache for per-issuer JWKS data. -func NewManager(nextHandler http.Handler, dynamicJWKSProvider jwks.DynamicJWKSProvider) *Manager { +// idpListGetter will be used as an in-memory cache of currently configured upstream IDPs. +func NewManager(nextHandler http.Handler, dynamicJWKSProvider jwks.DynamicJWKSProvider, idpListGetter auth.IDPListGetter) *Manager { return &Manager{ providerHandlers: make(map[string]http.Handler), nextHandler: nextHandler, dynamicJWKSProvider: dynamicJWKSProvider, + idpListGetter: idpListGetter, } } @@ -60,6 +69,17 @@ func (m *Manager) SetProviders(oidcProviders ...*provider.OIDCProvider) { jwksURL := strings.ToLower(incomingProvider.IssuerHost()) + "/" + incomingProvider.IssuerPath() + oidc.JWKSEndpointPath m.providerHandlers[jwksURL] = jwks.NewHandler(incomingProvider.Issuer(), m.dynamicJWKSProvider) + // Each OIDC provider gets its own storage. + oauthStore := &storage.MemoryStore{ + Clients: map[string]fosite.Client{oidc.PinnipedCLIOIDCClient().ID: oidc.PinnipedCLIOIDCClient()}, + AuthorizeCodes: map[string]storage.StoreAuthorizeCode{}, + PKCES: map[string]fosite.Requester{}, + } + oauthHelper := oidc.FositeOauth2Helper(oauthStore, []byte("some secret - must have at least 32 bytes")) // TODO replace this secret + + authURL := strings.ToLower(incomingProvider.IssuerHost()) + "/" + incomingProvider.IssuerPath() + oidc.AuthorizationEndpointPath + m.providerHandlers[authURL] = auth.NewHandler(incomingProvider.Issuer(), m.idpListGetter, oauthHelper, state.Generate, pkce.Generate, nonce.Generate) + klog.InfoS("oidc provider manager added or updated issuer", "issuer", incomingProvider.Issuer()) } } diff --git a/internal/oidc/provider/manager/manager_test.go b/internal/oidc/provider/manager/manager_test.go index 01122ad79..e2d55f322 100644 --- a/internal/oidc/provider/manager/manager_test.go +++ b/internal/oidc/provider/manager/manager_test.go @@ -8,6 +8,7 @@ import ( "io/ioutil" "net/http" "net/http/httptest" + "net/url" "strings" "testing" @@ -32,12 +33,15 @@ func TestManager(t *testing.T) { dynamicJWKSProvider jwks.DynamicJWKSProvider ) - issuer1 := "https://example.com/some/path" - issuer1DifferentCaseHostname := "https://eXamPle.coM/some/path" - issuer1KeyID := "issuer1-key" - issuer2 := "https://example.com/some/path/more/deeply/nested/path" // note that this is a sub-path of the other issuer url - issuer2DifferentCaseHostname := "https://exAmPlE.Com/some/path/more/deeply/nested/path" - issuer2KeyID := "issuer2-key" + const ( + issuer1 = "https://example.com/some/path" + issuer1DifferentCaseHostname = "https://eXamPle.coM/some/path" + issuer1KeyID = "issuer1-key" + issuer2 = "https://example.com/some/path/more/deeply/nested/path" // note that this is a sub-path of the other issuer url + issuer2DifferentCaseHostname = "https://exAmPlE.Com/some/path/more/deeply/nested/path" + issuer2KeyID = "issuer2-key" + upstreamIDPAuthorizationURL = "https://test-upstream.com/auth" + ) newGetRequest := func(url string) *http.Request { return httptest.NewRequest(http.MethodGet, url, nil) @@ -50,17 +54,33 @@ func TestManager(t *testing.T) { r.False(fallbackHandlerWasCalled) + // Minimal check to ensure that the right discovery endpoint was called r.Equal(http.StatusOK, recorder.Code) responseBody, err := ioutil.ReadAll(recorder.Body) r.NoError(err) parsedDiscoveryResult := discovery.Metadata{} err = json.Unmarshal(responseBody, &parsedDiscoveryResult) r.NoError(err) - - // Minimal check to ensure that the right discovery endpoint was called r.Equal(expectedIssuerInResponse, parsedDiscoveryResult.Issuer) } + requireAuthorizationRequestToBeHandled := func(requestIssuer, requestURLSuffix, expectedRedirectLocationPrefix string) { + recorder := httptest.NewRecorder() + + subject.ServeHTTP(recorder, newGetRequest(requestIssuer+oidc.AuthorizationEndpointPath+requestURLSuffix)) + + r.False(fallbackHandlerWasCalled) + + // Minimal check to ensure that the right endpoint was called + r.Equal(http.StatusFound, recorder.Code) + actualLocation := recorder.Header().Get("Location") + r.True( + strings.HasPrefix(actualLocation, expectedRedirectLocationPrefix), + "actual location %s did not start with expected prefix %s", + actualLocation, expectedRedirectLocationPrefix, + ) + } + requireJWKSRequestToBeHandled := func(requestIssuer, requestURLSuffix, expectedJWKKeyID string) { recorder := httptest.NewRecorder() @@ -68,14 +88,13 @@ func TestManager(t *testing.T) { r.False(fallbackHandlerWasCalled) + // Minimal check to ensure that the right JWKS endpoint was called r.Equal(http.StatusOK, recorder.Code) responseBody, err := ioutil.ReadAll(recorder.Body) r.NoError(err) parsedJWKSResult := jose.JSONWebKeySet{} err = json.Unmarshal(responseBody, &parsedJWKSResult) r.NoError(err) - - // Minimal check to ensure that the right JWKS endpoint was called r.Equal(expectedJWKKeyID, parsedJWKSResult.Keys[0].KeyID) } @@ -85,7 +104,20 @@ func TestManager(t *testing.T) { fallbackHandlerWasCalled = true } dynamicJWKSProvider = jwks.NewDynamicJWKSProvider() - subject = NewManager(nextHandler, dynamicJWKSProvider) + + parsedUpstreamIDPAuthorizationURL, err := url.Parse(upstreamIDPAuthorizationURL) + r.NoError(err) + idpListGetter := provider.NewDynamicUpstreamIDPProvider() + idpListGetter.SetIDPList([]provider.UpstreamOIDCIdentityProvider{ + { + Name: "test-idp", + ClientID: "test-client-id", + AuthorizationURL: *parsedUpstreamIDPAuthorizationURL, + Scopes: []string{"test-scope"}, + }, + }) + + subject = NewManager(nextHandler, dynamicJWKSProvider, idpListGetter) }) when("given no providers via SetProviders()", func() { @@ -113,6 +145,45 @@ func TestManager(t *testing.T) { return k } + requireRoutesMatchingRequestsToAppropriateProvider := func() { + requireDiscoveryRequestToBeHandled(issuer1, "", issuer1) + requireDiscoveryRequestToBeHandled(issuer2, "", issuer2) + requireDiscoveryRequestToBeHandled(issuer2, "?some=query", issuer2) + + // Hostnames are case-insensitive, so test that we can handle that. + requireDiscoveryRequestToBeHandled(issuer1DifferentCaseHostname, "", issuer1) + requireDiscoveryRequestToBeHandled(issuer2DifferentCaseHostname, "", issuer2) + requireDiscoveryRequestToBeHandled(issuer2DifferentCaseHostname, "?some=query", issuer2) + + requireJWKSRequestToBeHandled(issuer1, "", issuer1KeyID) + requireJWKSRequestToBeHandled(issuer2, "", issuer2KeyID) + requireJWKSRequestToBeHandled(issuer2, "?some=query", issuer2KeyID) + + // Hostnames are case-insensitive, so test that we can handle that. + requireJWKSRequestToBeHandled(issuer1DifferentCaseHostname, "", issuer1KeyID) + requireJWKSRequestToBeHandled(issuer2DifferentCaseHostname, "", issuer2KeyID) + requireJWKSRequestToBeHandled(issuer2DifferentCaseHostname, "?some=query", issuer2KeyID) + + authRedirectURI := "http://127.0.0.1/callback" + authRequestParams := "?" + url.Values{ + "response_type": []string{"code"}, + "scope": []string{"openid profile email"}, + "client_id": []string{"pinniped-cli"}, + "state": []string{"some-state-value"}, + "nonce": []string{"some-nonce-value"}, + "code_challenge": []string{"some-challenge"}, + "code_challenge_method": []string{"S256"}, + "redirect_uri": []string{authRedirectURI}, + }.Encode() + + requireAuthorizationRequestToBeHandled(issuer1, authRequestParams, upstreamIDPAuthorizationURL) + requireAuthorizationRequestToBeHandled(issuer2, authRequestParams, upstreamIDPAuthorizationURL) + + // Hostnames are case-insensitive, so test that we can handle that. + requireAuthorizationRequestToBeHandled(issuer1DifferentCaseHostname, authRequestParams, upstreamIDPAuthorizationURL) + requireAuthorizationRequestToBeHandled(issuer2DifferentCaseHostname, authRequestParams, upstreamIDPAuthorizationURL) + } + when("given some valid providers via SetProviders()", func() { it.Before(func() { p1, err := provider.NewOIDCProvider(issuer1) @@ -129,8 +200,8 @@ func TestManager(t *testing.T) { it("sends all non-matching host requests to the nextHandler", func() { r.False(fallbackHandlerWasCalled) - url := strings.ReplaceAll(issuer1+oidc.WellKnownEndpointPath, "example.com", "wrong-host.com") - subject.ServeHTTP(httptest.NewRecorder(), newGetRequest(url)) + wrongHostURL := strings.ReplaceAll(issuer1+oidc.WellKnownEndpointPath, "example.com", "wrong-host.com") + subject.ServeHTTP(httptest.NewRecorder(), newGetRequest(wrongHostURL)) r.True(fallbackHandlerWasCalled) }) @@ -147,23 +218,7 @@ func TestManager(t *testing.T) { }) it("routes matching requests to the appropriate provider", func() { - requireDiscoveryRequestToBeHandled(issuer1, "", issuer1) - requireDiscoveryRequestToBeHandled(issuer2, "", issuer2) - requireDiscoveryRequestToBeHandled(issuer2, "?some=query", issuer2) - - // Hostnames are case-insensitive, so test that we can handle that. - requireDiscoveryRequestToBeHandled(issuer1DifferentCaseHostname, "", issuer1) - requireDiscoveryRequestToBeHandled(issuer2DifferentCaseHostname, "", issuer2) - requireDiscoveryRequestToBeHandled(issuer2DifferentCaseHostname, "?some=query", issuer2) - - requireJWKSRequestToBeHandled(issuer1, "", issuer1KeyID) - requireJWKSRequestToBeHandled(issuer2, "", issuer2KeyID) - requireJWKSRequestToBeHandled(issuer2, "?some=query", issuer2KeyID) - - // Hostnames are case-insensitive, so test that we can handle that. - requireJWKSRequestToBeHandled(issuer1DifferentCaseHostname, "", issuer1KeyID) - requireJWKSRequestToBeHandled(issuer2DifferentCaseHostname, "", issuer2KeyID) - requireJWKSRequestToBeHandled(issuer2DifferentCaseHostname, "?some=query", issuer2KeyID) + requireRoutesMatchingRequestsToAppropriateProvider() }) }) @@ -182,23 +237,7 @@ func TestManager(t *testing.T) { }) it("still routes matching requests to the appropriate provider", func() { - requireDiscoveryRequestToBeHandled(issuer1, "", issuer1) - requireDiscoveryRequestToBeHandled(issuer2, "", issuer2) - requireDiscoveryRequestToBeHandled(issuer2, "?some=query", issuer2) - - // Hostnames are case-insensitive, so test that we can handle that. - requireDiscoveryRequestToBeHandled(issuer1DifferentCaseHostname, "", issuer1) - requireDiscoveryRequestToBeHandled(issuer2DifferentCaseHostname, "", issuer2) - requireDiscoveryRequestToBeHandled(issuer2DifferentCaseHostname, "?some=query", issuer2) - - requireJWKSRequestToBeHandled(issuer1, "", issuer1KeyID) - requireJWKSRequestToBeHandled(issuer2, "", issuer2KeyID) - requireJWKSRequestToBeHandled(issuer2, "?some=query", issuer2KeyID) - - // Hostnames are case-insensitive, so test that we can handle that. - requireJWKSRequestToBeHandled(issuer1DifferentCaseHostname, "", issuer1KeyID) - requireJWKSRequestToBeHandled(issuer2DifferentCaseHostname, "", issuer2KeyID) - requireJWKSRequestToBeHandled(issuer2DifferentCaseHostname, "?some=query", issuer2KeyID) + requireRoutesMatchingRequestsToAppropriateProvider() }) }) }) From 4032ed32aebde02786a51ed9800330d1b8705211 Mon Sep 17 00:00:00 2001 From: Andrew Keesler Date: Thu, 5 Nov 2020 10:59:03 -0500 Subject: [PATCH 14/21] Auth endpoint integration test initial thoughts This is awaiting the new upstream OIDC provider CRD in order to pass, however hopefully this is a starting point for us. Signed-off-by: Andrew Keesler --- test/integration/supervisor_login_test.go | 166 ++++++++++++++++++++++ 1 file changed, 166 insertions(+) create mode 100644 test/integration/supervisor_login_test.go diff --git a/test/integration/supervisor_login_test.go b/test/integration/supervisor_login_test.go new file mode 100644 index 000000000..e857d4002 --- /dev/null +++ b/test/integration/supervisor_login_test.go @@ -0,0 +1,166 @@ +// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package integration + +import ( + "context" + "fmt" + "net/http" + "net/url" + "testing" + "time" + + "github.com/coreos/go-oidc" + "github.com/stretchr/testify/require" + "golang.org/x/oauth2" + + "go.pinniped.dev/internal/oidcclient/nonce" + "go.pinniped.dev/internal/oidcclient/pkce" + "go.pinniped.dev/internal/oidcclient/state" + "go.pinniped.dev/test/library" +) + +func TestSupervisorLogin(t *testing.T) { + env := library.IntegrationEnv(t) + client := library.NewSupervisorClientset(t) + + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute) + defer cancel() + + // Create downstream OIDC provider (i.e., update supervisor with OIDC provider). + // TODO: these env vars might not be set, perform similar loop as discovery test? + scheme := "http" + addr := env.SupervisorHTTPAddress + caBundle := "" + path := "/some/path" + issuer := fmt.Sprintf("https://%s%s", addr, path) + _, _ = requireCreatingOIDCProviderCausesDiscoveryEndpointsToAppear( + ctx, + t, + scheme, + addr, + caBundle, + issuer, + client, + ) + + // Create HTTP client. + httpClient := newHTTPClient(t, caBundle, nil) + httpClient.CheckRedirect = func(_ *http.Request, _ []*http.Request) error { + // Don't follow any redirects right now, since we simply want to validate that our auth endpoint + // redirects us. + return http.ErrUseLastResponse + } + + // Declare the downstream auth endpoint url we will use. + downstreamAuthURL := makeDownstreamAuthURL(t, scheme, addr, path) + + // Make request to auth endpoint - should fail, since we have no upstreams. + req, err := http.NewRequestWithContext(ctx, http.MethodGet, downstreamAuthURL, nil) + require.NoError(t, err) + rsp, err := httpClient.Do(req) + require.NoError(t, err) + defer rsp.Body.Close() + require.Equal(t, http.StatusUnprocessableEntity, rsp.StatusCode) + + // Create upstream OIDC provider. + // TODO: this is where the new UpstreamOIDCProvider CRD might go. + upstreamIssuer := env.OIDCUpstream.Issuer + upstreamClientID := env.OIDCUpstream.ClientID + upstreamRedirectURI := fmt.Sprintf("http://127.0.0.1:%d/callback", env.OIDCUpstream.LocalhostPort) + + // Make request to authorize endpoint - should pass, since we now have an upstream. + req, err = http.NewRequestWithContext(ctx, http.MethodGet, downstreamAuthURL, nil) + require.NoError(t, err) + rsp, err = httpClient.Do(req) + require.NoError(t, err) + defer rsp.Body.Close() + require.Equal(t, http.StatusFound, rsp.StatusCode) + requireValidRedirectLocation( + ctx, + t, + upstreamIssuer, + upstreamClientID, + upstreamRedirectURI, + rsp.Header.Get("Location"), + ) +} + +func makeDownstreamAuthURL(t *testing.T, scheme, addr, path string) string { + t.Helper() + downstreamOAuth2Config := oauth2.Config{ + // This is the hardcoded public client that the supervisor supports. + ClientID: "pinniped-cli", + Endpoint: oauth2.Endpoint{ + AuthURL: fmt.Sprintf("%s://%s%s/oauth2/authorize", scheme, addr, path), + }, + // This is the hardcoded downstream redirect URI that the supervisor supports. + RedirectURL: "http://127.0.0.1/callback", + Scopes: []string{"openid"}, + } + state, nonce, pkce := generateAuthRequestParams(t) + return downstreamOAuth2Config.AuthCodeURL( + state.String(), + nonce.Param(), + pkce.Challenge(), + pkce.Method(), + ) +} + +func generateAuthRequestParams(t *testing.T) (state.State, nonce.Nonce, pkce.Code) { + t.Helper() + state, err := state.Generate() + require.NoError(t, err) + nonce, err := nonce.Generate() + require.NoError(t, err) + pkce, err := pkce.Generate() + require.NoError(t, err) + return state, nonce, pkce +} + +func requireValidRedirectLocation( + ctx context.Context, + t *testing.T, + issuer, clientID, redirectURI, actualLocation string, +) { + t.Helper() + + // Do OIDC discovery on our test issuer to get auth endpoint. + upstreamProvider, err := oidc.NewProvider(ctx, issuer) + require.NoError(t, err) + + // Parse expected upstream auth URL. + expectedLocationURL, err := url.Parse( + (&oauth2.Config{ + ClientID: clientID, + Endpoint: upstreamProvider.Endpoint(), + RedirectURL: redirectURI, + }).AuthCodeURL(""), + ) + require.NoError(t, err) + + // Parse actual upstream auth URL. + actualLocationURL, err := url.Parse(actualLocation) + require.NoError(t, err) + + // First make some assertions on the query values. Note that we will not be able to know what + // certain query values are since they may be random (e.g., state, pkce, nonce). + expectedLocationQuery := expectedLocationURL.Query() + actualLocationQuery := actualLocationURL.Query() + require.NotEmpty(t, actualLocationQuery.Get("state")) + actualLocationQuery.Del("state") + require.NotEmpty(t, actualLocationQuery.Get("code_challenge")) + actualLocationQuery.Del("code_challenge") + require.NotEmpty(t, actualLocationQuery.Get("code_challenge_method")) + actualLocationQuery.Del("code_challenge_method") + require.NotEmpty(t, actualLocationQuery.Get("nonce")) + actualLocationQuery.Del("nonce") + require.Equal(t, expectedLocationQuery, actualLocationQuery) + + // Zero-out query values, since we made specific assertions about those above, and assert that the + // URL's are equal otherwise. + expectedLocationURL.RawQuery = "" + actualLocationURL.RawQuery = "" + require.Equal(t, expectedLocationURL, actualLocationURL) +} From 246471bc919741ab9fa98cadf9cac5ac896ed95f Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Fri, 6 Nov 2020 14:44:58 -0800 Subject: [PATCH 15/21] Also run OIDC validations in supervisor authorize endpoint Signed-off-by: Andrew Keesler --- internal/oidc/auth/auth_handler.go | 23 +++++++++++++++---- internal/oidc/auth/auth_handler_test.go | 27 ++++++++++++++++++++--- internal/oidc/oidc.go | 2 +- internal/oidc/provider/manager/manager.go | 1 + 4 files changed, 45 insertions(+), 8 deletions(-) diff --git a/internal/oidc/auth/auth_handler.go b/internal/oidc/auth/auth_handler.go index 8fbcf5751..d74ad0947 100644 --- a/internal/oidc/auth/auth_handler.go +++ b/internal/oidc/auth/auth_handler.go @@ -7,11 +7,11 @@ package auth import ( "fmt" "net/http" - - "github.com/ory/fosite/handler/openid" + "time" "github.com/ory/fosite" - + "github.com/ory/fosite/handler/openid" + "github.com/ory/fosite/token/jwt" "golang.org/x/oauth2" "k8s.io/klog/v2" @@ -55,7 +55,22 @@ func NewHandler( return err } - _, err = oauthHelper.NewAuthorizeResponse(r.Context(), authorizeRequester, &openid.DefaultSession{}) + // Grant the openid scope (for now) if they asked for it so that `NewAuthorizeResponse` will perform its OIDC validations. + for _, scope := range authorizeRequester.GetRequestedScopes() { + if scope == "openid" { + authorizeRequester.GrantScope(scope) + } + } + + now := time.Now() + _, err = oauthHelper.NewAuthorizeResponse(r.Context(), authorizeRequester, &openid.DefaultSession{ + Claims: &jwt.IDTokenClaims{ + // Temporary claim values to allow `NewAuthorizeResponse` to perform other OIDC validations. + Subject: "none", + AuthTime: now, + RequestedAt: now, + }, + }) if err != nil { klog.InfoS("authorize response error", "err", err, "details", fosite.ErrorToRFC6749Error(err).ToValues()) oauthHelper.WriteAuthorizeError(w, authorizeRequester, err) diff --git a/internal/oidc/auth/auth_handler_test.go b/internal/oidc/auth/auth_handler_test.go index 1da217a96..399605e39 100644 --- a/internal/oidc/auth/auth_handler_test.go +++ b/internal/oidc/auth/auth_handler_test.go @@ -51,6 +51,13 @@ func TestAuthorizationEndpoint(t *testing.T) { } `) + fositePromptHasNoneAndOtherValueErrorQuery = map[string]string{ + "error": "invalid_request", + "error_description": "The request is missing a required parameter, includes an invalid parameter value, includes a parameter more than once, or is otherwise malformed\n\nParameter \"prompt\" was set to \"none\", but contains other values as well which is not allowed.", + "error_hint": "Parameter \"prompt\" was set to \"none\", but contains other values as well which is not allowed.", + "state": "some-state-value", + } + fositeMissingCodeChallengeErrorQuery = map[string]string{ "error": "invalid_request", "error_description": "The request is missing a required parameter, includes an invalid parameter value, includes a parameter more than once, or is otherwise malformed\n\nThis client must include a code_challenge when performing the authorize code flow, but it is missing.", @@ -118,6 +125,7 @@ func TestAuthorizationEndpoint(t *testing.T) { Clients: map[string]fosite.Client{oidc.PinnipedCLIOIDCClient().ID: oidc.PinnipedCLIOIDCClient()}, AuthorizeCodes: map[string]storage.StoreAuthorizeCode{}, PKCES: map[string]fosite.Requester{}, + IDSessions: map[string]fosite.Requester{}, } hmacSecret := []byte("some secret - must have at least 32 bytes") require.GreaterOrEqual(t, len(hmacSecret), 32, "fosite requires that hmac secrets have at least 32 bytes") @@ -415,6 +423,22 @@ func TestAuthorizationEndpoint(t *testing.T) { wantLocationHeader: urlWithQuery(downstreamRedirectURI, fositeMissingCodeChallengeMethodErrorQuery), wantBodyString: "", }, + { + // This is just one of the many OIDC validations run by fosite. This test is to ensure that we are running + // through that part of the fosite library. + name: "prompt param is not allowed to have none and another legal value at the same time", + issuer: issuer, + idpListGetter: newIDPListGetter(upstreamOIDCIdentityProvider), + generateState: happyStateGenerator, + generatePKCE: happyPKCEGenerator, + generateNonce: happyNonceGenerator, + method: http.MethodGet, + path: modifiedHappyGetRequestPath(map[string]string{"prompt": "none login"}), + wantStatus: http.StatusFound, + wantContentType: "application/json; charset=utf-8", + wantLocationHeader: urlWithQuery(downstreamRedirectURI, fositePromptHasNoneAndOtherValueErrorQuery), + wantBodyString: "", + }, { name: "state does not have enough entropy", issuer: issuer, @@ -526,9 +550,6 @@ func TestAuthorizationEndpoint(t *testing.T) { rsp := httptest.NewRecorder() subject.ServeHTTP(rsp, req) - t.Logf("response: %#v", rsp) - t.Logf("body: %q", rsp.Body.String()) - require.Equal(t, test.wantStatus, rsp.Code) requireEqualContentType(t, rsp.Header().Get("Content-Type"), test.wantContentType) diff --git a/internal/oidc/oidc.go b/internal/oidc/oidc.go index a03c86d04..8d319c1d1 100644 --- a/internal/oidc/oidc.go +++ b/internal/oidc/oidc.go @@ -44,7 +44,7 @@ func FositeOauth2Helper(oauthStore interface{}, hmacSecretOfLengthAtLeast32 []by nil, // hasher, defaults to using BCrypt when nil. Used for hashing client secrets. compose.OAuth2AuthorizeExplicitFactory, // compose.OAuth2RefreshTokenGrantFactory, - // compose.OpenIDConnectExplicitFactory, + compose.OpenIDConnectExplicitFactory, // compose.OpenIDConnectRefreshFactory, compose.OAuth2PKCEFactory, ) diff --git a/internal/oidc/provider/manager/manager.go b/internal/oidc/provider/manager/manager.go index fbee5db41..229dd3751 100644 --- a/internal/oidc/provider/manager/manager.go +++ b/internal/oidc/provider/manager/manager.go @@ -74,6 +74,7 @@ func (m *Manager) SetProviders(oidcProviders ...*provider.OIDCProvider) { Clients: map[string]fosite.Client{oidc.PinnipedCLIOIDCClient().ID: oidc.PinnipedCLIOIDCClient()}, AuthorizeCodes: map[string]storage.StoreAuthorizeCode{}, PKCES: map[string]fosite.Requester{}, + IDSessions: map[string]fosite.Requester{}, } oauthHelper := oidc.FositeOauth2Helper(oauthStore, []byte("some secret - must have at least 32 bytes")) // TODO replace this secret From 005225d5f90a08fe346e8ef0e60679d1cfa7dd98 Mon Sep 17 00:00:00 2001 From: Andrew Keesler Date: Tue, 10 Nov 2020 10:33:52 -0800 Subject: [PATCH 16/21] Use the new plog pkg in auth_handler.go - Add a new helper method to plog to make a consistent way to log expected errors at the info level (as opposed to unexpected system errors that would be logged using plog.Error) Signed-off-by: Ryan Richard --- internal/oidc/auth/auth_handler.go | 32 ++++++++++++++++++++---------- internal/plog/plog.go | 6 ++++++ 2 files changed, 28 insertions(+), 10 deletions(-) diff --git a/internal/oidc/auth/auth_handler.go b/internal/oidc/auth/auth_handler.go index d74ad0947..7eb31e316 100644 --- a/internal/oidc/auth/auth_handler.go +++ b/internal/oidc/auth/auth_handler.go @@ -9,17 +9,17 @@ import ( "net/http" "time" + "go.pinniped.dev/internal/plog" + "github.com/ory/fosite" "github.com/ory/fosite/handler/openid" "github.com/ory/fosite/token/jwt" - "golang.org/x/oauth2" - "k8s.io/klog/v2" - "go.pinniped.dev/internal/httputil/httperr" "go.pinniped.dev/internal/oidc/provider" "go.pinniped.dev/internal/oidcclient/nonce" "go.pinniped.dev/internal/oidcclient/pkce" "go.pinniped.dev/internal/oidcclient/state" + "golang.org/x/oauth2" ) type IDPListGetter interface { @@ -44,14 +44,14 @@ func NewHandler( authorizeRequester, err := oauthHelper.NewAuthorizeRequest(r.Context(), r) if err != nil { - klog.InfoS("authorize request error", "err", err, "details", fosite.ErrorToRFC6749Error(err).ToValues()) + plog.Info("authorize request error", fositeErrorForLog(err)...) oauthHelper.WriteAuthorizeError(w, authorizeRequester, err) return nil } upstreamIDP, err := chooseUpstreamIDP(idpListGetter) if err != nil { - klog.InfoS("authorize request upstream selection error", "err", err) + plog.InfoErr("authorize request error", err) return err } @@ -72,14 +72,14 @@ func NewHandler( }, }) if err != nil { - klog.InfoS("authorize response error", "err", err, "details", fosite.ErrorToRFC6749Error(err).ToValues()) + plog.Info("authorize response error", fositeErrorForLog(err)...) oauthHelper.WriteAuthorizeError(w, authorizeRequester, err) return nil } stateValue, nonceValue, pkceValue, err := generateParams(generateState, generateNonce, generatePKCE) if err != nil { - klog.InfoS("authorize generate error", "err", err) + plog.InfoErr("authorize generate error", err) return err } @@ -130,18 +130,30 @@ func generateParams( ) (state.State, nonce.Nonce, pkce.Code, error) { stateValue, err := generateState() if err != nil { - klog.InfoS("error generating state param", "err", err) + plog.InfoErr("error generating state param", err) return "", "", "", httperr.Wrap(http.StatusInternalServerError, "error generating state param", err) } nonceValue, err := generateNonce() if err != nil { - klog.InfoS("error generating nonce param", "err", err) + plog.InfoErr("error generating nonce param", err) return "", "", "", httperr.Wrap(http.StatusInternalServerError, "error generating nonce param", err) } pkceValue, err := generatePKCE() if err != nil { - klog.InfoS("error generating PKCE param", "err", err) + plog.InfoErr("error generating PKCE param", err) return "", "", "", httperr.Wrap(http.StatusInternalServerError, "error generating PKCE param", err) } return stateValue, nonceValue, pkceValue, nil } + +func fositeErrorForLog(err error) []interface{} { + rfc6749Error := fosite.ErrorToRFC6749Error(err) + keysAndValues := make([]interface{}, 0) + keysAndValues = append(keysAndValues, "name") + keysAndValues = append(keysAndValues, rfc6749Error.Name) + keysAndValues = append(keysAndValues, "status") + keysAndValues = append(keysAndValues, rfc6749Error.Status()) + keysAndValues = append(keysAndValues, "description") + keysAndValues = append(keysAndValues, rfc6749Error.Description) + return keysAndValues +} diff --git a/internal/plog/plog.go b/internal/plog/plog.go index b22450fbb..d3b6efa81 100644 --- a/internal/plog/plog.go +++ b/internal/plog/plog.go @@ -5,6 +5,7 @@ package plog import "k8s.io/klog/v2" +// Use Error to log an unexpected system error. func Error(err error, msg string, keysAndValues ...interface{}) { klog.ErrorS(err, msg, keysAndValues...) } @@ -22,6 +23,11 @@ func Info(msg string, keysAndValues ...interface{}) { klog.V(klogLevelInfo).InfoS(msg, keysAndValues...) } +// Use InfoErr to log an expected error, e.g. validation failure of an http parameter. +func InfoErr(msg string, err error, keysAndValues ...interface{}) { + klog.V(klogLevelInfo).InfoS(msg, append([]interface{}{"error", err}, keysAndValues)...) +} + func Debug(msg string, keysAndValues ...interface{}) { klog.V(klogLevelDebug).InfoS(msg, keysAndValues...) } From dd190dede62d220463460d2053b77201132722a9 Mon Sep 17 00:00:00 2001 From: Monis Khan Date: Tue, 10 Nov 2020 17:58:00 -0800 Subject: [PATCH 17/21] WIP for saving authorize endpoint state into upstream state param Signed-off-by: Ryan Richard --- go.mod | 1 + go.sum | 2 + internal/oidc/auth/auth_handler.go | 68 +++++++--- internal/oidc/auth/auth_handler_test.go | 143 ++++++++++++++-------- internal/oidc/csrftoken/csrftoken.go | 24 ++++ internal/oidc/provider/manager/manager.go | 11 +- 6 files changed, 183 insertions(+), 66 deletions(-) create mode 100644 internal/oidc/csrftoken/csrftoken.go diff --git a/go.mod b/go.mod index dcec2960c..fae73095a 100644 --- a/go.mod +++ b/go.mod @@ -14,6 +14,7 @@ require ( github.com/golang/mock v1.4.4 github.com/golangci/golangci-lint v1.31.0 github.com/google/go-cmp v0.5.2 + github.com/gorilla/securecookie v1.1.1 github.com/ory/fosite v0.35.1 github.com/pkg/browser v0.0.0-20180916011732-0a3d74bf9ce4 github.com/sclevine/agouti v3.0.0+incompatible diff --git a/go.sum b/go.sum index aa2e942e7..86a139ad1 100644 --- a/go.sum +++ b/go.sum @@ -301,6 +301,8 @@ github.com/gookit/color v1.2.5/go.mod h1:AhIE+pS6D4Ql0SQWbBeXPHw7gY0/sjHoA4s/n1K github.com/gopherjs/gopherjs v0.0.0-20181017120253-0766667cb4d1 h1:EGx4pi6eqNxGaHF6qqu48+N2wcFQ5qg5FXgOdqsJ5d8= github.com/gopherjs/gopherjs v0.0.0-20181017120253-0766667cb4d1/go.mod h1:wJfORRmW1u3UXTncJ5qlYoELFm8eSnnEO6hX4iZ3EWY= github.com/gorilla/mux v1.7.3/go.mod h1:1lud6UwP+6orDFRuTfBEV8e9/aOM/c4fVVCaMa2zaAs= +github.com/gorilla/securecookie v1.1.1 h1:miw7JPhV+b/lAHSXz4qd/nN9jRiAFV5FwjeKyCS8BvQ= +github.com/gorilla/securecookie v1.1.1/go.mod h1:ra0sb63/xPlUeL+yeDciTfxMRAA+MP+HVt/4epWDjd4= github.com/gorilla/websocket v0.0.0-20170926233335-4201258b820c/go.mod h1:E7qHFY5m1UJ88s3WnNqhKjPHQ0heANvMoAMk2YaljkQ= github.com/gorilla/websocket v1.4.0/go.mod h1:E7qHFY5m1UJ88s3WnNqhKjPHQ0heANvMoAMk2YaljkQ= github.com/gorilla/websocket v1.4.2 h1:+/TMaTYc4QFitKJxsQ7Yye35DkWvkdLcvGKqM+x0Ufc= diff --git a/internal/oidc/auth/auth_handler.go b/internal/oidc/auth/auth_handler.go index 7eb31e316..f562a3930 100644 --- a/internal/oidc/auth/auth_handler.go +++ b/internal/oidc/auth/auth_handler.go @@ -9,30 +9,42 @@ import ( "net/http" "time" - "go.pinniped.dev/internal/plog" - "github.com/ory/fosite" "github.com/ory/fosite/handler/openid" "github.com/ory/fosite/token/jwt" + "golang.org/x/oauth2" + "go.pinniped.dev/internal/httputil/httperr" + "go.pinniped.dev/internal/oidc/csrftoken" "go.pinniped.dev/internal/oidc/provider" "go.pinniped.dev/internal/oidcclient/nonce" "go.pinniped.dev/internal/oidcclient/pkce" - "go.pinniped.dev/internal/oidcclient/state" - "golang.org/x/oauth2" + "go.pinniped.dev/internal/plog" +) + +const ( + // Just in case we need to make a breaking change to the format of the upstream state param, + // we are including a format version number. This gives the opportunity for a future version of Pinniped + // to have the consumer of this format decide to reject versions that it doesn't understand. + stateParamFormatVersion = "1" ) type IDPListGetter interface { GetIDPList() []provider.UpstreamOIDCIdentityProvider } +type Encoder interface { + Encode(name string, value interface{}) (string, error) +} + func NewHandler( issuer string, idpListGetter IDPListGetter, oauthHelper fosite.OAuth2Provider, - generateState func() (state.State, error), + generateCSRF func() (csrftoken.CSRFToken, error), generatePKCE func() (pkce.Code, error), generateNonce func() (nonce.Nonce, error), + encoder Encoder, ) http.Handler { return httperr.HandlerFunc(func(w http.ResponseWriter, r *http.Request) error { if r.Method != http.MethodPost && r.Method != http.MethodGet { @@ -77,7 +89,7 @@ func NewHandler( return nil } - stateValue, nonceValue, pkceValue, err := generateParams(generateState, generateNonce, generatePKCE) + csrfValue, nonceValue, pkceValue, err := generateValues(generateCSRF, generateNonce, generatePKCE) if err != nil { plog.InfoErr("authorize generate error", err) return err @@ -92,9 +104,28 @@ func NewHandler( Scopes: upstreamIDP.Scopes, } + // `__Host` prefix has a special meaning. See https://developer.mozilla.org/en-US/docs/Web/HTTP/Cookies#Cookie_prefixes + http.SetCookie(w, &http.Cookie{ + Name: "__Host-pinniped-csrf", + Value: string(csrfValue), + HttpOnly: true, + SameSite: http.SameSiteStrictMode, + Secure: true, + }) + + stateParamData := upstreamStateParamData{ + AuthParams: authorizeRequester.GetRequestForm().Encode(), + Nonce: nonceValue, + CSRFToken: csrfValue, + PKCECode: pkceValue, + StateParamFormatVersion: stateParamFormatVersion, + } + encodedStateParamValue, err := encoder.Encode("s", stateParamData) + // TODO handle the above error + http.Redirect(w, r, upstreamOAuthConfig.AuthCodeURL( - stateValue.String(), + encodedStateParamValue, oauth2.AccessTypeOffline, nonceValue.Param(), pkceValue.Challenge(), @@ -107,6 +138,15 @@ func NewHandler( }) } +// Keep the JSON to a minimal size because the upstream provider could impose size limitations on the state param. +type upstreamStateParamData struct { + AuthParams string `json:"p"` + Nonce nonce.Nonce `json:"n"` + CSRFToken csrftoken.CSRFToken `json:"c"` + PKCECode pkce.Code `json:"k"` + StateParamFormatVersion string `json:"v"` +} + func chooseUpstreamIDP(idpListGetter IDPListGetter) (*provider.UpstreamOIDCIdentityProvider, error) { allUpstreamIDPs := idpListGetter.GetIDPList() if len(allUpstreamIDPs) == 0 { @@ -123,15 +163,15 @@ func chooseUpstreamIDP(idpListGetter IDPListGetter) (*provider.UpstreamOIDCIdent return &allUpstreamIDPs[0], nil } -func generateParams( - generateState func() (state.State, error), +func generateValues( + generateCSRF func() (csrftoken.CSRFToken, error), generateNonce func() (nonce.Nonce, error), generatePKCE func() (pkce.Code, error), -) (state.State, nonce.Nonce, pkce.Code, error) { - stateValue, err := generateState() +) (csrftoken.CSRFToken, nonce.Nonce, pkce.Code, error) { + csrfValue, err := generateCSRF() if err != nil { - plog.InfoErr("error generating state param", err) - return "", "", "", httperr.Wrap(http.StatusInternalServerError, "error generating state param", err) + plog.InfoErr("error generating csrf param", err) + return "", "", "", httperr.Wrap(http.StatusInternalServerError, "error generating CSRF token", err) } nonceValue, err := generateNonce() if err != nil { @@ -143,7 +183,7 @@ func generateParams( plog.InfoErr("error generating PKCE param", err) return "", "", "", httperr.Wrap(http.StatusInternalServerError, "error generating PKCE param", err) } - return stateValue, nonceValue, pkceValue, nil + return csrfValue, nonceValue, pkceValue, nil } func fositeErrorForLog(err error) []interface{} { diff --git a/internal/oidc/auth/auth_handler_test.go b/internal/oidc/auth/auth_handler_test.go index 399605e39..cc708f1a6 100644 --- a/internal/oidc/auth/auth_handler_test.go +++ b/internal/oidc/auth/auth_handler_test.go @@ -13,16 +13,17 @@ import ( "strings" "testing" + "github.com/gorilla/securecookie" "github.com/ory/fosite" "github.com/ory/fosite/storage" "github.com/stretchr/testify/require" "go.pinniped.dev/internal/here" "go.pinniped.dev/internal/oidc" + "go.pinniped.dev/internal/oidc/csrftoken" "go.pinniped.dev/internal/oidc/provider" "go.pinniped.dev/internal/oidcclient/nonce" "go.pinniped.dev/internal/oidcclient/pkce" - "go.pinniped.dev/internal/oidcclient/state" ) func TestAuthorizationEndpoint(t *testing.T) { @@ -131,30 +132,37 @@ func TestAuthorizationEndpoint(t *testing.T) { require.GreaterOrEqual(t, len(hmacSecret), 32, "fosite requires that hmac secrets have at least 32 bytes") oauthHelper := oidc.FositeOauth2Helper(oauthStore, hmacSecret) - happyStateGenerator := func() (state.State, error) { return "test-state", nil } - happyPKCEGenerator := func() (pkce.Code, error) { return "test-pkce", nil } - happyNonceGenerator := func() (nonce.Nonce, error) { return "test-nonce", nil } + happyCSRF := "test-csrf" + happyPKCE := "test-pkce" + happyNonce := "test-nonce" + happyCSRFGenerator := func() (csrftoken.CSRFToken, error) { return csrftoken.CSRFToken(happyCSRF), nil } + happyPKCEGenerator := func() (pkce.Code, error) { return pkce.Code(happyPKCE), nil } + happyNonceGenerator := func() (nonce.Nonce, error) { return nonce.Nonce(happyNonce), nil } // This is the PKCE challenge which is calculated as base64(sha256("test-pkce")). For example: // $ echo -n test-pkce | shasum -a 256 | cut -d" " -f1 | xxd -r -p | base64 | cut -d"=" -f1 expectedUpstreamCodeChallenge := "VVaezYqum7reIhoavCHD1n2d-piN3r_mywoYj7fCR7g" - pathWithQuery := func(path string, query map[string]string) string { + var encoderHashKey = []byte("fake-hash-secret") + var encoder = securecookie.New(encoderHashKey, nil) // note that nil block key argument turns off encryption + encoder.SetSerializer(securecookie.JSONEncoder{}) + + encodeQuery := func(query map[string]string) string { values := url.Values{} for k, v := range query { values[k] = []string{v} } - pathToReturn := fmt.Sprintf("%s?%s", path, values.Encode()) + return values.Encode() + } + + pathWithQuery := func(path string, query map[string]string) string { + pathToReturn := fmt.Sprintf("%s?%s", path, encodeQuery(query)) require.NotRegexp(t, "^http", pathToReturn, "pathWithQuery helper was used to create a URL") return pathToReturn } urlWithQuery := func(baseURL string, query map[string]string) string { - values := url.Values{} - for k, v := range query { - values[k] = []string{v} - } - urlToReturn := fmt.Sprintf("%s?%s", baseURL, values.Encode()) + urlToReturn := fmt.Sprintf("%s?%s", baseURL, encodeQuery(query)) _, err := url.Parse(urlToReturn) require.NoError(t, err, "urlWithQuery helper was used to create an illegal URL") return urlToReturn @@ -189,26 +197,47 @@ func TestAuthorizationEndpoint(t *testing.T) { return pathWithQuery("/some/path", copyOfHappyGetRequestQueryMap) } + // We're going to use this value to make assertions, so specify the exact expected value. + happyUpstreamStateParam, err := encoder.Encode("s", + // Ensure that the order of the serialized fields is exactly this order, so we can make simpler equality assertions below. + struct { + P string `json:"p"` + N string `json:"n"` + C string `json:"c"` + K string `json:"k"` + V string `json:"v"` + }{ + P: encodeQuery(happyGetRequestQueryMap), + N: happyNonce, + C: happyCSRF, + K: happyPKCE, + V: "1", + }, + ) + require.NoError(t, err) + happyGetRequestExpectedRedirectLocation := urlWithQuery(upstreamAuthURL.String(), map[string]string{ "response_type": "code", "access_type": "offline", "scope": "scope1 scope2", "client_id": "some-client-id", - "state": "test-state", - "nonce": "test-nonce", + "state": happyUpstreamStateParam, + "nonce": happyNonce, "code_challenge": expectedUpstreamCodeChallenge, "code_challenge_method": "S256", "redirect_uri": issuer + "/callback/some-idp", }, ) + happyCSRFSetCookieHeaderValue := fmt.Sprintf("__Host-pinniped-csrf=%s; HttpOnly; Secure; SameSite=Strict", happyCSRF) + type testCase struct { name string issuer string idpListGetter provider.DynamicUpstreamIDPProvider - generateState func() (state.State, error) + generateCSRF func() (csrftoken.CSRFToken, error) generatePKCE func() (pkce.Code, error) generateNonce func() (nonce.Nonce, error) method string @@ -216,11 +245,12 @@ func TestAuthorizationEndpoint(t *testing.T) { contentType string body string - wantStatus int - wantContentType string - wantBodyString string - wantBodyJSON string - wantLocationHeader string + wantStatus int + wantContentType string + wantBodyString string + wantBodyJSON string + wantLocationHeader string + wantCSRFCookieHeader string } tests := []testCase{ @@ -228,7 +258,7 @@ func TestAuthorizationEndpoint(t *testing.T) { name: "happy path using GET", issuer: issuer, idpListGetter: newIDPListGetter(upstreamOIDCIdentityProvider), - generateState: happyStateGenerator, + generateCSRF: happyCSRFGenerator, generatePKCE: happyPKCEGenerator, generateNonce: happyNonceGenerator, method: http.MethodGet, @@ -239,13 +269,14 @@ func TestAuthorizationEndpoint(t *testing.T) { html.EscapeString(happyGetRequestExpectedRedirectLocation), "\n\n", ), - wantLocationHeader: happyGetRequestExpectedRedirectLocation, + wantLocationHeader: happyGetRequestExpectedRedirectLocation, + wantCSRFCookieHeader: happyCSRFSetCookieHeaderValue, }, { name: "happy path using POST", issuer: issuer, idpListGetter: newIDPListGetter(upstreamOIDCIdentityProvider), - generateState: happyStateGenerator, + generateCSRF: happyCSRFGenerator, generatePKCE: happyPKCEGenerator, generateNonce: happyNonceGenerator, method: http.MethodPost, @@ -260,16 +291,17 @@ func TestAuthorizationEndpoint(t *testing.T) { "code_challenge_method": []string{"S256"}, "redirect_uri": []string{downstreamRedirectURI}, }.Encode(), - wantStatus: http.StatusFound, - wantContentType: "", - wantBodyString: "", - wantLocationHeader: happyGetRequestExpectedRedirectLocation, + wantStatus: http.StatusFound, + wantContentType: "", + wantBodyString: "", + wantLocationHeader: happyGetRequestExpectedRedirectLocation, + wantCSRFCookieHeader: happyCSRFSetCookieHeaderValue, }, { name: "downstream client does not exist", issuer: issuer, idpListGetter: newIDPListGetter(upstreamOIDCIdentityProvider), - generateState: happyStateGenerator, + generateCSRF: happyCSRFGenerator, generatePKCE: happyPKCEGenerator, generateNonce: happyNonceGenerator, method: http.MethodGet, @@ -282,7 +314,7 @@ func TestAuthorizationEndpoint(t *testing.T) { name: "downstream redirect uri does not match what is configured for client", issuer: issuer, idpListGetter: newIDPListGetter(upstreamOIDCIdentityProvider), - generateState: happyStateGenerator, + generateCSRF: happyCSRFGenerator, generatePKCE: happyPKCEGenerator, generateNonce: happyNonceGenerator, method: http.MethodGet, @@ -294,10 +326,10 @@ func TestAuthorizationEndpoint(t *testing.T) { wantBodyJSON: fositeInvalidRedirectURIErrorBody, }, { - name: "downstream redirect uri matches what is configured for client except for the port number", + name: "happy path when downstream redirect uri matches what is configured for client except for the port number", issuer: issuer, idpListGetter: newIDPListGetter(upstreamOIDCIdentityProvider), - generateState: happyStateGenerator, + generateCSRF: happyCSRFGenerator, generatePKCE: happyPKCEGenerator, generateNonce: happyNonceGenerator, method: http.MethodGet, @@ -310,13 +342,14 @@ func TestAuthorizationEndpoint(t *testing.T) { html.EscapeString(happyGetRequestExpectedRedirectLocation), "\n\n", ), - wantLocationHeader: happyGetRequestExpectedRedirectLocation, + wantLocationHeader: happyGetRequestExpectedRedirectLocation, + wantCSRFCookieHeader: happyCSRFSetCookieHeaderValue, }, { name: "response type is unsupported", issuer: issuer, idpListGetter: newIDPListGetter(upstreamOIDCIdentityProvider), - generateState: happyStateGenerator, + generateCSRF: happyCSRFGenerator, generatePKCE: happyPKCEGenerator, generateNonce: happyNonceGenerator, method: http.MethodGet, @@ -330,7 +363,7 @@ func TestAuthorizationEndpoint(t *testing.T) { name: "downstream scopes do not match what is configured for client", issuer: issuer, idpListGetter: newIDPListGetter(upstreamOIDCIdentityProvider), - generateState: happyStateGenerator, + generateCSRF: happyCSRFGenerator, generatePKCE: happyPKCEGenerator, generateNonce: happyNonceGenerator, method: http.MethodGet, @@ -344,7 +377,7 @@ func TestAuthorizationEndpoint(t *testing.T) { name: "missing response type in request", issuer: issuer, idpListGetter: newIDPListGetter(upstreamOIDCIdentityProvider), - generateState: happyStateGenerator, + generateCSRF: happyCSRFGenerator, generatePKCE: happyPKCEGenerator, generateNonce: happyNonceGenerator, method: http.MethodGet, @@ -358,7 +391,7 @@ func TestAuthorizationEndpoint(t *testing.T) { name: "missing client id in request", issuer: issuer, idpListGetter: newIDPListGetter(upstreamOIDCIdentityProvider), - generateState: happyStateGenerator, + generateCSRF: happyCSRFGenerator, generatePKCE: happyPKCEGenerator, generateNonce: happyNonceGenerator, method: http.MethodGet, @@ -371,7 +404,7 @@ func TestAuthorizationEndpoint(t *testing.T) { name: "missing PKCE code_challenge in request", // See https://tools.ietf.org/html/rfc7636#section-4.4.1 issuer: issuer, idpListGetter: newIDPListGetter(upstreamOIDCIdentityProvider), - generateState: happyStateGenerator, + generateCSRF: happyCSRFGenerator, generatePKCE: happyPKCEGenerator, generateNonce: happyNonceGenerator, method: http.MethodGet, @@ -385,7 +418,7 @@ func TestAuthorizationEndpoint(t *testing.T) { name: "invalid value for PKCE code_challenge_method in request", // https://tools.ietf.org/html/rfc7636#section-4.3 issuer: issuer, idpListGetter: newIDPListGetter(upstreamOIDCIdentityProvider), - generateState: happyStateGenerator, + generateCSRF: happyCSRFGenerator, generatePKCE: happyPKCEGenerator, generateNonce: happyNonceGenerator, method: http.MethodGet, @@ -399,7 +432,7 @@ func TestAuthorizationEndpoint(t *testing.T) { name: "when PKCE code_challenge_method in request is `plain`", // https://tools.ietf.org/html/rfc7636#section-4.3 issuer: issuer, idpListGetter: newIDPListGetter(upstreamOIDCIdentityProvider), - generateState: happyStateGenerator, + generateCSRF: happyCSRFGenerator, generatePKCE: happyPKCEGenerator, generateNonce: happyNonceGenerator, method: http.MethodGet, @@ -413,7 +446,7 @@ func TestAuthorizationEndpoint(t *testing.T) { name: "missing PKCE code_challenge_method in request", // See https://tools.ietf.org/html/rfc7636#section-4.4.1 issuer: issuer, idpListGetter: newIDPListGetter(upstreamOIDCIdentityProvider), - generateState: happyStateGenerator, + generateCSRF: happyCSRFGenerator, generatePKCE: happyPKCEGenerator, generateNonce: happyNonceGenerator, method: http.MethodGet, @@ -429,7 +462,7 @@ func TestAuthorizationEndpoint(t *testing.T) { name: "prompt param is not allowed to have none and another legal value at the same time", issuer: issuer, idpListGetter: newIDPListGetter(upstreamOIDCIdentityProvider), - generateState: happyStateGenerator, + generateCSRF: happyCSRFGenerator, generatePKCE: happyPKCEGenerator, generateNonce: happyNonceGenerator, method: http.MethodGet, @@ -443,7 +476,7 @@ func TestAuthorizationEndpoint(t *testing.T) { name: "state does not have enough entropy", issuer: issuer, idpListGetter: newIDPListGetter(upstreamOIDCIdentityProvider), - generateState: happyStateGenerator, + generateCSRF: happyCSRFGenerator, generatePKCE: happyPKCEGenerator, generateNonce: happyNonceGenerator, method: http.MethodGet, @@ -454,23 +487,23 @@ func TestAuthorizationEndpoint(t *testing.T) { wantBodyString: "", }, { - name: "error while generating state", + name: "error while generating CSRF token", issuer: issuer, idpListGetter: newIDPListGetter(upstreamOIDCIdentityProvider), - generateState: func() (state.State, error) { return "", fmt.Errorf("some state generator error") }, + generateCSRF: func() (csrftoken.CSRFToken, error) { return "", fmt.Errorf("some csrf generator error") }, generatePKCE: happyPKCEGenerator, generateNonce: happyNonceGenerator, method: http.MethodGet, path: happyGetRequestPath, wantStatus: http.StatusInternalServerError, wantContentType: "text/plain; charset=utf-8", - wantBodyString: "Internal Server Error: error generating state param\n", + wantBodyString: "Internal Server Error: error generating CSRF token\n", }, { name: "error while generating nonce", issuer: issuer, idpListGetter: newIDPListGetter(upstreamOIDCIdentityProvider), - generateState: happyStateGenerator, + generateCSRF: happyCSRFGenerator, generatePKCE: happyPKCEGenerator, generateNonce: func() (nonce.Nonce, error) { return "", fmt.Errorf("some nonce generator error") }, method: http.MethodGet, @@ -483,7 +516,7 @@ func TestAuthorizationEndpoint(t *testing.T) { name: "error while generating PKCE", issuer: issuer, idpListGetter: newIDPListGetter(upstreamOIDCIdentityProvider), - generateState: happyStateGenerator, + generateCSRF: happyCSRFGenerator, generatePKCE: func() (pkce.Code, error) { return "", fmt.Errorf("some PKCE generator error") }, generateNonce: happyNonceGenerator, method: http.MethodGet, @@ -562,13 +595,23 @@ func TestAuthorizationEndpoint(t *testing.T) { if test.wantLocationHeader != "" { actualLocation := rsp.Header().Get("Location") requireEqualURLs(t, actualLocation, test.wantLocationHeader) + } else { + require.Empty(t, rsp.Header().Values("Location")) + } + + if test.wantCSRFCookieHeader != "" { + require.Len(t, rsp.Header().Values("Set-Cookie"), 1) + actualCookie := rsp.Header().Get("Set-Cookie") + require.Equal(t, actualCookie, test.wantCSRFCookieHeader) + } else { + require.Empty(t, rsp.Header().Values("Set-Cookie")) } } for _, test := range tests { test := test t.Run(test.name, func(t *testing.T) { - subject := NewHandler(test.issuer, test.idpListGetter, oauthHelper, test.generateState, test.generatePKCE, test.generateNonce) + subject := NewHandler(test.issuer, test.idpListGetter, oauthHelper, test.generateCSRF, test.generatePKCE, test.generateNonce, encoder) runOneTestCase(t, test, subject) }) } @@ -577,7 +620,7 @@ func TestAuthorizationEndpoint(t *testing.T) { test := tests[0] require.Equal(t, "happy path using GET", test.name) // re-use the happy path test case - subject := NewHandler(test.issuer, test.idpListGetter, oauthHelper, test.generateState, test.generatePKCE, test.generateNonce) + subject := NewHandler(test.issuer, test.idpListGetter, oauthHelper, test.generateCSRF, test.generatePKCE, test.generateNonce, encoder) runOneTestCase(t, test, subject) @@ -597,8 +640,8 @@ func TestAuthorizationEndpoint(t *testing.T) { "access_type": "offline", "scope": "other-scope1 other-scope2", "client_id": "some-other-client-id", - "state": "test-state", - "nonce": "test-nonce", + "state": happyUpstreamStateParam, + "nonce": happyNonce, "code_challenge": expectedUpstreamCodeChallenge, "code_challenge_method": "S256", "redirect_uri": issuer + "/callback/some-other-idp", diff --git a/internal/oidc/csrftoken/csrftoken.go b/internal/oidc/csrftoken/csrftoken.go new file mode 100644 index 000000000..bc7b713cd --- /dev/null +++ b/internal/oidc/csrftoken/csrftoken.go @@ -0,0 +1,24 @@ +// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package csrftoken + +import ( + "crypto/rand" + "encoding/hex" + "fmt" + "io" +) + +// Generate generates a new random CSRF token value. +func Generate() (CSRFToken, error) { return generate(rand.Reader) } + +func generate(rand io.Reader) (CSRFToken, error) { + var buf [32]byte + if _, err := io.ReadFull(rand, buf[:]); err != nil { + return "", fmt.Errorf("could not generate CSRFToken: %w", err) + } + return CSRFToken(hex.EncodeToString(buf[:])), nil +} + +type CSRFToken string diff --git a/internal/oidc/provider/manager/manager.go b/internal/oidc/provider/manager/manager.go index e364e8fe9..77112fca0 100644 --- a/internal/oidc/provider/manager/manager.go +++ b/internal/oidc/provider/manager/manager.go @@ -8,6 +8,9 @@ import ( "strings" "sync" + "github.com/gorilla/securecookie" + "go.pinniped.dev/internal/oidc/csrftoken" + "github.com/ory/fosite" "github.com/ory/fosite/storage" @@ -18,7 +21,6 @@ import ( "go.pinniped.dev/internal/oidc/provider" "go.pinniped.dev/internal/oidcclient/nonce" "go.pinniped.dev/internal/oidcclient/pkce" - "go.pinniped.dev/internal/oidcclient/state" "go.pinniped.dev/internal/plog" ) @@ -78,8 +80,13 @@ func (m *Manager) SetProviders(oidcProviders ...*provider.OIDCProvider) { } oauthHelper := oidc.FositeOauth2Helper(oauthStore, []byte("some secret - must have at least 32 bytes")) // TODO replace this secret + var encoderHashKey = []byte("fake-hash-secret") // TODO fix this + var encoderBlockKey = []byte("16-bytes-aaaaaaa") // TODO fix this + var encoder = securecookie.New(encoderHashKey, encoderBlockKey) + encoder.SetSerializer(securecookie.JSONEncoder{}) + authURL := strings.ToLower(incomingProvider.IssuerHost()) + "/" + incomingProvider.IssuerPath() + oidc.AuthorizationEndpointPath - m.providerHandlers[authURL] = auth.NewHandler(incomingProvider.Issuer(), m.idpListGetter, oauthHelper, state.Generate, pkce.Generate, nonce.Generate) + m.providerHandlers[authURL] = auth.NewHandler(incomingProvider.Issuer(), m.idpListGetter, oauthHelper, csrftoken.Generate, pkce.Generate, nonce.Generate, encoder) plog.Debug("oidc provider manager added or updated issuer", "issuer", incomingProvider.Issuer()) } From c2262773e6e5b5d75a90e82ad60e91326537177f Mon Sep 17 00:00:00 2001 From: Andrew Keesler Date: Wed, 11 Nov 2020 12:29:14 -0800 Subject: [PATCH 18/21] Finish the WIP from the previous commit for saving authorize endpoint state Signed-off-by: Ryan Richard --- internal/oidc/auth/auth_handler.go | 90 ++++--- internal/oidc/auth/auth_handler_test.go | 272 ++++++++++++++-------- internal/oidc/provider/manager/manager.go | 3 +- internal/plog/doc.go | 22 -- internal/plog/plog.go | 44 +++- 5 files changed, 273 insertions(+), 158 deletions(-) delete mode 100644 internal/plog/doc.go diff --git a/internal/oidc/auth/auth_handler.go b/internal/oidc/auth/auth_handler.go index f562a3930..b47d4984e 100644 --- a/internal/oidc/auth/auth_handler.go +++ b/internal/oidc/auth/auth_handler.go @@ -26,13 +26,22 @@ const ( // Just in case we need to make a breaking change to the format of the upstream state param, // we are including a format version number. This gives the opportunity for a future version of Pinniped // to have the consumer of this format decide to reject versions that it doesn't understand. - stateParamFormatVersion = "1" + upstreamStateParamFormatVersion = "1" + + // The `name` passed to the encoder for encoding the upstream state param value. This name is short + // because it will be encoded into the upstream state param value and we're trying to keep that small. + upstreamStateParamEncodingName = "s" + + // The name of the browser cookie which shall hold our CSRF value. + // `__Host` prefix has a special meaning. See https://developer.mozilla.org/en-US/docs/Web/HTTP/Cookies#Cookie_prefixes + csrfCookieName = "__Host-pinniped-csrf" ) type IDPListGetter interface { GetIDPList() []provider.UpstreamOIDCIdentityProvider } +// This is the encoding side of the securecookie.Codec interface. type Encoder interface { Encode(name string, value interface{}) (string, error) } @@ -63,7 +72,7 @@ func NewHandler( upstreamIDP, err := chooseUpstreamIDP(idpListGetter) if err != nil { - plog.InfoErr("authorize request error", err) + plog.WarningErr("authorize upstream config", err) return err } @@ -91,7 +100,7 @@ func NewHandler( csrfValue, nonceValue, pkceValue, err := generateValues(generateCSRF, generateNonce, generatePKCE) if err != nil { - plog.InfoErr("authorize generate error", err) + plog.Error("authorize generate error", err) return err } @@ -104,24 +113,13 @@ func NewHandler( Scopes: upstreamIDP.Scopes, } - // `__Host` prefix has a special meaning. See https://developer.mozilla.org/en-US/docs/Web/HTTP/Cookies#Cookie_prefixes - http.SetCookie(w, &http.Cookie{ - Name: "__Host-pinniped-csrf", - Value: string(csrfValue), - HttpOnly: true, - SameSite: http.SameSiteStrictMode, - Secure: true, - }) - - stateParamData := upstreamStateParamData{ - AuthParams: authorizeRequester.GetRequestForm().Encode(), - Nonce: nonceValue, - CSRFToken: csrfValue, - PKCECode: pkceValue, - StateParamFormatVersion: stateParamFormatVersion, + encodedStateParamValue, err := upstreamStateParam(authorizeRequester, nonceValue, csrfValue, pkceValue, encoder) + if err != nil { + plog.Error("authorize upstream state param error", err) + return err } - encodedStateParamValue, err := encoder.Encode("s", stateParamData) - // TODO handle the above error + + addCSRFSetCookieHeader(w, csrfValue) http.Redirect(w, r, upstreamOAuthConfig.AuthCodeURL( @@ -138,15 +136,6 @@ func NewHandler( }) } -// Keep the JSON to a minimal size because the upstream provider could impose size limitations on the state param. -type upstreamStateParamData struct { - AuthParams string `json:"p"` - Nonce nonce.Nonce `json:"n"` - CSRFToken csrftoken.CSRFToken `json:"c"` - PKCECode pkce.Code `json:"k"` - StateParamFormatVersion string `json:"v"` -} - func chooseUpstreamIDP(idpListGetter IDPListGetter) (*provider.UpstreamOIDCIdentityProvider, error) { allUpstreamIDPs := idpListGetter.GetIDPList() if len(allUpstreamIDPs) == 0 { @@ -170,22 +159,59 @@ func generateValues( ) (csrftoken.CSRFToken, nonce.Nonce, pkce.Code, error) { csrfValue, err := generateCSRF() if err != nil { - plog.InfoErr("error generating csrf param", err) return "", "", "", httperr.Wrap(http.StatusInternalServerError, "error generating CSRF token", err) } nonceValue, err := generateNonce() if err != nil { - plog.InfoErr("error generating nonce param", err) return "", "", "", httperr.Wrap(http.StatusInternalServerError, "error generating nonce param", err) } pkceValue, err := generatePKCE() if err != nil { - plog.InfoErr("error generating PKCE param", err) return "", "", "", httperr.Wrap(http.StatusInternalServerError, "error generating PKCE param", err) } return csrfValue, nonceValue, pkceValue, nil } +// Keep the JSON to a minimal size because the upstream provider could impose size limitations on the state param. +type upstreamStateParamData struct { + AuthParams string `json:"p"` + Nonce nonce.Nonce `json:"n"` + CSRFToken csrftoken.CSRFToken `json:"c"` + PKCECode pkce.Code `json:"k"` + StateParamFormatVersion string `json:"v"` +} + +func upstreamStateParam( + authorizeRequester fosite.AuthorizeRequester, + nonceValue nonce.Nonce, + csrfValue csrftoken.CSRFToken, + pkceValue pkce.Code, + encoder Encoder, +) (string, error) { + stateParamData := upstreamStateParamData{ + AuthParams: authorizeRequester.GetRequestForm().Encode(), + Nonce: nonceValue, + CSRFToken: csrfValue, + PKCECode: pkceValue, + StateParamFormatVersion: upstreamStateParamFormatVersion, + } + encodedStateParamValue, err := encoder.Encode(upstreamStateParamEncodingName, stateParamData) + if err != nil { + return "", httperr.Wrap(http.StatusInternalServerError, "error encoding upstream state param", err) + } + return encodedStateParamValue, nil +} + +func addCSRFSetCookieHeader(w http.ResponseWriter, csrfValue csrftoken.CSRFToken) { + http.SetCookie(w, &http.Cookie{ + Name: csrfCookieName, + Value: string(csrfValue), + HttpOnly: true, + SameSite: http.SameSiteStrictMode, + Secure: true, + }) +} + func fositeErrorForLog(err error) []interface{} { rfc6749Error := fosite.ErrorToRFC6749Error(err) keysAndValues := make([]interface{}, 0) diff --git a/internal/oidc/auth/auth_handler_test.go b/internal/oidc/auth/auth_handler_test.go index cc708f1a6..465579378 100644 --- a/internal/oidc/auth/auth_handler_test.go +++ b/internal/oidc/auth/auth_handler_test.go @@ -28,7 +28,8 @@ import ( func TestAuthorizationEndpoint(t *testing.T) { const ( - downstreamRedirectURI = "http://127.0.0.1/callback" + downstreamRedirectURI = "http://127.0.0.1/callback" + downstreamRedirectURIWithDifferentPort = "http://127.0.0.1:42/callback" ) var ( @@ -144,8 +145,8 @@ func TestAuthorizationEndpoint(t *testing.T) { expectedUpstreamCodeChallenge := "VVaezYqum7reIhoavCHD1n2d-piN3r_mywoYj7fCR7g" var encoderHashKey = []byte("fake-hash-secret") - var encoder = securecookie.New(encoderHashKey, nil) // note that nil block key argument turns off encryption - encoder.SetSerializer(securecookie.JSONEncoder{}) + var happyEncoder = securecookie.New(encoderHashKey, nil) // note that nil block key argument turns off encryption + happyEncoder.SetSerializer(securecookie.JSONEncoder{}) encodeQuery := func(query map[string]string) string { values := url.Values{} @@ -168,22 +169,24 @@ func TestAuthorizationEndpoint(t *testing.T) { return urlToReturn } - happyGetRequestQueryMap := map[string]string{ - "response_type": "code", - "scope": "openid profile email", - "client_id": "pinniped-cli", - "state": "some-state-value", - "nonce": "some-nonce-value", - "code_challenge": "some-challenge", - "code_challenge_method": "S256", - "redirect_uri": downstreamRedirectURI, + happyGetRequestQueryMap := func(downstreamRedirectURI string) map[string]string { + return map[string]string{ + "response_type": "code", + "scope": "openid profile email", + "client_id": "pinniped-cli", + "state": "some-state-value", + "nonce": "some-nonce-value", + "code_challenge": "some-challenge", + "code_challenge_method": "S256", + "redirect_uri": downstreamRedirectURI, + } } - happyGetRequestPath := pathWithQuery("/some/path", happyGetRequestQueryMap) + happyGetRequestPath := pathWithQuery("/some/path", happyGetRequestQueryMap(downstreamRedirectURI)) modifiedHappyGetRequestPath := func(queryOverrides map[string]string) string { copyOfHappyGetRequestQueryMap := map[string]string{} - for k, v := range happyGetRequestQueryMap { + for k, v := range happyGetRequestQueryMap(downstreamRedirectURI) { copyOfHappyGetRequestQueryMap[k] = v } for k, v := range queryOverrides { @@ -197,38 +200,33 @@ func TestAuthorizationEndpoint(t *testing.T) { return pathWithQuery("/some/path", copyOfHappyGetRequestQueryMap) } - // We're going to use this value to make assertions, so specify the exact expected value. - happyUpstreamStateParam, err := encoder.Encode("s", - // Ensure that the order of the serialized fields is exactly this order, so we can make simpler equality assertions below. - struct { - P string `json:"p"` - N string `json:"n"` - C string `json:"c"` - K string `json:"k"` - V string `json:"v"` - }{ - P: encodeQuery(happyGetRequestQueryMap), - N: happyNonce, - C: happyCSRF, - K: happyPKCE, - V: "1", - }, - ) - require.NoError(t, err) + happyExpectedUpstreamStateParam := func(downstreamRedirectURI string) string { + encoded, err := happyEncoder.Encode("s", + expectedUpstreamStateParamFormat{ + P: encodeQuery(happyGetRequestQueryMap(downstreamRedirectURI)), + N: happyNonce, + C: happyCSRF, + K: happyPKCE, + V: "1", + }, + ) + require.NoError(t, err) + return encoded + } - happyGetRequestExpectedRedirectLocation := urlWithQuery(upstreamAuthURL.String(), - map[string]string{ + happyExpectedRedirectLocation := func(downstreamRedirectURI string) string { + return urlWithQuery(upstreamAuthURL.String(), map[string]string{ "response_type": "code", "access_type": "offline", "scope": "scope1 scope2", "client_id": "some-client-id", - "state": happyUpstreamStateParam, + "state": happyExpectedUpstreamStateParam(downstreamRedirectURI), "nonce": happyNonce, "code_challenge": expectedUpstreamCodeChallenge, "code_challenge_method": "S256", "redirect_uri": issuer + "/callback/some-idp", - }, - ) + }) + } happyCSRFSetCookieHeaderValue := fmt.Sprintf("__Host-pinniped-csrf=%s; HttpOnly; Secure; SameSite=Strict", happyCSRF) @@ -240,6 +238,7 @@ func TestAuthorizationEndpoint(t *testing.T) { generateCSRF func() (csrftoken.CSRFToken, error) generatePKCE func() (pkce.Code, error) generateNonce func() (nonce.Nonce, error) + encoder securecookie.Codec method string path string contentType string @@ -251,8 +250,9 @@ func TestAuthorizationEndpoint(t *testing.T) { wantBodyJSON string wantLocationHeader string wantCSRFCookieHeader string - } + wantUpstreamStateParamInLocationHeader bool + } tests := []testCase{ { name: "happy path using GET", @@ -261,54 +261,59 @@ func TestAuthorizationEndpoint(t *testing.T) { generateCSRF: happyCSRFGenerator, generatePKCE: happyPKCEGenerator, generateNonce: happyNonceGenerator, + encoder: happyEncoder, method: http.MethodGet, path: happyGetRequestPath, wantStatus: http.StatusFound, wantContentType: "text/html; charset=utf-8", wantBodyString: fmt.Sprintf(`Found.%s`, - html.EscapeString(happyGetRequestExpectedRedirectLocation), + html.EscapeString(happyExpectedRedirectLocation(downstreamRedirectURI)), "\n\n", ), - wantLocationHeader: happyGetRequestExpectedRedirectLocation, - wantCSRFCookieHeader: happyCSRFSetCookieHeaderValue, + wantCSRFCookieHeader: happyCSRFSetCookieHeaderValue, + wantLocationHeader: happyExpectedRedirectLocation(downstreamRedirectURI), + wantUpstreamStateParamInLocationHeader: true, }, { - name: "happy path using POST", + name: "happy path using POST", + issuer: issuer, + idpListGetter: newIDPListGetter(upstreamOIDCIdentityProvider), + generateCSRF: happyCSRFGenerator, + generatePKCE: happyPKCEGenerator, + generateNonce: happyNonceGenerator, + encoder: happyEncoder, + method: http.MethodPost, + path: "/some/path", + contentType: "application/x-www-form-urlencoded", + body: encodeQuery(happyGetRequestQueryMap(downstreamRedirectURI)), + wantStatus: http.StatusFound, + wantContentType: "", + wantBodyString: "", + wantCSRFCookieHeader: happyCSRFSetCookieHeaderValue, + wantLocationHeader: happyExpectedRedirectLocation(downstreamRedirectURI), + wantUpstreamStateParamInLocationHeader: true, + }, + { + name: "happy path when downstream redirect uri matches what is configured for client except for the port number", issuer: issuer, idpListGetter: newIDPListGetter(upstreamOIDCIdentityProvider), generateCSRF: happyCSRFGenerator, generatePKCE: happyPKCEGenerator, generateNonce: happyNonceGenerator, - method: http.MethodPost, - path: "/some/path", - contentType: "application/x-www-form-urlencoded", - body: url.Values{ - "response_type": []string{"code"}, - "scope": []string{"openid profile email"}, - "client_id": []string{"pinniped-cli"}, - "state": []string{"some-state-value"}, - "code_challenge": []string{"some-challenge"}, - "code_challenge_method": []string{"S256"}, - "redirect_uri": []string{downstreamRedirectURI}, - }.Encode(), - wantStatus: http.StatusFound, - wantContentType: "", - wantBodyString: "", - wantLocationHeader: happyGetRequestExpectedRedirectLocation, - wantCSRFCookieHeader: happyCSRFSetCookieHeaderValue, - }, - { - name: "downstream client does not exist", - issuer: issuer, - idpListGetter: newIDPListGetter(upstreamOIDCIdentityProvider), - generateCSRF: happyCSRFGenerator, - generatePKCE: happyPKCEGenerator, - generateNonce: happyNonceGenerator, - method: http.MethodGet, - path: modifiedHappyGetRequestPath(map[string]string{"client_id": "invalid-client"}), - wantStatus: http.StatusUnauthorized, - wantContentType: "application/json; charset=utf-8", - wantBodyJSON: fositeInvalidClientErrorBody, + encoder: happyEncoder, + method: http.MethodGet, + path: modifiedHappyGetRequestPath(map[string]string{ + "redirect_uri": downstreamRedirectURIWithDifferentPort, // not the same port number that is registered for the client + }), + wantStatus: http.StatusFound, + wantContentType: "text/html; charset=utf-8", + wantBodyString: fmt.Sprintf(`Found.%s`, + html.EscapeString(happyExpectedRedirectLocation(downstreamRedirectURIWithDifferentPort)), + "\n\n", + ), + wantCSRFCookieHeader: happyCSRFSetCookieHeaderValue, + wantLocationHeader: happyExpectedRedirectLocation(downstreamRedirectURIWithDifferentPort), + wantUpstreamStateParamInLocationHeader: true, }, { name: "downstream redirect uri does not match what is configured for client", @@ -317,6 +322,7 @@ func TestAuthorizationEndpoint(t *testing.T) { generateCSRF: happyCSRFGenerator, generatePKCE: happyPKCEGenerator, generateNonce: happyNonceGenerator, + encoder: happyEncoder, method: http.MethodGet, path: modifiedHappyGetRequestPath(map[string]string{ "redirect_uri": "http://127.0.0.1/does-not-match-what-is-configured-for-pinniped-cli-client", @@ -326,24 +332,18 @@ func TestAuthorizationEndpoint(t *testing.T) { wantBodyJSON: fositeInvalidRedirectURIErrorBody, }, { - name: "happy path when downstream redirect uri matches what is configured for client except for the port number", - issuer: issuer, - idpListGetter: newIDPListGetter(upstreamOIDCIdentityProvider), - generateCSRF: happyCSRFGenerator, - generatePKCE: happyPKCEGenerator, - generateNonce: happyNonceGenerator, - method: http.MethodGet, - path: modifiedHappyGetRequestPath(map[string]string{ - "redirect_uri": "http://127.0.0.1:42/callback", - }), - wantStatus: http.StatusFound, - wantContentType: "text/html; charset=utf-8", - wantBodyString: fmt.Sprintf(`Found.%s`, - html.EscapeString(happyGetRequestExpectedRedirectLocation), - "\n\n", - ), - wantLocationHeader: happyGetRequestExpectedRedirectLocation, - wantCSRFCookieHeader: happyCSRFSetCookieHeaderValue, + name: "downstream client does not exist", + issuer: issuer, + idpListGetter: newIDPListGetter(upstreamOIDCIdentityProvider), + generateCSRF: happyCSRFGenerator, + generatePKCE: happyPKCEGenerator, + generateNonce: happyNonceGenerator, + encoder: happyEncoder, + method: http.MethodGet, + path: modifiedHappyGetRequestPath(map[string]string{"client_id": "invalid-client"}), + wantStatus: http.StatusUnauthorized, + wantContentType: "application/json; charset=utf-8", + wantBodyJSON: fositeInvalidClientErrorBody, }, { name: "response type is unsupported", @@ -352,6 +352,7 @@ func TestAuthorizationEndpoint(t *testing.T) { generateCSRF: happyCSRFGenerator, generatePKCE: happyPKCEGenerator, generateNonce: happyNonceGenerator, + encoder: happyEncoder, method: http.MethodGet, path: modifiedHappyGetRequestPath(map[string]string{"response_type": "unsupported"}), wantStatus: http.StatusFound, @@ -366,6 +367,7 @@ func TestAuthorizationEndpoint(t *testing.T) { generateCSRF: happyCSRFGenerator, generatePKCE: happyPKCEGenerator, generateNonce: happyNonceGenerator, + encoder: happyEncoder, method: http.MethodGet, path: modifiedHappyGetRequestPath(map[string]string{"scope": "openid profile email tuna"}), wantStatus: http.StatusFound, @@ -380,6 +382,7 @@ func TestAuthorizationEndpoint(t *testing.T) { generateCSRF: happyCSRFGenerator, generatePKCE: happyPKCEGenerator, generateNonce: happyNonceGenerator, + encoder: happyEncoder, method: http.MethodGet, path: modifiedHappyGetRequestPath(map[string]string{"response_type": ""}), wantStatus: http.StatusFound, @@ -394,6 +397,7 @@ func TestAuthorizationEndpoint(t *testing.T) { generateCSRF: happyCSRFGenerator, generatePKCE: happyPKCEGenerator, generateNonce: happyNonceGenerator, + encoder: happyEncoder, method: http.MethodGet, path: modifiedHappyGetRequestPath(map[string]string{"client_id": ""}), wantStatus: http.StatusUnauthorized, @@ -407,6 +411,7 @@ func TestAuthorizationEndpoint(t *testing.T) { generateCSRF: happyCSRFGenerator, generatePKCE: happyPKCEGenerator, generateNonce: happyNonceGenerator, + encoder: happyEncoder, method: http.MethodGet, path: modifiedHappyGetRequestPath(map[string]string{"code_challenge": ""}), wantStatus: http.StatusFound, @@ -421,6 +426,7 @@ func TestAuthorizationEndpoint(t *testing.T) { generateCSRF: happyCSRFGenerator, generatePKCE: happyPKCEGenerator, generateNonce: happyNonceGenerator, + encoder: happyEncoder, method: http.MethodGet, path: modifiedHappyGetRequestPath(map[string]string{"code_challenge_method": "this-is-not-a-valid-pkce-alg"}), wantStatus: http.StatusFound, @@ -435,6 +441,7 @@ func TestAuthorizationEndpoint(t *testing.T) { generateCSRF: happyCSRFGenerator, generatePKCE: happyPKCEGenerator, generateNonce: happyNonceGenerator, + encoder: happyEncoder, method: http.MethodGet, path: modifiedHappyGetRequestPath(map[string]string{"code_challenge_method": "plain"}), wantStatus: http.StatusFound, @@ -449,6 +456,7 @@ func TestAuthorizationEndpoint(t *testing.T) { generateCSRF: happyCSRFGenerator, generatePKCE: happyPKCEGenerator, generateNonce: happyNonceGenerator, + encoder: happyEncoder, method: http.MethodGet, path: modifiedHappyGetRequestPath(map[string]string{"code_challenge_method": ""}), wantStatus: http.StatusFound, @@ -465,6 +473,7 @@ func TestAuthorizationEndpoint(t *testing.T) { generateCSRF: happyCSRFGenerator, generatePKCE: happyPKCEGenerator, generateNonce: happyNonceGenerator, + encoder: happyEncoder, method: http.MethodGet, path: modifiedHappyGetRequestPath(map[string]string{"prompt": "none login"}), wantStatus: http.StatusFound, @@ -479,6 +488,7 @@ func TestAuthorizationEndpoint(t *testing.T) { generateCSRF: happyCSRFGenerator, generatePKCE: happyPKCEGenerator, generateNonce: happyNonceGenerator, + encoder: happyEncoder, method: http.MethodGet, path: modifiedHappyGetRequestPath(map[string]string{"state": "short"}), wantStatus: http.StatusFound, @@ -486,6 +496,20 @@ func TestAuthorizationEndpoint(t *testing.T) { wantLocationHeader: urlWithQuery(downstreamRedirectURI, fositeInvalidStateErrorQuery), wantBodyString: "", }, + { + name: "error while encoding upstream state param", + issuer: issuer, + idpListGetter: newIDPListGetter(upstreamOIDCIdentityProvider), + generateCSRF: happyCSRFGenerator, + generatePKCE: happyPKCEGenerator, + generateNonce: happyNonceGenerator, + encoder: &errorReturningEncoder{}, + method: http.MethodGet, + path: happyGetRequestPath, + wantStatus: http.StatusInternalServerError, + wantContentType: "text/plain; charset=utf-8", + wantBodyString: "Internal Server Error: error encoding upstream state param\n", + }, { name: "error while generating CSRF token", issuer: issuer, @@ -493,6 +517,7 @@ func TestAuthorizationEndpoint(t *testing.T) { generateCSRF: func() (csrftoken.CSRFToken, error) { return "", fmt.Errorf("some csrf generator error") }, generatePKCE: happyPKCEGenerator, generateNonce: happyNonceGenerator, + encoder: happyEncoder, method: http.MethodGet, path: happyGetRequestPath, wantStatus: http.StatusInternalServerError, @@ -506,6 +531,7 @@ func TestAuthorizationEndpoint(t *testing.T) { generateCSRF: happyCSRFGenerator, generatePKCE: happyPKCEGenerator, generateNonce: func() (nonce.Nonce, error) { return "", fmt.Errorf("some nonce generator error") }, + encoder: happyEncoder, method: http.MethodGet, path: happyGetRequestPath, wantStatus: http.StatusInternalServerError, @@ -519,6 +545,7 @@ func TestAuthorizationEndpoint(t *testing.T) { generateCSRF: happyCSRFGenerator, generatePKCE: func() (pkce.Code, error) { return "", fmt.Errorf("some PKCE generator error") }, generateNonce: happyNonceGenerator, + encoder: happyEncoder, method: http.MethodGet, path: happyGetRequestPath, wantStatus: http.StatusInternalServerError, @@ -586,19 +613,22 @@ func TestAuthorizationEndpoint(t *testing.T) { require.Equal(t, test.wantStatus, rsp.Code) requireEqualContentType(t, rsp.Header().Get("Content-Type"), test.wantContentType) + if test.wantLocationHeader != "" { + actualLocation := rsp.Header().Get("Location") + if test.wantUpstreamStateParamInLocationHeader { + requireEqualDecodedStateParams(t, actualLocation, test.wantLocationHeader, test.encoder) + } + requireEqualURLs(t, actualLocation, test.wantLocationHeader) + } else { + require.Empty(t, rsp.Header().Values("Location")) + } + if test.wantBodyJSON != "" { require.JSONEq(t, test.wantBodyJSON, rsp.Body.String()) } else { require.Equal(t, test.wantBodyString, rsp.Body.String()) } - if test.wantLocationHeader != "" { - actualLocation := rsp.Header().Get("Location") - requireEqualURLs(t, actualLocation, test.wantLocationHeader) - } else { - require.Empty(t, rsp.Header().Values("Location")) - } - if test.wantCSRFCookieHeader != "" { require.Len(t, rsp.Header().Values("Set-Cookie"), 1) actualCookie := rsp.Header().Get("Set-Cookie") @@ -611,7 +641,7 @@ func TestAuthorizationEndpoint(t *testing.T) { for _, test := range tests { test := test t.Run(test.name, func(t *testing.T) { - subject := NewHandler(test.issuer, test.idpListGetter, oauthHelper, test.generateCSRF, test.generatePKCE, test.generateNonce, encoder) + subject := NewHandler(test.issuer, test.idpListGetter, oauthHelper, test.generateCSRF, test.generatePKCE, test.generateNonce, test.encoder) runOneTestCase(t, test, subject) }) } @@ -620,7 +650,7 @@ func TestAuthorizationEndpoint(t *testing.T) { test := tests[0] require.Equal(t, "happy path using GET", test.name) // re-use the happy path test case - subject := NewHandler(test.issuer, test.idpListGetter, oauthHelper, test.generateCSRF, test.generatePKCE, test.generateNonce, encoder) + subject := NewHandler(test.issuer, test.idpListGetter, oauthHelper, test.generateCSRF, test.generatePKCE, test.generateNonce, test.encoder) runOneTestCase(t, test, subject) @@ -640,7 +670,7 @@ func TestAuthorizationEndpoint(t *testing.T) { "access_type": "offline", "scope": "other-scope1 other-scope2", "client_id": "some-other-client-id", - "state": happyUpstreamStateParam, + "state": happyExpectedUpstreamStateParam(downstreamRedirectURI), "nonce": happyNonce, "code_challenge": expectedUpstreamCodeChallenge, "code_challenge_method": "S256", @@ -660,6 +690,26 @@ func TestAuthorizationEndpoint(t *testing.T) { }) } +// Declare a separate type from the production code to ensure that the state param's contents was serialized +// in the format that we expect, with the json keys that we expect, etc. This also ensure that the order of +// the serialized fields is the same, which doesn't really matter expect that we can make simpler equality +// assertions about the redirect URL in this test. +type expectedUpstreamStateParamFormat struct { + P string `json:"p"` + N string `json:"n"` + C string `json:"c"` + K string `json:"k"` + V string `json:"v"` +} + +type errorReturningEncoder struct { + securecookie.Codec +} + +func (*errorReturningEncoder) Encode(_ string, _ interface{}) (string, error) { + return "", fmt.Errorf("some encoding error") +} + func requireEqualContentType(t *testing.T, actual string, expected string) { t.Helper() @@ -676,6 +726,28 @@ func requireEqualContentType(t *testing.T, actual string, expected string) { require.Equal(t, actualContentTypeParams, expectedContentTypeParams) } +func requireEqualDecodedStateParams(t *testing.T, actualURL string, expectedURL string, stateParamDecoder securecookie.Codec) { + t.Helper() + actualLocationURL, err := url.Parse(actualURL) + require.NoError(t, err) + expectedLocationURL, err := url.Parse(expectedURL) + require.NoError(t, err) + + expectedQueryStateParam := expectedLocationURL.Query().Get("state") + require.NotEmpty(t, expectedQueryStateParam) + var expectedDecodedStateParam expectedUpstreamStateParamFormat + err = stateParamDecoder.Decode("s", expectedQueryStateParam, &expectedDecodedStateParam) + require.NoError(t, err) + + actualQueryStateParam := actualLocationURL.Query().Get("state") + require.NotEmpty(t, actualQueryStateParam) + var actualDecodedStateParam expectedUpstreamStateParamFormat + err = stateParamDecoder.Decode("s", actualQueryStateParam, &actualDecodedStateParam) + require.NoError(t, err) + + require.Equal(t, expectedDecodedStateParam, actualDecodedStateParam) +} + func requireEqualURLs(t *testing.T, actualURL string, expectedURL string) { t.Helper() actualLocationURL, err := url.Parse(actualURL) diff --git a/internal/oidc/provider/manager/manager.go b/internal/oidc/provider/manager/manager.go index 77112fca0..1cb37fc86 100644 --- a/internal/oidc/provider/manager/manager.go +++ b/internal/oidc/provider/manager/manager.go @@ -9,13 +9,12 @@ import ( "sync" "github.com/gorilla/securecookie" - "go.pinniped.dev/internal/oidc/csrftoken" - "github.com/ory/fosite" "github.com/ory/fosite/storage" "go.pinniped.dev/internal/oidc" "go.pinniped.dev/internal/oidc/auth" + "go.pinniped.dev/internal/oidc/csrftoken" "go.pinniped.dev/internal/oidc/discovery" "go.pinniped.dev/internal/oidc/jwks" "go.pinniped.dev/internal/oidc/provider" diff --git a/internal/plog/doc.go b/internal/plog/doc.go deleted file mode 100644 index 41cc3071b..000000000 --- a/internal/plog/doc.go +++ /dev/null @@ -1,22 +0,0 @@ -// Copyright 2020 the Pinniped contributors. All Rights Reserved. -// SPDX-License-Identifier: Apache-2.0 - -// Package plog implements a thin layer over klog to help enforce pinniped's logging convention. -// Logs are always structured as a constant message with key and value pairs of related metadata. -// The logging levels in order of increasing verbosity are: -// error, warning, info, debug, trace and all. -// error and warning logs are always emitted (there is no way for the end user to disable them), -// and thus should be used sparingly. Ideally, logs at these levels should be actionable. -// info should be reserved for "nice to know" information. It should be possible to run a production -// pinniped server at the info log level with no performance degradation due to high log volume. -// debug should be used for information targeted at developers and to aid in support cases. Care must -// be taken at this level to not leak any secrets into the log stream. That is, even though debug may -// cause performance issues in production, it must not cause security issues in production. -// trace should be used to log information related to timing (i.e. the time it took a controller to sync). -// Just like debug, trace should not leak secrets into the log stream. trace will likely leak information -// about the current state of the process, but that, along with performance degradation, is expected. -// all is reserved for the most verbose and security sensitive information. At this level, full request -// metadata such as headers and parameters along with the body may be logged. This level is completely -// unfit for production use both from a performance and security standpoint. Using it is generally an -// act of desperation to determine why the system is broken. -package plog diff --git a/internal/plog/plog.go b/internal/plog/plog.go index d3b6efa81..ffcefdb88 100644 --- a/internal/plog/plog.go +++ b/internal/plog/plog.go @@ -1,12 +1,37 @@ // Copyright 2020 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 +// Package plog implements a thin layer over klog to help enforce pinniped's logging convention. +// Logs are always structured as a constant message with key and value pairs of related metadata. +// +// The logging levels in order of increasing verbosity are: +// error, warning, info, debug, trace and all. +// +// error and warning logs are always emitted (there is no way for the end user to disable them), +// and thus should be used sparingly. Ideally, logs at these levels should be actionable. +// +// info should be reserved for "nice to know" information. It should be possible to run a production +// pinniped server at the info log level with no performance degradation due to high log volume. +// debug should be used for information targeted at developers and to aid in support cases. Care must +// be taken at this level to not leak any secrets into the log stream. That is, even though debug may +// cause performance issues in production, it must not cause security issues in production. +// +// trace should be used to log information related to timing (i.e. the time it took a controller to sync). +// Just like debug, trace should not leak secrets into the log stream. trace will likely leak information +// about the current state of the process, but that, along with performance degradation, is expected. +// +// all is reserved for the most verbose and security sensitive information. At this level, full request +// metadata such as headers and parameters along with the body may be logged. This level is completely +// unfit for production use both from a performance and security standpoint. Using it is generally an +// act of desperation to determine why the system is broken. package plog import "k8s.io/klog/v2" +const errorKey = "error" + // Use Error to log an unexpected system error. -func Error(err error, msg string, keysAndValues ...interface{}) { +func Error(msg string, err error, keysAndValues ...interface{}) { klog.ErrorS(err, msg, keysAndValues...) } @@ -19,23 +44,38 @@ func Warning(msg string, keysAndValues ...interface{}) { klog.V(klogLevelWarning).InfoS(msg, keysAndValues...) } +// Use WarningErr to issue a Warning message with an error object as part of the message. +func WarningErr(msg string, err error, keysAndValues ...interface{}) { + Warning(msg, append([]interface{}{errorKey, err}, keysAndValues)...) +} + func Info(msg string, keysAndValues ...interface{}) { klog.V(klogLevelInfo).InfoS(msg, keysAndValues...) } // Use InfoErr to log an expected error, e.g. validation failure of an http parameter. func InfoErr(msg string, err error, keysAndValues ...interface{}) { - klog.V(klogLevelInfo).InfoS(msg, append([]interface{}{"error", err}, keysAndValues)...) + Info(msg, append([]interface{}{errorKey, err}, keysAndValues)...) } func Debug(msg string, keysAndValues ...interface{}) { klog.V(klogLevelDebug).InfoS(msg, keysAndValues...) } +// Use DebugErr to issue a Debug message with an error object as part of the message. +func DebugErr(msg string, err error, keysAndValues ...interface{}) { + Debug(msg, append([]interface{}{errorKey, err}, keysAndValues)...) +} + func Trace(msg string, keysAndValues ...interface{}) { klog.V(klogLevelTrace).InfoS(msg, keysAndValues...) } +// Use TraceErr to issue a Trace message with an error object as part of the message. +func TraceErr(msg string, err error, keysAndValues ...interface{}) { + Trace(msg, append([]interface{}{errorKey, err}, keysAndValues)...) +} + func All(msg string, keysAndValues ...interface{}) { klog.V(klogLevelAll).InfoS(msg, keysAndValues...) } From 4b8c1de6470ba02b8cc74531cd7f3a4b5bee357a Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Wed, 11 Nov 2020 13:13:57 -0800 Subject: [PATCH 19/21] Add unit test to auth_handler_test.go for non-openid authorize requests Signed-off-by: Andrew Keesler --- internal/oidc/auth/auth_handler_test.go | 81 ++++++++++++++++--------- 1 file changed, 54 insertions(+), 27 deletions(-) diff --git a/internal/oidc/auth/auth_handler_test.go b/internal/oidc/auth/auth_handler_test.go index 465579378..b6d71f9e3 100644 --- a/internal/oidc/auth/auth_handler_test.go +++ b/internal/oidc/auth/auth_handler_test.go @@ -169,24 +169,22 @@ func TestAuthorizationEndpoint(t *testing.T) { return urlToReturn } - happyGetRequestQueryMap := func(downstreamRedirectURI string) map[string]string { - return map[string]string{ - "response_type": "code", - "scope": "openid profile email", - "client_id": "pinniped-cli", - "state": "some-state-value", - "nonce": "some-nonce-value", - "code_challenge": "some-challenge", - "code_challenge_method": "S256", - "redirect_uri": downstreamRedirectURI, - } + happyGetRequestQueryMap := map[string]string{ + "response_type": "code", + "scope": "openid profile email", + "client_id": "pinniped-cli", + "state": "some-state-value", + "nonce": "some-nonce-value", + "code_challenge": "some-challenge", + "code_challenge_method": "S256", + "redirect_uri": downstreamRedirectURI, } - happyGetRequestPath := pathWithQuery("/some/path", happyGetRequestQueryMap(downstreamRedirectURI)) + happyGetRequestPath := pathWithQuery("/some/path", happyGetRequestQueryMap) - modifiedHappyGetRequestPath := func(queryOverrides map[string]string) string { + modifiedHappyGetRequestQueryMap := func(queryOverrides map[string]string) map[string]string { copyOfHappyGetRequestQueryMap := map[string]string{} - for k, v := range happyGetRequestQueryMap(downstreamRedirectURI) { + for k, v := range happyGetRequestQueryMap { copyOfHappyGetRequestQueryMap[k] = v } for k, v := range queryOverrides { @@ -197,13 +195,17 @@ func TestAuthorizationEndpoint(t *testing.T) { copyOfHappyGetRequestQueryMap[k] = v } } - return pathWithQuery("/some/path", copyOfHappyGetRequestQueryMap) + return copyOfHappyGetRequestQueryMap } - happyExpectedUpstreamStateParam := func(downstreamRedirectURI string) string { + modifiedHappyGetRequestPath := func(queryOverrides map[string]string) string { + return pathWithQuery("/some/path", modifiedHappyGetRequestQueryMap(queryOverrides)) + } + + expectedUpstreamStateParam := func(queryOverrides map[string]string) string { encoded, err := happyEncoder.Encode("s", expectedUpstreamStateParamFormat{ - P: encodeQuery(happyGetRequestQueryMap(downstreamRedirectURI)), + P: encodeQuery(modifiedHappyGetRequestQueryMap(queryOverrides)), N: happyNonce, C: happyCSRF, K: happyPKCE, @@ -214,13 +216,13 @@ func TestAuthorizationEndpoint(t *testing.T) { return encoded } - happyExpectedRedirectLocation := func(downstreamRedirectURI string) string { + expectedRedirectLocation := func(expectedUpstreamState string) string { return urlWithQuery(upstreamAuthURL.String(), map[string]string{ "response_type": "code", "access_type": "offline", "scope": "scope1 scope2", "client_id": "some-client-id", - "state": happyExpectedUpstreamStateParam(downstreamRedirectURI), + "state": expectedUpstreamState, "nonce": happyNonce, "code_challenge": expectedUpstreamCodeChallenge, "code_challenge_method": "S256", @@ -267,11 +269,11 @@ func TestAuthorizationEndpoint(t *testing.T) { wantStatus: http.StatusFound, wantContentType: "text/html; charset=utf-8", wantBodyString: fmt.Sprintf(`Found.%s`, - html.EscapeString(happyExpectedRedirectLocation(downstreamRedirectURI)), + html.EscapeString(expectedRedirectLocation(expectedUpstreamStateParam(nil))), "\n\n", ), wantCSRFCookieHeader: happyCSRFSetCookieHeaderValue, - wantLocationHeader: happyExpectedRedirectLocation(downstreamRedirectURI), + wantLocationHeader: expectedRedirectLocation(expectedUpstreamStateParam(nil)), wantUpstreamStateParamInLocationHeader: true, }, { @@ -285,12 +287,12 @@ func TestAuthorizationEndpoint(t *testing.T) { method: http.MethodPost, path: "/some/path", contentType: "application/x-www-form-urlencoded", - body: encodeQuery(happyGetRequestQueryMap(downstreamRedirectURI)), + body: encodeQuery(happyGetRequestQueryMap), wantStatus: http.StatusFound, wantContentType: "", wantBodyString: "", wantCSRFCookieHeader: happyCSRFSetCookieHeaderValue, - wantLocationHeader: happyExpectedRedirectLocation(downstreamRedirectURI), + wantLocationHeader: expectedRedirectLocation(expectedUpstreamStateParam(nil)), wantUpstreamStateParamInLocationHeader: true, }, { @@ -308,11 +310,15 @@ func TestAuthorizationEndpoint(t *testing.T) { wantStatus: http.StatusFound, wantContentType: "text/html; charset=utf-8", wantBodyString: fmt.Sprintf(`Found.%s`, - html.EscapeString(happyExpectedRedirectLocation(downstreamRedirectURIWithDifferentPort)), + html.EscapeString(expectedRedirectLocation(expectedUpstreamStateParam(map[string]string{ + "redirect_uri": downstreamRedirectURIWithDifferentPort, // not the same port number that is registered for the client + }))), "\n\n", ), - wantCSRFCookieHeader: happyCSRFSetCookieHeaderValue, - wantLocationHeader: happyExpectedRedirectLocation(downstreamRedirectURIWithDifferentPort), + wantCSRFCookieHeader: happyCSRFSetCookieHeaderValue, + wantLocationHeader: expectedRedirectLocation(expectedUpstreamStateParam(map[string]string{ + "redirect_uri": downstreamRedirectURIWithDifferentPort, // not the same port number that is registered for the client + })), wantUpstreamStateParamInLocationHeader: true, }, { @@ -481,6 +487,27 @@ func TestAuthorizationEndpoint(t *testing.T) { wantLocationHeader: urlWithQuery(downstreamRedirectURI, fositePromptHasNoneAndOtherValueErrorQuery), wantBodyString: "", }, + { + name: "OIDC validations are skipped when the openid scope was not requested", + issuer: issuer, + idpListGetter: newIDPListGetter(upstreamOIDCIdentityProvider), + generateCSRF: happyCSRFGenerator, + generatePKCE: happyPKCEGenerator, + generateNonce: happyNonceGenerator, + encoder: happyEncoder, + method: http.MethodGet, + // The following prompt value is illegal when openid is requested, but note that openid is not requested. + path: modifiedHappyGetRequestPath(map[string]string{"prompt": "none login", "scope": "email"}), + wantStatus: http.StatusFound, + wantContentType: "text/html; charset=utf-8", + wantBodyString: fmt.Sprintf(`Found.%s`, + html.EscapeString(expectedRedirectLocation(expectedUpstreamStateParam(map[string]string{"prompt": "none login", "scope": "email"}))), + "\n\n", + ), + wantCSRFCookieHeader: happyCSRFSetCookieHeaderValue, + wantLocationHeader: expectedRedirectLocation(expectedUpstreamStateParam(map[string]string{"prompt": "none login", "scope": "email"})), + wantUpstreamStateParamInLocationHeader: true, + }, { name: "state does not have enough entropy", issuer: issuer, @@ -670,7 +697,7 @@ func TestAuthorizationEndpoint(t *testing.T) { "access_type": "offline", "scope": "other-scope1 other-scope2", "client_id": "some-other-client-id", - "state": happyExpectedUpstreamStateParam(downstreamRedirectURI), + "state": expectedUpstreamStateParam(nil), "nonce": happyNonce, "code_challenge": expectedUpstreamCodeChallenge, "code_challenge_method": "S256", From db6fc234b78101b7111263f5de7db6b553535daa Mon Sep 17 00:00:00 2001 From: Monis Khan Date: Wed, 11 Nov 2020 14:49:24 -0800 Subject: [PATCH 20/21] Add NullStorage for the authorize endpoint to use We want to run all of the fosite validations in the authorize endpoint, but we don't need to store anything yet because we are storing what we need for later in the upstream state parameter. Signed-off-by: Ryan Richard --- internal/oidc/auth/auth_handler_test.go | 9 +- internal/oidc/nullstorage.go | 101 ++++++++++++++++++++++ internal/oidc/nullstorage_test.go | 36 ++++++++ internal/oidc/provider/manager/manager.go | 17 ++-- 4 files changed, 143 insertions(+), 20 deletions(-) create mode 100644 internal/oidc/nullstorage.go create mode 100644 internal/oidc/nullstorage_test.go diff --git a/internal/oidc/auth/auth_handler_test.go b/internal/oidc/auth/auth_handler_test.go index b6d71f9e3..e7658e83b 100644 --- a/internal/oidc/auth/auth_handler_test.go +++ b/internal/oidc/auth/auth_handler_test.go @@ -14,8 +14,6 @@ import ( "testing" "github.com/gorilla/securecookie" - "github.com/ory/fosite" - "github.com/ory/fosite/storage" "github.com/stretchr/testify/require" "go.pinniped.dev/internal/here" @@ -123,12 +121,7 @@ func TestAuthorizationEndpoint(t *testing.T) { issuer := "https://my-issuer.com/some-path" // Configure fosite the same way that the production code would, except use in-memory storage. - oauthStore := &storage.MemoryStore{ - Clients: map[string]fosite.Client{oidc.PinnipedCLIOIDCClient().ID: oidc.PinnipedCLIOIDCClient()}, - AuthorizeCodes: map[string]storage.StoreAuthorizeCode{}, - PKCES: map[string]fosite.Requester{}, - IDSessions: map[string]fosite.Requester{}, - } + oauthStore := oidc.NullStorage{} hmacSecret := []byte("some secret - must have at least 32 bytes") require.GreaterOrEqual(t, len(hmacSecret), 32, "fosite requires that hmac secrets have at least 32 bytes") oauthHelper := oidc.FositeOauth2Helper(oauthStore, hmacSecret) diff --git a/internal/oidc/nullstorage.go b/internal/oidc/nullstorage.go new file mode 100644 index 000000000..3767f8898 --- /dev/null +++ b/internal/oidc/nullstorage.go @@ -0,0 +1,101 @@ +// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package oidc + +import ( + "context" + "time" + + "github.com/ory/fosite" + + "go.pinniped.dev/internal/constable" +) + +const errNotImplemented = constable.Error("NullStorage does not implement this method. It should not have been called.") + +type NullStorage struct{} + +func (NullStorage) RevokeRefreshToken(_ context.Context, _ string) error { + return errNotImplemented +} + +func (NullStorage) RevokeAccessToken(_ context.Context, _ string) error { + return errNotImplemented +} + +func (NullStorage) CreateRefreshTokenSession(_ context.Context, _ string, _ fosite.Requester) (err error) { + return nil +} + +func (NullStorage) GetRefreshTokenSession(_ context.Context, _ string, _ fosite.Session) (request fosite.Requester, err error) { + return nil, errNotImplemented +} + +func (NullStorage) DeleteRefreshTokenSession(_ context.Context, _ string) (err error) { + return errNotImplemented +} + +func (NullStorage) CreateAccessTokenSession(_ context.Context, _ string, _ fosite.Requester) (err error) { + return nil +} + +func (NullStorage) GetAccessTokenSession(_ context.Context, _ string, _ fosite.Session) (request fosite.Requester, err error) { + return nil, errNotImplemented +} + +func (NullStorage) DeleteAccessTokenSession(_ context.Context, _ string) (err error) { + return errNotImplemented +} + +func (NullStorage) CreateOpenIDConnectSession(_ context.Context, _ string, _ fosite.Requester) error { + return nil +} + +func (NullStorage) GetOpenIDConnectSession(_ context.Context, _ string, _ fosite.Requester) (fosite.Requester, error) { + return nil, errNotImplemented +} + +func (NullStorage) DeleteOpenIDConnectSession(_ context.Context, _ string) error { + return errNotImplemented +} + +func (NullStorage) GetPKCERequestSession(_ context.Context, _ string, _ fosite.Session) (fosite.Requester, error) { + return nil, errNotImplemented +} + +func (NullStorage) CreatePKCERequestSession(_ context.Context, _ string, _ fosite.Requester) error { + return nil +} + +func (NullStorage) DeletePKCERequestSession(_ context.Context, _ string) error { + return errNotImplemented +} + +func (NullStorage) CreateAuthorizeCodeSession(_ context.Context, _ string, _ fosite.Requester) (err error) { + return nil +} + +func (NullStorage) GetAuthorizeCodeSession(_ context.Context, _ string, _ fosite.Session) (request fosite.Requester, err error) { + return nil, errNotImplemented +} + +func (NullStorage) InvalidateAuthorizeCodeSession(_ context.Context, _ string) (err error) { + return errNotImplemented +} + +func (NullStorage) GetClient(_ context.Context, id string) (fosite.Client, error) { + client := PinnipedCLIOIDCClient() + if client.ID == id { + return client, nil + } + return nil, fosite.ErrNotFound +} + +func (NullStorage) ClientAssertionJWTValid(_ context.Context, _ string) error { + return errNotImplemented +} + +func (NullStorage) SetClientAssertionJWT(_ context.Context, _ string, _ time.Time) error { + return errNotImplemented +} diff --git a/internal/oidc/nullstorage_test.go b/internal/oidc/nullstorage_test.go new file mode 100644 index 000000000..8555c031b --- /dev/null +++ b/internal/oidc/nullstorage_test.go @@ -0,0 +1,36 @@ +// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package oidc + +import ( + "context" + "testing" + + "github.com/ory/fosite" + "github.com/stretchr/testify/require" +) + +func TestNullStorage_GetClient(t *testing.T) { + storage := NullStorage{} + + client, err := storage.GetClient(context.Background(), "some-other-client") + require.Equal(t, fosite.ErrNotFound, err) + require.Zero(t, client) + + client, err = storage.GetClient(context.Background(), "pinniped-cli") + require.NoError(t, err) + require.Equal(t, + &fosite.DefaultOpenIDConnectClient{ + DefaultClient: &fosite.DefaultClient{ + ID: "pinniped-cli", + Public: true, + RedirectURIs: []string{"http://127.0.0.1/callback"}, + ResponseTypes: []string{"code"}, + GrantTypes: []string{"authorization_code"}, + Scopes: []string{"openid", "profile", "email"}, + }, + }, + client, + ) +} diff --git a/internal/oidc/provider/manager/manager.go b/internal/oidc/provider/manager/manager.go index 1cb37fc86..6badff09b 100644 --- a/internal/oidc/provider/manager/manager.go +++ b/internal/oidc/provider/manager/manager.go @@ -9,8 +9,6 @@ import ( "sync" "github.com/gorilla/securecookie" - "github.com/ory/fosite" - "github.com/ory/fosite/storage" "go.pinniped.dev/internal/oidc" "go.pinniped.dev/internal/oidc/auth" @@ -70,17 +68,12 @@ func (m *Manager) SetProviders(oidcProviders ...*provider.OIDCProvider) { jwksURL := strings.ToLower(incomingProvider.IssuerHost()) + "/" + incomingProvider.IssuerPath() + oidc.JWKSEndpointPath m.providerHandlers[jwksURL] = jwks.NewHandler(incomingProvider.Issuer(), m.dynamicJWKSProvider) - // Each OIDC provider gets its own storage. - oauthStore := &storage.MemoryStore{ - Clients: map[string]fosite.Client{oidc.PinnipedCLIOIDCClient().ID: oidc.PinnipedCLIOIDCClient()}, - AuthorizeCodes: map[string]storage.StoreAuthorizeCode{}, - PKCES: map[string]fosite.Requester{}, - IDSessions: map[string]fosite.Requester{}, - } - oauthHelper := oidc.FositeOauth2Helper(oauthStore, []byte("some secret - must have at least 32 bytes")) // TODO replace this secret + // Use NullStorage for the authorize endpoint because we do not actually want to store anything until + // the upstream callback endpoint is called later. + oauthHelper := oidc.FositeOauth2Helper(oidc.NullStorage{}, []byte("some secret - must have at least 32 bytes")) // TODO replace this secret - var encoderHashKey = []byte("fake-hash-secret") // TODO fix this - var encoderBlockKey = []byte("16-bytes-aaaaaaa") // TODO fix this + var encoderHashKey = []byte("fake-hash-secret") // TODO replace this secret + var encoderBlockKey = []byte("16-bytes-aaaaaaa") // TODO replace this secret var encoder = securecookie.New(encoderHashKey, encoderBlockKey) encoder.SetSerializer(securecookie.JSONEncoder{}) From 203e040be1fae80a722670af22b7b31abaac0501 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Wed, 11 Nov 2020 15:40:40 -0800 Subject: [PATCH 21/21] Remove an unfinished integration test This commit is meant to be reverted when we are unblocked and ready to start working on this integration test again. Temporarily remove it so we can merge this PR to main. Note: I had tried using t.Skip() in the test, but then that caused lint failures, so decided to just remove it for now. --- test/integration/supervisor_login_test.go | 166 ---------------------- 1 file changed, 166 deletions(-) delete mode 100644 test/integration/supervisor_login_test.go diff --git a/test/integration/supervisor_login_test.go b/test/integration/supervisor_login_test.go deleted file mode 100644 index e857d4002..000000000 --- a/test/integration/supervisor_login_test.go +++ /dev/null @@ -1,166 +0,0 @@ -// Copyright 2020 the Pinniped contributors. All Rights Reserved. -// SPDX-License-Identifier: Apache-2.0 - -package integration - -import ( - "context" - "fmt" - "net/http" - "net/url" - "testing" - "time" - - "github.com/coreos/go-oidc" - "github.com/stretchr/testify/require" - "golang.org/x/oauth2" - - "go.pinniped.dev/internal/oidcclient/nonce" - "go.pinniped.dev/internal/oidcclient/pkce" - "go.pinniped.dev/internal/oidcclient/state" - "go.pinniped.dev/test/library" -) - -func TestSupervisorLogin(t *testing.T) { - env := library.IntegrationEnv(t) - client := library.NewSupervisorClientset(t) - - ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute) - defer cancel() - - // Create downstream OIDC provider (i.e., update supervisor with OIDC provider). - // TODO: these env vars might not be set, perform similar loop as discovery test? - scheme := "http" - addr := env.SupervisorHTTPAddress - caBundle := "" - path := "/some/path" - issuer := fmt.Sprintf("https://%s%s", addr, path) - _, _ = requireCreatingOIDCProviderCausesDiscoveryEndpointsToAppear( - ctx, - t, - scheme, - addr, - caBundle, - issuer, - client, - ) - - // Create HTTP client. - httpClient := newHTTPClient(t, caBundle, nil) - httpClient.CheckRedirect = func(_ *http.Request, _ []*http.Request) error { - // Don't follow any redirects right now, since we simply want to validate that our auth endpoint - // redirects us. - return http.ErrUseLastResponse - } - - // Declare the downstream auth endpoint url we will use. - downstreamAuthURL := makeDownstreamAuthURL(t, scheme, addr, path) - - // Make request to auth endpoint - should fail, since we have no upstreams. - req, err := http.NewRequestWithContext(ctx, http.MethodGet, downstreamAuthURL, nil) - require.NoError(t, err) - rsp, err := httpClient.Do(req) - require.NoError(t, err) - defer rsp.Body.Close() - require.Equal(t, http.StatusUnprocessableEntity, rsp.StatusCode) - - // Create upstream OIDC provider. - // TODO: this is where the new UpstreamOIDCProvider CRD might go. - upstreamIssuer := env.OIDCUpstream.Issuer - upstreamClientID := env.OIDCUpstream.ClientID - upstreamRedirectURI := fmt.Sprintf("http://127.0.0.1:%d/callback", env.OIDCUpstream.LocalhostPort) - - // Make request to authorize endpoint - should pass, since we now have an upstream. - req, err = http.NewRequestWithContext(ctx, http.MethodGet, downstreamAuthURL, nil) - require.NoError(t, err) - rsp, err = httpClient.Do(req) - require.NoError(t, err) - defer rsp.Body.Close() - require.Equal(t, http.StatusFound, rsp.StatusCode) - requireValidRedirectLocation( - ctx, - t, - upstreamIssuer, - upstreamClientID, - upstreamRedirectURI, - rsp.Header.Get("Location"), - ) -} - -func makeDownstreamAuthURL(t *testing.T, scheme, addr, path string) string { - t.Helper() - downstreamOAuth2Config := oauth2.Config{ - // This is the hardcoded public client that the supervisor supports. - ClientID: "pinniped-cli", - Endpoint: oauth2.Endpoint{ - AuthURL: fmt.Sprintf("%s://%s%s/oauth2/authorize", scheme, addr, path), - }, - // This is the hardcoded downstream redirect URI that the supervisor supports. - RedirectURL: "http://127.0.0.1/callback", - Scopes: []string{"openid"}, - } - state, nonce, pkce := generateAuthRequestParams(t) - return downstreamOAuth2Config.AuthCodeURL( - state.String(), - nonce.Param(), - pkce.Challenge(), - pkce.Method(), - ) -} - -func generateAuthRequestParams(t *testing.T) (state.State, nonce.Nonce, pkce.Code) { - t.Helper() - state, err := state.Generate() - require.NoError(t, err) - nonce, err := nonce.Generate() - require.NoError(t, err) - pkce, err := pkce.Generate() - require.NoError(t, err) - return state, nonce, pkce -} - -func requireValidRedirectLocation( - ctx context.Context, - t *testing.T, - issuer, clientID, redirectURI, actualLocation string, -) { - t.Helper() - - // Do OIDC discovery on our test issuer to get auth endpoint. - upstreamProvider, err := oidc.NewProvider(ctx, issuer) - require.NoError(t, err) - - // Parse expected upstream auth URL. - expectedLocationURL, err := url.Parse( - (&oauth2.Config{ - ClientID: clientID, - Endpoint: upstreamProvider.Endpoint(), - RedirectURL: redirectURI, - }).AuthCodeURL(""), - ) - require.NoError(t, err) - - // Parse actual upstream auth URL. - actualLocationURL, err := url.Parse(actualLocation) - require.NoError(t, err) - - // First make some assertions on the query values. Note that we will not be able to know what - // certain query values are since they may be random (e.g., state, pkce, nonce). - expectedLocationQuery := expectedLocationURL.Query() - actualLocationQuery := actualLocationURL.Query() - require.NotEmpty(t, actualLocationQuery.Get("state")) - actualLocationQuery.Del("state") - require.NotEmpty(t, actualLocationQuery.Get("code_challenge")) - actualLocationQuery.Del("code_challenge") - require.NotEmpty(t, actualLocationQuery.Get("code_challenge_method")) - actualLocationQuery.Del("code_challenge_method") - require.NotEmpty(t, actualLocationQuery.Get("nonce")) - actualLocationQuery.Del("nonce") - require.Equal(t, expectedLocationQuery, actualLocationQuery) - - // Zero-out query values, since we made specific assertions about those above, and assert that the - // URL's are equal otherwise. - expectedLocationURL.RawQuery = "" - actualLocationURL.RawQuery = "" - require.Equal(t, expectedLocationURL, actualLocationURL) -}