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

sync-server: add option to store hashed passwords #3083

Merged
merged 1 commit into from Mar 26, 2024

Conversation

laalsaas
Copy link
Contributor

Storing passwords in cleartext is immoral, and because I don't want to do that I wrote this PR.

I don't know rust very well, so please have a thorough look through my code and tell me if there are ways that are more rust-idiomatic.

Also, I'm willing to write tests/docs as soon as it's clear this goes in the right direction, but maybe I'll need some guidance for this.

@laalsaas
Copy link
Contributor Author

Does CI re-run automatically on force push or is there some way to trigger it manually?

@laalsaas laalsaas force-pushed the hashed-sync-passwords branch 2 times, most recently from 9515c7f to b3f61b2 Compare March 22, 2024 11:06
@laalsaas
Copy link
Contributor Author

I'm not sure why the CI is still failing, running cargo fmt locally works fine

@dae
Copy link
Member

dae commented Mar 22, 2024

  • Checks run on each push
  • Formatting is documented in docs/development.md

I feel there's not a lot of value to hashing passwords in this case, as if you're following best practice and not reusing them on other sites, the only way they get compromised is if the machine is compromised - and then a hashed password does not help you at all.

You are not the first person to want this however, and I feel like I might be better off accepting it than having to keep arguing about it. But some changes will be needed:

  • It must continue to work without hashes too, as the hash generation step makes initial setup harder for users, especially on platforms like Windows.
  • Instead of using a separate _HASHED, why not have an env var that controls whether passwords are interpreted as being hashed?
  • The env var would need to be documented, and you'd need to tell users how to generate the hashed password.

Optional:

  • Why not have the salt be embedded in the hash, like in a password file? Then it can be provided by the user.

@laalsaas
Copy link
Contributor Author

  • It must continue to work without hashes too, as the hash generation step makes initial setup harder for users, especially on platforms like Windows.

The current implementation is 100% backwards compatible: It'll take the normal SYNC_USER env variable with username:password, even the host key format doesn't change so no need to re-login.

  • Instead of using a separate _HASHED, why not have an env var that controls whether passwords are interpreted as being hashed?

The advantage of the current implementation is, that you can store the password for one user in hashed format, and for another one in plain text. But if you think this is too much of an edge case I can change it.

  • The env var would need to be documented, and you'd need to tell users how to generate the hashed password.

Obviously, I'll do a PR at the doc repo when we've the above detail sorted out.

Why not have the salt be embedded in the hash, like in a password file? Then it can be provided by the user.

This is already done, the hash string follows the phc-format for hash strings, i.e. $pbkdf2-sha256$i=600000,l=32$VU4SOxcZKBIhTlWtuzpFhA$1f7dmnr5o3zqkUmjCuuIRvjkWrOcliW/uAWfkD8JPqg, where the parameters are separated by $-signs, first "column" is algorithm, second is parameters, third is salt and the last one is the actual hash. Only for users where the password isn't hashed, a fixed salt is used. I've written a small program to generate a hash in that format from input.

@dae
Copy link
Member

dae commented Mar 22, 2024

Thanks for clarifying those points. I don't think there's a need to have a mix - for the home use case this server is targeted at, it's hard to imagine a good reason to allow mixing hashed and unhashed passwords.

@laalsaas laalsaas force-pushed the hashed-sync-passwords branch 2 times, most recently from a5e231c to 035f6a2 Compare March 22, 2024 20:32
laalsaas added a commit to laalsaas/anki-manual that referenced this pull request Mar 22, 2024
Copy link
Member

@dae dae left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, some minor comments, then happy to merge this in.

rslib/Cargo.toml Outdated
@@ -38,6 +38,7 @@ wiremock.workspace = true

[dependencies]
criterion = { workspace = true, optional = true }
pbkdf2 = { version = "0.12", features = ["simple"] }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move this into the workspace Cargo.toml and reference it here, like the other deps.

rslib/src/sync/http_server/mod.rs Outdated Show resolved Hide resolved
_ => (
name,
// hashes password with fixed salt, because why bother properly
// salting a password that is stored in plaintext anyways?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd find a comment like the following more useful:

// Plain text passwords provided; hash them with a fixed salt.

laalsaas added a commit to laalsaas/anki-manual that referenced this pull request Mar 26, 2024
@laalsaas laalsaas force-pushed the hashed-sync-passwords branch 2 times, most recently from 4a95da1 to 029d48c Compare March 26, 2024 06:57
@dae
Copy link
Member

dae commented Mar 26, 2024

Looks good - thanks!

@dae dae merged commit 798d9df into ankitects:main Mar 26, 2024
1 check passed
dae pushed a commit to ankitects/anki-manual that referenced this pull request Mar 26, 2024
* enclose all environment variables properly

* Document ability to store passwords hashed

As introduced by ankitects/anki#3083
tekinosman pushed a commit to tekinosman/anki-manual-it that referenced this pull request Mar 26, 2024
* enclose all environment variables properly

* Document ability to store passwords hashed

As introduced by ankitects/anki#3083
tekinosman pushed a commit to tekinosman/anki-manual-it that referenced this pull request Mar 26, 2024
* enclose all environment variables properly

* Document ability to store passwords hashed

As introduced by ankitects/anki#3083
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.

None yet

2 participants