From 4395d5a0ca93090908afb3578cdfedb392c348e3 Mon Sep 17 00:00:00 2001 From: Matt Moyer Date: Thu, 10 Dec 2020 09:46:07 -0600 Subject: [PATCH 1/3] Add a default --client-id in `pinniped login oidc` command. This default matches the static client we have defined in the supervisor, which will be the correct value in most cases. Signed-off-by: Matt Moyer --- cmd/pinniped/cmd/login_oidc.go | 4 ++-- cmd/pinniped/cmd/login_oidc_test.go | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/cmd/pinniped/cmd/login_oidc.go b/cmd/pinniped/cmd/login_oidc.go index 9070adf2..4bc3a203 100644 --- a/cmd/pinniped/cmd/login_oidc.go +++ b/cmd/pinniped/cmd/login_oidc.go @@ -48,7 +48,7 @@ func oidcLoginCommand(loginFunc func(issuer string, clientID string, opts ...oid requestAudience string ) cmd.Flags().StringVar(&issuer, "issuer", "", "OpenID Connect issuer URL.") - cmd.Flags().StringVar(&clientID, "client-id", "", "OpenID Connect client ID.") + cmd.Flags().StringVar(&clientID, "client-id", "pinniped-cli", "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{oidc.ScopeOfflineAccess, oidc.ScopeOpenID}, "OIDC scopes to request during login.") cmd.Flags().BoolVar(&skipBrowser, "skip-browser", false, "Skip opening the browser (just print the URL).") @@ -57,7 +57,7 @@ func oidcLoginCommand(loginFunc func(issuer string, clientID string, opts ...oid cmd.Flags().BoolVar(&debugSessionCache, "debug-session-cache", false, "Print debug logs related to the session cache.") cmd.Flags().StringVar(&requestAudience, "request-audience", "", "Request a token with an alternate audience using RF8693 token exchange.") mustMarkHidden(&cmd, "debug-session-cache") - mustMarkRequired(&cmd, "issuer", "client-id") + mustMarkRequired(&cmd, "issuer") cmd.RunE = func(cmd *cobra.Command, args []string) error { // Initialize the session cache. diff --git a/cmd/pinniped/cmd/login_oidc_test.go b/cmd/pinniped/cmd/login_oidc_test.go index b4ee5fde..9a23e9bf 100644 --- a/cmd/pinniped/cmd/login_oidc_test.go +++ b/cmd/pinniped/cmd/login_oidc_test.go @@ -42,7 +42,7 @@ func TestLoginOIDCCommand(t *testing.T) { Flags: --ca-bundle strings Path to TLS certificate authority bundle (PEM format, optional, can be repeated). - --client-id string OpenID Connect client ID. + --client-id string OpenID Connect client ID. (default "pinniped-cli") -h, --help help for oidc --issuer string OpenID Connect issuer URL. --listen-port uint16 TCP port for localhost listener (authorization code flow only). @@ -57,7 +57,7 @@ func TestLoginOIDCCommand(t *testing.T) { args: []string{}, wantError: true, wantStdout: here.Doc(` - Error: required flag(s) "client-id", "issuer" not set + Error: required flag(s) "issuer" not set `), }, { From 9d3c98232bef02c2d6da8366722fe3f7be7e957e Mon Sep 17 00:00:00 2001 From: Matt Moyer Date: Thu, 10 Dec 2020 10:09:42 -0600 Subject: [PATCH 2/3] Fix bug in handling response content-type in oidcclient. Before this, we weren't properly parsing the `Content-Type` header. This breaks in integration with the Supervisor since it sends an extra encoding parameter like `application/json;charset=UTF-8`. This change switches to properly parsing with the `mime.ParseMediaType` function, and adds test cases to match the supervisor behavior. Signed-off-by: Matt Moyer --- pkg/oidcclient/login.go | 9 +++++++-- pkg/oidcclient/login_test.go | 28 +++++++++++++++++++++++++++- 2 files changed, 34 insertions(+), 3 deletions(-) diff --git a/pkg/oidcclient/login.go b/pkg/oidcclient/login.go index 98146fa5..4d92a452 100644 --- a/pkg/oidcclient/login.go +++ b/pkg/oidcclient/login.go @@ -8,6 +8,7 @@ import ( "context" "encoding/json" "fmt" + "mime" "net" "net/http" "net/url" @@ -364,8 +365,12 @@ func (h *handlerState) tokenExchangeRFC8693(baseToken *oidctypes.Token) (*oidcty if resp.StatusCode != http.StatusOK { return nil, fmt.Errorf("unexpected HTTP response status %d", resp.StatusCode) } - if contentType := resp.Header.Get("content-type"); contentType != "application/json" { - return nil, fmt.Errorf("unexpected HTTP response content type %q", contentType) + mediaType, _, err := mime.ParseMediaType(resp.Header.Get("content-type")) + if err != nil { + return nil, fmt.Errorf("failed to decode content-type header: %w", err) + } + if mediaType != "application/json" { + return nil, fmt.Errorf("unexpected HTTP response content type %q", mediaType) } // Decode the JSON response body. diff --git a/pkg/oidcclient/login_test.go b/pkg/oidcclient/login_test.go index 10daf6ab..cc0edea5 100644 --- a/pkg/oidcclient/login_test.go +++ b/pkg/oidcclient/login_test.go @@ -164,11 +164,14 @@ func TestLogin(t *testing.T) { case "test-audience-produce-http-400": http.Error(w, "some server error", http.StatusBadRequest) return + case "test-audience-produce-invalid-content-type": + w.Header().Set("content-type", "invalid/invalid;=") + return case "test-audience-produce-wrong-content-type": w.Header().Set("content-type", "invalid") return case "test-audience-produce-invalid-json": - w.Header().Set("content-type", "application/json") + w.Header().Set("content-type", "application/json;charset=UTF-8") _, _ = w.Write([]byte(`{`)) return case "test-audience-produce-invalid-tokentype": @@ -601,6 +604,29 @@ func TestLogin(t *testing.T) { }, wantErr: `failed to exchange token: unexpected HTTP response status 400`, }, + { + name: "with requested audience, session cache hit with valid token, but token exchange request returns invalid content-type header", + issuer: successServer.URL, + clientID: "test-client-id", + opt: func(t *testing.T) Option { + return func(h *handlerState) error { + cache := &mockSessionCache{t: t, getReturnsToken: &testToken} + t.Cleanup(func() { + require.Equal(t, []SessionCacheKey{{ + Issuer: successServer.URL, + ClientID: "test-client-id", + Scopes: []string{"test-scope"}, + RedirectURI: "http://localhost:0/callback", + }}, cache.sawGetKeys) + require.Empty(t, cache.sawPutTokens) + }) + require.NoError(t, WithSessionCache(cache)(h)) + require.NoError(t, WithRequestAudience("test-audience-produce-invalid-content-type")(h)) + return nil + } + }, + wantErr: `failed to exchange token: failed to decode content-type header: mime: invalid media parameter`, + }, { name: "with requested audience, session cache hit with valid token, but token exchange request returns wrong content-type", issuer: successServer.URL, From e7338da3dcad8fc68bc1843bf72c9fb1256f6a08 Mon Sep 17 00:00:00 2001 From: Matt Moyer Date: Thu, 10 Dec 2020 10:33:43 -0600 Subject: [PATCH 3/3] Tweak default CLI `--scopes` parameter to match supervisor use case. This should be a better default for most cases. Signed-off-by: Matt Moyer --- cmd/pinniped/cmd/login_oidc.go | 2 +- cmd/pinniped/cmd/login_oidc_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/cmd/pinniped/cmd/login_oidc.go b/cmd/pinniped/cmd/login_oidc.go index 4bc3a203..ee74238a 100644 --- a/cmd/pinniped/cmd/login_oidc.go +++ b/cmd/pinniped/cmd/login_oidc.go @@ -50,7 +50,7 @@ func oidcLoginCommand(loginFunc func(issuer string, clientID string, opts ...oid cmd.Flags().StringVar(&issuer, "issuer", "", "OpenID Connect issuer URL.") cmd.Flags().StringVar(&clientID, "client-id", "pinniped-cli", "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{oidc.ScopeOfflineAccess, oidc.ScopeOpenID}, "OIDC scopes to request during login.") + cmd.Flags().StringSliceVar(&scopes, "scopes", []string{oidc.ScopeOfflineAccess, oidc.ScopeOpenID, "pinniped.sts.unrestricted"}, "OIDC scopes to request during login.") cmd.Flags().BoolVar(&skipBrowser, "skip-browser", false, "Skip opening the browser (just print the URL).") cmd.Flags().StringVar(&sessionCachePath, "session-cache", filepath.Join(mustGetConfigDir(), "sessions.yaml"), "Path to session cache file.") cmd.Flags().StringSliceVar(&caBundlePaths, "ca-bundle", nil, "Path to TLS certificate authority bundle (PEM format, optional, can be repeated).") diff --git a/cmd/pinniped/cmd/login_oidc_test.go b/cmd/pinniped/cmd/login_oidc_test.go index 9a23e9bf..f42a2224 100644 --- a/cmd/pinniped/cmd/login_oidc_test.go +++ b/cmd/pinniped/cmd/login_oidc_test.go @@ -47,7 +47,7 @@ func TestLoginOIDCCommand(t *testing.T) { --issuer string OpenID Connect issuer URL. --listen-port uint16 TCP port for localhost listener (authorization code flow only). --request-audience string Request a token with an alternate audience using RF8693 token exchange. - --scopes strings OIDC scopes to request during login. (default [offline_access,openid]) + --scopes strings OIDC scopes to request during login. (default [offline_access,openid,pinniped.sts.unrestricted]) --session-cache string Path to session cache file. (default "` + cfgDir + `/sessions.yaml") --skip-browser Skip opening the browser (just print the URL). `),