diff --git a/internal/controller/supervisorconfig/oidcclientwatcher/oidc_client_watcher.go b/internal/controller/supervisorconfig/oidcclientwatcher/oidc_client_watcher.go index eb6ab992..12123731 100644 --- a/internal/controller/supervisorconfig/oidcclientwatcher/oidc_client_watcher.go +++ b/internal/controller/supervisorconfig/oidcclientwatcher/oidc_client_watcher.go @@ -8,9 +8,6 @@ import ( "fmt" "strings" - "github.com/coreos/go-oidc/v3/oidc" - "golang.org/x/crypto/bcrypt" - v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/equality" k8serrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -23,37 +20,14 @@ import ( pinnipedcontroller "go.pinniped.dev/internal/controller" "go.pinniped.dev/internal/controller/conditionsutil" "go.pinniped.dev/internal/controllerlib" + "go.pinniped.dev/internal/oidc/oidcclientvalidator" "go.pinniped.dev/internal/oidcclientsecretstorage" "go.pinniped.dev/internal/plog" ) const ( - clientSecretExists = "ClientSecretExists" - allowedGrantTypesValid = "AllowedGrantTypesValid" - allowedScopesValid = "AllowedScopesValid" - - reasonSuccess = "Success" - reasonMissingRequiredValue = "MissingRequiredValue" - reasonNoClientSecretFound = "NoClientSecretFound" - reasonInvalidClientSecretFound = "InvalidClientSecretFound" - - authorizationCodeGrantTypeName = "authorization_code" - refreshTokenGrantTypeName = "refresh_token" - tokenExchangeGrantTypeName = "urn:ietf:params:oauth:grant-type:token-exchange" //nolint:gosec // this is not a credential - - openidScopeName = oidc.ScopeOpenID - offlineAccessScopeName = oidc.ScopeOfflineAccess - requestAudienceScopeName = "pinniped:request-audience" - usernameScopeName = "username" - groupsScopeName = "groups" - - allowedGrantTypesFieldName = "allowedGrantTypes" - allowedScopesFieldName = "allowedScopes" - secretTypeToObserve = "storage.pinniped.dev/oidc-client-secret" //nolint:gosec // this is not a credential oidcClientPrefixToObserve = "client.oauth.pinniped.dev-" //nolint:gosec // this is not a credential - - minimumRequiredBcryptCost = 15 ) type oidcClientWatcherController struct { @@ -133,9 +107,9 @@ func (c *oidcClientWatcherController) Sync(ctx controllerlib.Context) error { secret = nil } - conditions, totalClientSecrets := validateOIDCClient(oidcClient, secret) + _, conditions, clientSecrets := oidcclientvalidator.Validate(oidcClient, secret) - if err := c.updateStatus(ctx.Context, oidcClient, conditions, totalClientSecrets); err != nil { + 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) } @@ -150,185 +124,6 @@ func (c *oidcClientWatcherController) Sync(ctx controllerlib.Context) error { return nil } -// validateOIDCClient validates the OIDCClient and its corresponding client secret storage Secret. -// When the corresponding client secret storage Secret was not found, pass nil to this function to -// get the validation error for that case. It returns a slice of conditions along with the number -// of client secrets found. -func validateOIDCClient(oidcClient *v1alpha1.OIDCClient, secret *v1.Secret) ([]*v1alpha1.Condition, int) { - c, totalClientSecrets := validateSecret(secret, make([]*v1alpha1.Condition, 0, 3)) - c = validateAllowedGrantTypes(oidcClient, c) - c = validateAllowedScopes(oidcClient, c) - return c, totalClientSecrets -} - -// validateAllowedScopes checks if allowedScopes is valid on the OIDCClient. -func validateAllowedScopes(oidcClient *v1alpha1.OIDCClient, conditions []*v1alpha1.Condition) []*v1alpha1.Condition { - m := make([]string, 0, 4) - - if !allowedScopesContains(oidcClient, openidScopeName) { - m = append(m, fmt.Sprintf("%q must always be included in %q", openidScopeName, allowedScopesFieldName)) - } - if allowedGrantTypesContains(oidcClient, refreshTokenGrantTypeName) && !allowedScopesContains(oidcClient, offlineAccessScopeName) { - m = append(m, fmt.Sprintf("%q must be included in %q when %q is included in %q", - offlineAccessScopeName, allowedScopesFieldName, refreshTokenGrantTypeName, allowedGrantTypesFieldName)) - } - if allowedScopesContains(oidcClient, requestAudienceScopeName) && - (!allowedScopesContains(oidcClient, usernameScopeName) || !allowedScopesContains(oidcClient, groupsScopeName)) { - m = append(m, fmt.Sprintf("%q and %q must be included in %q when %q is included in %q", - usernameScopeName, groupsScopeName, allowedScopesFieldName, requestAudienceScopeName, allowedScopesFieldName)) - } - if allowedGrantTypesContains(oidcClient, tokenExchangeGrantTypeName) && !allowedScopesContains(oidcClient, requestAudienceScopeName) { - m = append(m, fmt.Sprintf("%q must be included in %q when %q is included in %q", - requestAudienceScopeName, allowedScopesFieldName, tokenExchangeGrantTypeName, allowedGrantTypesFieldName)) - } - - if len(m) == 0 { - conditions = append(conditions, &v1alpha1.Condition{ - Type: allowedScopesValid, - Status: v1alpha1.ConditionTrue, - Reason: reasonSuccess, - Message: fmt.Sprintf("%q is valid", allowedScopesFieldName), - }) - } else { - conditions = append(conditions, &v1alpha1.Condition{ - Type: allowedScopesValid, - Status: v1alpha1.ConditionFalse, - Reason: reasonMissingRequiredValue, - Message: strings.Join(m, "; "), - }) - } - - return conditions -} - -// validateAllowedGrantTypes checks if allowedGrantTypes is valid on the OIDCClient. -func validateAllowedGrantTypes(oidcClient *v1alpha1.OIDCClient, conditions []*v1alpha1.Condition) []*v1alpha1.Condition { - m := make([]string, 0, 3) - - if !allowedGrantTypesContains(oidcClient, authorizationCodeGrantTypeName) { - m = append(m, fmt.Sprintf("%q must always be included in %q", - authorizationCodeGrantTypeName, allowedGrantTypesFieldName)) - } - if allowedScopesContains(oidcClient, offlineAccessScopeName) && !allowedGrantTypesContains(oidcClient, refreshTokenGrantTypeName) { - m = append(m, fmt.Sprintf("%q must be included in %q when %q is included in %q", - refreshTokenGrantTypeName, allowedGrantTypesFieldName, offlineAccessScopeName, allowedScopesFieldName)) - } - if allowedScopesContains(oidcClient, requestAudienceScopeName) && !allowedGrantTypesContains(oidcClient, tokenExchangeGrantTypeName) { - m = append(m, fmt.Sprintf("%q must be included in %q when %q is included in %q", - tokenExchangeGrantTypeName, allowedGrantTypesFieldName, requestAudienceScopeName, allowedScopesFieldName)) - } - - if len(m) == 0 { - conditions = append(conditions, &v1alpha1.Condition{ - Type: allowedGrantTypesValid, - Status: v1alpha1.ConditionTrue, - Reason: reasonSuccess, - Message: fmt.Sprintf("%q is valid", allowedGrantTypesFieldName), - }) - } else { - conditions = append(conditions, &v1alpha1.Condition{ - Type: allowedGrantTypesValid, - Status: v1alpha1.ConditionFalse, - Reason: reasonMissingRequiredValue, - Message: strings.Join(m, "; "), - }) - } - - return conditions -} - -// validateSecret checks if the client secret storage Secret is valid and contains at least one client secret. -// It returns the updated conditions slice along with the number of client secrets found. -func validateSecret(secret *v1.Secret, conditions []*v1alpha1.Condition) ([]*v1alpha1.Condition, int) { - if secret == nil { - // Invalid: no storage Secret found. - conditions = append(conditions, &v1alpha1.Condition{ - Type: clientSecretExists, - Status: v1alpha1.ConditionFalse, - Reason: reasonNoClientSecretFound, - Message: "no client secret found (no Secret storage found)", - }) - return conditions, 0 - } - - storedClientSecret, err := oidcclientsecretstorage.ReadFromSecret(secret) - if err != nil { - // Invalid: storage Secret exists but its data could not be parsed. - conditions = append(conditions, &v1alpha1.Condition{ - Type: clientSecretExists, - Status: v1alpha1.ConditionFalse, - Reason: reasonNoClientSecretFound, - Message: fmt.Sprintf("error reading client secret storage: %s", err.Error()), - }) - return conditions, 0 - } - - // Successfully read the stored client secrets, so check if there are any stored in the list. - storedClientSecretsCount := len(storedClientSecret.SecretHashes) - if storedClientSecretsCount == 0 { - // Invalid: no client secrets stored. - conditions = append(conditions, &v1alpha1.Condition{ - Type: clientSecretExists, - Status: v1alpha1.ConditionFalse, - Reason: reasonNoClientSecretFound, - Message: "no client secret found (empty list in storage)", - }) - return conditions, 0 - } - - // Check each hashed password's format and bcrypt cost. - bcryptErrs := make([]string, 0, storedClientSecretsCount) - for i, p := range storedClientSecret.SecretHashes { - cost, err := bcrypt.Cost([]byte(p)) - if err != nil { - bcryptErrs = append(bcryptErrs, fmt.Sprintf( - "hashed client secret at index %d: %s", - i, err.Error())) - } else if cost < minimumRequiredBcryptCost { - bcryptErrs = append(bcryptErrs, fmt.Sprintf( - "hashed client secret at index %d: bcrypt cost %d is below the required minimum of %d", - i, cost, minimumRequiredBcryptCost)) - } - } - if len(bcryptErrs) > 0 { - // Invalid: some stored client secrets were not valid. - conditions = append(conditions, &v1alpha1.Condition{ - Type: clientSecretExists, - Status: v1alpha1.ConditionFalse, - Reason: reasonInvalidClientSecretFound, - Message: strings.Join(bcryptErrs, "; "), - }) - return conditions, storedClientSecretsCount - } - - // Valid: has at least one client secret stored for this OIDC client, and all stored client secrets are valid. - conditions = append(conditions, &v1alpha1.Condition{ - Type: clientSecretExists, - Status: v1alpha1.ConditionTrue, - Reason: reasonSuccess, - Message: fmt.Sprintf("%d client secret(s) found", storedClientSecretsCount), - }) - return conditions, storedClientSecretsCount -} - -func allowedGrantTypesContains(haystack *v1alpha1.OIDCClient, needle string) bool { - for _, hay := range haystack.Spec.AllowedGrantTypes { - if hay == v1alpha1.GrantType(needle) { - return true - } - } - return false -} - -func allowedScopesContains(haystack *v1alpha1.OIDCClient, needle string) bool { - for _, hay := range haystack.Spec.AllowedScopes { - if hay == v1alpha1.Scope(needle) { - return true - } - } - return false -} - func (c *oidcClientWatcherController) updateStatus( ctx context.Context, upstream *v1alpha1.OIDCClient, diff --git a/internal/controller/supervisorconfig/oidcclientwatcher/oidc_client_watcher_test.go b/internal/controller/supervisorconfig/oidcclientwatcher/oidc_client_watcher_test.go index 92a0d358..b1d147fe 100644 --- a/internal/controller/supervisorconfig/oidcclientwatcher/oidc_client_watcher_test.go +++ b/internal/controller/supervisorconfig/oidcclientwatcher/oidc_client_watcher_test.go @@ -5,9 +5,7 @@ package oidcclientwatcher import ( "context" - "encoding/base32" "fmt" - "strings" "testing" "time" @@ -257,51 +255,6 @@ func TestOIDCClientWatcherControllerSync(t *testing.T) { } } - secretNameForUID := func(uid string) string { - // See GetName() in OIDCClientSecretStorage for how the production code determines the Secret name. - // This test helper is intended to choose the same name. - return "pinniped-storage-oidc-client-secret-" + - strings.ToLower(base32.StdEncoding.WithPadding(base32.NoPadding).EncodeToString([]byte(uid))) - } - - secretStringDataWithZeroClientSecrets := map[string][]byte{ - "pinniped-storage-data": []byte(`{"version":"1","hashes":[]}`), - "pinniped-storage-version": []byte("1"), - } - - secretStringDataWithOneClientSecret := map[string][]byte{ - "pinniped-storage-data": []byte(`{"version":"1","hashes":["` + testBcryptSecret1 + `"]}`), - "pinniped-storage-version": []byte("1"), - } - - secretStringDataWithTwoClientSecrets := map[string][]byte{ - "pinniped-storage-data": []byte(`{"version":"1","hashes":["` + testBcryptSecret1 + `","` + testBcryptSecret2 + `"]}`), - "pinniped-storage-version": []byte("1"), - } - - secretStringDataWithSomeInvalidClientSecrets := map[string][]byte{ - "pinniped-storage-data": []byte(`{"version":"1","hashes":["` + - testBcryptSecret1 + `","` + testInvalidBcryptSecretCostTooLow + `","` + testInvalidBcryptSecretInvalidFormat + `"]}`), - "pinniped-storage-version": []byte("1"), - } - - secretStringDataWithWrongVersion := map[string][]byte{ - "pinniped-storage-data": []byte(`{"version":"wrong-version","hashes":[]}`), - "pinniped-storage-version": []byte("1"), - } - - storageSecretForUIDWithData := func(uid string, data map[string][]byte) *corev1.Secret { - return &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: testNamespace, - Name: secretNameForUID(uid), - Labels: map[string]string{"storage.pinniped.dev/type": "oidc-client-secret"}, - }, - Type: "storage.pinniped.dev/oidc-client-secret", - Data: data, - } - } - tests := []struct { name string inputObjects []runtime.Object @@ -338,7 +291,7 @@ func TestOIDCClientWatcherControllerSync(t *testing.T) { }, }, }, - inputSecrets: []runtime.Object{storageSecretForUIDWithData(testUID, secretStringDataWithOneClientSecret)}, + inputSecrets: []runtime.Object{testutil.OIDCClientSecretStorageSecretForUID(t, testNamespace, testUID, []string{testBcryptSecret1})}, wantAPIActions: 1, // one update wantResultingOIDCClients: []configv1alpha1.OIDCClient{ { @@ -367,7 +320,7 @@ func TestOIDCClientWatcherControllerSync(t *testing.T) { AllowedScopes: []configv1alpha1.Scope{"openid"}, }, }}, - inputSecrets: []runtime.Object{storageSecretForUIDWithData(testUID, secretStringDataWithTwoClientSecrets)}, + inputSecrets: []runtime.Object{testutil.OIDCClientSecretStorageSecretForUID(t, testNamespace, testUID, []string{testBcryptSecret1, testBcryptSecret2})}, wantAPIActions: 1, // one update wantResultingOIDCClients: []configv1alpha1.OIDCClient{{ ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234, UID: testUID}, @@ -400,7 +353,7 @@ func TestOIDCClientWatcherControllerSync(t *testing.T) { TotalClientSecrets: 1, }, }}, - inputSecrets: []runtime.Object{storageSecretForUIDWithData(testUID, secretStringDataWithOneClientSecret)}, + inputSecrets: []runtime.Object{testutil.OIDCClientSecretStorageSecretForUID(t, testNamespace, testUID, []string{testBcryptSecret1})}, wantAPIActions: 0, // no updates wantResultingOIDCClients: []configv1alpha1.OIDCClient{{ ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234, UID: testUID}, @@ -443,7 +396,7 @@ func TestOIDCClientWatcherControllerSync(t *testing.T) { AllowedScopes: []configv1alpha1.Scope{"openid"}, }, }}, - inputSecrets: []runtime.Object{storageSecretForUIDWithData(testUID, secretStringDataWithWrongVersion)}, + inputSecrets: []runtime.Object{testutil.OIDCClientSecretStorageSecretForUIDWithWrongVersion(t, testNamespace, testUID)}, wantAPIActions: 1, // one update wantResultingOIDCClients: []configv1alpha1.OIDCClient{{ ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234, UID: testUID}, @@ -466,7 +419,7 @@ func TestOIDCClientWatcherControllerSync(t *testing.T) { AllowedScopes: []configv1alpha1.Scope{"openid"}, }, }}, - inputSecrets: []runtime.Object{storageSecretForUIDWithData(testUID, secretStringDataWithZeroClientSecrets)}, + inputSecrets: []runtime.Object{testutil.OIDCClientSecretStorageSecretForUID(t, testNamespace, testUID, []string{})}, wantAPIActions: 1, // one update wantResultingOIDCClients: []configv1alpha1.OIDCClient{{ ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234, UID: testUID}, @@ -490,7 +443,10 @@ func TestOIDCClientWatcherControllerSync(t *testing.T) { AllowedScopes: []configv1alpha1.Scope{"openid"}, }, }}, - inputSecrets: []runtime.Object{storageSecretForUIDWithData(testUID, secretStringDataWithSomeInvalidClientSecrets)}, + inputSecrets: []runtime.Object{ + testutil.OIDCClientSecretStorageSecretForUID(t, testNamespace, testUID, + []string{testBcryptSecret1, testInvalidBcryptSecretCostTooLow, testInvalidBcryptSecretInvalidFormat}), + }, wantAPIActions: 1, // one update wantResultingOIDCClients: []configv1alpha1.OIDCClient{{ ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234, UID: testUID}, @@ -500,10 +456,11 @@ func TestOIDCClientWatcherControllerSync(t *testing.T) { happyAllowedGrantTypesCondition(now, 1234), happyAllowedScopesCondition(now, 1234), sadInvalidClientSecretsCondition(now, 1234, - "hashed client secret at index 1: bcrypt cost 14 is below the required minimum of 15; "+ + "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 2: crypto/bcrypt: hashedSecret too short to be a bcrypted password"), }, - TotalClientSecrets: 3, + TotalClientSecrets: 0, }, }}, }, @@ -522,7 +479,7 @@ func TestOIDCClientWatcherControllerSync(t *testing.T) { Spec: configv1alpha1.OIDCClientSpec{}, }, }, - inputSecrets: []runtime.Object{storageSecretForUIDWithData("uid1", secretStringDataWithOneClientSecret)}, + inputSecrets: []runtime.Object{testutil.OIDCClientSecretStorageSecretForUID(t, testNamespace, "uid1", []string{testBcryptSecret1})}, wantAPIActions: 2, // one update for each OIDCClient wantResultingOIDCClients: []configv1alpha1.OIDCClient{ { @@ -570,7 +527,7 @@ func TestOIDCClientWatcherControllerSync(t *testing.T) { TotalClientSecrets: 1, }, }}, - inputSecrets: []runtime.Object{storageSecretForUIDWithData(testUID, secretStringDataWithOneClientSecret)}, + inputSecrets: []runtime.Object{testutil.OIDCClientSecretStorageSecretForUID(t, testNamespace, testUID, []string{testBcryptSecret1})}, wantAPIActions: 1, // one update wantResultingOIDCClients: []configv1alpha1.OIDCClient{{ ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 4567, UID: testUID}, @@ -596,7 +553,7 @@ func TestOIDCClientWatcherControllerSync(t *testing.T) { }, }}, wantAPIActions: 1, // one update - inputSecrets: []runtime.Object{storageSecretForUIDWithData(testUID, secretStringDataWithOneClientSecret)}, + inputSecrets: []runtime.Object{testutil.OIDCClientSecretStorageSecretForUID(t, testNamespace, testUID, []string{testBcryptSecret1})}, wantResultingOIDCClients: []configv1alpha1.OIDCClient{{ ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234, UID: testUID}, Status: configv1alpha1.OIDCClientStatus{ @@ -620,7 +577,7 @@ func TestOIDCClientWatcherControllerSync(t *testing.T) { }, }}, wantAPIActions: 1, // one update - inputSecrets: []runtime.Object{storageSecretForUIDWithData(testUID, secretStringDataWithOneClientSecret)}, + inputSecrets: []runtime.Object{testutil.OIDCClientSecretStorageSecretForUID(t, testNamespace, testUID, []string{testBcryptSecret1})}, wantResultingOIDCClients: []configv1alpha1.OIDCClient{{ ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234, UID: testUID}, Status: configv1alpha1.OIDCClientStatus{ @@ -649,7 +606,7 @@ func TestOIDCClientWatcherControllerSync(t *testing.T) { }, }}, wantAPIActions: 1, // one update - inputSecrets: []runtime.Object{storageSecretForUIDWithData(testUID, secretStringDataWithOneClientSecret)}, + inputSecrets: []runtime.Object{testutil.OIDCClientSecretStorageSecretForUID(t, testNamespace, testUID, []string{testBcryptSecret1})}, wantResultingOIDCClients: []configv1alpha1.OIDCClient{{ ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234, UID: testUID}, Status: configv1alpha1.OIDCClientStatus{ @@ -676,7 +633,7 @@ func TestOIDCClientWatcherControllerSync(t *testing.T) { AllowedScopes: []configv1alpha1.Scope{"openid", "pinniped:request-audience", "username", "groups"}, }, }}, - inputSecrets: []runtime.Object{storageSecretForUIDWithData(testUID, secretStringDataWithOneClientSecret)}, + inputSecrets: []runtime.Object{testutil.OIDCClientSecretStorageSecretForUID(t, testNamespace, testUID, []string{testBcryptSecret1})}, wantAPIActions: 1, // one update wantResultingOIDCClients: []configv1alpha1.OIDCClient{{ ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234, UID: testUID}, @@ -700,7 +657,7 @@ func TestOIDCClientWatcherControllerSync(t *testing.T) { AllowedScopes: []configv1alpha1.Scope{"openid"}, }, }}, - inputSecrets: []runtime.Object{storageSecretForUIDWithData(testUID, secretStringDataWithOneClientSecret)}, + inputSecrets: []runtime.Object{testutil.OIDCClientSecretStorageSecretForUID(t, testNamespace, testUID, []string{testBcryptSecret1})}, wantAPIActions: 1, // one update wantResultingOIDCClients: []configv1alpha1.OIDCClient{{ ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234, UID: testUID}, @@ -724,7 +681,7 @@ func TestOIDCClientWatcherControllerSync(t *testing.T) { AllowedScopes: []configv1alpha1.Scope{"openid", "pinniped:request-audience"}, }, }}, - inputSecrets: []runtime.Object{storageSecretForUIDWithData(testUID, secretStringDataWithOneClientSecret)}, + inputSecrets: []runtime.Object{testutil.OIDCClientSecretStorageSecretForUID(t, testNamespace, testUID, []string{testBcryptSecret1})}, wantAPIActions: 1, // one update wantResultingOIDCClients: []configv1alpha1.OIDCClient{{ ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234, UID: testUID}, @@ -748,7 +705,7 @@ func TestOIDCClientWatcherControllerSync(t *testing.T) { AllowedScopes: []configv1alpha1.Scope{"openid", "pinniped:request-audience", "groups"}, }, }}, - inputSecrets: []runtime.Object{storageSecretForUIDWithData(testUID, secretStringDataWithOneClientSecret)}, + inputSecrets: []runtime.Object{testutil.OIDCClientSecretStorageSecretForUID(t, testNamespace, testUID, []string{testBcryptSecret1})}, wantAPIActions: 1, // one update wantResultingOIDCClients: []configv1alpha1.OIDCClient{{ ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234, UID: testUID}, @@ -772,7 +729,7 @@ func TestOIDCClientWatcherControllerSync(t *testing.T) { AllowedScopes: []configv1alpha1.Scope{"openid", "pinniped:request-audience", "username"}, }, }}, - inputSecrets: []runtime.Object{storageSecretForUIDWithData(testUID, secretStringDataWithOneClientSecret)}, + inputSecrets: []runtime.Object{testutil.OIDCClientSecretStorageSecretForUID(t, testNamespace, testUID, []string{testBcryptSecret1})}, wantAPIActions: 1, // one update wantResultingOIDCClients: []configv1alpha1.OIDCClient{{ ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234, UID: testUID}, @@ -796,7 +753,7 @@ func TestOIDCClientWatcherControllerSync(t *testing.T) { AllowedScopes: []configv1alpha1.Scope{"openid"}, }, }}, - inputSecrets: []runtime.Object{storageSecretForUIDWithData(testUID, secretStringDataWithOneClientSecret)}, + inputSecrets: []runtime.Object{testutil.OIDCClientSecretStorageSecretForUID(t, testNamespace, testUID, []string{testBcryptSecret1})}, wantAPIActions: 1, // one update wantResultingOIDCClients: []configv1alpha1.OIDCClient{{ ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234, UID: testUID}, @@ -820,7 +777,7 @@ func TestOIDCClientWatcherControllerSync(t *testing.T) { AllowedScopes: []configv1alpha1.Scope{"openid", "offline_access", "pinniped:request-audience", "username", "groups"}, }, }}, - inputSecrets: []runtime.Object{storageSecretForUIDWithData(testUID, secretStringDataWithOneClientSecret)}, + inputSecrets: []runtime.Object{testutil.OIDCClientSecretStorageSecretForUID(t, testNamespace, testUID, []string{testBcryptSecret1})}, wantAPIActions: 1, // one update wantResultingOIDCClients: []configv1alpha1.OIDCClient{{ ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234, UID: testUID}, @@ -844,7 +801,7 @@ func TestOIDCClientWatcherControllerSync(t *testing.T) { AllowedScopes: []configv1alpha1.Scope{"openid", "offline_access"}, }, }}, - inputSecrets: []runtime.Object{storageSecretForUIDWithData(testUID, secretStringDataWithOneClientSecret)}, + inputSecrets: []runtime.Object{testutil.OIDCClientSecretStorageSecretForUID(t, testNamespace, testUID, []string{testBcryptSecret1})}, wantAPIActions: 1, // one update wantResultingOIDCClients: []configv1alpha1.OIDCClient{{ ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234, UID: testUID}, @@ -868,7 +825,7 @@ func TestOIDCClientWatcherControllerSync(t *testing.T) { AllowedScopes: []configv1alpha1.Scope{"openid", "offline_access", "username"}, }, }}, - inputSecrets: []runtime.Object{storageSecretForUIDWithData(testUID, secretStringDataWithOneClientSecret)}, + inputSecrets: []runtime.Object{testutil.OIDCClientSecretStorageSecretForUID(t, testNamespace, testUID, []string{testBcryptSecret1})}, wantAPIActions: 1, // one update wantResultingOIDCClients: []configv1alpha1.OIDCClient{{ ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234, UID: testUID}, @@ -892,7 +849,7 @@ func TestOIDCClientWatcherControllerSync(t *testing.T) { AllowedScopes: []configv1alpha1.Scope{"openid", "offline_access", "groups"}, }, }}, - inputSecrets: []runtime.Object{storageSecretForUIDWithData(testUID, secretStringDataWithOneClientSecret)}, + inputSecrets: []runtime.Object{testutil.OIDCClientSecretStorageSecretForUID(t, testNamespace, testUID, []string{testBcryptSecret1})}, wantAPIActions: 1, // one update wantResultingOIDCClients: []configv1alpha1.OIDCClient{{ ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234, UID: testUID}, @@ -916,7 +873,7 @@ func TestOIDCClientWatcherControllerSync(t *testing.T) { AllowedScopes: []configv1alpha1.Scope{"openid", "offline_access", "username", "groups"}, }, }}, - inputSecrets: []runtime.Object{storageSecretForUIDWithData(testUID, secretStringDataWithOneClientSecret)}, + inputSecrets: []runtime.Object{testutil.OIDCClientSecretStorageSecretForUID(t, testNamespace, testUID, []string{testBcryptSecret1})}, wantAPIActions: 1, // one update wantResultingOIDCClients: []configv1alpha1.OIDCClient{{ ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234, UID: testUID}, @@ -940,7 +897,7 @@ func TestOIDCClientWatcherControllerSync(t *testing.T) { AllowedScopes: []configv1alpha1.Scope{"openid", "username"}, }, }}, - inputSecrets: []runtime.Object{storageSecretForUIDWithData(testUID, secretStringDataWithOneClientSecret)}, + inputSecrets: []runtime.Object{testutil.OIDCClientSecretStorageSecretForUID(t, testNamespace, testUID, []string{testBcryptSecret1})}, wantAPIActions: 1, // one update wantResultingOIDCClients: []configv1alpha1.OIDCClient{{ ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234, UID: testUID}, @@ -964,7 +921,7 @@ func TestOIDCClientWatcherControllerSync(t *testing.T) { AllowedScopes: []configv1alpha1.Scope{"openid", "username"}, }, }}, - inputSecrets: []runtime.Object{storageSecretForUIDWithData(testUID, secretStringDataWithOneClientSecret)}, + inputSecrets: []runtime.Object{testutil.OIDCClientSecretStorageSecretForUID(t, testNamespace, testUID, []string{testBcryptSecret1})}, wantAPIActions: 1, // one update wantResultingOIDCClients: []configv1alpha1.OIDCClient{{ ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234, UID: testUID}, @@ -988,7 +945,7 @@ func TestOIDCClientWatcherControllerSync(t *testing.T) { AllowedScopes: []configv1alpha1.Scope{"openid", "username", "groups"}, }, }}, - inputSecrets: []runtime.Object{storageSecretForUIDWithData(testUID, secretStringDataWithOneClientSecret)}, + inputSecrets: []runtime.Object{testutil.OIDCClientSecretStorageSecretForUID(t, testNamespace, testUID, []string{testBcryptSecret1})}, wantAPIActions: 1, // one update wantResultingOIDCClients: []configv1alpha1.OIDCClient{{ ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234, UID: testUID}, diff --git a/internal/crud/crud.go b/internal/crud/crud.go index 29ad6b65..2d33959a 100644 --- a/internal/crud/crud.go +++ b/internal/crud/crud.go @@ -193,14 +193,19 @@ func (s *secretsStorage) toSecret(signature, resourceVersion string, data JSON, labelsToAdd[labelName] = labelValue } + var annotations map[string]string + if s.lifetime > 0 { + annotations = map[string]string{ + SecretLifetimeAnnotationKey: s.clock().Add(s.lifetime).UTC().Format(SecretLifetimeAnnotationDateFormat), + } + } + return &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ Name: s.GetName(signature), ResourceVersion: resourceVersion, Labels: labelsToAdd, - Annotations: map[string]string{ - SecretLifetimeAnnotationKey: s.clock().Add(s.lifetime).UTC().Format(SecretLifetimeAnnotationDateFormat), - }, + Annotations: annotations, OwnerReferences: nil, }, Data: map[string][]byte{ diff --git a/internal/crud/crud_test.go b/internal/crud/crud_test.go index 61720a0f..25ffdfad 100644 --- a/internal/crud/crud_test.go +++ b/internal/crud/crud_test.go @@ -62,6 +62,7 @@ func TestStorage(t *testing.T) { name string resource string mocks func(*testing.T, mocker) + lifetime func() time.Duration run func(*testing.T, Storage, *clocktesting.FakeClock) error wantActions []coretesting.Action wantSecrets []corev1.Secret @@ -1014,7 +1015,69 @@ func TestStorage(t *testing.T) { }, wantErr: "", }, + { + name: "create and get with infinite lifetime when lifetime is specified as zero", + resource: "access-tokens", + mocks: nil, + lifetime: func() time.Duration { return 0 }, // 0 == infinity + run: func(t *testing.T, storage Storage, fakeClock *clocktesting.FakeClock) error { + signature := hmac.AuthorizeCodeSignature(authorizationCode1) + require.NotEmpty(t, signature) + require.NotEmpty(t, validateSecretName(signature, false)) // signature is not valid secret name as-is + + data := &testJSON{Data: "create-and-get"} + rv1, err := storage.Create(ctx, signature, data, nil) + require.Empty(t, rv1) // fake client does not set this + require.NoError(t, err) + + out := &testJSON{} + rv2, err := storage.Get(ctx, signature, out) + require.Empty(t, rv2) // fake client does not set this + require.NoError(t, err) + require.Equal(t, data, out) + + return nil + }, + wantActions: []coretesting.Action{ + coretesting.NewCreateAction(secretsGVR, namespace, &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pinniped-storage-access-tokens-i6mhp4azwdxshgsy3s2mvedxpxuh3nudh3ot3m4xamlugj4e6qoq", + ResourceVersion: "", + // No garbage collection annotation was added. + Labels: map[string]string{ + "storage.pinniped.dev/type": "access-tokens", + }, + }, + Data: map[string][]byte{ + "pinniped-storage-data": []byte(`{"Data":"create-and-get"}`), + "pinniped-storage-version": []byte("1"), + }, + Type: "storage.pinniped.dev/access-tokens", + }), + coretesting.NewGetAction(secretsGVR, namespace, "pinniped-storage-access-tokens-i6mhp4azwdxshgsy3s2mvedxpxuh3nudh3ot3m4xamlugj4e6qoq"), + }, + wantSecrets: []corev1.Secret{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "pinniped-storage-access-tokens-i6mhp4azwdxshgsy3s2mvedxpxuh3nudh3ot3m4xamlugj4e6qoq", + Namespace: namespace, + ResourceVersion: "", + // No garbage collection annotation was added. + Labels: map[string]string{ + "storage.pinniped.dev/type": "access-tokens", + }, + }, + Data: map[string][]byte{ + "pinniped-storage-data": []byte(`{"Data":"create-and-get"}`), + "pinniped-storage-version": []byte("1"), + }, + Type: "storage.pinniped.dev/access-tokens", + }, + }, + wantErr: "", + }, } + for _, tt := range tests { tt := tt t.Run(tt.name, func(t *testing.T) { @@ -1024,9 +1087,13 @@ func TestStorage(t *testing.T) { if tt.mocks != nil { tt.mocks(t, client) } + useLifetime := lifetime + if tt.lifetime != nil { + useLifetime = tt.lifetime() + } secrets := client.CoreV1().Secrets(namespace) fakeClock := clocktesting.NewFakeClock(fakeNow) - storage := New(tt.resource, secrets, fakeClock.Now, lifetime) + storage := New(tt.resource, secrets, fakeClock.Now, useLifetime) err := tt.run(t, storage, fakeClock) diff --git a/internal/oidc/auth/auth_handler.go b/internal/oidc/auth/auth_handler.go index adbbec7c..370f8baa 100644 --- a/internal/oidc/auth/auth_handler.go +++ b/internal/oidc/auth/auth_handler.go @@ -20,6 +20,7 @@ import ( "go.pinniped.dev/internal/httputil/httperr" "go.pinniped.dev/internal/httputil/securityheader" "go.pinniped.dev/internal/oidc" + "go.pinniped.dev/internal/oidc/clientregistry" "go.pinniped.dev/internal/oidc/csrftoken" "go.pinniped.dev/internal/oidc/downstreamsession" "go.pinniped.dev/internal/oidc/login" @@ -126,6 +127,10 @@ func handleAuthRequestForLDAPUpstreamCLIFlow( return nil } + if !requireStaticClientForUsernameAndPasswordHeaders(w, oauthHelper, authorizeRequester) { + return nil + } + username, password, hadUsernamePasswordValues := requireNonEmptyUsernameAndPasswordHeaders(r, w, oauthHelper, authorizeRequester) if !hadUsernamePasswordValues { return nil @@ -199,6 +204,10 @@ func handleAuthRequestForOIDCUpstreamPasswordGrant( return nil } + if !requireStaticClientForUsernameAndPasswordHeaders(w, oauthHelper, authorizeRequester) { + return nil + } + username, password, hadUsernamePasswordValues := requireNonEmptyUsernameAndPasswordHeaders(r, w, oauthHelper, authorizeRequester) if !hadUsernamePasswordValues { return nil @@ -312,6 +321,15 @@ func handleAuthRequestForOIDCUpstreamBrowserFlow( return nil } +func requireStaticClientForUsernameAndPasswordHeaders(w http.ResponseWriter, oauthHelper fosite.OAuth2Provider, authorizeRequester fosite.AuthorizeRequester) bool { + isStaticClient := authorizeRequester.GetClient().GetID() == clientregistry.PinnipedCLIClientID + if !isStaticClient { + oidc.WriteAuthorizeError(w, oauthHelper, authorizeRequester, + fosite.ErrAccessDenied.WithHintf("This client is not allowed to submit username or password headers to this endpoint."), true) + } + return isStaticClient +} + func requireNonEmptyUsernameAndPasswordHeaders(r *http.Request, w http.ResponseWriter, oauthHelper fosite.OAuth2Provider, authorizeRequester fosite.AuthorizeRequester) (string, string, bool) { username := r.Header.Get(supervisoroidc.AuthorizeUsernameHeaderName) password := r.Header.Get(supervisoroidc.AuthorizePasswordHeaderName) @@ -330,10 +348,12 @@ func newAuthorizeRequest(r *http.Request, w http.ResponseWriter, oauthHelper fos return nil, false } - // Automatically grant the openid, offline_access, and pinniped:request-audience scopes, but only if they were requested. + // Automatically grant the openid, offline_access, pinniped:request-audience, and groups scopes, but only if they were requested. // Grant the openid scope (for now) if they asked for it so that `NewAuthorizeResponse` will perform its OIDC validations. // There don't seem to be any validations inside `NewAuthorizeResponse` related to the offline_access scope // at this time, however we will temporarily grant the scope just in case that changes in a future release of fosite. + // This is instead of asking the user to approve these scopes. Note that `NewAuthorizeRequest` would have returned + // an error if the client requested a scope that they are not allowed to request, so we don't need to worry about that here. downstreamsession.GrantScopesIfRequested(authorizeRequester, []string{coreosoidc.ScopeOpenID, coreosoidc.ScopeOfflineAccess, oidc.RequestAudienceScope, oidc.DownstreamGroupsScope}) return authorizeRequester, true diff --git a/internal/oidc/auth/auth_handler_test.go b/internal/oidc/auth/auth_handler_test.go index 8847d8c4..2cc98471 100644 --- a/internal/oidc/auth/auth_handler_test.go +++ b/internal/oidc/auth/auth_handler_test.go @@ -24,8 +24,12 @@ import ( "k8s.io/apiserver/pkg/authentication/user" "k8s.io/client-go/kubernetes/fake" v1 "k8s.io/client-go/kubernetes/typed/core/v1" + 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" "go.pinniped.dev/internal/here" "go.pinniped.dev/internal/oidc" @@ -67,11 +71,16 @@ func TestAuthorizationEndpoint(t *testing.T) { downstreamPKCEChallenge = "some-challenge" downstreamPKCEChallengeMethod = "S256" happyState = "8b-state" - downstreamClientID = "pinniped-cli" upstreamLDAPURL = "ldaps://some-ldap-host:123?base=ou%3Dusers%2Cdc%3Dpinniped%2Cdc%3Ddev" htmlContentType = "text/html; charset=utf-8" jsonContentType = "application/json; charset=utf-8" formContentType = "application/x-www-form-urlencoded" + + 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") @@ -177,6 +186,12 @@ func TestAuthorizationEndpoint(t *testing.T) { "state": happyState, } + fositeAccessDeniedWithUsernamePasswordHeadersDisallowedHintErrorQuery = map[string]string{ + "error": "access_denied", + "error_description": "The resource owner or authorization server denied the request. This client is not allowed to submit username or password headers to this endpoint.", + "state": happyState, + } + fositeAccessDeniedWithInvalidEmailVerifiedHintErrorQuery = map[string]string{ "error": "access_denied", "error_description": "The resource owner or authorization server denied the request. Reason: email_verified claim in upstream ID token has invalid format.", @@ -219,16 +234,18 @@ func TestAuthorizationEndpoint(t *testing.T) { jwksProviderIsUnused := jwks.NewDynamicJWKSProvider() timeoutsConfiguration := oidc.DefaultOIDCTimeoutsConfiguration() - createOauthHelperWithRealStorage := func(secretsClient v1.SecretInterface) (fosite.OAuth2Provider, *oidc.KubeStorage) { + 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, timeoutsConfiguration) + kubeOauthStore := oidc.NewKubeStorage(secretsClient, oidcClientsClient, timeoutsConfiguration) return oidc.FositeOauth2Helper(kubeOauthStore, downstreamIssuer, hmacSecretFunc, jwksProviderIsUnused, timeoutsConfiguration), kubeOauthStore } - // Configure fosite the same way that the production code would, using NullStorage to turn off storage. - nullOauthStore := oidc.NullStorage{} - oauthHelperWithNullStorage := oidc.FositeOauth2Helper(nullOauthStore, downstreamIssuer, hmacSecretFunc, jwksProviderIsUnused, timeoutsConfiguration) + 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) + return oidc.FositeOauth2Helper(nullOauthStore, downstreamIssuer, hmacSecretFunc, jwksProviderIsUnused, timeoutsConfiguration), nullOauthStore + } upstreamAuthURL, err := url.Parse("https://some-upstream-idp:8443/auth") require.NoError(t, err) @@ -381,7 +398,7 @@ func TestAuthorizationEndpoint(t *testing.T) { happyGetRequestQueryMap := map[string]string{ "response_type": "code", "scope": strings.Join(happyDownstreamScopesRequested, " "), - "client_id": downstreamClientID, + "client_id": pinnipedCLIClientID, "state": happyState, "nonce": downstreamNonce, "code_challenge": downstreamPKCEChallenge, @@ -494,6 +511,26 @@ 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)) + } + // Note that fosite puts the granted scopes as a param in the redirect URI even though the spec doesn't seem to require it happyAuthcodeDownstreamRedirectLocationRegexp := downstreamRedirectURI + `\?code=([^&]+)&scope=openid\+groups&state=` + happyState @@ -517,6 +554,7 @@ func TestAuthorizationEndpoint(t *testing.T) { csrfCookie string customUsernameHeader *string // nil means do not send header, empty means send header with empty value customPasswordHeader *string // nil means do not send header, empty means send header with empty value + kubeResources func(t *testing.T, supervisorClient *supervisorfake.Clientset, kubeClient *fake.Clientset) wantStatus int wantContentType string @@ -540,6 +578,7 @@ func TestAuthorizationEndpoint(t *testing.T) { wantDownstreamPKCEChallenge string wantDownstreamPKCEChallengeMethod string wantDownstreamNonce string + wantDownstreamClientID string // defaults to wanting "pinniped-cli" when not set wantUnnecessaryStoredRecords int wantPasswordGrantCall *expectedPasswordGrant wantDownstreamCustomSessionData *psession.CustomSessionData @@ -562,6 +601,24 @@ func TestAuthorizationEndpoint(t *testing.T) { wantUpstreamStateParamInLocationHeader: true, wantBodyStringWithLocationInHref: true, }, + { + name: "OIDC upstream browser flow happy path using GET without a CSRF cookie using a dynamic client", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(upstreamOIDCIdentityProviderBuilder().Build()), + kubeResources: addFullyCapableDynamicClientAndSecretToKubeResources, + generateCSRF: happyCSRFGenerator, + generatePKCE: happyPKCEGenerator, + generateNonce: happyNonceGenerator, + stateEncoder: happyStateEncoder, + cookieEncoder: happyCookieEncoder, + method: http.MethodGet, + path: modifiedHappyGetRequestPath(map[string]string{"client_id": dynamicClientID, "scope": allDynamicClientScopes}), + wantStatus: http.StatusSeeOther, + wantContentType: htmlContentType, + wantCSRFValueInCookieHeader: happyCSRF, + wantLocationHeader: expectedRedirectLocationForUpstreamOIDC(expectedUpstreamStateParam(map[string]string{"client_id": dynamicClientID, "scope": allDynamicClientScopes}, "", oidcUpstreamName, "oidc"), nil), + wantUpstreamStateParamInLocationHeader: true, + wantBodyStringWithLocationInHref: true, + }, { name: "LDAP upstream browser flow happy path using GET without a CSRF cookie", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithLDAP(&upstreamLDAPIdentityProvider), @@ -579,6 +636,24 @@ func TestAuthorizationEndpoint(t *testing.T) { wantUpstreamStateParamInLocationHeader: true, wantBodyStringWithLocationInHref: true, }, + { + name: "LDAP upstream browser flow happy path using GET without a CSRF cookie using a dynamic client", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithLDAP(&upstreamLDAPIdentityProvider), + kubeResources: addFullyCapableDynamicClientAndSecretToKubeResources, + generateCSRF: happyCSRFGenerator, + generatePKCE: happyPKCEGenerator, + generateNonce: happyNonceGenerator, + stateEncoder: happyStateEncoder, + cookieEncoder: happyCookieEncoder, + method: http.MethodGet, + path: modifiedHappyGetRequestPath(map[string]string{"client_id": dynamicClientID, "scope": allDynamicClientScopes}), + 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")}), + wantUpstreamStateParamInLocationHeader: true, + wantBodyStringWithLocationInHref: true, + }, { name: "Active Directory upstream browser flow happy path using GET without a CSRF cookie", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithActiveDirectory(&upstreamActiveDirectoryIdentityProvider), @@ -596,6 +671,24 @@ func TestAuthorizationEndpoint(t *testing.T) { wantUpstreamStateParamInLocationHeader: true, wantBodyStringWithLocationInHref: true, }, + { + name: "Active Directory upstream browser flow happy path using GET without a CSRF cookie using a dynamic client", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithActiveDirectory(&upstreamActiveDirectoryIdentityProvider), + kubeResources: addFullyCapableDynamicClientAndSecretToKubeResources, + generateCSRF: happyCSRFGenerator, + generatePKCE: happyPKCEGenerator, + generateNonce: happyNonceGenerator, + stateEncoder: happyStateEncoder, + cookieEncoder: happyCookieEncoder, + method: http.MethodGet, + path: modifiedHappyGetRequestPath(map[string]string{"client_id": dynamicClientID, "scope": allDynamicClientScopes}), + 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")}), + wantUpstreamStateParamInLocationHeader: true, + wantBodyStringWithLocationInHref: true, + }, { name: "OIDC upstream password grant happy path using GET", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(passwordGrantUpstreamOIDCIdentityProviderBuilder().Build()), @@ -730,6 +823,26 @@ func TestAuthorizationEndpoint(t *testing.T) { wantLocationHeader: expectedRedirectLocationForUpstreamOIDC(expectedUpstreamStateParam(nil, "", oidcUpstreamName, "oidc"), nil), wantUpstreamStateParamInLocationHeader: true, }, + { + name: "OIDC upstream browser flow happy path using POST with a dynamic client", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(upstreamOIDCIdentityProviderBuilder().Build()), + kubeResources: addFullyCapableDynamicClientAndSecretToKubeResources, + generateCSRF: happyCSRFGenerator, + generatePKCE: happyPKCEGenerator, + generateNonce: happyNonceGenerator, + stateEncoder: happyStateEncoder, + cookieEncoder: happyCookieEncoder, + method: http.MethodPost, + path: "/some/path", + contentType: formContentType, + body: encodeQuery(modifiedHappyGetRequestQueryMap(map[string]string{"client_id": dynamicClientID, "scope": allDynamicClientScopes})), + wantStatus: http.StatusSeeOther, + wantContentType: "", + wantBodyString: "", + wantCSRFValueInCookieHeader: happyCSRF, + wantLocationHeader: expectedRedirectLocationForUpstreamOIDC(expectedUpstreamStateParam(map[string]string{"client_id": dynamicClientID, "scope": allDynamicClientScopes}, "", oidcUpstreamName, "oidc"), nil), + wantUpstreamStateParamInLocationHeader: true, + }, { name: "LDAP upstream browser flow happy path using POST", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithLDAP(&upstreamLDAPIdentityProvider), @@ -749,6 +862,26 @@ func TestAuthorizationEndpoint(t *testing.T) { wantLocationHeader: urlWithQuery(downstreamIssuer+"/login", map[string]string{"state": expectedUpstreamStateParam(nil, "", ldapUpstreamName, "ldap")}), wantUpstreamStateParamInLocationHeader: true, }, + { + name: "LDAP upstream browser flow happy path using POST with a dynamic client", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithLDAP(&upstreamLDAPIdentityProvider), + kubeResources: addFullyCapableDynamicClientAndSecretToKubeResources, + generateCSRF: happyCSRFGenerator, + generatePKCE: happyPKCEGenerator, + generateNonce: happyNonceGenerator, + stateEncoder: happyStateEncoder, + cookieEncoder: happyCookieEncoder, + method: http.MethodPost, + path: "/some/path", + contentType: formContentType, + body: encodeQuery(modifiedHappyGetRequestQueryMap(map[string]string{"client_id": dynamicClientID, "scope": allDynamicClientScopes})), + 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")}), + wantUpstreamStateParamInLocationHeader: true, + }, { name: "Active Directory upstream browser flow happy path using POST", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithActiveDirectory(&upstreamActiveDirectoryIdentityProvider), @@ -768,6 +901,26 @@ func TestAuthorizationEndpoint(t *testing.T) { wantLocationHeader: urlWithQuery(downstreamIssuer+"/login", map[string]string{"state": expectedUpstreamStateParam(nil, "", activeDirectoryUpstreamName, "activedirectory")}), wantUpstreamStateParamInLocationHeader: true, }, + { + name: "Active Directory upstream browser flow happy path using POST with a dynamic client", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithActiveDirectory(&upstreamActiveDirectoryIdentityProvider), + kubeResources: addFullyCapableDynamicClientAndSecretToKubeResources, + generateCSRF: happyCSRFGenerator, + generatePKCE: happyPKCEGenerator, + generateNonce: happyNonceGenerator, + stateEncoder: happyStateEncoder, + cookieEncoder: happyCookieEncoder, + method: http.MethodPost, + path: "/some/path", + contentType: formContentType, + body: encodeQuery(modifiedHappyGetRequestQueryMap(map[string]string{"client_id": dynamicClientID, "scope": allDynamicClientScopes})), + 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")}), + wantUpstreamStateParamInLocationHeader: true, + }, { name: "OIDC upstream password grant happy path using POST", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(passwordGrantUpstreamOIDCIdentityProviderBuilder().Build()), @@ -945,6 +1098,32 @@ func TestAuthorizationEndpoint(t *testing.T) { wantUpstreamStateParamInLocationHeader: true, wantBodyStringWithLocationInHref: true, }, + { + name: "OIDC upstream browser flow happy path using dynamic client when downstream redirect uri matches what is configured for client except for the port number", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(upstreamOIDCIdentityProviderBuilder().Build()), + kubeResources: addFullyCapableDynamicClientAndSecretToKubeResources, + generateCSRF: happyCSRFGenerator, + generatePKCE: happyPKCEGenerator, + generateNonce: happyNonceGenerator, + stateEncoder: happyStateEncoder, + cookieEncoder: happyCookieEncoder, + method: http.MethodGet, + path: modifiedHappyGetRequestPath(map[string]string{ + "redirect_uri": downstreamRedirectURIWithDifferentPort, // not the same port number that is registered for the client + "client_id": dynamicClientID, + "scope": allDynamicClientScopes, + }), + wantStatus: http.StatusSeeOther, + wantContentType: htmlContentType, + wantCSRFValueInCookieHeader: happyCSRF, + 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, + }, "", oidcUpstreamName, "oidc"), nil), + wantUpstreamStateParamInLocationHeader: true, + wantBodyStringWithLocationInHref: true, + }, { name: "OIDC upstream password grant happy path when downstream redirect uri matches what is configured for client except for the port number", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(passwordGrantUpstreamOIDCIdentityProviderBuilder().Build()), @@ -1342,6 +1521,45 @@ func TestAuthorizationEndpoint(t *testing.T) { wantLocationHeader: urlWithQuery(downstreamRedirectURI, fositeAccessDeniedWithPasswordGrantDisallowedHintErrorQuery), wantBodyString: "", }, + { + name: "dynamic clients are not allowed to use OIDC password grant because we don't want them to handle user credentials", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(passwordGrantUpstreamOIDCIdentityProviderBuilder().Build()), + kubeResources: addFullyCapableDynamicClientAndSecretToKubeResources, + method: http.MethodGet, + path: modifiedHappyGetRequestPath(map[string]string{"client_id": dynamicClientID, "scope": allDynamicClientScopes}), + customUsernameHeader: pointer.StringPtr(oidcUpstreamUsername), + customPasswordHeader: pointer.StringPtr(oidcUpstreamPassword), + wantStatus: http.StatusFound, + wantContentType: jsonContentType, + wantLocationHeader: urlWithQuery(downstreamRedirectURI, fositeAccessDeniedWithUsernamePasswordHeadersDisallowedHintErrorQuery), + wantBodyString: "", + }, + { + name: "dynamic clients are not allowed to use LDAP CLI-flow authentication because we don't want them to handle user credentials", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithLDAP(&upstreamLDAPIdentityProvider), + kubeResources: addFullyCapableDynamicClientAndSecretToKubeResources, + method: http.MethodGet, + path: modifiedHappyGetRequestPath(map[string]string{"client_id": dynamicClientID, "scope": allDynamicClientScopes}), + customUsernameHeader: pointer.StringPtr(happyLDAPUsername), + customPasswordHeader: pointer.StringPtr(happyLDAPPassword), + wantStatus: http.StatusFound, + wantContentType: jsonContentType, + wantLocationHeader: urlWithQuery(downstreamRedirectURI, fositeAccessDeniedWithUsernamePasswordHeadersDisallowedHintErrorQuery), + wantBodyString: "", + }, + { + name: "dynamic clients are not allowed to use Active Directory CLI-flow authentication because we don't want them to handle user credentials", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithActiveDirectory(&upstreamActiveDirectoryIdentityProvider), + kubeResources: addFullyCapableDynamicClientAndSecretToKubeResources, + method: http.MethodGet, + path: modifiedHappyGetRequestPath(map[string]string{"client_id": dynamicClientID, "scope": allDynamicClientScopes}), + customUsernameHeader: pointer.StringPtr(happyLDAPUsername), + customPasswordHeader: pointer.StringPtr(happyLDAPPassword), + wantStatus: http.StatusFound, + wantContentType: jsonContentType, + wantLocationHeader: urlWithQuery(downstreamRedirectURI, fositeAccessDeniedWithUsernamePasswordHeadersDisallowedHintErrorQuery), + wantBodyString: "", + }, { name: "downstream redirect uri does not match what is configured for client when using OIDC upstream browser flow", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(upstreamOIDCIdentityProviderBuilder().Build()), @@ -1358,6 +1576,25 @@ func TestAuthorizationEndpoint(t *testing.T) { wantContentType: jsonContentType, wantBodyJSON: fositeInvalidRedirectURIErrorBody, }, + { + name: "downstream redirect uri does not match what is configured for client when using OIDC upstream browser flow with a dynamic client", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(upstreamOIDCIdentityProviderBuilder().Build()), + kubeResources: addFullyCapableDynamicClientAndSecretToKubeResources, + generateCSRF: happyCSRFGenerator, + generatePKCE: happyPKCEGenerator, + generateNonce: happyNonceGenerator, + stateEncoder: happyStateEncoder, + cookieEncoder: happyCookieEncoder, + method: http.MethodGet, + 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, + }), + wantStatus: http.StatusBadRequest, + wantContentType: jsonContentType, + wantBodyJSON: fositeInvalidRedirectURIErrorBody, + }, { name: "downstream redirect uri does not match what is configured for client when using OIDC upstream password grant", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(passwordGrantUpstreamOIDCIdentityProviderBuilder().Build()), @@ -1455,6 +1692,26 @@ func TestAuthorizationEndpoint(t *testing.T) { wantLocationHeader: urlWithQuery(downstreamRedirectURI, fositeUnsupportedResponseTypeErrorQuery), wantBodyString: "", }, + { + name: "response type is unsupported when using OIDC upstream browser flow with dynamic client", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(upstreamOIDCIdentityProviderBuilder().Build()), + kubeResources: addFullyCapableDynamicClientAndSecretToKubeResources, + generateCSRF: happyCSRFGenerator, + generatePKCE: happyPKCEGenerator, + generateNonce: happyNonceGenerator, + stateEncoder: happyStateEncoder, + cookieEncoder: happyCookieEncoder, + method: http.MethodGet, + path: modifiedHappyGetRequestPath(map[string]string{ + "response_type": "unsupported", + "client_id": dynamicClientID, + "scope": allDynamicClientScopes, + }), + wantStatus: http.StatusSeeOther, + wantContentType: jsonContentType, + wantLocationHeader: urlWithQuery(downstreamRedirectURI, fositeUnsupportedResponseTypeErrorQuery), + wantBodyString: "", + }, { name: "response type is unsupported when using OIDC upstream password grant", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(passwordGrantUpstreamOIDCIdentityProviderBuilder().Build()), @@ -1489,6 +1746,21 @@ func TestAuthorizationEndpoint(t *testing.T) { wantLocationHeader: urlWithQuery(downstreamRedirectURI, fositeUnsupportedResponseTypeErrorQuery), wantBodyString: "", }, + { + name: "response type is unsupported when using LDAP browser upstream with dynamic client", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithLDAP(&upstreamLDAPIdentityProvider), + kubeResources: addFullyCapableDynamicClientAndSecretToKubeResources, + method: http.MethodGet, + path: modifiedHappyGetRequestPath(map[string]string{ + "response_type": "unsupported", + "client_id": dynamicClientID, + "scope": allDynamicClientScopes, + }), + wantStatus: http.StatusSeeOther, + wantContentType: jsonContentType, + wantLocationHeader: urlWithQuery(downstreamRedirectURI, fositeUnsupportedResponseTypeErrorQuery), + wantBodyString: "", + }, { name: "response type is unsupported when using active directory cli upstream", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithActiveDirectory(&upstreamActiveDirectoryIdentityProvider), @@ -1511,6 +1783,21 @@ func TestAuthorizationEndpoint(t *testing.T) { wantLocationHeader: urlWithQuery(downstreamRedirectURI, fositeUnsupportedResponseTypeErrorQuery), wantBodyString: "", }, + { + name: "response type is unsupported when using active directory browser upstream with dynamic client", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithActiveDirectory(&upstreamActiveDirectoryIdentityProvider), + kubeResources: addFullyCapableDynamicClientAndSecretToKubeResources, + method: http.MethodGet, + path: modifiedHappyGetRequestPath(map[string]string{ + "response_type": "unsupported", + "client_id": dynamicClientID, + "scope": allDynamicClientScopes, + }), + wantStatus: http.StatusSeeOther, + wantContentType: jsonContentType, + wantLocationHeader: urlWithQuery(downstreamRedirectURI, fositeUnsupportedResponseTypeErrorQuery), + wantBodyString: "", + }, { name: "downstream scopes do not match what is configured for client using OIDC upstream browser flow", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(upstreamOIDCIdentityProviderBuilder().Build()), @@ -1526,6 +1813,22 @@ func TestAuthorizationEndpoint(t *testing.T) { wantLocationHeader: urlWithQuery(downstreamRedirectURI, fositeInvalidScopeErrorQuery), wantBodyString: "", }, + { + name: "downstream scopes do not match what is configured for client using OIDC upstream browser flow with dynamic client", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(upstreamOIDCIdentityProviderBuilder().Build()), + kubeResources: addFullyCapableDynamicClientAndSecretToKubeResources, + generateCSRF: happyCSRFGenerator, + generatePKCE: happyPKCEGenerator, + generateNonce: happyNonceGenerator, + stateEncoder: happyStateEncoder, + cookieEncoder: happyCookieEncoder, + method: http.MethodGet, + path: modifiedHappyGetRequestPath(map[string]string{"client_id": dynamicClientID, "scope": "openid tuna"}), + wantStatus: http.StatusSeeOther, + wantContentType: jsonContentType, + wantLocationHeader: urlWithQuery(downstreamRedirectURI, fositeInvalidScopeErrorQuery), + wantBodyString: "", + }, { name: "downstream scopes do not match what is configured for client using OIDC upstream password grant", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(passwordGrantUpstreamOIDCIdentityProviderBuilder().Build()), @@ -1552,6 +1855,21 @@ func TestAuthorizationEndpoint(t *testing.T) { wantContentType: htmlContentType, wantBodyRegex: ` 0 { + // Invalid: some stored client secrets were not valid. + conditions = append(conditions, &v1alpha1.Condition{ + Type: clientSecretExists, + Status: v1alpha1.ConditionFalse, + Reason: reasonInvalidClientSecretFound, + Message: fmt.Sprintf("%d stored client secrets found, but some were invalid, so none will be used: %s", + storedClientSecretsCount, strings.Join(bcryptErrs, "; ")), + }) + return conditions, emptyList + } + + // Valid: has at least one client secret stored for this OIDC client, and all stored client secrets are valid. + conditions = append(conditions, &v1alpha1.Condition{ + Type: clientSecretExists, + Status: v1alpha1.ConditionTrue, + Reason: reasonSuccess, + Message: fmt.Sprintf("%d client secret(s) found", storedClientSecretsCount), + }) + return conditions, storedClientSecret.SecretHashes +} + +func allowedGrantTypesContains(haystack *v1alpha1.OIDCClient, needle string) bool { + for _, hay := range haystack.Spec.AllowedGrantTypes { + if hay == v1alpha1.GrantType(needle) { + return true + } + } + return false +} + +func allowedScopesContains(haystack *v1alpha1.OIDCClient, needle string) bool { + for _, hay := range haystack.Spec.AllowedScopes { + if hay == v1alpha1.Scope(needle) { + return true + } + } + return false +} diff --git a/internal/oidc/provider/manager/manager.go b/internal/oidc/provider/manager/manager.go index 2833efa2..83a91d07 100644 --- a/internal/oidc/provider/manager/manager.go +++ b/internal/oidc/provider/manager/manager.go @@ -10,6 +10,7 @@ import ( corev1client "k8s.io/client-go/kubernetes/typed/core/v1" + "go.pinniped.dev/generated/latest/client/supervisor/clientset/versioned/typed/config/v1alpha1" "go.pinniped.dev/internal/oidc" "go.pinniped.dev/internal/oidc/auth" "go.pinniped.dev/internal/oidc/callback" @@ -39,6 +40,7 @@ type Manager struct { upstreamIDPs oidc.UpstreamIdentityProvidersLister // in-memory cache of upstream IDPs secretCache *secret.Cache // in-memory cache of cryptographic material secretsClient corev1client.SecretInterface + oidcClientsClient v1alpha1.OIDCClientInterface } // NewManager returns an empty Manager. @@ -51,6 +53,7 @@ func NewManager( upstreamIDPs oidc.UpstreamIdentityProvidersLister, secretCache *secret.Cache, secretsClient corev1client.SecretInterface, + oidcClientsClient v1alpha1.OIDCClientInterface, ) *Manager { return &Manager{ providerHandlers: make(map[string]http.Handler), @@ -59,6 +62,7 @@ func NewManager( upstreamIDPs: upstreamIDPs, secretCache: secretCache, secretsClient: secretsClient, + oidcClientsClient: oidcClientsClient, } } @@ -93,10 +97,22 @@ 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.NullStorage{}, issuer, tokenHMACKeyGetter, nil, timeoutsConfiguration) + oauthHelperWithNullStorage := oidc.FositeOauth2Helper( + oidc.NewNullStorage(m.secretsClient, m.oidcClientsClient), + issuer, + tokenHMACKeyGetter, + nil, + timeoutsConfiguration, + ) // 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, timeoutsConfiguration), issuer, tokenHMACKeyGetter, m.dynamicJWKSProvider, timeoutsConfiguration) + oauthHelperWithKubeStorage := oidc.FositeOauth2Helper( + oidc.NewKubeStorage(m.secretsClient, m.oidcClientsClient, timeoutsConfiguration), + issuer, + tokenHMACKeyGetter, + m.dynamicJWKSProvider, + timeoutsConfiguration, + ) var upstreamStateEncoder = dynamiccodec.New( timeoutsConfiguration.UpstreamStateParamLifespan, diff --git a/internal/oidc/provider/manager/manager_test.go b/internal/oidc/provider/manager/manager_test.go index 1f18dcf7..272387e9 100644 --- a/internal/oidc/provider/manager/manager_test.go +++ b/internal/oidc/provider/manager/manager_test.go @@ -15,18 +15,18 @@ import ( "strings" "testing" - "go.pinniped.dev/internal/secret" - "github.com/sclevine/spec" "github.com/stretchr/testify/require" "gopkg.in/square/go-jose.v2" "k8s.io/client-go/kubernetes/fake" + supervisorfake "go.pinniped.dev/generated/latest/client/supervisor/clientset/versioned/fake" "go.pinniped.dev/internal/here" "go.pinniped.dev/internal/oidc" "go.pinniped.dev/internal/oidc/discovery" "go.pinniped.dev/internal/oidc/jwks" "go.pinniped.dev/internal/oidc/provider" + "go.pinniped.dev/internal/secret" "go.pinniped.dev/internal/testutil" "go.pinniped.dev/internal/testutil/oidctestutil" "go.pinniped.dev/pkg/oidcclient/nonce" @@ -271,6 +271,7 @@ func TestManager(t *testing.T) { kubeClient = fake.NewSimpleClientset() secretsClient := kubeClient.CoreV1().Secrets("some-namespace") + oidcClientsClient := supervisorfake.NewSimpleClientset().ConfigV1alpha1().OIDCClients("some-namespace") cache := secret.Cache{} cache.SetCSRFCookieEncoderHashKey([]byte("fake-csrf-hash-secret")) @@ -283,7 +284,7 @@ func TestManager(t *testing.T) { cache.SetStateEncoderHashKey(issuer2, []byte("some-state-encoder-hash-key-2")) cache.SetStateEncoderBlockKey(issuer2, []byte("16-bytes-STATE02")) - subject = NewManager(nextHandler, dynamicJWKSProvider, idpLister, &cache, secretsClient) + subject = NewManager(nextHandler, dynamicJWKSProvider, idpLister, &cache, secretsClient, oidcClientsClient) }) when("given no providers via SetProviders()", func() { diff --git a/internal/oidc/token/token_handler_test.go b/internal/oidc/token/token_handler_test.go index 0fc1143d..5bdd3688 100644 --- a/internal/oidc/token/token_handler_test.go +++ b/internal/oidc/token/token_handler_test.go @@ -37,6 +37,7 @@ import ( "k8s.io/client-go/kubernetes/fake" v1 "k8s.io/client-go/kubernetes/typed/core/v1" + supervisorfake "go.pinniped.dev/generated/latest/client/supervisor/clientset/versioned/fake" "go.pinniped.dev/internal/crud" "go.pinniped.dev/internal/fositestorage/accesstoken" "go.pinniped.dev/internal/fositestorage/authorizationcode" @@ -3068,10 +3069,11 @@ func exchangeAuthcodeForTokens(t *testing.T, test authcodeExchangeInputs, idps p client := fake.NewSimpleClientset() secrets = client.CoreV1().Secrets("some-namespace") + oidcClientsClient := supervisorfake.NewSimpleClientset().ConfigV1alpha1().OIDCClients("some-namespace") var oauthHelper fosite.OAuth2Provider - oauthStore = oidc.NewKubeStorage(secrets, oidc.DefaultOIDCTimeoutsConfiguration()) + oauthStore = oidc.NewKubeStorage(secrets, oidcClientsClient, oidc.DefaultOIDCTimeoutsConfiguration()) if test.makeOathHelper != nil { oauthHelper, authCode, jwtSigningKey = test.makeOathHelper(t, authRequest, oauthStore, test.customSessionData) } else { diff --git a/internal/oidc/token_exchange.go b/internal/oidc/token_exchange.go index a7a7812b..4c2f5500 100644 --- a/internal/oidc/token_exchange.go +++ b/internal/oidc/token_exchange.go @@ -14,6 +14,8 @@ import ( "github.com/ory/fosite/handler/oauth2" "github.com/ory/fosite/handler/openid" "github.com/pkg/errors" + + "go.pinniped.dev/internal/oidc/clientregistry" ) const ( @@ -142,8 +144,8 @@ func (t *TokenExchangeHandler) validateParams(params url.Values) (*stsParams, er if strings.Contains(result.requestedAudience, ".pinniped.dev") { return nil, fosite.ErrInvalidRequest.WithHintf("requested audience cannot contain '.pinniped.dev'") } - if result.requestedAudience == "pinniped-cli" { - return nil, fosite.ErrInvalidRequest.WithHintf("requested audience cannot equal 'pinniped-cli'") + if result.requestedAudience == clientregistry.PinnipedCLIClientID { + return nil, fosite.ErrInvalidRequest.WithHintf("requested audience cannot equal '%s'", clientregistry.PinnipedCLIClientID) } return &result, nil diff --git a/internal/oidcclientsecretstorage/oidcclientsecretstorage.go b/internal/oidcclientsecretstorage/oidcclientsecretstorage.go index 257e674c..7bec307e 100644 --- a/internal/oidcclientsecretstorage/oidcclientsecretstorage.go +++ b/internal/oidcclientsecretstorage/oidcclientsecretstorage.go @@ -4,11 +4,14 @@ package oidcclientsecretstorage import ( + "context" "encoding/base64" "fmt" "time" - v1 "k8s.io/api/core/v1" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" corev1client "k8s.io/client-go/kubernetes/typed/core/v1" @@ -26,6 +29,7 @@ const ( type OIDCClientSecretStorage struct { storage crud.Storage + secrets corev1client.SecretInterface } // StoredClientSecret defines the format of the content of a client's secrets when stored in a Secret @@ -39,12 +43,27 @@ type StoredClientSecret struct { } func New(secrets corev1client.SecretInterface, clock func() time.Time) *OIDCClientSecretStorage { - // TODO make lifetime = 0 mean that it does not get annotated with any garbage collection annotation - return &OIDCClientSecretStorage{storage: crud.New(TypeLabelValue, secrets, clock, 0)} + return &OIDCClientSecretStorage{ + storage: crud.New(TypeLabelValue, secrets, clock, 0), + secrets: secrets, + } } // TODO expose other methods as needed for get, create, update, etc. +// GetStorageSecret gets the corev1.Secret which is used to store the client secrets for the given client. +// Returns nil,nil when the corev1.Secret was not found, as this is not an error for a client to not have any secrets yet. +func (s *OIDCClientSecretStorage) GetStorageSecret(ctx context.Context, oidcClientUID types.UID) (*corev1.Secret, error) { + secret, err := s.secrets.Get(ctx, s.GetName(oidcClientUID), metav1.GetOptions{}) + if errors.IsNotFound(err) { + return nil, nil + } + if err != nil { + return nil, err + } + return secret, nil +} + // GetName returns the name of the Secret which would be used to store data for the given signature. func (s *OIDCClientSecretStorage) GetName(oidcClientUID types.UID) string { // Avoid having s.storage.GetName() base64 decode something that wasn't ever encoded by encoding it here. @@ -53,7 +72,7 @@ func (s *OIDCClientSecretStorage) GetName(oidcClientUID types.UID) string { } // ReadFromSecret reads the contents of a Secret as a StoredClientSecret. -func ReadFromSecret(secret *v1.Secret) (*StoredClientSecret, error) { +func ReadFromSecret(secret *corev1.Secret) (*StoredClientSecret, error) { storedClientSecret := &StoredClientSecret{} err := crud.FromSecret(TypeLabelValue, secret, storedClientSecret) if err != nil { diff --git a/internal/oidcclientsecretstorage/oidcclientsecretstorage_test.go b/internal/oidcclientsecretstorage/oidcclientsecretstorage_test.go index ac81565a..09ff908c 100644 --- a/internal/oidcclientsecretstorage/oidcclientsecretstorage_test.go +++ b/internal/oidcclientsecretstorage/oidcclientsecretstorage_test.go @@ -9,6 +9,8 @@ import ( "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + "go.pinniped.dev/internal/testutil" ) func TestGetName(t *testing.T) { @@ -106,6 +108,31 @@ func TestReadFromSecret(t *testing.T) { }, wantErr: "secret storage data has incorrect version", }, + { + name: "OIDCClientSecretStorageSecretForUID() test helper generates readable format, to ensure that test helpers are kept up to date", + secret: testutil.OIDCClientSecretStorageSecretForUID(t, + "some-namespace", "some-uid", []string{"first-hash", "second-hash"}, + ), + wantStored: &StoredClientSecret{ + Version: "1", + SecretHashes: []string{"first-hash", "second-hash"}, + }, + }, + { + name: "OIDCClientSecretStorageSecretWithoutName() test helper generates readable format, to ensure that test helpers are kept up to date", + secret: testutil.OIDCClientSecretStorageSecretWithoutName(t, + "some-namespace", []string{"first-hash", "second-hash"}, + ), + wantStored: &StoredClientSecret{ + Version: "1", + SecretHashes: []string{"first-hash", "second-hash"}, + }, + }, + { + name: "OIDCClientSecretStorageSecretForUIDWithWrongVersion() test helper generates readable format, to ensure that test helpers are kept up to date", + secret: testutil.OIDCClientSecretStorageSecretForUIDWithWrongVersion(t, "some-namespace", "some-uid"), + wantErr: "OIDC client secret storage data has wrong version: OIDC client secret storage has version wrong-version instead of 1", + }, } for _, tt := range tests { diff --git a/internal/supervisor/server/server.go b/internal/supervisor/server/server.go index 677165ee..ac71376a 100644 --- a/internal/supervisor/server/server.go +++ b/internal/supervisor/server/server.go @@ -439,6 +439,7 @@ func runSupervisor(ctx context.Context, podInfo *downward.PodInfo, cfg *supervis dynamicUpstreamIDPProvider, &secretCache, clientWithoutLeaderElection.Kubernetes.CoreV1().Secrets(serverInstallationNamespace), // writes to kube storage are allowed for non-leaders + clientWithoutLeaderElection.PinnipedSupervisor.ConfigV1alpha1().OIDCClients(serverInstallationNamespace), ) // Get the "real" name of the client secret supervisor API group (i.e., the API group name with the diff --git a/internal/testutil/oidcclientsecretstorage.go b/internal/testutil/oidcclientsecretstorage.go new file mode 100644 index 00000000..b7904fc6 --- /dev/null +++ b/internal/testutil/oidcclientsecretstorage.go @@ -0,0 +1,51 @@ +// Copyright 2022 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package testutil + +import ( + "encoding/base32" + "encoding/json" + "strings" + "testing" + + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +func secretNameForUID(uid string) string { + // See GetName() in OIDCClientSecretStorage for how the production code determines the Secret name. + // This test helper is intended to choose the same name. + return "pinniped-storage-oidc-client-secret-" + + strings.ToLower(base32.StdEncoding.WithPadding(base32.NoPadding).EncodeToString([]byte(uid))) +} + +func OIDCClientSecretStorageSecretWithoutName(t *testing.T, namespace string, hashes []string) *corev1.Secret { + hashesJSON, err := json.Marshal(hashes) + require.NoError(t, err) // this shouldn't really happen since we can always encode a slice of strings + + return &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: namespace, + Labels: map[string]string{"storage.pinniped.dev/type": "oidc-client-secret"}, + }, + Type: "storage.pinniped.dev/oidc-client-secret", + Data: map[string][]byte{ + "pinniped-storage-data": []byte(`{"version":"1","hashes":` + string(hashesJSON) + `}`), + "pinniped-storage-version": []byte("1"), + }, + } +} + +func OIDCClientSecretStorageSecretForUID(t *testing.T, namespace string, oidcClientUID string, hashes []string) *corev1.Secret { + secret := OIDCClientSecretStorageSecretWithoutName(t, namespace, hashes) + secret.Name = secretNameForUID(oidcClientUID) + return secret +} + +func OIDCClientSecretStorageSecretForUIDWithWrongVersion(t *testing.T, namespace string, oidcClientUID string) *corev1.Secret { + secret := OIDCClientSecretStorageSecretForUID(t, namespace, oidcClientUID, []string{}) + secret.Data["pinniped-storage-data"] = []byte(`{"version":"wrong-version","hashes":[]}`) + return secret +} diff --git a/test/integration/supervisor_login_test.go b/test/integration/supervisor_login_test.go index 1d43cdd0..af134fc1 100644 --- a/test/integration/supervisor_login_test.go +++ b/test/integration/supervisor_login_test.go @@ -156,29 +156,90 @@ func TestSupervisorLogin_Browser(t *testing.T) { return ldapIDP, secret } + // These tests attempt to exercise the entire login and refresh flow of the Supervisor for various cases. + // They do not use the Pinniped CLI as the client, which allows them to exercise the Supervisor as an + // OIDC provider in ways that the CLI might not use. Similar tests exist using the CLI in e2e_test.go. + // + // Each of these tests perform the following flow: + // 1. Create a FederationDomain with TLS configured and wait for its JWKS endpoint to be available. + // 2. Configure an IDP CR. + // 3. Call the authorization endpoint and log in as a specific user. + // Note that these tests do not use form_post response type (which is tested by e2e_test.go). + // 4. Listen on a local callback server for the authorization redirect, and assert that it was success or failure. + // 5. Call the token endpoint to exchange the authcode. + // 6. Call the token endpoint to perform the RFC8693 token exchange for the cluster-scoped ID token. + // 7. Potentially edit the refresh session data or IDP settings before the refresh. + // 8. Call the token endpoint to perform a refresh, and expect it to succeed. + // 9. Call the token endpoint again to perform another RFC8693 token exchange for the cluster-scoped ID token, + // this time using the recently refreshed tokens when submitting the request. + // 10. Potentially edit the refresh session data or IDP settings again, this time in such a way that the next + // refresh should fail. If done, then perform one more refresh and expect failure. tests := []struct { - name string - maybeSkip func(t *testing.T) - createTestUser func(t *testing.T) (string, string) - deleteTestUser func(t *testing.T, username string) - requestAuthorization func(t *testing.T, downstreamIssuer, downstreamAuthorizeURL, downstreamCallbackURL, username, password string, httpClient *http.Client) - createIDP func(t *testing.T) string - requestTokenExchangeAud string - downstreamScopes []string - wantLocalhostCallbackToNeverHappen bool - wantDownstreamIDTokenSubjectToMatch string - wantDownstreamIDTokenUsernameToMatch func(username string) string - wantDownstreamIDTokenGroups []string - wantErrorDescription string - wantErrorType string - wantTokenExchangeResponse func(t *testing.T, status int, body string) + name string - // Either revoke the user's session on the upstream provider, or manipulate the user's session + // This required function might choose to skip the test case, for example if the LDAP server is not + // available for an LDAP test. + maybeSkip func(t *testing.T) + + // This required function should configure an IDP CR. It should also wait for it to be ready and schedule + // its cleanup. Return the name of the IDP CR. + createIDP func(t *testing.T) string + + // Optionally create an OIDCClient CR for the test to use. Return the client ID and client secret for the + // test to use. When not set, the test will default to using the "pinniped-cli" static client with no secret. + // When a client secret is returned, it will be used for authcode exchange, refresh requests, and RFC8693 + // token exchanges for cluster-scoped tokens (client secrets are not needed in authorization requests). + createOIDCClient func(t *testing.T, callbackURL string) (string, string) + + // Optionally return the username and password for the test to use when logging in. This username/password + // will be passed to requestAuthorization(), or empty strings will be passed to indicate that the defaults + // should be used. If there is any cleanup required, then this function should also schedule that cleanup. + testUser func(t *testing.T) (string, string) + + // This required function should call the authorization endpoint using the given URL and also perform whatever + // interactions are needed to log in as the user. + requestAuthorization func(t *testing.T, downstreamIssuer, downstreamAuthorizeURL, downstreamCallbackURL, username, password string, httpClient *http.Client) + + // This string will be used as the requested audience in the RFC8693 token exchange for + // the cluster-scoped ID token. When it is not specified, a default string will be used. + requestTokenExchangeAud string + + // The scopes to request from the authorization endpoint. Defaults will be used when not specified. + downstreamScopes []string + + // When we want the localhost callback to have never happened, then the flow will stop there. The login was + // unable to finish so there is nothing to assert about what should have happened with the callback, and there + // won't be any error sent to the callback either. This would happen, for example, when the user fails to log + // in at the LDAP/AD login page, because then they would be redirected back to that page again, instead of + // getting a callback success/error redirect. + wantLocalhostCallbackToNeverHappen bool + + // The expected ID token subject claim value as a regexp, for the original ID token and the refreshed ID token. + wantDownstreamIDTokenSubjectToMatch string + // The expected ID token username claim value as a regexp, for the original ID token and the refreshed ID token. + wantDownstreamIDTokenUsernameToMatch func(username string) string + // The expected ID token groups claim value, for the original ID token and the refreshed ID token. + wantDownstreamIDTokenGroups []string + + // Want the authorization endpoint to redirect to the callback with this error type. + // The rest of the flow will be skipped since the initial authorization failed. + wantErrorType string + // Want the authorization endpoint to redirect to the callback with this error description. + // Should be used with wantErrorType. + wantErrorDescription string + + // Optionally make all required assertions about the response of the RFC8693 token exchange for + // the cluster-scoped ID token, given the http response status and response body from the token endpoint. + // When this is not specified then the appropriate default assertions for a successful exchange are made. + // Even if this expects failures, the rest of the flow will continue. + wantTokenExchangeResponse func(t *testing.T, status int, body string) + + // Optionally edit the refresh session data between the initial login and the first refresh, + // which is still expected to succeed after these edits. + editRefreshSessionDataWithoutBreaking func(t *testing.T, sessionData *psession.PinnipedSession, idpName, username string) []string + // Optionally either revoke the user's session on the upstream provider, or manipulate the user's session // data in such a way that it should cause the next upstream refresh attempt to fail. breakRefreshSessionData func(t *testing.T, sessionData *psession.PinnipedSession, idpName, username string) - // Edit the refresh session data between the initial login and the refresh, which is expected to - // succeed. - editRefreshSessionDataWithoutBreaking func(t *testing.T, sessionData *psession.PinnipedSession, idpName, username string) []string }{ { name: "oidc with default username and groups claim settings", @@ -389,7 +450,7 @@ func TestSupervisorLogin_Browser(t *testing.T) { idp, _ := createLDAPIdentityProvider(t, nil) return idp.Name }, - createTestUser: func(t *testing.T) (string, string) { + testUser: func(t *testing.T) (string, string) { // return the username and password of the existing user that we want to use for this test return env.SupervisorUpstreamLDAP.TestUserMailAttributeValue, // username to present to server during login env.SupervisorUpstreamLDAP.TestUserPassword // password to present to server during login @@ -414,7 +475,7 @@ func TestSupervisorLogin_Browser(t *testing.T) { idp, _ := createLDAPIdentityProvider(t, nil) return idp.Name }, - createTestUser: func(t *testing.T) (string, string) { + testUser: func(t *testing.T) (string, string) { // return the username and password of the existing user that we want to use for this test return env.SupervisorUpstreamLDAP.TestUserMailAttributeValue, // username to present to server during login "this is the wrong password" // password to present to server during login @@ -429,7 +490,7 @@ func TestSupervisorLogin_Browser(t *testing.T) { idp, _ := createLDAPIdentityProvider(t, nil) return idp.Name }, - createTestUser: func(t *testing.T) (string, string) { + testUser: func(t *testing.T) (string, string) { // return the username and password of the existing user that we want to use for this test return "this is the wrong username", // username to present to server during login env.SupervisorUpstreamLDAP.TestUserPassword // password to present to server during login @@ -444,7 +505,7 @@ func TestSupervisorLogin_Browser(t *testing.T) { idp, _ := createLDAPIdentityProvider(t, nil) return idp.Name }, - createTestUser: func(t *testing.T) (string, string) { + testUser: func(t *testing.T) (string, string) { // return the username and password of the existing user that we want to use for this test return env.SupervisorUpstreamLDAP.TestUserMailAttributeValue, // username to present to server during login env.SupervisorUpstreamLDAP.TestUserPassword // password to present to server during login @@ -964,12 +1025,9 @@ func TestSupervisorLogin_Browser(t *testing.T) { idp, _ := createActiveDirectoryIdentityProvider(t, nil) return idp.Name }, - createTestUser: func(t *testing.T) (string, string) { + testUser: func(t *testing.T) (string, string) { return testlib.CreateFreshADTestUser(t, env) }, - deleteTestUser: func(t *testing.T, username string) { - testlib.DeleteTestADUser(t, env, username) - }, requestAuthorization: func(t *testing.T, _, downstreamAuthorizeURL, _, testUserName, testUserPassword string, httpClient *http.Client) { requestAuthorizationUsingCLIPasswordFlow(t, downstreamAuthorizeURL, @@ -997,12 +1055,9 @@ func TestSupervisorLogin_Browser(t *testing.T) { idp, _ := createActiveDirectoryIdentityProvider(t, nil) return idp.Name }, - createTestUser: func(t *testing.T) (string, string) { + testUser: func(t *testing.T) (string, string) { return testlib.CreateFreshADTestUser(t, env) }, - deleteTestUser: func(t *testing.T, username string) { - testlib.DeleteTestADUser(t, env, username) - }, requestAuthorization: func(t *testing.T, _, downstreamAuthorizeURL, _, testUserName, testUserPassword string, httpClient *http.Client) { requestAuthorizationUsingCLIPasswordFlow(t, downstreamAuthorizeURL, @@ -1030,12 +1085,9 @@ func TestSupervisorLogin_Browser(t *testing.T) { idp, _ := createActiveDirectoryIdentityProvider(t, nil) return idp.Name }, - createTestUser: func(t *testing.T) (string, string) { + testUser: func(t *testing.T) (string, string) { return testlib.CreateFreshADTestUser(t, env) }, - deleteTestUser: func(t *testing.T, username string) { - testlib.DeleteTestADUser(t, env, username) - }, requestAuthorization: func(t *testing.T, _, downstreamAuthorizeURL, _, testUserName, testUserPassword string, httpClient *http.Client) { requestAuthorizationUsingCLIPasswordFlow(t, downstreamAuthorizeURL, @@ -1226,7 +1278,62 @@ func TestSupervisorLogin_Browser(t *testing.T) { body) }, }, + { + name: "oidc upstream with downstream dynamic client happy path", + maybeSkip: skipNever, + createIDP: func(t *testing.T) string { + return testlib.CreateTestOIDCIdentityProvider(t, basicOIDCIdentityProviderSpec(), idpv1alpha1.PhaseReady).Name + }, + createOIDCClient: func(t *testing.T, callbackURL string) (string, string) { + return testlib.CreateOIDCClient(t, configv1alpha1.OIDCClientSpec{ + AllowedRedirectURIs: []configv1alpha1.RedirectURI{configv1alpha1.RedirectURI(callbackURL)}, + AllowedGrantTypes: []configv1alpha1.GrantType{"authorization_code", "urn:ietf:params:oauth:grant-type:token-exchange", "refresh_token"}, + AllowedScopes: []configv1alpha1.Scope{"openid", "offline_access", "pinniped:request-audience", "groups"}, + }, configv1alpha1.PhaseReady) + }, + requestAuthorization: requestAuthorizationUsingBrowserAuthcodeFlowOIDC, + // the ID token Subject should include the upstream user ID after the upstream issuer name + wantDownstreamIDTokenSubjectToMatch: "^" + regexp.QuoteMeta(env.SupervisorUpstreamOIDC.Issuer+"?sub=") + ".+", + // the ID token Username should include the upstream user ID after the upstream issuer name + wantDownstreamIDTokenUsernameToMatch: func(_ string) string { return "^" + regexp.QuoteMeta(env.SupervisorUpstreamOIDC.Issuer+"?sub=") + ".+" }, + }, + { + name: "ldap upstream with downstream dynamic client happy path", + maybeSkip: skipLDAPTests, + createIDP: func(t *testing.T) string { + idp, _ := createLDAPIdentityProvider(t, nil) + return idp.Name + }, + createOIDCClient: func(t *testing.T, callbackURL string) (string, string) { + return testlib.CreateOIDCClient(t, configv1alpha1.OIDCClientSpec{ + AllowedRedirectURIs: []configv1alpha1.RedirectURI{configv1alpha1.RedirectURI(callbackURL)}, + AllowedGrantTypes: []configv1alpha1.GrantType{"authorization_code", "urn:ietf:params:oauth:grant-type:token-exchange", "refresh_token"}, + AllowedScopes: []configv1alpha1.Scope{"openid", "offline_access", "pinniped:request-audience", "groups"}, + }, configv1alpha1.PhaseReady) + }, + requestAuthorization: func(t *testing.T, _, downstreamAuthorizeURL, _, _, _ string, httpClient *http.Client) { + requestAuthorizationUsingCLIPasswordFlow(t, + downstreamAuthorizeURL, + env.SupervisorUpstreamLDAP.TestUserMailAttributeValue, // username to present to server during login + env.SupervisorUpstreamLDAP.TestUserPassword, // password to present to server during login + httpClient, + false, + ) + }, + // the ID token Subject should be the Host URL plus the value pulled from the requested UserSearch.Attributes.UID attribute + wantDownstreamIDTokenSubjectToMatch: "^" + regexp.QuoteMeta( + "ldaps://"+env.SupervisorUpstreamLDAP.Host+ + "?base="+url.QueryEscape(env.SupervisorUpstreamLDAP.UserSearchBase)+ + "&sub="+base64.RawURLEncoding.EncodeToString([]byte(env.SupervisorUpstreamLDAP.TestUserUniqueIDAttributeValue)), + ) + "$", + // the ID token Username should have been pulled from the requested UserSearch.Attributes.Username attribute + wantDownstreamIDTokenUsernameToMatch: func(_ string) string { + return "^" + regexp.QuoteMeta(env.SupervisorUpstreamLDAP.TestUserMailAttributeValue) + "$" + }, + wantDownstreamIDTokenGroups: env.SupervisorUpstreamLDAP.TestUserDirectGroupsDNs, + }, } + for _, test := range tests { tt := test t.Run(tt.name, func(t *testing.T) { @@ -1237,8 +1344,8 @@ func TestSupervisorLogin_Browser(t *testing.T) { tt.requestAuthorization, tt.editRefreshSessionDataWithoutBreaking, tt.breakRefreshSessionData, - tt.createTestUser, - tt.deleteTestUser, + tt.testUser, + tt.createOIDCClient, tt.downstreamScopes, tt.requestTokenExchangeAud, tt.wantLocalhostCallbackToNeverHappen, @@ -1377,8 +1484,8 @@ func testSupervisorLogin( requestAuthorization func(t *testing.T, downstreamIssuer string, downstreamAuthorizeURL string, downstreamCallbackURL string, username string, password string, httpClient *http.Client), editRefreshSessionDataWithoutBreaking func(t *testing.T, pinnipedSession *psession.PinnipedSession, idpName, username string) []string, breakRefreshSessionData func(t *testing.T, pinnipedSession *psession.PinnipedSession, idpName, username string), - createTestUser func(t *testing.T) (string, string), - deleteTestUser func(t *testing.T, username string), + testUser func(t *testing.T) (string, string), + createOIDCClient func(t *testing.T, callbackURL string) (string, string), downstreamScopes []string, requestTokenExchangeAud string, wantLocalhostCallbackToNeverHappen bool, @@ -1475,12 +1582,20 @@ func testSupervisorLogin( // Create upstream IDP and wait for it to become ready. idpName := createIDP(t) + // Start a callback server on localhost. + localCallbackServer := startLocalCallbackServer(t) + + // Optionally create an OIDCClient. Default to using the hardcoded public client that the Supervisor supports. + clientID, clientSecret := "pinniped-cli", "" //nolint:gosec // empty credential is not a hardcoded credential + if createOIDCClient != nil { + clientID, clientSecret = createOIDCClient(t, localCallbackServer.URL) + } + + // Optionally override which user to use for the test, or choose zero values to mean use the default for + // the test's IDP. username, password := "", "" - if createTestUser != nil { - username, password = createTestUser(t) - if deleteTestUser != nil { - defer deleteTestUser(t, username) - } + if testUser != nil { + username, password = testUser(t) } // Perform OIDC discovery for our downstream. @@ -1491,23 +1606,27 @@ func testSupervisorLogin( requireEventually.NoError(err) }, 30*time.Second, 200*time.Millisecond) - // Start a callback server on localhost. - localCallbackServer := startLocalCallbackServer(t) - if downstreamScopes == nil { downstreamScopes = []string{"openid", "pinniped:request-audience", "offline_access", "groups"} } - // Form the OAuth2 configuration corresponding to our CLI client. + // Create the OAuth2 configuration. // Note that this is not using response_type=form_post, so the Supervisor will redirect to the callback endpoint // directly, without using the Javascript form_post HTML page to POST back to the callback endpoint. The e2e // tests which use the Pinniped CLI are testing the form_post part of the flow, so that is covered elsewhere. + // When ClientSecret is set here, it will be used for all token endpoint requests, but not for the authorization + // request, where it is not needed. + endpoint := discovery.Endpoint() + if clientSecret != "" { + // We only support basic auth for dynamic clients, so use basic auth in these tests. + endpoint.AuthStyle = oauth2.AuthStyleInHeader + } downstreamOAuth2Config := oauth2.Config{ - // This is the hardcoded public client that the supervisor supports. - ClientID: "pinniped-cli", - Endpoint: discovery.Endpoint(), - RedirectURL: localCallbackServer.URL, - Scopes: downstreamScopes, + ClientID: clientID, + ClientSecret: clientSecret, + Endpoint: endpoint, + RedirectURL: localCallbackServer.URL, + Scopes: downstreamScopes, } // Build a valid downstream authorize URL for the supervisor. @@ -1573,9 +1692,9 @@ func testSupervisorLogin( signatureOfLatestRefreshToken := getFositeDataSignature(t, latestRefreshToken) // First use the latest downstream refresh token to look up the corresponding session in the Supervisor's storage. - kubeClient := testlib.NewKubernetesClientset(t) - supervisorSecretsClient := kubeClient.CoreV1().Secrets(env.SupervisorNamespace) - oauthStore := oidc.NewKubeStorage(supervisorSecretsClient, oidc.DefaultOIDCTimeoutsConfiguration()) + supervisorSecretsClient := testlib.NewKubernetesClientset(t).CoreV1().Secrets(env.SupervisorNamespace) + supervisorOIDCClientsClient := testlib.NewSupervisorClientset(t).ConfigV1alpha1().OIDCClients(env.SupervisorNamespace) + oauthStore := oidc.NewKubeStorage(supervisorSecretsClient, supervisorOIDCClientsClient, oidc.DefaultOIDCTimeoutsConfiguration()) storedRefreshSession, err := oauthStore.GetRefreshTokenSession(ctx, signatureOfLatestRefreshToken, nil) require.NoError(t, err) @@ -1618,9 +1737,9 @@ func testSupervisorLogin( signatureOfLatestRefreshToken := getFositeDataSignature(t, latestRefreshToken) // First use the latest downstream refresh token to look up the corresponding session in the Supervisor's storage. - kubeClient := testlib.NewKubernetesClientset(t) - supervisorSecretsClient := kubeClient.CoreV1().Secrets(env.SupervisorNamespace) - oauthStore := oidc.NewKubeStorage(supervisorSecretsClient, oidc.DefaultOIDCTimeoutsConfiguration()) + supervisorSecretsClient := testlib.NewKubernetesClientset(t).CoreV1().Secrets(env.SupervisorNamespace) + supervisorOIDCClientsClient := testlib.NewSupervisorClientset(t).ConfigV1alpha1().OIDCClients(env.SupervisorNamespace) + oauthStore := oidc.NewKubeStorage(supervisorSecretsClient, supervisorOIDCClientsClient, oidc.DefaultOIDCTimeoutsConfiguration()) storedRefreshSession, err := oauthStore.GetRefreshTokenSession(ctx, signatureOfLatestRefreshToken, nil) require.NoError(t, err) @@ -1922,6 +2041,10 @@ func doTokenExchange( req, err := http.NewRequestWithContext(ctx, http.MethodPost, config.Endpoint.TokenURL, reqBody) require.NoError(t, err) req.Header.Set("content-type", "application/x-www-form-urlencoded") + if config.ClientSecret != "" { + // We only support basic auth for dynamic clients, so use basic auth in these tests. + req.SetBasicAuth(config.ClientID, config.ClientSecret) + } resp, err := httpClient.Do(req) require.NoError(t, err) diff --git a/test/integration/supervisor_oidc_client_test.go b/test/integration/supervisor_oidc_client_test.go index cf059fd3..4ec9fc55 100644 --- a/test/integration/supervisor_oidc_client_test.go +++ b/test/integration/supervisor_oidc_client_test.go @@ -528,16 +528,7 @@ func TestOIDCClientControllerValidations_Parallel(t *testing.T) { AllowedScopes: []supervisorconfigv1alpha1.Scope{"openid"}, }, }, - secret: &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Labels: map[string]string{"storage.pinniped.dev/type": "oidc-client-secret"}, - }, - Type: "storage.pinniped.dev/oidc-client-secret", - Data: map[string][]byte{ - "pinniped-storage-data": []byte(`{"version":"1","hashes":[]}`), - "pinniped-storage-version": []byte("1"), - }, - }, + secret: testutil.OIDCClientSecretStorageSecretWithoutName(t, env.SupervisorNamespace, []string{}), wantPhase: "Error", wantConditions: []supervisorconfigv1alpha1.Condition{ { @@ -572,16 +563,7 @@ func TestOIDCClientControllerValidations_Parallel(t *testing.T) { AllowedScopes: []supervisorconfigv1alpha1.Scope{"openid", "offline_access", "pinniped:request-audience", "username", "groups"}, }, }, - secret: &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Labels: map[string]string{"storage.pinniped.dev/type": "oidc-client-secret"}, - }, - Type: "storage.pinniped.dev/oidc-client-secret", - Data: map[string][]byte{ - "pinniped-storage-data": []byte(`{"version":"1","hashes":["$2y$15$Kh7cRj0ScSD5QelE3ZNSl.nF04JDv7zb3SgGN.tSfLIX.4kt3UX7m"]}`), - "pinniped-storage-version": []byte("1"), - }, - }, + secret: testutil.OIDCClientSecretStorageSecretWithoutName(t, env.SupervisorNamespace, []string{"$2y$15$Kh7cRj0ScSD5QelE3ZNSl.nF04JDv7zb3SgGN.tSfLIX.4kt3UX7m"}), wantPhase: "Ready", wantConditions: []supervisorconfigv1alpha1.Condition{ { diff --git a/test/integration/supervisor_warnings_test.go b/test/integration/supervisor_warnings_test.go index 3fdfffb9..e3ea9485 100644 --- a/test/integration/supervisor_warnings_test.go +++ b/test/integration/supervisor_warnings_test.go @@ -186,9 +186,10 @@ func TestSupervisorWarnings_Browser(t *testing.T) { // using the refresh token signature contained in the cache, get the refresh token session // out of kube secret storage. - kubeClient := testlib.NewKubernetesClientset(t).CoreV1() + supervisorSecretsClient := testlib.NewKubernetesClientset(t).CoreV1().Secrets(env.SupervisorNamespace) + supervisorOIDCClientsClient := testlib.NewSupervisorClientset(t).ConfigV1alpha1().OIDCClients(env.SupervisorNamespace) + oauthStore := oidc.NewKubeStorage(supervisorSecretsClient, supervisorOIDCClientsClient, oidc.DefaultOIDCTimeoutsConfiguration()) refreshTokenSignature := strings.Split(token.RefreshToken.Token, ".")[1] - oauthStore := oidc.NewKubeStorage(kubeClient.Secrets(env.SupervisorNamespace), oidc.DefaultOIDCTimeoutsConfiguration()) storedRefreshSession, err := oauthStore.GetRefreshTokenSession(ctx, refreshTokenSignature, nil) require.NoError(t, err) @@ -246,9 +247,6 @@ func TestSupervisorWarnings_Browser(t *testing.T) { testlib.SkipTestWhenActiveDirectoryIsUnavailable(t, env) expectedUsername, password := testlib.CreateFreshADTestUser(t, env) - t.Cleanup(func() { - testlib.DeleteTestADUser(t, env, expectedUsername) - }) sAMAccountName := expectedUsername + "@" + env.SupervisorUpstreamActiveDirectory.Domain setupClusterForEndToEndActiveDirectoryTest(t, sAMAccountName, env) @@ -308,9 +306,6 @@ func TestSupervisorWarnings_Browser(t *testing.T) { // create an active directory group, and add our user to it. groupName := testlib.CreateFreshADTestGroup(t, env) - t.Cleanup(func() { - testlib.DeleteTestADUser(t, env, groupName) - }) testlib.AddTestUserToGroup(t, env, groupName, expectedUsername) // remove the credential cache, which includes the cached cert, so it won't be reused and the refresh flow will be triggered. @@ -499,9 +494,10 @@ func TestSupervisorWarnings_Browser(t *testing.T) { // using the refresh token signature contained in the cache, get the refresh token session // out of kube secret storage. - kubeClient := testlib.NewKubernetesClientset(t).CoreV1() + supervisorSecretsClient := testlib.NewKubernetesClientset(t).CoreV1().Secrets(env.SupervisorNamespace) + supervisorOIDCClientsClient := testlib.NewSupervisorClientset(t).ConfigV1alpha1().OIDCClients(env.SupervisorNamespace) + oauthStore := oidc.NewKubeStorage(supervisorSecretsClient, supervisorOIDCClientsClient, oidc.DefaultOIDCTimeoutsConfiguration()) refreshTokenSignature := strings.Split(token.RefreshToken.Token, ".")[1] - oauthStore := oidc.NewKubeStorage(kubeClient.Secrets(env.SupervisorNamespace), oidc.DefaultOIDCTimeoutsConfiguration()) storedRefreshSession, err := oauthStore.GetRefreshTokenSession(ctx, refreshTokenSignature, nil) require.NoError(t, err) diff --git a/test/testlib/activedirectory.go b/test/testlib/activedirectory.go index b4440a99..25580059 100644 --- a/test/testlib/activedirectory.go +++ b/test/testlib/activedirectory.go @@ -42,6 +42,11 @@ func CreateFreshADTestUser(t *testing.T, env *TestEnv) (string, string) { err = conn.Add(a) require.NoError(t, err) + // Now that it has been created, schedule it for cleanup. + t.Cleanup(func() { + deleteTestADUser(t, env, testUserName) + }) + // modify password and enable account testUserPassword := createRandomASCIIString(t, 20) enc := unicode.UTF16(unicode.LittleEndian, unicode.IgnoreBOM).NewEncoder() @@ -83,6 +88,11 @@ func CreateFreshADTestGroup(t *testing.T, env *TestEnv) string { err = conn.Add(a) require.NoError(t, err) + // Now that it has been created, schedule it for cleanup. + t.Cleanup(func() { + deleteTestADUser(t, env, testGroupName) + }) + time.Sleep(20 * time.Second) // intrasite domain controller replication can take up to 15 seconds, so wait to ensure the change has propogated. return testGroupName } @@ -164,8 +174,8 @@ func ChangeADTestUserPassword(t *testing.T, env *TestEnv, testUserName string) { // don't bother to return the new password... we won't be using it, just checking that it's changed. } -// DeleteTestADUser deletes the test user created for this test. -func DeleteTestADUser(t *testing.T, env *TestEnv, testUserName string) { +// deleteTestADUser deletes the test user created for this test. +func deleteTestADUser(t *testing.T, env *TestEnv, testUserName string) { t.Helper() conn := dialTLS(t, env) // bind diff --git a/test/testlib/client.go b/test/testlib/client.go index b395d6fe..2c514f7d 100644 --- a/test/testlib/client.go +++ b/test/testlib/client.go @@ -16,9 +16,11 @@ import ( "time" "github.com/stretchr/testify/require" + "golang.org/x/crypto/bcrypt" authorizationv1 "k8s.io/api/authorization/v1" corev1 "k8s.io/api/core/v1" rbacv1 "k8s.io/api/rbac/v1" + apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset/typed/apiextensions/v1" k8serrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/kubernetes" @@ -26,8 +28,6 @@ import ( "k8s.io/client-go/tools/clientcmd" aggregatorclient "k8s.io/kube-aggregator/pkg/client/clientset_generated/clientset" - apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset/typed/apiextensions/v1" - auth1alpha1 "go.pinniped.dev/generated/latest/apis/concierge/authentication/v1alpha1" "go.pinniped.dev/generated/latest/apis/concierge/login/v1alpha1" configv1alpha1 "go.pinniped.dev/generated/latest/apis/supervisor/config/v1alpha1" @@ -36,6 +36,7 @@ import ( supervisorclientset "go.pinniped.dev/generated/latest/client/supervisor/clientset/versioned" "go.pinniped.dev/internal/groupsuffix" "go.pinniped.dev/internal/kubeclient" + "go.pinniped.dev/internal/oidcclientsecretstorage" // Import to initialize client auth plugins - the kubeconfig that we use for // testing may use gcloud, az, oidc, etc. @@ -378,6 +379,89 @@ func CreateClientCredsSecret(t *testing.T, clientID string, clientSecret string) ) } +func CreateOIDCClient(t *testing.T, spec configv1alpha1.OIDCClientSpec, expectedPhase configv1alpha1.OIDCClientPhase) (string, string) { + t.Helper() + env := IntegrationEnv(t) + client := NewSupervisorClientset(t) + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute) + defer cancel() + + oidcClientClient := client.ConfigV1alpha1().OIDCClients(env.SupervisorNamespace) + + // Create the OIDCClient using GenerateName to get a random name. + created, err := oidcClientClient.Create(ctx, &configv1alpha1.OIDCClient{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: "client.oauth.pinniped.dev-test-", // use the required name prefix + Labels: map[string]string{"pinniped.dev/test": ""}, + Annotations: map[string]string{"pinniped.dev/testName": t.Name()}, + }, + Spec: spec, + }, metav1.CreateOptions{}) + require.NoError(t, err) + + // Always clean this up after this point. + t.Cleanup(func() { + t.Logf("cleaning up test OIDCClient %s/%s", created.Namespace, created.Name) + err := oidcClientClient.Delete(context.Background(), created.Name, metav1.DeleteOptions{}) + require.NoError(t, err) + }) + t.Logf("created test OIDCClient %s", created.Name) + + // Create a client secret for the new OIDCClient. + clientSecret := createOIDCClientSecret(t, created) + + // Wait for the OIDCClient to enter the expected phase (or time out). + var result *configv1alpha1.OIDCClient + RequireEventuallyf(t, func(requireEventually *require.Assertions) { + var err error + result, err = oidcClientClient.Get(ctx, created.Name, metav1.GetOptions{}) + requireEventually.NoErrorf(err, "error while getting OIDCClient %s/%s", created.Namespace, created.Name) + requireEventually.Equal(expectedPhase, result.Status.Phase) + }, 60*time.Second, 1*time.Second, "expected the OIDCClient to go into phase %s, OIDCClient was: %s", expectedPhase, Sdump(result)) + + return created.Name, clientSecret +} + +func createOIDCClientSecret(t *testing.T, forOIDCClient *configv1alpha1.OIDCClient) string { + // TODO Replace this with a call to the real Supervisor API for creating client secrets after that gets implemented. + // For now, just manually create a Secret with the right format so the tests can work. + t.Helper() + env := IntegrationEnv(t) + kubeClient := NewKubernetesClientset(t) + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute) + defer cancel() + + var buf [32]byte + _, err := io.ReadFull(rand.Reader, buf[:]) + require.NoError(t, err) + randomSecret := hex.EncodeToString(buf[:]) + hashedRandomSecret, err := bcrypt.GenerateFromPassword([]byte(randomSecret), 15) + require.NoError(t, err) + + created, err := kubeClient.CoreV1().Secrets(env.SupervisorNamespace).Create(ctx, &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: oidcclientsecretstorage.New(nil, nil).GetName(forOIDCClient.UID), // use the required name + Labels: map[string]string{"storage.pinniped.dev/type": "oidc-client-secret", "pinniped.dev/test": ""}, + Annotations: map[string]string{"pinniped.dev/testName": t.Name()}, + }, + Type: "storage.pinniped.dev/oidc-client-secret", + Data: map[string][]byte{ + "pinniped-storage-data": []byte(`{"version":"1","hashes":["` + string(hashedRandomSecret) + `"]}`), + "pinniped-storage-version": []byte("1"), + }, + }, metav1.CreateOptions{}) + require.NoError(t, err) + + t.Cleanup(func() { + t.Logf("cleaning up test Secret %s/%s", created.Namespace, created.Name) + err := kubeClient.CoreV1().Secrets(env.SupervisorNamespace).Delete(context.Background(), created.Name, metav1.DeleteOptions{}) + require.NoError(t, err) + }) + + t.Logf("created test Secret %s", created.Name) + return randomSecret +} + func CreateTestOIDCIdentityProvider(t *testing.T, spec idpv1alpha1.OIDCIdentityProviderSpec, expectedPhase idpv1alpha1.OIDCIdentityProviderPhase) *idpv1alpha1.OIDCIdentityProvider { t.Helper() env := IntegrationEnv(t) @@ -385,9 +469,9 @@ func CreateTestOIDCIdentityProvider(t *testing.T, spec idpv1alpha1.OIDCIdentityP ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute) defer cancel() - // Create the OIDCIdentityProvider using GenerateName to get a random name. upstreams := client.IDPV1alpha1().OIDCIdentityProviders(env.SupervisorNamespace) + // Create the OIDCIdentityProvider using GenerateName to get a random name. created, err := upstreams.Create(ctx, &idpv1alpha1.OIDCIdentityProvider{ ObjectMeta: testObjectMeta(t, "upstream-oidc-idp"), Spec: spec, @@ -420,9 +504,9 @@ func CreateTestLDAPIdentityProvider(t *testing.T, spec idpv1alpha1.LDAPIdentityP ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute) defer cancel() - // Create the LDAPIdentityProvider using GenerateName to get a random name. upstreams := client.IDPV1alpha1().LDAPIdentityProviders(env.SupervisorNamespace) + // Create the LDAPIdentityProvider using GenerateName to get a random name. created, err := upstreams.Create(ctx, &idpv1alpha1.LDAPIdentityProvider{ ObjectMeta: testObjectMeta(t, "upstream-ldap-idp"), Spec: spec, @@ -461,9 +545,9 @@ func CreateTestActiveDirectoryIdentityProvider(t *testing.T, spec idpv1alpha1.Ac ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute) defer cancel() - // Create the ActiveDirectoryIdentityProvider using GenerateName to get a random name. upstreams := client.IDPV1alpha1().ActiveDirectoryIdentityProviders(env.SupervisorNamespace) + // Create the ActiveDirectoryIdentityProvider using GenerateName to get a random name. created, err := upstreams.Create(ctx, &idpv1alpha1.ActiveDirectoryIdentityProvider{ ObjectMeta: testObjectMeta(t, "upstream-ad-idp"), Spec: spec, @@ -501,9 +585,9 @@ func CreateTestClusterRoleBinding(t *testing.T, subject rbacv1.Subject, roleRef ctx, cancel := context.WithTimeout(context.Background(), time.Minute) defer cancel() - // Create the ClusterRoleBinding using GenerateName to get a random name. clusterRoles := client.RbacV1().ClusterRoleBindings() + // Create the ClusterRoleBinding using GenerateName to get a random name. created, err := clusterRoles.Create(ctx, &rbacv1.ClusterRoleBinding{ ObjectMeta: testObjectMeta(t, "cluster-role"), Subjects: []rbacv1.Subject{subject},