> * 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@xxxxxxxxxx > https://www.redhat.com/mailman/listinfo/anaconda-devel-list _______________________________________________ Anaconda-devel-list mailing list Anaconda-devel-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/anaconda-devel-list
