-
Notifications
You must be signed in to change notification settings - Fork 28
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
refactor: Rewrite luks tpm2 script. #302
Conversation
The previous version of the tpm2 script did not fail out when encountering unsupported configurations. This script is narrowly focused at enabling tpm2 decryption for root devices. By default we expect this to be configured via the installer. This script will now fail out if it cannot find the disk. The disk is determined via the kernel commandline instead of /etc/crypttab. It still only supports a single disk. It now also provides hints on why the script has failed and the commands to run to disable and/or enable.
This only supports single disk decryption. If we want to support multi-disk decryption. We can instead parse the output of lsblk. For example |
@@ -1,57 +1,65 @@ | |||
#!/bin/bash | |||
## setup auto-unlock LUKS2 encrypted root on Fedora/Silverblue/maybe others | |||
set -u | |||
set -eou pipefail | |||
|
|||
[ "$UID" -eq 0 ] || { echo "This script must be run as root."; exit 1;} | |||
|
|||
read -p "This will modify your system and enable TPM2 auto-unlock of your LUKS partition! Are you sure you've read the script and are good with this? " -n 1 -r |
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.
Should we remove "you sure you've read the script and" and instead state that we only use PCR7 which and give some idea what will cause the auto-unlock to fail and require a manual unlock?
It's a lot to describe in a prompt, so maybe a link to a page describing the PCRs? The more I think and write about this, the more I think further changes, even to describe PCR usage, should be a distinct PR.
But... maybe we do still remove "are you sure you've read the script" as that's not happening when users call "ujust"
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.
We should indicate that this will bind to secureboot state via PCR7. That will give a user some idea on what this is doing.
And probably specify that this is using systemd-cryptenroll so a user can look up information about it.
Also, clearly users are not inspecting the script prior to running so I'm unsure if that warning is actually serving any purpose right now.
I really like the current changes... much better parsing the cmdline vs the crypttab. I do think multi-disk should definitely be a distinct PR/feature... as this current handles a default (recommended) install method. |
echo "TPM2 already present in /etc/crypttab. Exiting..." | ||
[[ "$0" = "$BASH_SOURCE" ]] && exit 1 || return 1 | ||
# Cut off the luks- | ||
DISK_UUID=${RD_LUKS_UUID:5} |
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 might be good to validate that the RD_LUKS_UUID starts with "luks-" in case this format ever changes in the future
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.
LUKS_PREFIX="luks-"
if [[ $RD_LUKS_UUID == ${LUKS_PREFIX}* ]]; then
DISK_UUID="${RD_LUKS_UUID#$LUKS_PREFIX}"
else
echo "LUKS UUID format mismatch."
exit 1
fi
something like this might be more elegant
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 like this. While I don't expect the prefix to change it's not a hard addition addition.
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.
sanity checks never hurt 😄 especially when messing with cryptsetup
fi | ||
|
||
# Cut off the luks- | ||
DISK_UUID=${RD_LUKS_UUID:5} |
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.
same thing here
fi | ||
|
||
## Wipe luks slot | ||
cryptsetup luksDump $CRYPT_DISK | grep systemd-tpm2 > /dev/null | ||
if [ 0 -eq $? ]; then | ||
if cryptsetup luksDump "$CRYPT_DISK" | grep systemd-tpm2 > /dev/null; then |
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.
This validation can be more thorough. For example, if the user accidentally enrolled multiple tokens, or has no backup passphrase.
In the latter case, I believe systemd-cryptenroll will throw an error, which would break the script if not handled
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.
DISK_LUKSDUMP=$(cryptsetup luksDump $CRYPT_DISK)
if echo $DISK_LUKSDUMP | grep fido > /dev/null || ! echo $DISK_LUKSDUMP | {add logic to check for passphrase here}; then
echo "cryptsetup has been manually configured, exiting..."
exit 1
fi
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.
This is the same logic that we had previously. We can put a warning at the top to make sure you still have a decryption method besides the tpm.
We could have a warning during the enrollment to always ensure that you have a password or recovery key to ensure you have access to the data.
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.
@m2Giles I don't see this logic in the previous script, unless you mean the backing up of the previous crypttab?
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 do like the check.
I think the simple thing will be checking if passphrase is in slot 0. If not it's safe to assume the user manually configured the device.
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.
@m2Giles yeah that's a reasonable assumption.
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.
@m2Giles I think this script should be as foolproof as possible, so if there's any sign that the user has manually touched cryptsetup, we should fail and say "it looks like you've got this covered :)"
otherwise you open this up to needing to support and/or work around custom configurations
if [ 0 -eq $? ]; then | ||
echo "TPM2 already present in /etc/crypttab. Exiting..." | ||
[[ "$0" = "$BASH_SOURCE" ]] && exit 1 || return 1 | ||
echo "Ensuring you have a known passphrase in key-slot 0." |
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.
This check seems redundant as systemd-cryptenroll requires the user to provide a valid description method (passphrase, recovery key, fido, etc) before it will add TPM2.
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.
Actually this is worse than redundant as it force the user to only have a passphrase in slot 0... So if a user added a recovery key and removed the original passphrase, this fails, but i think our script shouldn't fail for that use case.
echo "Wiping systemd-tpm2 from LUKS on $CRYPT_DISK" | ||
systemd-cryptenroll --wipe-slot=tpm2 $CRYPT_DISK | ||
systemd-cryptenroll --wipe-slot=tpm2 "$CRYPT_DISK" |
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.
Confirming a comment I made elsewhere, systemd-cryptenroll
will NOT wipe the tpm2 slot if it's the last remaining slot.
$ systemd-cryptenroll /dev/disk/by-uuid/d4f1e47c-a653-4cd5-a6ae-bfc62c0a6852 --wipe-slot=tpm2
Wipe operation would leave no valid slots around, can't allow that, sorry.
Granted, if a user is living dangerously if they get to this state, and I think it would have to be by use of having a tpm2 slot and recover-key slot both setup by systemd-cryptenroll, and then having wiped the recovery slot.
Similarly, cryptsetup luksKillSlot
prevents removing of a slot using that slot's passphrase.
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.
As an aside, if a user does reach the state of only a tpm2 slot... a passphrase can be re-added:
cryptsetup luksAddKey --token-id 2 --token-type systemd-tpm2 /dev/disk/by-uuid/d4f1e47c-a653-4cd5-a6ae-bfc62c0a6852
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.
https://github.com/ublue-os/config/pull/302/files#r1667429020
Please remove pre-emptive test for passphrase in key-slot 0... as systemd-cryptenroll handles this adequately.
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.
LGTM
* refactor: Rewrite luks tpm2 script. The previous version of the tpm2 script did not fail out when encountering unsupported configurations. This script is narrowly focused at enabling tpm2 decryption for root devices. By default we expect this to be configured via the installer. This script will now fail out if it cannot find the disk. The disk is determined via the kernel commandline instead of /etc/crypttab. It still only supports a single disk. It now also provides hints on why the script has failed and the commands to run to disable and/or enable. * refactor: Provide more information with script * refactor: Include more safety checks * fix: remove unnecessary luks check --------- Co-authored-by: Benjamin Sherman <benjamin@holyarmy.org>
The previous version of the tpm2 script did not fail out when encountering unsupported configurations. This script is narrowly focused at enabling tpm2 decryption for root devices. By default we expect this to be configured via the installer.
This script will now fail out if it cannot find the disk. The disk is determined via the kernel commandline instead of /etc/crypttab. It still only supports a single disk. It now also provides hints on why the script has failed and the commands to run to disable and/or enable.
Thank you for contributing to the Universal Blue project!
Please read the Contributor's Guide before submitting a pull request.