-
Notifications
You must be signed in to change notification settings - Fork 148
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
Add LUKS support via Clevis TPM 2 token #1200
Conversation
Thank you for contributing to the Leapp project!Please note that every PR needs to comply with the Leapp Guidelines and must pass all tests in order to be mergeable.
Packit will automatically schedule regression tests for this PR's build and latest upstream leapp build. If you need a different version of leapp, e.g. from PR#42, use It is possible to schedule specific on-demand tests as well. Currently 2 test sets are supported,
See other labels for particular jobs defined in the Please open ticket in case you experience technical problem with the CI. (RH internal only) Note: In case there are problems with tests not being triggered automatically on new PR/commit or pending for a long time, please contact leapp-infra. |
9fa57ae
to
f8346e4
Compare
@danzatt Hi Dan \o most likely I will get to the review during early May or later June. We are dealing now with additional stuff. |
/packit test |
@danzatt I haven't went through the whole code yet, but covered most of it. I found some things that could be changed, and some that needs to be changed. I do not expect I will find anything else in the rest of the code (and not sure when I will get to it), but i am letting you know about that in advance, in case you would like to wait for the full review. |
d7693a8
to
ff4adae
Compare
Hello @pirat89 thanks for the review! I've hopefully addressed all your remarks now. |
repos/system_upgrade/common/actors/inhibitwhenluks/libraries/inhibitwhenluks.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's mostly ok. Just some minor changes should be done. Let's sync during the day to discuss it.
repos/system_upgrade/common/actors/inhibitwhenluks/libraries/inhibitwhenluks.py
Outdated
Show resolved
Hide resolved
repos/system_upgrade/common/actors/inhibitwhenluks/libraries/inhibitwhenluks.py
Outdated
Show resolved
Hide resolved
repos/system_upgrade/common/actors/inhibitwhenluks/libraries/inhibitwhenluks.py
Outdated
Show resolved
Hide resolved
repos/system_upgrade/common/actors/inhibitwhenluks/libraries/inhibitwhenluks.py
Outdated
Show resolved
Hide resolved
repos/system_upgrade/common/actors/inhibitwhenluks/libraries/inhibitwhenluks.py
Outdated
Show resolved
Hide resolved
repos/system_upgrade/common/actors/inhibitwhenluks/libraries/inhibitwhenluks.py
Outdated
Show resolved
Hide resolved
repos/system_upgrade/common/actors/inhibitwhenluks/libraries/inhibitwhenluks.py
Outdated
Show resolved
Hide resolved
repos/system_upgrade/common/actors/luksscanner/libraries/luksscanner.py
Outdated
Show resolved
Hide resolved
repos/system_upgrade/common/actors/luksscanner/libraries/luksscanner.py
Outdated
Show resolved
Hide resolved
/packit copr-build |
ff4adae
to
ffcb92b
Compare
/packit copr-build |
24a3baa
to
6e3163d
Compare
6e3163d
to
93b48ca
Compare
/packit copr-build |
93b48ca
to
ffa0be6
Compare
/packit copr-build |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will wait for another co-review before merging as I helped with some parts of the code but for now I consider it ready to merge from my side. I will possibly do some additional testing before the merge too.
Modify the StorageInfo model to include path and name of the parent device. Use StorageScanner to collect this information. Morover fix lsblk test, there should be a full device path in "lsblk -pbnr" output (just names were used in the original test).
e7120a1
to
111213e
Compare
/packit copr-build |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Codewise okay, just a few non-blocking minor suggestions :).
repos/system_upgrade/common/actors/luksscanner/libraries/luksscanner.py
Outdated
Show resolved
Hide resolved
repos/system_upgrade/common/actors/inhibitwhenluks/libraries/inhibitwhenluks.py
Outdated
Show resolved
Hide resolved
repos/system_upgrade/common/actors/inhibitwhenluks/libraries/inhibitwhenluks.py
Outdated
Show resolved
Hide resolved
repos/system_upgrade/common/actors/luksscanner/libraries/luksdump_parser.py
Show resolved
Hide resolved
I went over the code and up to some details it looks good. I manually tested that the upgrade is inhibited on 8->9. Also performed a successful upgrade on a VM with TPM. |
Add LuksScanner actor that runs 'cryptsetup luksDump' for all 'crypt' from lsblk output. The output is then parsed and filled into LuksDump and LuksToken models. The LuksDump model contains information about LUKS version, device UUID, corresponding device path, name of the backing device (which contains the LUKS header) and a list of LuksToken models. LuksToken model represents a token associated with the given LUKS device. It contains token ID, IDs of associated keyslot and token type. If the token type is "clevis", we use "clevis luks list" command to determine the clevis-specific subtype and append it to the token name. E.g. if there is a "clevis" token and "clevis luks list" returns "tpm2", the token type will be "clevis-tpm2".
So far, upgrades with encrypted drives were not supported. Encrypted drives require interactively typing unlock passphrases, which is not suitable for automatic upgrades using Leapp. We add a feature, where systems with all drives configured with automatic unlock method can be upgraded. Currently, we only support drives configured with Clevis/TPM2 token, because networking is not configured during Leapp upgrade (excluding NBDE). We consume LuksDumps message to decide whether the upgrade process should be inhibited. If there is at least one LUKS2 device without Clevis TPM2 binding, we inhibit the upgrade because we cannot tell if the device is not a part of a more complex storage stack and the failure to unlock the device migt cause boot problem. Co-authored-by: Petr Stodůlka <[email protected]>
The actor nowadays does more then just inhibiting the upgrade when LUKS is detected. Let's rename it to respect current behaviour.
111213e
to
722b4d7
Compare
/packit copr-build |
/packit copr-build |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From manual testing (VM with emulated tpm2):
Adding some notes for simpler reproduce of steps I ddi:
# add cryptsetup to initramfs so the conversion from initramfs is possible
dracut -f --install cryptsetup
reboot
# after reboot, update kernel-cmdline: rd.break=initqueue
# once in emergency console (typically for minimal VM):
cryptsetup convert --type luks2 /dev/vda2
# install required rpms
dnf -y install clevis-luks clevis-dracut
# set the binding - note that possibly you will want to set also specific PCR register - in this case, using just default
clevis luks bind -yd /dev/vda2 tpm2 '{"hash":"sha256","key":"rsa"}'
dracut -f --regenerate-all NOTE: shortened URLs are not set yet; they will be updated in upcoming weeks most likely |
@danzatt 💯 🎆 it's finally over! thanks for your contribution and help!! |
tldr; Add a
LuksScanner
actor which scans all crypt devices usingcryptsetup luksDump
. Don't inhibit, when all devices are LUKS2 with clevs TPM2 token.jira: RHEL-3294
This PR introduces new shortened URLs:
/cc @pirat89