Add key derivation options support

[mikesposito]

This PR adds support for multiple key derivation algorithms and parameters, by modifying the shape of the vault (encrypted data) and the encryption key (also when exported), in order to hold some (nullable) metadata related to how the key was derived in the first place.

This is mainly needed to increase key derivation iterations, while keeping old vaults decryption possible. With this PR, PBKDF2 default iterations has been set to 900.000 - but when the old shape of the vault or key is detected (missing derivation metadata), the old number of iterations is used.

To keep compatibility with older formats, methods exported by this library will accept both old and new formats.

Changes

• ADDED: EncryptionKey type to hold a CryptoKey along with its derivation parameters

• ADDED: ExportedEncryptionKey type to hold a JsonWebKey along with its derivation parameters

• ADDED: Optional keyMetadata property of type KeyDerivationOptions to EncryptionResult

• ADDED: Optional opts argument to keyFromPassword to specify algorithm and parameters to be used in the key derivation. Defaults to PBKDF2 at 10.000 iterations

• ADDED: Added iterations argument to keyFromPassword function

• ADDED: Optional keyDerivationOptions argument to encrypt to specify algorithm and parameters to be used in the key derivation. Defaults to PBKDF2 at 900.000 iterations

• ADDED: Optional keyDerivationOptions argument to encryptWithDetail to specify algorithm and parameters to be used in the key derivation. Defaults to PBKDF2 at 900.000 iterations

• ADDED: updateVaultWithDetail function to update existing vault and exported key with a safer encryption method if available

• ADDED: updateVault function to update existing vault string with a safer encryption method if available

• CHANGED: encrypt method accepts both EncryptionKey and CryptoKey types as key argument

• CHANGED: encryptWithKey method accepts both EncryptionKey and CryptoKey types as key argument

• CHANGED: decrypt method accepts both EncryptionKey and CryptoKey types as key argument

• CHANGED: decryptWithKey method accepts both EncryptionKey and CryptoKey types as key argument

• CHANGED: importKey method returns a CryptoKey when a JWK string is passed, or an EncryptionKey when an ExportedEncryptionKey string is passed.

• CHANGED: exportKey method accepts both EncryptionKey and CryptoKey types as key argument, and returns an ExportedEncryptionKey for the former and a JsonWebKey for the latter.

[NicholasEllul]

Great idea, this will allow clients who can handle more iterations to increase this number without harming those which cannot.

[mikesposito]

This also gives the chance to a client to decrypt a vault with a weaker key to then re-encrypt it with a more secure one

[mikesposito]

  iterations = 900_000,

has been changed to:

  opts: KeyDerivationOptions = {
    algorithm: 'PBKDF2',
    params: {
      iterations: 900_000,
      exportable: false,
    },
  },

[cryptodev-2s]

LGTM!

[kanthesha]

LGTM!

[MajorLift]

nit: for extra clarity

