From 33ce79f89dc7b30627cd4adc235dda39c314edba Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Wed, 4 Nov 2020 17:06:47 -0800 Subject: [PATCH] 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 dd89c1e0..302d1d20 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 7f89eaee..8fbcf575 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 2fd61d3d..a03c86d0 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 46a5f071..fbee5db4 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 01122ad7..e2d55f32 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() }) }) })