Start introducing KeyUnsealer consumed by ActivateVolumeWithKeyUnsealer

[pedronis]

This is an abstraction over the TPM unsealing process.

Also factor prompting for secrets into Prompter so that it can be passed to KeyUnsealer, and be passed from outside
later to possibly match the passed KeyUnsealer as well.

[pedronis]

stacked on #110

[chrisccoulson]

This could do with a short description of the end goal - if the goal here is to make it possible to use the functions in crypt.go with a different key provider (not a TPM) and to be able to split out the TPM-specific parts in to a sub-package, then I'm not sure that the abstraction is quite right - see my comments on the KeyUnsealer interface.

Having an abstraction like this along with an keystore implementation-agnostic API in crypt.go is something I've been thinking about for a while though, because it fits in quite well with the model where all of the data required to recover a key is stored in a LUKS token. I've pushed some of the changes I've been working on to #114, which has an abstraction that does kind-of the same thing (without an equivalent for the Prompter interface though), but looks quite a bit different.

That abstraction is based around a couple of interfaces:

• KeyHandle, which is an abstraction of a key, and provides an interface to determine if the key requires a PIN (so asking for a PIN doesn't need to be handled by the implementation). There is a TPM implementation of this which just wraps a *SealedKeyObject, although KeyHandle implementations don't have to contain an actual sealed key - just some data about how to recover a key from a KeyStore - eg, imagine a toy-TPM implementation that just stores a key inside a NV index on the TPM - the KeyHandle could just contain a handle so that the KeyStore can read the corresponding key from that TPM index.
• KeyStore, which is an abstraction of a device from which keys can be recovered by providing a KeyHandle. There is a TPM implementation of this which just wraps a *TPMConnection, and which unseals a key using SealedKeyObject.UnsealFromTPM.

[chrisccoulson]

It seems like volumeName isn't used by the only implementation of it - is it needed?

But I'm not sure that this abstraction is at the right level.

It seems like sourceDevicePath is only provided here so that it can be supplied to the prompter, but this could be avoided if the caller of this interface was responsible for using the prompter instead, and then it just passes the returned PIN to the implementation (there would need to be a way for the implementation to indicate whether a PIN should be requested).

And resealToken feels like an implementation detail of TPM key protection, and doesn't seem appropriate for this kind of abstraction. I would keep that private to the implementation and have implementations of KeyUnsealer add keys to the keyring in an implementation-specific way, where required.

[pedronis]

This is draft, there are some boundaries questions I agree. I mostly tried to keep the diff mininal at the cost right now of some clunkiness. The idea of an optional secret that is received on unseal and need to be presented again on reseal, is not here not there, given that TPM is probably the only thing that needs a reseal as we know. Your PRs are 1000s of lines of unreviewed code away, skimming them they seem a bit overcomplicated... We do need a stepping stone in between that doesn't block us until we can get a full design in that everybody is happy with.

[pedronis]

I feel a bit about the PIN the way you feel about the auth stuff, it's very TPM specific atm. Either way it's intentional the Prompter is used by the Unsealer and not on behalf of it, it allows to control the experience more from the caller which can decided how coupled or not they are in practice.

[chrisccoulson]

Passing errors returned from UnsealKey back in to the KeyUnsealer implementation in order to decode the error suggests that perhaps this interface needs some new implementation-agnostic error types and UnsealKey should do the conversion from TPM error -> implementation agnostic error and return that instead. Then instead of calling UnderstoodError, the caller can just use the implementation agnostic error types to obtain the RecoveryKeyUsageReason or decide if the error is fatal.

[pedronis]

I would like to avoid UnderstoodError myself OTOH it's a bit unclear how much work it is, and whether it's premature to design those implementation agnostic errors. Most of the errors the code seems to care about are TPM specific and what we need more than errors is what to pass to the recovery key using code.

[chrisccoulson]

I'd picked "tpm" here because in my idea for an abstraction, there would be type-specific handlers for recovering keys, with a type field in the LUKS metadata that is used to look up the correct handler for a keyslot. Encoding and decoding the kernel keyring description and contents would also be handled by implementations of these handlers, and so the type here aligns with the corresponding type field in the LUKS header for a particular keyslot.

[pedronis]

Landing #116 first will make some bits of this immediately cleaner

[pedronis]

That abstraction is based around a couple of interfaces:

KeyHandle, which is an abstraction of a key, and provides an interface to determine if the key requires a PIN (so asking for a PIN doesn't need to be handled by the implementation). There is a TPM implementation of this which just wraps a *SealedKeyObject, although KeyHandle implementations don't have to contain an actual sealed key - just some data about how to recover a key from a KeyStore - eg, imagine a toy-TPM implementation that just stores a key inside a NV index on the TPM - the KeyHandle could just contain a handle so that the KeyStore can read the corresponding key from that TPM index.
KeyStore, which is an abstraction of a device from which keys can be recovered by providing a KeyHandle. There is a TPM implementation of this which just wraps a *TPMConnection, and which unseals a key using SealedKeyObject.UnsealFromTPM.

I looked at that a bit more, I'm not entirely convinced it gives enough control where to store the keys. It's a bit unclear to me that secboot itself should be very opinionated about that, instead of giving primitives to support a range of approaches, of course the preferred approach should be low friction, while wildly diverging ones shouldn't. But admittedly I haven't looked yet how that plans to use/abuse tokens.

[anonymouse64]

I agree that some of this PR is awkward and needs to be refactored again in short order, but I do think that the other PR proposed, is far too big to review at once, and the abstractions here are good steps in the right direction.

I did not do a thorough review of the crypto logic, etc. it seemed to be pretty much a straight refactor moving code inside other functions, but I will try to have another look tomorrow.

[pedronis]

marking blocked for now until we understand more how we will implement the multiple sealed keys, one policy objects

[pedronis]

Something similar is now achieved via ActivateVolumeWith*KeyData, so closing this.

We might still want to pursue something along the lines of the Prompter interface at some point because the issues remain of having UX details hard-coded here, and the fact that systemd-ask-password plugin mechanism is brittle wrt potentially desirable stateful designs.