Support LUKS keys for cold migrations from vSphere #676

liranr23 · 2023-12-07T16:03:55Z

Add LUKS key to the vm
This patch adds the LUKS keys to the VM specs using a secret ref. It
expected to have list of strings. Each value will use the all selector
option based on virt-v2v docs
(https://www.libguestfs.org/virt-v2v.1.html).

The secret should be provided in the destination namespace, the key
should be luks and the values should be the arguments provided to
virt-v2v, such as:

passphrase1
passphrase2
...

Fixes #567

pkg/apis/forklift/v1beta1/plan.go

ahadas · 2023-12-10T10:21:00Z

The secret should be provided in the destination namespace, the key should be luks and the values should be the arguments provided to virt-v2v, such as:
UUID:key:passphrase
UUID:clevis
DEVICE_NAME:key:passphrase
Once https://issues.redhat.com/browse/RHEL-18142 will be implemented,
it will be possible to use all:key:passphrase and ease on the user.

I see that it's already in progress, doesn't it make more sense to wait for it?

ahadas

nice, looks good overall

virt-v2v/cold/entrypoint

ahadas · 2023-12-10T10:29:40Z

virt-v2v/cold/entrypoint

+if [ -d "/etc/luks" ]; then
+    IFS=$'\n'
+    for line in $(cat /etc/luks/luks); do
+        args+=(--key "$line")


as I commented at the top-level, I think we can wait to get the all option because otherwise, users would have to specify the name or the UIID of the device that may be different for different VMs in the plan (we can find the device UID probably, not sure about OVAs though, but as the all option is just around the corner...)

since a selector of all devices will probably not be available for forklift 2.6, we decided not to wait for it
but I talked with @dankenigsberg, and he raised a vaild point that users that provide such input may expect us to support key per-devices also when that selector would be available.
@liranr23 can we save the user from specifying the device IDs, as we're supposed to have them in the inventory, and ask only for the encryption keys from users?

pkg/apis/forklift/v1beta1/plan.go

ahadas · 2023-12-10T10:57:25Z

The secret should be provided in the destination namespace, the key should be luks and the values should be the arguments provided to virt-v2v, such as:
UUID:key:passphrase
UUID:clevis
DEVICE_NAME:key:passphrase
Once https://issues.redhat.com/browse/RHEL-18142 will be implemented,
it will be possible to use all:key:passphrase and ease on the user.
I see that it's already in progress, doesn't it make more sense to wait for it?

BTW, need to check if the support for LUKS exists in el8 but if we rely on the all option then we anyway need to limit this to the flows we use virt-v2v on el9 (hopefully, all cold migration flows in 2.6)

liranr23 · 2023-12-10T14:21:57Z

The secret should be provided in the destination namespace, the key should be luks and the values should be the arguments provided to virt-v2v, such as:
UUID:key:passphrase
UUID:clevis
DEVICE_NAME:key:passphrase
Once https://issues.redhat.com/browse/RHEL-18142 will be implemented,
it will be possible to use all:key:passphrase and ease on the user.
I see that it's already in progress, doesn't it make more sense to wait for it?

yeah we can.
in this PR we give the chance for the user to choose (from all options).
there is a clear performance benefit if the user specify the device. i thought it might be better to give him the option. once we consume the change of virt-v2v, he can set all:key:passphrase. currently, i am not sure if we will have fallback to clevis, that means, we can either "force" it on the virt-v2v command and the user won't need to specify it at all or, adding all:clevis as well.
Bottom line, it's "all options[and possible better performance]" versus easy usage of the user (specifying list of keys and that's all).

liranr23 · 2023-12-10T14:22:46Z

BTW, need to check if the support for LUKS exists in el8 but if we rely on the all option then we anyway need to limit this to the flows we use virt-v2v on el9 (hopefully, all cold migration flows in 2.6)

it's not as written in the 3rd commit

ahadas · 2023-12-10T15:10:01Z

