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

luks_device: add built-in signature wiper to work around older wipefs versions with LUKS2 containers #327

Merged
merged 8 commits into from
Nov 11, 2021

Conversation

felixfontein
Copy link
Contributor

@felixfontein felixfontein commented Nov 7, 2021

SUMMARY

Fixes #326.

In the end I decided to keep wipefs, in case cryptsetup erase fails for some reason.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

luks_device

@felixfontein
Copy link
Contributor Author

Hmm, for whatever reason, this doesn't seem to suffice :/

@felixfontein felixfontein changed the title Use 'cryptsetup erase' to kill LUKS signature [WIP] Use 'cryptsetup erase' to kill LUKS signature Nov 7, 2021
@felixfontein
Copy link
Contributor Author

On my machine, wipefs wipes two 6-byte blocks, one at offset 0 and one at offset 16384. On the RHEL 7.9 VM, it only wipes a 6-byte block at offset 0. If I use dd to wipe the other block at 16384, isLuks stops returning 0.

Preparation:

dd if=/dev/zero of=/tmp/t bs=1M count=32
losetup -f /tmp/t
DEVICE=$(losetup -j /tmp/t --output name | head -2 | tail -1)

Reproducer:

echo hunter1324 | /sbin/cryptsetup luksFormat --type luks2 --iter-time 100 --pbkdf argon2i --pbkdf-memory 1000 --pbkdf-parallel 1 --sector-size 1024 -q ${DEVICE}
/sbin/cryptsetup isLuks ${DEVICE} ; echo $?  # expected and got: 0
/sbin/cryptsetup erase ${DEVICE} -q
wipefs -a ${DEVICE}
/sbin/cryptsetup isLuks ${DEVICE} ; echo $?  # expected 1, but got 0!
dd if=/dev/zero of=${DEVICE} count=6 bs=1 seek=0
dd if=/dev/zero of=${DEVICE} count=6 bs=1 seek=16384
/sbin/cryptsetup isLuks ${DEVICE} ; echo $?  # expected got

On my machine, the middle echo gives 1 (as expected), but on RHEL 7.9, it gives 0.

Interestingly, the RHEL 7.9 tests with ansible-core 2.11 seem to work since the order in which the tests are run is different. I guess the different order is because in one case (2.11), the controller runs on RHEL 7.9 as well (with Python 2), while in the other case (devel), the controller runs in a docker container (with Python 3). If I try this directly on the RHEL 7.9 VM used by ansible-core 2.11, it shows the exact same behvaior with the reproducer from above (i.e. wipefs doesn't properly work).

Another interesting thing: if I put another /sbin/cryptsetup isLuks ${DEVICE} ; echo $? between the dds, it doesn't work. The dds have to be run right after each other to properly work. That's ... really strange IMO.

Anyway, the result isn't that surprising; the LUKS header is version 2, and wipefs is part of util-linux, RHEL 7.9 has util-linux 2.23.2, and support for LUKS2 has only been added in util-linux 2.32, and a bug ("Check for a secondary LUKS2 header.") was fixed in util-linux 2.33.

(If you wonder where 16384 = 0x4000 comes from: https://github.com/util-linux/util-linux/blob/master/libblkid/src/superblocks/luks.c#L36-L38)

@felixfontein
Copy link
Contributor Author

(I find it somehow more worrying that cryptsetup erase / cryptsetup luksErase doesn't work either.)

@felixfontein
Copy link
Contributor Author

The easiest way to 'fix' this might be to simply implement a wiping scheme ourselves (https://gitlab.com/cryptsetup/LUKS2-docs/-/blob/master/luks2_doc_wip.pdf shows how this should work). Or to simply avoid LUKS2 tests on RHEL 7.9. Though that doesn't really fix that state=absent then won't work out of the box on RHEL 7.9.

@felixfontein
Copy link
Contributor Author

(We can potentially drop the wipefs dependency with that own code. I guess for now it's better to use both though.)

@felixfontein felixfontein changed the title [WIP] Use 'cryptsetup erase' to kill LUKS signature Use 'cryptsetup erase' to kill LUKS signature Nov 9, 2021
@felixfontein
Copy link
Contributor Author

ready_for_review

I'm wondering whether I should remove cryptsetup erase again, since it's not really needed.

Copy link
Collaborator

@Ajpantuso Ajpantuso left a comment

Choose a reason for hiding this comment

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

LGTM, not sure I know enough about LUKS to make recommendations, but I would remove cryptsetup if it's redundant and inconsistent across platforms.

@felixfontein felixfontein changed the title Use 'cryptsetup erase' to kill LUKS signature luks_device: add built-in signature wiper to work around older wipefs versions with LUKS2 containers Nov 10, 2021
@felixfontein
Copy link
Contributor Author

I've removed the cryptsetup erase part. So basically how we just have an additional wiper for the LUKS signatures.

Copy link
Collaborator

@Ajpantuso Ajpantuso left a comment

Choose a reason for hiding this comment

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

LGTM

@felixfontein felixfontein merged commit ebbfd7c into ansible-collections:main Nov 11, 2021
@felixfontein felixfontein deleted the fix-luks branch November 11, 2021 05:59
@patchback
Copy link

patchback bot commented Nov 11, 2021

Backport to stable-1: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-1/ebbfd7c56f99f626098a3b137763c9cbcee8a769/pr-327

Backported as #330

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

@felixfontein
Copy link
Contributor Author

@Ajpantuso thanks a lot for reviewing this!

patchback bot pushed a commit that referenced this pull request Nov 11, 2021
… versions with LUKS2 containers (#327)

* Use 'cryptsetup erase' to kill LUKS signature.

* Adjust unit test.

* Use own wiper for LUKS headers.

* Add comments.

* Fix tests.

* Update changelog.

* Remove 'cryptsetup erase'.

* Improve error messages.

(cherry picked from commit ebbfd7c)
felixfontein added a commit that referenced this pull request Nov 11, 2021
… versions with LUKS2 containers (#327) (#330)

* Use 'cryptsetup erase' to kill LUKS signature.

* Adjust unit test.

* Use own wiper for LUKS headers.

* Add comments.

* Fix tests.

* Update changelog.

* Remove 'cryptsetup erase'.

* Improve error messages.

(cherry picked from commit ebbfd7c)

Co-authored-by: Felix Fontein <felix@fontein.de>
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.

luks_device state=absent no longer works on some RHEL 7.9 VMs in CI
2 participants