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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement decryption #24

Open
wants to merge 5 commits into
base: decryption
Choose a base branch
from
Open

Conversation

ashfire908
Copy link

Implemented the stubbed-out decryption methods using systemd-ask-password. Also made some tweaks to some error output as well as returning exit code 1 when mounting fails.

Currently it doesn't have any retry logic so if you enter the wrong password, it'll fail and you'll get dropped to the shell during boot. Also the code is probably horrible - I don't actually know C, but I managed to get this working over the weekend within my testing VM for ZFS stuff.

Side note: I had to hack this at one point to scan based on /dev/disk/by-partlabel instead of the default of by-id since my VM software (VMware Workstation) doesn't expose IDs for it's SCSI disks 馃檨. That might be useful to have in the future as a config parameter somewhere.

src/mount.initrd_zfs.c Outdated Show resolved Hide resolved
src/zfs-generator.c Outdated Show resolved Hide resolved
char *linebuffer;
size_t size;
int nRead;
char **cmdline;
Copy link
Owner

Choose a reason for hiding this comment

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

I need to take another look at this

src/zfs-util.c Show resolved Hide resolved
src/zfs-util.c Show resolved Hide resolved
@dasJ
Copy link
Owner

dasJ commented Dec 13, 2017

Thank you very much for your pull request. It looks good so far, and I would have probably never done it. Your C isn't that awful, I know my C which isn't that good as well. But learning by doing I guess ;)
Other changes I'd like to see (but probably work for another PR, however I'd like to have them before creating a new relese), is retry when the password was incorrect and reading keyfiles. The only possible sources for key files are (IMO) raw (encrypted?) block devices and files inside the initrd. The keys being on a FS is probably a lot harder because we'd have to handle dependencies (what if the filesystem with the key is also an encrypted pool?)

@ashfire908
Copy link
Author

I agree with adding the additional features. I already wanted to add retry logic as a frequently typo my passwords, and the key file isn't a bad idea to add either, though that's a different flow. Also will take some thinking about if you want to allow external devices like a flash drive for the key source, but I've never used key files for system encryption, so I don't know how that flow normally goes. I do think that would be best addressed in a followup PR, to isolate the changes. When this PR gets far enough, we can just merge it into decryption branch and use that as a dev branch.

I'll be pushing some changes shortly to fix the first two comments.

@freswa
Copy link

freswa commented Apr 18, 2019

As zfs 0.8.0 is coming up with native encryption, I'd like to help out to push this onward. @ashfire908 Are you still working on the changes mentioned above?

@ashfire908
Copy link
Author

ashfire908 commented May 27, 2019

@freswa No. I have stopped using ZFS (with or without native encryption) as I found it to be too much trouble, especially after my ZFS-based NAS box recently experienced file corruption + driver errors that corrupted the OS (and a couple non-critical files).

@ChristophSchmidpeter
Copy link

@dasJ I have been using this PR on my machine for a long time without any issues. Could you merge this PR, please?

@@ -316,6 +316,10 @@ int generateSysrootUnit(char *directory, int bootfs, char *dataset, char *snapsh
return 1;
}
fprintf(fp, "# This file was generated by sd-zfs\n\
[Unit]\n\
Requires=systemd-ask-password-console.service\n\
Copy link

Choose a reason for hiding this comment

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

This should be Wants.

See systemd/systemd#29053

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

6 participants