From e454d7b03068ca67d2aef81d92f9ab1a6d7b7db2 Mon Sep 17 00:00:00 2001 From: akutz Date: Wed, 23 Oct 2024 16:37:06 -0500 Subject: [PATCH] Rekey disks for encrypted VMs This patch supports rekeying disks when an encrypted VM is rekeyed. --- .../storageclass_controller_intg_test.go | 4 +- .../storageclass_controller_unit_test.go | 4 +- pkg/util/kube/storage.go | 28 +++-- pkg/util/kube/storage_test.go | 40 ++++-- pkg/vmconfig/crypto/crypto_reconciler_pre.go | 114 +++++++++++++++++- .../mutation/virtualmachine_mutator.go | 2 +- .../validation/virtualmachine_validator.go | 2 +- 7 files changed, 161 insertions(+), 33 deletions(-) diff --git a/controllers/storageclass/storageclass_controller_intg_test.go b/controllers/storageclass/storageclass_controller_intg_test.go index 43bbfe207..5007bc55f 100644 --- a/controllers/storageclass/storageclass_controller_intg_test.go +++ b/controllers/storageclass/storageclass_controller_intg_test.go @@ -59,7 +59,7 @@ func intgTestsReconcile() { }) It("should eventually mark the storage class as encrypted in the context", func() { Eventually(func(g Gomega) { - ok, err := kubeutil.IsEncryptedStorageClass(ctx, ctx.Client, obj.Name) + ok, _, err := kubeutil.IsEncryptedStorageClass(ctx, ctx.Client, obj.Name) Expect(err).ToNot(HaveOccurred()) Expect(ok).To(BeTrue()) }) @@ -69,7 +69,7 @@ func intgTestsReconcile() { Context("reconciling a plain storage class", func() { It("should never mark the storage class as encrypted in the context", func() { Consistently(func(g Gomega) { - ok, err := kubeutil.IsEncryptedStorageClass(ctx, ctx.Client, obj.Name) + ok, _, err := kubeutil.IsEncryptedStorageClass(ctx, ctx.Client, obj.Name) Expect(err).ToNot(HaveOccurred()) Expect(ok).To(BeFalse()) }) diff --git a/controllers/storageclass/storageclass_controller_unit_test.go b/controllers/storageclass/storageclass_controller_unit_test.go index 5514b795a..5ebfdbcb8 100644 --- a/controllers/storageclass/storageclass_controller_unit_test.go +++ b/controllers/storageclass/storageclass_controller_unit_test.go @@ -123,7 +123,7 @@ func unitTestsReconcile() { }) It("marks item as encrypted and returns success", func() { Expect(err).ToNot(HaveOccurred()) - ok, err := kubeutil.IsEncryptedStorageClass(ctx, ctx.Client, obj.Name) + ok, _, err := kubeutil.IsEncryptedStorageClass(ctx, ctx.Client, obj.Name) Expect(err).ToNot(HaveOccurred()) Expect(ok).To(BeFalse()) }) @@ -136,7 +136,7 @@ func unitTestsReconcile() { }) It("marks item as encrypted and returns success", func() { Expect(err).ToNot(HaveOccurred()) - ok, err := kubeutil.IsEncryptedStorageClass(ctx, ctx.Client, obj.Name) + ok, _, err := kubeutil.IsEncryptedStorageClass(ctx, ctx.Client, obj.Name) Expect(err).ToNot(HaveOccurred()) Expect(ok).To(BeTrue()) }) diff --git a/pkg/util/kube/storage.go b/pkg/util/kube/storage.go index a380afae5..524efb37d 100644 --- a/pkg/util/kube/storage.go +++ b/pkg/util/kube/storage.go @@ -211,20 +211,21 @@ func MarkEncryptedStorageClass( return k8sClient.Patch(ctx, &obj, objPatch) } -// IsEncryptedStorageClass returns true if the provided StorageClass was marked -// as encrypted. +// IsEncryptedStorageClass returns true if the provided StorageClass name was +// marked as encrypted. If encryption is supported, the StorageClass's profile +// ID is also returned. func IsEncryptedStorageClass( ctx context.Context, k8sClient ctrlclient.Client, - storageClassName string) (bool, error) { + name string) (bool, string, error) { var obj storagev1.StorageClass if err := k8sClient.Get( ctx, - ctrlclient.ObjectKey{Name: storageClassName}, + ctrlclient.ObjectKey{Name: name}, &obj); err != nil { - return false, ctrlclient.IgnoreNotFound(err) + return false, "", ctrlclient.IgnoreNotFound(err) } return isEncryptedStorageClass(ctx, k8sClient, obj) @@ -244,7 +245,11 @@ func IsEncryptedStorageProfile( for i := range obj.Items { if pid, _ := GetStoragePolicyID(obj.Items[i]); pid == profileID { - return isEncryptedStorageClass(ctx, k8sClient, obj.Items[i]) + ok, _, err := isEncryptedStorageClass( + ctx, + k8sClient, + obj.Items[i]) + return ok, err } } @@ -254,7 +259,7 @@ func IsEncryptedStorageProfile( func isEncryptedStorageClass( ctx context.Context, k8sClient ctrlclient.Client, - storageClass storagev1.StorageClass) (bool, error) { + storageClass storagev1.StorageClass) (bool, string, error) { var ( obj corev1.ConfigMap @@ -266,8 +271,13 @@ func isEncryptedStorageClass( ) if err := k8sClient.Get(ctx, objKey, &obj); err != nil { - return false, ctrlclient.IgnoreNotFound(err) + return false, "", ctrlclient.IgnoreNotFound(err) } - return slices.Contains(obj.OwnerReferences, ownerRef), nil + if slices.Contains(obj.OwnerReferences, ownerRef) { + profileID, err := GetStoragePolicyID(storageClass) + return true, profileID, err + } + + return false, "", nil } diff --git a/pkg/util/kube/storage_test.go b/pkg/util/kube/storage_test.go index 72e7f6f1b..ea01d6a98 100644 --- a/pkg/util/kube/storage_test.go +++ b/pkg/util/kube/storage_test.go @@ -316,7 +316,7 @@ var _ = Describe("IsEncryptedStorageClass", func() { WithInterceptorFuncs(funcs). Build() - ok, err = kubeutil.IsEncryptedStorageClass( + ok, _, err = kubeutil.IsEncryptedStorageClass( ctx, client, storageClass.Name) }) @@ -480,6 +480,9 @@ var _ = Describe("EncryptedStorageClass", func() { Name: fakeString, UID: types.UID(uuid.NewString()), }, + Parameters: map[string]string{ + internal.StoragePolicyIDParameter: fakeString, + }, } }) @@ -495,18 +498,20 @@ var _ = Describe("EncryptedStorageClass", func() { Expect(kubeutil.MarkEncryptedStorageClass(ctx, client, storageClass, true)).To(Succeed()) }) It("should return true", func() { - ok, err := kubeutil.IsEncryptedStorageClass(ctx, client, storageClass.Name) + ok, pid, err := kubeutil.IsEncryptedStorageClass(ctx, client, storageClass.Name) Expect(err).ToNot(HaveOccurred()) Expect(ok).To(BeTrue()) + Expect(pid).To(Equal(fakeString)) }) When("the storage class is marked as encrypted again", func() { JustBeforeEach(func() { Expect(kubeutil.MarkEncryptedStorageClass(ctx, client, storageClass, true)).To(Succeed()) }) It("should return true", func() { - ok, err := kubeutil.IsEncryptedStorageClass(ctx, client, storageClass.Name) + ok, pid, err := kubeutil.IsEncryptedStorageClass(ctx, client, storageClass.Name) Expect(err).ToNot(HaveOccurred()) Expect(ok).To(BeTrue()) + Expect(pid).To(Equal(fakeString)) }) }) When("the storage class is marked as unencrypted", func() { @@ -514,9 +519,11 @@ var _ = Describe("EncryptedStorageClass", func() { Expect(kubeutil.MarkEncryptedStorageClass(ctx, client, storageClass, false)).To(Succeed()) }) It("should return false", func() { - ok, err := kubeutil.IsEncryptedStorageClass(ctx, client, storageClass.Name) + ok, pid, err := kubeutil.IsEncryptedStorageClass(ctx, client, storageClass.Name) Expect(err).ToNot(HaveOccurred()) Expect(ok).To(BeFalse()) + Expect(pid).To(BeEmpty()) + }) When("the storage class is marked as encrypted again", func() { @@ -524,9 +531,10 @@ var _ = Describe("EncryptedStorageClass", func() { Expect(kubeutil.MarkEncryptedStorageClass(ctx, client, storageClass, true)).To(Succeed()) }) It("should return true", func() { - ok, err := kubeutil.IsEncryptedStorageClass(ctx, client, storageClass.Name) + ok, pid, err := kubeutil.IsEncryptedStorageClass(ctx, client, storageClass.Name) Expect(err).ToNot(HaveOccurred()) Expect(ok).To(BeTrue()) + Expect(pid).To(Equal(fakeString)) }) }) }) @@ -537,9 +545,10 @@ var _ = Describe("EncryptedStorageClass", func() { Expect(kubeutil.MarkEncryptedStorageClass(ctx, client, storageClass, false)).To(Succeed()) }) It("should return false for the second storage class", func() { - ok, err := kubeutil.IsEncryptedStorageClass(ctx, client, storageClass.Name) + ok, pid, err := kubeutil.IsEncryptedStorageClass(ctx, client, storageClass.Name) Expect(err).ToNot(HaveOccurred()) Expect(ok).To(BeFalse()) + Expect(pid).To(BeEmpty()) }) }) }) @@ -549,17 +558,19 @@ var _ = Describe("EncryptedStorageClass", func() { Expect(kubeutil.MarkEncryptedStorageClass(ctx, client, storageClass, false)).To(Succeed()) }) It("should return false", func() { - ok, err := kubeutil.IsEncryptedStorageClass(ctx, client, storageClass.Name) + ok, pid, err := kubeutil.IsEncryptedStorageClass(ctx, client, storageClass.Name) Expect(err).ToNot(HaveOccurred()) Expect(ok).To(BeFalse()) + Expect(pid).To(BeEmpty()) }) }) When("the storage class is not marked at all", func() { It("should return false", func() { - ok, err := kubeutil.IsEncryptedStorageClass(ctx, client, storageClass.Name) + ok, pid, err := kubeutil.IsEncryptedStorageClass(ctx, client, storageClass.Name) Expect(err).ToNot(HaveOccurred()) Expect(ok).To(BeFalse()) + Expect(pid).To(BeEmpty()) }) }) @@ -580,9 +591,10 @@ var _ = Describe("EncryptedStorageClass", func() { } }) It("should return an error for IsEncryptedStorageClass", func() { - ok, err := kubeutil.IsEncryptedStorageClass(ctx, client, storageClass.Name) + ok, pid, err := kubeutil.IsEncryptedStorageClass(ctx, client, storageClass.Name) Expect(err).To(MatchError(apierrors.NewInternalError(errors.New(fakeString)).Error())) Expect(ok).To(BeFalse()) + Expect(pid).To(BeEmpty()) }) It("should return an error for MarkEncryptedStorageClass", func() { err := kubeutil.MarkEncryptedStorageClass(ctx, client, storageClass, false) @@ -683,10 +695,11 @@ var _ = Describe("EncryptedStorageClass", func() { Expect(refs).To(ContainElements(expRefs...)) for i := range storageClasses { - ok, err := kubeutil.IsEncryptedStorageClass( + ok, pid, err := kubeutil.IsEncryptedStorageClass( ctx, client, storageClasses[i].Name) Expect(err).ToNot(HaveOccurred()) Expect(ok).To(BeTrue()) + Expect(pid).To(Equal(fakeString)) } }) }) @@ -733,10 +746,11 @@ var _ = Describe("EncryptedStorageClass", func() { Expect(refs).To(BeEmpty()) for i := range storageClasses { - ok, err := kubeutil.IsEncryptedStorageClass( + ok, pid, err := kubeutil.IsEncryptedStorageClass( ctx, client, storageClasses[i].Name) Expect(err).ToNot(HaveOccurred()) Expect(ok).To(BeFalse()) + Expect(pid).To(BeEmpty()) } }) }) @@ -801,13 +815,15 @@ var _ = Describe("EncryptedStorageClass", func() { Expect(refs).To(ContainElements(expRefs...)) for i := range storageClasses { - ok, err := kubeutil.IsEncryptedStorageClass( + ok, pid, err := kubeutil.IsEncryptedStorageClass( ctx, client, storageClasses[i].Name) Expect(err).ToNot(HaveOccurred()) if i%2 == 0 { Expect(ok).To(BeFalse()) + Expect(pid).To(BeEmpty()) } else { Expect(ok).To(BeTrue()) + Expect(pid).To(Equal(fakeString)) } } }) diff --git a/pkg/vmconfig/crypto/crypto_reconciler_pre.go b/pkg/vmconfig/crypto/crypto_reconciler_pre.go index 6e84b8c49..5c2cfc134 100644 --- a/pkg/vmconfig/crypto/crypto_reconciler_pre.go +++ b/pkg/vmconfig/crypto/crypto_reconciler_pre.go @@ -42,6 +42,7 @@ type reconcileArgs struct { curKey cryptoKey newKey cryptoKey isEncStorClass bool + profileID string hasVTPM bool addVTPM bool remVTPM bool @@ -113,8 +114,8 @@ func (r reconciler) Reconcile( // ID and key ID. args.curKey = getCurCryptoKey(moVM) - // Check whether or not the storage class is encrypted. - args.isEncStorClass, err = kubeutil.IsEncryptedStorageClass( + // Check whether or not the StorageClass supports encryption. + args.isEncStorClass, args.profileID, err = kubeutil.IsEncryptedStorageClass( ctx, k8sClient, vm.Spec.StorageClass) @@ -123,8 +124,8 @@ func (r reconciler) Reconcile( } if paused.ByAdmin(moVM) || paused.ByDevOps(vm) { - // Do not proceed further if the VM is paused. - return nil + // If the VM is paused, just update the status. + return updateStatus(ctx, args, false) } // Record whether the VM has, is adding, or is removing a vTPM. @@ -242,7 +243,7 @@ func (r reconciler) reconcileUpdate( // // Update the VM's status. - return updateStatus(ctx, args) + return updateStatus(ctx, args, true) } func (r reconciler) reconcileUpdateEncryptionClass( @@ -381,7 +382,10 @@ func setConditionAndReturnErr(args reconcileArgs, err error, r Reason) error { return err } -func updateStatus(ctx context.Context, args reconcileArgs) error { +func updateStatus( + ctx context.Context, + args reconcileArgs, + doValidation bool) error { if args.curKey.provider == "" { @@ -417,6 +421,10 @@ func updateStatus(ctx context.Context, args reconcileArgs) error { } } + if !doValidation { + return nil + } + return doOp(ctx, args, doUpdateEncrypted) } @@ -571,6 +579,8 @@ func onRecrypt( }, } + onRecryptDisks(args) + logger.Info( "Recrypt VM", "currentKeyID", args.curKey.id, @@ -582,6 +592,98 @@ func onRecrypt( return 0, nil, nil } +func onRecryptDisks(args reconcileArgs) { + for i := range args.moVM.Config.Hardware.Device { + baseDev := args.moVM.Config.Hardware.Device[i] + if disk, ok := baseDev.(*vimtypes.VirtualDisk); ok { + if disk.VDiskId == nil { // Skip FCDs + + switch tBack := disk.Backing.(type) { + case *vimtypes.VirtualDiskFlatVer2BackingInfo: + if tBack.KeyId != nil { + updateDiskBackingForRecrypt(args, disk) + } + case *vimtypes.VirtualDiskSeSparseBackingInfo: + if tBack.KeyId != nil { + updateDiskBackingForRecrypt(args, disk) + } + case *vimtypes.VirtualDiskSparseVer2BackingInfo: + if tBack.KeyId != nil { + updateDiskBackingForRecrypt(args, disk) + } + } + } + } + } +} + +func updateDiskBackingForRecrypt( + args reconcileArgs, + disk *vimtypes.VirtualDisk) { + + devSpec := getOrCreateDeviceChangeForDisk(args, disk) + + // Set the device change's crypto spec to be the same as the VM's. + if devSpec.Backing == nil { + devSpec.Backing = &vimtypes.VirtualDeviceConfigSpecBackingSpec{} + } + devSpec.Backing.Crypto = args.configSpec.Crypto + + // Ensure the device change has the encryption storage profile. + var hasProfile bool + for i := range devSpec.Profile { + if p, ok := devSpec.Profile[i].(*vimtypes.VirtualMachineDefinedProfileSpec); ok { + if p.ProfileId == args.profileID { + hasProfile = true + break + } + } + } + if !hasProfile { + devSpec.Profile = append( + devSpec.Profile, + &vimtypes.VirtualMachineDefinedProfileSpec{ + ProfileId: args.profileID, + }) + } +} + +// getOrCreateDeviceChangeForDisk returns the device change for the specified +// disk. If there is an existing Edit change, that is returned. If there is an +// existing Add/Remove change, nil is returned. Otherwise a new Edit change is +// returned. +func getOrCreateDeviceChangeForDisk( + args reconcileArgs, + disk *vimtypes.VirtualDisk) *vimtypes.VirtualDeviceConfigSpec { + + for i := range args.configSpec.DeviceChange { + if dc := args.configSpec.DeviceChange[i]; dc != nil { + if ds := dc.GetVirtualDeviceConfigSpec(); ds != nil { + if bd := ds.Device; bd != nil { + if vd := bd.GetVirtualDevice(); vd != nil { + if vd.Key == disk.Key { + if ds.Operation == vimtypes.VirtualDeviceConfigSpecOperationEdit { + return ds + } + return nil + } + } + } + } + } + } + + ds := &vimtypes.VirtualDeviceConfigSpec{ + Device: disk, + Operation: vimtypes.VirtualDeviceConfigSpecOperationEdit, + Backing: &vimtypes.VirtualDeviceConfigSpecBackingSpec{}, + } + + args.configSpec.DeviceChange = append(args.configSpec.DeviceChange, ds) + + return ds +} + func validateEncrypt( ctx context.Context, args reconcileArgs) (Reason, []string, error) { diff --git a/webhooks/virtualmachine/mutation/virtualmachine_mutator.go b/webhooks/virtualmachine/mutation/virtualmachine_mutator.go index 59329a370..547170d6f 100644 --- a/webhooks/virtualmachine/mutation/virtualmachine_mutator.go +++ b/webhooks/virtualmachine/mutation/virtualmachine_mutator.go @@ -577,7 +577,7 @@ func SetDefaultEncryptionClass( } // Check if the VM's storage class supports encryption. - if ok, err := kubeutil.IsEncryptedStorageClass( + if ok, _, err := kubeutil.IsEncryptedStorageClass( ctx, k8sClient, vm.Spec.StorageClass); err != nil { diff --git a/webhooks/virtualmachine/validation/virtualmachine_validator.go b/webhooks/virtualmachine/validation/virtualmachine_validator.go index 42610e34a..547cf3253 100644 --- a/webhooks/virtualmachine/validation/virtualmachine_validator.go +++ b/webhooks/virtualmachine/validation/virtualmachine_validator.go @@ -517,7 +517,7 @@ func (v validator) validateCrypto( encClassNamePath = cryptoPath.Child("encryptionClassName") ) - if ok, err := kubeutil.IsEncryptedStorageClass( + if ok, _, err := kubeutil.IsEncryptedStorageClass( ctx, v.client, vm.Spec.StorageClass); err != nil {