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

Improve reencryption UX #1547

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

UndeadDevel
Copy link
Contributor

@UndeadDevel UndeadDevel commented Dec 7, 2023

Fixes #1538

With the first commit, which addresses point 1 in #1538:

  • Refactored the luks_reencrypt function (superfluous else clause with no unique code)
  • Added clear to the luks_reencrypt function to hide the new secrets as re-encryption starts to avoid having the secrets stuck on the screen in plaintext. Since Use a wider window to show the secrets #1543 fixed the final secrets whiptail window, it's appropriate to remove the secrets from screen for the potentially long re-encryption process so that people aren't stuck trying to cover their screen for an hour or more if they are not physically isolated.

This PR also addresses point 2 in #1538, using the alternative solution suggested there, namely it informs the user that after a passphrase change or re-encryption the checksums need to be updated and the TPM resealed, if applicable, thus giving pointers to an inexperienced user, who may be wondering why the default boot fails after using the separate "change passphrase" or "re-encrypt LUKS container" functions.

Copy link
Collaborator

@tlaurion tlaurion left a comment

Choose a reason for hiding this comment

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

@UndeadDevel reviewed!

fi
echo -n "$luks_current_Disk_Recovery_Key_passphrase" >/tmp/luks_current_Disk_Recovery_Key_passphrase
#make secrets disappear from screen as reencryption can take a long time (we show these to the user again later in whiptail anyway)
printf "\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n"
Copy link
Collaborator

@tlaurion tlaurion Dec 7, 2023

Choose a reason for hiding this comment

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

Please use "clear" or "reset" instead, provided by busybox. Clear being enough

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On NK Heads 2.1 neither clear nor reset are on the PATH. Will all new versions include it? If so, I will change this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator

@tlaurion tlaurion Dec 7, 2023

Choose a reason for hiding this comment

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

It was added under heads under d3ec6ac

So 2.1 nitrokey release is July and adding clear is October.

@daringer will nitrokey next release rebase include it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

jup, noted - we'll include it

initrd/bin/gui-init Outdated Show resolved Hide resolved
Signed-off-by: Christian Foerster <[email protected]>
This reverts commit ee6f299.

Will communicate to the user the need to update checksums instead.

Signed-off-by: Christian Foerster <[email protected]>
Signed-off-by: Christian Foerster <[email protected]>
@UndeadDevel
Copy link
Contributor Author

Okay so I fixed clear vs. printf as discussed.

The other issue I'm not touching beyond adding a message to the user as per my alternative suggestion in #1538. The reason is that too much would have to be refactored in order not to make UX worse (e.g. by having user enter the same new passphrase multiple times) and I can't test anything that involves TPM DUK functionality.

@tlaurion
Copy link
Collaborator

tlaurion commented Dec 18, 2023

@UndeadDevel please update op comment to reflect actual state of PR. I will test in next following days

@UndeadDevel
Copy link
Contributor Author

Done.

@tlaurion
Copy link
Collaborator

Need to test but LGTM

@tlaurion tlaurion self-assigned this Dec 29, 2023
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.

Improve LUKS container re-encryption and OEM factory reset process
3 participants