Cryptsetup toolstack version bump + reencryption cleanup (LUKSv2+Luksv1 proper support + reencryption on Q4.2 + BTRFS dul LUKS containers install) #1541
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
8048f06
to
4cb56d4
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
e110960
to
302452d
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
9d0458b
to
63ad6f9
Compare
20e884d
to
2ea3195
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
|
TODO: edit first post with conclusions. A lot of TODOs were added in code as well since this PR was growing forever. |
…LUKS containers (BTRFS QubesOS 4.2)
cryptsetup2 2.6.1 is a new release that supports reencryption of Q4.2 release LUKS2 volumes created at installation.
This is a critical feature for the Qubes OS 4.2 release for added data at rest protection
Cryptsetup 2.6.x internal changes:
- Argon2 used externally and internally: requires a lot of RAM and CPU to derivate passphrase to key validated in key slots.
- This is used to rate limit efficiently bruteforcing of LUKS key slots, requiring each offline brute force attempt to consume ~15-30 seconds per attempt
- OF course, strong passphrases are still recommended, but bruteforcing LUKSv2 containers with Argon2 would require immense time, ram and CPU even to bruteforce low entropy passphrase/PINs.
- passphrase change doesn't permit LUKS key slot specification anymore: key slot rotates (new one consusumed per op: then old one wiped internally. EG: LUKS key slot 1 created, then 0 deleted)
- reencryption doesn't permit old call arguments. No more direct-io; inadmissively slow through AIO (async) calls, need workarounds for good enough perfs (arguments + newer kernel with cloudfare fixes in tree)
cryptsetup 2.6.1 requires:
- lvm2 2.03.23, which is also included in this PR.
- requires libaio, which is also included in this PR (could be hacked out but deep dependency at first sight: left in)
- requires util-linux 2.39
- patches for reproducible builds are included for above 3 packages.
luks-functions was updated to support the new cryptsetup2 version calls/changes
- reencryption happen in direct-io, offline mode and without locking, requiring linux 5.10.9+ to bypass linux queues
- from tests, this is best for performance and reliability in single-user mode
- LUKS container ops now validate Disk Recovery Key (DRK) passphrase prior and DRK key slot prior of going forward if needed, failing early.
- Heads don't expect DRK to be in static key slot anymore, and finds the DRK key slot dynamically.
- If reencrytipn/passphrase change: make sure all LUKS containers on same block device can be unlocked with same DRK
- Reencryption: requires to know which key slot to reencrypt.
- Find LUKS key slot that unlocks with DRK passphrase unlock prior of reencrypt call
- Passphrase change: no slot can be passed, but key slot of DRK rotates.
kexec-seal-key
- TPM LUKS Disk Unlock Key key slots have changed to be set in max slots per LUKS version (LUKSv1:7 /LUKSv2: 31)
- If key slot != default LUKS version's keyslot outside of DRK key slot: prompt the user before wiping that key slot, otherwise wipe automatically
- This takes for granted that the DRK key slot alone is needed on the system and Heads controls the LUKS key slots.
- If user has something else going on, ie: Using USB Security dongle + TPM DUK, then the user will need to say no when wiping keys.
- It was suggested to leave LUKS key slots outside of DRK alone, but then: what to do when all key slots would be used?
- Alternative implementation could be to only prompt users to wipe keyslots other then DRK when key slots are all used (LUKSv1: 0-7, LUKSv2: 0-31)
- But then cleanup would need to happen prior of operations (LUKS passphrase change, TPM DUK setup) and could be problematic.
- LUKS containers now checked to be same LUKS version prior of permitting to set TPM DUK and will refuse to go forward of different versions.
TODO:
- async (AIO) calls are not used. direct-io is used instead. libaio could be hacked out
- this could be subject to future work
Notes:
- time to deprecated legacy boards the do not enough space for the new space requirements
- x230-legacy, x230-legacy-flash, x230-hotp-legacy
- t430-legacy, t430-legacy-flash, t430-hotp-legacy already deprecated
Unrelated:
- typos fixes found along the way
Signed-off-by: Thierry Laurion <[email protected]>
83845cf
to
1f3bf1d
Compare
…seems like luks passphrase change only happens on one of the containers; not all Signed-off-by: Thierry Laurion <[email protected]>
… DEBUG for TOTP secret/qrcode output to console Signed-off-by: Thierry Laurion <[email protected]>
… first LUKS volume, not all Remove unneeded loop under luks_reencrypt Signed-off-by: Thierry Laurion <[email protected]>
…wiped when going to recovery shell and upon automatic cleanup as all other secret Signed-off-by: Thierry Laurion <[email protected]>
34c3437
to
d7f7b08
Compare
Signed-off-by: Thierry Laurion <[email protected]>
d7f7b08
to
8711d19
Compare
…ne last time: seems like luks passphrase change only happens on one of the containers; not all" This reverts commit 20e9392. To test this PR without reencryption, just 'git revert' this commit Signed-off-by: Thierry Laurion <[email protected]>
c3d0a12
to
b508871
Compare
| LUKS=$FILE | ||
| detect_boot_device | ||
| mount -o remount,rw /boot | ||
| echo "$LUKS $(cryptsetup luksUUID $LUKS)" >/boot/kexec_key_devices.txt |
There was a problem hiding this comment.
It looks like kexec-save-key writes to kexec_key_devices.txt, I think.
That'd make sense, because it makes more sense to collect all the information we need, then execute and write everything, rather than writing one piece of information at a time. The latter can leave you in confusing partially-configured states when something gets canceled (which definitely happens from time to time for users, and is hard to diagnose after the fact).
That's just from a preliminary search though, I think this needs to be read through in more detail. I'm going to go over the rest of the PR first.
| # LUKS variable not exported yet, prompt for LUKS device | ||
| elif [ -z "$LUKS" ]; then |
There was a problem hiding this comment.
I think this would be easier to understand if this function always attempted to prompt for a LUKS device. Right now it's really difficult to see what is supposed to happen with the LUKS variable and I can't really tell if it'll work in various situations where you could cancel, try again, make a mistake, etc.
luks_reencryptseems to fight with this function trying to reuseLUKS. This one seems to reuse it if it's already set (I guess?), butluks_reencryptdoesn't want that so it unsets the variable in some situation, and to be frank I can't tell where it actually loops (does it?) that any of that could matter.test_luks_current_disk_recovery_key_passphrasewhich does loop, but doesn't know to unset LUKS, but does also export LUKS for some reason (not sure why).luks_change_passphrasedoesn't loop around the selection, but does have an unset and an export.
If this function always attempted to prompt for a LUKS device, we could make it clear where LUKS might already be set and not prompt, or be sure we are prompting again if we call it again. We could also make it clear where we might take an existing value from /boot.
If possible I'd encourage printing the selected value to stdout and capturing it in the caller (fewer global variables makes data flow much easier to understand), but sometimes that is tricking when prompting. (It's possible though!) If the prompting makes that too difficult, it'd be OK to store it in a variable, but always do the prompt and store it, let the callers handle business logic to decide whether to prompt I think.
It'd also be worthwhile to document what this function is trying to do, which I can't really isolate right now since it's pretty glued to the functions calling it.
There was a problem hiding this comment.
I think this would be easier to understand if this function always attempted to prompt for a LUKS device. Right now it's really difficult to see what is supposed to happen with the LUKS variable and I can't really tell if it'll work in various situations where you could cancel, try again, make a mistake, etc.
if $LUKS filled by configured TPM DUK provisioned (which is the only place filling /boot/kexec_key_devices.txt), it is reused.
luks_reencrypt seems to fight with this function trying to reuse LUKS. This one seems to reuse it if it's already set (I guess?), but luks_reencrypt doesn't want that so it unsets the variable in some situation, and to be frank I can't tell where it actually loops (does it?) that any of that could matter.
luks_reencrypt, test_luks_current_disk_recovery_key_passphrase and luks_change_passphrase will unset $LUKS if ops don't work, recalling slect_luks_container and test_luks_current_disk_recovery_key_passphrase. I tested pretty thoroughly luks_reenctypt and luks_change_passphrase codepaths direcyl without issue (menu options), as well as re-ownership
and TPM DUK when setting boot default option optional LUKS TPM DUK, which fails if two containers not sharing same DUK, which also gives warning for user to fix manually through Option for LUKS passphrase change.
There was a problem hiding this comment.
It'd also be worthwhile to document what this function is trying to do, which I can't really isolate right now since it's pretty glued to the functions calling it.
Will do
There was a problem hiding this comment.
@JonathonHall-Purism this thread should answer your initial comment at #1541 (comment)
| # and test passphrase on all of them | ||
| if grep -q "$(echo $FILE | sed 's/[0-9]*$//')" /tmp/luks_devices.txt; then | ||
| DEBUG "Multiple LUKS containers found on same block device, selecting them all" | ||
| LUKS=$(grep $(echo $FILE | sed 's/[0-9]*$//') /tmp/luks_devices.txt) |
There was a problem hiding this comment.
This seems very confusing and surprising that if you select one LUKS partition on a disk, it'll actually "automatically" alter the keys on all of the LUKS partitions on that disk. This could be very unexpected if, say, you weren't actually using Qubes but you do have a second encrypted partition for whatever reason.
If that's what it has to do, we should ask the user to select a disk instead of a partition, perhaps listing all of the encrypted partitions on that disk since it will affect them all.
I'm not really sure that this is what it has to do, discussion above, it'll help to understand what exactly Qubes does with its LVM partitions and why we need to manipulate all of them (if we do).
There was a problem hiding this comment.
@JonathonHall-Purism I knew you wouldn't like this for BTRF 2 LUKS containers setup and file selector. Again, confusion present on default Q4.2 with single LUKS containing swap, dom0 rootfs lvm and other qubes lvms, which Heads doesn't care about. Heads just cares about LUKS containers. Not sure how to deal otherwise then current situation, exactly for your concern. Will then have to think more about this, but again, as per current code, expectation is for multiple LUKS containers to be unlockable with same Disk Recovery Key passphrase when setting DUK and reencrypting. A user having multiple LUKS containers would deploy manually, and should not have to reencrypt, and if he decides to set a DUK, he would select both LUKS to be decrypted automatically on boot, or only pass the partition linked to his LUKS FDE?
Comments?
There was a problem hiding this comment.
I'm more confused. Is this trying to solve an issue with the default Qubes 4.2 partition layout or a custom layout?
OP currently says:
cryptsetup2 toolstack version bump and script fixes to support multi-LUKS containers (BTRFS QubesOS 4.2)
Which sounds like it's intending to solve a problem with the default setup for Qubes 4.2, but I don't know what a "multi-LUKS container" is (do you mean an LVM with mulitple logical volumes formatted with LUKS?), or what btrfs has to do with anything (we're just trying to unlock LUKS, not inspecting the root filesystem here)
This comment says:
A user having multiple LUKS containers would deploy manually, and should not have to reencrypt, and if he decides to set a DUK, he would select both LUKS to be decrypted automatically on boot, or only pass the partition linked to his LUKS FDE?
So it is actually about a manual setup, not the default? There are all sorts of manual setups possible.
Whenever I have had more than one LUKS partition (I do right now), I put an additional key in the root volume that's able to unlock the secondary volumes. So I only unlock one password, and if I used a TPM DUK it'd still just need to unlock the root volume. (I don't use that though 🤷 )
So again, the root question is, what partition setup are we talking about needing to support here?
There was a problem hiding this comment.
Wait, there's a comment above now answering my question I think, had to refresh the page (sorry, wish Github would either show updates or don't, really frustrating when it shows some of them...)
There was a problem hiding this comment.
OK, I get this now from the comment above.
Regarding the confusing UX, what would be wrong with prompting the user to select a disk instead of "prompt for partition, then assume they really want the whole disk and select all partitions on the disk"?
| #whiptail_warning --title 'tes' --msgbox 'test' 0 80 | ||
| #whiptail_error --title 'error' --msgbox 'error' 0 80 | ||
| #Neither work today. Not related to this PR... Using whiptail without coloring. | ||
|
|
There was a problem hiding this comment.
What happened when you tried to do this? We do this elsewhere
There was a problem hiding this comment.
I tried to pass a title with a expended variable and that failed. Will have to revisit later, this is left to investigate outside of this PR, where technological debt is that the added whiptail calls are not whiptail_warning now, I don't know what I missed.
There was a problem hiding this comment.
@JonathonHall-Purism I left notes here because I was unsuccessful calling whiptail_warn and whiptail_error with a varibale expanding in --title, which works in messagebox.
There was a problem hiding this comment.
These comments here don't really help with that and I don't see any other purpose to them 🤷
Variable expansions are done by the shell before invoking the command (which here is a shell function), there should be no difference between expanding a variable in different parameters.
You didn't accidentally leave it in single quotes right?
| luks_secrets_cleanup | ||
| # remove "known good" selected LUKS container so that next pass asks again user to select LUKS container. | ||
| # maybe the container was not the right one | ||
| unset LUKS |
There was a problem hiding this comment.
unsetting $LUKS in case of failing here @JonathonHall-Purism
| export luks_current_Disk_Recovery_Key_passphrase | ||
| break; | ||
| luks_secrets_cleanup | ||
| unset LUKS |
There was a problem hiding this comment.
unsetting $LUKS in case of failing here @JonathonHall-Purism
| rm -f /boot/kexec_key_devices.txt | ||
| mount -o remount,ro /boot | ||
| luks_secrets_cleanup | ||
| unset LUKS |
There was a problem hiding this comment.
unsetting $LUKS in case of failing here @JonathonHall-Purism
| rm -f /boot/kexec_key_devices.txt | ||
| mount -o remount,ro /boot | ||
| luks_secrets_cleanup | ||
| unset LUKS |
There was a problem hiding this comment.
unsetting $LUKS in case of failing here @JonathonHall-Purism
| rm -f /boot/kexec_key_devices.txt | ||
| mount -o remount,ro /boot | ||
| luks_secrets_cleanup | ||
| unset LUKS |
There was a problem hiding this comment.
unsetting $LUKS in case of failing here @JonathonHall-Purism
| unset luks_current_Disk_Recovery_Key_passphrase | ||
| unset luks_new_Disk_Recovery_Key_passphrase | ||
|
|
||
| #TODO: refactor logic of selec_luks_conatainer, where to put | ||
| #unset LUKS |
There was a problem hiding this comment.
unsetting $LUKS should be done sowhere more globally, but TODO: here, where @JonathonHall-Purism ?
There was a problem hiding this comment.
I have no idea where. Like I said, it's really hard to follow this LUKS variable around to figure out what's supposed to happen in lots of corner cases.
Like I said here (#1541 (comment)), it would be much easier to understand if the function did not do anything with a global variable.
It'd be some refactoring, but IMO to make this understandable:
- have the function always do what it says it will do (prompt), never decide that we're not really going to prompt
- don't produce output in a global variable, produce output on stdout for the caller to capture
- let the callers manage this state locally and decide whether to reuse the existing value or prompt
|
@JonathonHall-Purism ping. Can be tested under #1773 |
|
@JonathonHall-Purism note that file selector is asked only once as well with BTRFS under QubesOS 4.2.x, after which /boot/kexec_key_devices.txt is used as input and reused if found, so not really a technological debt to select all partitions under same block device as per this PR. Feel free to challenge/propose changes here. This PR is needed :) |
|
@UndeadDevel ping for review if you have a chance as well. Code suggestions welcome. Also incorporates #1547 AFAIK as said in other comments. |
|
@tlaurion Here's where I am on this PR: All the tool updates seem OK. I would not block you from merging the TPM DUK changes, because we do not ship that feature in PureBoot. The TPM DUK work is, IMO, confusing and I can't really tell from review whether it will work reliably or have issues in corner cases (per comments: #1541 (comment), #1541 (comment)). The UX is also confusing/surprising, not something you usually want when dealing with LUKS encryption keys (#1541 (comment)). I've made suggestions for each of those things. While I try to offer actual changes whenever I can, unfortunately due to internal needs I can't allocate more time toward this feature we don't ship right now. But like I said, I won't block based on those things. So, up to you @tlaurion whether you want to merge as-is or work on those suggestions, feel free to discuss further though if you'd like. |
Bump kernel from 5.10.5 -> 5.10.214
luks-functions was updated to support the new cryptsetup 2.6.1 version calls/changes as opposed to old 2.3 in master
kexec-seal-key
TODOs:
Notes:
Unrelated:
OLD:
I finally got a grip on where stems the problem discussed under #1539
cryptsesup requires async/sync ops from kernel (removed directio ops)kernel AIO needed otherwise warning (lets see if that is verbose without being under debug mode later)Todo:
initrd/test_reencrypt_ram.shwhen ramfs raw disk reencryption meets normal speedOld branch prior of cleaning at https://github.com/tlaurion/heads/tree/staging_cryptsetup_261
That was way too much and unexpected work.