Suggested change

	test('encryptor:decrypt encrypted data with key derived with different number of iterations', async ({
	test('encryptor:decrypt encrypted data using key derived with smaller number of iterations than the default configuration', async ({

[mikesposito]

I ended up removing this test case, and instead repeating all test cases for both types of encryption results (old and new), to better test backward compatibility

[mikesposito]

I had to dismiss all approvals as we need a way to know how the key was derived in some scenarios.

For this reason, in most of the functions, instead of accepting / returning a CryptoKey we'll return an EncryptionKey object, which has this shape:

export type EncryptionKey = {
  key: CryptoKey; // this might change in the future for other, more generic, types
  derivationFunction: KeyDerivationOptions;
};

export type KeyDerivationOptions = {
  algorithm: 'PBKDF2'; // this can easily become an enum in the future
  params: PBKDF2Params; // ..and this a union type, depending on `algorithm`
};

export type PBKDF2Params = {
  iterations?: number;
  exportable?: boolean;
};

And keyMetadata field (which reports the same object of derivationFunction) has been added also to the encryption result object:

export type EncryptionResult = {
  data: string;
  iv: string;
  salt?: string;
  // old encryption results will not have this
  keyMetadata?: KeyDerivationOptions;
};

Finally, instead of adding a single iterations argument to keyFromPassword function, the function has now this signature:

keyFromPassword(
  password: string,
  salt: string,
  opts: KeyDerivationOptions = {
    algorithm: 'PBKDF2',
    params: {
      iterations: 900_000,
      exportable: false,
    },
  },
): Promise<EncryptionKey>;

This allows:

1. The client to choose the preferred algorithm and parameters
2. To easily extend this in the future with other algorithms and parameters

[mikesposito]

Changes in f10e866 are needed to carry derivation function metadata in the exported key and to expect them (in new cases) when importing it.

[mikesposito]

This PR will also close #1

[NicholasEllul]

While I do acknowledge the benefit of having easy backwards compatibility, I am worried about the longer term technical debt this may incur. Given we are adding additional guards, conditions, and migrated tests to support two kinds of inputs for each operation, many of the the functions now require additional context on the various support cases to be modified effectively in the future.

IMO this is a case where I think verbosity may be better than the neatness of minimizing bundle size, as software bugs here can result in a bit more severe behaviour. Especially since a majority of the conditions are only relevant on that first migration.

What are your thoughts on this:

1. We denote this as being a breaking change and define a migration guide for developers who wish to update to this breaking change version.
2. Alongside the guide, we provide a transformation function which will take in the encrypted data, and transform it into the format we expect. Notably, adding the key argument, and adding the previous parameters that we used to use for the PBKDF2 function. This function will be a no-op if the vault is already migrated.
3. Consumers of this package will simply call the transformation function after they've loaded their vault, but before they interact with the updated interface.

In the MetaMask extension, this can be done with a migration, in other clients, they can use their own strategy, or simply call this each time their vault is loaded as it will later get saved with the new updated parameters.

This will remove all the additional complexity from these functions leaving them simple to understand and build on in the future without worry of breaking vault compatibility.

[NicholasEllul]

Nice! This also makes it pretty easy to expand to other algorithms one day doing something like:

export type KeyDerivationOptions = { algorithm: 'PBKDF2'; params: PBKDF2Params;} | { algorithm: 'OtherAlgo'; params: OtherAlgoParams;}

[NicholasEllul]

Don't we want to allow the client to specify its iterations here?

[mikesposito]

Good idea! this has been added as an optional argument, with DEFAULT_DERIVATION_PARAMS as the default value in 9d06954

[NicholasEllul]

Why do we only conditionally add the metadata to the encryption result?

[mikesposito]

Because if the user encrypts data using an arbitrary CryptoKey we can't assume that it was derived with this library, thus we should leave the metadata empty

[NicholasEllul]

Why do we only conditionally add the metadata to the encryption result?

[mikesposito]

Because if the user encrypts data using an arbitrary CryptoKey we can't assume that it was derived with this library, thus we should leave the metadata empty

[kumavis]

didnt read thread yet but want to add that this doesn't have to be breaking if we record the iterations and assume lack of iterations means the present old default

[danfinlay]

Thank you for this, I've wanted to do this since first writing the module.

I suspect there's a way to do this without breaking, and allowing for a smooth migration path for users of the library:

1. If a vault is restored with no params value, then assume the old default. Maybe log a warning when ever not providing a value.
2. When encrypting, default to the new iteration default, and include the params value in the payload.

This way, even a consumer who is unaware of this issue could have their vaults silently upgraded to a more secure number of iterations.

[mikesposito]

@kumavis @danfinlay Thanks for taking a look into this!

There are two reasons why making this non-breaking or assuming lower iterations might be erroneous:

1. Assuming that a key was derived with lower iterations might be wrong, as we allow users to use their own CryptoKey, which could have been derived with other libraries / algorithms
2. Even if we assume that all vaults without metadata are encrypted with PBKDF2 @10.000 iterations, this should be breaking anyway as keyFromPassword will have different default parameters - so users should only update this package voluntarily with a major bump.

Also, when encrypting new vaults, if we don't want to stop accepting arbitrary CryptoKey's, we'll not be able to assume that the key was derived with a certain algorithm or params. This is why, in this PR, the EncryptionResult metadata related to the key derivation is optional.

Please let me know if this makes sense, and if it would make sense to stop accepting arbitrary (with no metadata) keys when encrypting new vaults - given also that what @NicholasEllul wrote is correct: keeping compatibility with older versions increases complexity

[Gudahtt]

Assuming that a key was derived with lower iterations might be wrong

Perhaps I'm missing something, but I thought we only used these parameters to create the key. So if we have the key already, it doesn't matter how many iterations were used to create it.

[mcmire]

I have some other comments besides this but I will save that for another comment so it is more easily quotable. A lot of these comments are just about the tests.

[mikesposito]

This function can be used by the extension and other clients to migrate their vaults.

As this PR is non-breaking, this is not mandatory, as the library would silently use the new method when encrypting the vault again. But with this helper, the library gets the responsibility of knowing whether the vault is using the latest safe configuration available, resulting in a no-op if the vault is already up-to-date

[mcmire]

One final thing, but this otherwise looks good to me.

[Gudahtt]

LGTM!

[mikesposito]

On @metamask/eth-keyring-controller, we use detailed encryption / decryption as we have to cache the encryption key derived from the password. Without this function, when running the extension in MV3-compatible mode, we'll not be able to cache the encryption key if the vault gets updated - the cached encryption key will become invalid/outdated in case the vault gets updated, so we need to return the exported key in these cases.

[Gudahtt]

LGTM!