runner icon indicating copy to clipboard operation
runner copied to clipboard

usage of add-mask still echoes the value to the log

Open ericsampson opened this issue 5 years ago • 59 comments

Describe the bug According to https://github.com/actions/runner/issues/159, the issue where the add-mask workflow command echoes/leaks the secret was supposed to be fixed, but we still observe it. This was also mentioned on the GitHub forum by a Partner

To Reproduce Steps to reproduce the behavior: echo "::add-mask::${{ steps.mystep.outputs.myvalue }}"

Expected behavior raw output is not echoed to the log

Runner Version and Platform Hosted Ubuntu

ericsampson avatar May 11 '20 22:05 ericsampson

I can confirm this issue.

Nyholm avatar May 17 '20 16:05 Nyholm

Have there been any updates(Or workarounds) on this? It seems like they recently pushed a fix for this, but it doesn't seem to have resolved the issue.

Briggsdf avatar May 28 '20 22:05 Briggsdf

The mask needs to be added before the output is registered.

Otherwise at the time when the inputs are evaluated for the next step, the value is not yet registered as a secret. It doesn't get registered until the next step executes. When the step inputs are logged, it hasn't yet been registered as a secret.

ericsciple avatar May 28 '20 22:05 ericsciple

We may need to detect this specific case and error early, rather than evaluate the expression.

ericsciple avatar May 28 '20 22:05 ericsciple

OK, at what point in the workflow would that be?

Briggsdf avatar May 28 '20 22:05 Briggsdf

In the example from the description, the step mystep sets the output. It should register the mask value before setting the output.

The output is streaming, so too late if value is already printed.

ericsciple avatar May 28 '20 22:05 ericsciple

The mask needs to be added before the output is registered

Given the following example using AWS credentials and the setup-terraform action:

    - name: Terraform Output - Access Key ID
      id: terraform-output-1
      run: terraform output access_key_id
    - name: Terraform Output - Secret Key
      id: terraform-output-2
      run: terraform output secret_access_key
    - run: |
        echo "::add-mask::${{ steps.terraform-output-1.outputs.stdout }}"
        echo "::add-mask::${{ steps.terraform-output-2.outputs.stdout }}"
        echo "::set-env name=test_key_id::${{ steps.terraform-output-1.outputs.stdout }}"
        echo "::set-env name=test_secret::${{ steps.terraform-output-2.outputs.stdout }}"

I tried changing the masking to the following:

    - run: |
        echo "::add-mask::${{ steps.terraform-output-1.outputs.stdout }}"
        echo "::add-mask::${{ steps.terraform-output-2.outputs.stdout }}"
    - name: Capture Terraform Outputs
      run: |
        echo "::set-env name=test_key_id::${{ steps.terraform-output-1.outputs.stdout }}"
        echo "::set-env name=test_secret::${{ steps.terraform-output-2.outputs.stdout }}"

But my AWS secrets are still showing up in the logs. Those values are dynamic and not known until terraform outputs them, so I'm not sure what else I could do.

ZebraFlesh avatar May 28 '20 23:05 ZebraFlesh

It doesn't get registered until the next step executes. When the step inputs are logged, it hasn't yet been registered as a secret.

Regardless of anything else, this should be added to the documentation for add-mask. There's no mention that it's only effective for the step after it's registered.

ericsampson avatar May 28 '20 23:05 ericsampson

I guess I’m back to my original post then: what is the point of add-mask if it inherently exposes secrets?

ZebraFlesh avatar May 29 '20 01:05 ZebraFlesh

@ericsampson sorry bad phrasing on my part. I was referring to the specific example when I said that. The statement on it's own is not true.

The add-mask command takes effect from the moment it is processed. The step writes the command over stdout to the runner. From the moment the runner processes the line, all future lines will be scrubbed.

@ZebraFlesh @ericsampson here is an example how add-mask should be used with secret outputs:

on: push
jobs:
  my-job:
    runs-on: ubuntu-latest
    steps:
      - id: sets-a-secret
        run: |
          the_secret=$((RANDOM))
          echo "::add-mask::$the_secret"
          echo "::set-output name=secret-number::$the_secret"
      - run: |
          echo "the secret number is ${{ steps.sets-a-secret.outputs.secret-number }}"

If you set ACTIONS_STEP_DEBUG you will see lots of additional output.

The secret value will be masked everywhere. If you remove the add-mask command, you will see the secret value printed in many places - even when the set-output command is processed. So it is critical the add-mask command comes first.

Hope that explanation helps. Let me know.

ericsciple avatar May 29 '20 04:05 ericsciple

Thanks @ericsciple!

I was able to do a little playing around, and have a modified version of your example that shows what I'm experiencing. Pretend that the the_secret output below in init is set by something like an Terraform action as in the more realistic examples above. With the following code, the local_secret line does print the secret to the log:

on:
  pull_request:
jobs:
  my-job:
    runs-on: ubuntu-latest
    steps:
      - id: init
        run: |
          echo "::set-output name=the_secret::$((RANDOM))"
      - id: sets-a-secret
        run: |
          local_secret="${{ steps.init.outputs.the_secret }}"
          echo "::add-mask::$local_secret"
          echo "::set-output name=secret-number::$local_secret"
      - run: |
          echo "the secret number is ${{ steps.sets-a-secret.outputs.secret-number }}"     

So it seems to me like the issue is in the context expansion? It seems like it's being expanded first and then printed to the log, whereas something like $MYVAR or $(EXPRESSION) is printed to the log as is and expand later (which is desirable in the case of secrets). Is there a way to write the context expression to avoid this early expansion? I also tried it without the local, and just inline in the add-mask, but that didn't help:

... 
        run: |
          echo "::add-mask::${{ steps.init.outputs.the_secret }}"
          echo "::set-output name=secret-number::$local_secret"

ericsampson avatar May 29 '20 05:05 ericsampson

Hi @ericsampson! Just taking a look from the Terraform perspective and wanting to confirm this behavior isn't only after setup-terraform – from your example here, it looks like it's general?

imjohnbo avatar May 29 '20 14:05 imjohnbo

Hi @imjohnbo, I'm still coming up to speed on GH Actions, but AFAICT this is a platform thing and not specific to Terraform. It just tends to get exposed (and mentioned) by people because this scenario naturally happens often when using Terraform (or other IaC). My latest example uses no 3rd-party Actions, just the built in platform stuff, and still demonstrates it. And also the first time I came across this, I wasn't using setup-terraform. Unless I'm just out to lunch. Thanks for caring!

ericsampson avatar May 29 '20 15:05 ericsampson

Yes, that latest example/repo steps is fully self-contained and runnable, I didn't run it with a step using setup-terraform or other 3rd-party actions that could have messed up some internal Action runner state/settings, and then trim that out of the code shown here. That code sample demonstrates what I'm seeing.

ericsampson avatar May 29 '20 15:05 ericsampson

@imjohnbo what do you think about the terraform wrapper supporting a -secret argument?

Puts the burden on the caller to indicate whether they expect the output to be a secret.

For example:

  steps:
    - run: terraform -secret output secret_access_key

The terraform wrapper would:

  • Intercept the -secret flag and not pass it to the underlying command (terraform output secret_accesskey)
  • Echo ::add-mask::the secret value before echoing ::set-output name=stdout::the secret value

I skimmed the terraform CLI docs. Doesn't look like any commands accept a -secret flag today.

ericsciple avatar May 29 '20 18:05 ericsciple

this isn't limited to Terraform though, that's adressing one symptom rather than the issue which could come from a multitude of sources. It seems like it's not possible to mask any arbitrary context expression? Unless I'm mistaken and there is a way for the user to achieve this (without requiring every Action author everywhere to support inbuilt masking, which seems unrealistic).

ericsampson avatar May 29 '20 18:05 ericsampson

this isn't limited to Terraform though, that's adressing one symptom rather than the issue which could come from a multitude of sources. It seems like it's not possible to mask any arbitrary context expression? Unless I'm mistaken and there is a way for the user to achieve this (without requiring every Action author everywhere to support inbuilt masking, which seems unrealistic).

Agreed, the only sense I can make of this is that they want you to lock in with Github Secrets. But trying to mask anything in the GitHub context seems undoable. In my case I'm trying to mask a value sent in a Webhooks payload ${{github.event.client_payload.some_value}}

Briggsdf avatar May 29 '20 19:05 Briggsdf

Thanks all for the feedback!

There are two ways to register a secret today:

  1. When you use a repository secret, it gets masked automatically
  2. Otherwise need to echo add-mask before outputting a secret. For example, if using set-output, need to echo add-mask first.

I would advise against adding secrets into the github.event.client_payload. That is a good feature request though.

/cc @chrispat fyi regarding ^ feature request

ericsciple avatar May 29 '20 20:05 ericsciple

Thanks Eric, but how do you do point 2 for arbitrary context expressions? I'd love to understand, but either I'm missing something or we're talking by each other.

FWIW, regarding the Terraform Action, there is already a way in TF code to mark fields as containing sensitive/secret information, it just needs to get supported int the setup-terraform Action - no extra output argument required. @ZebraFlesh has already opened up this PR for that.

ericsampson avatar May 29 '20 21:05 ericsampson

@ZebraFlesh has already opened up this PR for that.

Issue, not PR šŸ˜› edit: I also think it unlikely that it will be solved in that action. It seems to invalidate the entire design of the setup-terraform action (thin wrapper that just forwards things as outputs; now it needs to be smart and not so thin).

ZebraFlesh avatar May 29 '20 21:05 ZebraFlesh

@ericsampson thank you for your patience

how do you do point 2 for arbitrary context expressions?

Sorry but it is not possible today. If the value is already in the context, it is too late.

Here are a few examples to illustrate. I'll summarize at the bottom.

Example 1: Debug output from set-output

This example illustrates why add-mask after set-output is too late. First, enable step debug logging. Then run:

on: push
jobs:
  my-job:
    runs-on: ubuntu-latest
    steps:
      - id: sets-a-secret
        run: |
          echo "::set-output name=my-secret::$((RANDOM))"

Because debug logging is on, and because add-mask was not echoed first, the log contains:

::set-output name=my-secret::31487

Example 2: Action input with expression

Here is another example (debug logging not required):

on: push
jobs:
  my-job:
    runs-on: ubuntu-latest
    steps:
      - id: sets-a-secret
        run: |
          echo "::set-output name=my-secret::$((RANDOM))"
      - uses: actions/checkout@v2
        with:
          not-a-real-input: ${{ steps.sets-a-secret.outputs.my-secret }}

Near the top of the logs for the checkout step, expand the folded line Run actions/checkout@v2 to see the inputs for the step. The inputs are printed to the log before the step executes. Because add-mask was not echoed, the log contains the secret in plain text.

Run actions/checkout@v2
  with:
    not-a-real-input: 32467
    repository: ericsciple/testing
    token: ***
    ssh-strict: true
    persist-credentials: true
    clean: true
    fetch-depth: 1
    lfs: false
    submodules: false

Example 3: run script with expression

on: push
jobs:
  my-job:
    runs-on: ubuntu-latest
    steps:
      - id: sets-a-secret
        run: |
          echo "::set-output name=my-secret::$((RANDOM))"
      - name: My script only contains bash comments
        run: |
          # comment 1
          # comment 2 ${{ steps.sets-a-secret.outputs.my-secret }}

Near the top of the logs for the second step, expand the folded line Run # comment 1 to see the inputs for the step. The inputs are printed to the log before the script executes. Because add-mask was not echoed, the log contains the secret in plain text.

Run # comment 1
  # comment 1
  # comment 2 26441
  shell: /bin/bash -e {0}

Example 4: Default display name for a run step

on: push
jobs:
  my-job:
    runs-on: ubuntu-latest
    steps:
      - id: sets-a-secret
        run: |
          echo "::set-output name=my-secret::$((RANDOM))"
      - run: |
          echo default step display name will the secret ${{ steps.sets-a-secret.outputs.my-secret }}

