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

Luks2 enablement #1380

Closed
wants to merge 22 commits into from
Closed

Luks2 enablement #1380

wants to merge 22 commits into from

Conversation

schubi2
Copy link
Member

@schubi2 schubi2 commented Apr 17, 2024

I have tried to fulfill the requirements of
yast/yast-installation#1088 (comment)

  • Removing YAST_LUKS2_AVAILABLE environment variable.
  • Taking LUKS2/PBKDF2 for default encryption (if selected).
  • New entries in section partitioning/proposal of the product description file
    (encryption_method and encryption_pbkdf) in order to set encryption while
    proposal. Default is no encryption.

Docu:
yast/yast-installation-control#126
yast/yast-installation#1115

@coveralls
Copy link

Coverage Status

coverage: 97.803% (-0.01%) from 97.815%
when pulling cede280 on luks2_enable
into 600ef23 on master.

@ancorgs
Copy link
Contributor

ancorgs commented May 3, 2024

Sorry for taking so long to review this. I'm starting now.

At first sight I must say this is more intrusive than what I tried to suggest at yast/yast-installation#1088 (comment)

What I suggested was making the encryption details (eg. use LUKS2) configurable:

  • by product
  • in case the user decided to use encryption.

But what I see here goes further. What I see with a first sight at the code (I may be wrong, so take things with a bit of grain).

  • It looks like this makes encryption details, like the LUKS version or the derivation function, configurable by all users in the Guided Setup interface.
    • Too much of an advanced setting for my taste. I expected those technical details of the GUIDED setup to be decided by the product, not configurable by the user. In my opinion, the Expert Partitioner is the only place for those things.
    • With the current implementation, I think it would be possible for the user to select "Pervasive Encryption" in s390 during the Guided Setup. I have serious doubts that would work out of the box. See, for example Select APQNs for a new secure key #1033.
  • This also makes it possible for a product to enforce encryption by default. I guess the plan is to activate that for Tumbleweed...
    • Which would make encryption the default for Tumbleweed except if the user runs some extra steps to have a non-encrypted system. Quite a significative change for the distribution.
    • Which results in a slightly weird workflow in my opinion.
      • The installer makes an initial proposal without encryption but prints an error about that being wrong, asking the user to run the Guided Setup to fix the inconsistency.
      • That's kind of an anti-pattern for YaST. The initial proposals should be sensible, not intentionally wrong and needing user intervention to fix them. All users of such a product will be faced with a installer error.

Anyway, I will continue with the review now.

@joseivanlopez
Copy link
Contributor

I also think this change should be much easier:

  • Adapt ProposalSettings to use Luks2 and PBKDF2 by default
  • Adapt ProposalSettings to read encryption method and function from a control file.
  • Adapt Expert Partitoner to use the proposal settings as default value in the form for encrypting a device.

@ancorgs
Copy link
Contributor

ancorgs commented May 3, 2024

What I suggested was making the encryption details (eg. use LUKS2) configurable:
- by product
- in case the user decided to use encryption.

So basically I meant #1383 (it was easy to implement based on this pull request but keeping the changes minimal).

@schubi2
Copy link
Member Author

schubi2 commented May 6, 2024

Sorry for taking so long to review this. I'm starting now.

I am happy that you have time for it again. After a so long time I would like to come to an end even it could be a "minimal" solution. ;-)

* The installer makes an initial proposal without encryption but prints an error about that being wrong, asking the user to run the Guided Setup to fix the inconsistency.
* That's kind of an anti-pattern for YaST. The initial proposals should be sensible, not intentionally wrong and needing user intervention to fix them. All users of such a product will be faced with a installer error.

Yes, you are right, but I have not found any other general solution for requesting the password if encryption (LUKS1/LUGS2) has been set by the product descriptions. I am really open for suggestions here.....

@ancorgs
Copy link
Contributor

ancorgs commented May 6, 2024

Yes, you are right, but I have not found any other general solution for requesting the password if encryption (LUKS1/LUGS2) has been set by the product descriptions. I am really open for suggestions here.....

First, let me double check I understand what's your intention. You want that anyone installing openSUSE Tumbleweed (or any other product configured in the same way, for that matter) always gets a screen asking for the encryption password at a relatively early stage of the installation process, so that password is then used for the initial storage proposal.

That possibility is not contemplated by the current installation workflow because none of the steps of the workflow make that possible. So you will need to change the installation workflow introducing a new YaST client (maybe replacing one of the current ones).

I'm pretty sure you (@schubi2) know what the installation workflow is. But let me go into the details for other readers landing here.

As we all know, everything about the installation process is configured at control.xml (stuff like the repositories to use, whether to use Btrfs or snapshots, etc.). And actually the most important aspect defined on that control.xml is the workflows for installing, for upgrading and so on. That's basically the sequence of steps (YaST clients) that are executed during installation. For Tumbleweed installation such a sequence can be found here. https://github.com/yast/skelcd-control-openSUSE/blob/master/control/control.xml#L738

That's the reason why the TW installation process looks like this (including the name of each YaST client):

install_inf complex_welcome system_analysis system_role
01-install_inf 05-complex_welcome 08-system_analysis 11-system_role
disk_proposal timezone user_first inst_proposal
13-disk_proposal 14-timezone 15-user_settings 18-inst_proposal

You can create your own YaST client asking for the encryption password and introduce the client as a step before the disk_proposal one. That's actually what the common criteria role does at SLE, because the installation roles have the ability to define their own installation steps that get injected to the workflow (if the user selects the role) after the role selection screen.

If you prefer, instead of adding an installation step, you could even fully substitute the disk_proposal client by an alternative that works in a different way. In its current incarnation, that client is not designed for a product in which encrypting is the default option and, thus, entering the password is mandatory for installing.

I hope that answers your question. If not, I hope it's at least illustrative information for someone. :-)

One final remark, if you introduce a completely new YaST client as mandatory in the Tumbleweed installation workflow then get ready to get all bugzilla reports redirected at you. ;-) It's quite a fundamental change for the distribution.

@schubi2
Copy link
Member Author

schubi2 commented May 7, 2024

Yes Ancor, you have summarised my intentions correctly and you have convinced me. :-)
So, I would suggest that we are agreeing on using LUKS2 as default or make it configurable in the product settings at least. The UI will not be changed. I am not sure what's you intention has been in:
https://github.com/yast/yast-storage-ng/pull/1383/files#diff-778a5f8930595efbaf3a5dc6db8b7b31ad8510ad94c536909870d3000300b7beR218 It is your decision which way we are
going there.
At least we should be consistent regarding the default value in expert and guided setup.
So, in order to come to an end, we should use your PR and simply reject my one:
#1383

@schubi2
Copy link
Member Author

schubi2 commented May 8, 2024

We have come to the decision to use
#1383
for this issue.

@schubi2 schubi2 closed this May 8, 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.

None yet

4 participants