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

Optionally output AWS temporary credentials #859

Closed
1 of 2 tasks
valentindeaconu opened this issue Sep 20, 2023 · 7 comments
Closed
1 of 2 tasks

Optionally output AWS temporary credentials #859

valentindeaconu opened this issue Sep 20, 2023 · 7 comments
Labels
feature-request A feature should be added or improved.

Comments

@valentindeaconu
Copy link

Describe the feature

The action should support optionally exporting temporary AWS credentials to the output, so the job that is running the action can export them from the step output to the job output. In this way, we can collect them and pass them to a service container that is running for a subsequent job.

Use Case

We want to run our tests in GitHub Actions pipeline and we have a Docker image that provisions a temporary database. The database needs to be restored from a backup that is currently uploaded in a private S3 bucket. We run the database in a Docker service container and because we need to access the S3 bucket, we have to provide a set of AWS credentials. As we don't want to inject permanent credentials inside the image at build time, the only left solution would be to inject them at runtime from the container environment. In this case, we have two possible solutions: store a pair of permanent credentials in GA repository secrets and inject them at runtime OR generate a pair of temporary credentials by assuming a role in a previous job, then output the credentials from the job and fulfill them in the container environment.

Example configuration:

# ...

jobs:
  login-to-amazon:
    name: Authenticate in AWS
    steps:
      - name: Assume Backup Role
        id: backup-role
        uses: aws-actions/configure-aws-credentials@v1-node16
        with:
          aws-region: 'eu-west-1'
          role-to-assume: 'arn:aws:iam::[ACCOUNT-ID]:role/[ROLE-NAME]'
          role-skip-session-tagging: true
          role-session-name: 'database-backup-e2e-tests'
          role-duration-seconds: 3600
    outputs:
      access_key_id: ${{ steps.backup-role.outputs.access_key_id }}
      secret_access_key: ${{ steps.backup-role.outputs.secret_access_key }}
      session_token: ${{ steps.backup-role.outputs.session_token }}
  run-tests:
    name: Run Tests
    needs: [login-to-amazon]
    services:
      db:
        # ...
        env:
          AWS_ACCESS_KEY_ID: ${{ needs.login-to-amazon.outputs.access_key_id }}
          AWS_SECRET_ACCESS_KEY: ${{ needs.login-to-amazon.outputs.secret_access_key }}
          AWS_SESSION_TOKEN: ${{ needs.login-to-amazon.outputs.session_token }}
          # ...

Proposed Solution

Add an opt-in flag that sets the credentials to the output (e.g. output-temporary-credentials). The flag should be marked with a warning that should inform the user that this is a security vulnerability and should be used with caution, similar to what the amazon-ecr-login action does.

In this case (assuming a role and generating a pair of temporary credentials), the security risk of leaking credentials in GA logs is reduced as the credentials are set to expire in a given amount of time.

Other Information

As described above, for our use case only, there are some workarounds:

  • Injecting permanent credentials from the GitHub Actions Repository Secrets store;
  • Bake in a set of permanent credentials inside the Docker image;
  • Change the image to only accept a link from where the file is to be downloaded, then generate a pre-sign URL in a previous job and only export the pre-signed URL from that particular job.

The first two "solutions" come with the disadvantage of being somewhat hard to rotate the credentials when needed; the third one would require a lot of effort in our case as we use the same image on multiple CI pipelines.

After having a look at the codebase, it looks like the action was built to export those credentials initially, but then the flag never made its way to the action input:

if (outputCredentials) {
if (creds?.AccessKeyId) {
core.setOutput('aws-access-key-id', creds.AccessKeyId);
}
if (creds?.SecretAccessKey) {
core.setOutput('aws-secret-access-key', creds.SecretAccessKey);
}
if (creds?.SessionToken) {
core.setOutput('aws-session-token', creds.SessionToken);
}
}

To implement this feature, all we need to do is to connect an input flag to the outputCredentials parameter of the exportCredentials helper method, and if this flag is set, output a warning message.

Acknowledgements

  • I may be able to implement this feature request
  • This feature might incur a breaking change
@valentindeaconu valentindeaconu added feature-request A feature should be added or improved. needs-triage This issue still needs to be triaged labels Sep 20, 2023
@valentindeaconu valentindeaconu changed the title Optionally allow AWS temporary credentials output Optionally output AWS temporary credentials Sep 20, 2023
@peterwoodworth
Copy link
Contributor

Thanks for the time to submit this feature request,

We do have the output-credentials prop, and we have this documented in our README up at the top and also with an example. This doesn't need any security warning, the security warning you linked is regarding masking your password 🙂

@peterwoodworth peterwoodworth added response-requested Waiting on additional info and feedback. Will move to 'closing-soon' in 5 days. and removed needs-triage This issue still needs to be triaged labels Sep 20, 2023
@valentindeaconu
Copy link
Author

valentindeaconu commented Sep 20, 2023

Thank you for your response. I'm sorry, for some reason I completely missed that (even after looking in code). However, this is not completely fixing the issue in our case (passing them from the step output to the job output) because the credentials are still marked as secrets from the following code:

