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

[ericsampson]

Describe the bug
According to #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

[Nyholm]

I can confirm this issue.

[Briggsdf]

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.

[ericsciple]

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]

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

[Briggsdf]

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

[ericsciple]

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.

[ZebraFlesh]

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.

[ericsampson]

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.

[ZebraFlesh]

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

[ericsciple]

@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.

[ericsampson]

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"

[imjohnbo]

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?

[ericsampson]

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]

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.