apyrgio/tindercrypt

Easy way to apply key derivation

djmitche opened this issue · 7 comments

Key derivation is a bit slow, but this library performs it on every call to seal_with_passphrase.

It would be great to have an API for deriving a key that is as hard-to-mess-up as the seal_with_passphrase function. Maybe RingCryptor::dervie_key? In fact, the existing private _derive_key looks like it might just do the trick, were it public!

Oh that's a very interesting problem. Currently, Tindercrypt assumes that if you give it a passphrase, it will need to "stretch" it with PBKDF2. This is a slow operation that accepts a salt, hash function and number of iterations, and produces a unique encryption key. What you're saying though is that you don't need to produce a new encryption key from the secret every time you try to communicate with the server. Ideally, you'd precompute the key, and use the same one in the encryption/decryption operations, correct?

If that's the case, I have some quick questions about how taskchampion works, before suggesting any solutions:

  1. Can it persist a precomputed key? I suppose that the client in question exits once an action completes, thus we can't keep the key in the client's memory, correct?
  2. Is the user ever prompted to enter their passphrase, or allowed to choose weak passphrases? If not, then maybe the secret is more akin to a key than a passphrase, and so seal_with_key() may be better and faster.

You've summarized the situation correctly.

The key ends up being used for a few transactions within a single process. it's a synchronization, so there's a pull-new-changes, push-local-changes loop; and there are plans to add more operations. So keeping the key in memory for those few seconds would be OK.

It is a user-provided passphrase, but yes, the other option is to just require that the user supply a full random key, with some easy method for generating that key.

It is a user-provided passphrase, but yes, the other option is to just require that the user supply a full random key, with some easy method for generating that key.

I see, that's a bit cumbersome, and the point of this library is to simplify these things.

As for the precomputed key, I'm a bit reluctant regarding its usage in the decryption phase. I don't see a clean way that a client can guess the (salt, hash_fn, iterations) metadata before attempting to decrypt the first ciphertext, unless they are somehow communicated via other means, or are static. Both of these approaches are not ideal though.

So, here's an alternative way we could solve this problem:

  • Tindercrypt should allow the user to specify the key derivation metadata for the encryption process.
  • When encrypting or decrypting, it should be able to derive the encryption key for a specific (metadata, secret) pair once.

The first part is already possible with seal_with_meta() and KeyDerivationAlgorithm. The second part though requires the introduction of a caching/memoization mechanism in RingCryptor. I can work on that if you're ok with the general idea.

Ah, I had forgotten that the metadata was derived from the ciphertext. I assume that open_with_meta() verifies that the Metadata it is given matches the metadata in the ciphertext?

Maybe an easier set of additions would be:

  • Provide an easy way to create a default Metadata object (so I can utilize the sane defaults and not make up my own) (apologies if I've missed this in the documentation!)
  • Provide a method to convert a passphrase to a key, given a Metadata object (derive_key).

Then I could cache the key in a fashion that suits my needs, and just use seal_with_meta() and open_with_meta() directly. I would be losing the functionality of automatically deriving metadata from the ciphertext, but that's OK in my use case.

(I suppose it would be useful if tindercrypt had a compatibility policy with respect to changes to sane defaults!)

Ah, I had forgotten that the metadata was derived from the ciphertext. I assume that open_with_meta() verifies that the Metadata it is given matches the metadata in the ciphertext?

I'm afraid not. The ciphertext is just an encrypted plaintext. The metadata sit right next to it, and when open_with_meta() is used on the data buffer that contains them and the ciphertext, they are deserialized and only the ciphertext remains. So, if the provided metadata don't match the ones that were used to create the ciphertext, then the decryption will simply fail. This should be fine though because we expect that the user can parse them easily with:

let (meta, meta_size) = metadata::Metadata::from_buf(buf)?;

Maybe an easier set of additions would be:

  • Provide an easy way to create a default Metadata object (so I can utilize the sane defaults and not make up my own) (apologies if I've missed this in the documentation!)

We already have a (hopefully simple) way to do so:

use tindercrypt::metadata::Metadata;
let data = "The cake is a lie";
let meta = Metadata::generate_for_passphrase(data.len());

(taken from here)

The same logic roughly applies to the generation of the key derivation metadata as well, which are probably more of interest to you as they are the ones that RingCryptor._derive_key() uses:

use tindercrypt::metadata::{HashFunction, KeyDerivationMetadata};

// Generate a struct instance for the key derivation metadata. The default
// is to choose an HMAC function based on SHA-256, 100,000 iterations of
// PBKDF2 and a unique salt.
let key_meta1 = KeyDerivationMetadata::generate();
assert_eq!(key_meta1.hash_fn, HashFunction::SHA256);
assert_eq!(key_meta1.iterations, 100000);

(taken from here)

  • Provide a method to convert a passphrase to a key, given a Metadata object (derive_key).

I agree, that's the part where making RingCryptor._derive_key() public comes in.

Then I could cache the key in a fashion that suits my needs, and just use seal_with_meta() and open_with_meta() directly. I would be losing the functionality of automatically deriving metadata from the ciphertext, but that's OK in my use case.

Sorry, I see now that my previous comment wasn't clear enough. The problem with seal_with_meta() and open_with_meta() is that even if you pass a precomputed key as a secret, they will assume it's a passphrase and attempt to derive a key from it. This will screw the encryption/decryption operation. That's why I proposed extending Tindercrypt by allowing to pass a secret but derive it once for the same metadata.

Now that you described a bit more your plan though, maybe adding a flag to RingCryptor in order to not do any key derivation, and accept the provided precomputed key as the final one would help?

(I suppose it would be useful if tindercrypt had a compatibility policy with respect to changes to sane defaults!)

Well, given that the metadata for the encryption are bundled with the ciphertext, any changes to the defaults (either by a new version of the library or the user) will be recorded in the serialized metadata and used during the decryption. That's the way Tindercrypt achieves backwards compatibility.

Those plans sound good to me. It would be good for the docs for open_with_meta() to explain that it ignores the embedded metadata entirely -- I don't think it was unreasonable of me to expect that it would verify that they match. Just making it clear that it doesn't check -- and how the caller could perform that check -- is probably sufficient.

You're right, it's worth clarifying this bit. So, I'll work on it and send a PR that you can check as well.