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

client: add support for password via environment variable #255

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tuannh99
Copy link
Contributor

Allow setting password through KEYMASTER_PASSWORD environment variable instead of interactive prompt for keymaster CLI.

This pattern aligns with other security tools like Vault CLI (VAULT_TOKEN) and maintains security best practices while enabling automation scenarios.

Hint: This also enables seamless integration with secret management tools like 1password:

Example workflow:

$ export KEYMASTER_PASSWORD=$(op read "op://<concealed-vault-name>/<concealed-item-name>/password")
$ keymaster ${KEYMASTER_ARGS} -configHost ${CONFIGHOST} -fileprefix ${CERT}

Changes:

  • Add environment variable check before password prompt in client util
  • Add test coverage for environment variable path

Allow setting password through KEYMASTER_PASSWORD environment variable
instead of interactive prompt for keymaster CLI. This aligns with other
security tools like Vault CLI (VAULT_TOKEN) for automation scenarios.
@rgooch
Copy link
Member

rgooch commented Dec 10, 2024

I'm not sure that it's a good idea to make it easier to use a password manager. We want people to type their passwords, since this protects against a compromise of the password manager.

@erikespinoza
Copy link
Collaborator

I'm not sure that it's a good idea to make it easier to use a password manager. We want people to type their passwords, since this protects against a compromise of the password manager.

Complicated passwords are generally in a password manager. This would allow one to skip the copy/paste which can be snooped or swiped from the clipboard. Feels safer to use the vault cli best practice IMHO.

Is there a middle ground?

@cviecco
Copy link
Contributor

cviecco commented Dec 11, 2024

I am also not convinced of this PR. Even Mitre has a CWE-526 for this. However given that you took the time to write a patch I am interested on your specific use case.

From your comments it seems like are using some other cli tool to fetch the password. From some tool, is this vault-cli? if so how are you authenticating to the vault. If something else: can you tell us? Also is the use case humans or automation?

top of my head (as brainstrom) : Maybe have an option to make the cli call another cli tool? maybe support other authentication schemes?

@erikespinoza
Copy link
Collaborator

I am not the author or know their needs.

From your comments it seems like are using some other cli tool to fetch the password. From some tool, is this vault-cli? if so how are you authenticating to the vault. If something else: can you tell us? Also is the use case humans or automation?

Looks like the author is using the 1password cli (op). I've used Bitwarden cli (bw) and Lastpass cli (lpass) for storing and pulling creds before.

top of my head (as brainstrom) : Maybe have an option to make the cli call another cli tool? maybe support other authentication schemes?

Not a bad option.

@tuannh99
Copy link
Contributor Author

Thank you all for the thoughtful feedback and security considerations. Let me clarify the use case:

This PR adds an option to use environment variable while still keeping the existing workflow for those who prefer to type password interactively. It supports automated scenarios, and integration with password manager is just one of them.

For context, many infrastructure engineers interact with multiple security tools daily across different environments (prod/staging/etc.).

This pattern of using environment variables for secure credentials is well-established across the infrastructure ecosystem:

  • Vault: export VAULT_TOKEN=$(VAULT_LDAP_PASSWORD=$(op read "op:////password") vault login -token-only -method=ldap username=$VAULT_LDAP_USERNAME)
  • AWS: export AWS_ACCESS_KEY_ID, AWS_SECRET_ACCESS_KEY, AWS_SESSION_TOKEN
  • GitHub: export GITHUB_TOKEN

Having to type complex passwords multiple times daily leads to several risks:

  • Password fatigue leading to weaker passwords
  • Increased clipboard usage exposing passwords to potential memory scraping
  • Tendency to store passwords in plain text notes for convenience
  • Higher chance of typos leading to frustrating workflow interruptions and potentially locked accounts

However, I understand the concerns about password manager compromise and CWE-526, especially when introducing new authentication methods. @cviecco's suggestion about direct CLI tool integration offers a more controlled approach. Instead of environment variables, we could:

  1. Add a --password-command flag that executes a specified command to obtain the password
  2. This would support multiple password managers (1password/op, Bitwarden/bw, Vault CLI, etc.)
  3. The command output would be read directly into memory without touching environment variables

Would this alternative approach better address the security concerns while still enabling secure automation for daily operations?

@rgooch
Copy link
Member

rgooch commented Dec 11, 2024

Passing the password over a pipe is much better, yes.
I'd be more comfortable with this feature if we could limit it to U2F 2nd-factor. I know that 1Password has a "feature" to store an OTP generator, which basically means you're handing over full trust to 1Password (or similar password manager).

@tuannh99
Copy link
Contributor Author

@rgooch Thank you for the feedback regarding U2F security considerations. It was a good idea - having a non-interactive way to provide passwords while maintaining strong security controls.

I've revised the approach to focus on security while enabling automation:

  1. Switched from environment variable to --password-stdin flag
  2. Added strict enforcement of U2F when using stdin input:
    • Automatically disables TOTP and VIPAccess
    • Returns error if U2F is not available
    • Requires physical security token presence

This ensures that even in automation scenarios, we maintain the security benefit of physical token verification.

I've implemented these changes in a new PR: #256

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.

4 participants