The secret should be provided in the destination namespace, the key should be luks and the values should be the arguments provided to virt-v2v, such as:
UUID:key:passphrase
UUID:clevis
DEVICE_NAME:key:passphrase
Once https://issues.redhat.com/browse/RHEL-18142 will be implemented,
it will be possible to use all:key:passphrase and ease on the user.
I see that it's already in progress, doesn't it make more sense to wait for it?
yeah we can. in this PR we give the chance for the user to choose (from all options). there is a clear performance benefit if the user specify the device. i thought it might be better to give him the option. once we consume the change of virt-v2v, he can set all:key:passphrase. currently, i am not sure if we will have fallback to clevis, that means, we can either "force" it on the virt-v2v command and the user won't need to specify it at all or, adding all:clevis as well. Bottom line, it's "all options[and possible better performance]" versus easy usage of the user (specifying list of keys and that's all).

if we like to consider the option of specifying devices, we need to see how to make it practical, especially in scale - specifying UUIDs/names is problematic, users would rather need to select disk(s) and a practical way...

ahadas · 2023-12-10T15:13:08Z

BTW, need to check if the support for LUKS exists in el8 but if we rely on the all option then we anyway need to limit this to the flows we use virt-v2v on el9 (hopefully, all cold migration flows in 2.6)

it's not as written in the 3rd commit

ok, so need to add it to the plan admitter

liranr23 · 2023-12-10T15:22:54Z

The secret should be provided in the destination namespace, the key should be luks and the values should be the arguments provided to virt-v2v, such as:
UUID:key:passphrase
UUID:clevis
DEVICE_NAME:key:passphrase
Once https://issues.redhat.com/browse/RHEL-18142 will be implemented,
it will be possible to use all:key:passphrase and ease on the user.
I see that it's already in progress, doesn't it make more sense to wait for it?
yeah we can. in this PR we give the chance for the user to choose (from all options). there is a clear performance benefit if the user specify the device. i thought it might be better to give him the option. once we consume the change of virt-v2v, he can set all:key:passphrase. currently, i am not sure if we will have fallback to clevis, that means, we can either "force" it on the virt-v2v command and the user won't need to specify it at all or, adding all:clevis as well. Bottom line, it's "all options[and possible better performance]" versus easy usage of the user (specifying list of keys and that's all).
if we like to consider the option of specifying devices, we need to see how to make it practical, especially in scale - specifying UUIDs/names is problematic, users would rather need to select disk(s) and a practical way...

OK, so we can wait for virt-v2v and change the commits + in the virt-v2v to handle it by adding all:key: prefix. Then the user will just provide keys in the secret.
We can also add --key all:clevis to support it as a fallback (in case virt-v2v won't do it).
This should be the simplest way to a user.

sonarcloud · 2023-12-10T16:51:06Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
7.1% Duplication

pkg/forklift-api/webhooks/validating-webhook/admitters/plan-admitter.go

ahadas · 2023-12-24T12:17:36Z

the --key all:<selector> for LUKS was added in virt-v2v-2.3.7-1.el9 which is not yet available on centos9-stream

pkg/apis/forklift/v1beta1/plan.go

pkg/controller/plan/kubevirt.go

ahadas · 2024-01-03T14:52:17Z

virt-v2v/cold/entrypoint

+if [ -d "/etc/luks" ]; then
+    IFS=$'\n'
+    for line in $(cat /etc/luks/luks); do
+        args+=(--key "$line")


since a selector of all devices will probably not be available for forklift 2.6, we decided not to wait for it
but I talked with @dankenigsberg, and he raised a vaild point that users that provide such input may expect us to support key per-devices also when that selector would be available.
@liranr23 can we save the user from specifying the device IDs, as we're supposed to have them in the inventory, and ask only for the encryption keys from users?

pkg/apis/forklift/v1beta1/plan.go

pkg/controller/plan/adapter/vsphere/builder.go

ahadas

to summarize an offline discussion about this - it would be better to switch back to passing the keys with a volume and ask virt-v2v to get it from the file, so that the keys won't be included in logs that are gathered from the cluster

virt-v2v/cold/entrypoint.go

pkg/controller/plan/kubevirt.go

virt-v2v/cold/entrypoint.go

ahadas

I've made some cleanup, besides the comment inside it looks good

pkg/controller/plan/kubevirt.go

This patch adds the LUKS keys to the VM specs using a secret ref. It expected to have list of strings. Each value will use the `all` selector option based on virt-v2v docs (https://www.libguestfs.org/virt-v2v.1.html). The secret should be provided in the destination namespace, the key should be `luks` and the values should be the arguments provided to virt-v2v, such as: ``` passphrase1 passphrase2 ... ``` Signed-off-by: Liran Rotenberg <[email protected]>

This patch will set the expected LUKS secret to virt-v2v pod as a volume mounted to the virt-v2v container. This will later allow us to read the secret and provide the arguments to the migration. Signed-off-by: Liran Rotenberg <[email protected]>

This patch will read the given LUKS key provided to the container and add them as arguments to the virt-v2v command. Since virt-v2v supports this feature only since 2.2, it applies only for cold migrations. It uses the `all` selector to each passphrase. Signed-off-by: Liran Rotenberg <[email protected]>

When importing from vSphere or OVA and using EL8 virt-v2v (warm migration), LUKS encryption is not supported. In case the plan is set with LUKS secret, fail to validate such plan. Signed-off-by: Liran Rotenberg <[email protected]>

Instead of the plan keep being running and the conversion pod try to init, we can query and check the secret existence for LUKS keys before. In the case it's doesn't exist, fail the migration properly. If it does exist, post it on the target namespace for the conversion pod. Signed-off-by: Liran Rotenberg <[email protected]>

Signed-off-by: Arik Hadas <[email protected]>

sonarcloud · 2024-06-09T13:04:13Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
7.2% Duplication on New Code

See analysis details on SonarCloud

liranr23 self-assigned this Dec 7, 2023

liranr23 requested review from nyoxi, mnecas and ahadas as code owners December 7, 2023 16:03

liranr23 requested review from bennyz and bkhizgiy December 7, 2023 16:06

liranr23 force-pushed the luks branch from c5545a3 to 6d85edf Compare December 7, 2023 16:59

liranr23 commented Dec 7, 2023

View reviewed changes

pkg/apis/forklift/v1beta1/plan.go Outdated Show resolved Hide resolved

liranr23 force-pushed the luks branch 3 times, most recently from 2b6d825 to a6ef6ee Compare December 10, 2023 09:06

ahadas reviewed Dec 10, 2023

View reviewed changes

liranr23 force-pushed the luks branch from a6ef6ee to 70c5caf Compare December 10, 2023 14:28

liranr23 force-pushed the luks branch from 70c5caf to fa7c28c Compare December 10, 2023 15:21

ahadas reviewed Dec 18, 2023

View reviewed changes

ahadas reviewed Jan 3, 2024

View reviewed changes

ahadas changed the title ~~Support LUKS keys when cold migrating from vsphere~~ Support LUKS keys for cold migrations from vSphere Feb 15, 2024

ahadas added this to the 2.6.1 milestone Apr 9, 2024

ahadas mentioned this pull request Apr 30, 2024

WIP: let the user choose the boot disk for multiboot #873

Closed

ahadas reviewed Apr 30, 2024

View reviewed changes

pkg/apis/forklift/v1beta1/plan.go Outdated Show resolved Hide resolved

ahadas removed this from the 2.6.1 milestone May 1, 2024

liranr23 commented May 23, 2024

View reviewed changes

pkg/controller/plan/adapter/vsphere/builder.go Outdated Show resolved Hide resolved

liranr23 force-pushed the luks branch 2 times, most recently from 8c3b642 to 1274d6e Compare May 23, 2024 12:29

ahadas reviewed May 27, 2024

View reviewed changes

liranr23 force-pushed the luks branch 2 times, most recently from e649f52 to 6afd76a Compare May 29, 2024 09:17

liranr23 commented May 29, 2024

View reviewed changes

virt-v2v/cold/entrypoint.go Outdated Show resolved Hide resolved

yaacov mentioned this pull request Jun 2, 2024

Update forklift crds, add LUKs kubev2v/forklift-console-types#6

Merged

ahadas reviewed Jun 2, 2024

View reviewed changes

liranr23 force-pushed the luks branch 3 times, most recently from d49e5e3 to 480d139 Compare June 4, 2024 13:05

yaacov mentioned this pull request Jun 5, 2024

🐾 Support LUKS keys for cold migrations from vSphere kubev2v/forklift-console-plugin#1197

Merged

ahadas force-pushed the luks branch from 30911ab to 08e937a Compare June 9, 2024 12:39

ahadas reviewed Jun 9, 2024

View reviewed changes

pkg/controller/plan/kubevirt.go Outdated Show resolved Hide resolved

liranr23 and others added 7 commits June 9, 2024 16:03

Build the LUKS secret mount

dfb0192

This patch will set the expected LUKS secret to virt-v2v pod as a volume mounted to the virt-v2v container. This will later allow us to read the secret and provide the arguments to the migration. Signed-off-by: Liran Rotenberg <[email protected]>

API: validate LUKS

db05a83

When importing from vSphere or OVA and using EL8 virt-v2v (warm migration), LUKS encryption is not supported. In case the plan is set with LUKS secret, fail to validate such plan. Signed-off-by: Liran Rotenberg <[email protected]>

v2v: hard-code command

5492823

Signed-off-by: Arik Hadas <[email protected]>

cleanup

2e538d3

Signed-off-by: Arik Hadas <[email protected]>

liranr23 force-pushed the luks branch from 08e937a to 2e538d3 Compare June 9, 2024 13:03

ahadas approved these changes Jun 9, 2024

View reviewed changes

ahadas merged commit 4b425e4 into kubev2v:main Jun 9, 2024
12 of 13 checks passed

ahadas added this to the 2.6.3 milestone Jun 9, 2024

liranr23 deleted the luks branch June 9, 2024 13:26

liranr23 mentioned this pull request Jun 9, 2024

Support LUKS keys for cold migrations from vSphere (backport) #925

Merged

ahadas removed this from the 2.6.3 milestone Jun 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support LUKS keys for cold migrations from vSphere #676

Support LUKS keys for cold migrations from vSphere #676

liranr23 commented Dec 7, 2023 •

edited

Loading

ahadas commented Dec 10, 2023 •

edited

Loading

ahadas left a comment

ahadas Dec 10, 2023 •

edited

Loading

ahadas Jan 3, 2024 •

edited

Loading

ahadas commented Dec 10, 2023 •

edited

Loading

liranr23 commented Dec 10, 2023

liranr23 commented Dec 10, 2023 •

edited

Loading

ahadas commented Dec 10, 2023 •

edited

Loading

ahadas commented Dec 10, 2023

liranr23 commented Dec 10, 2023 •

edited

Loading

sonarcloud bot commented Dec 10, 2023

ahadas commented Dec 24, 2023

ahadas Jan 3, 2024 •

edited

Loading

ahadas left a comment

ahadas left a comment

sonarcloud bot commented Jun 9, 2024

Support LUKS keys for cold migrations from vSphere #676

Support LUKS keys for cold migrations from vSphere #676

Conversation

liranr23 commented Dec 7, 2023 • edited Loading

ahadas commented Dec 10, 2023 • edited Loading

ahadas left a comment

Choose a reason for hiding this comment

ahadas Dec 10, 2023 • edited Loading

Choose a reason for hiding this comment

ahadas Jan 3, 2024 • edited Loading

Choose a reason for hiding this comment

ahadas commented Dec 10, 2023 • edited Loading

liranr23 commented Dec 10, 2023

liranr23 commented Dec 10, 2023 • edited Loading

ahadas commented Dec 10, 2023 • edited Loading

ahadas commented Dec 10, 2023

liranr23 commented Dec 10, 2023 • edited Loading

sonarcloud bot commented Dec 10, 2023

ahadas commented Dec 24, 2023

ahadas Jan 3, 2024 • edited Loading

Choose a reason for hiding this comment

ahadas left a comment

Choose a reason for hiding this comment

ahadas left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Jun 9, 2024

Quality Gate passed

liranr23 commented Dec 7, 2023 •

edited

Loading

ahadas commented Dec 10, 2023 •

edited

Loading

ahadas Dec 10, 2023 •

edited

Loading

ahadas Jan 3, 2024 •

edited

Loading

ahadas commented Dec 10, 2023 •

edited

Loading

liranr23 commented Dec 10, 2023 •

edited

Loading

ahadas commented Dec 10, 2023 •

edited

Loading

liranr23 commented Dec 10, 2023 •

edited

Loading

ahadas Jan 3, 2024 •

edited

Loading