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.

Sign up for GitHub

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

Jump to bottom

Rate limit password hashing operations #4353

Open
wants to merge 10 commits into
base: develop
Choose a base branch
from
Open

Rate limit password hashing operations #4353

wants to merge 10 commits into from

Conversation

akshaymankar
Copy link
Member

@akshaymankar akshaymankar commented Dec 2, 2024 •
edited
Loading

https://wearezeta.atlassian.net/browse/WPB-11901

Pending Tasks

  • Log when rate limiting
  • Adjust old integration tests to send X-Forwarded-For.

Checklist

  • Add a new entry in an appropriate subdirectory of changelog.d
  • Read and follow the PR guidelines

@echoes-hq echoes-hq bot added the echoes: technical-roadmap/security More specific category, to highlight task that tackle security requirements. label Dec 2, 2024
@zebot zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Dec 2, 2024
@akshaymankar akshaymankar force-pushed the hash-rate-limiting-haskell branch 10 times, most recently from 8fa6216 to f2fa7ab Compare December 9, 2024 09:35
@akshaymankar
Put all password hashing operations in one place
930fe7d 
Once they are all in that effect, we can start asking for either the user ID or
the IP address and use that to rate limit.

Changes:
1. Move verifyPassword and verifyPasswordWithStatus to wire-subsystems
2. verifyPasswordWithStatus doesn't ask password to be updated if the current
config says that the password should be hashed with Scrypt.
3. Move PasswordStatus type to wire-subsystems as it is not part of the API.
4. Use Wire.Sem.Random (and hence openssl in prod) to generate salt.
5. Move all password tests from wire-api to wire-subsystems.
6. Use HashPassword effect for password verification everywhere.
@akshaymankar akshaymankar force-pushed the hash-rate-limiting-haskell branch 2 times, most recently from 48b7817 to 997310f Compare December 9, 2024 12:08
@akshaymankar akshaymankar force-pushed the hash-rate-limiting-haskell branch from 997310f to 97ef364 Compare December 9, 2024 12:09
@akshaymankar akshaymankar marked this pull request as ready for review December 9, 2024 12:55
@akshaymankar akshaymankar mentioned this pull request Dec 9, 2024
Closed
2 tasks
@mdimjasevic
Copy link
Contributor

mdimjasevic commented Dec 10, 2024 •
edited
Loading

How is this PR different than the previous? I was half way through reviewing the previous one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mdimjasevic mdimjasevic Awaiting requested review from mdimjasevic

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
echoes: technical-roadmap/security More specific category, to highlight task that tackle security requirements. ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@akshaymankar @mdimjasevic @zebot