if (creds?.AccessKeyId) {
core.setSecret(creds.AccessKeyId);
core.exportVariable('AWS_ACCESS_KEY_ID', creds.AccessKeyId);
}
if (creds?.SecretAccessKey) {
core.setSecret(creds.SecretAccessKey);
core.exportVariable('AWS_SECRET_ACCESS_KEY', creds.SecretAccessKey);
}
if (creds?.SessionToken) {
core.setSecret(creds.SessionToken);
core.exportVariable('AWS_SESSION_TOKEN', creds.SessionToken);
} else if (process.env['AWS_SESSION_TOKEN']) {
// clear session token from previous credentials action
core.exportVariable('AWS_SESSION_TOKEN', '');
}

And, because they're marked as secrets, we are not allowed to output them from the job (check the example configuration in the initial message):

run-tests / Authenticate in AWS Skip output 'access_key_id' since it may contain secret.
run-tests / Authenticate in AWS Skip output 'secret_access_key' since it may contain secret.
run-tests / Authenticate in AWS Skip output 'session_token' since it may contain secret.

I think, in this case, we will need one more flag that allows the user to skip the core.setSecret call.

@github-actions github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to 'closing-soon' in 5 days. label Sep 20, 2023
@peterwoodworth
Copy link
Contributor

I think, in this case, we will need one more flag that allows the user to skip the core.setSecret call.

I don't comfortable introducing this as an option, but if you need this and feel confident in a secure implementation and usage of this, there's nothing stopping you from using your own fork with this change implemented 🙂

@peterwoodworth peterwoodworth added the response-requested Waiting on additional info and feedback. Will move to 'closing-soon' in 5 days. label Sep 20, 2023
@valentindeaconu
Copy link
Author

valentindeaconu commented Sep 21, 2023

I don't comfortable introducing this as an option

I understand your concern, that's why I suggested adding it as an opt-in feature, with a warning message that indicates this might be a security vulnerability (similar to what the ecr-login action does about the docker-password).

there's nothing stopping you from using your own fork with this change implemented

There's no need to fork this project and invest in keeping it up-to-date as doing this manually with aws-cli and jq is pretty easy (see the spoiler below), but it feels like reinventing the wheel since this action is already here and it only needs an extra flag which is disabled by default.

Workaround with aws-cli and jq
jobs:
  login-to-amazon:
    name: Authenticate in AWS
    steps:
      - name: Assume Backup Role
        id: backup-role
        env:
          AWS_ROLE_ARN: 'arn:aws:iam::[ACCOUNT-ID]:role/[ROLE-NAME]'
          SESSION_NAME: 'backup-role'
          SESSION_DURATION_SECONDS: '900' # 15 mins
        run: |
          credentials="$(aws sts assume-role --role-arn "$AWS_ROLE_ARN" --role-session-name "$SESSION_NAME" --duration-seconds "$SESSION_DURATION_SECONDS" --output json)"

          access_key_id="$(echo $credentials | jq -r .Credentials.AccessKeyId)"
          secret_access_key="$(echo $credentials | jq -r .Credentials.SecretAccessKey)"
          session_token="$(echo $credentials | jq -r .Credentials.SessionToken)"

          echo "access_key_id=$access_key_id" >> $GITHUB_OUTPUT
          echo "secret_access_key=$secret_access_key" >> $GITHUB_OUTPUT
          echo "session_token=$session_token" >> $GITHUB_OUTPUT
    outputs:
      access_key_id: ${{ steps.backup-role.outputs.access_key_id }}
      secret_access_key: ${{ steps.backup-role.outputs.secret_access_key }}
      session_token: ${{ steps.backup-role.outputs.session_token }}

@github-actions github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to 'closing-soon' in 5 days. label Sep 21, 2023
@peterwoodworth
Copy link
Contributor

peterwoodworth commented Sep 21, 2023

Exactly, I'm not comfortable with the idea of having this as an option. I'd love to trust that no one would ever slip up using it, but I can't. Masking these values is critical for the security of this workflow, I think to opt out of that the user needs to know what they're doing, and they can with a number of workarounds that don't take that long to setup (thanks for posting one that works for you)

@aws-actions aws-actions deleted a comment from Blacknight1260 Sep 21, 2023
@kellertk
Copy link
Contributor

@valentindeaconu Thanks for the suggestion, but we're not interested in implementing this at this time. Credentials output this way could show up in debug logs and it would be too easy for people to accidentally compromise themselves - remember that logs for action runs on public repos are also public. This action exists to populate the standard environment variables that are recognized by the AWS SDKs and related tools; assuming an STS role and then doing other things with the keys like printing them to standard output are out of scope for us. If you need this functionality, you should implement it yourself.

One way is with the AWS CLI like you mentioned. Another way might be to write your own simple SDK client and perform the AssumeRole call yourself. Regardless, because the workarounds are straightforward and because it would be too much of a security issue for some users of this action, we're going to close this.

@kellertk kellertk closed this as not planned Won't fix, can't repro, duplicate, stale Sep 21, 2023
@github-actions
Copy link

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request A feature should be added or improved.
Projects
None yet
Development

No branches or pull requests

3 participants