-
-
Notifications
You must be signed in to change notification settings - Fork 419
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
Add support for multiple pw hashes, use secret key and user-binding #558
base: master
Are you sure you want to change the base?
Conversation
Rebased on a current |
The PR looks good though I must admit it's a bit advanced given my knowledge about password oracles :). About the questions
I think the naming is good. Do you know if it requires a database migration or any value can be chosen? If data migration is needed, can we document it in https://github.com/simple-login/app/blob/95c8f14ea50240c1b59d008ea0dc911dd94da3cd/docs/upgrade.md#L1-L0 so people who self-host can run the migration too?
Yes without downtime is quite important in my opinion. |
Thanks; I would have sent it in smaller, easier-to-review chunks, but I couldn't find a clean way to split it up.
Yes, the PR will require a data migration, which I haven't implemented yet. (I'm no Alembic expert, I'm afraid)
OK; I will come back to you on that later, as there are 2 main strategies I'm aware of for managing key rollover in password storage. and I'd prefer to discuss them with a few cryptographers before committing to either approach, to check that my analysis is correct:
In both cases, the rollover process is a bit involved, if it has to be done without downtime, as one needs to ensure that all running instances can decrypt data produced by any other instance:
|
@nguyenkims Could you help me out with the ORM? Under PostgreSQL, it rejects both |
Also added support for extending to other password hashes.
This is harder to misuse and more obviously correct. It does change the private keys generated for subclasses, which is fine to do in the PR that introduces encryption.
This is only necessary temporarily, as there are no binary wheels available for the `aead` branch of PyNaCl.
This saves a tiny bit of computation when building multiple instances.
`functools.cache` was introduced in Python 3.9, while this is expected to run on Python 3.7, so the older solution is used instead.
This should work under ~all usual databases.
Had to update the dependency on PyNaCl (my branch for pyca/pynacl#676 doesn't exist anymore, and was merged upstream) and rebase to ensure that the deps can be resolved and installed at any point in the git history |
@nbraud I tried to run What error do you have when running the code? |
Hi,
I'm opening this as a draft PR, as there are aspects that make it not ready IMO (like
pyca/pynacl#676 not being merged yet,not having a lossless DB migration script, etc.) but I feel this might require extensive discussion prior to merging.Rationale
This PR implements two main changes:
XChacha20
wrapper that turns any unkeyed oracle into a keyed one, using AEAD.Those are submitted as a single PR as they depend on one-another:
XChacha20
needs theKeyedOracle
abstraction to get its private key, and thePasswordKind
enum to construct the AAD value, whilePasswordKind
needs at least oneKeyedOracle
implementation to be populated (otherwise, password login is not possible). AllowingUnkeyedOracle
s inPasswordKind
would break this dependency loop, and was considered, but ultimately rejected as strictly-worse design: unkeyed constructions are less secure, so only keyed constructions should be used anyhow, and the long-term benefit outweight the short-term cost of a larger PR to review as a single block.This also motivates the
KeyedOracle
vs.UnkeyedOracle
distinction, since this makes the cryptographic distinction readily apparent in the code.KeyedOracle
uses the Blake2b keyed hash function as a KDF, to automatically derive a unique private key for each derived class; the use of a KDF is to prevent using the same (or related) keys in different constructions, which can be insecure, and Blake2b was selected as it is fast, secure, supports personalisation, and readily available in the standard library since Python 3.6.The design of the
XChacha20
wrapper was motivated by multiple consideration:XChacha20-Poly1305
construction is fast regardless of whether specialized hardware support is available, suffers from no restrictions on the number of messages that can be encrypted under a given key or their size (unlike other AEAD constructions such asAES-GCM
), and a high-quality implementation is available as part oflibsodium
(I am exposing it inPyNaCl
, the binding maintained by the Python Cryptographic Authority) ;PyNaCl
as a dependency makes other password hash functions available (such as Argon2 and scrypt) that are more modern than bcrypt ;libsodium
'sSecretBox
AE construction) allows binding ciphertexts to a givenpw_kind
andid
, to protect user accounts from an adversary who gained write access to theusers
table of the database: this prevents attacks such as replacing thepw_kind
field of a victim user's record (substituting a weak oracle for a strong one, under the AEAD wrapper) or replacing thepw_blob
from that of a “donor” account whose password is known to the attacker.Assumptions
user.id
primary key is immutable. If theid
of a given user record is changed, decryption of theirpw_blob
will fail, effectively removing their ability to log in using a password.PW_SITE_KEY
environment variable, containing a uniformly-random, hex-encoded, 256b private key; if it isn't set, they are told to usepython3 -c 'import secrets; print(secrets.token_hex(32))'
to correctly generate one.Changes
PW_SITE_KEY
configuration variable.KeyedOracle
andUnkeyedOracle
abstract base classes, and related tests.bcrypt
as anUnkeyedOracle
XChacha20
, using encryption to convert anyUnkeyedOracle
into aKeyedOracle
.For instance, a user who last set their password when
DEFAULT_KIND
wasAEAD_XCHACHA20_BCRYPT
, should have it upgraded to another oracle construction when a newer one is implemented.Discussion needed
Should thePW_SITE_KEY
be renamed to something more generic, and potentially be used in a similar way as here to encrypt other secrets?Should there be support for multiple site keys, so key rollovers can be performed without downtime, and in the presence of non-upgradableKeyedOracle
s (such as Argon2, using its support for keying and AAD) ?