fix(cryptsetup): update the list of actions #759

akinomyoga · 2022-06-14T09:07:02Z

Fixes #758

@carbongreat13 Thank you for the report (#758). Can you test the behavior of the updated compeltions/cryptsetup? You can download and source it as

$ curl -Lo cryptsetup https://github.com/akinomyoga/bash-completion/raw/cryptsetup-action/completions/cryptsetup
$ source cryptsetup
$ cruptsetup luksE[Tab]

ghost · 2022-06-14T09:21:27Z

@carbongreat13 Thank you for the report (#758). Can you test the behavior of the updated compeltions/cryptsetup? You can download and source it as

I have tested the I was able to confirm that it works correctly.

akinomyoga · 2022-06-14T09:24:26Z

@carbongreat13 Thanks for checking!

scop · 2022-06-17T20:53:12Z

completions/cryptsetup

-                repair erase luksFormat luksAddKey luksRemoveKey luksChangeKey
-                luksKillSlot luksUUID isLuks luksDump tcryptDump luksSuspend
-                luksResume luksHeaderBackup luksHeaderRestore' -- "$cur"))
+            COMPREPLY=($(compgen -W 'benchmark bitlkClose bitlkDump bitlkOpen


If we want to keep hardcoding the list, could we add some pointers here to the source of the list, as well as how to keep the list sorted for easy maintenance? (I believe it before this change the entries were roughly in the order the man page has them, which wasn't noted either, though, but we're here to improve on things ;))

But actually, I think we should consider switching to scraping --help output instead of hardcoding. For example on Ubuntu 20.04, it contains

<action> is one of: open <device> [--type <type>] [<name>] - open device as <name> close <name> - close device (remove mapping) resize <name> - resize active device status <name> - show device status benchmark [--cipher <cipher>] - benchmark cipher repair <device> - try to repair on-disk metadata reencrypt <device> - reencrypt LUKS2 device erase <device> - erase all keyslots (remove encryption key) convert <device> - convert LUKS from/to LUKS2 format config <device> - set permanent configuration options for LUKS2 luksFormat <device> [<new key file>] - formats a LUKS device luksAddKey <device> [<new key file>] - add key to LUKS device luksRemoveKey <device> [<key file>] - removes supplied key or key file from LUKS device luksChangeKey <device> [<key file>] - changes supplied key or key file of LUKS device luksConvertKey <device> [<key file>] - converts a key to new pbkdf parameters luksKillSlot <device> <key slot> - wipes key with number <key slot> from LUKS device luksUUID <device> - print UUID of LUKS device isLuks <device> - tests <device> for LUKS partition header luksDump <device> - dump LUKS partition information tcryptDump <device> - dump TCRYPT device information luksSuspend <device> - Suspend LUKS device and wipe key (all IOs are frozen) luksResume <device> - Resume suspended LUKS device luksHeaderBackup <device> - Backup LUKS device header and keyslots luksHeaderRestore <device> - Restore LUKS device header and keyslots token <add|remove|import|export> <device> - Manipulate LUKS2 tokens You can also use old <action> syntax aliases: open: create (plainOpen), luksOpen, loopaesOpen, tcryptOpen close: remove (plainClose), luksClose, loopaesClose, tcryptClose

CentOS 7's cryptsetup --help emits similar looking sections, so I suppose this would be feasible to do in at least somewhat portable manner.

Thank you for the comment! I have updated the PR to include the analysis of --help (790e524). What is non-trivial is that not all the actions are found in the output of --help. For example, for cryptsetup 2.4.3 in Fedora 36, --help does not contain the reported luksErase (#758) while man contains the description of luksErase. I have decided to also analyze the man output.

For example, for cryptsetup 2.4.3 in Fedora 36, --help does not contain the reported luksErase

Perhaps upstream would appreciate a bug report about this?

scop · 2022-06-20T05:14:32Z

completions/cryptsetup

+    )
+
+    if [[ ! $actions ]]; then
+        # The fallback action list is extracted from the following source:


I think we should drop the fallback altogether, unless we know there is an old version we want to support but cannot because its output is not feasible to parse. If there is, we should document what exactly it is.

I was just so lazy that I didn't want to check the behavior of old versions. Do we need to check the behavior of old versions of cryptsetup?

That's kind of my point -- if we do check old versions and want to support them and can't find a way we like to make that happen otherwise, then I'm fine with a hardcoded list and the strings + grep approach. My opinion is that if we go to such lengths, we should document the reasoning and affected versions why we are doing it, why it is important here etc.

Without verifying from the sources, I believe our current standard is not to use fallback lists at all (we do have them in a few places, but it's not a standard), and the strings+grep verification is completely new, so I'm just validating if there's something exceptionally important here that warrants that stuff.

scop · 2022-06-20T05:17:12Z

completions/cryptsetup

+            filtering_pattern=${filtering_pattern%$'\n'}
+
+            local filtered_actions
+            filtered_actions=$(strings "$path" | command grep -Fx "$filtering_pattern" | sort -u) &&


If we decide to keep this block or to extract this as a helper to another PR, I suppose this would be in order:

Suggested change

filtered_actions=$(strings "$path" | command grep -Fx "$filtering_pattern" | sort -u) &&

filtered_actions=$(strings "$path" 2>/dev/null | command grep -Fx "$filtering_pattern" | sort -u) &&

I'm not sure if the fallback in this function finally gets into the repository, but such a function would be useful anyway where hardcoded lists are needed. I'll later consider it.

scop · 2022-06-20T05:19:46Z

completions/cryptsetup

+            open plainClose plainOpen reencrypt refresh remove repair resize
+            status tcryptClose tcryptDump tcryptOpen token'
+
+        # We attempt to filter the supported actions by the strings in the binary.


If we decide to keep the fallback, I suggest we don't do this. It's not something we do elsewhere, and frankly I think it's too much for a corner case here.

Then again, if there is a bunch of other cases we feel we should do this in other completions, then a separate helper function for doing it would be in order, in another PR. Not quite sure myself, but open to opinions.

scop · 2022-06-20T05:30:02Z

completions/cryptsetup

+            LC_ALL=C "$cmd" --help 2>&1 |
+                sed -n '/^<action> is one of:/,/^[^[:space:]]/s/^[[:space:]]\{1,\}\([^[:space:]]\{1,\}\).*/\1/p'
+            LC_ALL=C man cryptsetup 2>&1 |
+                awk '/^[[:space:]]+[[:alnum:]_]+([[:space:]]+(-[^[:space:].]+|<[^<>]+>|\[[^][]+\]|or))*$/ {print $1}'


I think we should add test cases for the parsing part here like we have for dnssec-keygen and xrandr. The test cases should verify that an exact set of things are extracted to make sure we don't get false positives.

Actually, with this extraction, there is no guarantee that we do not hit false positives. I just tested it with the version with v2.4.3 in Fedora 36. If we care about the false positives, maybe we should filter out them using strings for the result as well as used in the fallback.

akinomyoga · 2022-06-26T00:05:38Z

Sorry for the delay. I'm a bit busy this week, so it may delay further.

scop · 2022-06-27T14:56:47Z

Sorry for the delay. I'm a bit busy this week, so it may delay further.

No problem, take your time, thanks for letting me know.

akinomyoga force-pushed the cryptsetup-action branch from cd81c3b to c5cb01a Compare June 14, 2022 09:30

scop requested changes Jun 17, 2022

View reviewed changes

akinomyoga force-pushed the cryptsetup-action branch from 790e524 to 9dd9fb5 Compare June 17, 2022 23:49

scop requested changes Jun 20, 2022

View reviewed changes

akinomyoga force-pushed the cryptsetup-action branch from 3c046dd to 64125a5 Compare August 28, 2022 02:15

akinomyoga marked this pull request as draft August 28, 2022 02:37

akinomyoga mentioned this pull request Sep 2, 2022

chore(.gitignore): add more autotool files #792

Merged

akinomyoga force-pushed the cryptsetup-action branch from 64125a5 to 4b4e0ea Compare December 17, 2022 19:48

akinomyoga force-pushed the cryptsetup-action branch from 4b4e0ea to 6e7be1e Compare April 5, 2023 19:48

akinomyoga force-pushed the cryptsetup-action branch from 6e7be1e to 6666600 Compare April 16, 2023 20:19

akinomyoga force-pushed the cryptsetup-action branch from 6666600 to 5073137 Compare May 1, 2023 14:44

akinomyoga force-pushed the cryptsetup-action branch from 5073137 to 78dd5b0 Compare August 6, 2023 19:49

akinomyoga force-pushed the cryptsetup-action branch from 78dd5b0 to 6e6f09b Compare December 24, 2023 15:59

akinomyoga added 4 commits May 13, 2024 13:12

fix(cryptsetup): update the list of actions

2a527ca

fix(cryptsetup): parse --help and man for actions

deddbf5

test(cryptsetup): add a skip condition

830a868

test(cryptsetup): update the skip condition

1c5be70

akinomyoga force-pushed the cryptsetup-action branch from 6e6f09b to 1c5be70 Compare May 13, 2024 04:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(cryptsetup): update the list of actions #759

fix(cryptsetup): update the list of actions #759

akinomyoga commented Jun 14, 2022

ghost commented Jun 14, 2022

akinomyoga commented Jun 14, 2022

scop Jun 17, 2022

akinomyoga Jun 17, 2022

scop Jun 20, 2022

scop Jun 20, 2022

akinomyoga Jun 26, 2022

scop Jun 27, 2022

scop Jun 20, 2022

akinomyoga Jun 26, 2022

scop Jun 20, 2022

scop Jun 20, 2022 •

edited

Loading

akinomyoga Jun 26, 2022

akinomyoga commented Jun 26, 2022

scop commented Jun 27, 2022

	filtered_actions=$(strings "$path" \| command grep -Fx "$filtering_pattern" \| sort -u) &&
	filtered_actions=$(strings "$path" 2>/dev/null \| command grep -Fx "$filtering_pattern" \| sort -u) &&

fix(cryptsetup): update the list of actions #759

Are you sure you want to change the base?

fix(cryptsetup): update the list of actions #759

Conversation

akinomyoga commented Jun 14, 2022

ghost commented Jun 14, 2022

akinomyoga commented Jun 14, 2022

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

Choose a reason for hiding this comment

scop Jun 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akinomyoga commented Jun 26, 2022

scop commented Jun 27, 2022

scop Jun 20, 2022 •

edited

Loading