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

Enroll a new recovery key #132

Merged
merged 5 commits into from
Oct 28, 2024
Merged

Enroll a new recovery key #132

merged 5 commits into from
Oct 28, 2024

Conversation

danyspin97
Copy link
Collaborator

Add a method to enroll a new recovery key. The only things missing are reading the key from systemd-cryptenroll --recovery-key and it should be ready and testing the command.

@danyspin97 danyspin97 force-pushed the recovery-key branch 3 times, most recently from 3f15688 to c3b5ac0 Compare September 18, 2024 10:42
@danyspin97 danyspin97 changed the title WIP: Enroll a new recovery key Enroll a new recovery key Sep 18, 2024
@danyspin97 danyspin97 marked this pull request as ready for review September 18, 2024 10:48
@danyspin97 danyspin97 force-pushed the recovery-key branch 3 times, most recently from b2496d4 to 92f0e51 Compare September 20, 2024 13:33
@danyspin97
Copy link
Collaborator Author

Added the multidisk implementation.

@danyspin97
Copy link
Collaborator Author

Added the opposite flow: generated recovery PIN -> set recovery key.

sdbootutil Outdated Show resolved Hide resolved
sdbootutil Show resolved Hide resolved
sdbootutil Show resolved Hide resolved
sdbootutil Outdated Show resolved Hide resolved
sdbootutil Outdated
else
# A recovery key has already been generated, use it for all the devices
recovery_key="$(keyctl pipe "$recovery_key_id")"
if ! tmp_rk=$(systemd-cryptenroll --wipe-slot=recovery --recovery-key --unlock-tpm2-device=auto "$dev"); then
Copy link
Collaborator

Choose a reason for hiding this comment

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

same

Copy link
Collaborator

Choose a reason for hiding this comment

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

This requires that the TPM2 enrollment happen first, and there is no guarantee of that (nor that is only a FIDO2 enrollment, or a tpm2+pin)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This requires that the TPM2 enrollment happen first, and there is no guarantee of that.

This is true, but we know that there is a TPM2 valid slot during first-enrollment. If the user knows the recovery key can first do the TPM2 enrollment and then enroll a new recovery key.

FIDO2 enrollment

Maybe we can add a --unlock argument that defaults to auto and will use the first one of the available slots (TPM2, FIDO2, passphrase, recovery key) by checking systemd-cryptenroll output.

or a tpm2+pin

In this case shouldn't systemd ask for the PIN directly? I don't see any other options specific for TPM2.

sdbootutil Outdated
keyslot="${split[0]}"
# cryptsetup can only read the new passphrase from a keyfile
echo -n "$recovery_key" > recovery_key
cryptsetup luksChangeKey --key-slot "$keyslot" "$dev" recovery_key < <(echo "$tmp_rk")
Copy link
Collaborator

Choose a reason for hiding this comment

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

what if is the first enrollment? This key will not be present

Use tmpfs to store the recover key file. And I think that the echo can be removed via <<<"$tmp_rk"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The key is always generated above (systemd-cryptenroll --recovery-key) and then changed with a new one manually, so we can safely call luksChangeKey.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am missing something then. What I am reading is that if "recovery_key_id" is empty then it will generate a recovery key. This will the case for the first device.

But the second device does not have a recovery key keyslot, and at this point "recovery_key_id" is not empty.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If it is empty, generate a new one, save it in the kernel keyring and show it to the user. If it is not empty, get the recovery key stored in the kernel keyring, let systemd generate a new random recovery key (if tmp_rk ...), then replace it with the recovery key from the kernel keyring.

Copy link
Collaborator

Choose a reason for hiding this comment

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

aggg I see it now, sorry. Why not to enroll one using only cryptsetup?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because this way it's systemd-cryptenroll that uses the TPM2 to unlock the device instead of doing it manually (we don't know the current password / recovery key at this time) and the user might not know either.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not sure that I agree. In the old model d-e-t generate a recovery key and use it to drive the enrollment. We should use the same approach but with this "management-key" or "enrollment-key". This key is generated during installation and used only to drive the initial enrollment, and then dropped.

This means that yes, the installed (d-e-t has this role) knows the password to use.

Depends on the TPM2 is not realistic, as TPM2 is optional. I can decide to enroll the FIDO2 key and a recovery key, or only to enroll a simple old password.

Copy link
Collaborator

@aplanas aplanas left a comment

Choose a reason for hiding this comment

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

There are some nits appointed, and other I discovered later (error messages in certain uses cases).

I will merge and fix in a subsequent commit.

Comment on lines +2339 to +2340
echo "You can also scan it with your mobile phone:"
echo "$recovery_key" | qrencode -t utf8i
Copy link
Collaborator

Choose a reason for hiding this comment

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

Indent wrong

sdbootutil Outdated
@@ -2488,6 +2535,12 @@ unenroll_device()
"$dev"
;;

"recovery_key")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would amend this PR using the bit from the last PR that fix this issue

# only the first time and if there is no recovery pin selected (%user:pin or %user:cryptenroll).
# Unlock the LUKS2 header by using the TPM2 slot
if [ -z "$recovery_key_id" ]; then
if recovery_key=$(systemd-cryptenroll --wipe-slot=recovery --recovery-key "${extra_args[@]}" "$dev"); then
Copy link
Collaborator

Choose a reason for hiding this comment

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

I still think that is better something like this:

recovery_key="$(...)"
if [ -n "$recovery_key" ]; then
 ...

The reason are: (1) line is shorter, (2) it is explicit what we validate, (3) some return errno are not errors, (4) as we will see later, some functions needs to pipe the output to /dev/null, making it hard to understand

@aplanas aplanas merged commit f893bf3 into openSUSE:main Oct 28, 2024
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.

2 participants