matrix-appservice-irc
matrix-appservice-irc copied to clipboard
Migrate to symmetric encryption for passwords
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
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 My 5c: Yes, ideally:
- Passwords should be migrated
passkeyin 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:
- CLI utility to do a one-off migration
- 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 :)
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)
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.
@tadzik @Half-Shot @jaller94 How would you like to move forward with this?