Merge ~jibel/ubiquity:encryption_recovery_key into ubiquity:master

Proposed by Jean-Baptiste Lallement
Status: Merged
Merged at revision: 8774e460c1229f6ad38dacb3f1864a254c77cabb
Proposed branch: ~jibel/ubiquity:encryption_recovery_key
Merge into: ubiquity:master
Diff against target: 932 lines (+481/-144)
8 files modified
debian/changelog (+10/-0)
debian/ubiquity.templates (+49/-0)
gui/gtk/stepPartCrypto.ui (+279/-143)
scripts/plugininstall.py (+51/-0)
tests/test_gtkui.py (+1/-0)
ubiquity/components/plugininstall.py (+2/-1)
ubiquity/misc.py (+8/-0)
ubiquity/plugins/ubi-partman.py (+81/-0)
Reviewer Review Type Date Requested Status
Iain Lane (community) Approve
Ubuntu Installer Team Pending
Review via email: mp+397742@code.staging.launchpad.net

Commit message

This adds supports for a recovery key on LUKS + LVM installation:
- Added an extra field for a recovery key in the password page when encryption is selected
- This field is RO and the key is digits only to enter it with a key pad
- The key is saved to a file on disk, so it can be stored in a safe place.
- This key can be used to unlock the device is the usual key is lost for some reason.

Tested on up to date hirsute:
1. Unlock with the default key.
2. Unlock with the master key.

Build in a PPA: https://launchpad.net/~jibel/+archive/ubuntu/ppa/+sourcepub/12131976/+listing-archive-extra

To post a comment you must log in.
Revision history for this message
Iain Lane (laney) wrote :

Heya, sorry for the delay.

This looks mostly good to me. I've got some questions inline.

optional nitpick, which is probably already violated all over Ubiquity: Can we standardise on one for quotes in the changes made here? I see some instances of both " and '.

I've just looked at the code so far. I'll have a go at running it on Monday.

Revision history for this message
Jean-Baptiste Lallement (jibel) wrote :

The MP has been updated based on your comment.
The way the live session user home directory is retrieved and used has been changed a bit to work independently from the way ubiquity is started (from ubiquity-dm, launcher or command line)

Tests are passing
The resulting deb has been tested in ubiquity-dm and the live session.

Revision history for this message
Iain Lane (laney) wrote :

Cheers! I tried it and it worked, the recovery key was accepted.

I've recorded one nitpick inline.

I'm OK with this being uploaded now for testing if you want it in ASAP for translating & testing but there are two things I would like us to fix before hirsute please-

  - Must: please see the two screenshots in https://people.canonical.com/~laney/screenshots/. This MP makes Ubiquity's window quite a bit wider. Can you please look into how the new screen can be wrapped to reduce this?
  - Strongly recommended: the option to encrypt the system is buried too much. It's under Advanced features and shouldn't be - this should be considered a mainstream feature (it used to be). And it's connected to LVM; the checkbox is disabled until LVM is selected. I think that's kind of a mysterious implementation detail and not necessarily a connection users will make.

review: Approve
Revision history for this message
Jean-Baptiste Lallement (jibel) wrote :

Thanks for the review.

1. I've set a fixed size to the labels in stepPartCypto, so they wrap correctly and don't make the main window become extra large. The size of the main window also changed depending on the language due the difference of the length of the string. That should fixed it too.

2. Current implementation matches the design done by mpt when the option ZFS has been added to the installer to avoid too many options on the main partitioning page, especially when there is an already installed OS.
The encryption checkbox, becomes disabled when a volume manager (LVM or ZFS) is selected. The installer doesn't support encryption otherwise.

3. Regarding the comment inline, the content of the file has to be exactly the key that has been stored in LUKS otherwise it cannot be used as input to open the encrypted volume.

Revision history for this message
Iain Lane (laney) wrote :

Thanks for fixing the width.

2> I'm saying I think the design is wrong. Encrypting your system should not be an advanced, buried, option. We need to fix this. It is technically true that it is tied to LVM but you shouldn't have to figure that fact out to be able to click the option.

3> Ok, I thought it was just an alternative key you can type in, in which case the newline makes the text file easier to use. I don't know how to supply it as a file to be honest, is there documentation and should people be linked to that?

review: Approve

There was an error fetching revisions from git servers. Please try again in a few minutes. If the problem persists, contact Launchpad support.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches