-
Notifications
You must be signed in to change notification settings - Fork 174
chore: sanitize CI inputs via env var #4528
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR sanitizes CI workflow inputs by replacing direct GitHub Actions input references in shell commands with environment variables, improving security by preventing potential injection attacks.
- Replaces
${{ inputs.* }}
expressions in shell commands with environment variables - Adds
env
blocks to define environment variables from workflow inputs - Updates shell commands to reference environment variables instead of direct inputs
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
.github/workflows/update_ssm.yml | Sanitizes SSM parameter names using PREFIX and PACKAGE_VERSION env vars |
.github/workflows/run-e2e-tests.yml | Sanitizes PR number input for GitHub CLI checkout command |
.github/workflows/reusable_publish_docs.yml | Sanitizes version input and replaces inline GitHub expressions in jq commands |
.github/workflows/publish_layer.yml | Sanitizes CDK context parameter using LAYER_VERSION env var |
.github/workflows/layers_partitions.yml | Sanitizes layer version inputs and environment checks |
.github/workflows/layers_partition_verify.yml | Sanitizes version and partition version inputs |
.github/workflows/layer_balance.yml | Sanitizes region and start_at inputs for balance command |
.github/workflows/bootstrap_region.yml | Sanitizes region input for CDK bootstrap and balance commands |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
layer_output="AWSLambdaPowertoolsTypeScriptV2-${{ matrix.region }}.json" | ||
# Dynamic secret access is safe here - secrets are scoped per environment | ||
aws --region ${{ matrix.region}} lambda get-layer-version-by-arn --arn "arn:${{ needs.setup.outputs.partition }}:lambda:${{ matrix.region}}:${{ secrets[format('AWS_ACCOUNT_{0}', steps.transform.outputs.CONVERTED_REGION)] }}:layer:AWSLambdaPowertoolsTypeScriptV2:${{ steps.partition_version.outputs.partition_version }}" > $layer_output | ||
aws --region "${{ matrix.region }}" lambda get-layer-version-by-arn --arn "arn:${{ needs.setup.outputs.partition }}:lambda:${{ matrix.region }}:${{ secrets[format('AWS_ACCOUNT_{0}', steps.transform.outputs.CONVERTED_REGION)] }}:layer:AWSLambdaPowertoolsTypeScriptV2:${{ steps.partition_version.outputs.partition_version }}" > "$layer_output" |
Check warning
Code scanning / CodeQL
Excessive Secrets Exposure Medium
secrets[format('AWS_ACCOUNT_{0}', steps.transform.outputs.CONVERTED_REGION)]
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 1 day ago
To fix the problem, replace dynamic secret access with static mapping so that each job only receives the secret(s) it requires. Instead of ${{ secrets[format('AWS_ACCOUNT_{0}', steps.transform.outputs.CONVERTED_REGION)] }}
, declare environment-specific and region-specific secrets explicitly for each region/partition combination you intend to use. Then, in your workflow matrix or steps, use conditional logic (if) to select the correct secret and expose it only when needed.
The recommended approach is:
- Predefine a matrix for all supported regions.
- For each region, create explicit environment variables or conditional blocks, mapping the correct region to its static secret name, e.g.,
secrets.AWS_ACCOUNT_CN_NORTH_1
,secrets.AWS_ACCOUNT_US_GOV_WEST_1
, etc. - Use
if:
blocks or environment mapping to set the correct secret value per region. - Replace all usages of
secrets[format(...)]
with the correct explicit secret variable, i.e.,${{ secrets.AWS_ACCOUNT_CN_NORTH_1 }}
. - Similar mapping for
role-to-assume
.
This can be done either by:
- mapping the secret at the matrix level via
include
, or - using
env
per step/job with conditionals.
Edit only lines using dynamic secret access in the file .github/workflows/layers_partition_verify.yml
. No additional imports or method definitions are needed beyond YAML syntax.
-
Copy modified lines R120-R127 -
Copy modified line R140 -
Copy modified line R161
@@ -117,7 +117,14 @@ | ||
environment: ${{ inputs.partition }} ${{ inputs.environment }} | ||
strategy: | ||
matrix: | ||
region: ${{ fromJson(needs.setup.outputs.regions) }} | ||
include: | ||
- region: cn-north-1 | ||
aws_account_secret: AWS_ACCOUNT_CN_NORTH_1 | ||
iam_role_secret: IAM_ROLE_CN_NORTH_1 | ||
- region: us-gov-west-1 | ||
aws_account_secret: AWS_ACCOUNT_US_GOV_WEST_1 | ||
iam_role_secret: IAM_ROLE_US_GOV_WEST_1 | ||
# Add other regions as needed | ||
steps: | ||
- name: Download Metadata | ||
uses: actions/download-artifact@634f93cb2916e3fdff6788551b99b062d0335ce0 # v5.0.0 | ||
@@ -130,7 +137,7 @@ | ||
uses: aws-actions/configure-aws-credentials@a03048d87541d1d9fcf2ecf528a4a65ba9bd7838 # v5.0.0 | ||
with: | ||
# Dynamic secret access is safe here - secrets are scoped per environment | ||
role-to-assume: ${{ secrets[format('IAM_ROLE_{0}', steps.transform.outputs.CONVERTED_REGION)] }} | ||
role-to-assume: ${{ secrets[matrix.iam_role_secret] }} | ||
aws-region: ${{ matrix.region}} | ||
mask-aws-account-id: true | ||
audience: ${{ needs.setup.outputs.aud }} | ||
@@ -151,7 +158,7 @@ | ||
set -euo pipefail | ||
layer_output="AWSLambdaPowertoolsTypeScriptV2-${{ matrix.region }}.json" | ||
# Dynamic secret access is safe here - secrets are scoped per environment | ||
aws --region "${{ matrix.region }}" lambda get-layer-version-by-arn --arn "arn:${{ needs.setup.outputs.partition }}:lambda:${{ matrix.region }}:${{ secrets[format('AWS_ACCOUNT_{0}', steps.transform.outputs.CONVERTED_REGION)] }}:layer:AWSLambdaPowertoolsTypeScriptV2:${{ steps.partition_version.outputs.partition_version }}" > "$layer_output" | ||
aws --region "${{ matrix.region }}" lambda get-layer-version-by-arn --arn "arn:${{ needs.setup.outputs.partition }}:lambda:${{ matrix.region }}:${{ secrets[matrix.aws_account_secret] }}:layer:AWSLambdaPowertoolsTypeScriptV2:${{ steps.partition_version.outputs.partition_version }}" > "$layer_output" | ||
REMOTE_SHA=$(jq -r '.Content.CodeSha256' $layer_output) | ||
LOCAL_SHA=$(jq -r '.Content.CodeSha256' AWSLambdaPowertoolsTypeScriptV2.json) | ||
test "$REMOTE_SHA" == "$LOCAL_SHA" && echo "SHA OK: ${LOCAL_SHA}" || exit 1 |
export layer_output="AWSLambdaPowertoolsTypeScriptV2-${{ matrix.region }}.json" | ||
# Dynamic secret access is safe here - secrets are scoped per environment | ||
aws --region ${{ matrix.region}} lambda get-layer-version-by-arn --arn 'arn:${{ needs.setup.outputs.partition }}:lambda:${{ matrix.region}}:${{ secrets[format('AWS_ACCOUNT_{0}', steps.transform.outputs.CONVERTED_REGION)] }}:layer:AWSLambdaPowertoolsTypeScriptV2:${{ env.LAYER_VERSION }}' > $layer_output | ||
aws --region "${{ matrix.region }}" lambda get-layer-version-by-arn --arn "arn:${{ needs.setup.outputs.partition }}:lambda:${{ matrix.region }}:${{ secrets[format('AWS_ACCOUNT_{0}', steps.transform.outputs.CONVERTED_REGION)] }}:layer:AWSLambdaPowertoolsTypeScriptV2:${LAYER_VERSION}" > "$layer_output" |
Check warning
Code scanning / CodeQL
Excessive Secrets Exposure Medium
secrets[format('AWS_ACCOUNT_{0}', steps.transform.outputs.CONVERTED_REGION)]
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 1 day ago
To fix this problem, avoid dynamic (runtime) references to secrets. Instead, enumerate the possible environment/partition/region combinations and statically assign secrets for each one, ensuring that only the necessary secret is referenced in a given job or matrix entry. In a matrix job, you can "branch" on known values (e.g., matrix.region
) and set the required secret in static env:
blocks. For example, have an explicit mapping:
env:
AWS_ACCOUNT: ${{ secrets.AWS_ACCOUNT_CN1 }}
for region cn-north-1
, and so on for each allowed region value.
For this file, replace the dynamic ${{ secrets[format(...)...] }}
with static secret references, using the if:
conditional on steps so that only the relevant secret is used for each region. This requires one step per unique region, each with its explicit secret reference, rather than a single, dynamic step.
Steps:
- For each possible value that
steps.transform.outputs.CONVERTED_REGION
could produce, add a separate- name:
step with an explicit (static) secret reference for that region. - Use
if:
conditional to select the step based on thesteps.transform.outputs.CONVERTED_REGION
output and/or on thematrix.region
. - For each explicit region, use the appropriate
${{ secrets.AWS_ACCOUNT_REGION_NAME }}
. - Optionally, if there are many regions, you can group similar ones or use reusable workflows, but always via static secret names.
Files/regions to change:
- Only
.github/workflows/layers_partitions.yml
, specifically the step at line 194, must be modified to avoid dynamic secret referencing.
What's needed:
- Multiple steps (one per region), each referencing the correct secret explicitly and gated by an
if:
clause. - Or, if matrix is small, set an environment variable in the matrix/strategy definition (with explicit mapping).
-
Copy modified lines R186-R188 -
Copy modified line R195 -
Copy modified lines R203-R227
@@ -183,15 +183,16 @@ | ||
# 2. Description validation - confirms the version number in the description matches the source | ||
# 3. Layer Version number verification - validates that the layer version numbers match between source and target | ||
# 4. Tabular comparison output - displays side-by-side comparison of key layer properties | ||
- name: Verify Layer | ||
# Verify Layer for cn-north-1 | ||
- name: Verify Layer (cn-north-1) | ||
if: steps.transform.outputs.CONVERTED_REGION == 'CN1' | ||
env: | ||
LAYER_VERSION: ${{ steps.create-layer.outputs.LAYER_VERSION }} | ||
ENVIRONMENT: ${{ inputs.environment }} | ||
run: | | ||
set -euo pipefail | ||
export layer_output="AWSLambdaPowertoolsTypeScriptV2-${{ matrix.region }}.json" | ||
# Dynamic secret access is safe here - secrets are scoped per environment | ||
aws --region "${{ matrix.region }}" lambda get-layer-version-by-arn --arn "arn:${{ needs.setup.outputs.partition }}:lambda:${{ matrix.region }}:${{ secrets[format('AWS_ACCOUNT_{0}', steps.transform.outputs.CONVERTED_REGION)] }}:layer:AWSLambdaPowertoolsTypeScriptV2:${LAYER_VERSION}" > "$layer_output" | ||
aws --region "${{ matrix.region }}" lambda get-layer-version-by-arn --arn "arn:${{ needs.setup.outputs.partition }}:lambda:${{ matrix.region }}:${{ secrets.AWS_ACCOUNT_CN1 }}:layer:AWSLambdaPowertoolsTypeScriptV2:${LAYER_VERSION}" > "$layer_output" | ||
REMOTE_SHA=$(jq -r '.Content.CodeSha256' $layer_output) | ||
LOCAL_SHA=$(jq -r '.Content.CodeSha256' AWSLambdaPowertoolsTypeScriptV2.json) | ||
test "$REMOTE_SHA" == "$LOCAL_SHA" && echo "SHA OK: ${LOCAL_SHA}" || exit 1 | ||
@@ -205,6 +200,31 @@ | ||
fi | ||
jq -s -r '["Layer Arn", "Runtimes", "Version", "Description", "SHA256"], ([.[0], .[1]] | .[] | [.LayerArn, (.CompatibleRuntimes | join("/")), .Version, .Description, .Content.CodeSha256]) |@tsv' AWSLambdaPowertoolsTypeScriptV2.json $layer_output | column -t -s $'\t' | ||
|
||
# Verify Layer for cn-northwest-1 | ||
- name: Verify Layer (cn-northwest-1) | ||
if: steps.transform.outputs.CONVERTED_REGION == 'CN2' | ||
env: | ||
LAYER_VERSION: ${{ steps.create-layer.outputs.LAYER_VERSION }} | ||
ENVIRONMENT: ${{ inputs.environment }} | ||
run: | | ||
set -euo pipefail | ||
export layer_output="AWSLambdaPowertoolsTypeScriptV2-${{ matrix.region }}.json" | ||
aws --region "${{ matrix.region }}" lambda get-layer-version-by-arn --arn "arn:${{ needs.setup.outputs.partition }}:lambda:${{ matrix.region }}:${{ secrets.AWS_ACCOUNT_CN2 }}:layer:AWSLambdaPowertoolsTypeScriptV2:${LAYER_VERSION}" > "$layer_output" | ||
REMOTE_SHA=$(jq -r '.Content.CodeSha256' $layer_output) | ||
LOCAL_SHA=$(jq -r '.Content.CodeSha256' AWSLambdaPowertoolsTypeScriptV2.json) | ||
test "$REMOTE_SHA" == "$LOCAL_SHA" && echo "SHA OK: ${LOCAL_SHA}" || exit 1 | ||
REMOTE_DESCRIPTION=$(jq -r '.Description' $layer_output) | ||
LOCAL_DESCRIPTION=$(jq -r '.Description' AWSLambdaPowertoolsTypeScriptV2.json) | ||
test "$REMOTE_DESCRIPTION" == "$LOCAL_DESCRIPTION" && echo "Version number OK: ${LOCAL_DESCRIPTION}" || exit 1 | ||
if [ "$ENVIRONMENT" == "Prod" ]; then | ||
REMOTE_LAYER_VERSION=$(jq -r '.LayerVersionArn' $layer_output | sed 's/.*://') | ||
LOCAL_LAYER_VERSION=$(jq -r '.LayerVersionArn' AWSLambdaPowertoolsTypeScriptV2.json | sed 's/.*://') | ||
test "$REMOTE_LAYER_VERSION" == "$LOCAL_LAYER_VERSION" && echo "Layer Version number OK: ${LOCAL_LAYER_VERSION}" || exit 1 | ||
fi | ||
jq -s -r '["Layer Arn", "Runtimes", "Version", "Description", "SHA256"], ([.[0], .[1]] | .[] | [.LayerArn, (.CompatibleRuntimes | join("/")), .Version, .Description, .Content.CodeSha256]) |@tsv' AWSLambdaPowertoolsTypeScriptV2.json $layer_output | column -t -s $'\t' | ||
|
||
# Add similar steps for other regions/partitions as required, each with explicit static secret reference and 'if' gating. | ||
|
||
- name: Store Metadata - ${{ matrix.region }} | ||
uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4.6.2 | ||
with: |
|
@sdangol you worked on the secrets access last, can you please check if Copilot's suggestions are appropriate? If not we can dismiss them together with the findings that might pop up after merging. |
@dreamorosi Those are the same ones that we had discussed last time about the issue being mitigated by the use of environment specific secrets. So, the findings are a false positive. |
Yes, I know, we had to revert them because the solution we came up with didn't work. I was wondering if Copilot's solution is better and might work. |
The suggested solution will need some adjustments to make it work like the checking for the |
Ok, thanks for checking - I remember you mentioning this in the previous iteration as well. Happy to ignore the suggestions and merge whenever you're ready to approve (tomorrow after the release is fine). |
Summary
Changes
This PR updates several workflows to use environment variables to temporarily store workflow inputs, before passing them to the command and expanding them as a string. For example,
${{ input.version }}
now becomes$VERSION
with the variable being stored underenv
.The PR also adds double quotes in multiple places to ensure string expansion works as intended.
Issue number: closes #4529
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.