diff --git a/deploy/supervisor/deployment.yaml b/deploy/supervisor/deployment.yaml index e693dd62..293f1894 100644 --- a/deploy/supervisor/deployment.yaml +++ b/deploy/supervisor/deployment.yaml @@ -98,10 +98,26 @@ spec: readOnlyRootFilesystem: true resources: requests: - cpu: "100m" + #! If OIDCClient CRs are being used, then the Supervisor needs enough CPU to run expensive bcrypt + #! operations inside the implementation of the token endpoint for any authcode flows performed by those + #! clients, so for that use case administrators may wish to increase the requests.cpu value to more + #! closely align with their anticipated needs. Increasing this value will cause Kubernetes to give more + #! available CPU to this process during times of high CPU contention. By default, don't ask for too much + #! because that would make it impossible to install the Pinniped Supervisor on small clusters. + #! Aside from performing bcrypts at the token endpoint for those clients, the Supervisor is not a + #! particularly CPU-intensive process. + cpu: "100m" #! by default, request one-tenth of a CPU memory: "128Mi" limits: - cpu: "100m" + #! By declaring a CPU limit that is not equal to the CPU request value, the Supervisor will be classified + #! by Kubernetes to have "burstable" quality of service. + #! See https://kubernetes.io/docs/tasks/configure-pod-container/quality-service-pod/#create-a-pod-that-gets-assigned-a-qos-class-of-burstable + #! If OIDCClient CRs are being used, and lots of simultaneous users have active sessions, then it is hard + #! pre-determine what the CPU limit should be for that use case. Guessing too low would cause the + #! pod's CPU usage to be throttled, resulting in poor performance. Guessing too high would allow clients + #! to cause the usage of lots of CPU resources. Administrators who have a good sense of anticipated usage + #! patterns may choose to set the requests.cpu and limits.cpu differently from these defaults. + cpu: "1000m" #! by default, throttle each pod's usage at 1 CPU memory: "128Mi" volumeMounts: - name: config-volume diff --git a/internal/controller/supervisorconfig/oidcclientwatcher/oidc_client_watcher.go b/internal/controller/supervisorconfig/oidcclientwatcher/oidc_client_watcher.go index 69e513c6..44377fa3 100644 --- a/internal/controller/supervisorconfig/oidcclientwatcher/oidc_client_watcher.go +++ b/internal/controller/supervisorconfig/oidcclientwatcher/oidc_client_watcher.go @@ -82,7 +82,7 @@ func (c *oidcClientWatcherController) Sync(ctx controllerlib.Context) error { // We're only going to use storage to call GetName(), which happens to not need the constructor params. // This is because we can read the Secrets from the informer cache here, instead of doing live reads. - storage := oidcclientsecretstorage.New(nil, nil) + storage := oidcclientsecretstorage.New(nil) for _, oidcClient := range oidcClients { // Skip the OIDCClients that we are not trying to observe. diff --git a/internal/crud/crud.go b/internal/crud/crud.go index 12b3782f..e59b4652 100644 --- a/internal/crud/crud.go +++ b/internal/crud/crud.go @@ -14,8 +14,10 @@ import ( "time" corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" + "k8s.io/apimachinery/pkg/runtime/schema" corev1client "k8s.io/client-go/kubernetes/typed/core/v1" "go.pinniped.dev/internal/constable" @@ -40,7 +42,7 @@ const ( ) type Storage interface { - Create(ctx context.Context, signature string, data JSON, additionalLabels map[string]string) (resourceVersion string, err error) + Create(ctx context.Context, signature string, data JSON, additionalLabels map[string]string, ownerReferences []metav1.OwnerReference) (resourceVersion string, err error) Get(ctx context.Context, signature string, data JSON) (resourceVersion string, err error) Update(ctx context.Context, signature, resourceVersion string, data JSON) (newResourceVersion string, err error) Delete(ctx context.Context, signature string) error @@ -68,8 +70,8 @@ type secretsStorage struct { lifetime time.Duration } -func (s *secretsStorage) Create(ctx context.Context, signature string, data JSON, additionalLabels map[string]string) (string, error) { - secret, err := s.toSecret(signature, "", data, additionalLabels) +func (s *secretsStorage) Create(ctx context.Context, signature string, data JSON, additionalLabels map[string]string, ownerReferences []metav1.OwnerReference) (string, error) { + secret, err := s.toSecret(signature, "", data, additionalLabels, ownerReferences) if err != nil { return "", err } @@ -94,14 +96,26 @@ func (s *secretsStorage) Get(ctx context.Context, signature string, data JSON) ( } func (s *secretsStorage) Update(ctx context.Context, signature, resourceVersion string, data JSON) (string, error) { - // Note: There may be a small bug here in that toSecret will move the SecretLifetimeAnnotationKey date forward - // instead of keeping the storage resource's original SecretLifetimeAnnotationKey value. However, we only use - // this Update method in one place, and it doesn't matter in that place. Be aware that it might need improvement - // if we start using this Update method in more places. - secret, err := s.toSecret(signature, resourceVersion, data, nil) + secret, err := s.toSecret(signature, resourceVersion, data, nil, nil) if err != nil { return "", err } + + oldSecret, err := s.secrets.Get(ctx, secret.Name, metav1.GetOptions{}) + if err != nil { + return "", fmt.Errorf("failed to get %s for signature %s: %w", s.resource, signature, err) + } + // do not assume that our secret client does live reads + if oldSecret.ResourceVersion != resourceVersion { + return "", errors.NewConflict(schema.GroupResource{Resource: "Secret"}, secret.Name, + fmt.Errorf("resource version %s does not match expected value: %s", oldSecret.ResourceVersion, resourceVersion)) + } + + // preserve these fields - they are effectively immutable on update + secret.Labels = oldSecret.Labels + secret.Annotations = oldSecret.Annotations + secret.OwnerReferences = oldSecret.OwnerReferences + secret, err = s.secrets.Update(ctx, secret, metav1.UpdateOptions{}) if err != nil { return "", fmt.Errorf("failed to update %s for signature %s at resource version %s: %w", s.resource, signature, resourceVersion, err) @@ -180,18 +194,17 @@ func (s *secretsStorage) GetName(signature string) string { return fmt.Sprintf(secretNameFormat, s.resource, signatureAsValidName) } -func (s *secretsStorage) toSecret(signature, resourceVersion string, data JSON, additionalLabels map[string]string) (*corev1.Secret, error) { +func (s *secretsStorage) toSecret(signature, resourceVersion string, data JSON, additionalLabels map[string]string, ownerReferences []metav1.OwnerReference) (*corev1.Secret, error) { buf, err := json.Marshal(data) if err != nil { return nil, fmt.Errorf("failed to encode secret data for %s: %w", s.GetName(signature), err) } - labelsToAdd := map[string]string{ - SecretLabelKey: s.resource, // make it easier to find this stuff via kubectl - } + labelsToAdd := make(map[string]string, len(additionalLabels)+1) for labelName, labelValue := range additionalLabels { labelsToAdd[labelName] = labelValue } + labelsToAdd[SecretLabelKey] = s.resource // make it easier to find this stuff via kubectl var annotations map[string]string if s.lifetime > 0 { @@ -206,7 +219,7 @@ func (s *secretsStorage) toSecret(signature, resourceVersion string, data JSON, ResourceVersion: resourceVersion, Labels: labelsToAdd, Annotations: annotations, - OwnerReferences: nil, + OwnerReferences: ownerReferences, }, Data: map[string][]byte{ secretDataKey: buf, diff --git a/internal/crud/crud_test.go b/internal/crud/crud_test.go index 25ffdfad..33817c44 100644 --- a/internal/crud/crud_test.go +++ b/internal/crud/crud_test.go @@ -120,7 +120,7 @@ func TestStorage(t *testing.T) { 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) + rv1, err := storage.Create(ctx, signature, data, nil, nil) require.Empty(t, rv1) // fake client does not set this require.NoError(t, err) @@ -180,14 +180,14 @@ func TestStorage(t *testing.T) { mocks: nil, run: func(t *testing.T, storage Storage, fakeClock *clocktesting.FakeClock) error { data := &testJSON{Data: "create1"} - rv1, err := storage.Create(ctx, "sig1", data, nil) + rv1, err := storage.Create(ctx, "sig1", data, nil, nil) require.Empty(t, rv1) // fake client does not set this require.NoError(t, err) fakeClock.Step(42 * time.Minute) // simulate that a known amount of time has passed data = &testJSON{Data: "create2"} - rv1, err = storage.Create(ctx, "sig2", data, nil) + rv1, err = storage.Create(ctx, "sig2", data, nil, nil) require.Empty(t, rv1) // fake client does not set this require.NoError(t, err) @@ -279,7 +279,7 @@ func TestStorage(t *testing.T) { 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, map[string]string{"label1": "value1", "label2": "value2"}) + rv1, err := storage.Create(ctx, signature, data, map[string]string{"label1": "value1", "label2": "value2"}, nil) require.Empty(t, rv1) // fake client does not set this require.NoError(t, err) @@ -456,6 +456,7 @@ func TestStorage(t *testing.T) { return nil }, wantActions: []coretesting.Action{ + coretesting.NewGetAction(secretsGVR, namespace, "pinniped-storage-stores-4wssc5gzt5mlln6iux6gl7hzz3klsirisydaxn7indnpvdnrs5ba"), coretesting.NewGetAction(secretsGVR, namespace, "pinniped-storage-stores-4wssc5gzt5mlln6iux6gl7hzz3klsirisydaxn7indnpvdnrs5ba"), coretesting.NewUpdateAction(secretsGVR, namespace, &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ @@ -1026,7 +1027,7 @@ func TestStorage(t *testing.T) { 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) + rv1, err := storage.Create(ctx, signature, data, nil, nil) require.Empty(t, rv1) // fake client does not set this require.NoError(t, err) diff --git a/internal/fositestorage/accesstoken/accesstoken.go b/internal/fositestorage/accesstoken/accesstoken.go index 606b75d2..d897da4c 100644 --- a/internal/fositestorage/accesstoken/accesstoken.go +++ b/internal/fositestorage/accesstoken/accesstoken.go @@ -85,6 +85,7 @@ func (a *accessTokenStorage) CreateAccessTokenSession(ctx context.Context, signa signature, &Session{Request: request, Version: accessTokenStorageVersion}, map[string]string{fositestorage.StorageRequestIDLabelName: requester.GetID()}, + nil, ) return err } diff --git a/internal/fositestorage/authorizationcode/authorizationcode.go b/internal/fositestorage/authorizationcode/authorizationcode.go index 562980f4..0185baf1 100644 --- a/internal/fositestorage/authorizationcode/authorizationcode.go +++ b/internal/fositestorage/authorizationcode/authorizationcode.go @@ -89,7 +89,7 @@ func (a *authorizeCodeStorage) CreateAuthorizeCodeSession(ctx context.Context, s // of the consent authorization request. It is used to identify the session. // signature for lookup in the DB - _, err = a.storage.Create(ctx, signature, &Session{Active: true, Request: request, Version: authorizeCodeStorageVersion}, nil) + _, err = a.storage.Create(ctx, signature, &Session{Active: true, Request: request, Version: authorizeCodeStorageVersion}, nil, nil) return err } diff --git a/internal/fositestorage/authorizationcode/authorizationcode_test.go b/internal/fositestorage/authorizationcode/authorizationcode_test.go index 9e2fbe4a..68d4c01e 100644 --- a/internal/fositestorage/authorizationcode/authorizationcode_test.go +++ b/internal/fositestorage/authorizationcode/authorizationcode_test.go @@ -72,6 +72,7 @@ func TestAuthorizationCodeStorage(t *testing.T) { }), kubetesting.NewGetAction(secretsGVR, namespace, "pinniped-storage-authcode-pwu5zs7lekbhnln2w4"), kubetesting.NewGetAction(secretsGVR, namespace, "pinniped-storage-authcode-pwu5zs7lekbhnln2w4"), + kubetesting.NewGetAction(secretsGVR, namespace, "pinniped-storage-authcode-pwu5zs7lekbhnln2w4"), kubetesting.NewUpdateAction(secretsGVR, namespace, &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ Name: "pinniped-storage-authcode-pwu5zs7lekbhnln2w4", @@ -134,7 +135,7 @@ func TestAuthorizationCodeStorage(t *testing.T) { require.NoError(t, err) testutil.LogActualJSONFromCreateAction(t, client, 0) // makes it easier to update expected values when needed - testutil.LogActualJSONFromUpdateAction(t, client, 3) // makes it easier to update expected values when needed + testutil.LogActualJSONFromUpdateAction(t, client, 4) // makes it easier to update expected values when needed require.Equal(t, wantActions, client.Actions()) // Doing a Get on an invalidated session should still return the session, but also return an error. diff --git a/internal/fositestorage/openidconnect/openidconnect.go b/internal/fositestorage/openidconnect/openidconnect.go index 605ac523..6be3c862 100644 --- a/internal/fositestorage/openidconnect/openidconnect.go +++ b/internal/fositestorage/openidconnect/openidconnect.go @@ -60,7 +60,7 @@ func (a *openIDConnectRequestStorage) CreateOpenIDConnectSession(ctx context.Con return err } - _, err = a.storage.Create(ctx, signature, &session{Request: request, Version: oidcStorageVersion}, nil) + _, err = a.storage.Create(ctx, signature, &session{Request: request, Version: oidcStorageVersion}, nil, nil) return err } diff --git a/internal/fositestorage/pkce/pkce.go b/internal/fositestorage/pkce/pkce.go index f84b01da..e150cae1 100644 --- a/internal/fositestorage/pkce/pkce.go +++ b/internal/fositestorage/pkce/pkce.go @@ -53,7 +53,7 @@ func (a *pkceStorage) CreatePKCERequestSession(ctx context.Context, signature st return err } - _, err = a.storage.Create(ctx, signature, &session{Request: request, Version: pkceStorageVersion}, nil) + _, err = a.storage.Create(ctx, signature, &session{Request: request, Version: pkceStorageVersion}, nil, nil) return err } diff --git a/internal/fositestorage/refreshtoken/refreshtoken.go b/internal/fositestorage/refreshtoken/refreshtoken.go index 7f1147fb..d36d23c1 100644 --- a/internal/fositestorage/refreshtoken/refreshtoken.go +++ b/internal/fositestorage/refreshtoken/refreshtoken.go @@ -91,6 +91,7 @@ func (a *refreshTokenStorage) CreateRefreshTokenSession(ctx context.Context, sig signature, &Session{Request: request, Version: refreshTokenStorageVersion}, map[string]string{fositestorage.StorageRequestIDLabelName: requester.GetID()}, + nil, ) return err } diff --git a/internal/oidc/clientregistry/clientregistry_test.go b/internal/oidc/clientregistry/clientregistry_test.go index 5367d511..03651421 100644 --- a/internal/oidc/clientregistry/clientregistry_test.go +++ b/internal/oidc/clientregistry/clientregistry_test.go @@ -237,7 +237,7 @@ func TestClientManager(t *testing.T) { oidcClientsClient := supervisorClient.ConfigV1alpha1().OIDCClients(testNamespace) subject := NewClientManager( oidcClientsClient, - oidcclientsecretstorage.New(secrets, time.Now), + oidcclientsecretstorage.New(secrets), oidcclientvalidator.DefaultMinBcryptCost, ) diff --git a/internal/oidc/kube_storage.go b/internal/oidc/kube_storage.go index 529a5f6e..a197335e 100644 --- a/internal/oidc/kube_storage.go +++ b/internal/oidc/kube_storage.go @@ -43,7 +43,7 @@ func NewKubeStorage( ) *KubeStorage { nowFunc := time.Now return &KubeStorage{ - clientManager: clientregistry.NewClientManager(oidcClientsClient, oidcclientsecretstorage.New(secrets, nowFunc), minBcryptCost), + clientManager: clientregistry.NewClientManager(oidcClientsClient, oidcclientsecretstorage.New(secrets), minBcryptCost), authorizationCodeStorage: authorizationcode.New(secrets, nowFunc, timeoutsConfiguration.AuthorizationCodeSessionStorageLifetime), pkceStorage: pkce.New(secrets, nowFunc, timeoutsConfiguration.PKCESessionStorageLifetime), oidcStorage: openidconnect.New(secrets, nowFunc, timeoutsConfiguration.OIDCSessionStorageLifetime), diff --git a/internal/oidc/nullstorage.go b/internal/oidc/nullstorage.go index c6025a0c..61476f81 100644 --- a/internal/oidc/nullstorage.go +++ b/internal/oidc/nullstorage.go @@ -5,7 +5,6 @@ package oidc import ( "context" - "time" "github.com/ory/fosite" corev1client "k8s.io/client-go/kubernetes/typed/core/v1" @@ -32,7 +31,7 @@ func NewNullStorage( minBcryptCost int, ) *NullStorage { return &NullStorage{ - ClientManager: clientregistry.NewClientManager(oidcClientsClient, oidcclientsecretstorage.New(secrets, time.Now), minBcryptCost), + ClientManager: clientregistry.NewClientManager(oidcClientsClient, oidcclientsecretstorage.New(secrets), minBcryptCost), } } diff --git a/internal/oidc/oidcclientvalidator/oidcclientvalidator.go b/internal/oidc/oidcclientvalidator/oidcclientvalidator.go index fd09894c..ab16fef3 100644 --- a/internal/oidc/oidcclientvalidator/oidcclientvalidator.go +++ b/internal/oidc/oidcclientvalidator/oidcclientvalidator.go @@ -145,7 +145,7 @@ func validateSecret(secret *v1.Secret, conditions []*v1alpha1.Condition, minBcry return conditions, emptyList } - storedClientSecret, err := oidcclientsecretstorage.ReadFromSecret(secret) + storedClientSecrets, err := oidcclientsecretstorage.ReadFromSecret(secret) if err != nil { // Invalid: storage Secret exists but its data could not be parsed. conditions = append(conditions, &v1alpha1.Condition{ @@ -158,7 +158,7 @@ func validateSecret(secret *v1.Secret, conditions []*v1alpha1.Condition, minBcry } // Successfully read the stored client secrets, so check if there are any stored in the list. - storedClientSecretsCount := len(storedClientSecret.SecretHashes) + storedClientSecretsCount := len(storedClientSecrets) if storedClientSecretsCount == 0 { // Invalid: no client secrets stored. conditions = append(conditions, &v1alpha1.Condition{ @@ -172,7 +172,7 @@ func validateSecret(secret *v1.Secret, conditions []*v1alpha1.Condition, minBcry // Check each hashed password's format and bcrypt cost. bcryptErrs := make([]string, 0, storedClientSecretsCount) - for i, p := range storedClientSecret.SecretHashes { + for i, p := range storedClientSecrets { cost, err := bcrypt.Cost([]byte(p)) if err != nil { bcryptErrs = append(bcryptErrs, fmt.Sprintf( @@ -203,7 +203,7 @@ func validateSecret(secret *v1.Secret, conditions []*v1alpha1.Condition, minBcry Reason: reasonSuccess, Message: fmt.Sprintf("%d client secret(s) found", storedClientSecretsCount), }) - return conditions, storedClientSecret.SecretHashes + return conditions, storedClientSecrets } func allowedGrantTypesContains(haystack *v1alpha1.OIDCClient, needle string) bool { diff --git a/internal/oidc/provider/manager/manager_test.go b/internal/oidc/provider/manager/manager_test.go index 46039731..9f407b25 100644 --- a/internal/oidc/provider/manager/manager_test.go +++ b/internal/oidc/provider/manager/manager_test.go @@ -217,7 +217,7 @@ func TestManager(t *testing.T) { oidctestutil.VerifyECDSAIDToken(t, jwkIssuer, downstreamClientID, privateKey, idToken) // Make sure that we wired up the callback endpoint to use kube storage for fosite sessions. - r.Equal(len(kubeClient.Actions()), numberOfKubeActionsBeforeThisRequest+8, + r.Equal(len(kubeClient.Actions()), numberOfKubeActionsBeforeThisRequest+9, "did not perform any kube actions during the callback request, but should have") } diff --git a/internal/oidcclientsecretstorage/oidcclientsecretstorage.go b/internal/oidcclientsecretstorage/oidcclientsecretstorage.go index 7bec307e..b8eeaf78 100644 --- a/internal/oidcclientsecretstorage/oidcclientsecretstorage.go +++ b/internal/oidcclientsecretstorage/oidcclientsecretstorage.go @@ -7,7 +7,6 @@ import ( "context" "encoding/base64" "fmt" - "time" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" @@ -15,6 +14,7 @@ import ( "k8s.io/apimachinery/pkg/types" corev1client "k8s.io/client-go/kubernetes/typed/core/v1" + configv1alpha1 "go.pinniped.dev/generated/latest/apis/supervisor/config/v1alpha1" "go.pinniped.dev/internal/constable" "go.pinniped.dev/internal/crud" ) @@ -32,9 +32,9 @@ type OIDCClientSecretStorage struct { secrets corev1client.SecretInterface } -// StoredClientSecret defines the format of the content of a client's secrets when stored in a Secret +// storedClientSecret defines the format of the content of a client's secrets when stored in a Secret // as a JSON string value. -type StoredClientSecret struct { +type storedClientSecret struct { // List of bcrypt hashes. SecretHashes []string `json:"hashes"` // The format version. Take care when updating. We cannot simply bump the storage version and drop/ignore old data. @@ -42,14 +42,55 @@ type StoredClientSecret struct { Version string `json:"version"` } -func New(secrets corev1client.SecretInterface, clock func() time.Time) *OIDCClientSecretStorage { +func New(secrets corev1client.SecretInterface) *OIDCClientSecretStorage { return &OIDCClientSecretStorage{ - storage: crud.New(TypeLabelValue, secrets, clock, 0), + storage: crud.New(TypeLabelValue, secrets, nil, 0), // can use nil clock because we are using infinite lifetime secrets: secrets, } } -// TODO expose other methods as needed for get, create, update, etc. +func (s *OIDCClientSecretStorage) Get(ctx context.Context, oidcClientUID types.UID) (string, []string, error) { + secret := &storedClientSecret{} + rv, err := s.storage.Get(ctx, uidToName(oidcClientUID), secret) + if errors.IsNotFound(err) { + return "", nil, nil + } + if err != nil { + return "", nil, fmt.Errorf("failed to get client secret for uid %s: %w", oidcClientUID, err) + } + + return rv, secret.SecretHashes, nil +} + +func (s *OIDCClientSecretStorage) Set(ctx context.Context, resourceVersion, oidcClientName string, oidcClientUID types.UID, secretHashes []string) error { + secret := &storedClientSecret{ + SecretHashes: secretHashes, + Version: oidcClientSecretStorageVersion, + } + name := uidToName(oidcClientUID) + + if mustBeCreate := len(resourceVersion) == 0; mustBeCreate { + ownerReferences := []metav1.OwnerReference{ + { + APIVersion: configv1alpha1.SchemeGroupVersion.String(), + Kind: "OIDCClient", + Name: oidcClientName, + UID: oidcClientUID, + Controller: nil, // TODO should this be true? + BlockOwnerDeletion: nil, + }, + } + if _, err := s.storage.Create(ctx, name, secret, nil, ownerReferences); err != nil { + return fmt.Errorf("failed to create client secret for uid %s: %w", oidcClientUID, err) + } + return nil + } + + if _, err := s.storage.Update(ctx, name, resourceVersion, secret); err != nil { + return fmt.Errorf("failed to update client secret for uid %s: %w", oidcClientUID, err) + } + return nil +} // 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. @@ -66,21 +107,24 @@ func (s *OIDCClientSecretStorage) GetStorageSecret(ctx context.Context, oidcClie // 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. - b64encodedUID := base64.RawURLEncoding.EncodeToString([]byte(oidcClientUID)) - return s.storage.GetName(b64encodedUID) + return s.storage.GetName(uidToName(oidcClientUID)) } -// ReadFromSecret reads the contents of a Secret as a StoredClientSecret. -func ReadFromSecret(secret *corev1.Secret) (*StoredClientSecret, error) { - storedClientSecret := &StoredClientSecret{} - err := crud.FromSecret(TypeLabelValue, secret, storedClientSecret) +func uidToName(oidcClientUID types.UID) string { + // Avoid having s.storage.GetName() base64 decode something that wasn't ever encoded by encoding it here. + return base64.RawURLEncoding.EncodeToString([]byte(oidcClientUID)) +} + +// ReadFromSecret reads the contents of a Secret as a storedClientSecret and returns the associated hashes. +func ReadFromSecret(secret *corev1.Secret) ([]string, error) { + clientSecret := &storedClientSecret{} + err := crud.FromSecret(TypeLabelValue, secret, clientSecret) if err != nil { return nil, err } - if storedClientSecret.Version != oidcClientSecretStorageVersion { + if clientSecret.Version != oidcClientSecretStorageVersion { return nil, fmt.Errorf("%w: OIDC client secret storage has version %s instead of %s", - ErrOIDCClientSecretStorageVersion, storedClientSecret.Version, oidcClientSecretStorageVersion) + ErrOIDCClientSecretStorageVersion, clientSecret.Version, oidcClientSecretStorageVersion) } - return storedClientSecret, nil + return clientSecret.SecretHashes, nil } diff --git a/internal/oidcclientsecretstorage/oidcclientsecretstorage_test.go b/internal/oidcclientsecretstorage/oidcclientsecretstorage_test.go index 09ff908c..3092e53e 100644 --- a/internal/oidcclientsecretstorage/oidcclientsecretstorage_test.go +++ b/internal/oidcclientsecretstorage/oidcclientsecretstorage_test.go @@ -15,7 +15,7 @@ import ( func TestGetName(t *testing.T) { // Note that GetName() should not depend on the constructor params, to make it easier to use in various contexts. - subject := New(nil, nil) + subject := New(nil) require.Equal(t, "pinniped-storage-oidc-client-secret-onxw2zjnmv4gc3lqnrss25ljmqyq", @@ -30,7 +30,7 @@ func TestReadFromSecret(t *testing.T) { tests := []struct { name string secret *corev1.Secret - wantStored *StoredClientSecret + wantHashes []string wantErr string }{ { @@ -49,10 +49,7 @@ func TestReadFromSecret(t *testing.T) { }, Type: "storage.pinniped.dev/oidc-client-secret", }, - wantStored: &StoredClientSecret{ - Version: "1", - SecretHashes: []string{"first-hash", "second-hash"}, - }, + wantHashes: []string{"first-hash", "second-hash"}, }, { name: "wrong secret type", @@ -113,20 +110,14 @@ func TestReadFromSecret(t *testing.T) { secret: testutil.OIDCClientSecretStorageSecretForUID(t, "some-namespace", "some-uid", []string{"first-hash", "second-hash"}, ), - wantStored: &StoredClientSecret{ - Version: "1", - SecretHashes: []string{"first-hash", "second-hash"}, - }, + wantHashes: []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"}, - }, + wantHashes: []string{"first-hash", "second-hash"}, }, { name: "OIDCClientSecretStorageSecretForUIDWithWrongVersion() test helper generates readable format, to ensure that test helpers are kept up to date", @@ -139,13 +130,13 @@ func TestReadFromSecret(t *testing.T) { tt := tt t.Run(tt.name, func(t *testing.T) { t.Parallel() - session, err := ReadFromSecret(tt.secret) + hashes, err := ReadFromSecret(tt.secret) if tt.wantErr == "" { require.NoError(t, err) - require.Equal(t, tt.wantStored, session) + require.Equal(t, tt.wantHashes, hashes) } else { require.EqualError(t, err, tt.wantErr) - require.Nil(t, session) + require.Nil(t, hashes) } }) } diff --git a/internal/registry/clientsecretrequest/rest.go b/internal/registry/clientsecretrequest/rest.go index 03b245e1..b049aaa7 100644 --- a/internal/registry/clientsecretrequest/rest.go +++ b/internal/registry/clientsecretrequest/rest.go @@ -6,27 +6,76 @@ package clientsecretrequest import ( "context" + "crypto/rand" + "encoding/hex" "fmt" + "io" + "strings" + "golang.org/x/crypto/bcrypt" + apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + "k8s.io/apiextensions-apiserver/pkg/registry/customresource/tableconvertor" apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/api/meta" + genericvalidation "k8s.io/apimachinery/pkg/api/validation" + "k8s.io/apimachinery/pkg/api/validation/path" metainternalversion "k8s.io/apimachinery/pkg/apis/meta/internalversion" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/util/validation/field" + genericapirequest "k8s.io/apiserver/pkg/endpoints/request" "k8s.io/apiserver/pkg/registry/rest" + corev1client "k8s.io/client-go/kubernetes/typed/core/v1" "k8s.io/utils/trace" clientsecretapi "go.pinniped.dev/generated/latest/apis/supervisor/clientsecret" + configv1alpha1clientset "go.pinniped.dev/generated/latest/client/supervisor/clientset/versioned/typed/config/v1alpha1" + "go.pinniped.dev/internal/oidcclientsecretstorage" ) -func NewREST(resource schema.GroupResource) *REST { +// cost is a good bcrypt cost for 2022, should take about 250 ms to validate. +// This value is expected to be increased over time to match CPU improvements. +const cost = 12 + +// nolint: gochecknoglobals +var tableConvertor = func() rest.TableConvertor { + // sadly this is not useful at the moment because `kubectl create` does not support table output + columns := []apiextensionsv1.CustomResourceColumnDefinition{ + { + Name: "Secret", + Type: "string", + Description: "", // TODO generate SwaggerDoc() method to fill this field + JSONPath: ".status.generatedSecret", + }, + { + Name: "Total", + Type: "integer", + Description: "", // TODO generate SwaggerDoc() method to fill this field + JSONPath: ".status.totalClientSecrets", + }, + } + tc, err := tableconvertor.New(columns) // just re-use the CRD table code so we do not have to implement the interface ourselves + if err != nil { + panic(err) // inputs are static so this should never happen + } + return tc +}() + +func NewREST(secrets corev1client.SecretInterface, clients configv1alpha1clientset.OIDCClientInterface, namespace string) *REST { return &REST{ - tableConvertor: rest.NewDefaultTableConvertor(resource), + secretStorage: oidcclientsecretstorage.New(secrets), + clients: clients, + namespace: namespace, + rand: rand.Reader, } } type REST struct { - tableConvertor rest.TableConvertor + secretStorage *oidcclientsecretstorage.OIDCClientSecretStorage + clients configv1alpha1clientset.OIDCClientInterface + namespace string + rand io.Reader } // Assert that our *REST implements all the optional interfaces that we expect it to implement. @@ -50,6 +99,8 @@ func (*REST) NewList() runtime.Object { return &clientsecretapi.OIDCClientSecretRequestList{} } +// List implements the list verb. Support the list verb to support `kubectl get pinniped`, to make sure all resources +// are in the pinniped category, and avoid kubectl errors when kubectl lists. func (*REST) List(_ context.Context, _ *metainternalversion.ListOptions) (runtime.Object, error) { return &clientsecretapi.OIDCClientSecretRequestList{ ListMeta: metav1.ListMeta{ @@ -60,7 +111,7 @@ func (*REST) List(_ context.Context, _ *metainternalversion.ListOptions) (runtim } func (r *REST) ConvertToTable(ctx context.Context, obj runtime.Object, tableOptions runtime.Object) (*metav1.Table, error) { - return r.tableConvertor.ConvertToTable(ctx, obj, tableOptions) + return tableConvertor.ConvertToTable(ctx, obj, tableOptions) } func (*REST) NamespaceScoped() bool { @@ -72,32 +123,162 @@ func (*REST) Categories() []string { } func (r *REST) Create(ctx context.Context, obj runtime.Object, createValidation rest.ValidateObjectFunc, options *metav1.CreateOptions) (runtime.Object, error) { - t := trace.FromContext(ctx).Nest("create", trace.Field{ - Key: "kind", - Value: "OIDCClientSecretRequest", - }) + t := trace.FromContext(ctx).Nest("create", + trace.Field{Key: "kind", Value: "OIDCClientSecretRequest"}, + trace.Field{Key: "metadata.name", Value: name(obj)}, + ) defer t.Log() - _, err := validateRequest(obj, t) + // Validate the create request before honoring it. + req, err := r.validateRequest(ctx, obj, createValidation, options, t) if err != nil { return nil, err } + t.Step("validateRequest") + // Find the specified OIDCClient. + oidcClient, err := r.clients.Get(ctx, req.Name, metav1.GetOptions{}) + if err != nil { + if apierrors.IsNotFound(err) { + traceValidationFailure(t, fmt.Sprintf("client %q does not exist", req.Name)) + errs := field.ErrorList{field.NotFound(field.NewPath("metadata", "name"), req.Name)} + return nil, apierrors.NewInvalid(kindFromContext(ctx), req.Name, errs) + } + traceFailureWithError(t, "clients.Get", err) + return nil, apierrors.NewInternalError(fmt.Errorf("getting client %q failed", req.Name)) + } + t.Step("clients.Get") + + // Using the OIDCClient's UID, check to see if the storage Secret for its client secrets already exists. + // Note that when it does not exist, this Get() function will not return an error, and will return nil rv and hashes. + rv, hashes, err := r.secretStorage.Get(ctx, oidcClient.UID) + if err != nil { + traceFailureWithError(t, "secretStorage.Get", err) + return nil, apierrors.NewInternalError(fmt.Errorf("getting secret for client %q failed", req.Name)) + } + t.Step("secretStorage.Get") + + // If requested, generate a new client secret and add it to the list. + var secret string + if req.Spec.GenerateNewSecret { + secret, err = generateSecret(r.rand) + if err != nil { + traceFailureWithError(t, "generateSecret", err) + return nil, apierrors.NewInternalError(fmt.Errorf("client secret generation failed")) + } + t.Step("generateSecret") + + hash, err := bcrypt.GenerateFromPassword([]byte(secret), cost) + if err != nil { + traceFailureWithError(t, "bcrypt.GenerateFromPassword", err) + return nil, apierrors.NewInternalError(fmt.Errorf("hash generation failed")) + } + t.Step("bcrypt.GenerateFromPassword") + + hashes = append([]string{string(hash)}, hashes...) + } + + // If requested, remove all client secrets except for the most recent one. + needsRevoke := req.Spec.RevokeOldSecrets && len(hashes) > 0 + if needsRevoke { + hashes = []string{hashes[0]} + } + + // If anything was requested to change... + if req.Spec.GenerateNewSecret || needsRevoke { + // Each bcrypt comparison is expensive, and we do not want a large list to cause wasted CPU. + if len(hashes) > 5 { + return nil, apierrors.NewRequestEntityTooLargeError( + fmt.Sprintf("OIDCClient %s has too many secrets, spec.revokeOldSecrets must be true", oidcClient.Name)) + } + + // Create or update the storage Secret for client secrets. + if err := r.secretStorage.Set(ctx, rv, oidcClient.Name, oidcClient.UID, hashes); err != nil { + if apierrors.IsAlreadyExists(err) || apierrors.IsConflict(err) { + return nil, apierrors.NewConflict(qualifiedResourceFromContext(ctx), req.Name, + fmt.Errorf("multiple concurrent secret generation requests for same client")) + } + + traceFailureWithError(t, "secretStorage.Set", err) + return nil, apierrors.NewInternalError(fmt.Errorf("setting client secret failed")) + } + t.Step("secretStorage.Set") + } + + // Return the new secret in plaintext, if one was generated, along with the total number of secrets. return &clientsecretapi.OIDCClientSecretRequest{ Status: clientsecretapi.OIDCClientSecretRequestStatus{ - GeneratedSecret: "not-a-real-secret", - TotalClientSecrets: 20, + GeneratedSecret: secret, + TotalClientSecrets: len(hashes), }, }, nil } -func validateRequest(obj runtime.Object, t *trace.Trace) (*clientsecretapi.OIDCClientSecretRequest, error) { +func (r *REST) validateRequest( + ctx context.Context, + obj runtime.Object, + createValidation rest.ValidateObjectFunc, + options *metav1.CreateOptions, + t *trace.Trace, +) (*clientsecretapi.OIDCClientSecretRequest, error) { clientSecretRequest, ok := obj.(*clientsecretapi.OIDCClientSecretRequest) if !ok { traceValidationFailure(t, "not an OIDCClientSecretRequest") return nil, apierrors.NewBadRequest(fmt.Sprintf("not an OIDCClientSecretRequest: %#v", obj)) } + // Ensure namespace on the object is correct, or error if a conflicting namespace was set in the object. + requestNamespace, ok := genericapirequest.NamespaceFrom(ctx) + if !ok { + return nil, apierrors.NewInternalError(fmt.Errorf("no namespace information found in request context")) + } + if err := rest.EnsureObjectNamespaceMatchesRequestNamespace(requestNamespace, clientSecretRequest); err != nil { + return nil, err + } + // Making client secrets outside the supervisor's namespace does not make sense. + if requestNamespace != r.namespace { + msg := fmt.Sprintf("namespace must be %s on OIDCClientSecretRequest, was %s", r.namespace, requestNamespace) + traceValidationFailure(t, msg) + return nil, apierrors.NewBadRequest(msg) + } + + if errs := genericvalidation.ValidateObjectMetaAccessor( + clientSecretRequest, + true, + func(name string, prefix bool) []string { + if prefix { + return []string{"generateName is not supported"} + } + var errs []string + if name == "client.oauth.pinniped.dev-" { + errs = append(errs, `must not equal 'client.oauth.pinniped.dev-'`) + } + if !strings.HasPrefix(name, "client.oauth.pinniped.dev-") { + errs = append(errs, `must start with 'client.oauth.pinniped.dev-'`) + } + return append(errs, path.IsValidPathSegmentName(name)...) + }, + field.NewPath("metadata"), + ); len(errs) > 0 { + return nil, apierrors.NewInvalid(kindFromContext(ctx), clientSecretRequest.Name, errs) + } + + // just a sanity check, not sure how to honor a dry run on a virtual API + if options != nil { + if len(options.DryRun) != 0 { + traceValidationFailure(t, "dryRun not supported") + errs := field.ErrorList{field.NotSupported(field.NewPath("dryRun"), options.DryRun, nil)} + return nil, apierrors.NewInvalid(kindFromContext(ctx), clientSecretRequest.Name, errs) + } + } + + if createValidation != nil { + if err := createValidation(ctx, obj.DeepCopyObject()); err != nil { + traceFailureWithError(t, "validation webhook", err) + return nil, err + } + } + return clientSecretRequest, nil } @@ -107,3 +288,42 @@ func traceValidationFailure(t *trace.Trace, msg string) { trace.Field{Key: "msg", Value: msg}, ) } + +func traceFailureWithError(t *trace.Trace, failureType string, err error) { + t.Step("failure", + trace.Field{Key: "failureType", Value: failureType}, + trace.Field{Key: "msg", Value: err.Error()}, + ) +} + +func generateSecret(rand io.Reader) (string, error) { + var buf [32]byte + if _, err := io.ReadFull(rand, buf[:]); err != nil { + return "", fmt.Errorf("could not generate client secret: %w", err) + } + return hex.EncodeToString(buf[:]), nil +} + +func name(obj runtime.Object) string { + accessor, err := meta.Accessor(obj) + if err != nil { + return "" + } + return accessor.GetName() +} + +func qualifiedResourceFromContext(ctx context.Context) schema.GroupResource { + if info, ok := genericapirequest.RequestInfoFrom(ctx); ok { + return schema.GroupResource{Group: info.APIGroup, Resource: info.Resource} + } + // this should never happen in practice + return clientsecretapi.Resource("oidcclientsecretrequests") +} + +func kindFromContext(ctx context.Context) schema.GroupKind { + if info, ok := genericapirequest.RequestInfoFrom(ctx); ok { + return schema.GroupKind{Group: info.APIGroup, Kind: "OIDCClientSecretRequest"} + } + // this should never happen in practice + return clientsecretapi.Kind("OIDCClientSecretRequest") +} diff --git a/internal/supervisor/apiserver/apiserver.go b/internal/supervisor/apiserver/apiserver.go index 2da90422..9f0cbc52 100644 --- a/internal/supervisor/apiserver/apiserver.go +++ b/internal/supervisor/apiserver/apiserver.go @@ -14,8 +14,10 @@ import ( "k8s.io/apimachinery/pkg/util/errors" "k8s.io/apiserver/pkg/registry/rest" genericapiserver "k8s.io/apiserver/pkg/server" + corev1client "k8s.io/client-go/kubernetes/typed/core/v1" "k8s.io/client-go/pkg/version" + configv1alpha1clientset "go.pinniped.dev/generated/latest/client/supervisor/clientset/versioned/typed/config/v1alpha1" "go.pinniped.dev/internal/controllerinit" "go.pinniped.dev/internal/plog" "go.pinniped.dev/internal/registry/clientsecretrequest" @@ -31,6 +33,9 @@ type ExtraConfig struct { Scheme *runtime.Scheme NegotiatedSerializer runtime.NegotiatedSerializer ClientSecretSupervisorGroupVersion schema.GroupVersion + Secrets corev1client.SecretInterface + OIDCClients configv1alpha1clientset.OIDCClientInterface + Namespace string } type PinnipedServer struct { @@ -75,7 +80,7 @@ func (c completedConfig) New() (*PinnipedServer, error) { for _, f := range []func() (schema.GroupVersionResource, rest.Storage){ func() (schema.GroupVersionResource, rest.Storage) { clientSecretReqGVR := c.ExtraConfig.ClientSecretSupervisorGroupVersion.WithResource("oidcclientsecretrequests") - clientSecretReqStorage := clientsecretrequest.NewREST(clientSecretReqGVR.GroupResource()) + clientSecretReqStorage := clientsecretrequest.NewREST(c.ExtraConfig.Secrets, c.ExtraConfig.OIDCClients, c.ExtraConfig.Namespace) return clientSecretReqGVR, clientSecretReqStorage }, } { diff --git a/internal/supervisor/server/server.go b/internal/supervisor/server/server.go index 02574854..31d691e2 100644 --- a/internal/supervisor/server/server.go +++ b/internal/supervisor/server/server.go @@ -31,6 +31,7 @@ import ( genericoptions "k8s.io/apiserver/pkg/server/options" kubeinformers "k8s.io/client-go/informers" "k8s.io/client-go/kubernetes" + corev1client "k8s.io/client-go/kubernetes/typed/core/v1" "k8s.io/client-go/pkg/version" "k8s.io/client-go/rest" aggregatorclient "k8s.io/kube-aggregator/pkg/client/clientset_generated/clientset" @@ -38,6 +39,7 @@ import ( configv1alpha1 "go.pinniped.dev/generated/latest/apis/supervisor/config/v1alpha1" pinnipedclientset "go.pinniped.dev/generated/latest/client/supervisor/clientset/versioned" + "go.pinniped.dev/generated/latest/client/supervisor/clientset/versioned/typed/config/v1alpha1" pinnipedinformers "go.pinniped.dev/generated/latest/client/supervisor/informers/externalversions" "go.pinniped.dev/internal/apiserviceref" "go.pinniped.dev/internal/config/supervisor" @@ -475,6 +477,9 @@ func runSupervisor(ctx context.Context, podInfo *downward.PodInfo, cfg *supervis *cfg.AggregatedAPIServerPort, scheme, clientSecretGV, + clientWithoutLeaderElection.Kubernetes.CoreV1().Secrets(serverInstallationNamespace), + client.PinnipedSupervisor.ConfigV1alpha1().OIDCClients(serverInstallationNamespace), + serverInstallationNamespace, ) if err != nil { return fmt.Errorf("could not configure aggregated API server: %w", err) @@ -568,7 +573,6 @@ func runSupervisor(ctx context.Context, podInfo *downward.PodInfo, cfg *supervis return nil } -// Create a configuration for the aggregated API server. func getAggregatedAPIServerConfig( dynamicCertProvider dynamiccert.Private, buildControllers controllerinit.RunnerBuilder, @@ -576,6 +580,9 @@ func getAggregatedAPIServerConfig( aggregatedAPIServerPort int64, scheme *runtime.Scheme, clientSecretSupervisorGroupVersion schema.GroupVersion, + secrets corev1client.SecretInterface, + oidcClients v1alpha1.OIDCClientInterface, + serverInstallationNamespace string, ) (*apiserver.Config, error) { codecs := serializer.NewCodecFactory(scheme) @@ -620,6 +627,9 @@ func getAggregatedAPIServerConfig( Scheme: scheme, NegotiatedSerializer: codecs, ClientSecretSupervisorGroupVersion: clientSecretSupervisorGroupVersion, + Secrets: secrets, + OIDCClients: oidcClients, + Namespace: serverInstallationNamespace, }, } return apiServerConfig, nil diff --git a/proposals/1125_dynamic-supervisor-oidc-clients/ACCEPTANCE-NOTES.md b/proposals/1125_dynamic-supervisor-oidc-clients/ACCEPTANCE-NOTES.md new file mode 100644 index 00000000..1155d3a1 --- /dev/null +++ b/proposals/1125_dynamic-supervisor-oidc-clients/ACCEPTANCE-NOTES.md @@ -0,0 +1,200 @@ +# Notes for story acceptance for the dynamic clients feature + +Rather than writing a webapp to manually test the dynamic client features during user story acceptance, +we can simulate the requests that a webapp would make to the Supervisor using the commands shown below. +The commands below the happy path for a fully-capable OIDCClient which is allowed to use all supported +grant types and scopes. These commands can be adjusted to test other scenarios of interest. + +## Deploy and configure a basic Supervisor locally + +We can use the developer hack scripts to deploy a working Supervisor on a local Kind cluster. +These clusters have no ingress, so we use Kind's port mapping feature to expose a web proxy outside +the cluster. The proxy can then be used to access the Supervisor. In this setup, the Supervisor's CA +is not trusted by the web browser, however, the curl commands can trust the CA cert by using the `--cacert` flag. + +```shell +./hack/prepare-for-integration-tests.sh -c +source /tmp/integration-test-env +# We'll use LDAP so we can login in via curl commands through the Supervisor. +./hack/prepare-supervisor-on-kind.sh --ldap --flow browser_authcode +``` + +Alternatively, the Supervisor could be installed into a cluster in a more production-like way, with ingress, +a DNS entry, and TLS certs. In this case, the proxy env vars used below would not be needed, and the issuer string +would be adjusted to match the Supervisor's ingress DNS hostname. + +## Create an OIDCClient + +```shell +cat <