From a13d7ec5a1ee2253401f40885d21530138f9c619 Mon Sep 17 00:00:00 2001 From: Matt Moyer Date: Tue, 6 Oct 2020 15:59:14 -0500 Subject: [PATCH 1/9] Remove temporary --debug-auth-code-exchange flag for OIDC client CLI. Signed-off-by: Matt Moyer --- cmd/pinniped/cmd/login_oidc.go | 34 ++++++---------------------------- 1 file changed, 6 insertions(+), 28 deletions(-) diff --git a/cmd/pinniped/cmd/login_oidc.go b/cmd/pinniped/cmd/login_oidc.go index e9287d43..41602714 100644 --- a/cmd/pinniped/cmd/login_oidc.go +++ b/cmd/pinniped/cmd/login_oidc.go @@ -11,7 +11,6 @@ import ( "github.com/spf13/cobra" "golang.org/x/oauth2" - "go.pinniped.dev/internal/here" "go.pinniped.dev/internal/oidc/pkce" "go.pinniped.dev/internal/oidc/state" ) @@ -27,13 +26,12 @@ func init() { type oidcLoginParams struct { // These parameters capture CLI flags. - issuer string - clientID string - listenPort uint16 - scopes []string - skipBrowser bool - usePKCE bool - debugAuthCode bool + issuer string + clientID string + listenPort uint16 + scopes []string + skipBrowser bool + usePKCE bool // These parameters capture dependencies that we want to mock during testing. generateState func() (state.State, error) @@ -56,11 +54,6 @@ func (o *oidcLoginParams) cmd() *cobra.Command { 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") - - // TODO: temporary - cmd.Flags().BoolVar(&o.debugAuthCode, "debug-auth-code-exchange", true, "Debug the authorization code exchange (temporary).") - _ = cmd.Flags().MarkHidden("debug-auth-code-exchange") - return &cmd } @@ -107,20 +100,5 @@ func (o *oidcLoginParams) runE(cmd *cobra.Command, args []string) error { return fmt.Errorf("could not open browser (run again with --skip-browser?): %w", err) } - // TODO: this temporary so we can complete the auth code exchange manually - - if o.debugAuthCode { - cmd.PrintErr(here.Docf(` - DEBUG INFO: - Token URL: %s - State: %s - PKCE: %s - `, - cfg.Endpoint.TokenURL, - stateParam, - pkceCode.Verifier(), - )) - } - return nil } From ce49d8bd7b1b7bf618c6dcf0ab287de172afd4c3 Mon Sep 17 00:00:00 2001 From: Matt Moyer Date: Tue, 6 Oct 2020 16:10:20 -0500 Subject: [PATCH 2/9] 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", From 67b692b11f4bdf02568481472cb572ec863e3f6e Mon Sep 17 00:00:00 2001 From: Matt Moyer Date: Tue, 6 Oct 2020 17:27:36 -0500 Subject: [PATCH 3/9] Implement the rest of an OIDC client CLI library. Signed-off-by: Matt Moyer --- cmd/pinniped/cmd/login_oidc.go | 146 +++--- cmd/pinniped/cmd/login_oidc_test.go | 210 +++------ go.mod | 2 +- internal/httputil/httperr/httperr.go | 67 +++ internal/httputil/httperr/httperr_test.go | 52 +++ .../httputil/securityheader/securityheader.go | 20 + .../securityheader/securityheader_test.go | 30 ++ internal/mocks/mockkeyset/generate.go | 6 + internal/mocks/mockkeyset/mockkeyset.go | 53 +++ internal/oidc/state/state.go | 37 -- internal/oidcclient/login/login.go | 271 +++++++++++ internal/oidcclient/login/login_test.go | 420 ++++++++++++++++++ internal/oidcclient/nonce/nonce.go | 58 +++ internal/oidcclient/nonce/nonce_test.go | 40 ++ internal/{oidc => oidcclient}/pkce/pkce.go | 9 +- .../{oidc => oidcclient}/pkce/pkce_test.go | 0 internal/oidcclient/state/state.go | 56 +++ .../{oidc => oidcclient}/state/state_test.go | 8 +- 18 files changed, 1199 insertions(+), 286 deletions(-) create mode 100644 internal/httputil/httperr/httperr.go create mode 100644 internal/httputil/httperr/httperr_test.go create mode 100644 internal/httputil/securityheader/securityheader.go create mode 100644 internal/httputil/securityheader/securityheader_test.go create mode 100644 internal/mocks/mockkeyset/generate.go create mode 100644 internal/mocks/mockkeyset/mockkeyset.go delete mode 100644 internal/oidc/state/state.go create mode 100644 internal/oidcclient/login/login.go create mode 100644 internal/oidcclient/login/login_test.go create mode 100644 internal/oidcclient/nonce/nonce.go create mode 100644 internal/oidcclient/nonce/nonce_test.go rename internal/{oidc => oidcclient}/pkce/pkce.go (76%) rename internal/{oidc => oidcclient}/pkce/pkce_test.go (100%) create mode 100644 internal/oidcclient/state/state.go rename internal/{oidc => oidcclient}/state/state_test.go (68%) diff --git a/cmd/pinniped/cmd/login_oidc.go b/cmd/pinniped/cmd/login_oidc.go index a3517c31..0ad9998d 100644 --- a/cmd/pinniped/cmd/login_oidc.go +++ b/cmd/pinniped/cmd/login_oidc.go @@ -4,101 +4,75 @@ package cmd import ( - "fmt" + "encoding/json" - "github.com/coreos/go-oidc" - "github.com/pkg/browser" "github.com/spf13/cobra" - "golang.org/x/oauth2" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + clientauthenticationv1beta1 "k8s.io/client-go/pkg/apis/clientauthentication/v1beta1" - "go.pinniped.dev/internal/oidc/pkce" - "go.pinniped.dev/internal/oidc/state" + "go.pinniped.dev/internal/oidcclient/login" ) //nolint: gochecknoinits func init() { - loginCmd.AddCommand((&oidcLoginParams{ - generateState: state.Generate, - generatePKCE: pkce.Generate, - openURL: browser.OpenURL, - }).cmd()) + loginCmd.AddCommand(oidcLoginCommand(login.Run)) } -type oidcLoginParams struct { - // These parameters capture CLI flags. - issuer string - clientID string - listenPort uint16 - scopes []string - skipBrowser bool - - // These parameters capture dependencies that we want to mock during testing. - generateState func() (state.State, error) - generatePKCE func() (pkce.Code, error) - openURL func(string) error -} - -func (o *oidcLoginParams) cmd() *cobra.Command { - cmd := cobra.Command{ - Args: cobra.NoArgs, - Use: "oidc --issuer ISSUER --client-id CLIENT_ID", - Short: "Login using an OpenID Connect provider", - RunE: o.runE, - SilenceUsage: true, - } - cmd.Flags().StringVar(&o.issuer, "issuer", "", "OpenID Connect issuer URL.") - cmd.Flags().StringVar(&o.clientID, "client-id", "", "OpenID Connect client ID.") - 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).") +func oidcLoginCommand(loginFunc func(issuer string, clientID string, opts ...login.Option) (*login.Token, error)) *cobra.Command { + var ( + cmd = cobra.Command{ + Args: cobra.NoArgs, + Use: "oidc --issuer ISSUER --client-id CLIENT_ID", + Short: "Login using an OpenID Connect provider", + SilenceUsage: true, + } + issuer string + clientID string + listenPort uint16 + scopes []string + skipBrowser bool + ) + cmd.Flags().StringVar(&issuer, "issuer", "", "OpenID Connect issuer URL.") + cmd.Flags().StringVar(&clientID, "client-id", "", "OpenID Connect client ID.") + cmd.Flags().Uint16Var(&listenPort, "listen-port", 0, "TCP port for localhost listener (authorization code flow only).") + cmd.Flags().StringSliceVar(&scopes, "scopes", []string{"offline_access", "openid", "email", "profile"}, "OIDC scopes to request during login.") + cmd.Flags().BoolVar(&skipBrowser, "skip-browser", false, "Skip opening the browser (just print the URL).") mustMarkRequired(&cmd, "issuer", "client-id") + + cmd.RunE = func(cmd *cobra.Command, args []string) error { + opts := []login.Option{ + login.WithContext(cmd.Context()), + login.WithScopes(scopes), + } + + if listenPort != 0 { + opts = append(opts, login.WithListenPort(listenPort)) + } + + // --skip-browser replaces the default "browser open" function with one that prints to stderr. + if skipBrowser { + opts = append(opts, login.WithBrowserOpen(func(url string) error { + cmd.PrintErr("Please log in: ", url, "\n") + return nil + })) + } + + tok, err := loginFunc(issuer, clientID, opts...) + if err != nil { + return err + } + + // Convert the token out to Kubernetes ExecCredential JSON format for output. + return json.NewEncoder(cmd.OutOrStdout()).Encode(&clientauthenticationv1beta1.ExecCredential{ + TypeMeta: metav1.TypeMeta{ + Kind: "ExecCredential", + APIVersion: "client.authentication.k8s.io/v1beta1", + }, + Status: &clientauthenticationv1beta1.ExecCredentialStatus{ + ExpirationTimestamp: &metav1.Time{Time: tok.IDTokenExpiry}, + Token: tok.IDToken, + }, + }) + } return &cmd } - -func (o *oidcLoginParams) runE(cmd *cobra.Command, args []string) error { - metadata, err := oidc.NewProvider(cmd.Context(), o.issuer) - if err != nil { - return fmt.Errorf("could not perform OIDC discovery for %q: %w", o.issuer, err) - } - - cfg := oauth2.Config{ - ClientID: o.clientID, - Endpoint: metadata.Endpoint(), - RedirectURL: fmt.Sprintf("http://localhost:%d/callback", o.listenPort), - Scopes: o.scopes, - } - - authCodeOptions := []oauth2.AuthCodeOption{oauth2.AccessTypeOffline} - - stateParam, err := o.generateState() - if err != nil { - return fmt.Errorf("could not generate OIDC state parameter: %w", err) - } - - // 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 - if o.skipBrowser { - openURL = func(s string) error { - cmd.PrintErr("Please log in: ", s, "\n") - return nil - } - } - - authorizeURL := cfg.AuthCodeURL(stateParam.String(), authCodeOptions...) - if err := openURL(authorizeURL); err != nil { - return fmt.Errorf("could not open browser (run again with --skip-browser?): %w", err) - } - - return nil -} diff --git a/cmd/pinniped/cmd/login_oidc_test.go b/cmd/pinniped/cmd/login_oidc_test.go index 6d562587..b2bd9348 100644 --- a/cmd/pinniped/cmd/login_oidc_test.go +++ b/cmd/pinniped/cmd/login_oidc_test.go @@ -5,30 +5,29 @@ package cmd import ( "bytes" - "fmt" - "net/http" - "net/http/httptest" - "net/url" - "regexp" - "strings" "testing" + "time" - "github.com/spf13/cobra" "github.com/stretchr/testify/require" "go.pinniped.dev/internal/here" - "go.pinniped.dev/internal/oidc/pkce" - "go.pinniped.dev/internal/oidc/state" + "go.pinniped.dev/internal/oidcclient/login" ) func TestLoginOIDCCommand(t *testing.T) { t.Parallel() + + time1 := time.Date(3020, 10, 12, 13, 14, 15, 16, time.UTC) + tests := []struct { - name string - args []string - wantError bool - wantStdout string - wantStderr string + name string + args []string + wantError bool + wantStdout string + wantStderr string + wantIssuer string + wantClientID string + wantOptionsCount int }{ { name: "help flag passed", @@ -43,7 +42,7 @@ func TestLoginOIDCCommand(t *testing.T) { --client-id string OpenID Connect client ID. -h, --help help for oidc --issuer string OpenID Connect issuer URL. - --listen-port uint16 TCP port for localhost listener (authorization code flow only). (default 48095) + --listen-port uint16 TCP port for localhost listener (authorization code flow only). --scopes strings OIDC scopes to request during login. (default [offline_access,openid,email,profile]) --skip-browser Skip opening the browser (just print the URL). `), @@ -56,12 +55,46 @@ func TestLoginOIDCCommand(t *testing.T) { Error: required flag(s) "client-id", "issuer" not set `), }, + { + name: "success with minimal options", + args: []string{ + "--client-id", "test-client-id", + "--issuer", "test-issuer", + }, + wantIssuer: "test-issuer", + wantClientID: "test-client-id", + wantOptionsCount: 2, + wantStdout: `{"kind":"ExecCredential","apiVersion":"client.authentication.k8s.io/v1beta1","spec":{},"status":{"expirationTimestamp":"3020-10-12T13:14:15Z","token":"test-id-token"}}` + "\n", + }, + { + name: "success with all options", + args: []string{ + "--client-id", "test-client-id", + "--issuer", "test-issuer", + "--skip-browser", + "--listen-port", "1234", + }, + wantIssuer: "test-issuer", + wantClientID: "test-client-id", + wantOptionsCount: 4, + wantStdout: `{"kind":"ExecCredential","apiVersion":"client.authentication.k8s.io/v1beta1","spec":{},"status":{"expirationTimestamp":"3020-10-12T13:14:15Z","token":"test-id-token"}}` + "\n", + }, } for _, tt := range tests { tt := tt t.Run(tt.name, func(t *testing.T) { t.Parallel() - cmd := (&oidcLoginParams{}).cmd() + var ( + gotIssuer string + gotClientID string + gotOptions []login.Option + ) + cmd := oidcLoginCommand(func(issuer string, clientID string, opts ...login.Option) (*login.Token, error) { + gotIssuer = issuer + gotClientID = clientID + gotOptions = opts + return &login.Token{IDToken: "test-id-token", IDTokenExpiry: time1}, nil + }) require.NotNil(t, cmd) var stdout, stderr bytes.Buffer @@ -76,148 +109,9 @@ func TestLoginOIDCCommand(t *testing.T) { } require.Equal(t, tt.wantStdout, stdout.String(), "unexpected stdout") require.Equal(t, tt.wantStderr, stderr.String(), "unexpected stderr") - }) - } -} - -func TestOIDCLoginRunE(t *testing.T) { - t.Parallel() - - // Start a server that returns 500 errors. - brokenServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - http.Error(w, http.StatusText(http.StatusInternalServerError), http.StatusInternalServerError) - })) - t.Cleanup(brokenServer.Close) - - // Start a server that returns successfully. - var validResponse string - validServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - _, _ = w.Write([]byte(validResponse)) - })) - t.Cleanup(validServer.Close) - validResponse = strings.ReplaceAll(here.Docf(` - { - "issuer": "${ISSUER}", - "authorization_endpoint": "${ISSUER}/auth", - "token_endpoint": "${ISSUER}/token", - "jwks_uri": "${ISSUER}/keys", - "userinfo_endpoint": "${ISSUER}/userinfo", - "grant_types_supported": ["authorization_code","refresh_token"], - "response_types_supported": ["code"], - "id_token_signing_alg_values_supported": ["RS256"], - "scopes_supported": ["openid","email","groups","profile","offline_access"], - "token_endpoint_auth_methods_supported": ["client_secret_basic"], - "claims_supported": ["aud","email","email_verified","exp","iat","iss","locale","name","sub"] - } - `), "${ISSUER}", validServer.URL) - validServerURL, err := url.Parse(validServer.URL) - require.NoError(t, err) - - tests := []struct { - name string - params oidcLoginParams - wantError string - wantStdout string - wantStderr string - wantStderrAuthURL func(*testing.T, *url.URL) - }{ - { - name: "broken discovery", - params: oidcLoginParams{ - issuer: brokenServer.URL, - }, - wantError: fmt.Sprintf("could not perform OIDC discovery for %q: 500 Internal Server Error: Internal Server Error\n", brokenServer.URL), - }, - { - name: "broken state generation", - params: oidcLoginParams{ - issuer: validServer.URL, - generateState: func() (state.State, error) { return "", fmt.Errorf("some error generating a state value") }, - }, - wantError: "could not generate OIDC state parameter: some error generating a state value", - }, - { - name: "broken PKCE generation", - params: oidcLoginParams{ - issuer: validServer.URL, - generateState: func() (state.State, error) { return "test-state", nil }, - 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", - }, - { - name: "broken browser open", - params: oidcLoginParams{ - issuer: validServer.URL, - generateState: func() (state.State, error) { return "test-state", nil }, - 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", - params: oidcLoginParams{ - issuer: validServer.URL, - clientID: "test-client-id", - generateState: func() (state.State, error) { return "test-state", nil }, - generatePKCE: func() (pkce.Code, error) { return "test-pkce", nil }, - listenPort: 12345, - skipBrowser: true, - }, - wantStderrAuthURL: func(t *testing.T, actual *url.URL) { - 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"}, - "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", - }, - } - for _, tt := range tests { - tt := tt - t.Run(tt.name, func(t *testing.T) { - var stdout, stderr bytes.Buffer - cmd := cobra.Command{RunE: tt.params.runE, SilenceUsage: true, SilenceErrors: true} - cmd.SetOut(&stdout) - cmd.SetErr(&stderr) - err := cmd.Execute() - if tt.wantError != "" { - require.EqualError(t, err, tt.wantError) - } else { - require.NoError(t, err) - } - - if tt.wantStderrAuthURL != nil { - var urls []string - redacted := regexp.MustCompile(`http://\S+`).ReplaceAllStringFunc(stderr.String(), func(url string) string { - urls = append(urls, url) - return "" - }) - require.Lenf(t, urls, 1, "expected to find authorization URL in stderr:\n%s", stderr.String()) - authURL, err := url.Parse(urls[0]) - require.NoError(t, err, "invalid authorization URL") - tt.wantStderrAuthURL(t, authURL) - - // Replace the stderr buffer with the redacted version. - stderr.Reset() - stderr.WriteString(redacted) - } - - require.Equal(t, tt.wantStdout, stdout.String(), "unexpected stdout") - require.Equal(t, tt.wantStderr, stderr.String(), "unexpected stderr") + require.Equal(t, tt.wantIssuer, gotIssuer, "unexpected issuer") + require.Equal(t, tt.wantClientID, gotClientID, "unexpected client ID") + require.Len(t, gotOptions, tt.wantOptionsCount) }) } } diff --git a/go.mod b/go.mod index a3f63b6b..f2c36c58 100644 --- a/go.mod +++ b/go.mod @@ -13,7 +13,6 @@ require ( github.com/golangci/golangci-lint v1.31.0 github.com/google/go-cmp v0.5.2 github.com/pkg/browser v0.0.0-20180916011732-0a3d74bf9ce4 - github.com/pkg/errors v0.9.1 github.com/sclevine/spec v1.4.0 github.com/spf13/cobra v1.0.0 github.com/spf13/pflag v1.0.5 @@ -22,6 +21,7 @@ require ( go.pinniped.dev/generated/1.19/client v0.0.0-00010101000000-000000000000 golang.org/x/crypto v0.0.0-20200622213623-75b288015ac9 golang.org/x/oauth2 v0.0.0-20191202225959-858c2ad4c8b6 + gopkg.in/square/go-jose.v2 v2.2.2 k8s.io/api v0.19.2 k8s.io/apimachinery v0.19.2 k8s.io/apiserver v0.19.2 diff --git a/internal/httputil/httperr/httperr.go b/internal/httputil/httperr/httperr.go new file mode 100644 index 00000000..1fb21cb7 --- /dev/null +++ b/internal/httputil/httperr/httperr.go @@ -0,0 +1,67 @@ +// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +// Package httperr contains some helpers for nicer error handling in http.Handler implementations. +package httperr + +import ( + "fmt" + "net/http" +) + +// Responder represents an error that can emit a useful HTTP error response to an http.ResponseWriter. +type Responder interface { + error + Respond(http.ResponseWriter) +} + +// New returns a Responder that emits the given HTTP status code and message. +func New(code int, msg string) error { + return httpErr{code: code, msg: msg} +} + +// Newf returns a Responder that emits the given HTTP status code and fmt.Sprintf formatted message. +func Newf(code int, format string, args ...interface{}) error { + return httpErr{code: code, msg: fmt.Sprintf(format, args...)} +} + +// Wrap returns a Responder that emits the given HTTP status code and message, and also wraps an internal error. +func Wrap(code int, msg string, cause error) error { + return httpErr{code: code, msg: msg, cause: cause} +} + +type httpErr struct { + code int + msg string + cause error +} + +func (e httpErr) Error() string { + if e.cause != nil { + return fmt.Sprintf("%s: %v", e.msg, e.cause) + } + return e.msg +} + +func (e httpErr) Respond(w http.ResponseWriter) { + // http.Error is important here because it prevents content sniffing by forcing text/plain. + http.Error(w, http.StatusText(e.code)+": "+e.msg, e.code) +} + +func (e httpErr) Unwrap() error { + return e.cause +} + +// HandlerFunc is like http.HandlerFunc, but with a function signature that allows easier error handling. +type HandlerFunc func(http.ResponseWriter, *http.Request) error + +func (f HandlerFunc) ServeHTTP(w http.ResponseWriter, r *http.Request) { + switch err := f(w, r).(type) { + case nil: + return + case Responder: + err.Respond(w) + default: + http.Error(w, http.StatusText(http.StatusInternalServerError), http.StatusInternalServerError) + } +} diff --git a/internal/httputil/httperr/httperr_test.go b/internal/httputil/httperr/httperr_test.go new file mode 100644 index 00000000..29d7e7f9 --- /dev/null +++ b/internal/httputil/httperr/httperr_test.go @@ -0,0 +1,52 @@ +// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package httperr + +import ( + "errors" + "fmt" + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/require" +) + +func TestHTTPErrs(t *testing.T) { + t.Run("new", func(t *testing.T) { + err := New(http.StatusBadRequest, "bad request error") + require.EqualError(t, err, "bad request error") + }) + + t.Run("newf", func(t *testing.T) { + err := Newf(http.StatusMethodNotAllowed, "expected method %s", "POST") + require.EqualError(t, err, "expected method POST") + }) + + t.Run("wrap", func(t *testing.T) { + wrappedErr := fmt.Errorf("some internal error") + err := Wrap(http.StatusInternalServerError, "unexpected error", wrappedErr) + require.EqualError(t, err, "unexpected error: some internal error") + require.True(t, errors.Is(err, wrappedErr), "expected error to be wrapped") + }) + + t.Run("respond", func(t *testing.T) { + err := Wrap(http.StatusForbidden, "boring public bits", fmt.Errorf("some secret internal bits")) + require.Implements(t, (*Responder)(nil), err) + rec := httptest.NewRecorder() + err.(Responder).Respond(rec) + require.Equal(t, http.StatusForbidden, rec.Code) + require.Equal(t, "Forbidden: boring public bits\n", rec.Body.String()) + require.Equal(t, http.Header{ + "Content-Type": []string{"text/plain; charset=utf-8"}, + "X-Content-Type-Options": []string{"nosniff"}, + }, rec.Header()) + }) +} + +func TestHandlerFunc(t *testing.T) { + t.Run("success", func(t *testing.T) { + + }) +} diff --git a/internal/httputil/securityheader/securityheader.go b/internal/httputil/securityheader/securityheader.go new file mode 100644 index 00000000..47fd8d26 --- /dev/null +++ b/internal/httputil/securityheader/securityheader.go @@ -0,0 +1,20 @@ +// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +// Package securityheader implements an HTTP middleware for setting security-related response headers. +package securityheader + +import "net/http" + +// Wrap the provided http.Handler so it sets appropriate security-related response headers. +func Wrap(wrapped http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + h := w.Header() + h.Set("Content-Security-Policy", "default-src 'none'; frame-ancestors 'none'") + h.Set("X-Frame-Options", "DENY") + h.Set("X-XSS-Protection", "1; mode=block") + h.Set("X-Content-Type-Options", "nosniff") + h.Set("Referrer-Policy", "no-referrer") + wrapped.ServeHTTP(w, r) + }) +} diff --git a/internal/httputil/securityheader/securityheader_test.go b/internal/httputil/securityheader/securityheader_test.go new file mode 100644 index 00000000..e5527cb6 --- /dev/null +++ b/internal/httputil/securityheader/securityheader_test.go @@ -0,0 +1,30 @@ +// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package securityheader + +import ( + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/require" +) + +func TestWrap(t *testing.T) { + handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write([]byte("hello world")) + }) + rec := httptest.NewRecorder() + Wrap(handler).ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/", nil)) + require.Equal(t, http.StatusOK, rec.Code) + require.Equal(t, "hello world", rec.Body.String()) + require.EqualValues(t, http.Header{ + "Content-Security-Policy": []string{"default-src 'none'; frame-ancestors 'none'"}, + "Content-Type": []string{"text/plain; charset=utf-8"}, + "Referrer-Policy": []string{"no-referrer"}, + "X-Content-Type-Options": []string{"nosniff"}, + "X-Frame-Options": []string{"DENY"}, + "X-Xss-Protection": []string{"1; mode=block"}, + }, rec.Header()) +} diff --git a/internal/mocks/mockkeyset/generate.go b/internal/mocks/mockkeyset/generate.go new file mode 100644 index 00000000..a04777fc --- /dev/null +++ b/internal/mocks/mockkeyset/generate.go @@ -0,0 +1,6 @@ +// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package mockkeyset + +//go:generate go run -v github.com/golang/mock/mockgen -destination=mockkeyset.go -package=mockkeyset -copyright_file=../../../hack/header.txt github.com/coreos/go-oidc KeySet diff --git a/internal/mocks/mockkeyset/mockkeyset.go b/internal/mocks/mockkeyset/mockkeyset.go new file mode 100644 index 00000000..ff1050ff --- /dev/null +++ b/internal/mocks/mockkeyset/mockkeyset.go @@ -0,0 +1,53 @@ +// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 +// + +// Code generated by MockGen. DO NOT EDIT. +// Source: github.com/coreos/go-oidc (interfaces: KeySet) + +// Package mockkeyset is a generated GoMock package. +package mockkeyset + +import ( + context "context" + gomock "github.com/golang/mock/gomock" + reflect "reflect" +) + +// MockKeySet is a mock of KeySet interface +type MockKeySet struct { + ctrl *gomock.Controller + recorder *MockKeySetMockRecorder +} + +// MockKeySetMockRecorder is the mock recorder for MockKeySet +type MockKeySetMockRecorder struct { + mock *MockKeySet +} + +// NewMockKeySet creates a new mock instance +func NewMockKeySet(ctrl *gomock.Controller) *MockKeySet { + mock := &MockKeySet{ctrl: ctrl} + mock.recorder = &MockKeySetMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use +func (m *MockKeySet) EXPECT() *MockKeySetMockRecorder { + return m.recorder +} + +// VerifySignature mocks base method +func (m *MockKeySet) VerifySignature(arg0 context.Context, arg1 string) ([]byte, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "VerifySignature", arg0, arg1) + ret0, _ := ret[0].([]byte) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// VerifySignature indicates an expected call of VerifySignature +func (mr *MockKeySetMockRecorder) VerifySignature(arg0, arg1 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "VerifySignature", reflect.TypeOf((*MockKeySet)(nil).VerifySignature), arg0, arg1) +} diff --git a/internal/oidc/state/state.go b/internal/oidc/state/state.go deleted file mode 100644 index 7d70e51b..00000000 --- a/internal/oidc/state/state.go +++ /dev/null @@ -1,37 +0,0 @@ -// Copyright 2020 the Pinniped contributors. All Rights Reserved. -// SPDX-License-Identifier: Apache-2.0 - -package state - -import ( - "crypto/rand" - "crypto/subtle" - "encoding/hex" - "io" - - "github.com/pkg/errors" -) - -// Generate generates a new random state parameter of an appropriate size. -func Generate() (State, error) { return generate(rand.Reader) } - -func generate(rand io.Reader) (State, error) { - var buf [16]byte - if _, err := io.ReadFull(rand, buf[:]); err != nil { - return "", errors.WithMessage(err, "could not generate random state") - } - return State(hex.EncodeToString(buf[:])), nil -} - -// State implements some utilities for working with OAuth2 state parameters. -type State string - -// String returns the string encoding of this state value. -func (s *State) String() string { - return string(*s) -} - -// Validate the returned state (from a callback parameter). Returns true iff the state is valid. -func (s *State) Valid(returnedState string) bool { - return subtle.ConstantTimeCompare([]byte(returnedState), []byte(*s)) == 1 -} diff --git a/internal/oidcclient/login/login.go b/internal/oidcclient/login/login.go new file mode 100644 index 00000000..9f7df3da --- /dev/null +++ b/internal/oidcclient/login/login.go @@ -0,0 +1,271 @@ +// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +// Package login implements a CLI OIDC login flow. +package login + +import ( + "context" + "fmt" + "net" + "net/http" + "net/url" + "time" + + "github.com/coreos/go-oidc" + "github.com/pkg/browser" + "golang.org/x/oauth2" + + "go.pinniped.dev/internal/httputil/httperr" + "go.pinniped.dev/internal/httputil/securityheader" + "go.pinniped.dev/internal/oidcclient/nonce" + "go.pinniped.dev/internal/oidcclient/pkce" + "go.pinniped.dev/internal/oidcclient/state" +) + +type handlerState struct { + // Basic parameters. + ctx context.Context + issuer string + clientID string + scopes []string + + // Parameters of the localhost listener. + listenAddr string + callbackPath string + + // Generated parameters of a login flow. + idTokenVerifier *oidc.IDTokenVerifier + oauth2Config *oauth2.Config + state state.State + nonce nonce.Nonce + pkce pkce.Code + + // External calls for things. + generateState func() (state.State, error) + generatePKCE func() (pkce.Code, error) + generateNonce func() (nonce.Nonce, error) + openURL func(string) error + + callbacks chan callbackResult +} + +type callbackResult struct { + token *Token + err error +} + +type Token struct { + *oauth2.Token + IDToken string `json:"id_token"` + IDTokenExpiry time.Time `json:"id_token_expiry"` +} + +// Option is an optional configuration for Run(). +type Option func(*handlerState) error + +// WithContext specifies a specific context.Context under which to perform the login. If this option is not specified, +// login happens under context.Background(). +func WithContext(ctx context.Context) Option { + return func(h *handlerState) error { + h.ctx = ctx + return nil + } +} + +// WithListenPort specifies a TCP listen port on localhost, which will be used for the redirect_uri and to handle the +// authorization code callback. By default, a random high port will be chosen which requires the authorization server +// to support wildcard port numbers as described by https://tools.ietf.org/html/rfc8252: +// The authorization server MUST allow any port to be specified at the +// time of the request for loopback IP redirect URIs, to accommodate +// clients that obtain an available ephemeral port from the operating +// system at the time of the request. +func WithListenPort(port uint16) Option { + return func(h *handlerState) error { + h.listenAddr = fmt.Sprintf("localhost:%d", port) + return nil + } +} + +// WithScopes sets the OAuth2 scopes to request during login. If not specified, it defaults to +// "offline_access openid email profile". +func WithScopes(scopes []string) Option { + return func(h *handlerState) error { + h.scopes = scopes + return nil + } +} + +// WithBrowserOpen overrides the default "open browser" functionality with a custom callback. If not specified, +// an implementation using https://github.com/pkg/browser will be used by default. +func WithBrowserOpen(openURL func(url string) error) Option { + return func(h *handlerState) error { + h.openURL = openURL + return nil + } +} + +// Run an OAuth2/OIDC authorization code login using a localhost listener. +func Run(issuer string, clientID string, opts ...Option) (*Token, error) { + h := handlerState{ + issuer: issuer, + clientID: clientID, + listenAddr: "localhost:0", + scopes: []string{"offline_access", "openid", "email", "profile"}, + callbackPath: "/callback", + ctx: context.Background(), + callbacks: make(chan callbackResult), + + // Default implementations of external dependencies (to be mocked in tests). + generateState: state.Generate, + generateNonce: nonce.Generate, + generatePKCE: pkce.Generate, + openURL: browser.OpenURL, + } + for _, opt := range opts { + if err := opt(&h); err != nil { + return nil, err + } + } + + // Always set a long, but non-infinite timeout for this operation. + ctx, cancel := context.WithTimeout(h.ctx, 10*time.Minute) + defer cancel() + h.ctx = ctx + + // Initialize login parameters. + var err error + h.state, err = h.generateState() + if err != nil { + return nil, err + } + h.nonce, err = h.generateNonce() + if err != nil { + return nil, err + } + h.pkce, err = h.generatePKCE() + if err != nil { + return nil, err + } + + // Perform OIDC discovery. + provider, err := oidc.NewProvider(h.ctx, h.issuer) + if err != nil { + return nil, fmt.Errorf("could not perform OIDC discovery for %q: %w", h.issuer, err) + } + h.idTokenVerifier = provider.Verifier(&oidc.Config{ClientID: h.clientID}) + + // Open a TCP listener. + listener, err := net.Listen("tcp", h.listenAddr) + if err != nil { + return nil, fmt.Errorf("could not open callback listener: %w", err) + } + + // Build an OAuth2 configuration based on the OIDC discovery data and our callback endpoint. + h.oauth2Config = &oauth2.Config{ + ClientID: h.clientID, + Endpoint: provider.Endpoint(), + RedirectURL: (&url.URL{ + Scheme: "http", + Host: listener.Addr().String(), + Path: h.callbackPath, + }).String(), + Scopes: h.scopes, + } + + // Start a callback server in a background goroutine. + mux := http.NewServeMux() + mux.Handle(h.callbackPath, httperr.HandlerFunc(h.handleAuthCodeCallback)) + srv := http.Server{ + Handler: securityheader.Wrap(mux), + BaseContext: func(_ net.Listener) context.Context { return h.ctx }, + } + go func() { _ = srv.Serve(listener) }() + defer func() { + // Gracefully shut down the server, allowing up to 5 seconds for + // clients to receive any in-flight responses. + shutdownCtx, cancel := context.WithTimeout(h.ctx, 1*time.Second) + _ = srv.Shutdown(shutdownCtx) + cancel() + }() + + // 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) + } + + // Wait for either the callback or a timeout. + select { + case <-h.ctx.Done(): + return nil, fmt.Errorf("timed out waiting for token callback: %w", h.ctx.Err()) + case callback := <-h.callbacks: + if callback.err != nil { + return nil, fmt.Errorf("error handling callback: %w", callback.err) + } + return callback.token, nil + } +} + +func (h *handlerState) handleAuthCodeCallback(w http.ResponseWriter, r *http.Request) (err error) { + // If we return an error, also report it back over the channel to the main CLI thread. + defer func() { + if err != nil { + h.callbacks <- callbackResult{err: err} + } + }() + + // Return HTTP 405 for anything that's not a GET. + if r.Method != http.MethodGet { + return httperr.Newf(http.StatusMethodNotAllowed, "wanted GET") + } + + // Validate OAuth2 state and fail if it's incorrect (to block CSRF). + params := r.URL.Query() + if err := h.state.Validate(params.Get("state")); err != nil { + return httperr.New(http.StatusForbidden, "missing or invalid state parameter") + } + + // Check for error response parameters. + if errorParam := params.Get("error"); errorParam != "" { + return httperr.Newf(http.StatusBadRequest, "login failed with code %q", errorParam) + } + + // Exchange the authorization code for access, ID, and refresh tokens. + oauth2Tok, err := h.oauth2Config.Exchange(r.Context(), params.Get("code"), h.pkce.Verifier()) + if err != nil { + return httperr.Wrap(http.StatusBadRequest, "could not complete code exchange", err) + } + + // Perform required validations on the returned ID token. + idTok, hasIDTok := oauth2Tok.Extra("id_token").(string) + if !hasIDTok { + return httperr.New(http.StatusBadRequest, "received response missing ID token") + } + validated, err := h.idTokenVerifier.Verify(r.Context(), idTok) + if err != nil { + return httperr.Wrap(http.StatusBadRequest, "received invalid ID token", err) + } + if validated.AccessTokenHash != "" { + if err := validated.VerifyAccessToken(oauth2Tok.AccessToken); err != nil { + return httperr.Wrap(http.StatusBadRequest, "received invalid ID token", err) + } + } + if err := h.nonce.Validate(validated); err != nil { + return httperr.Wrap(http.StatusBadRequest, "received ID token with invalid nonce", err) + } + + h.callbacks <- callbackResult{token: &Token{ + Token: oauth2Tok, + IDToken: idTok, + IDTokenExpiry: validated.Expiry, + }} + _, _ = w.Write([]byte("you have been logged in and may now close this tab")) + return nil +} diff --git a/internal/oidcclient/login/login_test.go b/internal/oidcclient/login/login_test.go new file mode 100644 index 00000000..4050ebf2 --- /dev/null +++ b/internal/oidcclient/login/login_test.go @@ -0,0 +1,420 @@ +// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package login + +import ( + "context" + "encoding/json" + "fmt" + "net/http" + "net/http/httptest" + "net/url" + "testing" + "time" + + "github.com/coreos/go-oidc" + "github.com/golang/mock/gomock" + "github.com/stretchr/testify/require" + "golang.org/x/oauth2" + "gopkg.in/square/go-jose.v2" + + "go.pinniped.dev/internal/httputil/httperr" + "go.pinniped.dev/internal/mocks/mockkeyset" + "go.pinniped.dev/internal/oidcclient/nonce" + "go.pinniped.dev/internal/oidcclient/pkce" + "go.pinniped.dev/internal/oidcclient/state" +) + +func TestRun(t *testing.T) { + time1 := time.Date(3020, 10, 12, 13, 14, 15, 16, time.UTC) + testToken := Token{ + Token: &oauth2.Token{ + AccessToken: "test-access-token", + RefreshToken: "test-refresh-token", + Expiry: time1.Add(1 * time.Minute), + }, + IDToken: "test-id-token", + IDTokenExpiry: time1.Add(2 * time.Minute), + } + _ = testToken + + // Start a test server that returns 500 errors + errorServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Error(w, "some discovery error", http.StatusInternalServerError) + })) + t.Cleanup(errorServer.Close) + + // Start a test server that returns a real keyset + providerMux := http.NewServeMux() + successServer := httptest.NewServer(providerMux) + t.Cleanup(successServer.Close) + providerMux.HandleFunc("/.well-known/openid-configuration", func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("content-type", "application/json") + type providerJSON struct { + Issuer string `json:"issuer"` + AuthURL string `json:"authorization_endpoint"` + TokenURL string `json:"token_endpoint"` + JWKSURL string `json:"jwks_uri"` + } + _ = json.NewEncoder(w).Encode(&providerJSON{ + Issuer: successServer.URL, + AuthURL: successServer.URL + "/authorize", + TokenURL: successServer.URL + "/token", + JWKSURL: successServer.URL + "/keys", + }) + }) + + tests := []struct { + name string + opt func(t *testing.T) Option + issuer string + clientID string + wantErr string + wantToken *Token + }{ + { + name: "option error", + opt: func(t *testing.T) Option { + return func(h *handlerState) error { + return fmt.Errorf("some option error") + } + }, + wantErr: "some option error", + }, + { + name: "error generating state", + opt: func(t *testing.T) Option { + return func(h *handlerState) error { + h.generateState = func() (state.State, error) { return "", fmt.Errorf("some error generating state") } + return nil + } + }, + wantErr: "some error generating state", + }, + { + name: "error generating nonce", + opt: func(t *testing.T) Option { + return func(h *handlerState) error { + h.generateNonce = func() (nonce.Nonce, error) { return "", fmt.Errorf("some error generating nonce") } + return nil + } + }, + wantErr: "some error generating nonce", + }, + { + name: "error generating PKCE", + opt: func(t *testing.T) Option { + return func(h *handlerState) error { + h.generatePKCE = func() (pkce.Code, error) { return "", fmt.Errorf("some error generating PKCE") } + return nil + } + }, + wantErr: "some error generating PKCE", + }, + { + name: "discovery failure", + opt: func(t *testing.T) Option { + return func(h *handlerState) error { return nil } + }, + issuer: errorServer.URL, + wantErr: fmt.Sprintf("could not perform OIDC discovery for %q: 500 Internal Server Error: some discovery error\n", errorServer.URL), + }, + { + name: "listen failure", + opt: func(t *testing.T) Option { + return func(h *handlerState) error { + h.listenAddr = "invalid-listen-address" + return nil + } + }, + issuer: successServer.URL, + wantErr: "could not open callback listener: listen tcp: address invalid-listen-address: missing port in address", + }, + { + name: "browser open failure", + opt: func(t *testing.T) Option { + return WithBrowserOpen(func(url string) error { + return fmt.Errorf("some browser open error") + }) + }, + issuer: successServer.URL, + wantErr: "could not open browser: some browser open error", + }, + { + name: "timeout waiting for callback", + opt: func(t *testing.T) Option { + return func(h *handlerState) error { + ctx, cancel := context.WithCancel(h.ctx) + h.ctx = ctx + + h.openURL = func(_ string) error { + cancel() + return nil + } + return nil + } + }, + issuer: successServer.URL, + wantErr: "timed out waiting for token callback: context canceled", + }, + { + name: "callback returns error", + opt: func(t *testing.T) Option { + return func(h *handlerState) error { + h.openURL = func(_ string) error { + go func() { + h.callbacks <- callbackResult{err: fmt.Errorf("some callback error")} + }() + return nil + } + return nil + } + }, + issuer: successServer.URL, + wantErr: "error handling callback: some callback error", + }, + { + name: "callback returns success", + clientID: "test-client-id", + opt: func(t *testing.T) Option { + return func(h *handlerState) error { + h.generateState = func() (state.State, error) { return "test-state", nil } + h.generatePKCE = func() (pkce.Code, error) { return "test-pkce", nil } + h.generateNonce = func() (nonce.Nonce, error) { return "test-nonce", nil } + + h.openURL = func(actualURL string) error { + parsedActualURL, err := url.Parse(actualURL) + require.NoError(t, err) + actualParams := parsedActualURL.Query() + + require.Contains(t, actualParams.Get("redirect_uri"), "http://127.0.0.1:") + actualParams.Del("redirect_uri") + + require.Equal(t, url.Values{ + // 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"}, + "code_challenge_method": []string{"S256"}, + "response_type": []string{"code"}, + "scope": []string{"test-scope"}, + "nonce": []string{"test-nonce"}, + "state": []string{"test-state"}, + "access_type": []string{"offline"}, + "client_id": []string{"test-client-id"}, + }, actualParams) + + parsedActualURL.RawQuery = "" + require.Equal(t, successServer.URL+"/authorize", parsedActualURL.String()) + + go func() { + h.callbacks <- callbackResult{token: &testToken} + }() + return nil + } + return nil + } + }, + issuer: successServer.URL, + wantToken: &testToken, + }, + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + tok, err := Run(tt.issuer, tt.clientID, + WithContext(context.Background()), + WithListenPort(0), + WithScopes([]string{"test-scope"}), + tt.opt(t), + ) + if tt.wantErr != "" { + require.EqualError(t, err, tt.wantErr) + require.Nil(t, tok) + return + } + require.Equal(t, tt.wantToken, tok) + }) + } +} + +func TestHandleAuthCodeCallback(t *testing.T) { + tests := []struct { + name string + method string + query string + returnIDTok string + wantErr string + wantHTTPStatus int + }{ + { + name: "wrong method", + method: "POST", + query: "", + wantErr: "wanted GET", + wantHTTPStatus: http.StatusMethodNotAllowed, + }, + { + name: "invalid state", + query: "state=invalid", + wantErr: "missing or invalid state parameter", + wantHTTPStatus: http.StatusForbidden, + }, + { + name: "error code from provider", + query: "state=test-state&error=some_error", + wantErr: `login failed with code "some_error"`, + wantHTTPStatus: http.StatusBadRequest, + }, + { + name: "invalid code", + query: "state=test-state&code=invalid", + wantErr: "could not complete code exchange: oauth2: cannot fetch token: 403 Forbidden\nResponse: invalid authorization code\n", + wantHTTPStatus: http.StatusBadRequest, + }, + { + name: "missing ID token", + query: "state=test-state&code=valid", + returnIDTok: "", + wantErr: "received response missing ID token", + wantHTTPStatus: http.StatusBadRequest, + }, + { + name: "invalid ID token", + query: "state=test-state&code=valid", + returnIDTok: "invalid-jwt", + wantErr: "received invalid ID token: oidc: malformed jwt: square/go-jose: compact JWS format must have three parts", + wantHTTPStatus: http.StatusBadRequest, + }, + { + name: "invalid access token hash", + query: "state=test-state&code=valid", + + // Test JWT generated with https://smallstep.com/docs/cli/crypto/jwt/: + // step crypto keypair key.pub key.priv --kty RSA --no-password --insecure --force && echo '{"at_hash": "invalid-at-hash"}' | step crypto jwt sign --key key.priv --aud test-client-id --sub test-user --subtle --kid="test-kid" --jti="test-jti" + returnIDTok: "eyJhbGciOiJSUzI1NiIsImtpZCI6InRlc3Qta2lkIiwidHlwIjoiSldUIn0.eyJhdF9oYXNoIjoiaW52YWxpZC1hdC1oYXNoIiwiYXVkIjoidGVzdC1jbGllbnQtaWQiLCJpYXQiOjE2MDIyODM3OTEsImp0aSI6InRlc3QtanRpIiwibmJmIjoxNjAyMjgzNzkxLCJzdWIiOiJ0ZXN0LXVzZXIifQ.jryXr4jiwcf79wBLaHpjdclEYHoUFGhvTu95QyA6Hnk9NQ0x1vsWYurtj7a8uKydNPryC_HNZi9QTAE_tRIJjycseog3695-5y4B4EZlqL-a94rdOtffuF2O_lnPbKvoja9EKNrp0kLBCftFRHhLAEwuP0N9E5padZwPpIGK0yE_JqljnYgCySvzsQu7tasR38yaULny13h3mtp2WRHPG5DrLyuBuF8Z01hSgRi5hGcVpgzTwBgV5-eMaSUCUo-ZDkqUsLQI6dVlaikCSKYZRb53HeexH0tB_R9PJJHY7mIr-rS76kkQEx9pLuVnheIH9Oc6zbdYWg-zWMijopA8Pg", + + wantErr: "received invalid ID token: access token hash does not match value in ID token", + wantHTTPStatus: http.StatusBadRequest, + }, + { + name: "invalid nonce", + query: "state=test-state&code=valid", + + // Test JWT generated with https://smallstep.com/docs/cli/crypto/jwt/: + // step crypto keypair key.pub key.priv --kty RSA --no-password --insecure --force && echo '{"nonce": "invalid-nonce"}' | step crypto jwt sign --key key.priv --aud test-client-id --sub test-user --subtle --kid="test-kid" --jti="test-jti" + returnIDTok: "eyJhbGciOiJSUzI1NiIsImtpZCI6InRlc3Qta2lkIiwidHlwIjoiSldUIn0.eyJhdWQiOiJ0ZXN0LWNsaWVudC1pZCIsImlhdCI6MTYwMjI4Mzc0MSwianRpIjoidGVzdC1qdGkiLCJuYmYiOjE2MDIyODM3NDEsIm5vbmNlIjoiaW52YWxpZC1ub25jZSIsInN1YiI6InRlc3QtdXNlciJ9.PRpq-7j5djaIAkraL-8t8ad9Xm4hM8RW67gyD1VIe0BecWeBFxsTuh3SZVKM9zmcwTgjudsyn8kQOwipDa49IN4PV8FcJA_uUJZi2wiqGJUSTG2K5I89doV_7e0RM1ZYIDDW1G2heKJNW7MbKkX7iEPr7u4MyEzswcPcupbyDA-CQFeL95vgwawoqa6yO94ympTbozqiNfj6Xyw_nHtThQnstjWsJZ9s2mUgppZezZv4HZYTQ7c3e_bzwhWgCzh2CSDJn9_Ra_n_4GcVkpHbsHTP35dFsnf0vactPx6CAu6A1-Apk-BruCktpZ3B4Ercf1UnUOHdGqzQKJtqvB03xQ", + + wantHTTPStatus: http.StatusBadRequest, + wantErr: `received ID token with invalid nonce: invalid nonce (expected "test-nonce", got "invalid-nonce")`, + }, + { + name: "valid", + query: "state=test-state&code=valid", + + // Test JWT generated with https://smallstep.com/docs/cli/crypto/jwt/: + // step crypto keypair key.pub key.priv --kty RSA --no-password --insecure --force && echo '{"nonce": "test-nonce"}' | step crypto jwt sign --key key.priv --aud test-client-id --sub test-user --subtle --kid="test-kid" --jti="test-jti" + returnIDTok: "eyJhbGciOiJSUzI1NiIsImtpZCI6InRlc3Qta2lkIiwidHlwIjoiSldUIn0.eyJhdWQiOiJ0ZXN0LWNsaWVudC1pZCIsImlhdCI6MTYwMjUzMTU2NywianRpIjoidGVzdC1qdGkiLCJuYmYiOjE2MDI1MzE1NjcsIm5vbmNlIjoidGVzdC1ub25jZSIsInN1YiI6InRlc3QtdXNlciJ9.LbOA31iwJZBM4ayY5Oud-HArLXbmtAIhZv_LazDqbzA2Iw87RxoBemfiPUJeAesdnO1LKSjBwbltZwtjvbLWHp1R5tqrSMr_hl2OyZv1cpEX-9QaTcQILJ5qR00riRLz34ZCQFyF-FfQpP1r4dNqFrxHuiBwKuPE7zogc83ZYJgAQM5Fao9rIRY9JStL_3pURa9JnnSHFlkLvFYv3TKEUyvnW4pWvYZcsGI7mys43vuSjpG7ZSrW3vCxovuIpXYqAhamZL_XexWUsXvi3ej9HNlhnhOFhN4fuPSc0PWDWaN0CLWmoo8gvOdQWo5A4GD4bNGBzjYOd-pYqsDfseRt1Q", + }, + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + tokenServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + require.Equal(t, http.MethodPost, r.Method) + require.NoError(t, r.ParseForm()) + require.Equal(t, "test-client-id", r.Form.Get("client_id")) + require.Equal(t, "test-pkce", r.Form.Get("code_verifier")) + require.Equal(t, "authorization_code", r.Form.Get("grant_type")) + require.NotEmpty(t, r.Form.Get("code")) + if r.Form.Get("code") != "valid" { + http.Error(w, "invalid authorization code", http.StatusForbidden) + return + } + var response struct { + oauth2.Token + IDToken string `json:"id_token,omitempty"` + } + response.AccessToken = "test-access-token" + response.Expiry = time.Now().Add(time.Hour) + response.IDToken = tt.returnIDTok + w.Header().Set("content-type", "application/json") + require.NoError(t, json.NewEncoder(w).Encode(&response)) + })) + t.Cleanup(tokenServer.Close) + + h := &handlerState{ + callbacks: make(chan callbackResult, 1), + state: state.State("test-state"), + pkce: pkce.Code("test-pkce"), + nonce: nonce.Nonce("test-nonce"), + oauth2Config: &oauth2.Config{ + ClientID: "test-client-id", + RedirectURL: "http://localhost:12345/callback", + Endpoint: oauth2.Endpoint{ + TokenURL: tokenServer.URL, + AuthStyle: oauth2.AuthStyleInParams, + }, + }, + idTokenVerifier: mockVerifier(), + } + + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + resp := httptest.NewRecorder() + req, err := http.NewRequestWithContext(ctx, "GET", "/test-callback", nil) + require.NoError(t, err) + req.URL.RawQuery = tt.query + if tt.method != "" { + req.Method = tt.method + } + + err = h.handleAuthCodeCallback(resp, req) + if tt.wantErr != "" { + require.EqualError(t, err, tt.wantErr) + if tt.wantHTTPStatus != 0 { + rec := httptest.NewRecorder() + err.(httperr.Responder).Respond(rec) + require.Equal(t, tt.wantHTTPStatus, rec.Code) + } + } else { + require.NoError(t, err) + } + + select { + case <-time.After(1 * time.Second): + require.Fail(t, "timed out waiting to receive from callbacks channel") + case result := <-h.callbacks: + if tt.wantErr != "" { + require.EqualError(t, result.err, tt.wantErr) + return + } + require.NoError(t, result.err) + require.NotNil(t, result.token) + require.Equal(t, result.token.IDToken, tt.returnIDTok) + } + }) + } +} + +// mockVerifier returns an *oidc.IDTokenVerifier that validates any correctly serialized JWT without doing much else. +func mockVerifier() *oidc.IDTokenVerifier { + mockKeySet := mockkeyset.NewMockKeySet(gomock.NewController(nil)) + mockKeySet.EXPECT().VerifySignature(gomock.Any(), gomock.Any()). + AnyTimes(). + DoAndReturn(func(ctx context.Context, jwt string) ([]byte, error) { + jws, err := jose.ParseSigned(jwt) + if err != nil { + return nil, err + } + return jws.UnsafePayloadWithoutVerification(), nil + }) + + return oidc.NewVerifier("", mockKeySet, &oidc.Config{ + SkipIssuerCheck: true, + SkipExpiryCheck: true, + SkipClientIDCheck: true, + }) +} diff --git a/internal/oidcclient/nonce/nonce.go b/internal/oidcclient/nonce/nonce.go new file mode 100644 index 00000000..fc10112e --- /dev/null +++ b/internal/oidcclient/nonce/nonce.go @@ -0,0 +1,58 @@ +// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +// Package nonce implements +package nonce + +import ( + "crypto/rand" + "crypto/subtle" + "encoding/hex" + "fmt" + "io" + + "github.com/coreos/go-oidc" + "golang.org/x/oauth2" +) + +// Generate generates a new random OIDC nonce parameter of an appropriate size. +func Generate() (Nonce, error) { return generate(rand.Reader) } + +func generate(rand io.Reader) (Nonce, error) { + var buf [16]byte + if _, err := io.ReadFull(rand, buf[:]); err != nil { + return "", fmt.Errorf("could not generate random nonce: %w", err) + } + return Nonce(hex.EncodeToString(buf[:])), nil +} + +// Nonce implements some utilities for working with OIDC nonce parameters. +type Nonce string + +// String returns the string encoding of this state value. +func (n *Nonce) String() string { + return string(*n) +} + +// Param returns the OAuth2 auth code parameter for sending the nonce during the authorization request. +func (n *Nonce) Param() oauth2.AuthCodeOption { + return oidc.Nonce(string(*n)) +} + +// Validate the returned ID token). Returns true iff the nonce matches or the returned JWT does not have a nonce. +func (n *Nonce) Validate(token *oidc.IDToken) error { + if subtle.ConstantTimeCompare([]byte(token.Nonce), []byte(*n)) != 1 { + return InvalidNonceError{Expected: *n, Got: Nonce(token.Nonce)} + } + return nil +} + +// InvalidNonceError is returned by Validate when the observed nonce is invalid. +type InvalidNonceError struct { + Expected Nonce + Got Nonce +} + +func (e InvalidNonceError) Error() string { + return fmt.Sprintf("invalid nonce (expected %q, got %q)", e.Expected.String(), e.Got.String()) +} diff --git a/internal/oidcclient/nonce/nonce_test.go b/internal/oidcclient/nonce/nonce_test.go new file mode 100644 index 00000000..9738a245 --- /dev/null +++ b/internal/oidcclient/nonce/nonce_test.go @@ -0,0 +1,40 @@ +// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package nonce + +import ( + "bytes" + "errors" + "net/url" + "testing" + + "github.com/coreos/go-oidc" + "github.com/stretchr/testify/require" + "golang.org/x/oauth2" +) + +func TestNonce(t *testing.T) { + n, err := Generate() + require.NoError(t, err) + require.Len(t, n, 32) + require.Len(t, n.String(), 32) + + cfg := oauth2.Config{} + authCodeURL, err := url.Parse(cfg.AuthCodeURL("", n.Param())) + require.NoError(t, err) + require.Equal(t, n.String(), authCodeURL.Query().Get("nonce")) + + require.Error(t, n.Validate(&oidc.IDToken{})) + require.NoError(t, n.Validate(&oidc.IDToken{Nonce: string(n)})) + + err = n.Validate(&oidc.IDToken{Nonce: string(n) + "x"}) + require.Error(t, err) + require.True(t, errors.As(err, &InvalidNonceError{})) + require.Contains(t, err.Error(), string(n)+"x") + + var empty bytes.Buffer + n, err = generate(&empty) + require.EqualError(t, err, "could not generate random nonce: EOF") + require.Empty(t, n) +} diff --git a/internal/oidc/pkce/pkce.go b/internal/oidcclient/pkce/pkce.go similarity index 76% rename from internal/oidc/pkce/pkce.go rename to internal/oidcclient/pkce/pkce.go index 309b3d4d..d28f55df 100644 --- a/internal/oidc/pkce/pkce.go +++ b/internal/oidcclient/pkce/pkce.go @@ -8,9 +8,9 @@ import ( "crypto/sha256" "encoding/base64" "encoding/hex" + "fmt" "io" - "github.com/pkg/errors" "golang.org/x/oauth2" ) @@ -18,9 +18,14 @@ import ( func Generate() (Code, error) { return generate(rand.Reader) } func generate(rand io.Reader) (Code, error) { + // From https://tools.ietf.org/html/rfc7636#section-4.1: + // code_verifier = high-entropy cryptographic random STRING using the + // unreserved characters [A-Z] / [a-z] / [0-9] / "-" / "." / "_" / "~" + // from Section 2.3 of [RFC3986], with a minimum length of 43 characters + // and a maximum length of 128 characters. var buf [32]byte if _, err := io.ReadFull(rand, buf[:]); err != nil { - return "", errors.WithMessage(err, "could not generate PKCE code") + return "", fmt.Errorf("could not generate PKCE code: %w", err) } return Code(hex.EncodeToString(buf[:])), nil } diff --git a/internal/oidc/pkce/pkce_test.go b/internal/oidcclient/pkce/pkce_test.go similarity index 100% rename from internal/oidc/pkce/pkce_test.go rename to internal/oidcclient/pkce/pkce_test.go diff --git a/internal/oidcclient/state/state.go b/internal/oidcclient/state/state.go new file mode 100644 index 00000000..ff71e726 --- /dev/null +++ b/internal/oidcclient/state/state.go @@ -0,0 +1,56 @@ +// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package state + +import ( + "crypto/rand" + "crypto/subtle" + "encoding/hex" + "fmt" + "io" +) + +// Generate generates a new random state parameter of an appropriate size. +func Generate() (State, error) { return generate(rand.Reader) } + +func generate(rand io.Reader) (State, error) { + // From https://tools.ietf.org/html/rfc6749#section-10.12: + // The binding value used for CSRF + // protection MUST contain a non-guessable value (as described in + // Section 10.10), and the user-agent's authenticated state (e.g., + // session cookie, HTML5 local storage) MUST be kept in a location + // accessible only to the client and the user-agent (i.e., protected by + // same-origin policy). + var buf [16]byte + if _, err := io.ReadFull(rand, buf[:]); err != nil { + return "", fmt.Errorf("could not generate random state: %w", err) + } + return State(hex.EncodeToString(buf[:])), nil +} + +// State implements some utilities for working with OAuth2 state parameters. +type State string + +// String returns the string encoding of this state value. +func (s *State) String() string { + return string(*s) +} + +// Validate the returned state (from a callback parameter). +func (s *State) Validate(returnedState string) error { + if subtle.ConstantTimeCompare([]byte(returnedState), []byte(*s)) != 1 { + return InvalidStateError{Expected: *s, Got: State(returnedState)} + } + return nil +} + +// InvalidStateError is returned by Validate when the returned state is invalid. +type InvalidStateError struct { + Expected State + Got State +} + +func (e InvalidStateError) Error() string { + return fmt.Sprintf("invalid state (expected %q, got %q)", e.Expected.String(), e.Got.String()) +} diff --git a/internal/oidc/state/state_test.go b/internal/oidcclient/state/state_test.go similarity index 68% rename from internal/oidc/state/state_test.go rename to internal/oidcclient/state/state_test.go index ff181839..a12a3b16 100644 --- a/internal/oidc/state/state_test.go +++ b/internal/oidcclient/state/state_test.go @@ -5,6 +5,7 @@ package state import ( "bytes" + "errors" "testing" "github.com/stretchr/testify/require" @@ -15,8 +16,11 @@ func TestState(t *testing.T) { require.NoError(t, err) require.Len(t, s, 32) require.Len(t, s.String(), 32) - require.True(t, s.Valid(string(s))) - require.False(t, s.Valid(string(s)+"x")) + require.NoError(t, s.Validate(string(s))) + err = s.Validate(string(s) + "x") + require.Error(t, err) + require.True(t, errors.As(err, &InvalidStateError{})) + require.Contains(t, err.Error(), string(s)+"x") var empty bytes.Buffer s, err = generate(&empty) From d1e86e2616b637cefa238c67c5e2f73c1958ae03 Mon Sep 17 00:00:00 2001 From: Matt Moyer Date: Tue, 13 Oct 2020 09:13:40 -0500 Subject: [PATCH 4/9] Rename "TestClusterCapability" to more generic "Capability." This will be used for other types of "capabilities" of the test environment besides just those of the test cluster, such as those of an upstream OIDC provider. Signed-off-by: Matt Moyer --- test/library/env.go | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/test/library/env.go b/test/library/env.go index f97aa5f8..82ab0b46 100644 --- a/test/library/env.go +++ b/test/library/env.go @@ -15,10 +15,10 @@ import ( idpv1alpha1 "go.pinniped.dev/generated/1.19/apis/idp/v1alpha1" ) -type TestClusterCapability string +type Capability string const ( - ClusterSigningKeyIsAvailable = TestClusterCapability("clusterSigningKeyIsAvailable") + ClusterSigningKeyIsAvailable = Capability("clusterSigningKeyIsAvailable") ) // TestEnv captures all the external parameters consumed by our integration tests. @@ -29,7 +29,7 @@ type TestEnv struct { SupervisorNamespace string `json:"supervisorNamespace"` ConciergeAppName string `json:"conciergeAppName"` SupervisorAppName string `json:"supervisorAppName"` - Capabilities map[TestClusterCapability]bool `json:"capabilities"` + Capabilities map[Capability]bool `json:"capabilities"` TestWebhook idpv1alpha1.WebhookIdentityProviderSpec `json:"testWebhook"` SupervisorAddress string `json:"supervisorAddress"` @@ -83,25 +83,25 @@ func IntegrationEnv(t *testing.T) *TestEnv { return &result } -func (e *TestEnv) HasCapability(cap TestClusterCapability) bool { +func (e *TestEnv) HasCapability(cap Capability) bool { e.t.Helper() isCapable, capabilityWasDescribed := e.Capabilities[cap] - require.True(e.t, capabilityWasDescribed, `the cluster's "%s" capability was not described`, cap) + require.Truef(e.t, capabilityWasDescribed, "the %q capability of the test environment was not described", cap) return isCapable } -func (e *TestEnv) WithCapability(cap TestClusterCapability) *TestEnv { +func (e *TestEnv) WithCapability(cap Capability) *TestEnv { e.t.Helper() if !e.HasCapability(cap) { - e.t.Skipf(`skipping integration test because cluster lacks the "%s" capability`, cap) + e.t.Skipf("skipping integration test because test environment lacks the %q capability", cap) } return e } -func (e *TestEnv) WithoutCapability(cap TestClusterCapability) *TestEnv { +func (e *TestEnv) WithoutCapability(cap Capability) *TestEnv { e.t.Helper() if e.HasCapability(cap) { - e.t.Skipf(`skipping integration test because cluster has the "%s" capability`, cap) + e.t.Skipf("skipping integration test because test environment has the %q capability", cap) } return e } From 8a16a92c016998603ebd212db48a940b9b5bcfae Mon Sep 17 00:00:00 2001 From: Matt Moyer Date: Tue, 13 Oct 2020 10:25:39 -0500 Subject: [PATCH 5/9] Rename some existing CLI test code. It will no longer be the only CLI test, so the names should be a bit more specific. Signed-off-by: Matt Moyer --- test/integration/cli_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/integration/cli_test.go b/test/integration/cli_test.go index c1b13c74..9bb6502e 100644 --- a/test/integration/cli_test.go +++ b/test/integration/cli_test.go @@ -16,7 +16,7 @@ import ( "go.pinniped.dev/test/library" ) -func TestCLI(t *testing.T) { +func TestCLIGetKubeconfig(t *testing.T) { env := library.IntegrationEnv(t).WithCapability(library.ClusterSigningKeyIsAvailable) // Create a test webhook configuration to use with the CLI. @@ -30,7 +30,7 @@ func TestCLI(t *testing.T) { defer cleanupFunc() // Run pinniped CLI to get kubeconfig. - kubeConfigYAML := runPinnipedCLI(t, pinnipedExe, env.TestUser.Token, env.ConciergeNamespace, "webhook", idp.Name) + kubeConfigYAML := runPinnipedCLIGetKubeconfig(t, pinnipedExe, env.TestUser.Token, env.ConciergeNamespace, "webhook", idp.Name) // In addition to the client-go based testing below, also try the kubeconfig // with kubectl to validate that it works. @@ -79,7 +79,7 @@ func buildPinnipedCLI(t *testing.T) (string, func()) { } } -func runPinnipedCLI(t *testing.T, pinnipedExe, token, namespaceName, idpType, idpName string) string { +func runPinnipedCLIGetKubeconfig(t *testing.T, pinnipedExe, token, namespaceName, idpType, idpName string) string { t.Helper() output, err := exec.Command( From 50d80489be05b9dbf3f7977dcb2c858ec322deaa Mon Sep 17 00:00:00 2001 From: Matt Moyer Date: Tue, 13 Oct 2020 10:41:53 -0500 Subject: [PATCH 6/9] Add initial CLI integration test for OIDC login. This is our first test using a real browser to interact with an upstream provider. Signed-off-by: Matt Moyer --- go.mod | 2 + go.sum | 3 + test/integration/cli_test.go | 198 +++++++++++++++++++++++++++++++++-- test/library/env.go | 25 ++++- 4 files changed, 219 insertions(+), 9 deletions(-) diff --git a/go.mod b/go.mod index f2c36c58..0b476b08 100644 --- a/go.mod +++ b/go.mod @@ -4,6 +4,7 @@ go 1.14 require ( github.com/MakeNowJust/heredoc/v2 v2.0.1 + github.com/blang/semver v3.5.1+incompatible // indirect github.com/coreos/go-oidc v2.2.1+incompatible github.com/davecgh/go-spew v1.1.1 github.com/ghodss/yaml v1.0.0 @@ -13,6 +14,7 @@ require ( github.com/golangci/golangci-lint v1.31.0 github.com/google/go-cmp v0.5.2 github.com/pkg/browser v0.0.0-20180916011732-0a3d74bf9ce4 + github.com/sclevine/agouti v3.0.0+incompatible github.com/sclevine/spec v1.4.0 github.com/spf13/cobra v1.0.0 github.com/spf13/pflag v1.0.5 diff --git a/go.sum b/go.sum index dae855b9..a51ae7d4 100644 --- a/go.sum +++ b/go.sum @@ -71,6 +71,8 @@ github.com/bgentry/speakeasy v0.1.0/go.mod h1:+zsyZBPWlz7T6j88CTgSN5bM796AkVf0kB github.com/bketelsen/crypt v0.0.3-0.20200106085610-5cbc8cc4026c/go.mod h1:MKsuJmJgSg28kpZDP6UIiPt0e0Oz0kqKNGyRaWEPv84= github.com/blang/semver v3.5.0+incompatible h1:CGxCgetQ64DKk7rdZ++Vfnb1+ogGNnB17OJKJXD2Cfs= github.com/blang/semver v3.5.0+incompatible/go.mod h1:kRBLl5iJ+tD4TcOOxsy/0fnwebNt5EWlYSAyrTnjyyk= +github.com/blang/semver v3.5.1+incompatible h1:cQNTCjp13qL8KC3Nbxr/y2Bqb63oX6wdnnjpJbkM4JQ= +github.com/blang/semver v3.5.1+incompatible/go.mod h1:kRBLl5iJ+tD4TcOOxsy/0fnwebNt5EWlYSAyrTnjyyk= github.com/bombsimon/wsl/v3 v3.1.0 h1:E5SRssoBgtVFPcYWUOFJEcgaySgdtTNYzsSKDOY7ss8= github.com/bombsimon/wsl/v3 v3.1.0/go.mod h1:st10JtZYLE4D5sC7b8xV4zTKZwAQjCH/Hy2Pm1FNZIc= github.com/census-instrumentation/opencensus-proto v0.2.1/go.mod h1:f6KPmirojxKA12rnyqOA5BBL4O983OfeGPqjHWSTneU= @@ -498,6 +500,7 @@ github.com/ryancurrah/gomodguard v1.1.0/go.mod h1:4O8tr7hBODaGE6VIhfJDHcwzh5GUcc github.com/ryanrolds/sqlclosecheck v0.3.0 h1:AZx+Bixh8zdUBxUA1NxbxVAS78vTPq4rCb8OUZI9xFw= github.com/ryanrolds/sqlclosecheck v0.3.0/go.mod h1:1gREqxyTGR3lVtpngyFo3hZAgk0KCtEdgEkHwDbigdA= github.com/ryanuber/columnize v0.0.0-20160712163229-9b3edd62028f/go.mod h1:sm1tb6uqfes/u+d4ooFouqFdy9/2g9QGwK3SQygK0Ts= +github.com/sclevine/agouti v3.0.0+incompatible h1:8IBJS6PWz3uTlMP3YBIR5f+KAldcGuOeFkFbUWfBgK4= github.com/sclevine/agouti v3.0.0+incompatible/go.mod h1:b4WX9W9L1sfQKXeJf1mUTLZKJ48R1S7H23Ji7oFO5Bw= github.com/sclevine/spec v1.4.0 h1:z/Q9idDcay5m5irkZ28M7PtQM4aOISzOpj4bUPkDee8= github.com/sclevine/spec v1.4.0/go.mod h1:LvpgJaFyvQzRvc1kaDs0bulYwzC70PbiYjC4QnFHkOM= diff --git a/test/integration/cli_test.go b/test/integration/cli_test.go index 9bb6502e..8365d8c0 100644 --- a/test/integration/cli_test.go +++ b/test/integration/cli_test.go @@ -3,15 +3,25 @@ package integration import ( + "bufio" "context" + "encoding/json" + "io" "io/ioutil" "os" "os/exec" "path/filepath" + "regexp" + "strconv" + "strings" + "sync" "testing" "time" + "github.com/sclevine/agouti" "github.com/stretchr/testify/require" + "gopkg.in/square/go-jose.v2" + clientauthenticationv1beta1 "k8s.io/client-go/pkg/apis/clientauthentication/v1beta1" "go.pinniped.dev/test/library" ) @@ -26,8 +36,7 @@ func TestCLIGetKubeconfig(t *testing.T) { idp := library.CreateTestWebhookIDP(ctx, t) // Build pinniped CLI. - pinnipedExe, cleanupFunc := buildPinnipedCLI(t) - defer cleanupFunc() + pinnipedExe := buildPinnipedCLI(t) // Run pinniped CLI to get kubeconfig. kubeConfigYAML := runPinnipedCLIGetKubeconfig(t, pinnipedExe, env.TestUser.Token, env.ConciergeNamespace, "webhook", idp.Name) @@ -58,11 +67,12 @@ func TestCLIGetKubeconfig(t *testing.T) { } } -func buildPinnipedCLI(t *testing.T) (string, func()) { +func buildPinnipedCLI(t *testing.T) string { t.Helper() pinnipedExeDir, err := ioutil.TempDir("", "pinniped-cli-test-*") require.NoError(t, err) + t.Cleanup(func() { require.NoError(t, os.RemoveAll(pinnipedExeDir)) }) pinnipedExe := filepath.Join(pinnipedExeDir, "pinniped") output, err := exec.Command( @@ -73,10 +83,7 @@ func buildPinnipedCLI(t *testing.T) (string, func()) { "go.pinniped.dev/cmd/pinniped", ).CombinedOutput() require.NoError(t, err, string(output)) - - return pinnipedExe, func() { - require.NoError(t, os.RemoveAll(pinnipedExeDir)) - } + return pinnipedExe } func runPinnipedCLIGetKubeconfig(t *testing.T, pinnipedExe, token, namespaceName, idpType, idpName string) string { @@ -94,3 +101,180 @@ func runPinnipedCLIGetKubeconfig(t *testing.T, pinnipedExe, token, namespaceName return string(output) } + +func TestCLILoginOIDC(t *testing.T) { + var ( + oktaURLPattern = regexp.MustCompile(`\Ahttps://.+.okta.com/.+\z`) + localURLPattern = regexp.MustCompile(`\Ahttp://127.0.0.1.+\z`) + ) + + env := library.IntegrationEnv(t).WithCapability(library.ExternalOIDCProviderIsAvailable) + + // Build pinniped CLI. + t.Logf("building CLI binary") + pinnipedExe := buildPinnipedCLI(t) + + cmd := exec.Command(pinnipedExe, "alpha", "login", "oidc", + "--issuer", env.OIDCUpstream.Issuer, + "--client-id", env.OIDCUpstream.ClientID, + "--listen-port", strconv.Itoa(env.OIDCUpstream.LocalhostPort), + "--skip-browser", + ) + + // Create a WaitGroup that will wait for all child goroutines to finish, so they can assert errors. + var wg sync.WaitGroup + defer wg.Wait() + + // Start a background goroutine to read stderr from the CLI and parse out the login URL. + loginURLChan := make(chan string) + stderr, err := cmd.StderrPipe() + require.NoError(t, err) + wg.Add(1) + go func() { + r := bufio.NewReader(stderr) + line, err := r.ReadString('\n') + require.NoError(t, err) + const prompt = "Please log in: " + require.Truef(t, strings.HasPrefix(line, prompt), "expected %q to have prefix %q", line, prompt) + loginURLChan <- strings.TrimPrefix(line, prompt) + _, err = io.Copy(ioutil.Discard, r) + t.Logf("stderr stream closed") + require.NoError(t, err) + wg.Done() + }() + + // Start a background goroutine to read stdout from the CLI and parse out an ExecCredential. + credOutputChan := make(chan clientauthenticationv1beta1.ExecCredential) + stdout, err := cmd.StdoutPipe() + require.NoError(t, err) + wg.Add(1) + go func() { + r := bufio.NewReader(stdout) + + var out clientauthenticationv1beta1.ExecCredential + require.NoError(t, json.NewDecoder(r).Decode(&out)) + credOutputChan <- out + + _, err = io.Copy(ioutil.Discard, r) + t.Logf("stdout stream closed") + require.NoError(t, err) + wg.Done() + }() + + t.Logf("starting CLI subprocess") + require.NoError(t, cmd.Start()) + wg.Add(1) + defer func() { + err := cmd.Wait() + t.Logf("CLI subprocess exited") + require.NoError(t, err) + wg.Done() + }() + + // Start the browser driver. + t.Logf("opening browser driver") + agoutiDriver := agouti.ChromeDriver( + // Comment out this line to see the tests happen in a visible browser window. + agouti.ChromeOptions("args", []string{"--headless"}), + ) + require.NoError(t, agoutiDriver.Start()) + t.Cleanup(func() { require.NoError(t, agoutiDriver.Stop()) }) + page, err := agoutiDriver.NewPage(agouti.Browser("chrome")) + require.NoError(t, err) + + // Wait for the CLI to print out the login URL and open the browser to it. + t.Logf("waiting for CLI to output login URL") + var loginURL string + select { + case <-time.After(1 * time.Minute): + require.Fail(t, "timed out waiting for login URL") + case loginURL = <-loginURLChan: + } + t.Logf("navigating to login page") + require.NoError(t, page.Navigate(loginURL)) + + // Expect to be redirected to the Okta login page. + t.Logf("waiting for redirect to Okta login page") + waitForURL(t, page, oktaURLPattern) + + // Wait for the login page to be rendered. + waitForVisibleElements(t, page, + "input#okta-signin-username", + "input#okta-signin-password", + "input#okta-signin-submit", + ) + + // Fill in the username and password and click "submit". + t.Logf("logging into Okta") + require.NoError(t, page.First("input#okta-signin-username").Fill(env.OIDCUpstream.Username)) + require.NoError(t, page.First("input#okta-signin-password").Fill(env.OIDCUpstream.Password)) + require.NoError(t, page.First("input#okta-signin-submit").Click()) + + // Wait for the login to happen and us be redirected back to a localhost callback. + t.Logf("waiting for redirect to localhost callback") + waitForURL(t, page, localURLPattern) + + // Wait for the "pre" element that gets rendered for a `text/plain` page, and + // assert that it contains the success message. + t.Logf("verifying success page") + waitForVisibleElements(t, page, "pre") + msg, err := page.First("pre").Text() + require.NoError(t, err) + require.Equal(t, "you have been logged in and may now close this tab", msg) + require.NoError(t, page.CloseWindow()) + + // Expect the CLI to output an ExecCredential in JSON format. + t.Logf("waiting for CLI to output ExecCredential JSON") + var credOutput clientauthenticationv1beta1.ExecCredential + select { + case <-time.After(10 * time.Second): + require.Fail(t, "timed out waiting for exec credential output") + case credOutput = <-credOutputChan: + } + + // Assert some properties of the ExecCredential. + t.Logf("validating ExecCredential") + require.NotNil(t, credOutput.Status) + require.Empty(t, credOutput.Status.ClientKeyData) + require.Empty(t, credOutput.Status.ClientCertificateData) + + // There should be at least 1 minute of remaining expiration (probably more). + require.NotNil(t, credOutput.Status.ExpirationTimestamp) + ttl := time.Until(credOutput.Status.ExpirationTimestamp.Time) + require.Greater(t, ttl.Milliseconds(), (1 * time.Minute).Milliseconds()) + + // Assert some properties about the token, which should be a valid JWT. + require.NotEmpty(t, credOutput.Status.Token) + jws, err := jose.ParseSigned(credOutput.Status.Token) + require.NoError(t, err) + claims := map[string]interface{}{} + require.NoError(t, json.Unmarshal(jws.UnsafePayloadWithoutVerification(), &claims)) + require.Equal(t, env.OIDCUpstream.Issuer, claims["iss"]) + require.Equal(t, env.OIDCUpstream.ClientID, claims["aud"]) + require.Equal(t, env.OIDCUpstream.Username, claims["email"]) + require.NotEmpty(t, claims["nonce"]) +} + +func waitForVisibleElements(t *testing.T, page *agouti.Page, selectors ...string) { + t.Helper() + require.Eventually(t, + func() bool { + for _, sel := range selectors { + vis, err := page.First(sel).Visible() + if !(err == nil && vis) { + return false + } + } + return true + }, + 30*time.Second, + 100*time.Millisecond, + ) +} + +func waitForURL(t *testing.T, page *agouti.Page, pat *regexp.Regexp) { + require.Eventually(t, func() bool { + url, err := page.URL() + return err == nil && pat.MatchString(url) + }, 30*time.Second, 100*time.Millisecond) +} diff --git a/test/library/env.go b/test/library/env.go index 82ab0b46..7858ee41 100644 --- a/test/library/env.go +++ b/test/library/env.go @@ -6,6 +6,7 @@ package library import ( "io/ioutil" "os" + "strconv" "strings" "testing" @@ -18,7 +19,8 @@ import ( type Capability string const ( - ClusterSigningKeyIsAvailable = Capability("clusterSigningKeyIsAvailable") + ClusterSigningKeyIsAvailable Capability = "clusterSigningKeyIsAvailable" + ExternalOIDCProviderIsAvailable Capability = "externalOIDCProviderIsAvailable" ) // TestEnv captures all the external parameters consumed by our integration tests. @@ -38,9 +40,17 @@ type TestEnv struct { ExpectedUsername string `json:"expectedUsername"` ExpectedGroups []string `json:"expectedGroups"` } `json:"testUser"` + + OIDCUpstream struct { + Issuer string `json:"issuer"` + ClientID string `json:"clientID"` + LocalhostPort int `json:"localhostPort"` + Username string `json:"username"` + Password string `json:"password"` + } `json:"oidcUpstream"` } -// IntegrationEnv gets the integration test environment from a Kubernetes Secret in the test cluster. This +// IntegrationEnv gets the integration test environment from OS environment variables. This // method also implies SkipUnlessIntegration(). func IntegrationEnv(t *testing.T) *TestEnv { t.Helper() @@ -79,6 +89,17 @@ func IntegrationEnv(t *testing.T) *TestEnv { result.SupervisorAppName = needEnv("PINNIPED_TEST_SUPERVISOR_APP_NAME") result.SupervisorAddress = needEnv("PINNIPED_TEST_SUPERVISOR_ADDRESS") result.TestWebhook.TLS = &idpv1alpha1.TLSSpec{CertificateAuthorityData: needEnv("PINNIPED_TEST_WEBHOOK_CA_BUNDLE")} + + result.OIDCUpstream.Issuer = os.Getenv("PINNIPED_TEST_CLI_OIDC_ISSUER") + result.OIDCUpstream.ClientID = os.Getenv("PINNIPED_TEST_CLI_OIDC_CLIENT_ID") + result.OIDCUpstream.LocalhostPort, _ = strconv.Atoi(os.Getenv("PINNIPED_TEST_CLI_OIDC_LOCALHOST_PORT")) + result.OIDCUpstream.Username = os.Getenv("PINNIPED_TEST_CLI_OIDC_USERNAME") + result.OIDCUpstream.Password = os.Getenv("PINNIPED_TEST_CLI_OIDC_PASSWORD") + + result.Capabilities[ExternalOIDCProviderIsAvailable] = !(result.OIDCUpstream.Issuer == "" || + result.OIDCUpstream.ClientID == "" || + result.OIDCUpstream.Username == "" || + result.OIDCUpstream.Password == "") result.t = t return &result } From 33fcc7441738c6c577d6215e0d58cbd198b4797a Mon Sep 17 00:00:00 2001 From: Matt Moyer Date: Tue, 13 Oct 2020 16:09:13 -0500 Subject: [PATCH 7/9] Add Dex to our integration test environment and use it to test the CLI. Signed-off-by: Matt Moyer --- CONTRIBUTING.md | 5 +- hack/lib/kind-config/multi-node.yaml | 10 ++- hack/lib/kind-config/single-node.yaml | 10 ++- hack/lib/tilt/Tiltfile | 17 +++++ hack/prepare-for-integration-tests.sh | 18 ++++- test/deploy/dex/dex.yaml | 102 ++++++++++++++++++++++++++ test/deploy/dex/values.yaml | 17 +++++ test/integration/cli_test.go | 94 ++++++++++++++++-------- test/library/env.go | 18 ++--- 9 files changed, 244 insertions(+), 47 deletions(-) create mode 100644 test/deploy/dex/dex.yaml create mode 100644 test/deploy/dex/values.yaml diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 553a8745..80f3846f 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -99,11 +99,12 @@ docker build . - [`tilt`](https://docs.tilt.dev/install.html) - [`ytt`](https://carvel.dev/#getting-started) - [`kubectl`](https://kubernetes.io/docs/tasks/tools/install-kubectl/) + - [`chromedriver`](https://chromedriver.chromium.org/) (and [Chrome](https://www.google.com/chrome/)) - On macOS, these tools can be installed with [Homebrew](https://brew.sh/): + On macOS, these tools can be installed with [Homebrew](https://brew.sh/) (assuming you have Chrome installed already): ```bash - brew install kind tilt-dev/tap/tilt k14s/tap/ytt kubectl + brew install kind tilt-dev/tap/tilt k14s/tap/ytt kubectl chromedriver ``` 1. Create a local Kubernetes cluster using `kind`: diff --git a/hack/lib/kind-config/multi-node.yaml b/hack/lib/kind-config/multi-node.yaml index b9bdaf79..2bfa337b 100644 --- a/hack/lib/kind-config/multi-node.yaml +++ b/hack/lib/kind-config/multi-node.yaml @@ -4,4 +4,12 @@ nodes: - role: control-plane - role: worker - role: worker - extraPortMappings: [{containerPort: 31234, hostPort: 12345, protocol: TCP}] + extraPortMappings: + - protocol: TCP + containerPort: 31234 + hostPort: 12345 + listenAddress: 127.0.0.1 + - protocol: TCP + containerPort: 31235 + hostPort: 12346 + listenAddress: 127.0.0.1 diff --git a/hack/lib/kind-config/single-node.yaml b/hack/lib/kind-config/single-node.yaml index d62f3a59..5fb71715 100644 --- a/hack/lib/kind-config/single-node.yaml +++ b/hack/lib/kind-config/single-node.yaml @@ -2,4 +2,12 @@ kind: Cluster apiVersion: kind.x-k8s.io/v1alpha4 nodes: - role: control-plane - extraPortMappings: [{containerPort: 31234, hostPort: 12345, protocol: TCP}] + extraPortMappings: + - protocol: TCP + containerPort: 31234 + hostPort: 12345 + listenAddress: 127.0.0.1 + - protocol: TCP + containerPort: 31235 + hostPort: 12346 + listenAddress: 127.0.0.1 diff --git a/hack/lib/tilt/Tiltfile b/hack/lib/tilt/Tiltfile index 26213238..4366db38 100644 --- a/hack/lib/tilt/Tiltfile +++ b/hack/lib/tilt/Tiltfile @@ -18,6 +18,23 @@ local_resource( deps=['../../../cmd', '../../../internal', '../../../pkg', '../../../generated'], ) +##################################################################################################### +# Dex +# + +# Render the Dex installation manifest using ytt. +k8s_yaml(local(['ytt','--file', '../../../test/deploy/dex'])) + +# Collect all the deployed Dex resources under a "dex" resource tab. +k8s_resource( + workload='dex', # this is the deployment name + objects=[ + # these are the objects that would otherwise appear in the "uncategorized" tab in the tilt UI + 'dex:namespace', + 'dex-config:configmap', + ], +) + ##################################################################################################### # Local-user-authenticator app # diff --git a/hack/prepare-for-integration-tests.sh b/hack/prepare-for-integration-tests.sh index 2037bb41..f0eb92ca 100755 --- a/hack/prepare-for-integration-tests.sh +++ b/hack/prepare-for-integration-tests.sh @@ -119,7 +119,7 @@ if ! tilt_mode; then log_note "Checking for running kind clusters..." if ! kind get clusters | grep -q -e '^pinniped$'; then log_note "Creating a kind cluster..." - # single-node.yaml exposes node port 31234 as 127.0.0.1:12345 + # single-node.yaml exposes node port 31234 as 127.0.0.1:12345 and port 31235 as 127.0.0.1:12346 kind create cluster --config "$pinniped_path/hack/lib/kind-config/single-node.yaml" --name pinniped else if ! kubectl cluster-info | grep master | grep -q 127.0.0.1; then @@ -176,6 +176,17 @@ if ! tilt_mode; then popd >/dev/null + # + # Deploy dex + # + pushd test/deploy/dex >/dev/null + + log_note "Deploying Dex to the cluster..." + ytt --file . >"$manifest" + kubectl apply --dry-run=client -f "$manifest" # Validate manifest schema. + kapp deploy --yes --app dex --diff-changes --file "$manifest" + + popd >/dev/null fi test_username="test-username" @@ -261,6 +272,11 @@ export PINNIPED_TEST_WEBHOOK_CA_BUNDLE=${webhook_ca_bundle} export PINNIPED_TEST_SUPERVISOR_NAMESPACE=${supervisor_namespace} export PINNIPED_TEST_SUPERVISOR_APP_NAME=${supervisor_app_name} export PINNIPED_TEST_SUPERVISOR_ADDRESS="127.0.0.1:12345" +export PINNIPED_TEST_CLI_OIDC_ISSUER=http://127.0.0.1:12346/dex +export PINNIPED_TEST_CLI_OIDC_CLIENT_ID=pinniped-cli +export PINNIPED_TEST_CLI_OIDC_LOCALHOST_PORT=48095 +export PINNIPED_TEST_CLI_OIDC_USERNAME=pinny@example.com +export PINNIPED_TEST_CLI_OIDC_PASSWORD=password read -r -d '' PINNIPED_TEST_CLUSTER_CAPABILITY_YAML << PINNIPED_TEST_CLUSTER_CAPABILITY_YAML_EOF || true ${pinniped_cluster_capability_file_content} diff --git a/test/deploy/dex/dex.yaml b/test/deploy/dex/dex.yaml new file mode 100644 index 00000000..dcea584f --- /dev/null +++ b/test/deploy/dex/dex.yaml @@ -0,0 +1,102 @@ +#! Copyright 2020 the Pinniped contributors. All Rights Reserved. +#! SPDX-License-Identifier: Apache-2.0 + +#@ load("@ytt:data", "data") +#@ load("@ytt:sha256", "sha256") +#@ load("@ytt:yaml", "yaml") + +#@ def dexConfig(): +issuer: #@ "http://127.0.0.1:" + str(data.values.ports.local) + "/dex" +storage: + type: sqlite3 + config: + file: ":memory:" +web: + http: 0.0.0.0:5556 +oauth2: + skipApprovalScreen: true +staticClients: +- id: pinniped-cli + name: 'Pinniped CLI' + #! we can't have "public: true" until https://github.com/dexidp/dex/pull/1822 lands in Dex. + redirectURIs: + - #@ "http://127.0.0.1:" + str(data.values.ports.cli) + "/callback" + - #@ "http://[::1]:" + str(data.values.ports.cli) + "/callback" +enablePasswordDB: true +staticPasswords: +- username: "pinny" + email: "pinny@example.com" + hash: "$2a$10$2b2cU8CPhOTaGrs1HRQuAueS7JTT5ZHsHSzYiFPm1leZck7Mc8T4W" #! bcrypt("password") + userID: "061d23d1-fe1e-4777-9ae9-59cd12abeaaa" +#@ end + +--- +apiVersion: v1 +kind: Namespace +metadata: + name: dex + labels: + name: dex +--- +apiVersion: v1 +kind: ConfigMap +metadata: + name: dex-config + namespace: dex + labels: + app: dex +data: + config.yaml: #@ yaml.encode(dexConfig()) +--- +apiVersion: apps/v1 +kind: Deployment +metadata: + name: dex + namespace: dex + labels: + app: dex +spec: + replicas: 1 + selector: + matchLabels: + app: dex + template: + metadata: + labels: + app: dex + annotations: + dexConfigHash: #@ sha256.sum(yaml.encode(dexConfig())) + spec: + containers: + - name: dex + image: quay.io/dexidp/dex:v2.10.0 + imagePullPolicy: IfNotPresent + command: + - /usr/local/bin/dex + - serve + - /etc/dex/cfg/config.yaml + ports: + - name: http + containerPort: 5556 + volumeMounts: + - name: config + mountPath: /etc/dex/cfg + volumes: + - name: config + configMap: + name: dex-config +--- +apiVersion: v1 +kind: Service +metadata: + name: dex + namespace: dex + labels: + app: dex +spec: + type: NodePort + selector: + app: dex + ports: + - port: 5556 + nodePort: #@ data.values.ports.node diff --git a/test/deploy/dex/values.yaml b/test/deploy/dex/values.yaml new file mode 100644 index 00000000..7d04cdb1 --- /dev/null +++ b/test/deploy/dex/values.yaml @@ -0,0 +1,17 @@ +#! Copyright 2020 the Pinniped contributors. All Rights Reserved. +#! SPDX-License-Identifier: Apache-2.0 + +#@data/values +--- +ports: + #! Port on which the Pinniped CLI is listening for a callback (`--listen-port` flag value) + #! Used in the Dex configuration to form the valid redirect URIs for our test client. + cli: 48095 + + #! Kubernetes NodePort that should be forwarded to the Dex service. + #! Used to create a Service of type: NodePort + node: 31235 + + #! External port where Dex ends up exposed on localhost during tests. This value comes from our + #! Kind configuration which maps 127.0.0.1:12346 to port 31235 on the Kind worker node. + local: 12346 diff --git a/test/integration/cli_test.go b/test/integration/cli_test.go index 8365d8c0..45ed4476 100644 --- a/test/integration/cli_test.go +++ b/test/integration/cli_test.go @@ -102,19 +102,58 @@ func runPinnipedCLIGetKubeconfig(t *testing.T, pinnipedExe, token, namespaceName return string(output) } -func TestCLILoginOIDC(t *testing.T) { - var ( - oktaURLPattern = regexp.MustCompile(`\Ahttps://.+.okta.com/.+\z`) - localURLPattern = regexp.MustCompile(`\Ahttp://127.0.0.1.+\z`) - ) +type loginProviderPatterns struct { + Name string + IssuerPattern *regexp.Regexp + LoginPagePattern *regexp.Regexp + UsernameSelector string + PasswordSelector string + LoginButtonSelector string +} - env := library.IntegrationEnv(t).WithCapability(library.ExternalOIDCProviderIsAvailable) +func getLoginProvider(t *testing.T) *loginProviderPatterns { + t.Helper() + issuer := library.IntegrationEnv(t).OIDCUpstream.Issuer + for _, p := range []loginProviderPatterns{ + { + Name: "Okta", + IssuerPattern: regexp.MustCompile(`\Ahttps://.+\.okta\.com/.+\z`), + LoginPagePattern: regexp.MustCompile(`\Ahttps://.+\.okta\.com/.+\z`), + UsernameSelector: "input#okta-signin-username", + PasswordSelector: "input#okta-signin-password", + LoginButtonSelector: "input#okta-signin-submit", + }, + { + Name: "Dex", + IssuerPattern: regexp.MustCompile(`\Ahttp://127\.0\.0\.1.+/dex.*\z`), + LoginPagePattern: regexp.MustCompile(`\Ahttp://127\.0\.0\.1.+/dex/auth/local.+\z`), + UsernameSelector: "input#login", + PasswordSelector: "input#password", + LoginButtonSelector: "button#submit-login", + }, + } { + if p.IssuerPattern.MatchString(issuer) { + return &p + } + } + require.Failf(t, "could not find login provider for issuer %q", issuer) + return nil +} + +func TestCLILoginOIDC(t *testing.T) { + env := library.IntegrationEnv(t) + + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) + defer cancel() + + // Find the login CSS selectors for the test issuer, or fail fast. + loginProvider := getLoginProvider(t) // Build pinniped CLI. t.Logf("building CLI binary") pinnipedExe := buildPinnipedCLI(t) - cmd := exec.Command(pinnipedExe, "alpha", "login", "oidc", + cmd := exec.CommandContext(ctx, pinnipedExe, "alpha", "login", "oidc", "--issuer", env.OIDCUpstream.Issuer, "--client-id", env.OIDCUpstream.ClientID, "--listen-port", strconv.Itoa(env.OIDCUpstream.LocalhostPort), @@ -123,7 +162,7 @@ func TestCLILoginOIDC(t *testing.T) { // Create a WaitGroup that will wait for all child goroutines to finish, so they can assert errors. var wg sync.WaitGroup - defer wg.Wait() + t.Cleanup(wg.Wait) // Start a background goroutine to read stderr from the CLI and parse out the login URL. loginURLChan := make(chan string) @@ -131,6 +170,7 @@ func TestCLILoginOIDC(t *testing.T) { require.NoError(t, err) wg.Add(1) go func() { + defer wg.Done() r := bufio.NewReader(stderr) line, err := r.ReadString('\n') require.NoError(t, err) @@ -138,9 +178,9 @@ func TestCLILoginOIDC(t *testing.T) { require.Truef(t, strings.HasPrefix(line, prompt), "expected %q to have prefix %q", line, prompt) loginURLChan <- strings.TrimPrefix(line, prompt) _, err = io.Copy(ioutil.Discard, r) + t.Logf("stderr stream closed") require.NoError(t, err) - wg.Done() }() // Start a background goroutine to read stdout from the CLI and parse out an ExecCredential. @@ -149,6 +189,7 @@ func TestCLILoginOIDC(t *testing.T) { require.NoError(t, err) wg.Add(1) go func() { + defer wg.Done() r := bufio.NewReader(stdout) var out clientauthenticationv1beta1.ExecCredential @@ -158,18 +199,15 @@ func TestCLILoginOIDC(t *testing.T) { _, err = io.Copy(ioutil.Discard, r) t.Logf("stdout stream closed") require.NoError(t, err) - wg.Done() }() t.Logf("starting CLI subprocess") require.NoError(t, cmd.Start()) - wg.Add(1) - defer func() { + t.Cleanup(func() { err := cmd.Wait() t.Logf("CLI subprocess exited") require.NoError(t, err) - wg.Done() - }() + }) // Start the browser driver. t.Logf("opening browser driver") @@ -193,26 +231,23 @@ func TestCLILoginOIDC(t *testing.T) { t.Logf("navigating to login page") require.NoError(t, page.Navigate(loginURL)) - // Expect to be redirected to the Okta login page. - t.Logf("waiting for redirect to Okta login page") - waitForURL(t, page, oktaURLPattern) + // Expect to be redirected to the login page. + t.Logf("waiting for redirect to %s login page", loginProvider.Name) + waitForURL(t, page, loginProvider.LoginPagePattern) // Wait for the login page to be rendered. - waitForVisibleElements(t, page, - "input#okta-signin-username", - "input#okta-signin-password", - "input#okta-signin-submit", - ) + waitForVisibleElements(t, page, loginProvider.UsernameSelector, loginProvider.PasswordSelector, loginProvider.LoginButtonSelector) // Fill in the username and password and click "submit". - t.Logf("logging into Okta") - require.NoError(t, page.First("input#okta-signin-username").Fill(env.OIDCUpstream.Username)) - require.NoError(t, page.First("input#okta-signin-password").Fill(env.OIDCUpstream.Password)) - require.NoError(t, page.First("input#okta-signin-submit").Click()) + t.Logf("logging into %s", loginProvider.Name) + require.NoError(t, page.First(loginProvider.UsernameSelector).Fill(env.OIDCUpstream.Username)) + require.NoError(t, page.First(loginProvider.PasswordSelector).Fill(env.OIDCUpstream.Password)) + require.NoError(t, page.First(loginProvider.LoginButtonSelector).Click()) // Wait for the login to happen and us be redirected back to a localhost callback. t.Logf("waiting for redirect to localhost callback") - waitForURL(t, page, localURLPattern) + callbackURLPattern := regexp.MustCompile(`\Ahttp://127.0.0.1:` + strconv.Itoa(env.OIDCUpstream.LocalhostPort) + `/.+\z`) + waitForURL(t, page, callbackURLPattern) // Wait for the "pre" element that gets rendered for a `text/plain` page, and // assert that it contains the success message. @@ -221,7 +256,6 @@ func TestCLILoginOIDC(t *testing.T) { msg, err := page.First("pre").Text() require.NoError(t, err) require.Equal(t, "you have been logged in and may now close this tab", msg) - require.NoError(t, page.CloseWindow()) // Expect the CLI to output an ExecCredential in JSON format. t.Logf("waiting for CLI to output ExecCredential JSON") @@ -267,7 +301,7 @@ func waitForVisibleElements(t *testing.T, page *agouti.Page, selectors ...string } return true }, - 30*time.Second, + 10*time.Second, 100*time.Millisecond, ) } @@ -276,5 +310,5 @@ func waitForURL(t *testing.T, page *agouti.Page, pat *regexp.Regexp) { require.Eventually(t, func() bool { url, err := page.URL() return err == nil && pat.MatchString(url) - }, 30*time.Second, 100*time.Millisecond) + }, 10*time.Second, 100*time.Millisecond) } diff --git a/test/library/env.go b/test/library/env.go index 7858ee41..eba2a0d1 100644 --- a/test/library/env.go +++ b/test/library/env.go @@ -19,8 +19,7 @@ import ( type Capability string const ( - ClusterSigningKeyIsAvailable Capability = "clusterSigningKeyIsAvailable" - ExternalOIDCProviderIsAvailable Capability = "externalOIDCProviderIsAvailable" + ClusterSigningKeyIsAvailable Capability = "clusterSigningKeyIsAvailable" ) // TestEnv captures all the external parameters consumed by our integration tests. @@ -90,16 +89,11 @@ func IntegrationEnv(t *testing.T) *TestEnv { result.SupervisorAddress = needEnv("PINNIPED_TEST_SUPERVISOR_ADDRESS") result.TestWebhook.TLS = &idpv1alpha1.TLSSpec{CertificateAuthorityData: needEnv("PINNIPED_TEST_WEBHOOK_CA_BUNDLE")} - result.OIDCUpstream.Issuer = os.Getenv("PINNIPED_TEST_CLI_OIDC_ISSUER") - result.OIDCUpstream.ClientID = os.Getenv("PINNIPED_TEST_CLI_OIDC_CLIENT_ID") - result.OIDCUpstream.LocalhostPort, _ = strconv.Atoi(os.Getenv("PINNIPED_TEST_CLI_OIDC_LOCALHOST_PORT")) - result.OIDCUpstream.Username = os.Getenv("PINNIPED_TEST_CLI_OIDC_USERNAME") - result.OIDCUpstream.Password = os.Getenv("PINNIPED_TEST_CLI_OIDC_PASSWORD") - - result.Capabilities[ExternalOIDCProviderIsAvailable] = !(result.OIDCUpstream.Issuer == "" || - result.OIDCUpstream.ClientID == "" || - result.OIDCUpstream.Username == "" || - result.OIDCUpstream.Password == "") + result.OIDCUpstream.Issuer = needEnv("PINNIPED_TEST_CLI_OIDC_ISSUER") + result.OIDCUpstream.ClientID = needEnv("PINNIPED_TEST_CLI_OIDC_CLIENT_ID") + result.OIDCUpstream.LocalhostPort, _ = strconv.Atoi(needEnv("PINNIPED_TEST_CLI_OIDC_LOCALHOST_PORT")) + result.OIDCUpstream.Username = needEnv("PINNIPED_TEST_CLI_OIDC_USERNAME") + result.OIDCUpstream.Password = needEnv("PINNIPED_TEST_CLI_OIDC_PASSWORD") result.t = t return &result } From 19a1d569c9598babf4b9f83a0d2368c779a53a9c Mon Sep 17 00:00:00 2001 From: Matt Moyer Date: Wed, 14 Oct 2020 12:28:08 -0500 Subject: [PATCH 8/9] Restructure this test to avoid data races. Signed-off-by: Matt Moyer --- go.mod | 1 + test/integration/cli_test.go | 118 ++++++++++++++++++++++------------- 2 files changed, 77 insertions(+), 42 deletions(-) diff --git a/go.mod b/go.mod index 0b476b08..6adfc17b 100644 --- a/go.mod +++ b/go.mod @@ -23,6 +23,7 @@ require ( go.pinniped.dev/generated/1.19/client v0.0.0-00010101000000-000000000000 golang.org/x/crypto v0.0.0-20200622213623-75b288015ac9 golang.org/x/oauth2 v0.0.0-20191202225959-858c2ad4c8b6 + golang.org/x/sync v0.0.0-20200625203802-6e8e738ad208 gopkg.in/square/go-jose.v2 v2.2.2 k8s.io/api v0.19.2 k8s.io/apimachinery v0.19.2 diff --git a/test/integration/cli_test.go b/test/integration/cli_test.go index 45ed4476..4ba66899 100644 --- a/test/integration/cli_test.go +++ b/test/integration/cli_test.go @@ -4,8 +4,11 @@ package integration import ( "bufio" + "bytes" "context" "encoding/json" + "errors" + "fmt" "io" "io/ioutil" "os" @@ -14,12 +17,12 @@ import ( "regexp" "strconv" "strings" - "sync" "testing" "time" "github.com/sclevine/agouti" "github.com/stretchr/testify/require" + "golang.org/x/sync/errgroup" "gopkg.in/square/go-jose.v2" clientauthenticationv1beta1 "k8s.io/client-go/pkg/apis/clientauthentication/v1beta1" @@ -153,60 +156,70 @@ func TestCLILoginOIDC(t *testing.T) { t.Logf("building CLI binary") pinnipedExe := buildPinnipedCLI(t) + // Start the CLI running the "alpha login oidc [...]" command with stdout/stderr connected to pipes. + t.Logf("starting CLI subprocess") cmd := exec.CommandContext(ctx, pinnipedExe, "alpha", "login", "oidc", "--issuer", env.OIDCUpstream.Issuer, "--client-id", env.OIDCUpstream.ClientID, "--listen-port", strconv.Itoa(env.OIDCUpstream.LocalhostPort), "--skip-browser", ) - - // Create a WaitGroup that will wait for all child goroutines to finish, so they can assert errors. - var wg sync.WaitGroup - t.Cleanup(wg.Wait) - - // Start a background goroutine to read stderr from the CLI and parse out the login URL. - loginURLChan := make(chan string) stderr, err := cmd.StderrPipe() require.NoError(t, err) - wg.Add(1) - go func() { - defer wg.Done() - r := bufio.NewReader(stderr) - line, err := r.ReadString('\n') - require.NoError(t, err) - const prompt = "Please log in: " - require.Truef(t, strings.HasPrefix(line, prompt), "expected %q to have prefix %q", line, prompt) - loginURLChan <- strings.TrimPrefix(line, prompt) - _, err = io.Copy(ioutil.Discard, r) - - t.Logf("stderr stream closed") - require.NoError(t, err) - }() - - // Start a background goroutine to read stdout from the CLI and parse out an ExecCredential. - credOutputChan := make(chan clientauthenticationv1beta1.ExecCredential) stdout, err := cmd.StdoutPipe() require.NoError(t, err) - wg.Add(1) - go func() { - defer wg.Done() - r := bufio.NewReader(stdout) - - var out clientauthenticationv1beta1.ExecCredential - require.NoError(t, json.NewDecoder(r).Decode(&out)) - credOutputChan <- out - - _, err = io.Copy(ioutil.Discard, r) - t.Logf("stdout stream closed") - require.NoError(t, err) - }() - - t.Logf("starting CLI subprocess") require.NoError(t, cmd.Start()) t.Cleanup(func() { err := cmd.Wait() - t.Logf("CLI subprocess exited") - require.NoError(t, err) + t.Logf("CLI subprocess exited with code %d", cmd.ProcessState.ExitCode()) + require.NoErrorf(t, err, "CLI process did not exit cleanly") + }) + + // Start a background goroutine to read stderr from the CLI and parse out the login URL. + loginURLChan := make(chan string) + spawnTestGoroutine(t, func() (err error) { + defer func() { + closeErr := stderr.Close() + if closeErr == nil || errors.Is(closeErr, os.ErrClosed) { + return + } + if err == nil { + err = fmt.Errorf("stderr stream closed with error: %w", closeErr) + } + }() + + reader := bufio.NewReader(stderr) + line, err := reader.ReadString('\n') + if err != nil { + return fmt.Errorf("could not read login URL line from stderr: %w", err) + } + const prompt = "Please log in: " + if !strings.HasPrefix(line, prompt) { + return fmt.Errorf("expected %q to have prefix %q", line, prompt) + } + loginURLChan <- strings.TrimPrefix(line, prompt) + return readAndExpectEmpty(reader) + }) + + // Start a background goroutine to read stdout from the CLI and parse out an ExecCredential. + credOutputChan := make(chan clientauthenticationv1beta1.ExecCredential) + spawnTestGoroutine(t, func() (err error) { + defer func() { + closeErr := stderr.Close() + if closeErr == nil || errors.Is(closeErr, os.ErrClosed) { + return + } + if err == nil { + err = fmt.Errorf("stdout stream closed with error: %w", closeErr) + } + }() + reader := bufio.NewReader(stdout) + var out clientauthenticationv1beta1.ExecCredential + if err := json.NewDecoder(reader).Decode(&out); err != nil { + return fmt.Errorf("could not read ExecCredential from stdout: %w", err) + } + credOutputChan <- out + return readAndExpectEmpty(reader) }) // Start the browser driver. @@ -312,3 +325,24 @@ func waitForURL(t *testing.T, page *agouti.Page, pat *regexp.Regexp) { return err == nil && pat.MatchString(url) }, 10*time.Second, 100*time.Millisecond) } + +func readAndExpectEmpty(r io.Reader) (err error) { + var remainder bytes.Buffer + _, err = io.Copy(&remainder, r) + if err != nil { + return err + } + if r := remainder.String(); r != "" { + return fmt.Errorf("expected remainder to be empty, but got %q", r) + } + return nil +} + +func spawnTestGoroutine(t *testing.T, f func() error) { + t.Helper() + var eg errgroup.Group + t.Cleanup(func() { + require.NoError(t, eg.Wait(), "background goroutine failed") + }) + eg.Go(f) +} From 68d20298f2b4a4b754f1d07080fce7a332d0b5cc Mon Sep 17 00:00:00 2001 From: Matt Moyer Date: Wed, 14 Oct 2020 12:33:52 -0500 Subject: [PATCH 9/9] Fix chromedriver usage inside our test container. Signed-off-by: Matt Moyer --- test/integration/cli_test.go | 27 ++++++++++++++++----------- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/test/integration/cli_test.go b/test/integration/cli_test.go index 4ba66899..334640ce 100644 --- a/test/integration/cli_test.go +++ b/test/integration/cli_test.go @@ -152,6 +152,22 @@ func TestCLILoginOIDC(t *testing.T) { // Find the login CSS selectors for the test issuer, or fail fast. loginProvider := getLoginProvider(t) + // Start the browser driver. + t.Logf("opening browser driver") + agoutiDriver := agouti.ChromeDriver( + agouti.ChromeOptions("args", []string{ + "--no-sandbox", + "--headless", // Comment out this line to see the tests happen in a visible browser window. + }), + // Uncomment this to see stdout/stderr from chromedriver. + // agouti.Debug, + ) + require.NoError(t, agoutiDriver.Start()) + t.Cleanup(func() { require.NoError(t, agoutiDriver.Stop()) }) + page, err := agoutiDriver.NewPage(agouti.Browser("chrome")) + require.NoError(t, err) + require.NoError(t, page.Reset()) + // Build pinniped CLI. t.Logf("building CLI binary") pinnipedExe := buildPinnipedCLI(t) @@ -222,17 +238,6 @@ func TestCLILoginOIDC(t *testing.T) { return readAndExpectEmpty(reader) }) - // Start the browser driver. - t.Logf("opening browser driver") - agoutiDriver := agouti.ChromeDriver( - // Comment out this line to see the tests happen in a visible browser window. - agouti.ChromeOptions("args", []string{"--headless"}), - ) - require.NoError(t, agoutiDriver.Start()) - t.Cleanup(func() { require.NoError(t, agoutiDriver.Stop()) }) - page, err := agoutiDriver.NewPage(agouti.Browser("chrome")) - require.NoError(t, err) - // Wait for the CLI to print out the login URL and open the browser to it. t.Logf("waiting for CLI to output login URL") var loginURL string