diff --git a/cmd/pinniped/cmd/login_oidc.go b/cmd/pinniped/cmd/login_oidc.go index 41602714..a3517c31 100644 --- a/cmd/pinniped/cmd/login_oidc.go +++ b/cmd/pinniped/cmd/login_oidc.go @@ -31,7 +31,6 @@ type oidcLoginParams struct { listenPort uint16 scopes []string skipBrowser bool - usePKCE bool // These parameters capture dependencies that we want to mock during testing. generateState func() (state.State, error) @@ -52,7 +51,6 @@ func (o *oidcLoginParams) cmd() *cobra.Command { cmd.Flags().Uint16Var(&o.listenPort, "listen-port", 48095, "TCP port for localhost listener (authorization code flow only).") cmd.Flags().StringSliceVar(&o.scopes, "scopes", []string{"offline_access", "openid", "email", "profile"}, "OIDC scopes to request during login.") cmd.Flags().BoolVar(&o.skipBrowser, "skip-browser", false, "Skip opening the browser (just print the URL).") - cmd.Flags().BoolVar(&o.usePKCE, "use-pkce", true, "Use Proof Key for Code Exchange (RFC 7636) during login.") mustMarkRequired(&cmd, "issuer", "client-id") return &cmd } @@ -77,14 +75,16 @@ func (o *oidcLoginParams) runE(cmd *cobra.Command, args []string) error { return fmt.Errorf("could not generate OIDC state parameter: %w", err) } - var pkceCode pkce.Code - if o.usePKCE { - pkceCode, err = o.generatePKCE() - if err != nil { - return fmt.Errorf("could not generate OIDC PKCE parameter: %w", err) - } - authCodeOptions = append(authCodeOptions, pkceCode.Challenge(), pkceCode.Method()) + // We can always use PKCE (RFC 7636) because the server should always ignore the parameters if it doesn't + // understand them. Per https://tools.ietf.org/html/rfc7636#section-5: + // As the OAuth 2.0 [RFC6749] server responses are unchanged by this specification, client implementations of + // this specification do not need to know if the server has implemented this specification or not and SHOULD + // send the additional parameters as defined in Section 4 to all servers. + pkceCode, err := o.generatePKCE() + if err != nil { + return fmt.Errorf("could not generate OIDC PKCE parameter: %w", err) } + authCodeOptions = append(authCodeOptions, pkceCode.Challenge(), pkceCode.Method()) // If --skip-browser was passed, override the default browser open function with a Printf() call. openURL := o.openURL diff --git a/cmd/pinniped/cmd/login_oidc_test.go b/cmd/pinniped/cmd/login_oidc_test.go index 9258cc30..6d562587 100644 --- a/cmd/pinniped/cmd/login_oidc_test.go +++ b/cmd/pinniped/cmd/login_oidc_test.go @@ -46,7 +46,6 @@ func TestLoginOIDCCommand(t *testing.T) { --listen-port uint16 TCP port for localhost listener (authorization code flow only). (default 48095) --scopes strings OIDC scopes to request during login. (default [offline_access,openid,email,profile]) --skip-browser Skip opening the browser (just print the URL). - --use-pkce Use Proof Key for Code Exchange (RFC 7636) during login. (default true) `), }, { @@ -142,7 +141,6 @@ func TestOIDCLoginRunE(t *testing.T) { params: oidcLoginParams{ issuer: validServer.URL, generateState: func() (state.State, error) { return "test-state", nil }, - usePKCE: true, generatePKCE: func() (pkce.Code, error) { return "", fmt.Errorf("some error generating a PKCE code") }, }, wantError: "could not generate OIDC PKCE parameter: some error generating a PKCE code", @@ -152,19 +150,18 @@ func TestOIDCLoginRunE(t *testing.T) { params: oidcLoginParams{ issuer: validServer.URL, generateState: func() (state.State, error) { return "test-state", nil }, - usePKCE: true, generatePKCE: func() (pkce.Code, error) { return "test-pkce", nil }, openURL: func(_ string) error { return fmt.Errorf("some browser open error") }, }, wantError: "could not open browser (run again with --skip-browser?): some browser open error", }, { - name: "success without PKCE", + name: "success", params: oidcLoginParams{ issuer: validServer.URL, clientID: "test-client-id", generateState: func() (state.State, error) { return "test-state", nil }, - usePKCE: false, + generatePKCE: func() (pkce.Code, error) { return "test-pkce", nil }, listenPort: 12345, skipBrowser: true, }, @@ -172,12 +169,18 @@ func TestOIDCLoginRunE(t *testing.T) { require.Equal(t, validServerURL.Host, actual.Host) require.Equal(t, "/auth", actual.Path) require.Equal(t, "", actual.Fragment) + require.Equal(t, url.Values{ - "access_type": []string{"offline"}, - "client_id": []string{"test-client-id"}, - "redirect_uri": []string{"http://localhost:12345/callback"}, - "response_type": []string{"code"}, - "state": []string{"test-state"}, + "access_type": []string{"offline"}, + "client_id": []string{"test-client-id"}, + "redirect_uri": []string{"http://localhost:12345/callback"}, + "response_type": []string{"code"}, + "state": []string{"test-state"}, + "code_challenge_method": []string{"S256"}, + // 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 + // VVaezYqum7reIhoavCHD1n2d+piN3r/mywoYj7fCR7g + "code_challenge": []string{"VVaezYqum7reIhoavCHD1n2d-piN3r_mywoYj7fCR7g"}, }, actual.Query()) }, wantStderr: "Please log in: \n",