Skip to content

Don't store plaintext passwords #1040

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

gdsmith
Copy link

@gdsmith gdsmith commented May 22, 2025

Rather store hashed passwords in the way mysql server does.

See #1037

here we:

  • update CredentialProvider to return new Credential struct
  • Credential includes the plugin that the user was created with
  • update InMemoryProvider to handle hashing of passwords and add default auth method to make usage backwards compatible
  • update server authentication to use mysql server methods of comparing hashes rather than relying on having the plaintext password available
  • rework the password negotiation to switch plugin type to match the stored credentials
  • add hashing and comparison functions for the above where missing from existing libraries

Rather store hashed passwords in the way mysql server does.

here we:

  - update CredentialProvider to return new Credential struct
  - Credential includes the plugin that the user was created with
  - update InMemoryProvider to handle hashing of passwords and add default auth method to make usage backwards compatible
  - update server authentication to use mysql server methods of comparing hashes rather than relying on having the plaintext password available
  - rework the password negotiation to switch plugin type to match the stored credentials
  - add hashing and comparison functions for the above where missing from existing libraries
@gdsmith
Copy link
Author

gdsmith commented May 22, 2025

Code is fully working as much as I can see but I am seeing failures with the TestCachingSha2Cache tests. I don't fully understand if it's a client problem or a change I've introduced means the server is pulling those credentials multiple times when it didn't previously. I did try debugging and it seemed like the cache was empty even if I confirmed the cache was actually written. Would love some feedback, and some help with the failing tests if at all possible.

@lance6716 lance6716 requested a review from dveeden May 23, 2025 01:41
also update the caching test to allow for the fact that we request the password once on every auth attempt
@gdsmith
Copy link
Author

gdsmith commented May 23, 2025

The changes to the code is what has broken the test, we are now getting the password to confirm the auth method. I think the code changes break the way you are checking if we are using the cache response, but I'm not sure how to update the test to ensure we are using the cache (though through debugging I can confirm that it does)

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.

1 participant