I've made an initial attempt at support for creating LUKS-encrypted partitions at install time. A patch is available here:
http://dlehman.fedorapeople.org/anaconda-fscrypto-20071109.patch
The basic idea is a class to represent a generic encrypted device, with a subclass for dm-crypt, and another for LUKS (the LUKS class inherits from the dm-crypt class, not the base class). The fsset Device classes all get a member containing either a pass-through encryption device or a LUKS one. The main operations on the Device objects are reworked slightly so that, down to the encryption class code, the code paths are identical for encrypted and non-encrypted devices.
Some notes: - My testing was done on rawhide from 24 October, although the patch I posted is against rawhide as of now. - I tested basic LVM (LUKS PVs), RAID 0 (LUKS mdX), and normal partitions. - Code to load the needed crypto kmods is non-existent. I've been using a hacked up /sbin/anaconda for that, but it's not for keeps. - Although I think the building blocks are there, there is no support whatsoever for existing LUKS partitions.
Have a look, if interested, and provide feedback.
Dave
David Lehman wrote:
I've made an initial attempt at support for creating LUKS-encrypted partitions at install time. A patch is available here:
http://dlehman.fedorapeople.org/anaconda-fscrypto-20071109.patch
Just for the record: Do you plan to support external USB sticks(smart cards) for saving/loading passwords? I will assume this is a must have feature once the crypto stuff is in place.
Greetings, Alexander.
On Mon, 2007-11-12 at 09:36 +0100, Alexander Todorov wrote:
David Lehman wrote:
I've made an initial attempt at support for creating LUKS-encrypted partitions at install time. A patch is available here:
http://dlehman.fedorapeople.org/anaconda-fscrypto-20071109.patch
Just for the record: Do you plan to support external USB sticks(smart cards) for saving/loading passwords? I will assume this is a must have feature once the crypto stuff is in place.
Personally, I think that supporting 10 different types of setting up crypto in the installer is insanity. The UI will get incredibly out of hand (not to mention the code). Especially with the fact that luks allows you to add things after the fact, I'd lean more towards using that and _just_ letting people do passphrases at install time.
A real look through the patch hopefully forthcoming later today or tomorrow
Jeremy
On Fri, 2007-11-09 at 17:33 -0600, David Lehman wrote:
I've made an initial attempt at support for creating LUKS-encrypted partitions at install time. A patch is available here:
http://dlehman.fedorapeople.org/anaconda-fscrypto-20071109.patch
Okay, I haven't actually tried it as my current tree is a bit muddled with the resizing patch, but I wanted to be sure to at least look over the patch here and provide some feedback.
* Some of the code is obviously forward-looking (the cryptodev registering in particular). Which is fine, but it'd probably be better to hold off on those bits from an initial commit * I'm a little unsure about adding the crypto dev to the fsset.Device object. I wonder if instead it's cleaner to just integrate the crypto code into the Device object. But I'm on the fence here I think * If we go this route, NullCrypto is probably better than Passthrough * Multiple different types of crypto block devices seems like it's going to end up being a UI nitemare. We should pick one path rather than trying to support everything under the sun * Is the filesystem.supportsEncryption attribute really needed? The filesystem doesn't have to really support it at all as it's all done at the block level * The UI is definitely along the right lines, although I'm not convinced about the passphrase prompting. Also, the code would be cleaner if it wasn't trying to support multiple ways of encryption :) * The sanity checking should probably be combined a bit and likely in partitions.sanityCheckAllRequests() as that's where we do other checks for, eg, /boot not being on a PV
Overall, though, this looks very good and promising.
- Some of the code is obviously forward-looking (the cryptodev
registering in particular). Which is fine, but it'd probably be better to hold off on those bits from an initial commit
Makes sense, especially since it's unused at this point.
- I'm a little unsure about adding the crypto dev to the fsset.Device
object. I wonder if instead it's cleaner to just integrate the crypto code into the Device object. But I'm on the fence here I think
- If we go this route, NullCrypto is probably better than Passthrough
I started out just using the Device classes, but ended up up feeling like it was the wrong way.
It seems logical to establish the passphrase at the same time we set up the partition, and often we don't know the device/partition name at that point (right?). Certainly we don't have a Device instance handy. I ended up adding an encryption boolean and a passphrase to the RequestSpec class, which I later passed to the Device constructor. So the Device also has an encryption boolean and a passphrase, along with a bunch of code that depends on the value of the boolean. It started to feel like a hack. It may be cleaner in terms of overhead/minimalism, but it's way dirtier in terms of design IMO.
That said, it may be that the more committed you/we are to only supporting LUKS longer-term, the more acceptable it becomes to just stuff it in where it fits in (the RequestSpec and Device classes).
- Multiple different types of crypto block devices seems like it's going
to end up being a UI nitemare. We should pick one path rather than trying to support everything under the sun
Yes. I mainly wanted to lay the foundation so that extension is easier if we choose to pursue it. I agree completely that we should only support LUKS now, and anything beyond establishing a passphrase and a crypttab entry is left as an exercise for the user.
- Is the filesystem.supportsEncryption attribute really needed? The
filesystem doesn't have to really support it at all as it's all done at the block level
It's only there so the UI knows when to display the "Encrypt this partition" checkbox. It could just as easily be a list in cryptodev.py. I mainly want to be very clear about what we support, and I don't think it should include crazy stuff like encrypted VGs.
- The UI is definitely along the right lines, although I'm not convinced
about the passphrase prompting. Also, the code would be cleaner if it wasn't trying to support multiple ways of encryption :)
Yeah, all that is meant to be forward-looking but could easily be abbreviated if we choose to. It's sort of there to illustrate the extensibility of the current model.
- The sanity checking should probably be combined a bit and likely in
partitions.sanityCheckAllRequests() as that's where we do other checks for, eg, /boot not being on a PV
Makes sense. I basically tried to add the checks in the RequestSpec to cover things like kickstart once we add support there, and to the UI to prevent the user from ever thinking they can create, for example, an encrypted /boot (nobody wants to think of a passphrase, enter it twice, then get an error saying that encrypted /boot is not allowed).
Overall, though, this looks very good and promising.
Anaconda-devel-list mailing list Anaconda-devel-list@redhat.com https://www.redhat.com/mailman/listinfo/anaconda-devel-list
Dave Lehman wrote:
- I'm a little unsure about adding the crypto dev to the fsset.Device
object. I wonder if instead it's cleaner to just integrate the crypto code into the Device object. But I'm on the fence here I think
- If we go this route, NullCrypto is probably better than Passthrough
I started out just using the Device classes, but ended up up feeling like it was the wrong way.
It seems logical to establish the passphrase at the same time we set up the partition, and often we don't know the device/partition name at that point (right?). Certainly we don't have a Device instance handy. I ended up adding an encryption boolean and a passphrase to the RequestSpec class, which I later passed to the Device constructor. So the Device also has an encryption boolean and a passphrase, along with a bunch of code that depends on the value of the boolean. It started to feel like a hack. It may be cleaner in terms of overhead/minimalism, but it's way dirtier in terms of design IMO.
That said, it may be that the more committed you/we are to only supporting LUKS longer-term, the more acceptable it becomes to just stuff it in where it fits in (the RequestSpec and Device classes).
I think that if we're to switch away from LUKS, it'll be a switch. Not a matter of supporting multiple at a time. Maybe it makes sense to just have a EncryptionInfo that contains all of the encryption related pieces and then just do if that's None or not. But like I said, I'm a little on the fence here.
- Multiple different types of crypto block devices seems like it's going
to end up being a UI nitemare. We should pick one path rather than trying to support everything under the sun
Yes. I mainly wanted to lay the foundation so that extension is easier if we choose to pursue it. I agree completely that we should only support LUKS now, and anything beyond establishing a passphrase and a crypttab entry is left as an exercise for the user.
The problem is if we leave the code such that it's there, someone is going to want to extend it ;-)
- Is the filesystem.supportsEncryption attribute really needed? The
filesystem doesn't have to really support it at all as it's all done at the block level
It's only there so the UI knows when to display the "Encrypt this partition" checkbox. It could just as easily be a list in cryptodev.py. I mainly want to be very clear about what we support, and I don't think it should include crazy stuff like encrypted VGs.
But it's not based on the filesystem -- what wouldn't we show as being able to encrypt? If it's just VGs, well, that's different code anyway and we can just not include the checkbox on the VG editor
- The sanity checking should probably be combined a bit and likely in
partitions.sanityCheckAllRequests() as that's where we do other checks for, eg, /boot not being on a PV
Makes sense. I basically tried to add the checks in the RequestSpec to cover things like kickstart once we add support there, and to the UI to prevent the user from ever thinking they can create, for example, an encrypted /boot (nobody wants to think of a passphrase, enter it twice, then get an error saying that encrypted /boot is not allowed).
sanityCheckAllRequests will get all the cases including kickstart. And it's really the only right place. Otherwise, there's lots of "fun" in ordering how you create /boot and /.
Jeremy
I've completed another iteration, with the following changes:
- removed EncryptedDevice and DMCryptDevice classes from cryptodev.py - Device objects now contain a member, crypto, containing either a LUKSDevice or None (as do RequestSpec objects, which hasn't changed) - UI code no longer contains rules about what FSs and/or mountpoints can be encrypted - removed the encrypted device catalog code from cryptodev.py
I think that's it. Here's the patch:
http://dlehman.fedorapeople.org/anaconda-fscrypto-20071115.patch
Dave
On Fri, 2007-11-16 at 11:07 -0600, Dave Lehman wrote:
I've completed another iteration, with the following changes:
- removed EncryptedDevice and DMCryptDevice classes from cryptodev.py
- Device objects now contain a member, crypto, containing either a LUKSDevice or None (as do RequestSpec objects, which hasn't changed)
- UI code no longer contains rules about what FSs and/or mountpoints can be encrypted
- removed the encrypted device catalog code from cryptodev.py
I think that's it. Here's the patch:
As we discussed on IRC, I think we still want to look harder at the UI we present, but we should do that at the same time as integrating the resizing pieces. So I'm fine with going ahead and getting this in and iterating from there.
So feel free to commit and push to the master
Jeremy
anaconda-devel@lists.fedoraproject.org
-
Alexander Todorov -
Dave Lehman -
David Lehman
-
Jeremy Katz