From 4c2a0b4872619c9072268cc18d23e8a768a74f7c Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Fri, 16 Apr 2021 18:30:31 -0700 Subject: [PATCH] Add new command-line flags to the `login oidc` command - Also some light prefactoring in login.go to make room for LDAP-style login, which is not implemented yet in this commit. TODOs are added. - And fix a test pollution problem in login_oidc_test.go where it was using a real on-disk CLI cache file, so the tests were polluted by the contents of that file and would sometimes cause each other to fail. --- cmd/pinniped/cmd/login_oidc.go | 55 ++++++++++----- cmd/pinniped/cmd/login_oidc_test.go | 78 ++++++++++++++++------ pkg/oidcclient/login.go | 100 +++++++++++++++++++++++++--- 3 files changed, 188 insertions(+), 45 deletions(-) diff --git a/cmd/pinniped/cmd/login_oidc.go b/cmd/pinniped/cmd/login_oidc.go index 34ead8f8..0a1e2fe0 100644 --- a/cmd/pinniped/cmd/login_oidc.go +++ b/cmd/pinniped/cmd/login_oidc.go @@ -50,23 +50,25 @@ func oidcLoginCommandRealDeps() oidcLoginCommandDeps { } type oidcLoginFlags struct { - issuer string - clientID string - listenPort uint16 - scopes []string - skipBrowser bool - sessionCachePath string - caBundlePaths []string - caBundleData []string - debugSessionCache bool - requestAudience string - conciergeEnabled bool - conciergeAuthenticatorType string - conciergeAuthenticatorName string - conciergeEndpoint string - conciergeCABundle string - conciergeAPIGroupSuffix string - credentialCachePath string + issuer string + clientID string + listenPort uint16 + scopes []string + skipBrowser bool + sessionCachePath string + caBundlePaths []string + caBundleData []string + debugSessionCache bool + requestAudience string + conciergeEnabled bool + conciergeAuthenticatorType string + conciergeAuthenticatorName string + conciergeEndpoint string + conciergeCABundle string + conciergeAPIGroupSuffix string + credentialCachePath string + upstreamIdentityProviderName string + upstreamIdentityProviderType string } func oidcLoginCommand(deps oidcLoginCommandDeps) *cobra.Command { @@ -98,6 +100,8 @@ func oidcLoginCommand(deps oidcLoginCommandDeps) *cobra.Command { cmd.Flags().StringVar(&flags.conciergeCABundle, "concierge-ca-bundle-data", "", "CA bundle to use when connecting to the Concierge") cmd.Flags().StringVar(&flags.conciergeAPIGroupSuffix, "concierge-api-group-suffix", groupsuffix.PinnipedDefaultSuffix, "Concierge API group suffix") cmd.Flags().StringVar(&flags.credentialCachePath, "credential-cache", filepath.Join(mustGetConfigDir(), "credentials.yaml"), "Path to cluster-specific credentials cache (\"\" disables the cache)") + cmd.Flags().StringVar(&flags.upstreamIdentityProviderName, "upstream-identity-provider-name", "", "The name of the upstream identity provider used during login with a Supervisor") + cmd.Flags().StringVar(&flags.upstreamIdentityProviderType, "upstream-identity-provider-type", "oidc", "The type of the upstream identity provider used during login with a Supervisor (e.g. 'oidc', 'ldap')") mustMarkHidden(cmd, "debug-session-cache") mustMarkRequired(cmd, "issuer") @@ -137,6 +141,23 @@ func runOIDCLogin(cmd *cobra.Command, deps oidcLoginCommandDeps, flags oidcLogin opts = append(opts, oidcclient.WithRequestAudience(flags.requestAudience)) } + if flags.upstreamIdentityProviderName != "" { + opts = append(opts, oidcclient.WithUpstreamIdentityProvider( + flags.upstreamIdentityProviderName, flags.upstreamIdentityProviderType)) + } + + switch flags.upstreamIdentityProviderType { + case "oidc": + // this is the default, so don't need to do anything + case "ldap": + opts = append(opts, oidcclient.WithLDAPUpstreamIdentityProvider()) + default: + // Surprisingly cobra does not support this kind of flag validation. See https://github.com/spf13/pflag/issues/236 + return fmt.Errorf( + "--upstream-identity-provider-type value not recognized: %s (supported values: oidc, ldap)", + flags.upstreamIdentityProviderType) + } + var concierge *conciergeclient.Client if flags.conciergeEnabled { var err error diff --git a/cmd/pinniped/cmd/login_oidc_test.go b/cmd/pinniped/cmd/login_oidc_test.go index 8472f6af..5f18fa05 100644 --- a/cmd/pinniped/cmd/login_oidc_test.go +++ b/cmd/pinniped/cmd/login_oidc_test.go @@ -56,23 +56,25 @@ func TestLoginOIDCCommand(t *testing.T) { oidc --issuer ISSUER [flags] Flags: - --ca-bundle strings Path to TLS certificate authority bundle (PEM format, optional, can be repeated) - --ca-bundle-data strings Base64 encoded TLS certificate authority bundle (base64 encoded PEM format, optional, can be repeated) - --client-id string OpenID Connect client ID (default "pinniped-cli") - --concierge-api-group-suffix string Concierge API group suffix (default "pinniped.dev") - --concierge-authenticator-name string Concierge authenticator name - --concierge-authenticator-type string Concierge authenticator type (e.g., 'webhook', 'jwt') - --concierge-ca-bundle-data string CA bundle to use when connecting to the Concierge - --concierge-endpoint string API base for the Concierge endpoint - --credential-cache string Path to cluster-specific credentials cache ("" disables the cache) (default "` + cfgDir + `/credentials.yaml") - --enable-concierge Use the Concierge to login - -h, --help help for oidc - --issuer string OpenID Connect issuer URL - --listen-port uint16 TCP port for localhost listener (authorization code flow only) - --request-audience string Request a token with an alternate audience using RFC8693 token exchange - --scopes strings OIDC scopes to request during login (default [offline_access,openid,pinniped:request-audience]) - --session-cache string Path to session cache file (default "` + cfgDir + `/sessions.yaml") - --skip-browser Skip opening the browser (just print the URL) + --ca-bundle strings Path to TLS certificate authority bundle (PEM format, optional, can be repeated) + --ca-bundle-data strings Base64 encoded TLS certificate authority bundle (base64 encoded PEM format, optional, can be repeated) + --client-id string OpenID Connect client ID (default "pinniped-cli") + --concierge-api-group-suffix string Concierge API group suffix (default "pinniped.dev") + --concierge-authenticator-name string Concierge authenticator name + --concierge-authenticator-type string Concierge authenticator type (e.g., 'webhook', 'jwt') + --concierge-ca-bundle-data string CA bundle to use when connecting to the Concierge + --concierge-endpoint string API base for the Concierge endpoint + --credential-cache string Path to cluster-specific credentials cache ("" disables the cache) (default "` + cfgDir + `/credentials.yaml") + --enable-concierge Use the Concierge to login + -h, --help help for oidc + --issuer string OpenID Connect issuer URL + --listen-port uint16 TCP port for localhost listener (authorization code flow only) + --request-audience string Request a token with an alternate audience using RFC8693 token exchange + --scopes strings OIDC scopes to request during login (default [offline_access,openid,pinniped:request-audience]) + --session-cache string Path to session cache file (default "` + cfgDir + `/sessions.yaml") + --skip-browser Skip opening the browser (just print the URL) + --upstream-identity-provider-name string The name of the upstream identity provider used during login with a Supervisor + --upstream-identity-provider-type string The type of the upstream identity provider used during login with a Supervisor (e.g. 'oidc', 'ldap') (default "oidc") `), }, { @@ -134,11 +136,45 @@ func TestLoginOIDCCommand(t *testing.T) { Error: invalid Concierge parameters: invalid API group suffix: a lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*') `), }, + { + name: "invalid upstream type", + args: []string{ + "--issuer", "test-issuer", + "--upstream-identity-provider-type", "invalid", + }, + wantError: true, + wantStderr: here.Doc(` + Error: --upstream-identity-provider-type value not recognized: invalid (supported values: oidc, ldap) + `), + }, + { + name: "oidc upstream type is allowed", + args: []string{ + "--issuer", "test-issuer", + "--client-id", "test-client-id", + "--upstream-identity-provider-type", "oidc", + "--credential-cache", "", // must specify --credential-cache or else the cache file on disk causes test pollution + }, + wantOptionsCount: 3, + wantStdout: `{"kind":"ExecCredential","apiVersion":"client.authentication.k8s.io/v1beta1","spec":{},"status":{"expirationTimestamp":"3020-10-12T13:14:15Z","token":"test-id-token"}}` + "\n", + }, + { + name: "ldap upstream type is allowed", + args: []string{ + "--issuer", "test-issuer", + "--client-id", "test-client-id", + "--upstream-identity-provider-type", "ldap", + "--credential-cache", "", // must specify --credential-cache or else the cache file on disk causes test pollution + }, + wantOptionsCount: 4, + wantStdout: `{"kind":"ExecCredential","apiVersion":"client.authentication.k8s.io/v1beta1","spec":{},"status":{"expirationTimestamp":"3020-10-12T13:14:15Z","token":"test-id-token"}}` + "\n", + }, { name: "login error", args: []string{ "--client-id", "test-client-id", "--issuer", "test-issuer", + "--credential-cache", "", // must specify --credential-cache or else the cache file on disk causes test pollution }, loginErr: fmt.Errorf("some login error"), wantOptionsCount: 3, @@ -156,6 +192,7 @@ func TestLoginOIDCCommand(t *testing.T) { "--concierge-authenticator-type", "jwt", "--concierge-authenticator-name", "test-authenticator", "--concierge-endpoint", "https://127.0.0.1:1234/", + "--credential-cache", "", // must specify --credential-cache or else the cache file on disk causes test pollution }, conciergeErr: fmt.Errorf("some concierge error"), wantOptionsCount: 3, @@ -169,6 +206,7 @@ func TestLoginOIDCCommand(t *testing.T) { args: []string{ "--client-id", "test-client-id", "--issuer", "test-issuer", + "--credential-cache", "", // must specify --credential-cache or else the cache file on disk causes test pollution }, wantOptionsCount: 3, wantStdout: `{"kind":"ExecCredential","apiVersion":"client.authentication.k8s.io/v1beta1","spec":{},"status":{"expirationTimestamp":"3020-10-12T13:14:15Z","token":"test-id-token"}}` + "\n", @@ -190,9 +228,11 @@ func TestLoginOIDCCommand(t *testing.T) { "--concierge-endpoint", "https://127.0.0.1:1234/", "--concierge-ca-bundle-data", base64.StdEncoding.EncodeToString(testCA.Bundle()), "--concierge-api-group-suffix", "some.suffix.com", - "--credential-cache", testutil.TempDir(t) + "/credentials.yaml", + "--credential-cache", testutil.TempDir(t) + "/credentials.yaml", // must specify --credential-cache or else the cache file on disk causes test pollution + "--upstream-identity-provider-name", "some-upstream-name", + "--upstream-identity-provider-type", "ldap", }, - wantOptionsCount: 7, + wantOptionsCount: 9, wantStdout: `{"kind":"ExecCredential","apiVersion":"client.authentication.k8s.io/v1beta1","spec":{},"status":{"token":"exchanged-token"}}` + "\n", }, } diff --git a/pkg/oidcclient/login.go b/pkg/oidcclient/login.go index 53b6efd4..8a0c80d6 100644 --- a/pkg/oidcclient/login.go +++ b/pkg/oidcclient/login.go @@ -1,4 +1,4 @@ -// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 // Package oidcclient implements a CLI OIDC login flow. @@ -54,6 +54,10 @@ type handlerState struct { scopes []string cache SessionCache + upstreamIdentityProviderName string + upstreamIdentityProviderType string + ldapUpstreamIdentityProvider bool + requestedAudience string httpClient *http.Client @@ -167,6 +171,30 @@ func WithRequestAudience(audience string) Option { } } +// WithLDAPUpstreamIdentityProvider causes the login flow to use CLI prompts for username and password and causes the +// call to the Issuer's authorize endpoint to be made directly (no web browser) with the username and password on custom +// HTTP headers. This is only intended to be used when the issuer is a Pinniped Supervisor and the upstream identity +// provider is an LDAP provider. It should never be used with non-Supervisor issuers because it will send the user's +// password as a custom header, which would be ignored but could potentially get logged somewhere by the issuer. +func WithLDAPUpstreamIdentityProvider() Option { + return func(h *handlerState) error { + h.ldapUpstreamIdentityProvider = true + return nil + } +} + +// WithUpstreamIdentityProvider causes the specified name and type to be sent as custom query parameters to the +// issuer's authorize endpoint. This is only intended to be used when the issuer is a Pinniped Supervisor, in which +// case it provides a mechanism to choose among several upstream identity providers. +// Other issuers will ignore these custom query parameters. +func WithUpstreamIdentityProvider(upstreamName, upstreamType string) Option { + return func(h *handlerState) error { + h.upstreamIdentityProviderName = upstreamName + h.upstreamIdentityProviderType = upstreamType + return nil + } +} + // nopCache is a SessionCache that doesn't actually do anything. type nopCache struct{} @@ -281,6 +309,63 @@ func (h *handlerState) baseLogin() (*oidctypes.Token, error) { } } + // Prepare the common options for the authorization URL. We don't have the redirect URL yet though. + authorizeOptions := []oauth2.AuthCodeOption{ + oauth2.AccessTypeOffline, + h.nonce.Param(), + h.pkce.Challenge(), + h.pkce.Method(), + } + if h.upstreamIdentityProviderName != "" { + authorizeOptions = append(authorizeOptions, oauth2.SetAuthURLParam("upstream_name", h.upstreamIdentityProviderName)) + authorizeOptions = append(authorizeOptions, oauth2.SetAuthURLParam("upstream_type", h.upstreamIdentityProviderType)) + } + + // Choose the appropriate authorization and authcode exchange strategy. + var authFunc = h.webBrowserBasedAuth + if h.ldapUpstreamIdentityProvider { + authFunc = h.cliBasedAuth + } + + // Perform the authorize request and authcode exchange to get back OIDC tokens. + token, err := authFunc(&authorizeOptions) + + // If we got tokens, put them in the cache. + if err == nil { + h.cache.PutToken(cacheKey, token) + } + + return token, err +} + +// Make a direct call to the authorize endpoint and parse the authcode from the response. +// Exchange the authcode for tokens. Return the tokens or an error. +func (h *handlerState) cliBasedAuth(authorizeOptions *[]oauth2.AuthCodeOption) (*oidctypes.Token, error) { + // Make a callback URL even though we won't be listening on this port, because providing a redirect URL is + // required for OIDC authorize endpoints, and it must match the allowed redirect URL of the OIDC client + // registered on the server. + h.oauth2Config.RedirectURL = (&url.URL{ + Scheme: "http", + Host: h.listenAddr, + Path: h.callbackPath, + }).String() + + // Now that we have a redirect URL, we can build the authorize URL. + _ = h.oauth2Config.AuthCodeURL(h.state.String(), *authorizeOptions...) + + // TODO prompt for username and password + // TODO request the authorizeURL directly using h.httpClient, with the custom username and password headers + // TODO error if the response is not a 302 + // TODO error if the response Location does not include a code param (in this case it could have an error message query param to show) + // TODO check the response Location state param to see if it matches, similar to how it is done in handleAuthCodeCallback() + // TODO exchange the authcode, similar to how it is done in handleAuthCodeCallback() + // TODO return the token or any error encountered along the way + return nil, nil +} + +// Open a web browser, or ask the user to open a web browser, to visit the authorize endpoint. +// Create a localhost callback listener which exchanges the authcode for tokens. Return the tokens or an error. +func (h *handlerState) webBrowserBasedAuth(authorizeOptions *[]oauth2.AuthCodeOption) (*oidctypes.Token, error) { // Open a TCP listener and update the OAuth2 redirect_uri to match (in case we are using an ephemeral port number). listener, err := net.Listen("tcp", h.listenAddr) if err != nil { @@ -292,18 +377,14 @@ func (h *handlerState) baseLogin() (*oidctypes.Token, error) { Path: h.callbackPath, }).String() + // Now that we have a redirect URL with the listener port, we can build the authorize URL. + authorizeURL := h.oauth2Config.AuthCodeURL(h.state.String(), *authorizeOptions...) + // Start a callback server in a background goroutine. shutdown := h.serve(listener) defer shutdown() // Open the authorize URL in the users browser. - authorizeURL := h.oauth2Config.AuthCodeURL( - h.state.String(), - oauth2.AccessTypeOffline, - h.nonce.Param(), - h.pkce.Challenge(), - h.pkce.Method(), - ) if err := h.openURL(authorizeURL); err != nil { return nil, fmt.Errorf("could not open browser: %w", err) } @@ -316,7 +397,6 @@ func (h *handlerState) baseLogin() (*oidctypes.Token, error) { if callback.err != nil { return nil, fmt.Errorf("error handling callback: %w", callback.err) } - h.cache.PutToken(cacheKey, callback.token) return callback.token, nil } } @@ -447,6 +527,8 @@ func (h *handlerState) handleAuthCodeCallback(w http.ResponseWriter, r *http.Req // Check for error response parameters. if errorParam := params.Get("error"); errorParam != "" { + // TODO This should also show the value of the optional "error_description" param if it exists. + // See https://openid.net/specs/openid-connect-core-1_0.html#AuthError return httperr.Newf(http.StatusBadRequest, "login failed with code %q", errorParam) }