Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

akinomyoga
Copy link
Collaborator

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
Copy link

ghost commented Jun 14, 2022

@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
Copy link
Collaborator Author

@carbongreat13 Thanks for checking!

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

)

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

filtering_pattern=${filtering_pattern%$'\n'}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) &&

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +21 to +24
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}'
Copy link
Owner

@scop scop Jun 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Collaborator Author

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

@scop
Copy link
Owner

scop commented Jun 27, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[cryptsetup] luksErase is not completed
2 participants