argon2: choose a steady parallelism value

[jtolio]

Huge thanks to Least Authority, our security audit team, for finding and notifying us of this issue!!! This is almost certainly the cause of lots of bugs we’ve been unable to identify for some time now.

Problem

At the time of this writing, this line of code is how we turn a passphrase into a root encryption key:
https://github.com/storj/storj/blob/ee6c1ca/pkg/encryption/password.go#L40
keyData := argon2.IDKey(password, pathSalt, 1, uint32(64*memory.MiB/memory.KiB), uint8(runtime.GOMAXPROCS(-1)), 32)

The documentation for argon2’s IDKey says:

IDKey derives a key from the password, salt, and cost parameters using Argon2id returning a byte slice of length keyLen that can be used as cryptographic key. The CPU cost and parallelism degree must be greater than zero.

For example, you can get a derived key for e.g. AES-256 (which needs a 32-byte key) by doing:

key := argon2.IDKey([]byte("some password"), salt, 1, 64*1024, 4, 32)

The draft RFC recommends[2] time=1, and memory=64*1024 is a sensible number. If using that amount of memory (64 MB) is not possible in some contexts then the time parameter can be increased to compensate.

The time parameter specifies the number of passes over the memory and the memory parameter specifies the size of the memory in KiB. For example memory=64*1024 sets the memory cost to ~64 MB. The number of threads can be adjusted to the numbers of available CPUs. The cost parameters should be increased as memory latency and CPU parallelism increases. Remember to get a good random salt.

Emphasis mine. (runtime.GOMAXPROCS(-1) returns the number of CPUs available).

It turns out, argon2 generates different keys depending on the requested parallelism!!

https://play.golang.org/p/cyuYw2cXQxI

In our defense, I think the documentation for the argon2.IDKey method is really misleading, even though we probably should have checked this.

Implications

We generate keys from passphrases whenever someone calls SaltedKeyFromPassphrase in libuplink, or whenever someone runs uplink setup or gateway setup. This is the only time this matters.

However, if someone runs uplink setup on two different devices with the same project, API key, and passphrase, but the devices have a different number of CPU cores, they will not be able to encrypt or decrypt each installation’s data.

If instead, someone uses scopes/encryption accesses for sharing access, key generation is not involved, and this problem will not affect the user.

This is a broad problem that certainly affects all of our users if they ever use more than one device. A sample of Storj employees had surprisingly large variation in the number of cores people had (4, 8, 12, and 16 were all common).

Proposed solution

Uggghhhhh

Well, so it’s easy to fix for all new users going forward. We pick the argon2 parallelism statically.

For all existing users it’s hard. There is no default value that will broadly work for most people. Continuing to use CPU-specific settings means that decryption does not interoperate well.

We could give up on passphrase determinism (scopes are unaffected, and just require everyone to save their scopes), but then why have passphrases at all?

So, we have a couple of different users affected.

• People using libuplink directly - To libuplink, we add a ProjectConfig volatile field where the user can set the Argon2 concurrency value with documentation and a warning. We default to, say, 8, but in the comments say that prior to version x, we used the number of CPU cores your computer had. If you need to decrypt data you uploaded, please set this value. They set the value going forward and everything keeps working. New users ignore it.
• People using commandline tools - To the uplink CLI, we clean up our decryption errors significantly. If we get a decryption error, instead of log spew we say something nice about the encryption settings being wrong. We can even add: “Beta warning: prior to version X, we used the number of cores your computer had. If you just ran setup, try moving your existing configuration to a safe location and rerunning setup again with --pbkdf-concurrency 6” or something. Then, users of the uplink CLI could specify during setup what they wanted that value to be, and then it could get preserved to their configuration. It again would not affect imported scopes.

This change does everything but the error cleanup.

Code Review Checklist (to be filled out by reviewer)

• NEW: Are there any Satellite database migrations? Are they forwards and backwards compatible?
• Does the PR describe what changes are being made?
• Does the PR describe why the changes are being made?
• Does the code follow our style guide?
• Does the code follow our testing guide?
• Is the PR appropriately sized? (If it could be broken into smaller PRs it should be)
• Does the new code have enough tests? (every PR should have tests or justification otherwise. Bug-fix PRs especially)
• Does the new code have enough documentation that answers "how do I use it?" and "what does it do?"? (both source documentation and higher level, diagrams?)
• Does any documentation need updating?
• Do the database access patterns make sense?

[zeebo]

Removing the path changes the design (https://github.com/storj/storj/blob/master/docs/blueprints/password-key-derivation.md#design)

While it's not used, it exists to allow different root encryption keys at different paths inside of a bucket.

[jtolio]

okay nbd to bring it back. i wrongly assumed it was refactor cruft

[isaachess]

Do we need to put an actual URL value here?

[jtolio]

yes, we also need to update the comment about which version numbers the problem applies to, once we have a version it no longer applies to. i'll submit a new pr with updates for both when that happens