In the web UI you will see Run echo default step display name will the secret 31510. The default display name is calculated before the script executes. Because add-mask was not echoed, the default display contains the secret in plain text.

Example 5: Debug output from expression evaluation

First, enable step debug logging. Then run:

on: push
jobs:
  my-job:
    runs-on: ubuntu-latest
    steps:
      - id: sets-a-secret
        run: |
          echo "::set-output name=my-secret::$((RANDOM))"
      - run: |
          echo hello
        env:
          MY_SECRET: ${{ steps.sets-a-secret.outputs.my-secret }}

Because debug logging is on, and because add-mask was not echoed first, the log contains debug output from the expression evaluation:

##[debug]Evaluating: steps.sets-a-secret.outputs.my-secret
[...]
##[debug]Result: '16195'

Summary

The key factors why add-mask must be called first:

  • Logs are streaming to the web console (the runner scrubs the lines before sending)
  • Context values can be printed to the log for a variety of reasons (critical add-mask echoed prior):
    • When step debug logging is enabled, the set-output command is logged (example 1 above)
    • Action inputs (i.e. uses) are printed to the log (example 2 above)
    • Inline scripts (i.e. run) are printed to the log (example 3 above)
    • Default display name for inline scripts (i.e. run) is calculated before the script executes (example 4 above)
    • Debug output from expression evaluation (example 5 above)

Hope that helps. Feedback is welcome.

I need to look over the docs, and see whether the guidance can be improved. Open to ideas and I can pass along.

I also wonder whether we should add a secret parameter for the set-output command. It may guide folks down the correct path. For example:

echo "::set-output name=my-secret, secret=true::the value"

ericsciple avatar May 29 '20 23:05 ericsciple

(edited the above comment, added 2 more examples)

ericsciple avatar May 31 '20 23:05 ericsciple

Thanks very much for the detailed information @ericsciple.

  1. I'm going to open up a new feature request issue to add a new add-mask bool to set-output, to match the syntax of the standalone add-mask command: echo "::set-output name=my-secret, add-mask=true::the value" This will help make it so that every Action author (like the TF action) isn't forced to reimplement this. They can if it makes sense for their application and they choose to, but if not there is always the fallback for the user. 1b) this will still not cover every case where a person might want to mask content in the wider non-output action context, like @Briggsdf's use case of webhooks. There are probably others.

  2. I think it would be very valuable to add more information to the documentation somewhere to describe what gets executed before the script gets run, along the lines of your bullet points. Because this isn't obvious at all, without laboriously playing with scenarios : )

Along sort of similar lines, is there any mention in the documentation on the ability to run sub-shells in inline scripts? I only stumbled upon that in a Forum posting when trying to figure out how to do something.

ericsampson avatar Jun 01 '20 23:06 ericsampson

Accidentally discovered the following undocumented feature that can be used as a workaround for masking sensitive data. GitHub Actions appears to automatically mask inputs / environment variables following certain naming conventions. For instance, a plaintext variable named WEBHOOK_TOKEN holding a JWT is masked same way as encrypted secrets would. It would be great to officially document this behavior along with the supported keywords to make it safe to rely upon.

GitHub Action configuration:

name: Test
on:
  workflow_dispatch:
    inputs:
      WEBHOOK_URL:
        description: 'Webhook URL'
        required: true
      WEBHOOK_METHOD:
        description: 'Webhook method'
        required: true
        default: 'GET'
      WEBHOOK_TOKEN:
        description: 'Webhook token'
        required: true
jobs:
  test:
    name: Test sensitive data masking
    runs-on: ubuntu-latest
    env:
      WEBHOOK_URL: ${{ github.event.inputs.WEBHOOK_URL }}
      WEBHOOK_METHOD: ${{ github.event.inputs.WEBHOOK_METHOD }}
      WEBHOOK_TOKEN: ${{ github.event.inputs.WEBHOOK_TOKEN }}
    steps:
      - name: Notify job start
        run: |
          curl -s -o /dev/null -w "%{http_code}\n" \
            -X "$WEBHOOK_METHOD" "$WEBHOOK_URL" \
            -H "Authorization: Bearer $WEBHOOK_TOKEN"

