From ce49d8bd7b1b7bf618c6dcf0ab287de172afd4c3 Mon Sep 17 00:00:00 2001 From: Matt Moyer Date: Tue, 6 Oct 2020 16:10:20 -0500 Subject: [PATCH] Remove the --use-pkce flag and just always use it. Based on the spec, it seems like it's required that OAuth2 servers which do not support PKCE should just ignore the parameters, so this should always work. Signed-off-by: Matt Moyer --- cmd/pinniped/cmd/login_oidc.go | 18 +++++++++--------- cmd/pinniped/cmd/login_oidc_test.go | 23 +++++++++++++---------- 2 files changed, 22 insertions(+), 19 deletions(-) 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",