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

Directly exec ExternalTokenHelper rather than using a SHELL #29653

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

afresh1
Copy link

@afresh1 afresh1 commented Feb 14, 2025

Description

What does this PR do?

Should resolve #29417

There are many ways using a shell to exec the sub-command can go wrong. Whether that is unexpected spaces in the command or that the shell reads config files that output unexpected values. We encountered the unexpected output due to a very strange interaction with vault being used in a shell. We ran into this using GNU bash, version 5.0.17(1)-release (x86_64-pc-linux-gnu) in a pipe over an ssh connection.

This stemmed from folks setting a default export VAULT_ADDR=https://vault.example.com in their ~/.bashrc, and that somehow when compiled with SSH_SOURCE_BASHRC defined as is apparently the default in some linux distributions, when bash detects it is used in a pipe and that variable is set (as it is when you have ssh'd to a remote host), bash will source ~/.bashrc, overriding the desired VAULT_ADDR. This would happen, for example, with vault list -address https://test-vault.example.com foo | cat. This fails to login because the token_helper returns the token for vault.example.com instead of test-vault.example.com. It is also an issue with updating the VAULT_ADDR in your environment to something other than the default before executing vault.

Because there was already a requirement that the token_helper path was a fully qualified path without any extra flags, it should be safe to directly exec the helper. This avoids any surprise interactions with the environment or with shell startup scripts.

TODO only if you're a HashiCorp employee

  • Backport Labels: If this fix needs to be backported, use the appropriate backport/ label that matches the desired release branch. Note that in the CE repo, the latest release branch will look like backport/x.x.x, but older release branches will be backport/ent/x.x.x+ent.
    • LTS: If this fixes a critical security vulnerability or severity 1 bug, it will also need to be backported to the current LTS versions of Vault. To ensure this, use all available enterprise labels.
  • ENT Breakage: If this PR either 1) removes a public function OR 2) changes the signature
    of a public function, even if that change is in a CE file, double check that
    applying the patch for this PR to the ENT repo and running tests doesn't
    break any tests. Sometimes ENT only tests rely on public functions in CE
    files.
  • Jira: If this change has an associated Jira, it's referenced either
    in the PR description, commit message, or branch name.
  • RFC: If this change has an associated RFC, please link it in the description.
  • ENT PR: If this change has an associated ENT PR, please link it in the
    description. Also, make sure the changelog is in this PR, not in your ENT PR.

RBird111 and others added 7 commits January 24, 2025 11:10
`new(ExternalTokenHelper)` is a lot easier to parse than
`(*ExternalTokenHelper)(nil)`
This will be used to store any extra command arguments and allows
`BinaryPath` to hold *just* the binary path.
Since `BinPath` no longer has to hold any additional arguments we can
execute the command directly without inoking the shell first.
Currently using 0.txt until we have a PR id.
Merge in GITHUB/vault from SM-5317 to main

* commit 'cc8fb0d445349cb9c08d4e00cdcb2e6705752c3e':
  Add changelog entry for token_helper without shell
  updated `ExternalTokenHelper` documentation
  update `testExternalTokenHelper` to make use of the new `Args` field
  remove shell invocation
  add `Args` field to `ExternalTokenHelper`
  [OT] use `new` builtin for visual clarity
@afresh1 afresh1 requested a review from a team as a code owner February 14, 2025 22:48
@afresh1 afresh1 requested a review from ldilalla-HC February 14, 2025 22:48
Copy link

hashicorp-cla-app bot commented Feb 14, 2025

CLA assistant check
All committers have signed the CLA.

We got a PR ID, so fix the changelog file
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.

Using a shell to exec the token_helper is unreliable
2 participants