matrix-appservice-irc icon indicating copy to clipboard operation
matrix-appservice-irc copied to clipboard

Migrate to symmetric encryption for passwords

Open 3nprob opened this issue 4 years ago • 5 comments

I may be missing some context on why it was decided to use an RSA private key for symmetrically encrypting a database field.

While it's perfectly secure, it has drawbacks over AES:

  • RSA is very slow/CPU intensive compared to cryptographic security
  • It can't encrypt messages longer than the key length (manifesting in #481, which this fixes)
  • PEM private keys are large and more cumbersome to manage than terse secret keys.

This changes the encryption scheme from RSA to AES-256-GCM.

To dodge implementing key rotation logic and make this change transparent for users, the AES key is constructed from the hash of the existing private key. Decryption of both modes works transparently.

The real motivation for this change is to facilitate introducing a new encrypted column for per-user SASL keys in #1463

3nprob avatar Aug 18 '21 09:08 3nprob

Seems fine to me – I wonder if we want to actually migrate the old passwords though, to be able to cut out that code path. @Half-Shot?

tadzik avatar Aug 25 '21 10:08 tadzik

@tadzik My 5c: Yes, ideally:

  • Passwords should be migrated
  • passkey in config should be deprecated and replaced with an actual AES key (better inlined than as a separate file)

However;

  • Switching to AES key in config will be a breaking change and need user intervention (either logging the secret key or having the process rewriting config under the user are Bad)
  • Migration code will be a bigger change than this PR
  • Users will skip versions when upgrading, so before removing code there will need to be proper announcement in release notes in the future version where that is actually done

I see two options:

  1. CLI utility to do a one-off migration
  2. Have a period where both new and old config fields are required when passwords already exist, and in presence of that reencrypt with the user-supplied AES key

So IMO it would be best to treat this in isolation from that and address migration/changing config in separate PR(s) to not have this be blocked. Step by step :)

3nprob avatar Aug 25 '21 11:08 3nprob

Completely separate from the above, but a CLI utility for re-encrypting passwords with a new key will also be generally useful to do rotation of the encryption key, either as part of a security routine, or if the encryption key is suspected to be compromised (like if someone mistakenly checks it into git etc)

3nprob avatar Aug 25 '21 11:08 3nprob

Just my two cents on this (and they are really minor): We should not require inlining secrets into the configuration files. I'd much rather have those as a separate file that can be loaded and managed like a secret. I treat configuration files as mostly public in my deployments.

andir avatar Dec 30 '21 15:12 andir

@tadzik @Half-Shot @jaller94 How would you like to move forward with this?

3nprob avatar Apr 05 '22 02:04 3nprob