From c05fc7b51aa588d1d04ef3996a5e084865b72004 Mon Sep 17 00:00:00 2001 From: Andre Aranha Date: Tue, 1 Oct 2024 15:39:59 +0200 Subject: [PATCH] Support rotation and variable number of Fernet keys Add configuration for specifying the number of fernet keys stored in the keystone secret. More than 2 keys are needed, since rotating 2 keys would expire sessions on every rotation. After configuration change, keys need to be added/removed and rotated in the proper order, to ensure that the sessions don't expire prematurely. Fernet key rotation is triggered in the reconcile loop. The "rotated at" timestamp is set in the secret annotation. Co-Authored-By: Grzegorz Grasza --- .../keystone.openstack.org_keystoneapis.yaml | 14 ++ api/v1beta1/keystoneapi_types.go | 12 ++ api/v1beta1/zz_generated.deepcopy.go | 10 ++ .../keystone.openstack.org_keystoneapis.yaml | 14 ++ controllers/keystoneapi_controller.go | 127 ++++++++++++++++-- pkg/keystone/bootstrap.go | 4 +- pkg/keystone/const.go | 5 + pkg/keystone/cronjob.go | 4 +- pkg/keystone/dbsync.go | 5 +- pkg/keystone/deployment.go | 2 +- pkg/keystone/fernet.go | 1 - pkg/keystone/volumes.go | 29 ++-- .../functional/keystoneapi_controller_test.go | 68 ++++++++++ tests/kuttl/tests/keystone_tls/01-assert.yaml | 6 + 14 files changed, 271 insertions(+), 30 deletions(-) diff --git a/api/bases/keystone.openstack.org_keystoneapis.yaml b/api/bases/keystone.openstack.org_keystoneapis.yaml index 1ce9da9e..f460b115 100644 --- a/api/bases/keystone.openstack.org_keystoneapis.yaml +++ b/api/bases/keystone.openstack.org_keystoneapis.yaml @@ -89,6 +89,20 @@ spec: description: EnableSecureRBAC - Enable Consistent and Secure RBAC policies type: boolean + fernetMaxActiveKeys: + default: 5 + description: FernetMaxActiveKeys - Maximum number of fernet token + keys after rotation + format: int32 + minimum: 3 + type: integer + fernetRotationDays: + default: 1 + description: FernetRotationDays - Rotate fernet token keys every X + days + format: int32 + minimum: 1 + type: integer memcachedInstance: default: memcached description: Memcached instance name. diff --git a/api/v1beta1/keystoneapi_types.go b/api/v1beta1/keystoneapi_types.go index a2064d7f..a4c9b3cc 100644 --- a/api/v1beta1/keystoneapi_types.go +++ b/api/v1beta1/keystoneapi_types.go @@ -119,6 +119,18 @@ type KeystoneAPISpecCore struct { // TrustFlushSuspend - Suspend the cron job to purge trusts TrustFlushSuspend bool `json:"trustFlushSuspend"` + // +kubebuilder:validation:Optional + // +kubebuilder:default=1 + // +kubebuilder:validation:Minimum=1 + // FernetRotationDays - Rotate fernet token keys every X days + FernetRotationDays *int32 `json:"fernetRotationDays"` + + // +kubebuilder:validation:Optional + // +kubebuilder:default=5 + // +kubebuilder:validation:Minimum=3 + // FernetMaxActiveKeys - Maximum number of fernet token keys after rotation + FernetMaxActiveKeys *int32 `json:"fernetMaxActiveKeys"` + // +kubebuilder:validation:Optional // +kubebuilder:default={admin: AdminPassword} // PasswordSelectors - Selectors to identify the AdminUser password from the Secret diff --git a/api/v1beta1/zz_generated.deepcopy.go b/api/v1beta1/zz_generated.deepcopy.go index f079bd32..7ee47492 100644 --- a/api/v1beta1/zz_generated.deepcopy.go +++ b/api/v1beta1/zz_generated.deepcopy.go @@ -147,6 +147,16 @@ func (in *KeystoneAPISpecCore) DeepCopyInto(out *KeystoneAPISpecCore) { *out = new(int32) **out = **in } + if in.FernetRotationDays != nil { + in, out := &in.FernetRotationDays, &out.FernetRotationDays + *out = new(int32) + **out = **in + } + if in.FernetMaxActiveKeys != nil { + in, out := &in.FernetMaxActiveKeys, &out.FernetMaxActiveKeys + *out = new(int32) + **out = **in + } out.PasswordSelectors = in.PasswordSelectors if in.NodeSelector != nil { in, out := &in.NodeSelector, &out.NodeSelector diff --git a/config/crd/bases/keystone.openstack.org_keystoneapis.yaml b/config/crd/bases/keystone.openstack.org_keystoneapis.yaml index 1ce9da9e..f460b115 100644 --- a/config/crd/bases/keystone.openstack.org_keystoneapis.yaml +++ b/config/crd/bases/keystone.openstack.org_keystoneapis.yaml @@ -89,6 +89,20 @@ spec: description: EnableSecureRBAC - Enable Consistent and Secure RBAC policies type: boolean + fernetMaxActiveKeys: + default: 5 + description: FernetMaxActiveKeys - Maximum number of fernet token + keys after rotation + format: int32 + minimum: 3 + type: integer + fernetRotationDays: + default: 1 + description: FernetRotationDays - Rotate fernet token keys every X + days + format: int32 + minimum: 1 + type: integer memcachedInstance: default: memcached description: Memcached instance name. diff --git a/controllers/keystoneapi_controller.go b/controllers/keystoneapi_controller.go index f8db9a4b..e7f3e2da 100644 --- a/controllers/keystoneapi_controller.go +++ b/controllers/keystoneapi_controller.go @@ -857,7 +857,6 @@ func (r *KeystoneAPIReconciler) reconcileNormal( // // Create secret holding fernet keys (for token and credential) // - // TODO key rotation err = r.ensureFernetKeys(ctx, instance, helper, &configMapVars) if err != nil { instance.Status.Conditions.Set(condition.FalseCondition( @@ -1321,37 +1320,53 @@ func (r *KeystoneAPIReconciler) reconcileCloudConfig( return oko_secret.EnsureSecrets(ctx, h, instance, secrets, nil) } -// ensureFernetKeys - creates secret with fernet keys +// ensureFernetKeys - creates secret with fernet keys, rotates the keys func (r *KeystoneAPIReconciler) ensureFernetKeys( ctx context.Context, instance *keystonev1.KeystoneAPI, helper *helper.Helper, envVars *map[string]env.Setter, ) error { + fernetAnnotation := labels.GetGroupLabel(keystone.ServiceName) + "/rotatedat" labels := labels.GetLabels(instance, labels.GetGroupLabel(keystone.ServiceName), map[string]string{}) + now := time.Now().UTC() // // check if secret already exist // secretName := keystone.ServiceName + var numberKeys int + if instance.Spec.FernetMaxActiveKeys == nil { + numberKeys = keystone.DefaultFernetMaxActiveKeys + } else { + numberKeys = int(*instance.Spec.FernetMaxActiveKeys) + } + secret, hash, err := oko_secret.GetSecret(ctx, helper, secretName, instance.Namespace) + if err != nil && !k8s_errors.IsNotFound(err) { return err } else if k8s_errors.IsNotFound(err) { fernetKeys := map[string]string{ - "FernetKeys0": keystone.GenerateFernetKey(), - "FernetKeys1": keystone.GenerateFernetKey(), "CredentialKeys0": keystone.GenerateFernetKey(), "CredentialKeys1": keystone.GenerateFernetKey(), } + for i := 0; i < numberKeys; i++ { + fernetKeys[fmt.Sprintf("FernetKeys%d", i)] = keystone.GenerateFernetKey() + } + + annotations := map[string]string{ + fernetAnnotation: now.Format(time.RFC3339)} + tmpl := []util.Template{ { - Name: secretName, - Namespace: instance.Namespace, - Type: util.TemplateTypeNone, - CustomData: fernetKeys, - Labels: labels, + Name: secretName, + Namespace: instance.Namespace, + Type: util.TemplateTypeNone, + CustomData: fernetKeys, + Labels: labels, + Annotations: annotations, }, } err := oko_secret.EnsureSecrets(ctx, helper, instance, tmpl, envVars) @@ -1361,9 +1376,99 @@ func (r *KeystoneAPIReconciler) ensureFernetKeys( } else { // add hash to envVars (*envVars)[secret.Name] = env.SetValue(hash) - } - // TODO: fernet key rotation + changedKeys := false + + extraKey := fmt.Sprintf("FernetKeys%d", numberKeys) + + // + // Fernet Key rotation + // + if secret.Annotations == nil { + secret.Annotations = map[string]string{} + } + rotatedAt, err := time.Parse(time.RFC3339, secret.Annotations[fernetAnnotation]) + + var duration int + if instance.Spec.FernetRotationDays == nil { + duration = keystone.DefaultFernetRotationDays + } else { + duration = int(*instance.Spec.FernetRotationDays) + } + + if err != nil { + changedKeys = true + } else if rotatedAt.AddDate(0, 0, duration).Before(now) { + secret.Data[extraKey] = secret.Data["FernetKeys0"] + secret.Data["FernetKeys0"] = []byte(keystone.GenerateFernetKey()) + } + + // + // Remove extra keys when FernetMaxActiveKeys changes + // + for { + _, exists := secret.Data[extraKey] + if !exists { + break + } + changedKeys = true + i := 1 + for { + key := fmt.Sprintf("FernetKeys%d", i) + i++ + nextKey := fmt.Sprintf("FernetKeys%d", i) + _, exists = secret.Data[nextKey] + if !exists { + break + } + secret.Data[key] = secret.Data[nextKey] + delete(secret.Data, nextKey) + } + } + + // + // Add extra keys when FernetMaxActiveKeys changes + // + lastKey := fmt.Sprintf("FernetKeys%d", numberKeys-1) + for { + _, exists := secret.Data[lastKey] + if exists { + break + } + changedKeys = true + i := 1 + nextKeyValue := []byte(keystone.GenerateFernetKey()) + for { + key := fmt.Sprintf("FernetKeys%d", i) + i++ + keyValue, exists := secret.Data[key] + secret.Data[key] = nextKeyValue + nextKeyValue = keyValue + if !exists { + break + } + } + } + + if !changedKeys { + return nil + } + + fernetKeys := make(map[string]string, len(secret.Data)) + for k, v := range secret.Data { + fernetKeys[k] = string(v[:]) + } + + secret.Annotations[fernetAnnotation] = now.Format(time.RFC3339) + + // use update to apply changes to the secret, since EnsureSecrets + // does not handle annotation updates, also CreateOrPatchSecret would + // preserve the existing annotation + err = helper.GetClient().Update(ctx, secret, &client.UpdateOptions{}) + if err != nil { + return err + } + } return nil } diff --git a/pkg/keystone/bootstrap.go b/pkg/keystone/bootstrap.go index f03b1cdc..8748673d 100644 --- a/pkg/keystone/bootstrap.go +++ b/pkg/keystone/bootstrap.go @@ -60,12 +60,12 @@ func BootstrapJob( } // create Volume and VolumeMounts - volumes := getVolumes(instance.Name) + volumes := getVolumes(instance) volumeMounts := getVolumeMounts() // add CA cert if defined if instance.Spec.TLS.CaBundleSecretName != "" { - volumes = append(getVolumes(instance.Name), instance.Spec.TLS.CreateVolume()) + volumes = append(getVolumes(instance), instance.Spec.TLS.CreateVolume()) volumeMounts = append(getVolumeMounts(), instance.Spec.TLS.CreateVolumeMounts(nil)...) } diff --git a/pkg/keystone/const.go b/pkg/keystone/const.go index b2e112f6..205b68b5 100644 --- a/pkg/keystone/const.go +++ b/pkg/keystone/const.go @@ -28,4 +28,9 @@ const ( KeystonePublicPort int32 = 5000 // KeystoneInternalPort - KeystoneInternalPort int32 = 5000 + + // DefaultFernetMaxActiveKeys - + DefaultFernetMaxActiveKeys = 5 + // DefaultFernetRotationDays - + DefaultFernetRotationDays = 1 ) diff --git a/pkg/keystone/cronjob.go b/pkg/keystone/cronjob.go index 179ade74..38a1051b 100644 --- a/pkg/keystone/cronjob.go +++ b/pkg/keystone/cronjob.go @@ -46,12 +46,12 @@ func CronJob( completions := int32(1) // create Volume and VolumeMounts - volumes := getVolumes(instance.Name) + volumes := getVolumes(instance) volumeMounts := getVolumeMounts() // add CA cert if defined if instance.Spec.TLS.CaBundleSecretName != "" { - volumes = append(getVolumes(instance.Name), instance.Spec.TLS.CreateVolume()) + volumes = append(getVolumes(instance), instance.Spec.TLS.CreateVolume()) volumeMounts = append(getVolumeMounts(), instance.Spec.TLS.CreateVolumeMounts(nil)...) } diff --git a/pkg/keystone/dbsync.go b/pkg/keystone/dbsync.go index 652f1185..e760b4d6 100644 --- a/pkg/keystone/dbsync.go +++ b/pkg/keystone/dbsync.go @@ -45,12 +45,13 @@ func DbSyncJob( envVars["KOLLA_BOOTSTRAP"] = env.SetValue("true") // create Volume and VolumeMounts - volumes := getVolumes(instance.Name) + volumes := getVolumes(instance) volumeMounts := getVolumeMounts() // add CA cert if defined if instance.Spec.TLS.CaBundleSecretName != "" { - volumes = append(getVolumes(instance.Name), instance.Spec.TLS.CreateVolume()) + //TODO(afaranha): Why not reuse the 'volumes'? + volumes = append(getVolumes(instance), instance.Spec.TLS.CreateVolume()) volumeMounts = append(getVolumeMounts(), instance.Spec.TLS.CreateVolumeMounts(nil)...) } diff --git a/pkg/keystone/deployment.go b/pkg/keystone/deployment.go index 70cbb663..00c418f4 100644 --- a/pkg/keystone/deployment.go +++ b/pkg/keystone/deployment.go @@ -80,7 +80,7 @@ func Deployment( envVars["CONFIG_HASH"] = env.SetValue(configHash) // create Volume and VolumeMounts - volumes := getVolumes(instance.Name) + volumes := getVolumes(instance) volumeMounts := getVolumeMounts() // add CA cert if defined diff --git a/pkg/keystone/fernet.go b/pkg/keystone/fernet.go index 635b77db..10f9a08f 100644 --- a/pkg/keystone/fernet.go +++ b/pkg/keystone/fernet.go @@ -17,7 +17,6 @@ package keystone import ( "encoding/base64" - "math/rand" ) diff --git a/pkg/keystone/volumes.go b/pkg/keystone/volumes.go index 3055a62d..2f863ace 100644 --- a/pkg/keystone/volumes.go +++ b/pkg/keystone/volumes.go @@ -16,14 +16,30 @@ limitations under the License. package keystone import ( + "fmt" + keystonev1 "github.com/openstack-k8s-operators/keystone-operator/api/v1beta1" corev1 "k8s.io/api/core/v1" ) // getVolumes - service volumes -func getVolumes(name string) []corev1.Volume { +func getVolumes(instance *keystonev1.KeystoneAPI) []corev1.Volume { + name := instance.Name var scriptsVolumeDefaultMode int32 = 0755 var config0640AccessMode int32 = 0640 + fernetKeys := []corev1.KeyToPath{} + numberKeys := int(*instance.Spec.FernetMaxActiveKeys) + + for i := 0; i < numberKeys; i++ { + fernetKeys = append( + fernetKeys, + corev1.KeyToPath{ + Key: fmt.Sprintf("FernetKeys%d", i), + Path: fmt.Sprintf("%d", i), + }, + ) + } + return []corev1.Volume{ { Name: "scripts", @@ -48,16 +64,7 @@ func getVolumes(name string) []corev1.Volume { VolumeSource: corev1.VolumeSource{ Secret: &corev1.SecretVolumeSource{ SecretName: ServiceName, - Items: []corev1.KeyToPath{ - { - Key: "FernetKeys0", - Path: "0", - }, - { - Key: "FernetKeys1", - Path: "1", - }, - }, + Items: fernetKeys, }, }, }, diff --git a/tests/functional/keystoneapi_controller_test.go b/tests/functional/keystoneapi_controller_test.go index 5633afb0..7dd98fa9 100644 --- a/tests/functional/keystoneapi_controller_test.go +++ b/tests/functional/keystoneapi_controller_test.go @@ -19,6 +19,8 @@ package functional_test import ( "fmt" "os" + "strconv" + "time" . "github.com/onsi/ginkgo/v2" //revive:disable:dot-imports . "github.com/onsi/gomega" //revive:disable:dot-imports @@ -33,6 +35,7 @@ import ( corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/types" "k8s.io/utils/ptr" + "sigs.k8s.io/controller-runtime/pkg/client" ) var _ = Describe("Keystone controller", func() { @@ -1106,6 +1109,71 @@ var _ = Describe("Keystone controller", func() { }) }) + // Set rotated at to past date, triggering rotation + When("When the fernet token rotate", func() { + BeforeEach(func() { + DeferCleanup( + k8sClient.Delete, ctx, CreateKeystoneMessageBusSecret(namespace, "rabbitmq-secret")) + DeferCleanup(th.DeleteInstance, CreateKeystoneAPI(keystoneAPIName, GetDefaultKeystoneAPISpec())) + DeferCleanup( + k8sClient.Delete, ctx, CreateKeystoneAPISecret(namespace, SecretName)) + DeferCleanup(infra.DeleteMemcached, infra.CreateMemcached(namespace, "memcached", memcachedSpec)) + DeferCleanup( + mariadb.DeleteDBService, + mariadb.CreateDBService( + namespace, + GetKeystoneAPI(keystoneAPIName).Spec.DatabaseInstance, + corev1.ServiceSpec{ + Ports: []corev1.ServicePort{{Port: 3306}}, + }, + ), + ) + mariadb.SimulateMariaDBAccountCompleted(keystoneAccountName) + mariadb.SimulateMariaDBDatabaseCompleted(keystoneDatabaseName) + infra.SimulateTransportURLReady(types.NamespacedName{ + Name: fmt.Sprintf("%s-keystone-transport", keystoneAPIName.Name), + Namespace: namespace, + }) + infra.SimulateMemcachedReady(types.NamespacedName{ + Name: "memcached", + Namespace: namespace, + }) + th.SimulateJobSuccess(dbSyncJobName) + th.SimulateJobSuccess(bootstrapJobName) + th.SimulateDeploymentReplicaReady(deploymentName) + }) + + It("rotates the fernet keys", func() { + keystone := GetKeystoneAPI(keystoneAPIName) + currentHash := keystone.Status.Hash["input"] + + currentSecret := th.GetSecret(types.NamespacedName{Namespace: keystoneAPIName.Namespace, Name: "keystone"}) + Expect(currentSecret).ToNot(BeNil()) + + rotatedAt, err := time.Parse(time.RFC3339, currentSecret.Annotations["keystone.openstack.org/rotatedat"]) + Expect(err).ToNot(HaveOccurred()) + + // set date to yesterday + currentSecret.Annotations["keystone.openstack.org/rotatedat"] = rotatedAt.Add(-25 * time.Hour).Format(time.RFC3339) + err = k8sClient.Update(ctx, ptr.To(currentSecret), &client.UpdateOptions{}) + Expect(err).ToNot(HaveOccurred()) + + Eventually(func(g Gomega) { + keystone = GetKeystoneAPI(keystoneAPIName) + g.Expect(keystone.Status.Hash["input"]).ToNot(Equal(currentHash)) + + updatedSecret := th.GetSecret(types.NamespacedName{Namespace: keystoneAPIName.Namespace, Name: "keystone"}) + g.Expect(updatedSecret).ToNot(BeNil()) + + oldKey := string(currentSecret.Data["FernetKeys"+strconv.Itoa(0)]) + newKey := string(updatedSecret.Data["FernetKeys"+strconv.Itoa((4))]) + + g.Expect(oldKey).To(Equal(newKey)) + }, timeout, interval).Should(Succeed()) + + }) + }) + // Run MariaDBAccount suite tests. these are pre-packaged ginkgo tests // that exercise standard account create / update patterns that should be // common to all controllers that ensure MariaDBAccount CRs. diff --git a/tests/kuttl/tests/keystone_tls/01-assert.yaml b/tests/kuttl/tests/keystone_tls/01-assert.yaml index fd413a48..0f44dce3 100644 --- a/tests/kuttl/tests/keystone_tls/01-assert.yaml +++ b/tests/kuttl/tests/keystone_tls/01-assert.yaml @@ -88,6 +88,12 @@ spec: path: "0" - key: FernetKeys1 path: "1" + - key: FernetKeys2 + path: "2" + - key: FernetKeys3 + path: "3" + - key: FernetKeys4 + path: "4" secretName: keystone - name: credential-keys secret: