When using `exec` there exists the possibility of leaking secret data

[jtslear]

trafficstars

Sometime between versions 0.116.0 and 0.125.1 the output has been improved, but with a drawback.

Let's say we utilize the exec function inside of a template. In the below example we'll use curl to find information. If curl requires authentication in order to grab the information required, we'll end up printing that authentication header in the output. This is safe for local testing, but not in a production CI/CD pipeline. Example failed output:

in ./helmfile.yaml: in .helmfiles[1]: in releases/gitlab/helmfile.yaml: failed processing release gitlab: failed to render values files "values/values-from-external-sources.yaml.gotmpl": failed to render [values/values-from-external-sources.yaml.gotmpl], because of template: stringTemplate:7:45: executing "stringTemplate" at <exec "curl" (list "-s" "--retry" "5" "-H" (print "PRIVATE-TOKEN:" $gitlab_ops_api_token) (print "https://ops.gitlab.net/api/v4/projects/139/repository/files/roles%2F" (.Environment.Values | getOrNil "gitlab_chef_source_name" | default .Environment.Name) "-base-db-redis-sentinel-cache.json/raw?ref=master"))>: error calling exec: command "/usr/bin/curl" exited with non-zero status:

PATH:
  /usr/bin/curl

ARGS:
  0: curl (4 bytes)
  1: -s (2 bytes)
  2: --retry (7 bytes)
  3: 5 (1 bytes)
  4: -H (2 bytes)
  5: PRIVATE-TOKEN:<SECRET_EXPOSED> (34 bytes)
  6: https://ops.gitlab.net/api/v4/projects/139/repository/files/roles%2Fpre-base-db-redis-sentinel-cache.json/raw?ref=master (120 bytes)

ERROR:
  exit status 35

EXIT STATUS
  35

In the above example, I don't care why curl failed, I care primarily that we output our arguments, which included a secret token.

This occurs during a helmfile --log-level info --suppress-secrets apply. helmfile doesn't know it's exposing a secret, suppress-secrets is meant for Kubernetes objects anyways, so this option isn't helpful as expected.

I propose that these detailed messages be output only when utilizing log-level debug. In our use case, the CI/CD pipeline explicitly prevents helmfile from operating with this level of logging as it can potentially expose secrets even with the suppress-secrets flag. This option may not be suitable for all situations, so perhaps a bit of documentation detailing that troubleshooting exec with a high log level will be required. Having this information is certainly handy when troubleshooting exec, so I don't want it to be removed. But if there's a way to we can prevent exec arguments which may contain sensitive data from being exposed unintentionally, this will benefit many.

[mumoshu]

@jtslear Hey! Thank you so much for the detailed feedback.

Enabling the detailed error reporting only when log-level=debug might be a possible solution.

But I'm not yet sure how might be used in practice. I think a hook run error can be either permanent or transient. If it's permanent, an error message like error calling exec: command "/usr/bin/curl" exited with non-zero status: please rerun helmfile with --debug to see the detailed error report help. But for transient errors, instructions like that doesn't help the user, as rerunning helmfile may or may not reproduce the error.

So my suggestion is to enhance Helmfile to set secrets that can be used from within the hook args.

  hooks:
  - events: ["postsync"]
    command: "curl"
    args:
    - -s
    - --retry
    - "5"
    - -H
    - PRIVATE_TOKEN:${PRIVATE_TOKEN}
    - ...
    secrets:
      PRIVATE_TOKEN: somesecretvalue

Helmfile will replace {$PRIVATE_TOKEN} immediately before running the hook command. As Helmfile now knows which part of the arg has the token contained, it can now emit a detailed error with the args before the replacement:

PATH:
  /usr/bin/curl

ARGS:
  0: curl (4 bytes)
  1: -s (2 bytes)
  2: --retry (7 bytes)
  3: 5 (1 bytes)
  4: -H (2 bytes)
  5: PRIVATE-TOKEN:${PRIVATE_TOKEN} (34 bytes)
  6: https://ops.gitlab.net/api/v4/projects/139/repository/files/roles%2Fpre-base-db-redis-sentinel-cache.json/raw?ref=master (120 bytes)

ERROR:
  exit status 35

EXIT STATUS
  35

Otherwise, explicitly showing it as "redacted" to let the user confirm that helmfile did replace the value for run may be better:

PATH:
  /usr/bin/curl

ARGS:
  0: curl (4 bytes)
  1: -s (2 bytes)
  2: --retry (7 bytes)
  3: 5 (1 bytes)
  4: -H (2 bytes)
  5: PRIVATE-TOKEN:**REDACTED BY HELMFILE** (34 bytes)
  6: https://ops.gitlab.net/api/v4/projects/139/repository/files/roles%2Fpre-base-db-redis-sentinel-cache.json/raw?ref=master (120 bytes)

ERROR:
  exit status 35

EXIT STATUS
  35

WDYT?

[jtslear]

I enjoy your suggestion. I would prefer the former output, that way one can compare variable names to ensure one does not accidentally typo it.

My proposal I thought would be a good quick fix, but your reasoning for avoiding such is very much a common theme.

[cdunford]

I like this approach; I have a few questions/comments;

1. I notice you mention hooks, but we're talking about exec; I assume somehow we would leverage this for both? I guess the one thing that isn't clear is how this would be used for exec - the example provided is a postsync hook; exec is typically a single inline template function call and would likely need different syntax to provide secret values.

2. I would hope the plan would be to mask the secret value in stdout/stderr as well? (for example if showlogs is true for the hook and something echos the secret value).

[jswager-cisco]

This is a good feature request - had something similar happen when the command failed and dumped a token to the output.

But I see this issue hasn't been duplicated to https://github.com/helmfile/helmfile/issues - is there a quick way to do that? Or just open a whole new issue with the contents copied?