GitHub Action log: github_action_log_mask_token

sshymko avatar Dec 10 '20 06:12 sshymko

Is there any progress on this? Its been almost a year and a half since it was originally identified.

edeandrea avatar Dec 13 '21 22:12 edeandrea

Thank you for mention about this issue. I'm stuck with same problem and thank you all for your answers.

I just want to read some secret from encrypted files (like SOPS) and hide it.

Here is my solution,

steps:
  - uses: actions/checkout@v2
  - name: Mask Password
    id: mark_password
    run: |
      secret=`cat "README.md"`
      echo "::add-mask::$secret"
      echo "::set-output name=password::$secret"
  - run: echo "${{ steps.mark_password.outputs.password }}"

Here is result:

image

I think at the time in the step that we setting add-mask, somehow we should don't show the actual value on the console.

I hope this might be help.

mildronize avatar Apr 08 '22 10:04 mildronize

I think the most elegant solution would be to introduce a new Workflow command called set-secret. It would work the same way like set-output but it would not be outputed into logs by default:

jobs:
  job1:
    runs-on: ubuntu-latest
    steps:
      - id: set_pass
        run: |
          secret=$((RANDOM))
          echo "::set-secret name=password::$secret"

      - run: |
          first_command --password '${{ steps.set_pass.secrets.password }}'

  job2:
    runs-on: ubuntu-latest
    needs: job1
    steps:
      - run: |
          second_command --password '${{ needs.job1.secrets.password }}'

      - run: |
          third_command
        env:
          PASSWORD: ${{ needs.job1.secrets.password }}

jtyr avatar Apr 29 '22 23:04 jtyr

I think the most elegant solution would be to introduce a new Workflow command called set-secret. It would work the same way like set-output but it would not be outputed into logs by default:

... and it would be great, if it would then allow to set a secret to a variable (e.g. during multiple ifs, detecting an environment). This way, you don't have to replicate e.g. the Azure login command, but reference the detected login credentials from the set-secret variable.

stefan-schilling avatar May 04 '22 17:05 stefan-schilling

echo "::add-mask::${{ needs.joba.outputs.foo }}" evaluates to a string, then I get the value in 2 separate echo outputs. My syntax off?

echo "ACTIONS_RUNNER_DEBUG=:$ACTIONS_RUNNER_DEBUG" resulted in ACTIONS_RUNNER_DEBUG=:\n being logged so I don't think that's a factor, but my attempts to mask become the revealing.

echo ::add-mask::${{ needs.previousjob.outputs.mysecret }} results in the following getting logged (I truncated for this post).

2022-05-09T19:06:45.7571168Z ##[group]Run echo ::add-mask::Z2hzX3Bj*truncated*==
2022-05-09T19:06:45.7571962Z echo ::add-mask::Z2hzX3Bj*truncated*==

set-secret is no better. It exposes the value in the very call, in the same way. What else might I have missed?

rainabba avatar May 09 '22 18:05 rainabba

I do find some strange stuff happening with masking.

The masking behaviour of the title depends on how you 'fetch' the secret value. The pipeline code is a prove of concept and i don't think people will use it like this but it shows the strange behaviour of masking, it is not always masking.

Here are the steps parts of the pipeline code

      - run: echo ::set-output name=key::secret-value
        id: setter

      - name: create mask 
        id: test-secret
        run: |
          echo ::add-mask::${{steps.setter.outputs.key}}
      
      - run: echo secret-value # title is the same as the run and is not masked

      - run: echo ${{steps.setter.outputs.key}}

      - name: run with name # value in run is masked without reference to the key
        run: echo secret-value

      - name: run with secret-value # title is not masked, value is masked
        run: echo secret-value

      - name: run with ${{steps.setter.outputs.key}} # title is masked, value also
        run: echo secret-value

      - run: echo secret-value ${{steps.setter.outputs.key}} # both values are masked

Screenshot of output image

AtzeDeVries avatar Jun 22 '22 11:06 AtzeDeVries