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

Feature request: improve password hygeine / security #2370

Open
1 of 2 tasks
ferbs opened this issue Jun 18, 2024 · 6 comments
Open
1 of 2 tasks

Feature request: improve password hygeine / security #2370

ferbs opened this issue Jun 18, 2024 · 6 comments
Labels
enhancement type enhancement good first issue Good for newcomers help wanted Good for newcomers

Comments

@ferbs
Copy link

ferbs commented Jun 18, 2024

Search before asking

  • I had searched in the issues and found no similar issues.

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?

  • I'm willing to submit a PR!
@ferbs ferbs added the enhancement type enhancement label Jun 18, 2024
@PragmaTwice
Copy link
Member

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.

@ferbs
Copy link
Author

ferbs commented Jun 21, 2024

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 --requirepass switch on start, but in a containerized environment using a secrets manager, this requires overriding the project's original dockerfile entrypoint and dealing with environment variable expansion headaches.

What would be much better is telling kvrocks where to find the secret. For example, masterpass_env KVROCKS_MASTER_PASS tells the app to use the value of that environment variable as its password. Better yet, adding masterpass_file /var/run/secrets/kvrocks.ini tells kvrocks to find that value within a file, making it even easier to lock it down using a secrets manager/vault, and is more likely to keep the cleartext secret out logs/history/environment, places that might leak.

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.

@PragmaTwice
Copy link
Member

PragmaTwice commented Jun 22, 2024

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.

@ferbs
Copy link
Author

ferbs commented Jun 22, 2024

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.

@git-hulk
Copy link
Member

@ferbs
Copy link
Author

ferbs commented Jun 23, 2024

Good idea to review theirs! Looks like their file_env() would work without modification, and is MIT license. I personally try to minimize time with bash, so nice to borrow from experts.

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: docker run --env KVROCKS_PASSWORD_FILE=/var/run/secrets/kvrocks.ini apache/kvrocks --requirepass '$${KVROCKS_PASSWORD}'

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 $([conditional -Z] printf) stuff.

@git-hulk git-hulk added good first issue Good for newcomers help wanted Good for newcomers labels Jun 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement type enhancement good first issue Good for newcomers help wanted Good for newcomers
3 participants