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

Create key file for LUKS2 devices #2146

Merged
merged 9 commits into from Jun 13, 2023

Conversation

abalfoort
Copy link
Contributor

This change will handle LUKS2 encrypted devices.

Five installation scenarios were used for testing:
luks2_testing.tar.gz

Note:
Not tested with an encrypted boot because Grub2 cannot handle that yet.

To safely convert LUKS1 devices to LUKS2 devices I created a separate module that is run after the partition module: partition-luks2

If this is something that should be implemented in the partition module, please let me know.

@dalto8
Copy link
Contributor

dalto8 commented May 23, 2023

I need to spend some additional time reviewing this in more detail but there are a couple of things I noticed in a brief review.

People use Calamares to install into existing luks partitions so it isn't safe to remove all the keys from a luks partition. Even if it was safe, we shouldn't be doing it in the luksbootkeyfile module.

Why have you started creating a keyfile when /boot is unencrypted? I don't see a reason to have a keyfile in that scenario.

To safely convert LUKS1 devices to LUKS2 devices I created a separate module that is run after the partition module: partition-luks2

The partition module supports luks2 since a recent commit.

@abalfoort
Copy link
Contributor Author

People use Calamares to install into existing luks partitions so it isn't safe to remove all the keys from a luks partition. Even if it was safe, we shouldn't be doing it in the luksbootkeyfile module.

Normally, I would agree. However, if you user decides not to format the root partition luksbootkeyfile will add a new key to the key file. Eventually Calamares will crash (I had that) when there are no more free slots.

Why have you started creating a keyfile when /boot is unencrypted? I don't see a reason to have a keyfile in that scenario.

You are right, I'll have to look at that. Thanks.

The partition module supports luks2 since a recent commit.

True, but in some scenarios the user would end up in a unbootable system. E.g. if the user decides a simple install and erase the system and create a fully encrypted system with LUKS2, the root partition is going to be LUKS2 encrypted. Grub2 doesn't like that, yet.

The module I created converts only those devices from LUKS1 to LUKS2 if it can safely be converted. So, in the above example the root partition is still LUKS1 while the /home partition is being converted in LUKS2. A compromise as long as Grub2 cannot handle LUKS2 partitions.

@dalto8
Copy link
Contributor

dalto8 commented May 23, 2023

if you user decides not to format the root partition luksbootkeyfile will add a new key to the key file. Eventually Calamares will crash (I had that) when there are no more free slots.

In that case, we should check for that and simply not add a keyfile if there are no slots remaining.

Failing to create a keyfile shouldn't stop the system from booting. It may result in a user having to enter the password more than once if they have multiple luks volumes. That seems like a much lower risk than stripping off all the existing keyslots.

@kkofler
Copy link
Contributor

kkofler commented May 23, 2023

Yes, IMHO, stripping existing keys from an existing LUKS group is a no-go.

@abalfoort
Copy link
Contributor Author

abalfoort commented May 23, 2023

Checking for the number of key slots used is simple in the case of LUKS1. For LUKS2 it is not easy to determine:

The number of keyslots is limited only by the provided header area size.

LUKS2 doesn't seem to have a hard limit on the number of slots available, like LUKS1 has. So, I can check for LUKS1 slots, but I don't know how to calculate the amount of slots available from the LUKS2 header information.

Or I can assume there are 32 LUKS2 slots. That should be more than enough for this case:

    if ( ( luks2_hash.isEmpty() && slots_count == 8 ) ||
         ( !luks2_hash.isEmpty() && slots_count == 32 ))
    {
        // No key slots left: return gracefully
        return true;
    }

src/modules/luksbootkeyfile/LuksBootKeyFileJob.cpp Outdated Show resolved Hide resolved
src/modules/luksbootkeyfile/LuksBootKeyFileJob.cpp Outdated Show resolved Hide resolved
src/modules/luksbootkeyfile/LuksBootKeyFileJob.cpp Outdated Show resolved Hide resolved
src/modules/luksbootkeyfile/LuksBootKeyFileJob.cpp Outdated Show resolved Hide resolved
src/modules/luksbootkeyfile/LuksBootKeyFileJob.cpp Outdated Show resolved Hide resolved
src/modules/luksbootkeyfile/LuksBootKeyFileJob.cpp Outdated Show resolved Hide resolved
@abalfoort abalfoort requested a review from dalto8 May 31, 2023 12:52
src/modules/luksbootkeyfile/luksbootkeyfile.conf Outdated Show resolved Hide resolved
src/modules/luksbootkeyfile/LuksBootKeyFileJob.cpp Outdated Show resolved Hide resolved
src/modules/luksbootkeyfile/LuksBootKeyFileJob.cpp Outdated Show resolved Hide resolved
src/modules/luksbootkeyfile/LuksBootKeyFileJob.cpp Outdated Show resolved Hide resolved
src/modules/luksbootkeyfile/LuksBootKeyFileJob.cpp Outdated Show resolved Hide resolved
src/modules/luksbootkeyfile/LuksBootKeyFileJob.cpp Outdated Show resolved Hide resolved
@abalfoort abalfoort requested a review from dalto8 May 31, 2023 16:25
@dalto8 dalto8 merged commit f1e4bcd into calamares:calamares Jun 13, 2023
0 of 2 checks passed
@abalfoort abalfoort deleted the luksbootkeyfile branch June 13, 2023 16:13
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.

None yet

3 participants