-
Notifications
You must be signed in to change notification settings - Fork 59
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
Encrypted Volumes #100
Encrypted Volumes #100
Conversation
This is built on top of my swap-minimal branch (from #90). Only the last five commits are specific to this. |
@sergio-correia FYI |
Weird that the luks tests should be what fails since that's what I ran locally on F32 prior to opening/updating this pull request. |
Oh, good grief. It only passes on the OS I developed and initially tested on? It'll have to wait til next week. |
What's missing at this point is unknown, aside from better testing:
|
The relevant work starts with 'Add basic support for managing LUKS volumes.' When I started this the swap work was already done, so I assumed it would be on master by the time any serious review of this work began. |
It is probably not specific to the LUKS functionality. The systemd-managed fstab is really quite fussy in my experience so far. I have to run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
README.md
Outdated
This string specifies a non-default cipher to be used by LUKS. | ||
|
||
##### `encryption_key_size` | ||
This integer specifies the LUKS key size (in bytes). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The key size should be in bits, e.g. 512
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doh, good catch -- thanks!
This probably merits an explanation. In blivet, the backing device will have a format of type 'luks'. This is the encrypted, or backing, device. The next layer out is a LUKSDevice, which represents the (decrypted/open) device-mapper device. Because the LUKS layer is optional and can be effectively toggled, there are many occasions on which it is convenient to look past the LUKS layer directly to the backing device. For this purpose, all of blivet's StorageDevice classes have a 'raw_device' property. For unexcrypted leaf devices, the raw device is the same as the actual device. For encrypted leaf devices, the raw device points to the backing device. In other words, adding _raw_device establishes a line between the raw/backing/encrypted device and the decrypted/mapped/open device which makes test validation quite a bit easier.
Doing so for all formats can trigger deactivation of device stacks that needlessly complicates things.
Relying on _reformat to create the formatting worked because a DiskDevice always exists. Now that self._device can be an optional, non-existent, LUKS layer on top of the disk we have to make the disk volume class behave more like the other volume classes -- namely, it has to create its format as part of _create since we will not always call _reformat (eg: when we've set up, but not yet created, a new LUKS layer on the disk).
This adds support for LUKS volumes.
The testing is very minimal. I'm curious what it will take to get it to pass CI.