Repository: ansible/ansible
Pull request: luks_device.py: allows user explicity define luks format version
##### SUMMARY
 Allow user pass an option 'type' that explicits define the version of LUKS container.

- if type is defined and label too, label takes the preference (LUKS2 will be used)
- if type is defined and label not, luks format will use the preference of user.
- if type and label is not defined it will be used the default option of cryptsetup binary.

Fixes #58973

##### ISSUE TYPE
- Feature Pull Request

##### COMPONENT NAME
lib/ansible/modules/crypto/luks_device.py

##### ADDITIONAL INFORMATION
Reproduce with the playbook bellow, change the commentary of label and type as you wish.

```
---
- name: test issue 58973
  gather_facts: no
  hosts: localhost
  remote_user: root
  tasks:
    - name: create LUKS container
      luks_device:
        device: "/dev/loop0"
        state: "present"
        keyfile: "/root/password"
        #label: personalLabelName
        type: luks1
```

Problem statement:
Support creating LUKS2 container in luks_device module
<!--- Verify first that your feature was not already discussed on GitHub -->
<!--- Complete *all* sections as described, this form is processed automatically -->

##### SUMMARY
<!--- Describe the new feature/improvement briefly below -->
Ansible by default only creates LUKS1 format devices. Considering LUKS2 extra features and age, I think it would be beneficial to allow users to create LUKS2 devices.

##### ISSUE TYPE
- Feature Idea

##### COMPONENT NAME
luks_device

##### ADDITIONAL INFORMATION
<!--- Describe how the feature would be used, why it is needed and what it would solve -->
For reliability, one should use either disk UUID's or labels. UUID's are (in my opinion) a hassle when it comes to using them in Ansible.

Enter labels. Labels can be the same across multiple hosts, and will be tied to the right disk/partition, even if sda1 suddenly becomes sdb1 after some hardware reconfiguration.

Now, the LUKS1 format, which Ansible creates by default with the `luks_device` module, does not support labels... Support was added with LUKS2.

It should be noted that, from what I can see, the module *does already support removing keys from LUKS2 format devices*.

<!--- Paste example playbooks or commands between quotes below -->
```yaml
- name: create LUKS2 container
  luks_device:
    device: "/dev/loop0"
    state: "present"
    keyfile: "/vault/keyfile"
    format: luks2
```

Hints from issue discussion:
Files identified in the description:
* [`lib/ansible/modules/crypto/luks_device.py`](https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/crypto/luks_device.py)

If these files are inaccurate, please update the `component name` section of the description or use the `!component` bot command.

[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: components_banner --->
cc @MarkusTeufelberger @Shaps @Spredzy @Xyon @felixfontein @japokorn @puiterwijk @resmo
[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: notify --->
It should be noted that - to actually make use of the use-case I mentioned - this requires a systemd version of 239 or newer, because of https://github.com/systemd/systemd/pull/8998
needs_contributor
Reopening since this is not completely implemented (you can get LUKS2 containers by specifying a label, but you can't get one without a label).
Interesting development :)

Progress is progress!
Hmmm. What are you thinking to close this issue permanently? Add an option
"type" and make LUKS use '--type luks2' ? If yes, I can make that too. :-)
@mulatinho Probably an option `type` with choices `luks1`, `luks2` and no default (default is `luks2` is `label` is specified, and nothing passed to `cryptsetup` otherwise) should do the job. Feel free to work on this :)
cool @felixfontein , I will work on this :D

Commit message: added the changelog fragment

Signed-off-by: Alexandre Mulatinho <alex@mulatinho.net>

Patch to review:
diff --git a/changelogs/fragments/58973-luks_device_add-type-option.yml b/changelogs/fragments/58973-luks_device_add-type-option.yml
new file mode 100644
index 00000000000000..f12c35971e6776
--- /dev/null
+++ b/changelogs/fragments/58973-luks_device_add-type-option.yml
@@ -0,0 +1,2 @@
+minor_changes:
+- luks_device - added the `type` option that allows user explicit define the LUKS container format version (can be `luks1` or `luks2`)
diff --git a/lib/ansible/modules/crypto/luks_device.py b/lib/ansible/modules/crypto/luks_device.py
index f3f74b45d1df86..82a23f479bb44b 100644
--- a/lib/ansible/modules/crypto/luks_device.py
+++ b/lib/ansible/modules/crypto/luks_device.py
@@ -115,6 +115,16 @@
             - "Will only be used when I(device) and I(label) are not specified."
         type: str
         version_added: "2.10"
