[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: Block device encryption support

> * 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

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]