-
Notifications
You must be signed in to change notification settings - Fork 438
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
Feature request: improve password hygeine / security #2370
Comments
I'm not sure if I understand your words correctly. You can currently specify the master password in kvrocks.conf. And the configuration file should only be accessible to trusted individuals. |
Placing the master password in kvrocks.conf means you can't check that file into git, and that it won't work with secret managers / vaults in a containerized environment. Rather than place the password in that file, someone could use the What would be much better is telling kvrocks where to find the secret. For example, The other suggestion is an option to use sha256 verification of the master password. Where kvrocks is only provided with the hashed password so even if it leaked, it's not in a usable form. |
IMHO from my side it doesn't seem to be the logic that needs to be implemented inside Kvrocks. e.g. if you are using helm charts, you can refer to some helm packages like https://artifacthub.io/packages/helm/bitnami/redis which support configmap construction and secret management for Redis via several ways, as Kvrocks configuration mechanism is similiar to Redis. For stronger security, you can refer to TLS connection support in Kvrocks and provide your own client-side certificates, instead of just passwords. |
Not TLS, I'm talking about password management: how to properly provide the secret to kvrocks and to clients without it leaking into git, logs, etc. With Redis, I can use an ACL file. With postgres, I can use POSTGRES_PASSWORD_FILE. Systems that don't offer that simple mechanism add a burden... modifying config files during deployment with sed, or reconfiguring helm/other, or maintaining a custom entrypoint, etc. I'm personally overriding the entrypoint and am fine, but am suggesting modest ways you could make it easier for people to secure kvrocks with their current tooling. |
Yes, perhaps we can support this in Dockerfile like PostgreSQL: https://github.com/docker-library/postgres/blob/cefde5ff6f102fcd381a03210c7734816c59aa3e/docker-entrypoint.sh#L9. |
Good idea to review theirs! Looks like their Off the top of my head, a new kvrocks-entrypoint.sh might look like: #!/usr/bin/env bash
# copy of file_env() function here
file_env 'KVROCKS_PASSWORD'
exec kvrocks "$@" If using the above entrypoint, --requirepass could be set using a positional [COMMAND] argument on start, but would require escaping var expansion. I think it would look something like: It would be possible for the entrypoint itself to conditionally include --requirepass when $KVROCKS_PASSWORD is present... should someone with more bash ambition come along. :) Prob involves |
Search before asking
Motivation
Improve handling of cleartext passwords, keeping them out of version control in particular, and to follow industry recommended practices.
Solution
Please add an option to configure kvrocks with a sha256 hash of the actual password, rather than using cleartext. Though similar to the Redis
#<hashedpassword>
ACL option, it could be implemented separately from any ACL functionality, as an alternative to using the requirepass directive, perhaps with a kvrocks.conf requirepass_sha256 directive, and/or by setting an environment variable, eg: KVROCKS_SECRET_SHA256=e3b0c442...etc... When a client connects and provides its pass, kvrocks would compare the sha256 of that value if enabled, rather than use the cleartext.Related, it would helpful for kvrocks to support the file-based convention recommended for secrets used in containers. It's often hard to keep regular environment variables out of version control, and they can leak during runtime too. Instead, one environment variable (eg, KVROCKS_PASSWORD_FILE) holds a path to a file in .ini format (key=value pairs just like env vars) which the app loads and uses for its config.
Are you willing to submit a PR?
The text was updated successfully, but these errors were encountered: