Add new ConfigureAwsStaticCredentials rule to detect static AWS credentials in GitHub Actions - #18
Add new ConfigureAwsStaticCredentials rule to detect static AWS credentials in GitHub Actions#18cosmosbit wants to merge 9 commits into
Conversation
…WS credentials in workflows
| on_job %( | ||
| get_key($job.env, "AWS_ACCESS_KEY_ID") =~ "{{.*secrets\..*" || | ||
| get_key($job.env, "AWS_ACCESS_KEY_ID") =~ "{{.*env\..*" || | ||
| get_key($job.env, "AWS_ACCESS_KEY_ID") =~ "{{.*vars\..*" || | ||
| get_key($job.env, "AWS_ACCESS_KEY_ID") =~ "AKIA.*" | ||
| ), highlight: "env.AWS_ACCESS_KEY_ID" |
There was a problem hiding this comment.
So after running this, what do you think about flagging JUST AWS_SECRET_ACCESS_KEY? I think that would make the rule shorter, and I think there may be cases where AWS_ACCESS_KEY_ID could be fine to come from a static value since it's not secret
There was a problem hiding this comment.
We need both checks because neither variable alone tells the full story.
If we only looked at AWS_SECRET_ACCESS_KEY, we would miss static creds that show up as AWS_ACCESS_KEY_ID from secrets, vars, or env, or hardcoded AKIA values, without the secret sitting next to it in the same YAML. That is common with configure-aws-credentials (two separate inputs), job or step env, and shell scripts. The access key ID is often what people set first, and it still signals long lived static auth even when the secret is stored elsewhere.
If we only looked at AWS_ACCESS_KEY_ID, we would miss the half that actually grants access when it is wired separately. People might not always set both in the same block. They might put AWS_SECRET_ACCESS_KEY in env or a run step from GitHub secrets while the access key comes from a step output, another job, or is not in the file yet. The secret has no AKIA style prefix, so there is no way to catch it unless we explicitly check AWS_SECRET_ACCESS_KEY.
Static AWS auth is a pair, but workflows split that pair across workflow env, job env, step env, shell, and action inputs. Checking only one variable lets the other half through and makes it look like we have coverage when we do not. Checking both is how we cover both halves everywhere they can appear, not the same check twice.
There was a problem hiding this comment.
we would miss static creds that show up as AWS_ACCESS_KEY_ID from secrets, vars, or env, or hardcoded AKIA values, without the secret sitting next to it in the same YAML.
yes but AWS_ACCESS_KEY_ID is not secret? the real "smoking gun" of static AWS creds is the AWS_SECRET_ACCESS_KEY coming from an unsafe place.
you also technically cannot have one be static without the other and given that our bases are pretty well covered with detecting all iterations of AWS_SECRET_ACCESS_KEY misuse, I think we're not actually losing any coverage by dropping the AWS_ACCESS_KEY_ID check, and we gain a simpler to read/maintain detection
There was a problem hiding this comment.
oh also IMO every detection should return one violation per ... violating activity. checking for both access and secret keys means for every misuse of aws creds means you will get two findings each time which isn't really representative of the problem
| AWS_ACCESS_KEY_ID: ${{ secrets.AWS_ACCESS_KEY_ID }} | ||
| AWS_SECRET_ACCESS_KEY: ${{ secrets.AWS_SECRET_ACCESS_KEY }} | ||
| steps: | ||
| - run: echo hi |
There was a problem hiding this comment.
a little silly but could we replace these echo tests with something that uses aws? aws sts get-caller-identity, whatever that makes it look a little like something someone might use aws environment variables for
There was a problem hiding this comment.
updated tests to look more realistic per request
There was a problem hiding this comment.
Pull request overview
This PR adds a new Claws rule (ConfigureAwsStaticCredentials) to detect long-lived/static AWS credential usage in GitHub Actions workflows and steer users toward OIDC + role-to-assume authentication.
Changes:
- Introduces
Claws::Rule::ConfigureAwsStaticCredentialswith detections acrossenv:blocks,aws-actions/configure-aws-credentialsinputs, and common shell-based patterns. - Adds an RSpec suite covering positive/negative cases for the new rule.
- Documents the rule in
README.mdand enables it inexample-config.yml.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| spec/claws/rule/configure_aws_static_credentials_spec.rb | Adds test coverage for static AWS credential detection scenarios. |
| README.md | Documents the new rule and recommended OIDC usage. |
| lib/claws/rule/configure_aws_static_credentials.rb | Implements the new detection rule and workflow/job/step checks. |
| lib/claws/rule.rb | Requires the new rule so it is loaded by the ruleset. |
| example-config.yml | Enables the new rule in the sample configuration. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| on_step %( | ||
| $step.run =~ ".*AWS_ACCESS_KEY_ID.*(\{\{.*secrets\.|\{\{.*vars\.|\{\{.*env\.|AKIA)" || | ||
| $step.run =~ ".*export AWS_ACCESS_KEY_ID.*(\{\{.*secrets\.|\{\{.*vars\.|\{\{.*env\.)" || | ||
| $step.run =~ ".*GITHUB_ENV.*AWS_ACCESS_KEY_ID.*(\{\{.*secrets\.|\{\{.*vars\.|\{\{.*env\.)" || | ||
| $step.run =~ ".*aws configure set aws_access_key_id.*(\{\{.*secrets\.|\{\{.*vars\.|\{\{.*env\.|AKIA)" | ||
| ), highlight: "run" | ||
|
|
||
| on_step %( | ||
| $step.run =~ ".*AWS_SECRET_ACCESS_KEY.*(\{\{.*secrets\.|\{\{.*vars\.|\{\{.*env\.)" || | ||
| $step.run =~ ".*export AWS_SECRET_ACCESS_KEY.*(\{\{.*secrets\.|\{\{.*vars\.|\{\{.*env\.)" || | ||
| $step.run =~ ".*GITHUB_ENV.*AWS_SECRET_ACCESS_KEY.*(\{\{.*secrets\.|\{\{.*vars\.|\{\{.*env\.)" || | ||
| $step.run =~ ".*aws configure set aws_secret_access_key.*(\{\{.*secrets\.|\{\{.*vars\.|\{\{.*env\.)" | ||
| ), highlight: "run" |
There was a problem hiding this comment.
yep this is a good catch! I think we should avoid .* if we can here. for example to catch an export, we could do this:
$step.run =~ "export\s*AWS_SECRET_ACCESS_KEY=\$\{\{\s*(secrets|vars)"
This way if you export this variable from anything that implies it's a long lived static secret, we can reasonably catch it being exported. We could leave out env from this since we have a separate rule that will flag secret keys from static strings being placed inside an env block.
There was a problem hiding this comment.
I think this still suffers from the same issue of the export AWS_ACCESS_KEY_ID and secret access appearing near each other in the command even if they're unrelated, e.g.
- run: |
export AWS_SECRET_ACCESS_KEY=${{ steps.aws-creds.outputs.secret-access-key }} SOMETHING_ELSE=${{ secrets.something_unrelated }}is valid and relatively safe but will get flagged. I think barring the regex double escaping I left out of my comment, \s* is preferable to .* to limit the false positives
There was a problem hiding this comment.
Also, this GITHUB_ENV regex is incorrect; the only reason the related tests pass is because the second regex in this set ($step.run =~ ".*AWS_SECRET_ACCESS_KEY=.*\\{\\{.*(secrets\\.|vars\\.)" ||) overlaps with it. The first regex in this set (the export AWS_...) also overlaps with the second.
Instead of fixing it I would advocate for removing the $GITHUB_ENV related detections outright, it's not really idiomatic Github Actions code, it's kind of a self induced indirection that would hopefully be flagged by a reviewer for independently being weird
There was a problem hiding this comment.
Updated the shell regexes based on your feedback. For run steps we only flag when the value right after the equals is ${{ secrets. or ${{ vars., not when secrets shows up somewhere else on the same line. That should fix the false positive with step outputs and an unrelated secret on one line.
I removed the separate export and GITHUB_ENV patterns since they overlapped. One assignment check covers export and non export. We are not keying off $GITHUB_ENV anymore, though a line that still assigns from secrets in the string could flag on the assignment itself. aws configure set is unchanged, same tight secrets or vars check.
I added a spec for your example line and dropped the GITHUB_ENV only test. All 14 specs pass on my side. I also trimmed the GITHUB_ENV case out of test-workflow.yml. Happy to tweak if you want it stricter or looser.
There was a problem hiding this comment.
cool, thanks. could we drop the public key (AWS_ACCESS_KEY_ID) checks too? and lean on just the secret key checks? I think we should be set to approve right afterwards. my main concern is bascially surfacing two findings for each access/secret pair, even though it's technically all just one misuse of it
| it "doesn't flag unrelated actions" do | ||
| violations = analyze(<<~YAML) | ||
| on: push | ||
|
|
||
| jobs: | ||
| deploy: | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| with: | ||
| token: ${{ secrets.AWS_ACCESS_KEY_ID }} | ||
| YAML | ||
|
|
||
| expect(violations.count).to eq(0) | ||
| end |
…s and implement new rule to flag long-lived AWS credentials in workflows
… spec for environment variable handling
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
README.md:299
- Within this new rule section the text uses both "Github" and "GitHub" in the same paragraph; "GitHub" is the correct capitalization and it’s already used later in the sentence. Please make the capitalization consistent.
This rule flags long-lived AWS credentials in Github workflows — not just `aws-actions/configure-aws-credentials`, but also `env:` blocks and common shell patterns (`export`, `$GITHUB_ENV`, `aws configure set`). Static credentials stored in GitHub secrets, variables, or environment variables can be tricky to audit and rotate. In the event of an incident where they are leaked, incident response may be tough.
Where possible, configure AWS to trust GitHub's OIDC provider and use `role-to-assume` to get short-lived credentials for each workflow run. Check [GitHub's documentation on configuring OIDC in AWS](https://docs.github.com/en/actions/security-for-github-actions/security-hardening-for-github-actions/configuring-openid-connect-in-amazon-web-services) for setup instructions.
lib/claws/rule/static_aws_credentials.rb:9
- The rule description uses "Github" here but uses "GitHub" later in the same description (and "GitHub" is the correct capitalization). Please make this consistent.
Avoid using long-lived AWS access keys in Github workflows. Static credentials
can be tricky to audit and rotate, making them risky to hold onto, especially
in the event of an incident where they may be leaked.
Use GitHub's OIDC provider and authenticate with `role-to-assume` instead.
README.md:299
- PR metadata describes adding a
ConfigureAwsStaticCredentialsrule, but the implementation/docs added here are forStaticAwsCredentials. Please align the naming (either rename the rule/classes/docs, or update the PR title/description) to avoid confusion for users/configuration.
### StaticAwsCredentials
This rule flags long-lived AWS credentials in Github workflows — not just `aws-actions/configure-aws-credentials`, but also `env:` blocks and common shell patterns (`export`, `$GITHUB_ENV`, `aws configure set`). Static credentials stored in GitHub secrets, variables, or environment variables can be tricky to audit and rotate. In the event of an incident where they are leaked, incident response may be tough.
Where possible, configure AWS to trust GitHub's OIDC provider and use `role-to-assume` to get short-lived credentials for each workflow run. Check [GitHub's documentation on configuring OIDC in AWS](https://docs.github.com/en/actions/security-for-github-actions/security-hardening-for-github-actions/configuring-openid-connect-in-amazon-web-services) for setup instructions.
| return @on if key.to_s == "on" | ||
| return @jobs if key.to_s == "jobs" | ||
| return @name if key.to_s == "name" | ||
| return @env if key.to_s == "env" |
There was a problem hiding this comment.
wait did #19 not work without this line? I tested locally and it seemed that $workflow.env worked as expected but maybe I missed something?
There was a problem hiding this comment.
Hey Omar, I added the env line in Workflow#[] on our branch. Might not be the right place long term, so happy to move or drop it if you’d rather handle it differently.
Your $workflow.env change already let the rule see top-level env. I hit a case where the violation still showed up but the >>> line was off. I traced it to [] not handling "env" when claws looks up the line for highlight: "env.AWS_ACCESS_KEY_ID".
So this was me trying to get the underline on the right line in the report, not changing what gets flagged. If there’s a cleaner way with your API, I’m good to switch.
There was a problem hiding this comment.
oh, great catch! that should have been a test case I had in the other PR. I'll tackle it in a followup
| - run: aws sts get-caller-identity | ||
| YAML | ||
|
|
||
| expect(violations.count).to eq(2) |
There was a problem hiding this comment.
For every test case that expects more than zero violations, could we assert 1. the type of violation and 2. the line the violations occur on? Here's some examples from the CheckoutWithStaticCredentials tests.
It's a bit tedious but it assures us that every violation we expect to be surfaced is surfaced. If we assert on just count for example, if the test cases end up with violations for unrelated reasons (unlikely) we could end up coincidentally passing
There was a problem hiding this comment.
Updated the positive cases to assert violation name and line numbers, not just count. Added a small expect_rule_violations helper in claws_helper.rb so we are not repeating the same expects everywhere, but it checks the same things as checkout (StaticAwsCredentials and the expected lines in each sample YAML). All 14 specs pass on my side. Happy to inline the expects like checkout if you would rather not extend the helper.
…hance spec for environment variable handling
There was a problem hiding this comment.
🟡 Not ready to approve
There are material inconsistencies between the PR metadata and the implemented rule name/claimed coverage that should be reconciled to avoid confusing users.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Comments suppressed due to low confidence (4)
lib/claws/rule/static_aws_credentials.rb:5
- Spelling/capitalization is inconsistent in this rule description ("Github" vs "GitHub"). Use the official "GitHub" capitalization for consistency with the rest of the README/link text.
Avoid using long-lived AWS access keys in Github workflows. Static credentials
README.md:297
- This new README section mixes "Github" and "GitHub" in the same paragraph. Use consistent "GitHub" capitalization (official branding) to avoid looking like a typo.
This rule flags long-lived AWS credentials in Github workflows — not just `aws-actions/configure-aws-credentials`, but also `env:` blocks and common shell patterns (`export`, `aws configure set`). Static credentials stored in GitHub secrets, variables, or environment variables can be tricky to audit and rotate. In the event of an incident where they are leaked, incident response may be tough.
lib/claws/rule/static_aws_credentials.rb:83
- The PR description claims this rule covers
$GITHUB_ENV-based patterns, but the current implementation only checks direct assignments (e.g.,AWS_*=) andaws configure setinrun:blocks. Either update the PR description to remove$GITHUB_ENVfrom the stated coverage, or add explicit detection + tests for writes likeecho "AWS_ACCESS_KEY_ID=..." >> $GITHUB_ENV.
on_step %(
$step.run =~ "AWS_ACCESS_KEY_ID\\s*=\\s*\\$\\{\\{\\s*(secrets\\.|vars\\.)" ||
$step.run =~ "AWS_ACCESS_KEY_ID\\s*=\\s*AKIA" ||
$step.run =~ "aws configure set aws_access_key_id\\s+\\$\\{\\{\\s*(secrets\\.|vars\\.)" ||
$step.run =~ "aws configure set aws_access_key_id\\s+AKIA"
), highlight: "run"
on_step %(
$step.run =~ "AWS_SECRET_ACCESS_KEY\\s*=\\s*\\$\\{\\{\\s*(secrets\\.|vars\\.)" ||
$step.run =~ "aws configure set aws_secret_access_key\\s+\\$\\{\\{\\s*(secrets\\.|vars\\.)"
), highlight: "run"
lib/claws/rule/static_aws_credentials.rb:3
- PR metadata refers to a new
ConfigureAwsStaticCredentialsrule, but the implementation/docs/specs added here are all namedStaticAwsCredentials. This mismatch can confuse users enabling/disabling rules and searching the docs; either update the PR title/description to match, or rename the rule and all references to the intended name.
This issue also appears on line 73 of the same file.
class StaticAwsCredentials < BaseRule
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Low
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
…sing expect_rule_violations helper
| def expect_rule_violations(violations, name:, lines:) | ||
| expect(violations.count).to eq(lines.length) | ||
| expect(violations.map(&:name).uniq).to eq([name]) | ||
| expect(violations.map(&:line).sort).to eq(lines.sort) | ||
| end |
|
Hey @cosmosbit this is looking good. My one last piece of feedback is I'd like for us to remove the One thing I really like about Claws is that we're /a lot/ less noisy than alternative tools. It's not the end of the world if we flag two findings per violation (especially fixing one fixes the other) but if we can help end users avoid the mental overhead of digging through findings, I would consider that a victory. Not to mention, we would simplify the detection code by quite a bit. There's two places where we lose coverage: static string checks for secret keys: right now we can't do regex matches for secret keys because they're just random bytes. we make up for this by also checking for access keys. alternatively, what we could do is check that the secret key is not null, AND that it does not contain a template reference. for example: This basically asks "Does AWS_SECRET_ACCESS_KEY contain a static string?", which is what we're really after. The other place where we lose coverage for static secret keys is shell scripts (e.g. This basically says
This way, we don't need to check the AWS_ACCESS_KEY_ID, and our regex is more constrained to what we're actually after (this var being set to a static string). Note, I removed the I think with these changes we should be set. |
This PR introduces a new rule,
ConfigureAwsStaticCredentials, to flag long-lived/static AWS credential usage in GitHub Actions workflows and promote OIDC-based auth withrole-to-assume.What this adds
aws-actions/configure-aws-credentialsstatic key inputsexport,GITHUB_ENV, andaws configure set)AKIA...) where applicableTests included
role-to-assumeflowsWhy
Static AWS credentials are difficult to rotate and audit, and increase incident risk if exposed. This rule adds guardrails to help teams adopt short-lived, federated authentication patterns by default.