From 34509e74305e7779e5064c1e70f83888f77ca5d6 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Wed, 20 Jul 2022 13:55:56 -0700 Subject: [PATCH] Add more unit tests for dynamic clients and enhance token exchange - Enhance the token exchange to check that the same client is used compared to the client used during the original authorization and token requests, and also check that the client has the token-exchange grant type allowed in its configuration. - Reduce the minimum required bcrypt cost for OIDCClient secrets because 15 is too slow for real-life use, especially considering that every login and every refresh flow will require two client auths. - In unit tests, use bcrypt hashes with a cost of 4, because bcrypt slows down by 13x when run with the race detector, and we run our tests with the race detector enabled, causing the tests to be unacceptably slow. The production code uses a higher minimum cost. - Centralize all pre-computed bcrypt hashes used by unit tests to a single place. Also extract some other useful test helpers for unit tests related to OIDCClients. - Add tons of unit tests for the token endpoint related to dynamic clients for authcode exchanges, token exchanges, and refreshes. --- .../oidcclientwatcher/oidc_client_watcher.go | 2 +- .../oidc_client_watcher_test.go | 57 +- internal/oidc/auth/auth_handler_test.go | 93 +-- .../oidc/callback/callback_handler_test.go | 28 +- .../oidc/clientregistry/clientregistry.go | 5 +- .../clientregistry/clientregistry_test.go | 20 +- internal/oidc/kube_storage.go | 9 +- .../oidc/login/post_login_handler_test.go | 22 +- internal/oidc/nullstorage.go | 8 +- .../oidcclientvalidator.go | 14 +- internal/oidc/provider/manager/manager.go | 5 +- internal/oidc/token/token_handler_test.go | 763 ++++++++++++++++-- internal/oidc/token_exchange.go | 22 +- internal/testutil/assertions.go | 18 + internal/testutil/oidcclient.go | 67 ++ internal/testutil/oidcclient_test.go | 61 ++ test/integration/supervisor_login_test.go | 5 +- .../supervisor_oidc_client_test.go | 2 +- test/integration/supervisor_warnings_test.go | 5 +- 19 files changed, 1007 insertions(+), 199 deletions(-) create mode 100644 internal/testutil/oidcclient.go create mode 100644 internal/testutil/oidcclient_test.go diff --git a/internal/controller/supervisorconfig/oidcclientwatcher/oidc_client_watcher.go b/internal/controller/supervisorconfig/oidcclientwatcher/oidc_client_watcher.go index 12123731..041e5c94 100644 --- a/internal/controller/supervisorconfig/oidcclientwatcher/oidc_client_watcher.go +++ b/internal/controller/supervisorconfig/oidcclientwatcher/oidc_client_watcher.go @@ -107,7 +107,7 @@ func (c *oidcClientWatcherController) Sync(ctx controllerlib.Context) error { secret = nil } - _, conditions, clientSecrets := oidcclientvalidator.Validate(oidcClient, secret) + _, conditions, clientSecrets := oidcclientvalidator.Validate(oidcClient, secret, oidcclientvalidator.DefaultMinBcryptCost) if err := c.updateStatus(ctx.Context, oidcClient, conditions, len(clientSecrets)); err != nil { return fmt.Errorf("cannot update OIDCClient '%s/%s': %w", oidcClient.Namespace, oidcClient.Name, err) diff --git a/internal/controller/supervisorconfig/oidcclientwatcher/oidc_client_watcher_test.go b/internal/controller/supervisorconfig/oidcclientwatcher/oidc_client_watcher_test.go index b1d147fe..05ea4fd8 100644 --- a/internal/controller/supervisorconfig/oidcclientwatcher/oidc_client_watcher_test.go +++ b/internal/controller/supervisorconfig/oidcclientwatcher/oidc_client_watcher_test.go @@ -164,15 +164,6 @@ func TestOIDCClientWatcherControllerSync(t *testing.T) { testName = "client.oauth.pinniped.dev-test-name" testNamespace = "test-namespace" testUID = "test-uid-123" - - //nolint:gosec // this is not a credential - testBcryptSecret1 = "$2y$15$Kh7cRj0ScSD5QelE3ZNSl.nF04JDv7zb3SgGN.tSfLIX.4kt3UX7m" // bcrypt of "password1" at cost 15 - //nolint:gosec // this is not a credential - testBcryptSecret2 = "$2y$15$Kh7cRj0ScSD5QelE3ZNSl.nF04JDv7zb3SgGN.tSfLIX.4kt3UX7m" // bcrypt of "password2" at cost 15 - //nolint:gosec // this is not a credential - testInvalidBcryptSecretCostTooLow = "$2y$14$njwk1cItiRy6cb6u9aiJLuhtJG83zM9111t.xU6MxvnqqYbkXxzwy" // bcrypt of "password1" at cost 14 - //nolint:gosec // this is not a credential - testInvalidBcryptSecretInvalidFormat = "$2y$14$njwk1cItiRy6cb6u9aiJLuhtJG83zM9111t.xU6MxvnqqYbkXxz" // not enough characters in hash value ) now := metav1.NewTime(time.Now().UTC()) @@ -291,7 +282,7 @@ func TestOIDCClientWatcherControllerSync(t *testing.T) { }, }, }, - inputSecrets: []runtime.Object{testutil.OIDCClientSecretStorageSecretForUID(t, testNamespace, testUID, []string{testBcryptSecret1})}, + inputSecrets: []runtime.Object{testutil.OIDCClientSecretStorageSecretForUID(t, testNamespace, testUID, []string{testutil.HashedPassword1AtSupervisorMinCost})}, wantAPIActions: 1, // one update wantResultingOIDCClients: []configv1alpha1.OIDCClient{ { @@ -320,7 +311,7 @@ func TestOIDCClientWatcherControllerSync(t *testing.T) { AllowedScopes: []configv1alpha1.Scope{"openid"}, }, }}, - inputSecrets: []runtime.Object{testutil.OIDCClientSecretStorageSecretForUID(t, testNamespace, testUID, []string{testBcryptSecret1, testBcryptSecret2})}, + inputSecrets: []runtime.Object{testutil.OIDCClientSecretStorageSecretForUID(t, testNamespace, testUID, []string{testutil.HashedPassword1AtSupervisorMinCost, testutil.HashedPassword2AtSupervisorMinCost})}, wantAPIActions: 1, // one update wantResultingOIDCClients: []configv1alpha1.OIDCClient{{ ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234, UID: testUID}, @@ -353,7 +344,7 @@ func TestOIDCClientWatcherControllerSync(t *testing.T) { TotalClientSecrets: 1, }, }}, - inputSecrets: []runtime.Object{testutil.OIDCClientSecretStorageSecretForUID(t, testNamespace, testUID, []string{testBcryptSecret1})}, + inputSecrets: []runtime.Object{testutil.OIDCClientSecretStorageSecretForUID(t, testNamespace, testUID, []string{testutil.HashedPassword1AtSupervisorMinCost})}, wantAPIActions: 0, // no updates wantResultingOIDCClients: []configv1alpha1.OIDCClient{{ ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234, UID: testUID}, @@ -445,7 +436,7 @@ func TestOIDCClientWatcherControllerSync(t *testing.T) { }}, inputSecrets: []runtime.Object{ testutil.OIDCClientSecretStorageSecretForUID(t, testNamespace, testUID, - []string{testBcryptSecret1, testInvalidBcryptSecretCostTooLow, testInvalidBcryptSecretInvalidFormat}), + []string{testutil.HashedPassword1AtSupervisorMinCost, testutil.HashedPassword1JustBelowSupervisorMinCost, testutil.HashedPassword1InvalidFormat}), }, wantAPIActions: 1, // one update wantResultingOIDCClients: []configv1alpha1.OIDCClient{{ @@ -457,7 +448,7 @@ func TestOIDCClientWatcherControllerSync(t *testing.T) { happyAllowedScopesCondition(now, 1234), sadInvalidClientSecretsCondition(now, 1234, "3 stored client secrets found, but some were invalid, so none will be used: "+ - "hashed client secret at index 1: bcrypt cost 14 is below the required minimum of 15; "+ + "hashed client secret at index 1: bcrypt cost 11 is below the required minimum of 12; "+ "hashed client secret at index 2: crypto/bcrypt: hashedSecret too short to be a bcrypted password"), }, TotalClientSecrets: 0, @@ -479,7 +470,7 @@ func TestOIDCClientWatcherControllerSync(t *testing.T) { Spec: configv1alpha1.OIDCClientSpec{}, }, }, - inputSecrets: []runtime.Object{testutil.OIDCClientSecretStorageSecretForUID(t, testNamespace, "uid1", []string{testBcryptSecret1})}, + inputSecrets: []runtime.Object{testutil.OIDCClientSecretStorageSecretForUID(t, testNamespace, "uid1", []string{testutil.HashedPassword1AtSupervisorMinCost})}, wantAPIActions: 2, // one update for each OIDCClient wantResultingOIDCClients: []configv1alpha1.OIDCClient{ { @@ -527,7 +518,7 @@ func TestOIDCClientWatcherControllerSync(t *testing.T) { TotalClientSecrets: 1, }, }}, - inputSecrets: []runtime.Object{testutil.OIDCClientSecretStorageSecretForUID(t, testNamespace, testUID, []string{testBcryptSecret1})}, + inputSecrets: []runtime.Object{testutil.OIDCClientSecretStorageSecretForUID(t, testNamespace, testUID, []string{testutil.HashedPassword1AtSupervisorMinCost})}, wantAPIActions: 1, // one update wantResultingOIDCClients: []configv1alpha1.OIDCClient{{ ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 4567, UID: testUID}, @@ -553,7 +544,7 @@ func TestOIDCClientWatcherControllerSync(t *testing.T) { }, }}, wantAPIActions: 1, // one update - inputSecrets: []runtime.Object{testutil.OIDCClientSecretStorageSecretForUID(t, testNamespace, testUID, []string{testBcryptSecret1})}, + inputSecrets: []runtime.Object{testutil.OIDCClientSecretStorageSecretForUID(t, testNamespace, testUID, []string{testutil.HashedPassword1AtSupervisorMinCost})}, wantResultingOIDCClients: []configv1alpha1.OIDCClient{{ ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234, UID: testUID}, Status: configv1alpha1.OIDCClientStatus{ @@ -577,7 +568,7 @@ func TestOIDCClientWatcherControllerSync(t *testing.T) { }, }}, wantAPIActions: 1, // one update - inputSecrets: []runtime.Object{testutil.OIDCClientSecretStorageSecretForUID(t, testNamespace, testUID, []string{testBcryptSecret1})}, + inputSecrets: []runtime.Object{testutil.OIDCClientSecretStorageSecretForUID(t, testNamespace, testUID, []string{testutil.HashedPassword1AtSupervisorMinCost})}, wantResultingOIDCClients: []configv1alpha1.OIDCClient{{ ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234, UID: testUID}, Status: configv1alpha1.OIDCClientStatus{ @@ -606,7 +597,7 @@ func TestOIDCClientWatcherControllerSync(t *testing.T) { }, }}, wantAPIActions: 1, // one update - inputSecrets: []runtime.Object{testutil.OIDCClientSecretStorageSecretForUID(t, testNamespace, testUID, []string{testBcryptSecret1})}, + inputSecrets: []runtime.Object{testutil.OIDCClientSecretStorageSecretForUID(t, testNamespace, testUID, []string{testutil.HashedPassword1AtSupervisorMinCost})}, wantResultingOIDCClients: []configv1alpha1.OIDCClient{{ ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234, UID: testUID}, Status: configv1alpha1.OIDCClientStatus{ @@ -633,7 +624,7 @@ func TestOIDCClientWatcherControllerSync(t *testing.T) { AllowedScopes: []configv1alpha1.Scope{"openid", "pinniped:request-audience", "username", "groups"}, }, }}, - inputSecrets: []runtime.Object{testutil.OIDCClientSecretStorageSecretForUID(t, testNamespace, testUID, []string{testBcryptSecret1})}, + inputSecrets: []runtime.Object{testutil.OIDCClientSecretStorageSecretForUID(t, testNamespace, testUID, []string{testutil.HashedPassword1AtSupervisorMinCost})}, wantAPIActions: 1, // one update wantResultingOIDCClients: []configv1alpha1.OIDCClient{{ ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234, UID: testUID}, @@ -657,7 +648,7 @@ func TestOIDCClientWatcherControllerSync(t *testing.T) { AllowedScopes: []configv1alpha1.Scope{"openid"}, }, }}, - inputSecrets: []runtime.Object{testutil.OIDCClientSecretStorageSecretForUID(t, testNamespace, testUID, []string{testBcryptSecret1})}, + inputSecrets: []runtime.Object{testutil.OIDCClientSecretStorageSecretForUID(t, testNamespace, testUID, []string{testutil.HashedPassword1AtSupervisorMinCost})}, wantAPIActions: 1, // one update wantResultingOIDCClients: []configv1alpha1.OIDCClient{{ ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234, UID: testUID}, @@ -681,7 +672,7 @@ func TestOIDCClientWatcherControllerSync(t *testing.T) { AllowedScopes: []configv1alpha1.Scope{"openid", "pinniped:request-audience"}, }, }}, - inputSecrets: []runtime.Object{testutil.OIDCClientSecretStorageSecretForUID(t, testNamespace, testUID, []string{testBcryptSecret1})}, + inputSecrets: []runtime.Object{testutil.OIDCClientSecretStorageSecretForUID(t, testNamespace, testUID, []string{testutil.HashedPassword1AtSupervisorMinCost})}, wantAPIActions: 1, // one update wantResultingOIDCClients: []configv1alpha1.OIDCClient{{ ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234, UID: testUID}, @@ -705,7 +696,7 @@ func TestOIDCClientWatcherControllerSync(t *testing.T) { AllowedScopes: []configv1alpha1.Scope{"openid", "pinniped:request-audience", "groups"}, }, }}, - inputSecrets: []runtime.Object{testutil.OIDCClientSecretStorageSecretForUID(t, testNamespace, testUID, []string{testBcryptSecret1})}, + inputSecrets: []runtime.Object{testutil.OIDCClientSecretStorageSecretForUID(t, testNamespace, testUID, []string{testutil.HashedPassword1AtSupervisorMinCost})}, wantAPIActions: 1, // one update wantResultingOIDCClients: []configv1alpha1.OIDCClient{{ ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234, UID: testUID}, @@ -729,7 +720,7 @@ func TestOIDCClientWatcherControllerSync(t *testing.T) { AllowedScopes: []configv1alpha1.Scope{"openid", "pinniped:request-audience", "username"}, }, }}, - inputSecrets: []runtime.Object{testutil.OIDCClientSecretStorageSecretForUID(t, testNamespace, testUID, []string{testBcryptSecret1})}, + inputSecrets: []runtime.Object{testutil.OIDCClientSecretStorageSecretForUID(t, testNamespace, testUID, []string{testutil.HashedPassword1AtSupervisorMinCost})}, wantAPIActions: 1, // one update wantResultingOIDCClients: []configv1alpha1.OIDCClient{{ ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234, UID: testUID}, @@ -753,7 +744,7 @@ func TestOIDCClientWatcherControllerSync(t *testing.T) { AllowedScopes: []configv1alpha1.Scope{"openid"}, }, }}, - inputSecrets: []runtime.Object{testutil.OIDCClientSecretStorageSecretForUID(t, testNamespace, testUID, []string{testBcryptSecret1})}, + inputSecrets: []runtime.Object{testutil.OIDCClientSecretStorageSecretForUID(t, testNamespace, testUID, []string{testutil.HashedPassword1AtSupervisorMinCost})}, wantAPIActions: 1, // one update wantResultingOIDCClients: []configv1alpha1.OIDCClient{{ ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234, UID: testUID}, @@ -777,7 +768,7 @@ func TestOIDCClientWatcherControllerSync(t *testing.T) { AllowedScopes: []configv1alpha1.Scope{"openid", "offline_access", "pinniped:request-audience", "username", "groups"}, }, }}, - inputSecrets: []runtime.Object{testutil.OIDCClientSecretStorageSecretForUID(t, testNamespace, testUID, []string{testBcryptSecret1})}, + inputSecrets: []runtime.Object{testutil.OIDCClientSecretStorageSecretForUID(t, testNamespace, testUID, []string{testutil.HashedPassword1AtSupervisorMinCost})}, wantAPIActions: 1, // one update wantResultingOIDCClients: []configv1alpha1.OIDCClient{{ ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234, UID: testUID}, @@ -801,7 +792,7 @@ func TestOIDCClientWatcherControllerSync(t *testing.T) { AllowedScopes: []configv1alpha1.Scope{"openid", "offline_access"}, }, }}, - inputSecrets: []runtime.Object{testutil.OIDCClientSecretStorageSecretForUID(t, testNamespace, testUID, []string{testBcryptSecret1})}, + inputSecrets: []runtime.Object{testutil.OIDCClientSecretStorageSecretForUID(t, testNamespace, testUID, []string{testutil.HashedPassword1AtSupervisorMinCost})}, wantAPIActions: 1, // one update wantResultingOIDCClients: []configv1alpha1.OIDCClient{{ ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234, UID: testUID}, @@ -825,7 +816,7 @@ func TestOIDCClientWatcherControllerSync(t *testing.T) { AllowedScopes: []configv1alpha1.Scope{"openid", "offline_access", "username"}, }, }}, - inputSecrets: []runtime.Object{testutil.OIDCClientSecretStorageSecretForUID(t, testNamespace, testUID, []string{testBcryptSecret1})}, + inputSecrets: []runtime.Object{testutil.OIDCClientSecretStorageSecretForUID(t, testNamespace, testUID, []string{testutil.HashedPassword1AtSupervisorMinCost})}, wantAPIActions: 1, // one update wantResultingOIDCClients: []configv1alpha1.OIDCClient{{ ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234, UID: testUID}, @@ -849,7 +840,7 @@ func TestOIDCClientWatcherControllerSync(t *testing.T) { AllowedScopes: []configv1alpha1.Scope{"openid", "offline_access", "groups"}, }, }}, - inputSecrets: []runtime.Object{testutil.OIDCClientSecretStorageSecretForUID(t, testNamespace, testUID, []string{testBcryptSecret1})}, + inputSecrets: []runtime.Object{testutil.OIDCClientSecretStorageSecretForUID(t, testNamespace, testUID, []string{testutil.HashedPassword1AtSupervisorMinCost})}, wantAPIActions: 1, // one update wantResultingOIDCClients: []configv1alpha1.OIDCClient{{ ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234, UID: testUID}, @@ -873,7 +864,7 @@ func TestOIDCClientWatcherControllerSync(t *testing.T) { AllowedScopes: []configv1alpha1.Scope{"openid", "offline_access", "username", "groups"}, }, }}, - inputSecrets: []runtime.Object{testutil.OIDCClientSecretStorageSecretForUID(t, testNamespace, testUID, []string{testBcryptSecret1})}, + inputSecrets: []runtime.Object{testutil.OIDCClientSecretStorageSecretForUID(t, testNamespace, testUID, []string{testutil.HashedPassword1AtSupervisorMinCost})}, wantAPIActions: 1, // one update wantResultingOIDCClients: []configv1alpha1.OIDCClient{{ ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234, UID: testUID}, @@ -897,7 +888,7 @@ func TestOIDCClientWatcherControllerSync(t *testing.T) { AllowedScopes: []configv1alpha1.Scope{"openid", "username"}, }, }}, - inputSecrets: []runtime.Object{testutil.OIDCClientSecretStorageSecretForUID(t, testNamespace, testUID, []string{testBcryptSecret1})}, + inputSecrets: []runtime.Object{testutil.OIDCClientSecretStorageSecretForUID(t, testNamespace, testUID, []string{testutil.HashedPassword1AtSupervisorMinCost})}, wantAPIActions: 1, // one update wantResultingOIDCClients: []configv1alpha1.OIDCClient{{ ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234, UID: testUID}, @@ -921,7 +912,7 @@ func TestOIDCClientWatcherControllerSync(t *testing.T) { AllowedScopes: []configv1alpha1.Scope{"openid", "username"}, }, }}, - inputSecrets: []runtime.Object{testutil.OIDCClientSecretStorageSecretForUID(t, testNamespace, testUID, []string{testBcryptSecret1})}, + inputSecrets: []runtime.Object{testutil.OIDCClientSecretStorageSecretForUID(t, testNamespace, testUID, []string{testutil.HashedPassword1AtSupervisorMinCost})}, wantAPIActions: 1, // one update wantResultingOIDCClients: []configv1alpha1.OIDCClient{{ ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234, UID: testUID}, @@ -945,7 +936,7 @@ func TestOIDCClientWatcherControllerSync(t *testing.T) { AllowedScopes: []configv1alpha1.Scope{"openid", "username", "groups"}, }, }}, - inputSecrets: []runtime.Object{testutil.OIDCClientSecretStorageSecretForUID(t, testNamespace, testUID, []string{testBcryptSecret1})}, + inputSecrets: []runtime.Object{testutil.OIDCClientSecretStorageSecretForUID(t, testNamespace, testUID, []string{testutil.HashedPassword1AtSupervisorMinCost})}, wantAPIActions: 1, // one update wantResultingOIDCClients: []configv1alpha1.OIDCClient{{ ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234, UID: testUID}, diff --git a/internal/oidc/auth/auth_handler_test.go b/internal/oidc/auth/auth_handler_test.go index 2cc98471..768ab10f 100644 --- a/internal/oidc/auth/auth_handler_test.go +++ b/internal/oidc/auth/auth_handler_test.go @@ -19,6 +19,7 @@ import ( "github.com/gorilla/securecookie" "github.com/ory/fosite" "github.com/stretchr/testify/require" + "golang.org/x/crypto/bcrypt" "golang.org/x/oauth2" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apiserver/pkg/authentication/user" @@ -27,7 +28,6 @@ import ( kubetesting "k8s.io/client-go/testing" "k8s.io/utils/pointer" - configv1alpha1 "go.pinniped.dev/generated/latest/apis/supervisor/config/v1alpha1" supervisorfake "go.pinniped.dev/generated/latest/client/supervisor/clientset/versioned/fake" "go.pinniped.dev/generated/latest/client/supervisor/clientset/versioned/typed/config/v1alpha1" "go.pinniped.dev/internal/authenticators" @@ -79,8 +79,6 @@ func TestAuthorizationEndpoint(t *testing.T) { pinnipedCLIClientID = "pinniped-cli" dynamicClientID = "client.oauth.pinniped.dev-test-name" dynamicClientUID = "fake-client-uid" - //nolint:gosec // this is not a credential - dynamicClientHashedSecret = "$2y$15$Kh7cRj0ScSD5QelE3ZNSl.nF04JDv7zb3SgGN.tSfLIX.4kt3UX7m" // bcrypt of "password1" at cost 15 ) require.Len(t, happyState, 8, "we expect fosite to allow 8 byte state params, so we want to test that boundary case") @@ -237,13 +235,15 @@ func TestAuthorizationEndpoint(t *testing.T) { createOauthHelperWithRealStorage := func(secretsClient v1.SecretInterface, oidcClientsClient v1alpha1.OIDCClientInterface) (fosite.OAuth2Provider, *oidc.KubeStorage) { // Configure fosite the same way that the production code would when using Kube storage. // Inject this into our test subject at the last second so we get a fresh storage for every test. - kubeOauthStore := oidc.NewKubeStorage(secretsClient, oidcClientsClient, timeoutsConfiguration) + // Use lower minimum required bcrypt cost than we would use in production to keep unit the tests fast. + kubeOauthStore := oidc.NewKubeStorage(secretsClient, oidcClientsClient, timeoutsConfiguration, bcrypt.MinCost) return oidc.FositeOauth2Helper(kubeOauthStore, downstreamIssuer, hmacSecretFunc, jwksProviderIsUnused, timeoutsConfiguration), kubeOauthStore } createOauthHelperWithNullStorage := func(secretsClient v1.SecretInterface, oidcClientsClient v1alpha1.OIDCClientInterface) (fosite.OAuth2Provider, *oidc.NullStorage) { // Configure fosite the same way that the production code would, using NullStorage to turn off storage. - nullOauthStore := oidc.NewNullStorage(secretsClient, oidcClientsClient) + // Use lower minimum required bcrypt cost than we would use in production to keep unit the tests fast. + nullOauthStore := oidc.NewNullStorage(secretsClient, oidcClientsClient, bcrypt.MinCost) return oidc.FositeOauth2Helper(nullOauthStore, downstreamIssuer, hmacSecretFunc, jwksProviderIsUnused, timeoutsConfiguration), nullOauthStore } @@ -511,24 +511,11 @@ func TestAuthorizationEndpoint(t *testing.T) { }, } - fullyCapableDynamicClient := &configv1alpha1.OIDCClient{ - ObjectMeta: metav1.ObjectMeta{Namespace: "some-namespace", Name: dynamicClientID, Generation: 1, UID: dynamicClientUID}, - Spec: configv1alpha1.OIDCClientSpec{ - AllowedGrantTypes: []configv1alpha1.GrantType{"authorization_code", "urn:ietf:params:oauth:grant-type:token-exchange", "refresh_token"}, - AllowedScopes: []configv1alpha1.Scope{"openid", "offline_access", "pinniped:request-audience", "username", "groups"}, - AllowedRedirectURIs: []configv1alpha1.RedirectURI{downstreamRedirectURI}, - }, - } - - allDynamicClientScopes := "openid offline_access pinniped:request-audience username groups" - - storageSecretWithOneClientSecretForDynamicClient := testutil.OIDCClientSecretStorageSecretForUID(t, - "some-namespace", dynamicClientUID, []string{dynamicClientHashedSecret}, - ) - addFullyCapableDynamicClientAndSecretToKubeResources := func(t *testing.T, supervisorClient *supervisorfake.Clientset, kubeClient *fake.Clientset) { - require.NoError(t, supervisorClient.Tracker().Add(fullyCapableDynamicClient)) - require.NoError(t, kubeClient.Tracker().Add(storageSecretWithOneClientSecretForDynamicClient)) + oidcClient, secret := testutil.FullyCapableOIDCClientAndStorageSecret(t, + "some-namespace", dynamicClientID, dynamicClientUID, downstreamRedirectURI, []string{testutil.HashedPassword1AtGoMinCost}) + require.NoError(t, supervisorClient.Tracker().Add(oidcClient)) + require.NoError(t, kubeClient.Tracker().Add(secret)) } // Note that fosite puts the granted scopes as a param in the redirect URI even though the spec doesn't seem to require it @@ -611,11 +598,11 @@ func TestAuthorizationEndpoint(t *testing.T) { stateEncoder: happyStateEncoder, cookieEncoder: happyCookieEncoder, method: http.MethodGet, - path: modifiedHappyGetRequestPath(map[string]string{"client_id": dynamicClientID, "scope": allDynamicClientScopes}), + path: modifiedHappyGetRequestPath(map[string]string{"client_id": dynamicClientID, "scope": testutil.AllDynamicClientScopesSpaceSep}), wantStatus: http.StatusSeeOther, wantContentType: htmlContentType, wantCSRFValueInCookieHeader: happyCSRF, - wantLocationHeader: expectedRedirectLocationForUpstreamOIDC(expectedUpstreamStateParam(map[string]string{"client_id": dynamicClientID, "scope": allDynamicClientScopes}, "", oidcUpstreamName, "oidc"), nil), + wantLocationHeader: expectedRedirectLocationForUpstreamOIDC(expectedUpstreamStateParam(map[string]string{"client_id": dynamicClientID, "scope": testutil.AllDynamicClientScopesSpaceSep}, "", oidcUpstreamName, "oidc"), nil), wantUpstreamStateParamInLocationHeader: true, wantBodyStringWithLocationInHref: true, }, @@ -646,11 +633,11 @@ func TestAuthorizationEndpoint(t *testing.T) { stateEncoder: happyStateEncoder, cookieEncoder: happyCookieEncoder, method: http.MethodGet, - path: modifiedHappyGetRequestPath(map[string]string{"client_id": dynamicClientID, "scope": allDynamicClientScopes}), + path: modifiedHappyGetRequestPath(map[string]string{"client_id": dynamicClientID, "scope": testutil.AllDynamicClientScopesSpaceSep}), wantStatus: http.StatusSeeOther, wantContentType: htmlContentType, wantCSRFValueInCookieHeader: happyCSRF, - wantLocationHeader: urlWithQuery(downstreamIssuer+"/login", map[string]string{"state": expectedUpstreamStateParam(map[string]string{"client_id": dynamicClientID, "scope": allDynamicClientScopes}, "", ldapUpstreamName, "ldap")}), + wantLocationHeader: urlWithQuery(downstreamIssuer+"/login", map[string]string{"state": expectedUpstreamStateParam(map[string]string{"client_id": dynamicClientID, "scope": testutil.AllDynamicClientScopesSpaceSep}, "", ldapUpstreamName, "ldap")}), wantUpstreamStateParamInLocationHeader: true, wantBodyStringWithLocationInHref: true, }, @@ -681,11 +668,11 @@ func TestAuthorizationEndpoint(t *testing.T) { stateEncoder: happyStateEncoder, cookieEncoder: happyCookieEncoder, method: http.MethodGet, - path: modifiedHappyGetRequestPath(map[string]string{"client_id": dynamicClientID, "scope": allDynamicClientScopes}), + path: modifiedHappyGetRequestPath(map[string]string{"client_id": dynamicClientID, "scope": testutil.AllDynamicClientScopesSpaceSep}), wantStatus: http.StatusSeeOther, wantContentType: htmlContentType, wantCSRFValueInCookieHeader: happyCSRF, - wantLocationHeader: urlWithQuery(downstreamIssuer+"/login", map[string]string{"state": expectedUpstreamStateParam(map[string]string{"client_id": dynamicClientID, "scope": allDynamicClientScopes}, "", activeDirectoryUpstreamName, "activedirectory")}), + wantLocationHeader: urlWithQuery(downstreamIssuer+"/login", map[string]string{"state": expectedUpstreamStateParam(map[string]string{"client_id": dynamicClientID, "scope": testutil.AllDynamicClientScopesSpaceSep}, "", activeDirectoryUpstreamName, "activedirectory")}), wantUpstreamStateParamInLocationHeader: true, wantBodyStringWithLocationInHref: true, }, @@ -835,12 +822,12 @@ func TestAuthorizationEndpoint(t *testing.T) { method: http.MethodPost, path: "/some/path", contentType: formContentType, - body: encodeQuery(modifiedHappyGetRequestQueryMap(map[string]string{"client_id": dynamicClientID, "scope": allDynamicClientScopes})), + body: encodeQuery(modifiedHappyGetRequestQueryMap(map[string]string{"client_id": dynamicClientID, "scope": testutil.AllDynamicClientScopesSpaceSep})), wantStatus: http.StatusSeeOther, wantContentType: "", wantBodyString: "", wantCSRFValueInCookieHeader: happyCSRF, - wantLocationHeader: expectedRedirectLocationForUpstreamOIDC(expectedUpstreamStateParam(map[string]string{"client_id": dynamicClientID, "scope": allDynamicClientScopes}, "", oidcUpstreamName, "oidc"), nil), + wantLocationHeader: expectedRedirectLocationForUpstreamOIDC(expectedUpstreamStateParam(map[string]string{"client_id": dynamicClientID, "scope": testutil.AllDynamicClientScopesSpaceSep}, "", oidcUpstreamName, "oidc"), nil), wantUpstreamStateParamInLocationHeader: true, }, { @@ -874,12 +861,12 @@ func TestAuthorizationEndpoint(t *testing.T) { method: http.MethodPost, path: "/some/path", contentType: formContentType, - body: encodeQuery(modifiedHappyGetRequestQueryMap(map[string]string{"client_id": dynamicClientID, "scope": allDynamicClientScopes})), + body: encodeQuery(modifiedHappyGetRequestQueryMap(map[string]string{"client_id": dynamicClientID, "scope": testutil.AllDynamicClientScopesSpaceSep})), wantStatus: http.StatusSeeOther, wantContentType: "", wantBodyString: "", wantCSRFValueInCookieHeader: happyCSRF, - wantLocationHeader: urlWithQuery(downstreamIssuer+"/login", map[string]string{"state": expectedUpstreamStateParam(map[string]string{"client_id": dynamicClientID, "scope": allDynamicClientScopes}, "", ldapUpstreamName, "ldap")}), + wantLocationHeader: urlWithQuery(downstreamIssuer+"/login", map[string]string{"state": expectedUpstreamStateParam(map[string]string{"client_id": dynamicClientID, "scope": testutil.AllDynamicClientScopesSpaceSep}, "", ldapUpstreamName, "ldap")}), wantUpstreamStateParamInLocationHeader: true, }, { @@ -913,12 +900,12 @@ func TestAuthorizationEndpoint(t *testing.T) { method: http.MethodPost, path: "/some/path", contentType: formContentType, - body: encodeQuery(modifiedHappyGetRequestQueryMap(map[string]string{"client_id": dynamicClientID, "scope": allDynamicClientScopes})), + body: encodeQuery(modifiedHappyGetRequestQueryMap(map[string]string{"client_id": dynamicClientID, "scope": testutil.AllDynamicClientScopesSpaceSep})), wantStatus: http.StatusSeeOther, wantContentType: "", wantBodyString: "", wantCSRFValueInCookieHeader: happyCSRF, - wantLocationHeader: urlWithQuery(downstreamIssuer+"/login", map[string]string{"state": expectedUpstreamStateParam(map[string]string{"client_id": dynamicClientID, "scope": allDynamicClientScopes}, "", activeDirectoryUpstreamName, "activedirectory")}), + wantLocationHeader: urlWithQuery(downstreamIssuer+"/login", map[string]string{"state": expectedUpstreamStateParam(map[string]string{"client_id": dynamicClientID, "scope": testutil.AllDynamicClientScopesSpaceSep}, "", activeDirectoryUpstreamName, "activedirectory")}), wantUpstreamStateParamInLocationHeader: true, }, { @@ -1111,7 +1098,7 @@ func TestAuthorizationEndpoint(t *testing.T) { path: modifiedHappyGetRequestPath(map[string]string{ "redirect_uri": downstreamRedirectURIWithDifferentPort, // not the same port number that is registered for the client "client_id": dynamicClientID, - "scope": allDynamicClientScopes, + "scope": testutil.AllDynamicClientScopesSpaceSep, }), wantStatus: http.StatusSeeOther, wantContentType: htmlContentType, @@ -1119,7 +1106,7 @@ func TestAuthorizationEndpoint(t *testing.T) { wantLocationHeader: expectedRedirectLocationForUpstreamOIDC(expectedUpstreamStateParam(map[string]string{ "redirect_uri": downstreamRedirectURIWithDifferentPort, // not the same port number that is registered for the client "client_id": dynamicClientID, - "scope": allDynamicClientScopes, + "scope": testutil.AllDynamicClientScopesSpaceSep, }, "", oidcUpstreamName, "oidc"), nil), wantUpstreamStateParamInLocationHeader: true, wantBodyStringWithLocationInHref: true, @@ -1526,7 +1513,7 @@ func TestAuthorizationEndpoint(t *testing.T) { idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(passwordGrantUpstreamOIDCIdentityProviderBuilder().Build()), kubeResources: addFullyCapableDynamicClientAndSecretToKubeResources, method: http.MethodGet, - path: modifiedHappyGetRequestPath(map[string]string{"client_id": dynamicClientID, "scope": allDynamicClientScopes}), + path: modifiedHappyGetRequestPath(map[string]string{"client_id": dynamicClientID, "scope": testutil.AllDynamicClientScopesSpaceSep}), customUsernameHeader: pointer.StringPtr(oidcUpstreamUsername), customPasswordHeader: pointer.StringPtr(oidcUpstreamPassword), wantStatus: http.StatusFound, @@ -1539,7 +1526,7 @@ func TestAuthorizationEndpoint(t *testing.T) { idps: oidctestutil.NewUpstreamIDPListerBuilder().WithLDAP(&upstreamLDAPIdentityProvider), kubeResources: addFullyCapableDynamicClientAndSecretToKubeResources, method: http.MethodGet, - path: modifiedHappyGetRequestPath(map[string]string{"client_id": dynamicClientID, "scope": allDynamicClientScopes}), + path: modifiedHappyGetRequestPath(map[string]string{"client_id": dynamicClientID, "scope": testutil.AllDynamicClientScopesSpaceSep}), customUsernameHeader: pointer.StringPtr(happyLDAPUsername), customPasswordHeader: pointer.StringPtr(happyLDAPPassword), wantStatus: http.StatusFound, @@ -1552,7 +1539,7 @@ func TestAuthorizationEndpoint(t *testing.T) { idps: oidctestutil.NewUpstreamIDPListerBuilder().WithActiveDirectory(&upstreamActiveDirectoryIdentityProvider), kubeResources: addFullyCapableDynamicClientAndSecretToKubeResources, method: http.MethodGet, - path: modifiedHappyGetRequestPath(map[string]string{"client_id": dynamicClientID, "scope": allDynamicClientScopes}), + path: modifiedHappyGetRequestPath(map[string]string{"client_id": dynamicClientID, "scope": testutil.AllDynamicClientScopesSpaceSep}), customUsernameHeader: pointer.StringPtr(happyLDAPUsername), customPasswordHeader: pointer.StringPtr(happyLDAPPassword), wantStatus: http.StatusFound, @@ -1589,7 +1576,7 @@ func TestAuthorizationEndpoint(t *testing.T) { path: modifiedHappyGetRequestPath(map[string]string{ "redirect_uri": "http://127.0.0.1/does-not-match-what-is-configured-for-dynamic-client", "client_id": dynamicClientID, - "scope": allDynamicClientScopes, + "scope": testutil.AllDynamicClientScopesSpaceSep, }), wantStatus: http.StatusBadRequest, wantContentType: jsonContentType, @@ -1705,7 +1692,7 @@ func TestAuthorizationEndpoint(t *testing.T) { path: modifiedHappyGetRequestPath(map[string]string{ "response_type": "unsupported", "client_id": dynamicClientID, - "scope": allDynamicClientScopes, + "scope": testutil.AllDynamicClientScopesSpaceSep, }), wantStatus: http.StatusSeeOther, wantContentType: jsonContentType, @@ -1754,7 +1741,7 @@ func TestAuthorizationEndpoint(t *testing.T) { path: modifiedHappyGetRequestPath(map[string]string{ "response_type": "unsupported", "client_id": dynamicClientID, - "scope": allDynamicClientScopes, + "scope": testutil.AllDynamicClientScopesSpaceSep, }), wantStatus: http.StatusSeeOther, wantContentType: jsonContentType, @@ -1791,7 +1778,7 @@ func TestAuthorizationEndpoint(t *testing.T) { path: modifiedHappyGetRequestPath(map[string]string{ "response_type": "unsupported", "client_id": dynamicClientID, - "scope": allDynamicClientScopes, + "scope": testutil.AllDynamicClientScopesSpaceSep, }), wantStatus: http.StatusSeeOther, wantContentType: jsonContentType, @@ -1865,7 +1852,7 @@ func TestAuthorizationEndpoint(t *testing.T) { stateEncoder: happyStateEncoder, cookieEncoder: happyCookieEncoder, method: http.MethodGet, - path: modifiedHappyGetRequestPath(map[string]string{"response_mode": "form_post", "client_id": dynamicClientID, "scope": allDynamicClientScopes}), + path: modifiedHappyGetRequestPath(map[string]string{"response_mode": "form_post", "client_id": dynamicClientID, "scope": testutil.AllDynamicClientScopesSpaceSep}), wantStatus: http.StatusOK, // this is weird, but fosite uses a form_post response to tell the client that it is not allowed to use form_post responses wantContentType: htmlContentType, wantBodyRegex: ` 0 { diff --git a/internal/oidc/provider/manager/manager.go b/internal/oidc/provider/manager/manager.go index 83a91d07..47d25981 100644 --- a/internal/oidc/provider/manager/manager.go +++ b/internal/oidc/provider/manager/manager.go @@ -20,6 +20,7 @@ import ( "go.pinniped.dev/internal/oidc/idpdiscovery" "go.pinniped.dev/internal/oidc/jwks" "go.pinniped.dev/internal/oidc/login" + "go.pinniped.dev/internal/oidc/oidcclientvalidator" "go.pinniped.dev/internal/oidc/provider" "go.pinniped.dev/internal/oidc/token" "go.pinniped.dev/internal/plog" @@ -98,7 +99,7 @@ func (m *Manager) SetProviders(federationDomains ...*provider.FederationDomainIs // Use NullStorage for the authorize endpoint because we do not actually want to store anything until // the upstream callback endpoint is called later. oauthHelperWithNullStorage := oidc.FositeOauth2Helper( - oidc.NewNullStorage(m.secretsClient, m.oidcClientsClient), + oidc.NewNullStorage(m.secretsClient, m.oidcClientsClient, oidcclientvalidator.DefaultMinBcryptCost), issuer, tokenHMACKeyGetter, nil, @@ -107,7 +108,7 @@ func (m *Manager) SetProviders(federationDomains ...*provider.FederationDomainIs // For all the other endpoints, make another oauth helper with exactly the same settings except use real storage. oauthHelperWithKubeStorage := oidc.FositeOauth2Helper( - oidc.NewKubeStorage(m.secretsClient, m.oidcClientsClient, timeoutsConfiguration), + oidc.NewKubeStorage(m.secretsClient, m.oidcClientsClient, timeoutsConfiguration, oidcclientvalidator.DefaultMinBcryptCost), issuer, tokenHMACKeyGetter, m.dynamicJWKSProvider, diff --git a/internal/oidc/token/token_handler_test.go b/internal/oidc/token/token_handler_test.go index 5bdd3688..b22e8ad4 100644 --- a/internal/oidc/token/token_handler_test.go +++ b/internal/oidc/token/token_handler_test.go @@ -29,14 +29,17 @@ import ( "github.com/ory/fosite/token/jwt" "github.com/pkg/errors" "github.com/stretchr/testify/require" + "golang.org/x/crypto/bcrypt" "golang.org/x/oauth2" "gopkg.in/square/go-jose.v2" josejwt "gopkg.in/square/go-jose.v2/jwt" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" + "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/kubernetes/fake" v1 "k8s.io/client-go/kubernetes/typed/core/v1" + configv1alpha1 "go.pinniped.dev/generated/latest/apis/supervisor/config/v1alpha1" supervisorfake "go.pinniped.dev/generated/latest/client/supervisor/clientset/versioned/fake" "go.pinniped.dev/internal/crud" "go.pinniped.dev/internal/fositestorage/accesstoken" @@ -50,6 +53,7 @@ import ( "go.pinniped.dev/internal/oidc" "go.pinniped.dev/internal/oidc/jwks" "go.pinniped.dev/internal/oidc/provider" + "go.pinniped.dev/internal/oidcclientsecretstorage" "go.pinniped.dev/internal/psession" "go.pinniped.dev/internal/testutil" "go.pinniped.dev/internal/testutil/oidctestutil" @@ -59,20 +63,23 @@ import ( const ( goodIssuer = "https://some-issuer.com" goodUpstreamSubject = "some-subject" - goodClient = "pinniped-cli" goodRedirectURI = "http://127.0.0.1/callback" goodPKCECodeVerifier = "some-pkce-verifier-that-must-be-at-least-43-characters-to-meet-entropy-requirements" goodNonce = "some-nonce-value-with-enough-bytes-to-exceed-min-allowed" goodSubject = "https://issuer?sub=some-subject" goodUsername = "some-username" + pinnipedCLIClientID = "pinniped-cli" + dynamicClientID = "client.oauth.pinniped.dev-test-name" + dynamicClientUID = "fake-client-uid" + hmacSecret = "this needs to be at least 32 characters to meet entropy requirements" authCodeExpirationSeconds = 10 * 60 // Current, we set our auth code expiration to 10 minutes accessTokenExpirationSeconds = 2 * 60 // Currently, we set our access token expiration to 2 minutes idTokenExpirationSeconds = 2 * 60 // Currently, we set our ID token expiration to 2 minutes - timeComparisonFudgeSeconds = 15 + timeComparisonFudge = 15 * time.Second ) var ( @@ -156,6 +163,20 @@ var ( } `) + fositeClientIDMismatchDuringAuthcodeExchangeErrorBody = here.Doc(` + { + "error": "invalid_grant", + "error_description": "The provided authorization grant (e.g., authorization code, resource owner credentials) or refresh token is invalid, expired, revoked, does not match the redirection URI used in the authorization request, or was issued to another client. The OAuth 2.0 Client ID from this request does not match the one from the authorize request." + } + `) + + fositeClientIDMismatchDuringRefreshErrorBody = here.Doc(` + { + "error": "invalid_grant", + "error_description": "The provided authorization grant (e.g., authorization code, resource owner credentials) or refresh token is invalid, expired, revoked, does not match the redirection URI used in the authorization request, or was issued to another client. The OAuth 2.0 Client ID from this request does not match the ID during the initial token issuance." + } + `) + fositeInvalidRedirectURIErrorBody = here.Doc(` { "error": "invalid_grant", @@ -198,11 +219,25 @@ var ( } `) + fositeClientAuthFailedErrorBody = here.Doc(` + { + "error": "invalid_client", + "error_description": "Client authentication failed (e.g., unknown client, no client authentication included, or unsupported authentication method)." + } + `) + + fositeClientAuthMustBeBasicAuthErrorBody = here.Doc(` + { + "error": "invalid_client", + "error_description": "Client authentication failed (e.g., unknown client, no client authentication included, or unsupported authentication method). The OAuth 2.0 Client supports client authentication method 'client_secret_basic', but method 'client_secret_post' was requested. You must configure the OAuth 2.0 client's 'token_endpoint_auth_method' value to accept 'client_secret_post'." + } + `) + happyAuthRequest = &http.Request{ Form: url.Values{ "response_type": {"code"}, "scope": {"openid profile email groups"}, - "client_id": {goodClient}, + "client_id": {pinnipedCLIClientID}, "state": {"some-state-value-with-enough-bytes-to-exceed-min-allowed"}, "nonce": {goodNonce}, "code_challenge": {testutil.SHA256(goodPKCECodeVerifier)}, @@ -219,7 +254,7 @@ var ( "subject_token": {subjectToken}, "subject_token_type": {"urn:ietf:params:oauth:token-type:access_token"}, "requested_token_type": {"urn:ietf:params:oauth:token-type:jwt"}, - "client_id": {goodClient}, + "client_id": {pinnipedCLIClientID}, }, } } @@ -239,6 +274,7 @@ type tokenEndpointResponseExpectedValues struct { wantStatus int wantSuccessBodyFields []string wantErrorResponseBody string + wantClientID string wantRequestedScopes []string wantGrantedScopes []string wantGroups []string @@ -260,10 +296,32 @@ type authcodeExchangeInputs struct { want tokenEndpointResponseExpectedValues } +func addFullyCapableDynamicClientAndSecretToKubeResources(t *testing.T, supervisorClient *supervisorfake.Clientset, kubeClient *fake.Clientset) { + oidcClient, secret := testutil.FullyCapableOIDCClientAndStorageSecret(t, + "some-namespace", + dynamicClientID, + dynamicClientUID, + goodRedirectURI, + []string{testutil.HashedPassword1AtGoMinCost, testutil.HashedPassword2AtGoMinCost}, + ) + require.NoError(t, supervisorClient.Tracker().Add(oidcClient)) + require.NoError(t, kubeClient.Tracker().Add(secret)) +} + +func modifyAuthcodeTokenRequestWithDynamicClientAuth(r *http.Request, authCode string) { + r.Body = happyAuthcodeRequestBody(authCode).WithClientID("").ReadCloser() // No client_id in body. + r.SetBasicAuth(dynamicClientID, testutil.PlaintextPassword1) // Use basic auth header instead. +} + +func addDynamicClientIDToFormPostBody(r *http.Request) { + r.Form.Set("client_id", dynamicClientID) +} + func TestTokenEndpointAuthcodeExchange(t *testing.T) { tests := []struct { name string authcodeExchange authcodeExchangeInputs + kubeResources func(t *testing.T, supervisorClient *supervisorfake.Clientset, kubeClient *fake.Clientset) }{ // happy path { @@ -272,6 +330,7 @@ func TestTokenEndpointAuthcodeExchange(t *testing.T) { modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid profile email groups") }, want: tokenEndpointResponseExpectedValues{ wantStatus: http.StatusOK, + wantClientID: pinnipedCLIClientID, wantSuccessBodyFields: []string{"id_token", "access_token", "token_type", "scope", "expires_in"}, // no refresh token wantRequestedScopes: []string{"openid", "profile", "email", "groups"}, wantGrantedScopes: []string{"openid", "groups"}, @@ -279,25 +338,84 @@ func TestTokenEndpointAuthcodeExchange(t *testing.T) { }, }, }, + { + name: "request is valid and tokens are issued for dynamic client", + kubeResources: addFullyCapableDynamicClientAndSecretToKubeResources, + authcodeExchange: authcodeExchangeInputs{ + modifyAuthRequest: func(r *http.Request) { + addDynamicClientIDToFormPostBody(r) + r.Form.Set("scope", "openid pinniped:request-audience groups") + }, + modifyTokenRequest: modifyAuthcodeTokenRequestWithDynamicClientAuth, + want: tokenEndpointResponseExpectedValues{ + wantStatus: http.StatusOK, + wantClientID: dynamicClientID, + wantSuccessBodyFields: []string{"id_token", "access_token", "token_type", "scope", "expires_in"}, // no refresh token + wantRequestedScopes: []string{"openid", "pinniped:request-audience", "groups"}, + wantGrantedScopes: []string{"openid", "pinniped:request-audience", "groups"}, + wantGroups: goodGroups, + }, + }, + }, { name: "openid scope was not requested from authorize endpoint", authcodeExchange: authcodeExchangeInputs{ modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "profile email") }, want: tokenEndpointResponseExpectedValues{ wantStatus: http.StatusOK, + wantClientID: pinnipedCLIClientID, wantSuccessBodyFields: []string{"access_token", "token_type", "scope", "expires_in"}, // no id or refresh tokens wantRequestedScopes: []string{"profile", "email"}, wantGrantedScopes: []string{}, - wantGroups: goodGroups, + wantGroups: nil, }, }, }, { - name: "offline_access and openid scopes were requested and granted from authorize endpoint", + name: "openid scope was not requested from authorize endpoint for dynamic client", + kubeResources: addFullyCapableDynamicClientAndSecretToKubeResources, + authcodeExchange: authcodeExchangeInputs{ + modifyAuthRequest: func(r *http.Request) { + addDynamicClientIDToFormPostBody(r) + r.Form.Set("scope", "pinniped:request-audience groups") + }, + modifyTokenRequest: modifyAuthcodeTokenRequestWithDynamicClientAuth, + want: tokenEndpointResponseExpectedValues{ + wantStatus: http.StatusOK, + wantClientID: dynamicClientID, + wantSuccessBodyFields: []string{"access_token", "token_type", "scope", "expires_in"}, // no id or refresh tokens + wantRequestedScopes: []string{"pinniped:request-audience", "groups"}, + wantGrantedScopes: []string{"pinniped:request-audience", "groups"}, + wantGroups: nil, + }, + }, + }, + { + name: "offline_access and openid scopes were requested and granted from authorize endpoint (no groups)", authcodeExchange: authcodeExchangeInputs{ modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, want: tokenEndpointResponseExpectedValues{ wantStatus: http.StatusOK, + wantClientID: pinnipedCLIClientID, + wantSuccessBodyFields: []string{"id_token", "access_token", "token_type", "scope", "expires_in", "refresh_token"}, // all possible tokens + wantRequestedScopes: []string{"openid", "offline_access"}, + wantGrantedScopes: []string{"openid", "offline_access"}, + wantGroups: nil, + }, + }, + }, + { + name: "offline_access and openid scopes were requested and granted from authorize endpoint for dynamic client (no groups)", + kubeResources: addFullyCapableDynamicClientAndSecretToKubeResources, + authcodeExchange: authcodeExchangeInputs{ + modifyAuthRequest: func(r *http.Request) { + addDynamicClientIDToFormPostBody(r) + r.Form.Set("scope", "openid offline_access") + }, + modifyTokenRequest: modifyAuthcodeTokenRequestWithDynamicClientAuth, + want: tokenEndpointResponseExpectedValues{ + wantStatus: http.StatusOK, + wantClientID: dynamicClientID, wantSuccessBodyFields: []string{"id_token", "access_token", "token_type", "scope", "expires_in", "refresh_token"}, // all possible tokens wantRequestedScopes: []string{"openid", "offline_access"}, wantGrantedScopes: []string{"openid", "offline_access"}, @@ -311,10 +429,30 @@ func TestTokenEndpointAuthcodeExchange(t *testing.T) { modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "offline_access") }, want: tokenEndpointResponseExpectedValues{ wantStatus: http.StatusOK, + wantClientID: pinnipedCLIClientID, wantSuccessBodyFields: []string{"access_token", "token_type", "scope", "expires_in", "refresh_token"}, // no id token wantRequestedScopes: []string{"offline_access"}, wantGrantedScopes: []string{"offline_access"}, - wantGroups: goodGroups, + wantGroups: nil, + }, + }, + }, + { + name: "offline_access (without openid scope) was requested and granted from authorize endpoint for dynamic client", + kubeResources: addFullyCapableDynamicClientAndSecretToKubeResources, + authcodeExchange: authcodeExchangeInputs{ + modifyAuthRequest: func(r *http.Request) { + addDynamicClientIDToFormPostBody(r) + r.Form.Set("scope", "offline_access") + }, + modifyTokenRequest: modifyAuthcodeTokenRequestWithDynamicClientAuth, + want: tokenEndpointResponseExpectedValues{ + wantStatus: http.StatusOK, + wantClientID: dynamicClientID, + wantSuccessBodyFields: []string{"access_token", "token_type", "scope", "expires_in", "refresh_token"}, // no id token + wantRequestedScopes: []string{"offline_access"}, + wantGrantedScopes: []string{"offline_access"}, + wantGroups: nil, }, }, }, @@ -324,6 +462,7 @@ func TestTokenEndpointAuthcodeExchange(t *testing.T) { modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid profile email groups") }, want: tokenEndpointResponseExpectedValues{ wantStatus: http.StatusOK, + wantClientID: pinnipedCLIClientID, wantSuccessBodyFields: []string{"id_token", "access_token", "token_type", "scope", "expires_in"}, // no refresh token wantRequestedScopes: []string{"openid", "profile", "email", "groups"}, wantGrantedScopes: []string{"openid", "groups"}, @@ -331,6 +470,28 @@ func TestTokenEndpointAuthcodeExchange(t *testing.T) { }, }, }, + { + name: "dynamic client uses a secondary client secret (one of the other client secrets after the first one in the list)", + kubeResources: addFullyCapableDynamicClientAndSecretToKubeResources, + authcodeExchange: authcodeExchangeInputs{ + modifyAuthRequest: func(r *http.Request) { + addDynamicClientIDToFormPostBody(r) + r.Form.Set("scope", "openid pinniped:request-audience groups") + }, + modifyTokenRequest: func(r *http.Request, authCode string) { + r.Body = happyAuthcodeRequestBody(authCode).WithClientID("").ReadCloser() + r.SetBasicAuth(dynamicClientID, testutil.PlaintextPassword2) // use the second client secret that was configured on the client + }, + want: tokenEndpointResponseExpectedValues{ + wantStatus: http.StatusOK, + wantClientID: dynamicClientID, + wantSuccessBodyFields: []string{"id_token", "access_token", "token_type", "scope", "expires_in"}, // no refresh token + wantRequestedScopes: []string{"openid", "pinniped:request-audience", "groups"}, + wantGrantedScopes: []string{"openid", "pinniped:request-audience", "groups"}, + wantGroups: goodGroups, + }, + }, + }, // sad path { @@ -373,6 +534,57 @@ func TestTokenEndpointAuthcodeExchange(t *testing.T) { }, }, }, + { + name: "dynamic client uses wrong client secret", + kubeResources: addFullyCapableDynamicClientAndSecretToKubeResources, + authcodeExchange: authcodeExchangeInputs{ + modifyAuthRequest: func(r *http.Request) { + addDynamicClientIDToFormPostBody(r) + r.Form.Set("scope", "openid pinniped:request-audience groups") + }, + modifyTokenRequest: func(r *http.Request, authCode string) { + r.Body = happyAuthcodeRequestBody(authCode).WithClientID("").ReadCloser() + r.SetBasicAuth(dynamicClientID, "wrong client secret") + }, + want: tokenEndpointResponseExpectedValues{ + wantStatus: http.StatusUnauthorized, + wantErrorResponseBody: fositeClientAuthFailedErrorBody, + }, + }, + }, + { + name: "dynamic client uses wrong auth method (must use basic auth)", + kubeResources: addFullyCapableDynamicClientAndSecretToKubeResources, + authcodeExchange: authcodeExchangeInputs{ + modifyAuthRequest: func(r *http.Request) { + addDynamicClientIDToFormPostBody(r) + r.Form.Set("scope", "openid pinniped:request-audience groups") + }, + modifyTokenRequest: func(r *http.Request, authCode string) { + // Add client auth to the form, when it should be in basic auth headers. + r.Body = happyAuthcodeRequestBody(authCode).WithClientID(dynamicClientID).WithClientSecret(testutil.PlaintextPassword1).ReadCloser() + }, + want: tokenEndpointResponseExpectedValues{ + wantStatus: http.StatusUnauthorized, + wantErrorResponseBody: fositeClientAuthMustBeBasicAuthErrorBody, + }, + }, + }, + { + name: "tries to change client ID between authorization request and token request", + kubeResources: addFullyCapableDynamicClientAndSecretToKubeResources, + authcodeExchange: authcodeExchangeInputs{ + modifyAuthRequest: func(r *http.Request) { + // Test uses pinniped-cli client_id by default here. + r.Form.Set("scope", "openid pinniped:request-audience") + }, + modifyTokenRequest: modifyAuthcodeTokenRequestWithDynamicClientAuth, + want: tokenEndpointResponseExpectedValues{ + wantStatus: http.StatusBadRequest, + wantErrorResponseBody: fositeClientIDMismatchDuringAuthcodeExchangeErrorBody, + }, + }, + }, { name: "content type is invalid", authcodeExchange: authcodeExchangeInputs{ @@ -417,18 +629,6 @@ func TestTokenEndpointAuthcodeExchange(t *testing.T) { }, }, }, - { - name: "grant type is not authorization_code", - authcodeExchange: authcodeExchangeInputs{ - modifyTokenRequest: func(r *http.Request, authCode string) { - r.Body = happyAuthcodeRequestBody(authCode).WithGrantType("bogus").ReadCloser() - }, - want: tokenEndpointResponseExpectedValues{ - wantStatus: http.StatusBadRequest, - wantErrorResponseBody: fositeInvalidRequestErrorBody, - }, - }, - }, { name: "client id is missing in request", authcodeExchange: authcodeExchangeInputs{ @@ -568,7 +768,8 @@ func TestTokenEndpointAuthcodeExchange(t *testing.T) { t.Parallel() // Authcode exchange doesn't use the upstream provider cache, so just pass an empty cache. - exchangeAuthcodeForTokens(t, test.authcodeExchange, oidctestutil.NewUpstreamIDPListerBuilder().Build()) + exchangeAuthcodeForTokens(t, + test.authcodeExchange, oidctestutil.NewUpstreamIDPListerBuilder().Build(), test.kubeResources) }) } } @@ -577,6 +778,7 @@ func TestTokenEndpointWhenAuthcodeIsUsedTwice(t *testing.T) { tests := []struct { name string authcodeExchange authcodeExchangeInputs + kubeResources func(t *testing.T, supervisorClient *supervisorfake.Clientset, kubeClient *fake.Clientset) }{ { name: "authcode exchange succeeds once and then fails when the same authcode is used again", @@ -584,6 +786,7 @@ func TestTokenEndpointWhenAuthcodeIsUsedTwice(t *testing.T) { modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access profile email groups") }, want: tokenEndpointResponseExpectedValues{ wantStatus: http.StatusOK, + wantClientID: pinnipedCLIClientID, wantSuccessBodyFields: []string{"id_token", "refresh_token", "access_token", "token_type", "expires_in", "scope"}, wantRequestedScopes: []string{"openid", "offline_access", "profile", "email", "groups"}, wantGrantedScopes: []string{"openid", "offline_access", "groups"}, @@ -600,7 +803,7 @@ func TestTokenEndpointWhenAuthcodeIsUsedTwice(t *testing.T) { // First call - should be successful. // Authcode exchange doesn't use the upstream provider cache, so just pass an empty cache. subject, rsp, authCode, _, secrets, oauthStore := exchangeAuthcodeForTokens(t, - test.authcodeExchange, oidctestutil.NewUpstreamIDPListerBuilder().Build()) + test.authcodeExchange, oidctestutil.NewUpstreamIDPListerBuilder().Build(), test.kubeResources) var parsedResponseBody map[string]interface{} require.NoError(t, json.Unmarshal(rsp.Body.Bytes(), &parsedResponseBody)) @@ -611,6 +814,7 @@ func TestTokenEndpointWhenAuthcodeIsUsedTwice(t *testing.T) { req := httptest.NewRequest("POST", "/path/shouldn't/matter", happyAuthcodeRequestBody(authCode).ReadCloser()) req.Header.Set("Content-Type", "application/x-www-form-urlencoded") reusedAuthcodeResponse := httptest.NewRecorder() + approxRequestTime := time.Now() subject.ServeHTTP(reusedAuthcodeResponse, req) t.Logf("second response: %#v", reusedAuthcodeResponse) t.Logf("second response body: %q", reusedAuthcodeResponse.Body.String()) @@ -619,7 +823,7 @@ func TestTokenEndpointWhenAuthcodeIsUsedTwice(t *testing.T) { require.JSONEq(t, fositeReusedAuthCodeErrorBody, reusedAuthcodeResponse.Body.String()) // This was previously invalidated by the first request, so it remains invalidated - requireInvalidAuthCodeStorage(t, authCode, oauthStore, secrets) + requireInvalidAuthCodeStorage(t, authCode, oauthStore, secrets, approxRequestTime) // Has now invalidated the access token that was previously handed out by the first request requireInvalidAccessTokenStorage(t, parsedResponseBody, oauthStore) // This was previously invalidated by the first request, so it remains invalidated @@ -628,7 +832,9 @@ func TestTokenEndpointWhenAuthcodeIsUsedTwice(t *testing.T) { // Note that customSessionData is only relevant to refresh grant, so we leave it as nil for this // authcode exchange test, even though in practice it would actually be in the session. requireValidOIDCStorage(t, parsedResponseBody, authCode, oauthStore, - test.authcodeExchange.want.wantRequestedScopes, test.authcodeExchange.want.wantGrantedScopes, test.authcodeExchange.want.wantGroups, nil) + test.authcodeExchange.want.wantClientID, test.authcodeExchange.want.wantRequestedScopes, + test.authcodeExchange.want.wantGrantedScopes, test.authcodeExchange.want.wantGroups, nil, + approxRequestTime) // Check that the access token and refresh token storage were both deleted, and the number of other storage objects did not change. testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: authorizationcode.TypeLabelValue}, 1) @@ -636,7 +842,8 @@ func TestTokenEndpointWhenAuthcodeIsUsedTwice(t *testing.T) { testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: accesstoken.TypeLabelValue}, 0) testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: refreshtoken.TypeLabelValue}, 0) testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: storagepkce.TypeLabelValue}, 0) - testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{}, 2) + // Assert the number of all secrets, excluding any OIDCClient's storage secret, since those are not related to session storage. + testutil.RequireNumberOfSecretsExcludingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: oidcclientsecretstorage.TypeLabelValue}, 2) }) } } @@ -644,6 +851,16 @@ func TestTokenEndpointWhenAuthcodeIsUsedTwice(t *testing.T) { func TestTokenEndpointTokenExchange(t *testing.T) { // tests for grant_type "urn:ietf:params:oauth:grant-type:token-exchange" successfulAuthCodeExchange := tokenEndpointResponseExpectedValues{ wantStatus: http.StatusOK, + wantClientID: pinnipedCLIClientID, + wantSuccessBodyFields: []string{"id_token", "access_token", "token_type", "expires_in", "scope"}, + wantRequestedScopes: []string{"openid", "pinniped:request-audience", "groups"}, + wantGrantedScopes: []string{"openid", "pinniped:request-audience", "groups"}, + wantGroups: goodGroups, + } + + successfulAuthCodeExchangeUsingDynamicClient := tokenEndpointResponseExpectedValues{ + wantStatus: http.StatusOK, + wantClientID: dynamicClientID, wantSuccessBodyFields: []string{"id_token", "access_token", "token_type", "expires_in", "scope"}, wantRequestedScopes: []string{"openid", "pinniped:request-audience", "groups"}, wantGrantedScopes: []string{"openid", "pinniped:request-audience", "groups"}, @@ -657,13 +874,24 @@ func TestTokenEndpointTokenExchange(t *testing.T) { // tests for grant_type "urn want: successfulAuthCodeExchange, } + doValidAuthCodeExchangeUsingDynamicClient := authcodeExchangeInputs{ + modifyAuthRequest: func(authRequest *http.Request) { + addDynamicClientIDToFormPostBody(authRequest) + authRequest.Form.Set("scope", "openid pinniped:request-audience groups") + }, + modifyTokenRequest: modifyAuthcodeTokenRequestWithDynamicClientAuth, + want: successfulAuthCodeExchangeUsingDynamicClient, + } + tests := []struct { name string - authcodeExchange authcodeExchangeInputs - modifyRequestParams func(t *testing.T, params url.Values) - modifyStorage func(t *testing.T, storage *oidc.KubeStorage, pendingRequest *http.Request) - requestedAudience string + authcodeExchange authcodeExchangeInputs + modifyRequestParams func(t *testing.T, params url.Values) + modifyRequestHeaders func(r *http.Request) + modifyStorage func(t *testing.T, storage *oidc.KubeStorage, pendingRequest *http.Request) + requestedAudience string + kubeResources func(t *testing.T, supervisorClient *supervisorfake.Clientset, kubeClient *fake.Clientset) wantStatus int wantResponseBodyContains string @@ -674,6 +902,116 @@ func TestTokenEndpointTokenExchange(t *testing.T) { // tests for grant_type "urn requestedAudience: "some-workload-cluster", wantStatus: http.StatusOK, }, + { + name: "happy path with dynamic client", + kubeResources: addFullyCapableDynamicClientAndSecretToKubeResources, + authcodeExchange: doValidAuthCodeExchangeUsingDynamicClient, + modifyRequestParams: func(t *testing.T, params url.Values) { + params.Del("client_id") // client auth for dynamic clients must be in basic auth header + }, + modifyRequestHeaders: func(r *http.Request) { + r.SetBasicAuth(dynamicClientID, testutil.PlaintextPassword1) + }, + requestedAudience: "some-workload-cluster", + wantStatus: http.StatusOK, + }, + { + name: "dynamic client lacks the required urn:ietf:params:oauth:grant-type:token-exchange grant type", + kubeResources: func(t *testing.T, supervisorClient *supervisorfake.Clientset, kubeClient *fake.Clientset) { + namespace, clientID, clientUID, redirectURI := "some-namespace", dynamicClientID, dynamicClientUID, goodRedirectURI + oidcClient := &configv1alpha1.OIDCClient{ + ObjectMeta: metav1.ObjectMeta{Namespace: namespace, Name: clientID, Generation: 1, UID: types.UID(clientUID)}, + Spec: configv1alpha1.OIDCClientSpec{ + AllowedGrantTypes: []configv1alpha1.GrantType{"authorization_code", "refresh_token"}, // does not have the grant type + AllowedScopes: []configv1alpha1.Scope{"openid", "offline_access", "username", "groups"}, // would be invalid if it also asked for pinniped:request-audience since it lacks the grant type + AllowedRedirectURIs: []configv1alpha1.RedirectURI{configv1alpha1.RedirectURI(redirectURI)}, + }, + } + secret := testutil.OIDCClientSecretStorageSecretForUID(t, namespace, clientUID, []string{testutil.HashedPassword1AtGoMinCost, testutil.HashedPassword2AtGoMinCost}) + require.NoError(t, supervisorClient.Tracker().Add(oidcClient)) + require.NoError(t, kubeClient.Tracker().Add(secret)) + }, + authcodeExchange: authcodeExchangeInputs{ + modifyAuthRequest: func(authRequest *http.Request) { + addDynamicClientIDToFormPostBody(authRequest) + authRequest.Form.Set("scope", "openid groups") // don't request pinniped:request-audience scope + }, + modifyTokenRequest: modifyAuthcodeTokenRequestWithDynamicClientAuth, + want: tokenEndpointResponseExpectedValues{ + wantStatus: http.StatusOK, + wantClientID: dynamicClientID, + wantSuccessBodyFields: []string{"id_token", "access_token", "token_type", "expires_in", "scope"}, + wantRequestedScopes: []string{"openid", "groups"}, // don't want pinniped:request-audience scope + wantGrantedScopes: []string{"openid", "groups"}, // don't want pinniped:request-audience scope + wantGroups: goodGroups, + }, + }, + modifyRequestParams: func(t *testing.T, params url.Values) { + params.Del("client_id") // client auth for dynamic clients must be in basic auth header + }, + modifyRequestHeaders: func(r *http.Request) { + r.SetBasicAuth(dynamicClientID, testutil.PlaintextPassword1) + }, + requestedAudience: "some-workload-cluster", + wantStatus: http.StatusBadRequest, + wantResponseBodyContains: `The client is not authorized to request a token using this method. The OAuth 2.0 Client is not allowed to use token exchange grant 'urn:ietf:params:oauth:grant-type:token-exchange'.`, + }, + { + name: "dynamic client did not ask for the pinniped:request-audience scope in the original authorization request, so the access token submitted during token exchange lacks the scope", + kubeResources: addFullyCapableDynamicClientAndSecretToKubeResources, + authcodeExchange: authcodeExchangeInputs{ + modifyAuthRequest: func(authRequest *http.Request) { + addDynamicClientIDToFormPostBody(authRequest) + authRequest.Form.Set("scope", "openid groups") // don't request pinniped:request-audience scope + }, + modifyTokenRequest: modifyAuthcodeTokenRequestWithDynamicClientAuth, + want: tokenEndpointResponseExpectedValues{ + wantStatus: http.StatusOK, + wantClientID: dynamicClientID, + wantSuccessBodyFields: []string{"id_token", "access_token", "token_type", "expires_in", "scope"}, + wantRequestedScopes: []string{"openid", "groups"}, // don't want pinniped:request-audience scope + wantGrantedScopes: []string{"openid", "groups"}, // don't want pinniped:request-audience scope + wantGroups: goodGroups, + }, + }, + modifyRequestParams: func(t *testing.T, params url.Values) { + params.Del("client_id") // client auth for dynamic clients must be in basic auth header + }, + modifyRequestHeaders: func(r *http.Request) { + r.SetBasicAuth(dynamicClientID, testutil.PlaintextPassword1) + }, + requestedAudience: "some-workload-cluster", + wantStatus: http.StatusForbidden, + wantResponseBodyContains: `The resource owner or authorization server denied the request. missing the 'pinniped:request-audience' scope`, + }, + { + name: "dynamic client did not ask for the openid scope in the original authorization request, so the access token submitted during token exchange lacks the scope", + kubeResources: addFullyCapableDynamicClientAndSecretToKubeResources, + authcodeExchange: authcodeExchangeInputs{ + modifyAuthRequest: func(authRequest *http.Request) { + addDynamicClientIDToFormPostBody(authRequest) + authRequest.Form.Set("scope", "pinniped:request-audience groups") // don't request openid scope + }, + modifyTokenRequest: modifyAuthcodeTokenRequestWithDynamicClientAuth, + want: tokenEndpointResponseExpectedValues{ + wantStatus: http.StatusOK, + wantClientID: dynamicClientID, + wantSuccessBodyFields: []string{"access_token", "token_type", "expires_in", "scope"}, // no id token + wantRequestedScopes: []string{"pinniped:request-audience", "groups"}, // don't want openid scope + wantGrantedScopes: []string{"pinniped:request-audience", "groups"}, // don't want openid scope + wantGroups: nil, + }, + }, + modifyRequestParams: func(t *testing.T, params url.Values) { + params.Del("client_id") // client auth for dynamic clients must be in basic auth header + }, + modifyRequestHeaders: func(r *http.Request) { + r.SetBasicAuth(dynamicClientID, testutil.PlaintextPassword1) + }, + requestedAudience: "some-workload-cluster", + wantStatus: http.StatusForbidden, + wantResponseBodyContains: `The resource owner or authorization server denied the request. missing the 'openid' scope`, + }, { name: "missing audience", authcodeExchange: doValidAuthCodeExchange, @@ -752,6 +1090,60 @@ func TestTokenEndpointTokenExchange(t *testing.T) { // tests for grant_type "urn wantStatus: http.StatusBadRequest, wantResponseBodyContains: `Invalid token format`, }, + { + name: "bad client ID", + authcodeExchange: doValidAuthCodeExchange, + requestedAudience: "some-workload-cluster", + modifyRequestParams: func(t *testing.T, params url.Values) { + params.Set("client_id", "some-bogus-value") + }, + wantStatus: http.StatusUnauthorized, + wantResponseBodyContains: `Client authentication failed (e.g., unknown client, no client authentication included, or unsupported authentication method).`, + }, + { + name: "dynamic client uses wrong client secret", + kubeResources: addFullyCapableDynamicClientAndSecretToKubeResources, + authcodeExchange: doValidAuthCodeExchangeUsingDynamicClient, + modifyRequestParams: func(t *testing.T, params url.Values) { + params.Del("client_id") // client auth for dynamic clients must be in basic auth header + }, + modifyRequestHeaders: func(r *http.Request) { + r.SetBasicAuth(dynamicClientID, "bad client secret") + }, + requestedAudience: "some-workload-cluster", + wantStatus: http.StatusUnauthorized, + wantResponseBodyContains: `Client authentication failed (e.g., unknown client, no client authentication included, or unsupported authentication method).`, + }, + { + name: "dynamic client uses wrong auth method (must use basic auth)", + kubeResources: addFullyCapableDynamicClientAndSecretToKubeResources, + authcodeExchange: doValidAuthCodeExchangeUsingDynamicClient, + modifyRequestParams: func(t *testing.T, params url.Values) { + // Dynamic clients do not support this method of auth. + params.Set("client_id", dynamicClientID) + params.Set("client_secret", testutil.PlaintextPassword1) + }, + modifyRequestHeaders: func(r *http.Request) { + // would usually set the basic auth header here, but we don't for this test case + }, + requestedAudience: "some-workload-cluster", + wantStatus: http.StatusUnauthorized, + wantResponseBodyContains: `Client authentication failed (e.g., unknown client, no client authentication included, or unsupported authentication method). The OAuth 2.0 Client supports client authentication method 'client_secret_basic', but method 'client_secret_post' was requested. You must configure the OAuth 2.0 client's 'token_endpoint_auth_method' value to accept 'client_secret_post'.`, + }, + { + name: "different client used between authorize/authcode calls and the call to token exchange", + kubeResources: addFullyCapableDynamicClientAndSecretToKubeResources, + authcodeExchange: doValidAuthCodeExchange, // use pinniped-cli for authorize and authcode exchange + modifyRequestParams: func(t *testing.T, params url.Values) { + params.Del("client_id") // client auth for dynamic clients must be in basic auth header + }, + modifyRequestHeaders: func(r *http.Request) { + r.SetBasicAuth(dynamicClientID, testutil.PlaintextPassword1) // use dynamic client for token exchange + }, + requestedAudience: "some-workload-cluster", + wantStatus: http.StatusBadRequest, + wantResponseBodyContains: `The provided authorization grant (e.g., authorization code, resource owner credentials) or refresh token is invalid, expired, revoked, does not match the redirection URI used in the authorization request, or was issued to another client. The OAuth 2.0 Client ID from this request does not match the one from the authorize request.`, + }, { name: "valid access token, but deleted from storage", authcodeExchange: doValidAuthCodeExchange, @@ -772,6 +1164,7 @@ func TestTokenEndpointTokenExchange(t *testing.T) { // tests for grant_type "urn }, want: tokenEndpointResponseExpectedValues{ wantStatus: http.StatusOK, + wantClientID: pinnipedCLIClientID, wantSuccessBodyFields: []string{"id_token", "access_token", "token_type", "expires_in", "scope"}, wantRequestedScopes: []string{"openid", "groups"}, wantGrantedScopes: []string{"openid", "groups"}, @@ -790,10 +1183,11 @@ func TestTokenEndpointTokenExchange(t *testing.T) { // tests for grant_type "urn }, want: tokenEndpointResponseExpectedValues{ wantStatus: http.StatusOK, + wantClientID: pinnipedCLIClientID, wantSuccessBodyFields: []string{"access_token", "token_type", "expires_in", "scope"}, wantRequestedScopes: []string{"pinniped:request-audience", "groups"}, wantGrantedScopes: []string{"pinniped:request-audience", "groups"}, - wantGroups: goodGroups, + wantGroups: nil, }, }, requestedAudience: "some-workload-cluster", @@ -808,6 +1202,7 @@ func TestTokenEndpointTokenExchange(t *testing.T) { // tests for grant_type "urn }, want: tokenEndpointResponseExpectedValues{ wantStatus: http.StatusOK, + wantClientID: pinnipedCLIClientID, wantSuccessBodyFields: []string{"access_token", "token_type", "expires_in", "scope", "id_token"}, wantRequestedScopes: []string{"openid", "pinniped:request-audience"}, wantGrantedScopes: []string{"openid", "pinniped:request-audience"}, @@ -839,7 +1234,7 @@ func TestTokenEndpointTokenExchange(t *testing.T) { // tests for grant_type "urn // Authcode exchange doesn't use the upstream provider cache, so just pass an empty cache. subject, rsp, _, _, secrets, storage := exchangeAuthcodeForTokens(t, - test.authcodeExchange, oidctestutil.NewUpstreamIDPListerBuilder().Build()) + test.authcodeExchange, oidctestutil.NewUpstreamIDPListerBuilder().Build(), test.kubeResources) var parsedAuthcodeExchangeResponseBody map[string]interface{} require.NoError(t, json.Unmarshal(rsp.Body.Bytes(), &parsedAuthcodeExchangeResponseBody)) @@ -855,6 +1250,10 @@ func TestTokenEndpointTokenExchange(t *testing.T) { // tests for grant_type "urn req.Header.Set("Content-Type", "application/x-www-form-urlencoded") rsp = httptest.NewRecorder() + if test.modifyRequestHeaders != nil { + test.modifyRequestHeaders(req) + } + // Measure the secrets in storage after the auth code flow. existingSecrets, err := secrets.List(context.Background(), metav1.ListOptions{}) require.NoError(t, err) @@ -1062,6 +1461,7 @@ func TestRefreshGrant(t *testing.T) { happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess := func(wantCustomSessionDataStored *psession.CustomSessionData) tokenEndpointResponseExpectedValues { want := tokenEndpointResponseExpectedValues{ wantStatus: http.StatusOK, + wantClientID: pinnipedCLIClientID, wantSuccessBodyFields: []string{"id_token", "refresh_token", "access_token", "token_type", "expires_in", "scope"}, wantRequestedScopes: []string{"openid", "offline_access", "groups"}, wantGrantedScopes: []string{"openid", "offline_access", "groups"}, @@ -1071,6 +1471,16 @@ func TestRefreshGrant(t *testing.T) { return want } + withWantDynamicClientID := func(w tokenEndpointResponseExpectedValues) tokenEndpointResponseExpectedValues { + w.wantClientID = dynamicClientID + return w + } + + modifyRefreshTokenRequestWithDynamicClientAuth := func(tokenRequest *http.Request, refreshToken string, accessToken string) { + tokenRequest.Body = happyRefreshRequestBody(refreshToken).WithClientID("").ReadCloser() // No client_id in body. + tokenRequest.SetBasicAuth(dynamicClientID, testutil.PlaintextPassword1) // Use basic auth header instead. + } + happyRefreshTokenResponseForOpenIDAndOfflineAccess := func(wantCustomSessionDataStored *psession.CustomSessionData, expectToValidateToken *oauth2.Token) tokenEndpointResponseExpectedValues { // Should always have some custom session data stored. The other expectations happens to be the // same as the same values as the authcode exchange case. @@ -1134,6 +1544,7 @@ func TestRefreshGrant(t *testing.T) { tests := []struct { name string idps *oidctestutil.UpstreamIDPListerBuilder + kubeResources func(t *testing.T, supervisorClient *supervisorfake.Clientset, kubeClient *fake.Clientset) authcodeExchange authcodeExchangeInputs refreshRequest refreshRequestInputs modifyRefreshTokenStorage func(t *testing.T, oauthStore *oidc.KubeStorage, refreshToken string) @@ -1160,6 +1571,34 @@ func TestRefreshGrant(t *testing.T) { ), }, }, + { + name: "happy path refresh grant with openid scope granted (id token returned) using dynamic client", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC( + upstreamOIDCIdentityProviderBuilder().WithValidatedAndMergedWithUserInfoTokens(&oidctypes.Token{ + IDToken: &oidctypes.IDToken{ + Claims: map[string]interface{}{ + "sub": goodUpstreamSubject, + }, + }, + }).WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()), + kubeResources: addFullyCapableDynamicClientAndSecretToKubeResources, + authcodeExchange: authcodeExchangeInputs{ + customSessionData: initialUpstreamOIDCRefreshTokenCustomSessionData(), + modifyAuthRequest: func(r *http.Request) { + addDynamicClientIDToFormPostBody(r) + r.Form.Set("scope", "openid offline_access groups") + }, + modifyTokenRequest: modifyAuthcodeTokenRequestWithDynamicClientAuth, + want: withWantDynamicClientID(happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCRefreshTokenCustomSessionData())), + }, + refreshRequest: refreshRequestInputs{ + modifyTokenRequest: modifyRefreshTokenRequestWithDynamicClientAuth, + want: withWantDynamicClientID(happyRefreshTokenResponseForOpenIDAndOfflineAccess( + upstreamOIDCCustomSessionDataWithNewRefreshToken(oidcUpstreamRefreshedRefreshToken), + refreshedUpstreamTokensWithIDAndRefreshTokens(), + )), + }, + }, { name: "refresh grant with unchanged username claim", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC( @@ -1208,6 +1647,7 @@ func TestRefreshGrant(t *testing.T) { refreshRequest: refreshRequestInputs{ want: tokenEndpointResponseExpectedValues{ wantStatus: http.StatusOK, + wantClientID: pinnipedCLIClientID, wantSuccessBodyFields: []string{"refresh_token", "id_token", "access_token", "token_type", "expires_in", "scope"}, wantRequestedScopes: []string{"openid", "offline_access", "groups"}, wantGrantedScopes: []string{"openid", "offline_access", "groups"}, @@ -1239,6 +1679,7 @@ func TestRefreshGrant(t *testing.T) { modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "offline_access") }, want: tokenEndpointResponseExpectedValues{ wantStatus: http.StatusOK, + wantClientID: pinnipedCLIClientID, wantSuccessBodyFields: []string{"refresh_token", "access_token", "token_type", "expires_in", "scope"}, wantRequestedScopes: []string{"offline_access"}, wantGrantedScopes: []string{"offline_access"}, @@ -1248,6 +1689,7 @@ func TestRefreshGrant(t *testing.T) { refreshRequest: refreshRequestInputs{ want: tokenEndpointResponseExpectedValues{ wantStatus: http.StatusOK, + wantClientID: pinnipedCLIClientID, wantSuccessBodyFields: []string{"refresh_token", "access_token", "token_type", "expires_in", "scope"}, wantRequestedScopes: []string{"offline_access"}, wantGrantedScopes: []string{"offline_access"}, @@ -1273,6 +1715,7 @@ func TestRefreshGrant(t *testing.T) { refreshRequest: refreshRequestInputs{ want: tokenEndpointResponseExpectedValues{ wantStatus: http.StatusOK, + wantClientID: pinnipedCLIClientID, wantSuccessBodyFields: []string{"refresh_token", "access_token", "id_token", "token_type", "expires_in", "scope"}, wantRequestedScopes: []string{"openid", "offline_access", "groups"}, wantGrantedScopes: []string{"openid", "offline_access", "groups"}, @@ -1302,6 +1745,7 @@ func TestRefreshGrant(t *testing.T) { refreshRequest: refreshRequestInputs{ want: tokenEndpointResponseExpectedValues{ wantStatus: http.StatusOK, + wantClientID: pinnipedCLIClientID, wantSuccessBodyFields: []string{"refresh_token", "access_token", "id_token", "token_type", "expires_in", "scope"}, wantRequestedScopes: []string{"openid", "offline_access", "groups"}, wantGrantedScopes: []string{"openid", "offline_access", "groups"}, @@ -1331,6 +1775,7 @@ func TestRefreshGrant(t *testing.T) { refreshRequest: refreshRequestInputs{ want: tokenEndpointResponseExpectedValues{ wantStatus: http.StatusOK, + wantClientID: pinnipedCLIClientID, wantSuccessBodyFields: []string{"refresh_token", "access_token", "id_token", "token_type", "expires_in", "scope"}, wantRequestedScopes: []string{"openid", "offline_access", "groups"}, wantGrantedScopes: []string{"openid", "offline_access", "groups"}, @@ -1360,6 +1805,7 @@ func TestRefreshGrant(t *testing.T) { refreshRequest: refreshRequestInputs{ want: tokenEndpointResponseExpectedValues{ wantStatus: http.StatusOK, + wantClientID: pinnipedCLIClientID, wantSuccessBodyFields: []string{"refresh_token", "access_token", "id_token", "token_type", "expires_in", "scope"}, wantRequestedScopes: []string{"openid", "offline_access", "groups"}, wantGrantedScopes: []string{"openid", "offline_access", "groups"}, @@ -1389,6 +1835,7 @@ func TestRefreshGrant(t *testing.T) { refreshRequest: refreshRequestInputs{ want: tokenEndpointResponseExpectedValues{ wantStatus: http.StatusOK, + wantClientID: pinnipedCLIClientID, wantSuccessBodyFields: []string{"refresh_token", "access_token", "id_token", "token_type", "expires_in", "scope"}, wantRequestedScopes: []string{"openid", "offline_access", "groups"}, wantGrantedScopes: []string{"openid", "offline_access", "groups"}, @@ -1417,6 +1864,7 @@ func TestRefreshGrant(t *testing.T) { refreshRequest: refreshRequestInputs{ want: tokenEndpointResponseExpectedValues{ wantStatus: http.StatusOK, + wantClientID: pinnipedCLIClientID, wantSuccessBodyFields: []string{"refresh_token", "access_token", "id_token", "token_type", "expires_in", "scope"}, wantRequestedScopes: []string{"openid", "offline_access", "groups"}, wantGrantedScopes: []string{"openid", "offline_access", "groups"}, @@ -1444,6 +1892,7 @@ func TestRefreshGrant(t *testing.T) { refreshRequest: refreshRequestInputs{ want: tokenEndpointResponseExpectedValues{ wantStatus: http.StatusOK, + wantClientID: pinnipedCLIClientID, wantSuccessBodyFields: []string{"refresh_token", "access_token", "id_token", "token_type", "expires_in", "scope"}, wantRequestedScopes: []string{"openid", "offline_access", "groups"}, wantGrantedScopes: []string{"openid", "offline_access", "groups"}, @@ -1466,6 +1915,7 @@ func TestRefreshGrant(t *testing.T) { customSessionData: happyLDAPCustomSessionData, want: tokenEndpointResponseExpectedValues{ wantStatus: http.StatusOK, + wantClientID: pinnipedCLIClientID, wantSuccessBodyFields: []string{"id_token", "refresh_token", "access_token", "token_type", "expires_in", "scope"}, wantRequestedScopes: []string{"openid", "offline_access"}, wantGrantedScopes: []string{"openid", "offline_access"}, @@ -1479,6 +1929,7 @@ func TestRefreshGrant(t *testing.T) { }, want: tokenEndpointResponseExpectedValues{ wantStatus: http.StatusOK, + wantClientID: pinnipedCLIClientID, wantSuccessBodyFields: []string{"refresh_token", "access_token", "id_token", "token_type", "expires_in", "scope"}, wantRequestedScopes: []string{"openid", "offline_access"}, wantGrantedScopes: []string{"openid", "offline_access"}, @@ -1504,6 +1955,7 @@ func TestRefreshGrant(t *testing.T) { customSessionData: initialUpstreamOIDCRefreshTokenCustomSessionData(), want: tokenEndpointResponseExpectedValues{ wantStatus: http.StatusOK, + wantClientID: pinnipedCLIClientID, wantSuccessBodyFields: []string{"id_token", "refresh_token", "access_token", "token_type", "expires_in", "scope"}, wantRequestedScopes: []string{"openid", "offline_access"}, wantGrantedScopes: []string{"openid", "offline_access"}, @@ -1517,6 +1969,54 @@ func TestRefreshGrant(t *testing.T) { }, want: tokenEndpointResponseExpectedValues{ wantStatus: http.StatusOK, + wantClientID: pinnipedCLIClientID, + wantSuccessBodyFields: []string{"refresh_token", "access_token", "id_token", "token_type", "expires_in", "scope"}, + wantRequestedScopes: []string{"openid", "offline_access"}, + wantGrantedScopes: []string{"openid", "offline_access"}, + wantGroups: nil, + wantUpstreamRefreshCall: happyOIDCUpstreamRefreshCall(), + wantUpstreamOIDCValidateTokenCall: happyUpstreamValidateTokenCall(refreshedUpstreamTokensWithIDAndRefreshTokens(), true), + wantCustomSessionDataStored: upstreamOIDCCustomSessionDataWithNewRefreshToken(oidcUpstreamRefreshedRefreshToken), + }, + }, + }, + { + name: "oidc refresh grant when the upstream refresh when groups scope not requested on original request or refresh when using dynamic client", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC( + upstreamOIDCIdentityProviderBuilder().WithGroupsClaim("my-groups-claim").WithValidatedAndMergedWithUserInfoTokens(&oidctypes.Token{ + IDToken: &oidctypes.IDToken{ + Claims: map[string]interface{}{ + "sub": goodUpstreamSubject, + "my-groups-claim": []string{"new-group1", "new-group2", "new-group3"}, // refreshed claims includes updated groups + }, + }, + }).WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()), + kubeResources: addFullyCapableDynamicClientAndSecretToKubeResources, + authcodeExchange: authcodeExchangeInputs{ + modifyAuthRequest: func(r *http.Request) { + addDynamicClientIDToFormPostBody(r) + r.Form.Set("scope", "openid offline_access") + }, + modifyTokenRequest: modifyAuthcodeTokenRequestWithDynamicClientAuth, + customSessionData: initialUpstreamOIDCRefreshTokenCustomSessionData(), + want: tokenEndpointResponseExpectedValues{ + wantStatus: http.StatusOK, + wantClientID: dynamicClientID, + wantSuccessBodyFields: []string{"id_token", "refresh_token", "access_token", "token_type", "expires_in", "scope"}, + wantRequestedScopes: []string{"openid", "offline_access"}, + wantGrantedScopes: []string{"openid", "offline_access"}, + wantCustomSessionDataStored: initialUpstreamOIDCRefreshTokenCustomSessionData(), + wantGroups: nil, + }, + }, + refreshRequest: refreshRequestInputs{ + modifyTokenRequest: func(r *http.Request, refreshToken string, accessToken string) { + r.Body = happyRefreshRequestBody(refreshToken).WithClientID("").WithScope("openid offline_access").ReadCloser() + r.SetBasicAuth(dynamicClientID, testutil.PlaintextPassword1) // Use basic auth header instead. + }, + want: tokenEndpointResponseExpectedValues{ + wantStatus: http.StatusOK, + wantClientID: dynamicClientID, wantSuccessBodyFields: []string{"refresh_token", "access_token", "id_token", "token_type", "expires_in", "scope"}, wantRequestedScopes: []string{"openid", "offline_access"}, wantGrantedScopes: []string{"openid", "offline_access"}, @@ -1542,6 +2042,7 @@ func TestRefreshGrant(t *testing.T) { customSessionData: happyLDAPCustomSessionData, want: tokenEndpointResponseExpectedValues{ wantStatus: http.StatusOK, + wantClientID: pinnipedCLIClientID, wantSuccessBodyFields: []string{"id_token", "refresh_token", "access_token", "token_type", "expires_in", "scope"}, wantRequestedScopes: []string{"openid", "offline_access", "groups"}, wantGrantedScopes: []string{"openid", "offline_access", "groups"}, @@ -1555,6 +2056,7 @@ func TestRefreshGrant(t *testing.T) { }, want: tokenEndpointResponseExpectedValues{ wantStatus: http.StatusOK, + wantClientID: pinnipedCLIClientID, wantSuccessBodyFields: []string{"refresh_token", "access_token", "id_token", "token_type", "expires_in", "scope"}, wantRequestedScopes: []string{"openid", "offline_access", "groups"}, wantGrantedScopes: []string{"openid", "offline_access", "groups"}, @@ -1651,6 +2153,7 @@ func TestRefreshGrant(t *testing.T) { modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access pinniped:request-audience groups") }, want: tokenEndpointResponseExpectedValues{ wantStatus: http.StatusOK, + wantClientID: pinnipedCLIClientID, wantSuccessBodyFields: []string{"id_token", "refresh_token", "access_token", "token_type", "expires_in", "scope"}, wantRequestedScopes: []string{"openid", "offline_access", "pinniped:request-audience", "groups"}, wantGrantedScopes: []string{"openid", "offline_access", "pinniped:request-audience", "groups"}, @@ -1664,6 +2167,7 @@ func TestRefreshGrant(t *testing.T) { }, want: tokenEndpointResponseExpectedValues{ wantStatus: http.StatusOK, + wantClientID: pinnipedCLIClientID, wantSuccessBodyFields: []string{"id_token", "refresh_token", "access_token", "token_type", "expires_in", "scope"}, wantRequestedScopes: []string{"openid", "offline_access", "pinniped:request-audience", "groups"}, wantGrantedScopes: []string{"openid", "offline_access", "pinniped:request-audience", "groups"}, @@ -1707,6 +2211,7 @@ func TestRefreshGrant(t *testing.T) { modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "offline_access") }, want: tokenEndpointResponseExpectedValues{ wantStatus: http.StatusOK, + wantClientID: pinnipedCLIClientID, wantSuccessBodyFields: []string{"refresh_token", "access_token", "token_type", "expires_in", "scope"}, wantRequestedScopes: []string{"offline_access"}, wantGrantedScopes: []string{"offline_access"}, @@ -1731,6 +2236,7 @@ func TestRefreshGrant(t *testing.T) { modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "offline_access") }, want: tokenEndpointResponseExpectedValues{ wantStatus: http.StatusOK, + wantClientID: pinnipedCLIClientID, wantSuccessBodyFields: []string{"refresh_token", "access_token", "token_type", "expires_in", "scope"}, wantRequestedScopes: []string{"offline_access"}, wantGrantedScopes: []string{"offline_access"}, @@ -1755,6 +2261,7 @@ func TestRefreshGrant(t *testing.T) { modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "offline_access") }, want: tokenEndpointResponseExpectedValues{ wantStatus: http.StatusOK, + wantClientID: pinnipedCLIClientID, wantSuccessBodyFields: []string{"refresh_token", "access_token", "token_type", "expires_in", "scope"}, wantRequestedScopes: []string{"offline_access"}, wantGrantedScopes: []string{"offline_access"}, @@ -1771,6 +2278,96 @@ func TestRefreshGrant(t *testing.T) { }, }, }, + { + name: "when the refresh request uses a different client than the one that was used to get the refresh token", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC( + upstreamOIDCIdentityProviderBuilder().WithValidatedAndMergedWithUserInfoTokens(&oidctypes.Token{ + IDToken: &oidctypes.IDToken{ + Claims: map[string]interface{}{ + "sub": goodUpstreamSubject, + }, + }, + }).WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()), + kubeResources: addFullyCapableDynamicClientAndSecretToKubeResources, + authcodeExchange: authcodeExchangeInputs{ + // Make the auth request and authcode exchange request using the pinniped-cli client. + customSessionData: initialUpstreamOIDCRefreshTokenCustomSessionData(), + modifyAuthRequest: func(r *http.Request) { + r.Form.Set("scope", "openid offline_access groups") + }, + want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCRefreshTokenCustomSessionData()), + }, + refreshRequest: refreshRequestInputs{ + // Make the refresh request with the dynamic client. + modifyTokenRequest: modifyRefreshTokenRequestWithDynamicClientAuth, + want: tokenEndpointResponseExpectedValues{ + wantStatus: http.StatusBadRequest, + wantErrorResponseBody: fositeClientIDMismatchDuringRefreshErrorBody, + }, + }, + }, + { + name: "when the client auth fails on the refresh request using dynamic client", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC( + upstreamOIDCIdentityProviderBuilder().WithValidatedAndMergedWithUserInfoTokens(&oidctypes.Token{ + IDToken: &oidctypes.IDToken{ + Claims: map[string]interface{}{ + "sub": goodUpstreamSubject, + }, + }, + }).WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()), + kubeResources: addFullyCapableDynamicClientAndSecretToKubeResources, + authcodeExchange: authcodeExchangeInputs{ + customSessionData: initialUpstreamOIDCRefreshTokenCustomSessionData(), + modifyAuthRequest: func(r *http.Request) { + addDynamicClientIDToFormPostBody(r) + r.Form.Set("scope", "openid offline_access groups") + }, + modifyTokenRequest: modifyAuthcodeTokenRequestWithDynamicClientAuth, + want: withWantDynamicClientID(happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCRefreshTokenCustomSessionData())), + }, + refreshRequest: refreshRequestInputs{ + modifyTokenRequest: func(tokenRequest *http.Request, refreshToken string, accessToken string) { + tokenRequest.Body = happyRefreshRequestBody(refreshToken).WithClientID("").ReadCloser() + tokenRequest.SetBasicAuth(dynamicClientID, "wrong client secret") + }, + want: tokenEndpointResponseExpectedValues{ + wantStatus: http.StatusUnauthorized, + wantErrorResponseBody: fositeClientAuthFailedErrorBody, + }, + }, + }, + { + name: "dynamic client uses wrong auth method on the refresh request (must use basic auth)", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC( + upstreamOIDCIdentityProviderBuilder().WithValidatedAndMergedWithUserInfoTokens(&oidctypes.Token{ + IDToken: &oidctypes.IDToken{ + Claims: map[string]interface{}{ + "sub": goodUpstreamSubject, + }, + }, + }).WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()), + kubeResources: addFullyCapableDynamicClientAndSecretToKubeResources, + authcodeExchange: authcodeExchangeInputs{ + customSessionData: initialUpstreamOIDCRefreshTokenCustomSessionData(), + modifyAuthRequest: func(r *http.Request) { + addDynamicClientIDToFormPostBody(r) + r.Form.Set("scope", "openid offline_access groups") + }, + modifyTokenRequest: modifyAuthcodeTokenRequestWithDynamicClientAuth, + want: withWantDynamicClientID(happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCRefreshTokenCustomSessionData())), + }, + refreshRequest: refreshRequestInputs{ + modifyTokenRequest: func(tokenRequest *http.Request, refreshToken string, accessToken string) { + // Add client auth to the form, when it should be in basic auth headers. + tokenRequest.Body = happyRefreshRequestBody(refreshToken).WithClientID(dynamicClientID).WithClientSecret(testutil.PlaintextPassword1).ReadCloser() + }, + want: tokenEndpointResponseExpectedValues{ + wantStatus: http.StatusUnauthorized, + wantErrorResponseBody: fositeClientAuthMustBeBasicAuthErrorBody, + }, + }, + }, { name: "when there is no custom session data found in the session storage during the refresh request", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(upstreamOIDCIdentityProviderBuilder().Build()), @@ -2920,7 +3517,8 @@ func TestRefreshGrant(t *testing.T) { // First exchange the authcode for tokens, including a refresh token. // its actually fine to use this function even when simulating ldap (which uses a different flow) because it's // just populating a secret in storage. - subject, rsp, authCode, jwtSigningKey, secrets, oauthStore := exchangeAuthcodeForTokens(t, test.authcodeExchange, test.idps.Build()) + subject, rsp, authCode, jwtSigningKey, secrets, oauthStore := exchangeAuthcodeForTokens(t, + test.authcodeExchange, test.idps.Build(), test.kubeResources) var parsedAuthcodeExchangeResponseBody map[string]interface{} require.NoError(t, json.Unmarshal(rsp.Body.Bytes(), &parsedAuthcodeExchangeResponseBody)) @@ -2951,6 +3549,7 @@ func TestRefreshGrant(t *testing.T) { } refreshResponse := httptest.NewRecorder() + approxRequestTime := time.Now() subject.ServeHTTP(refreshResponse, req) t.Logf("second response: %#v", refreshResponse) t.Logf("second response body: %q", refreshResponse.Body.String()) @@ -2994,6 +3593,7 @@ func TestRefreshGrant(t *testing.T) { oauthStore, jwtSigningKey, secrets, + approxRequestTime, ) if test.refreshRequest.want.wantStatus == http.StatusOK { @@ -3054,7 +3654,12 @@ func requireClaimsAreEqual(t *testing.T, claimName string, claimsOfTokenA map[st require.Equal(t, claimsOfTokenA[claimName], claimsOfTokenB[claimName]) } -func exchangeAuthcodeForTokens(t *testing.T, test authcodeExchangeInputs, idps provider.DynamicUpstreamIDPProvider) ( +func exchangeAuthcodeForTokens( + t *testing.T, + test authcodeExchangeInputs, + idps provider.DynamicUpstreamIDPProvider, + kubeResources func(t *testing.T, supervisorClient *supervisorfake.Clientset, kubeClient *fake.Clientset), +) ( subject http.Handler, rsp *httptest.ResponseRecorder, authCode string, @@ -3067,13 +3672,18 @@ func exchangeAuthcodeForTokens(t *testing.T, test authcodeExchangeInputs, idps p test.modifyAuthRequest(authRequest) } - client := fake.NewSimpleClientset() - secrets = client.CoreV1().Secrets("some-namespace") - oidcClientsClient := supervisorfake.NewSimpleClientset().ConfigV1alpha1().OIDCClients("some-namespace") + kubeClient := fake.NewSimpleClientset() + supervisorClient := supervisorfake.NewSimpleClientset() + secrets = kubeClient.CoreV1().Secrets("some-namespace") + oidcClientsClient := supervisorClient.ConfigV1alpha1().OIDCClients("some-namespace") + + if kubeResources != nil { + kubeResources(t, supervisorClient, kubeClient) + } var oauthHelper fosite.OAuth2Provider - - oauthStore = oidc.NewKubeStorage(secrets, oidcClientsClient, oidc.DefaultOIDCTimeoutsConfiguration()) + // Use lower minimum required bcrypt cost than we would use in production to keep unit the tests fast. + oauthStore = oidc.NewKubeStorage(secrets, oidcClientsClient, oidc.DefaultOIDCTimeoutsConfiguration(), bcrypt.MinCost) if test.makeOathHelper != nil { oauthHelper, authCode, jwtSigningKey = test.makeOathHelper(t, authRequest, oauthStore, test.customSessionData) } else { @@ -3096,7 +3706,8 @@ func exchangeAuthcodeForTokens(t *testing.T, test authcodeExchangeInputs, idps p testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: authorizationcode.TypeLabelValue}, 1) testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: storagepkce.TypeLabelValue}, 1) testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: openidconnect.TypeLabelValue}, expectedNumberOfIDSessionsStored) - testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{}, 2+expectedNumberOfIDSessionsStored) + // Assert the number of all secrets, excluding any OIDCClient's storage secret, since those are not related to session storage. + testutil.RequireNumberOfSecretsExcludingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: oidcclientsecretstorage.TypeLabelValue}, 2+expectedNumberOfIDSessionsStored) req := httptest.NewRequest("POST", "/path/shouldn't/matter", happyAuthcodeRequestBody(authCode).ReadCloser()) req.Header.Set("Content-Type", "application/x-www-form-urlencoded") @@ -3105,12 +3716,13 @@ func exchangeAuthcodeForTokens(t *testing.T, test authcodeExchangeInputs, idps p } rsp = httptest.NewRecorder() + approxRequestTime := time.Now() subject.ServeHTTP(rsp, req) t.Logf("response: %#v", rsp) t.Logf("response body: %q", rsp.Body.String()) wantAtHashClaimInIDToken := false // due to a bug in fosite, the at_hash claim is not filled in during authcode exchange - wantNonceValueInIDToken := true // ID tokens returned by the authcode exchange must include the nonce from the auth request (unliked refreshed ID tokens) + wantNonceValueInIDToken := true // ID tokens returned by the authcode exchange must include the nonce from the auth request (unlike refreshed ID tokens) requireTokenEndpointBehavior(t, test.want, @@ -3123,6 +3735,7 @@ func exchangeAuthcodeForTokens(t *testing.T, test authcodeExchangeInputs, idps p oauthStore, jwtSigningKey, secrets, + approxRequestTime, ) return subject, rsp, authCode, jwtSigningKey, secrets, oauthStore @@ -3140,6 +3753,7 @@ func requireTokenEndpointBehavior( oauthStore *oidc.KubeStorage, jwtSigningKey *ecdsa.PrivateKey, secrets v1.SecretInterface, + requestTime time.Time, ) { testutil.RequireEqualContentType(t, tokenEndpointResponse.Header().Get("Content-Type"), "application/json") require.Equal(t, test.wantStatus, tokenEndpointResponse.Code) @@ -3154,11 +3768,11 @@ func requireTokenEndpointBehavior( wantIDToken := contains(test.wantSuccessBodyFields, "id_token") wantRefreshToken := contains(test.wantSuccessBodyFields, "refresh_token") - requireInvalidAuthCodeStorage(t, authCode, oauthStore, secrets) - requireValidAccessTokenStorage(t, parsedResponseBody, oauthStore, test.wantRequestedScopes, test.wantGrantedScopes, test.wantGroups, test.wantCustomSessionDataStored, secrets) + requireInvalidAuthCodeStorage(t, authCode, oauthStore, secrets, requestTime) + requireValidAccessTokenStorage(t, parsedResponseBody, oauthStore, test.wantClientID, test.wantRequestedScopes, test.wantGrantedScopes, test.wantGroups, test.wantCustomSessionDataStored, secrets, requestTime) requireInvalidPKCEStorage(t, authCode, oauthStore) // Performing a refresh does not update the OIDC storage, so after a refresh it should still have the old custom session data and old groups from the initial login. - requireValidOIDCStorage(t, parsedResponseBody, authCode, oauthStore, test.wantRequestedScopes, test.wantGrantedScopes, oldGroups, oldCustomSessionData) + requireValidOIDCStorage(t, parsedResponseBody, authCode, oauthStore, test.wantClientID, test.wantRequestedScopes, test.wantGrantedScopes, oldGroups, oldCustomSessionData, requestTime) expectedNumberOfRefreshTokenSessionsStored := 0 if wantRefreshToken { @@ -3167,10 +3781,10 @@ func requireTokenEndpointBehavior( expectedNumberOfIDSessionsStored := 0 if wantIDToken { expectedNumberOfIDSessionsStored = 1 - requireValidIDToken(t, parsedResponseBody, jwtSigningKey, wantAtHashClaimInIDToken, wantNonceValueInIDToken, test.wantGroups, parsedResponseBody["access_token"].(string)) + requireValidIDToken(t, parsedResponseBody, jwtSigningKey, test.wantClientID, wantAtHashClaimInIDToken, wantNonceValueInIDToken, test.wantGroups, parsedResponseBody["access_token"].(string), requestTime) } if wantRefreshToken { - requireValidRefreshTokenStorage(t, parsedResponseBody, oauthStore, test.wantRequestedScopes, test.wantGrantedScopes, test.wantGroups, test.wantCustomSessionDataStored, secrets) + requireValidRefreshTokenStorage(t, parsedResponseBody, oauthStore, test.wantClientID, test.wantRequestedScopes, test.wantGrantedScopes, test.wantGroups, test.wantCustomSessionDataStored, secrets, requestTime) } testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: authorizationcode.TypeLabelValue}, 1) @@ -3178,7 +3792,8 @@ func requireTokenEndpointBehavior( testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: storagepkce.TypeLabelValue}, 0) testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: refreshtoken.TypeLabelValue}, expectedNumberOfRefreshTokenSessionsStored) testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: openidconnect.TypeLabelValue}, expectedNumberOfIDSessionsStored) - testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{}, 2+expectedNumberOfRefreshTokenSessionsStored+expectedNumberOfIDSessionsStored) + // Assert the number of all secrets, excluding any OIDCClient's storage secret, since those are not related to session storage. + testutil.RequireNumberOfSecretsExcludingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: oidcclientsecretstorage.TypeLabelValue}, 2+expectedNumberOfRefreshTokenSessionsStored+expectedNumberOfIDSessionsStored) } else { require.NotNil(t, test.wantErrorResponseBody, "problem with test table setup: wanted failure but did not specify failure response body") @@ -3204,7 +3819,7 @@ func happyAuthcodeRequestBody(happyAuthCode string) body { "code": {happyAuthCode}, "redirect_uri": {goodRedirectURI}, "code_verifier": {goodPKCECodeVerifier}, - "client_id": {goodClient}, + "client_id": {pinnipedCLIClientID}, } } @@ -3212,7 +3827,7 @@ func happyRefreshRequestBody(refreshToken string) body { return map[string][]string{ "grant_type": {"refresh_token"}, "scope": {"openid"}, - "client_id": {goodClient}, + "client_id": {pinnipedCLIClientID}, "refresh_token": {refreshToken}, } } @@ -3229,6 +3844,10 @@ func (b body) WithClientID(clientID string) body { return b.with("client_id", clientID) } +func (b body) WithClientSecret(clientSecret string) body { + return b.with("client_secret", clientSecret) +} + func (b body) WithAuthCode(code string) body { return b.with("code", code) } @@ -3395,6 +4014,7 @@ func requireInvalidAuthCodeStorage( code string, storage fositeoauth2.CoreStorage, secrets v1.SecretInterface, + requestTime time.Time, ) { t.Helper() @@ -3402,18 +4022,20 @@ func requireInvalidAuthCodeStorage( _, err := storage.GetAuthorizeCodeSession(context.Background(), getFositeDataSignature(t, code), nil) require.True(t, errors.Is(err, fosite.ErrInvalidatedAuthorizeCode)) // make sure that its still around in storage so if someone tries to use it again we invalidate everything - requireGarbageCollectTimeInDelta(t, code, "authcode", secrets, time.Now().Add(9*time.Hour).Add(10*time.Minute), 30*time.Second) + requireGarbageCollectTimeInDelta(t, code, "authcode", secrets, requestTime.Add(9*time.Hour).Add(10*time.Minute), 30*time.Second) } func requireValidRefreshTokenStorage( t *testing.T, body map[string]interface{}, storage fositeoauth2.CoreStorage, + wantClientID string, wantRequestedScopes []string, wantGrantedScopes []string, wantGroups []string, wantCustomSessionData *psession.CustomSessionData, secrets v1.SecretInterface, + requestTime time.Time, ) { t.Helper() @@ -3434,25 +4056,29 @@ func requireValidRefreshTokenStorage( t, storedRequest, storedRequest.Sanitize([]string{}).GetRequestForm(), + wantClientID, wantRequestedScopes, wantGrantedScopes, true, wantGroups, wantCustomSessionData, + requestTime, ) - requireGarbageCollectTimeInDelta(t, refreshTokenString, "refresh-token", secrets, time.Now().Add(9*time.Hour).Add(2*time.Minute), 1*time.Minute) + requireGarbageCollectTimeInDelta(t, refreshTokenString, "refresh-token", secrets, requestTime.Add(9*time.Hour).Add(2*time.Minute), 1*time.Minute) } func requireValidAccessTokenStorage( t *testing.T, body map[string]interface{}, storage fositeoauth2.CoreStorage, + wantClientID string, wantRequestedScopes []string, wantGrantedScopes []string, wantGroups []string, wantCustomSessionData *psession.CustomSessionData, secrets v1.SecretInterface, + requestTime time.Time, ) { t.Helper() @@ -3492,14 +4118,16 @@ func requireValidAccessTokenStorage( t, storedRequest, storedRequest.Sanitize([]string{}).GetRequestForm(), + wantClientID, wantRequestedScopes, wantGrantedScopes, true, wantGroups, wantCustomSessionData, + requestTime, ) - requireGarbageCollectTimeInDelta(t, accessTokenString, "access-token", secrets, time.Now().Add(9*time.Hour).Add(2*time.Minute), 1*time.Minute) + requireGarbageCollectTimeInDelta(t, accessTokenString, "access-token", secrets, requestTime.Add(9*time.Hour).Add(2*time.Minute), 1*time.Minute) } func requireInvalidAccessTokenStorage( @@ -3536,10 +4164,12 @@ func requireValidOIDCStorage( body map[string]interface{}, code string, storage openid.OpenIDConnectRequestStorage, + wantClientID string, wantRequestedScopes []string, wantGrantedScopes []string, wantGroups []string, wantCustomSessionData *psession.CustomSessionData, + requestTime time.Time, ) { t.Helper() @@ -3559,11 +4189,13 @@ func requireValidOIDCStorage( t, storedRequest, storedRequest.Sanitize([]string{"nonce"}).GetRequestForm(), + wantClientID, wantRequestedScopes, wantGrantedScopes, false, wantGroups, wantCustomSessionData, + requestTime, ) } else { _, err := storage.GetOpenIDConnectSession(context.Background(), code, nil) @@ -3575,18 +4207,20 @@ func requireValidStoredRequest( t *testing.T, request fosite.Requester, wantRequestForm url.Values, + wantClientID string, wantRequestedScopes []string, wantGrantedScopes []string, wantAccessTokenExpiresAt bool, wantGroups []string, wantCustomSessionData *psession.CustomSessionData, + requestTime time.Time, ) { t.Helper() // Assert that the getters on the request return what we think they should. require.NotEmpty(t, request.GetID()) - testutil.RequireTimeInDelta(t, request.GetRequestedAt(), time.Now().UTC(), timeComparisonFudgeSeconds*time.Second) - require.Equal(t, goodClient, request.GetClient().GetID()) + testutil.RequireTimeInDelta(t, request.GetRequestedAt(), requestTime.UTC(), timeComparisonFudge) + require.Equal(t, wantClientID, request.GetClient().GetID()) require.Equal(t, fosite.Arguments(wantRequestedScopes), request.GetRequestedScopes()) require.Equal(t, fosite.Arguments(wantGrantedScopes), request.GetGrantedScopes()) require.Empty(t, request.GetRequestedAudience()) @@ -3636,6 +4270,9 @@ func requireValidStoredRequest( require.Empty(t, claims.AuthenticationContextClassReference) require.Empty(t, claims.AuthenticationMethodsReferences) require.Empty(t, claims.CodeHash) + } else if wantGroups != nil { + t.Fatal("test did not want the openid scope to be granted, but also wanted groups, " + + "which is a combination that doesn't make sense since you need an ID token to get groups") } // Assert that the session headers are what we think they should be. @@ -3647,9 +4284,9 @@ func requireValidStoredRequest( require.True(t, ok, "expected session to hold expiration time for auth code") testutil.RequireTimeInDelta( t, - time.Now().UTC().Add(authCodeExpirationSeconds*time.Second), + requestTime.UTC().Add(authCodeExpirationSeconds*time.Second), authCodeExpiresAt, - timeComparisonFudgeSeconds*time.Second, + timeComparisonFudge, ) // OpenID Connect sessions do not store access token expiration information. @@ -3658,9 +4295,9 @@ func requireValidStoredRequest( require.True(t, ok, "expected session to hold expiration time for access token") testutil.RequireTimeInDelta( t, - time.Now().UTC().Add(accessTokenExpirationSeconds*time.Second), + requestTime.UTC().Add(accessTokenExpirationSeconds*time.Second), accessTokenExpiresAt, - timeComparisonFudgeSeconds*time.Second, + timeComparisonFudge, ) } else { require.False(t, ok, "expected session to not hold expiration time for access token, but it did") @@ -3696,10 +4333,12 @@ func requireValidIDToken( t *testing.T, body map[string]interface{}, jwtSigningKey *ecdsa.PrivateKey, + wantClientID string, wantAtHashClaimInIDToken bool, wantNonceValueInIDToken bool, wantGroupsInIDToken []string, actualAccessToken string, + requestTime time.Time, ) { t.Helper() @@ -3709,7 +4348,7 @@ func requireValidIDToken( require.Truef(t, ok, "wanted id_token to be a string, but got %T", idToken) // The go-oidc library will validate the signature and the client claim in the ID token. - token := oidctestutil.VerifyECDSAIDToken(t, goodIssuer, goodClient, jwtSigningKey, idTokenString) + token := oidctestutil.VerifyECDSAIDToken(t, goodIssuer, wantClientID, jwtSigningKey, idTokenString) var claims struct { Subject string `json:"sub"` @@ -3752,7 +4391,7 @@ func requireValidIDToken( require.Equal(t, goodUsername, claims.Username) require.Equal(t, wantGroupsInIDToken, claims.Groups) require.Len(t, claims.Audience, 1) - require.Equal(t, goodClient, claims.Audience[0]) + require.Equal(t, wantClientID, claims.Audience[0]) require.Equal(t, goodIssuer, claims.Issuer) require.NotEmpty(t, claims.JTI) @@ -3766,10 +4405,10 @@ func requireValidIDToken( issuedAt := time.Unix(claims.IssuedAt, 0) requestedAt := time.Unix(claims.RequestedAt, 0) authTime := time.Unix(claims.AuthTime, 0) - testutil.RequireTimeInDelta(t, time.Now().UTC().Add(idTokenExpirationSeconds*time.Second), expiresAt, timeComparisonFudgeSeconds*time.Second) - testutil.RequireTimeInDelta(t, time.Now().UTC(), issuedAt, timeComparisonFudgeSeconds*time.Second) - testutil.RequireTimeInDelta(t, goodRequestedAtTime, requestedAt, timeComparisonFudgeSeconds*time.Second) - testutil.RequireTimeInDelta(t, goodAuthTime, authTime, timeComparisonFudgeSeconds*time.Second) + testutil.RequireTimeInDelta(t, requestTime.UTC().Add(idTokenExpirationSeconds*time.Second), expiresAt, timeComparisonFudge) + testutil.RequireTimeInDelta(t, requestTime.UTC(), issuedAt, timeComparisonFudge) + testutil.RequireTimeInDelta(t, goodRequestedAtTime, requestedAt, timeComparisonFudge) + testutil.RequireTimeInDelta(t, goodAuthTime, authTime, timeComparisonFudge) if wantAtHashClaimInIDToken { require.NotEmpty(t, actualAccessToken) diff --git a/internal/oidc/token_exchange.go b/internal/oidc/token_exchange.go index 4c2f5500..5ed83b5e 100644 --- a/internal/oidc/token_exchange.go +++ b/internal/oidc/token_exchange.go @@ -13,15 +13,17 @@ import ( "github.com/ory/fosite/compose" "github.com/ory/fosite/handler/oauth2" "github.com/ory/fosite/handler/openid" + "github.com/ory/x/errorsx" "github.com/pkg/errors" "go.pinniped.dev/internal/oidc/clientregistry" ) const ( - tokenTypeAccessToken = "urn:ietf:params:oauth:token-type:access_token" //nolint: gosec - tokenTypeJWT = "urn:ietf:params:oauth:token-type:jwt" //nolint: gosec - pinnipedTokenExchangeScope = "pinniped:request-audience" //nolint: gosec + tokenExchangeGrantType = "urn:ietf:params:oauth:grant-type:token-exchange" //nolint: gosec + tokenTypeAccessToken = "urn:ietf:params:oauth:token-type:access_token" //nolint: gosec + tokenTypeJWT = "urn:ietf:params:oauth:token-type:jwt" //nolint: gosec + pinnipedTokenExchangeScope = "pinniped:request-audience" //nolint: gosec ) type stsParams struct { @@ -70,6 +72,18 @@ func (t *TokenExchangeHandler) PopulateTokenEndpointResponse(ctx context.Context return errors.WithStack(err) } + // Check that the currently authenticated client and the client which was originally used to get the access token are the same. + if originalRequester.GetClient().GetID() != requester.GetClient().GetID() { + // This error message is copied from the similar check in fosite's flow_authorize_code_token.go. + return errorsx.WithStack(fosite.ErrInvalidGrant.WithHint("The OAuth 2.0 Client ID from this request does not match the one from the authorize request.")) + } + + // Check that the client is allowed to perform this grant type. + if !requester.GetClient().GetGrantTypes().Has(tokenExchangeGrantType) { + // This error message is trying to be similar to the analogous one in fosite's flow_authorize_code_token.go. + return errorsx.WithStack(fosite.ErrUnauthorizedClient.WithHintf("The OAuth 2.0 Client is not allowed to use token exchange grant \"%s\".", tokenExchangeGrantType)) + } + // Require that the incoming access token has the pinniped:request-audience and OpenID scopes. if !originalRequester.GetGrantedScopes().Has(pinnipedTokenExchangeScope) { return errors.WithStack(fosite.ErrAccessDenied.WithHintf("missing the %q scope", pinnipedTokenExchangeScope)) @@ -168,5 +182,5 @@ func (t *TokenExchangeHandler) CanSkipClientAuth(_ fosite.AccessRequester) bool } func (t *TokenExchangeHandler) CanHandleTokenEndpointRequest(requester fosite.AccessRequester) bool { - return requester.GetGrantTypes().ExactOne("urn:ietf:params:oauth:grant-type:token-exchange") + return requester.GetGrantTypes().ExactOne(tokenExchangeGrantType) } diff --git a/internal/testutil/assertions.go b/internal/testutil/assertions.go index ee7bc2ed..6117357f 100644 --- a/internal/testutil/assertions.go +++ b/internal/testutil/assertions.go @@ -13,6 +13,7 @@ import ( "github.com/stretchr/testify/require" v12 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" + "k8s.io/apimachinery/pkg/selection" v1 "k8s.io/client-go/kubernetes/typed/core/v1" ) @@ -54,6 +55,23 @@ func RequireNumberOfSecretsMatchingLabelSelector(t *testing.T, secrets v1.Secret require.Len(t, storedAuthcodeSecrets.Items, expectedNumberOfSecrets) } +func RequireNumberOfSecretsExcludingLabelSelector(t *testing.T, secrets v1.SecretInterface, labelSet labels.Set, expectedNumberOfSecrets int) { + t.Helper() + + selector := labels.Everything() + for k, v := range labelSet { + requirement, err := labels.NewRequirement(k, selection.NotEquals, []string{v}) + require.NoError(t, err) + selector = selector.Add(*requirement) + } + + storedAuthcodeSecrets, err := secrets.List(context.Background(), v12.ListOptions{ + LabelSelector: selector.String(), + }) + require.NoError(t, err) + require.Len(t, storedAuthcodeSecrets.Items, expectedNumberOfSecrets) +} + func RequireSecurityHeadersWithFormPostPageCSPs(t *testing.T, response *httptest.ResponseRecorder) { // Loosely confirm that the unique CSPs needed for the form_post page were used. cspHeader := response.Header().Get("Content-Security-Policy") diff --git a/internal/testutil/oidcclient.go b/internal/testutil/oidcclient.go new file mode 100644 index 00000000..621aea2e --- /dev/null +++ b/internal/testutil/oidcclient.go @@ -0,0 +1,67 @@ +// Copyright 2022 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package testutil + +import ( + "strings" + "testing" + + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + + configv1alpha1 "go.pinniped.dev/generated/latest/apis/supervisor/config/v1alpha1" +) + +const ( + AllDynamicClientScopesSpaceSep = "openid offline_access pinniped:request-audience username groups" + + // PlaintextPassword1 is a fake client secret for use in unit tests, along with several flavors of the bcrypt + // hashed version of the password. Do not use for integration tests. + PlaintextPassword1 = "password1" + HashedPassword1AtGoMinCost = "$2a$04$JfX1ba/ctAt3AGk73E9Zz.Fdki5GiQtj.O/CnPbRRSKQWWfv1svoe" //nolint:gosec // this is not a credential + HashedPassword1JustBelowSupervisorMinCost = "$2a$11$w/incy7Z1/ljLYvv2XRg4.WrPgY9oR7phebcgr6rGA3u/5TG9MKOe" //nolint:gosec // this is not a credential + HashedPassword1AtSupervisorMinCost = "$2a$12$id4i/yFYxS99txKOFEeboea2kU6DyZY0Nh4ul0eR46sDuoFoNTRV." //nolint:gosec // this is not a credential + HashedPassword1InvalidFormat = "$2a$12$id4i/yFYxS99txKOFEeboea2kU6DyZY0Nh4ul0eR46sDuo" //nolint:gosec // this is not a credential + + // PlaintextPassword2 is a second fake client secret for use in unit tests, along with several flavors of the bcrypt + // hashed version of the password. Do not use for integration tests. + PlaintextPassword2 = "password2" + HashedPassword2AtGoMinCost = "$2a$04$VQ5z6kkgU8JPLGSGctg.s.iYyoac3Oisa/SIM3sDK5BxTrVbCkyNm" //nolint:gosec // this is not a credential + HashedPassword2AtSupervisorMinCost = "$2a$12$SdUqoJOn4/3yEQfJx616V.q.f76KaXD.ISgJT1oydqFdgfjJpBh6u" //nolint:gosec // this is not a credential +) + +// allDynamicClientScopes returns a slice of all scopes that are supported by the Supervisor for dynamic clients. +func allDynamicClientScopes() []configv1alpha1.Scope { + scopes := []configv1alpha1.Scope{} + for _, s := range strings.Split(AllDynamicClientScopesSpaceSep, " ") { + scopes = append(scopes, configv1alpha1.Scope(s)) + } + return scopes +} + +// fullyCapableOIDCClient returns an OIDC client which is allowed to use all grant types and all scopes that +// are supported by the Supervisor for dynamic clients. +func fullyCapableOIDCClient(namespace string, clientID string, clientUID string, redirectURI string) *configv1alpha1.OIDCClient { + return &configv1alpha1.OIDCClient{ + ObjectMeta: metav1.ObjectMeta{Namespace: namespace, Name: clientID, Generation: 1, UID: types.UID(clientUID)}, + Spec: configv1alpha1.OIDCClientSpec{ + AllowedGrantTypes: []configv1alpha1.GrantType{"authorization_code", "urn:ietf:params:oauth:grant-type:token-exchange", "refresh_token"}, + AllowedScopes: allDynamicClientScopes(), + AllowedRedirectURIs: []configv1alpha1.RedirectURI{configv1alpha1.RedirectURI(redirectURI)}, + }, + } +} + +func FullyCapableOIDCClientAndStorageSecret( + t *testing.T, + namespace string, + clientID string, + clientUID string, + redirectURI string, + hashes []string, +) (*configv1alpha1.OIDCClient, *corev1.Secret) { + return fullyCapableOIDCClient(namespace, clientID, clientUID, redirectURI), + OIDCClientSecretStorageSecretForUID(t, namespace, clientUID, hashes) +} diff --git a/internal/testutil/oidcclient_test.go b/internal/testutil/oidcclient_test.go new file mode 100644 index 00000000..cd892313 --- /dev/null +++ b/internal/testutil/oidcclient_test.go @@ -0,0 +1,61 @@ +// Copyright 2022 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package testutil + +import ( + "testing" + + "go.pinniped.dev/internal/oidc/oidcclientvalidator" + + "github.com/stretchr/testify/require" + "golang.org/x/crypto/bcrypt" +) + +func TestBcryptConstants(t *testing.T) { + t.Parallel() + + // It would be helpful to know if upgrading golang changes these constants some day, so test them here for visibility during upgrades. + require.Equal(t, 4, bcrypt.MinCost, "golang has changed bcrypt.MinCost: please consider implications to the other tests") + require.Equal(t, 10, bcrypt.DefaultCost, "golang has changed bcrypt.DefaultCost: please consider implications to the production code and tests") +} + +func TestBcryptHashedPassword1TestHelpers(t *testing.T) { + t.Parallel() + + // Can use this to help generate or regenerate the test helper hash constants. + // t.Log(generateHash(t, PlaintextPassword1, 12)) + + require.NoError(t, bcrypt.CompareHashAndPassword([]byte(HashedPassword1AtGoMinCost), []byte(PlaintextPassword1))) + require.NoError(t, bcrypt.CompareHashAndPassword([]byte(HashedPassword1JustBelowSupervisorMinCost), []byte(PlaintextPassword1))) + require.NoError(t, bcrypt.CompareHashAndPassword([]byte(HashedPassword1AtSupervisorMinCost), []byte(PlaintextPassword1))) + + requireCost(t, bcrypt.MinCost, HashedPassword1AtGoMinCost) + requireCost(t, oidcclientvalidator.DefaultMinBcryptCost-1, HashedPassword1JustBelowSupervisorMinCost) + requireCost(t, oidcclientvalidator.DefaultMinBcryptCost, HashedPassword1AtSupervisorMinCost) +} + +func TestBcryptHashedPassword2TestHelpers(t *testing.T) { + t.Parallel() + + // Can use this to help generate or regenerate the test helper hash constants. + // t.Log(generateHash(t, PlaintextPassword2, 12)) + + require.NoError(t, bcrypt.CompareHashAndPassword([]byte(HashedPassword2AtGoMinCost), []byte(PlaintextPassword2))) + require.NoError(t, bcrypt.CompareHashAndPassword([]byte(HashedPassword2AtSupervisorMinCost), []byte(PlaintextPassword2))) + + requireCost(t, bcrypt.MinCost, HashedPassword2AtGoMinCost) + requireCost(t, oidcclientvalidator.DefaultMinBcryptCost, HashedPassword2AtSupervisorMinCost) +} + +func generateHash(t *testing.T, password string, cost int) string { //nolint:unused,deadcode // used in comments above + hash, err := bcrypt.GenerateFromPassword([]byte(password), cost) + require.NoError(t, err) + return string(hash) +} + +func requireCost(t *testing.T, wantCost int, hash string) { + cost, err := bcrypt.Cost([]byte(hash)) + require.NoError(t, err) + require.Equal(t, wantCost, cost) +} diff --git a/test/integration/supervisor_login_test.go b/test/integration/supervisor_login_test.go index fa9c74b3..86c3f67f 100644 --- a/test/integration/supervisor_login_test.go +++ b/test/integration/supervisor_login_test.go @@ -31,6 +31,7 @@ import ( idpv1alpha1 "go.pinniped.dev/generated/latest/apis/supervisor/idp/v1alpha1" "go.pinniped.dev/internal/certauthority" "go.pinniped.dev/internal/oidc" + "go.pinniped.dev/internal/oidc/oidcclientvalidator" "go.pinniped.dev/internal/psession" "go.pinniped.dev/internal/testutil" "go.pinniped.dev/pkg/oidcclient/nonce" @@ -1727,7 +1728,7 @@ func testSupervisorLogin( // First use the latest downstream refresh token to look up the corresponding session in the Supervisor's storage. supervisorSecretsClient := testlib.NewKubernetesClientset(t).CoreV1().Secrets(env.SupervisorNamespace) supervisorOIDCClientsClient := testlib.NewSupervisorClientset(t).ConfigV1alpha1().OIDCClients(env.SupervisorNamespace) - oauthStore := oidc.NewKubeStorage(supervisorSecretsClient, supervisorOIDCClientsClient, oidc.DefaultOIDCTimeoutsConfiguration()) + oauthStore := oidc.NewKubeStorage(supervisorSecretsClient, supervisorOIDCClientsClient, oidc.DefaultOIDCTimeoutsConfiguration(), oidcclientvalidator.DefaultMinBcryptCost) storedRefreshSession, err := oauthStore.GetRefreshTokenSession(ctx, signatureOfLatestRefreshToken, nil) require.NoError(t, err) @@ -1772,7 +1773,7 @@ func testSupervisorLogin( // First use the latest downstream refresh token to look up the corresponding session in the Supervisor's storage. supervisorSecretsClient := testlib.NewKubernetesClientset(t).CoreV1().Secrets(env.SupervisorNamespace) supervisorOIDCClientsClient := testlib.NewSupervisorClientset(t).ConfigV1alpha1().OIDCClients(env.SupervisorNamespace) - oauthStore := oidc.NewKubeStorage(supervisorSecretsClient, supervisorOIDCClientsClient, oidc.DefaultOIDCTimeoutsConfiguration()) + oauthStore := oidc.NewKubeStorage(supervisorSecretsClient, supervisorOIDCClientsClient, oidc.DefaultOIDCTimeoutsConfiguration(), oidcclientvalidator.DefaultMinBcryptCost) storedRefreshSession, err := oauthStore.GetRefreshTokenSession(ctx, signatureOfLatestRefreshToken, nil) require.NoError(t, err) diff --git a/test/integration/supervisor_oidc_client_test.go b/test/integration/supervisor_oidc_client_test.go index 4ec9fc55..d8a5ac55 100644 --- a/test/integration/supervisor_oidc_client_test.go +++ b/test/integration/supervisor_oidc_client_test.go @@ -563,7 +563,7 @@ func TestOIDCClientControllerValidations_Parallel(t *testing.T) { AllowedScopes: []supervisorconfigv1alpha1.Scope{"openid", "offline_access", "pinniped:request-audience", "username", "groups"}, }, }, - secret: testutil.OIDCClientSecretStorageSecretWithoutName(t, env.SupervisorNamespace, []string{"$2y$15$Kh7cRj0ScSD5QelE3ZNSl.nF04JDv7zb3SgGN.tSfLIX.4kt3UX7m"}), + secret: testutil.OIDCClientSecretStorageSecretWithoutName(t, env.SupervisorNamespace, []string{testutil.HashedPassword1AtSupervisorMinCost}), wantPhase: "Ready", wantConditions: []supervisorconfigv1alpha1.Condition{ { diff --git a/test/integration/supervisor_warnings_test.go b/test/integration/supervisor_warnings_test.go index e3ea9485..55cbf7dd 100644 --- a/test/integration/supervisor_warnings_test.go +++ b/test/integration/supervisor_warnings_test.go @@ -31,6 +31,7 @@ import ( idpv1alpha1 "go.pinniped.dev/generated/latest/apis/supervisor/idp/v1alpha1" "go.pinniped.dev/internal/certauthority" "go.pinniped.dev/internal/oidc" + "go.pinniped.dev/internal/oidc/oidcclientvalidator" "go.pinniped.dev/internal/psession" "go.pinniped.dev/internal/testutil" "go.pinniped.dev/pkg/oidcclient" @@ -188,7 +189,7 @@ func TestSupervisorWarnings_Browser(t *testing.T) { // out of kube secret storage. supervisorSecretsClient := testlib.NewKubernetesClientset(t).CoreV1().Secrets(env.SupervisorNamespace) supervisorOIDCClientsClient := testlib.NewSupervisorClientset(t).ConfigV1alpha1().OIDCClients(env.SupervisorNamespace) - oauthStore := oidc.NewKubeStorage(supervisorSecretsClient, supervisorOIDCClientsClient, oidc.DefaultOIDCTimeoutsConfiguration()) + oauthStore := oidc.NewKubeStorage(supervisorSecretsClient, supervisorOIDCClientsClient, oidc.DefaultOIDCTimeoutsConfiguration(), oidcclientvalidator.DefaultMinBcryptCost) refreshTokenSignature := strings.Split(token.RefreshToken.Token, ".")[1] storedRefreshSession, err := oauthStore.GetRefreshTokenSession(ctx, refreshTokenSignature, nil) require.NoError(t, err) @@ -496,7 +497,7 @@ func TestSupervisorWarnings_Browser(t *testing.T) { // out of kube secret storage. supervisorSecretsClient := testlib.NewKubernetesClientset(t).CoreV1().Secrets(env.SupervisorNamespace) supervisorOIDCClientsClient := testlib.NewSupervisorClientset(t).ConfigV1alpha1().OIDCClients(env.SupervisorNamespace) - oauthStore := oidc.NewKubeStorage(supervisorSecretsClient, supervisorOIDCClientsClient, oidc.DefaultOIDCTimeoutsConfiguration()) + oauthStore := oidc.NewKubeStorage(supervisorSecretsClient, supervisorOIDCClientsClient, oidc.DefaultOIDCTimeoutsConfiguration(), oidcclientvalidator.DefaultMinBcryptCost) refreshTokenSignature := strings.Split(token.RefreshToken.Token, ".")[1] storedRefreshSession, err := oauthStore.GetRefreshTokenSession(ctx, refreshTokenSignature, nil) require.NoError(t, err)