-
-
Notifications
You must be signed in to change notification settings - Fork 133
Add PEPPER to password handling for improved security #81
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
base: main
Are you sure you want to change the base?
Conversation
Introduces a PEPPER value from config and appends it to passwords during both admin user creation and login authentication. This enhances password security by making hashes more resilient to attacks.
smittix
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution! The pepper concept is good for defense-in-depth, but there are two issues that need to be addressed before merging:
1. Hardcoded Default Pepper
PEPPER = _get_env('PEPPER', '7aJBLRSimPO0l6Wkfic8YhO3PPqwPwD7oikTfK4TYjzIFxS4Tk')A pepper's security comes from being secret and not in the source code. With a hardcoded default, anyone with access to the repo has the pepper, defeating its purpose.
Suggested fix: Either:
- Require explicit configuration (no default) and fail startup if not set
- Generate a random pepper on first run and store it in the database or a local file outside the repo
- Document that users must set a custom
PEPPERenv var in production
2. Breaking Change for Existing Accounts
After this change, existing admin accounts will fail to authenticate because:
- Existing password hash =
hash(password) - New login check =
hash(password + pepper)
These won't match.
Suggested fix: Add a migration path:
- Option A: On first run after upgrade, detect missing pepper and rehash existing passwords
- Option B: Add a one-time migration command/script
- Option C: At minimum, document that users need to reset their admin password after upgrading
Once these are addressed, happy to merge!
|
Thank you for the detailed feedback. These are excellent points regarding security and the upgrade path. I will be implementing the following changes to address them:
I will remove the hardcoded default pepper to ensure the secret is never part of the source code. I will adopt the following approach:
To handle the transition without locking out current users, I will implement a "Lazy Rehash" strategy:
To be honest, I left the PEPPER hardcoded, much like the default database username and password, assuming that the first thing any user would do after cloning the repo for deployment is customize these values. However, I agree that your approach is much cleaner and more secure. I will push these updates as soon as they are ready. |
Introduces secure PEPPER-based password hashing for user authentication. Updates documentation and setup to require a unique INTERCEPT_PEPPER environment variable, modifies login logic to use the new verification method, and adds automatic migration for legacy password hashes. Updates dependencies and Docker configuration to support these changes.
|
I've implemented the requested changes. This includes both the UV requirements and functionality, as well as Docker and setup.sh. I've tested it, but please review it carefully in case you find anything missing, as there are many possible combinations and I don't want to have overlooked anything. I've also added the missing dependencies to the .toml file after the latest updates. |
Introduces a PEPPER value from config and appends it to passwords during both admin user creation and login authentication. This enhances password security by making hashes more resilient to attacks.