From 39fd9ba2701477db1f8b1032e1fb6fe8c8f87a15 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Thu, 19 May 2022 16:02:08 -0700 Subject: [PATCH] Small refactors and comments for LDAP/AD UI --- cmd/pinniped/cmd/kubeconfig.go | 4 +- cmd/pinniped/cmd/kubeconfig_test.go | 3 +- internal/oidc/auth/auth_handler.go | 58 ++++++++++++------- internal/oidc/login/get_login_handler.go | 4 +- internal/oidc/login/get_login_handler_test.go | 2 +- internal/oidc/login/login_handler.go | 3 + internal/oidc/login/loginhtml/loginhtml.go | 6 +- .../oidc/login/loginhtml/loginhtml_test.go | 4 +- internal/oidc/oidc.go | 3 + .../provider/formposthtml/formposthtml.go | 6 +- .../formposthtml/formposthtml_test.go | 4 +- internal/oidc/provider/manager/manager.go | 5 +- ...re-supervisor-with-workspace_one_access.md | 4 -- 13 files changed, 64 insertions(+), 42 deletions(-) diff --git a/cmd/pinniped/cmd/kubeconfig.go b/cmd/pinniped/cmd/kubeconfig.go index 74280ec5..e46eebde 100644 --- a/cmd/pinniped/cmd/kubeconfig.go +++ b/cmd/pinniped/cmd/kubeconfig.go @@ -922,7 +922,9 @@ func selectUpstreamIDPFlow(discoveredIDPFlows []idpdiscoveryv1alpha1.IDPFlow, se return discoveredIDPFlows[0], nil default: // The user did not specify a flow, and more than one was found. - log.Info("multiple client flows found, selecting first value as default: "+discoveredIDPFlows[0].String(), "idpName", selectedIDPName, "idpType", selectedIDPType) + log.Info("multiple client flows found, selecting first value as default", + "idpName", selectedIDPName, "idpType", selectedIDPType, + "selectedFlow", discoveredIDPFlows[0].String(), "availableFlows", discoveredIDPFlows) return discoveredIDPFlows[0], nil } } diff --git a/cmd/pinniped/cmd/kubeconfig_test.go b/cmd/pinniped/cmd/kubeconfig_test.go index e5c27797..76a216f0 100644 --- a/cmd/pinniped/cmd/kubeconfig_test.go +++ b/cmd/pinniped/cmd/kubeconfig_test.go @@ -1305,7 +1305,8 @@ func TestGetKubeconfig(t *testing.T) { base64.StdEncoding.EncodeToString([]byte(issuerCABundle))) }, wantLogs: func(_ string, _ string) []string { - return []string{"\"level\"=0 \"msg\"=\"multiple client flows found, selecting first value as default: cli_password\" \"idpName\"=\"some-ldap-idp\" \"idpType\"=\"ldap\""} + return []string{`"level"=0 "msg"="multiple client flows found, selecting first value as default" ` + + `"availableFlows"=["cli_password","flow2"] "idpName"="some-ldap-idp" "idpType"="ldap" "selectedFlow"="cli_password"`} }, }, { diff --git a/internal/oidc/auth/auth_handler.go b/internal/oidc/auth/auth_handler.go index ae502d3a..b4b9fccd 100644 --- a/internal/oidc/auth/auth_handler.go +++ b/internal/oidc/auth/auth_handler.go @@ -159,7 +159,7 @@ func handleAuthRequestForLDAPUpstreamBrowserFlow( upstreamStateEncoder oidc.Encoder, cookieCodec oidc.Codec, ) error { - encodedStateParamValue, _, _, err := handleBrowserAuthRequest( + authRequestState, err := handleBrowserFlowAuthRequest( r, w, oauthHelper, @@ -174,11 +174,12 @@ func handleAuthRequestForLDAPUpstreamBrowserFlow( if err != nil { return err } - if encodedStateParamValue == "" { + if authRequestState == nil { + // There was an error but handleBrowserFlowAuthRequest() already took care of writing the response for it. return nil } - return login.RedirectToLoginPage(r, w, downstreamIssuer, encodedStateParamValue, login.ShowNoError) + return login.RedirectToLoginPage(r, w, downstreamIssuer, authRequestState.encodedStateParam, login.ShowNoError) } func handleAuthRequestForOIDCUpstreamPasswordGrant( @@ -255,7 +256,7 @@ func handleAuthRequestForOIDCUpstreamBrowserFlow( upstreamStateEncoder oidc.Encoder, cookieCodec oidc.Codec, ) error { - encodedStateParamValue, pkceValue, nonceValue, err := handleBrowserAuthRequest( + authRequestState, err := handleBrowserFlowAuthRequest( r, w, oauthHelper, @@ -270,7 +271,8 @@ func handleAuthRequestForOIDCUpstreamBrowserFlow( if err != nil { return err } - if encodedStateParamValue == "" { + if authRequestState == nil { + // There was an error but handleBrowserFlowAuthRequest() already took care of writing the response for it. return nil } @@ -284,9 +286,9 @@ func handleAuthRequestForOIDCUpstreamBrowserFlow( } authCodeOptions := []oauth2.AuthCodeOption{ - nonceValue.Param(), - pkceValue.Challenge(), - pkceValue.Method(), + authRequestState.nonce.Param(), + authRequestState.pkce.Challenge(), + authRequestState.pkce.Method(), } for key, val := range oidcUpstream.GetAdditionalAuthcodeParams() { @@ -295,7 +297,7 @@ func handleAuthRequestForOIDCUpstreamBrowserFlow( http.Redirect(w, r, upstreamOAuthConfig.AuthCodeURL( - encodedStateParamValue, + authRequestState.encodedStateParam, authCodeOptions..., ), http.StatusSeeOther, // match fosite and https://tools.ietf.org/id/draft-ietf-oauth-security-topics-18.html#section-4.11 @@ -387,10 +389,21 @@ func chooseUpstreamIDP(idpLister oidc.UpstreamIdentityProvidersLister) (provider } } -// handleBrowserAuthRequest performs the shared validations and setup between browser based auth requests -// regardless of IDP type-- LDAP, Active Directory and OIDC. +type browserFlowAuthRequestState struct { + encodedStateParam string + pkce pkce.Code + nonce nonce.Nonce +} + +// handleBrowserFlowAuthRequest performs the shared validations and setup between browser based +// auth requests regardless of IDP type-- LDAP, Active Directory and OIDC. // It generates the state param, sets the CSRF cookie, and validates the prompt param. -func handleBrowserAuthRequest( +// It returns an error when it encounters an error without handling it, leaving it to +// the caller to decide how to handle it. +// It returns nil with no error when it encounters an error and also has already handled writing +// the error response to the ResponseWriter, in which case the caller should not also try to +// write the error response. +func handleBrowserFlowAuthRequest( r *http.Request, w http.ResponseWriter, oauthHelper fosite.OAuth2Provider, @@ -401,10 +414,10 @@ func handleBrowserAuthRequest( idpType psession.ProviderType, cookieCodec oidc.Codec, upstreamStateEncoder oidc.Encoder, -) (string, pkce.Code, nonce.Nonce, error) { +) (*browserFlowAuthRequestState, error) { authorizeRequester, created := newAuthorizeRequest(r, w, oauthHelper, false) if !created { - return "", "", "", nil + return nil, nil // already wrote the error response, don't return error } now := time.Now() @@ -420,13 +433,13 @@ func handleBrowserAuthRequest( }) if err != nil { oidc.WriteAuthorizeError(w, oauthHelper, authorizeRequester, err, false) - return "", "", "", nil + return nil, nil // already wrote the error response, don't return error } csrfValue, nonceValue, pkceValue, err := generateValues(generateCSRF, generateNonce, generatePKCE) if err != nil { plog.Error("authorize generate error", err) - return "", "", "", err + return nil, err } csrfFromCookie := readCSRFCookie(r, cookieCodec) if csrfFromCookie != "" { @@ -444,13 +457,13 @@ func handleBrowserAuthRequest( ) if err != nil { plog.Error("authorize upstream state param error", err) - return "", "", "", err + return nil, err } promptParam := r.Form.Get(promptParamName) if promptParam == promptParamNone && oidc.ScopeWasRequested(authorizeRequester, coreosoidc.ScopeOpenID) { oidc.WriteAuthorizeError(w, oauthHelper, authorizeRequester, fosite.ErrLoginRequired, false) - return "", "", "", nil + return nil, nil // already wrote the error response, don't return error } if csrfFromCookie == "" { @@ -458,10 +471,15 @@ func handleBrowserAuthRequest( err = addCSRFSetCookieHeader(w, csrfValue, cookieCodec) if err != nil { plog.Error("error setting CSRF cookie", err) - return "", "", "", err + return nil, err } } - return encodedStateParamValue, pkceValue, nonceValue, nil + + return &browserFlowAuthRequestState{ + encodedStateParam: encodedStateParamValue, + pkce: pkceValue, + nonce: nonceValue, + }, nil } func generateValues( diff --git a/internal/oidc/login/get_login_handler.go b/internal/oidc/login/get_login_handler.go index 3e33c937..d6da85a6 100644 --- a/internal/oidc/login/get_login_handler.go +++ b/internal/oidc/login/get_login_handler.go @@ -15,12 +15,12 @@ const ( incorrectUsernameOrPasswordErrorMessage = "Incorrect username or password." ) -func NewGetHandler() HandlerFunc { +func NewGetHandler(loginPath string) HandlerFunc { return func(w http.ResponseWriter, r *http.Request, encodedState string, decodedState *oidc.UpstreamStateParamData) error { alertMessage, hasAlert := getAlert(r) pageInputs := &loginhtml.PageData{ - PostPath: r.URL.Path, // the path for POST is the same as for GET + PostPath: loginPath, State: encodedState, IDPName: decodedState.UpstreamName, HasAlertError: hasAlert, diff --git a/internal/oidc/login/get_login_handler_test.go b/internal/oidc/login/get_login_handler_test.go index 472148d5..bb85b8f2 100644 --- a/internal/oidc/login/get_login_handler_test.go +++ b/internal/oidc/login/get_login_handler_test.go @@ -96,7 +96,7 @@ func TestGetLogin(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() - handler := NewGetHandler() + handler := NewGetHandler(testPath) target := testPath + "?state=" + tt.encodedState if tt.errParam != "" { target += "&err=" + tt.errParam diff --git a/internal/oidc/login/login_handler.go b/internal/oidc/login/login_handler.go index 06444bc1..1b358f2b 100644 --- a/internal/oidc/login/login_handler.go +++ b/internal/oidc/login/login_handler.go @@ -94,6 +94,9 @@ func wrapSecurityHeaders(handler http.Handler) http.Handler { }) } +// RedirectToLoginPage redirects to the GET /login page of the specified issuer. +// The specified issuer should never end with a "/", which is validated by +// provider.FederationDomainIssuer when the issuer string comes from that type. func RedirectToLoginPage( r *http.Request, w http.ResponseWriter, diff --git a/internal/oidc/login/loginhtml/loginhtml.go b/internal/oidc/login/loginhtml/loginhtml.go index 1493979f..2cd97c57 100644 --- a/internal/oidc/login/loginhtml/loginhtml.go +++ b/internal/oidc/login/loginhtml/loginhtml.go @@ -18,7 +18,7 @@ import ( var ( //go:embed login_form.css rawCSS string - minifiedCSS = mustMinify(minify.CSS(rawCSS)) + minifiedCSS = panicOnError(minify.CSS(rawCSS)) //go:embed login_form.gohtml rawHTMLTemplate string @@ -26,7 +26,7 @@ var ( // Parse the Go templated HTML and inject functions providing the minified inline CSS and JS. var parsedHTMLTemplate = template.Must(template.New("login_form.gohtml").Funcs(template.FuncMap{ - "minifiedCSS": func() template.CSS { return template.CSS(minifiedCSS) }, + "minifiedCSS": func() template.CSS { return template.CSS(CSS()) }, }).Parse(rawHTMLTemplate)) // Generate the CSP header value once since it's effectively constant. @@ -36,7 +36,7 @@ var cspValue = strings.Join([]string{ `frame-ancestors 'none'`, }, "; ") -func mustMinify(s string, err error) string { +func panicOnError(s string, err error) string { if err != nil { panic(err) } diff --git a/internal/oidc/login/loginhtml/loginhtml_test.go b/internal/oidc/login/loginhtml/loginhtml_test.go index a2e91ed1..50d8dc95 100644 --- a/internal/oidc/login/loginhtml/loginhtml_test.go +++ b/internal/oidc/login/loginhtml/loginhtml_test.go @@ -63,6 +63,6 @@ func TestCSS(t *testing.T) { } func TestHelpers(t *testing.T) { - require.Equal(t, "test", mustMinify("test", nil)) - require.PanicsWithError(t, "some error", func() { mustMinify("", fmt.Errorf("some error")) }) + require.Equal(t, "test", panicOnError("test", nil)) + require.PanicsWithError(t, "some error", func() { panicOnError("", fmt.Errorf("some error")) }) } diff --git a/internal/oidc/oidc.go b/internal/oidc/oidc.go index b45e757a..79380df7 100644 --- a/internal/oidc/oidc.go +++ b/internal/oidc/oidc.go @@ -43,6 +43,9 @@ 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. + // + // Version 1 was the original version. + // Version 2 added the UpstreamType field to the UpstreamStateParamData struct. UpstreamStateParamFormatVersion = "2" // The `name` passed to the encoder for encoding the upstream state param value. This name is short diff --git a/internal/oidc/provider/formposthtml/formposthtml.go b/internal/oidc/provider/formposthtml/formposthtml.go index b96f0d5d..d1a26c34 100644 --- a/internal/oidc/provider/formposthtml/formposthtml.go +++ b/internal/oidc/provider/formposthtml/formposthtml.go @@ -18,11 +18,11 @@ import ( var ( //go:embed form_post.css rawCSS string - minifiedCSS = mustMinify(minify.CSS(rawCSS)) + minifiedCSS = panicOnError(minify.CSS(rawCSS)) //go:embed form_post.js rawJS string - minifiedJS = mustMinify(minify.JS(rawJS)) + minifiedJS = panicOnError(minify.JS(rawJS)) //go:embed form_post.gohtml rawHTMLTemplate string @@ -44,7 +44,7 @@ var cspValue = strings.Join([]string{ `frame-ancestors 'none'`, }, "; ") -func mustMinify(s string, err error) string { +func panicOnError(s string, err error) string { if err != nil { panic(err) } diff --git a/internal/oidc/provider/formposthtml/formposthtml_test.go b/internal/oidc/provider/formposthtml/formposthtml_test.go index d5d69c9d..e28714c0 100644 --- a/internal/oidc/provider/formposthtml/formposthtml_test.go +++ b/internal/oidc/provider/formposthtml/formposthtml_test.go @@ -93,6 +93,6 @@ func TestContentSecurityPolicyHashes(t *testing.T) { } func TestHelpers(t *testing.T) { - require.Equal(t, "test", mustMinify("test", nil)) - require.PanicsWithError(t, "some error", func() { mustMinify("", fmt.Errorf("some error")) }) + require.Equal(t, "test", panicOnError("test", nil)) + require.PanicsWithError(t, "some error", func() { panicOnError("", fmt.Errorf("some error")) }) } diff --git a/internal/oidc/provider/manager/manager.go b/internal/oidc/provider/manager/manager.go index ffa33139..2833efa2 100644 --- a/internal/oidc/provider/manager/manager.go +++ b/internal/oidc/provider/manager/manager.go @@ -8,8 +8,6 @@ import ( "strings" "sync" - "go.pinniped.dev/internal/oidc/login" - corev1client "k8s.io/client-go/kubernetes/typed/core/v1" "go.pinniped.dev/internal/oidc" @@ -20,6 +18,7 @@ import ( "go.pinniped.dev/internal/oidc/dynamiccodec" "go.pinniped.dev/internal/oidc/idpdiscovery" "go.pinniped.dev/internal/oidc/jwks" + "go.pinniped.dev/internal/oidc/login" "go.pinniped.dev/internal/oidc/provider" "go.pinniped.dev/internal/oidc/token" "go.pinniped.dev/internal/plog" @@ -139,7 +138,7 @@ func (m *Manager) SetProviders(federationDomains ...*provider.FederationDomainIs m.providerHandlers[(issuerHostWithPath + oidc.PinnipedLoginPath)] = login.NewHandler( upstreamStateEncoder, csrfCookieEncoder, - login.NewGetHandler(), + login.NewGetHandler(incomingProvider.IssuerPath()+oidc.PinnipedLoginPath), login.NewPostHandler(issuer, m.upstreamIDPs, oauthHelperWithKubeStorage), ) diff --git a/site/content/docs/howto/configure-supervisor-with-workspace_one_access.md b/site/content/docs/howto/configure-supervisor-with-workspace_one_access.md index 7d411169..fc6b6ffd 100644 --- a/site/content/docs/howto/configure-supervisor-with-workspace_one_access.md +++ b/site/content/docs/howto/configure-supervisor-with-workspace_one_access.md @@ -118,10 +118,6 @@ stringData: clientSecret: "" ``` -Note that the `metadata.name` of the OIDCIdentityProvider resource may be visible to end users at login prompts -if you choose to enable `allowPasswordGrant`, so choose a name which will be understood by your end users. -For example, if you work at Acme Corp, choose something like `acme-corporate-workspace-one` over `my-idp`. - The following claims are returned by Workspace ONE Access. The `group` scope is required to use the `group_ids` and `group_names` claims. The `email` scope is required to use the `email` claim. The remaining claims are always available.