+    type:
+        description:
+            - "This option allow the user explicit define the format of LUKS
+              container that wants to work with. Options are 'luks1' or 'luks2'"
+            - "Will use per default 'luks2' if I(label) is specified, and the
+              option defined by user if I(label) is not specified."
+        type: str
+        choices: [luks1, luks2]
+        version_added: "2.10"
+
 
 
 requirements:
@@ -195,6 +205,13 @@
     uuid: 03ecd578-fad4-4e6c-9348-842e3e8fa340
     state: "closed"
     name: "mycrypt"
+
+- name: create a container using luks2 format
+  luks_device:
+    device: "/dev/loop0"
+    state: "present"
+    keyfile: "/vault/keyfile"
+    type: luks2
 '''
 
 RETURN = '''
@@ -317,16 +334,21 @@ def is_luks(self, device):
 
     def run_luks_create(self, device, keyfile, keysize):
         # create a new luks container; use batch mode to auto confirm
-        label = self._module.params.get('label')
+        type = self._module.params['type']
+        label = self._module.params['label']
+
         options = []
         if keysize is not None:
             options.append('--key-size=' + str(keysize))
         if label is not None:
-            # create luks container v2 with label
             options.extend(['--type', 'luks2', '--label', label])
+        elif type is not None:
+            options.extend(['--type', type])
+
         args = [self._cryptsetup_bin, 'luksFormat']
         args.extend(options)
         args.extend(['-q', device, keyfile])
+
         result = self._run_command(args)
         if result[RETURN_CODE] != 0:
             raise ValueError('Error while creating LUKS on %s: %s'
@@ -541,6 +563,7 @@ def run_module():
         keysize=dict(type='int'),
         label=dict(type='str'),
         uuid=dict(type='str'),
+        type=dict(type='str', choices=['luks1', 'luks2']),
     )
 
     # seed the result dict in the object


Code review comments:
diff_hunk: @@ -115,6 +115,16 @@
             - "Will only be used when I(device) and I(label) are not specified."
         type: str
         version_added: "2.10"
+    type:
+        description:
+            - "This option allow the user explicit define the format of LUKS
+              container that wants to work with. Options are 'luks1' or 'luks2'"
+            - "Will use per default 'luks2' if I(label) is specified, and the
path: lib/ansible/modules/crypto/luks_device.py
text: @user1:
I think it is a lot better if the module does not allow `type=luks1` to be specified when `label` is specified. I would remove this part from the documentation, and add something like `This cannot be specified if I(type) is set to C(luks1).` to the `label` documentation, and add some code which errors out when `type=luks1` is combined with `label`.

@author:
You are right, done.

diff_hunk: @@ -317,16 +334,21 @@ def is_luks(self, device):
 
     def run_luks_create(self, device, keyfile, keysize):
         # create a new luks container; use batch mode to auto confirm
-        label = self._module.params.get('label')
+        type = self._module.params['type']
path: lib/ansible/modules/crypto/luks_device.py
text: @user1:
`type` is a reserved identifier in Python. You can use it, but it is not good style. Better use `luks_type` or something for the variable name. (Using `type` for the option name is totally fine.)

@author:
Done

diff_hunk: @@ -317,16 +334,21 @@ def is_luks(self, device):
 
     def run_luks_create(self, device, keyfile, keysize):
         # create a new luks container; use batch mode to auto confirm
-        label = self._module.params.get('label')
+        type = self._module.params['type']
+        label = self._module.params['label']
+
         options = []
         if keysize is not None:
             options.append('--key-size=' + str(keysize))
         if label is not None:
-            # create luks container v2 with label
             options.extend(['--type', 'luks2', '--label', label])
path: lib/ansible/modules/crypto/luks_device.py
text: @user1:
```suggestion
            options.extend(['--label', label])
            type = 'luks2'
```

@author:
cool

diff_hunk: @@ -317,16 +334,21 @@ def is_luks(self, device):
 
     def run_luks_create(self, device, keyfile, keysize):
         # create a new luks container; use batch mode to auto confirm
-        label = self._module.params.get('label')
+        type = self._module.params['type']
+        label = self._module.params['label']
+
         options = []
         if keysize is not None:
             options.append('--key-size=' + str(keysize))
         if label is not None:
-            # create luks container v2 with label
             options.extend(['--type', 'luks2', '--label', label])
+        elif type is not None:
path: lib/ansible/modules/crypto/luks_device.py
text: @user1:
```suggestion
        if type is not None:
```

@author:
done