Add ability to use a LUKS keys vault #65

ghost · 2019-11-07T11:54:56Z

For customers that have LUKS encrypted devices, we need a way to pass the LUKS keys to virt-v2v. This PR implements it by allowing a luks_keys_vault key in the JSON input. Currently, the implementation is really basic and only accept a clear JSON file as vault. The expected JSON file structure is:

{
    "my_vm": [
        {
            "device": "/dev/sda1",
            "key": "0123456789abcdef"
        },
        {
            "device": "/dev/sda2",
            "key": "fedcba9876543210"
        }
    ]
}

With this format, we can have many virtual machines declared into a single file.
To reduce coupling with ManageIQ, there's a default path for the vault file, so that it only has to exist to be leveraged by virt-v2v-wrapper. In the future, ManageIQ may generate the vault file(s) and override that default value.

ovirt-infra · 2019-11-07T11:54:58Z

Hello contributor, thanks for submitting a PR for this project!

I am the bot who triggers "standard-CI" builds for this project.
As a security measure, I will not run automated tests on PRs that are not from white-listed contributors.

In order to allow automated tests to run, please ask one of the project maintainers to review the code and then do one of the following:

Type ci test please on this PR to trigger automated tests for it.
Type ci add to whitelist on this PR to trigger automated tests for it and also add you to the contributor white-list so that your future PRs will be tested automatically. ( keep in mind this list might be overwritten if the job XML is refreshed, for permanent whitelisting, please follow Added support for network mappings #3 option )
If you are planning to contribute to more than one project, maybe it's better to ask them to add you to the project organization, so you'll be able to run tests for all the organization's projects.

nyoxi · 2019-11-08T12:25:17Z

wrapper/tests/test_v2v_args.py

+                'device': '/dev/sda2',
+                'filename': '/tmp/luks/sda2',
+            }
+        ]


Create a new dictionary with the LUKS specific lines. I.e. copy VDDK_RHV and extend it with the above lines.

Should it still be the case, now that I moved it to wrapper/tests/test_v2v_args.py ?

Please pick this commit 745fe83. It will fix the tests.

nyoxi · 2019-11-08T12:28:46Z

wrapper/hosts.py

+                v2v_args.extend([
+                    '--key',
+                    '%s:file:%s' % (luks_key['device'], luks_key['filename'])
+                ])


These changes should go to prepare_command() in virt_v2v_wrapper.py instead of into every host class.

nyoxi · 2019-11-08T12:29:52Z

wrapper/tests/test_v2v_args.py

@@ -62,6 +73,8 @@ def test_vddk_basic(self):
            '-io', 'vddk-libdir=/opt/vmware-vix-disklib-distrib',
            '-io', 'vddk-thumbprint=01:23:45:67:89:AB:CD:EA:DB:EE:F0:12:34:56:78:9A:BC:DE:F0:12',  # NOQA E501
            '--password-file', '/vmware/password',
+            '--key', '/dev/sda1:file:/tmp/luks/sda1',
+            '--key', '/dev/sda2:file:/tmp/luks/sda2',


create new test function for the LUKS related changes instead of reusing test_vddk_basic()

nyoxi · 2019-11-08T12:50:18Z

wrapper/virt_v2v_wrapper.py

@@ -487,6 +487,21 @@ def main():
                                                  host.get_uid(),
                                                  host.get_gid())

+        if 'luks_keys_vault' not in data:
+            data['luks_keys_vault'] = '/tmp/v2v_luks_keys_vault.json'


Do we need a default? If yes, it may be worth using some other directory then /tmp, e.g. /etc or user's home directory. What do you think? Also what do you think about adding a check and refusing to load file that is readable/writable by someone else than owner?

Good points. Changed to use user's home directory and added checks on permissions.

ghost · 2019-11-08T21:06:47Z

@nyoxi the tests fail, but I don't know what's causing the errors. Could you look at it, please ?

nyoxi · 2019-11-13T16:11:53Z

wrapper/hosts.py

@@ -748,7 +748,6 @@ def prepare_command(self, data, v2v_args, v2v_env, v2v_caps):
                v2v_args.extend(['-oo', 'rhv-verifypeer=%s' %
                                ('false' if data['insecure_connection'] else
                                 'true')])
-


some leftover here

not sure I understand. this line is already deleted.

nyoxi · 2019-11-13T16:12:51Z

wrapper/tests/test_v2v_args.py

+                'device': '/dev/sda2',
+                'filename': '/tmp/luks/sda2',
+            }
+        ]


Please pick this commit 745fe83. It will fix the tests.

nyoxi · 2019-11-13T16:14:45Z

wrapper/virt_v2v_wrapper.py

+            if file_stat.st_uid != host.get_uid():
+                hard_error('LUKS keys vault does\'nt belong to'
+                           'user running virt-v2v-wrapper')
+            if not bool(file_stat.st_mode & stat.S_IRWXO):


This cast is ugly. I would prefer if file_stat.st_mode & stat.S_IRWXO > 0:

nyoxi · 2019-11-13T16:15:00Z

wrapper/virt_v2v_wrapper.py

+                           'user running virt-v2v-wrapper')
+            if not bool(file_stat.st_mode & stat.S_IRWXO):
+                hard_error('LUKS keys vault is accessible to others')
+            if not bool(file_stat.st_mode & stat.S_IRWXG):


same as above

nyoxi · 2019-11-13T21:14:27Z

CI failure is unrelated

nyoxi · 2019-11-13T21:14:49Z

ci test please

fabiendupont added 2 commits November 7, 2019 06:36

Add ability to use a LUKS keys vault

e2f430e

Add a default path for LUKS keys vault

bf63680

ghost requested a review from nyoxi November 7, 2019 11:54

fabiendupont added 2 commits November 7, 2019 07:02

Updating test with valid 'luks_keys_files' entry

4188d27

Add the feature to VDSMHost

f66d2be

nyoxi requested changes Nov 8, 2019

View reviewed changes

fabiendupont added 3 commits November 8, 2019 08:39

Update tests for RHV to use LUKS keys vault

9345bfc

Move code to virt-v2v-wrapper.py. Add check on vault permissions

b883c53

Add missing module

302c32b

nyoxi requested changes Nov 13, 2019

View reviewed changes

Fixes

5ea1fd7

nyoxi merged commit 86ba00e into oVirt:master Nov 13, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ability to use a LUKS keys vault #65

Add ability to use a LUKS keys vault #65

ghost commented Nov 7, 2019

ovirt-infra commented Nov 7, 2019

nyoxi Nov 8, 2019

ghost Nov 8, 2019

nyoxi Nov 13, 2019

nyoxi Nov 8, 2019

ghost Nov 8, 2019

nyoxi Nov 8, 2019

nyoxi Nov 8, 2019

ghost Nov 8, 2019

ghost commented Nov 8, 2019

nyoxi Nov 13, 2019

ghost Nov 13, 2019

nyoxi Nov 13, 2019

nyoxi Nov 13, 2019

ghost Nov 13, 2019

nyoxi Nov 13, 2019

ghost Nov 13, 2019

nyoxi commented Nov 13, 2019

nyoxi commented Nov 13, 2019

Add ability to use a LUKS keys vault #65

Add ability to use a LUKS keys vault #65

Conversation

ghost commented Nov 7, 2019

ovirt-infra commented Nov 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Nov 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nyoxi commented Nov 13, 2019

nyoxi commented Nov 13, 2019