Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate to symmetric encryption for passwords #1462

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

3nprob
Copy link

@3nprob 3nprob commented Aug 18, 2021

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:

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 3nprob force-pushed the aes-passkey branch 2 times, most recently from 7e28b54 to 4030d79 Compare August 18, 2021 09:26
* RSA is slow and can't encrypt longer texts than key length
* Derive AES key from private key to avoid breaking config schema
@tadzik
Copy link
Contributor

tadzik commented Aug 25, 2021

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 tadzik requested a review from a team August 25, 2021 10:59
@3nprob
Copy link
Author

3nprob commented Aug 25, 2021

@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
Copy link
Author

3nprob commented Aug 25, 2021

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)

@andir
Copy link

andir commented Dec 30, 2021

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.

@3nprob
Copy link
Author

3nprob commented Apr 5, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants