feat: Audit secrets outside an environment #637

aldur · 2025-03-31T11:52:32Z

Would close #184.

Early draft, mainly to gather feedback (as this scratch an itch I have :)). Specifically, doing string matching in the with/env section might not be robust enough and/or might lead to false positives.

Sorry about the noise in the imports, that is what rustfmt has suggested for me. That can be reverted if it doesn't match the project style.

woodruffw · 2025-03-31T19:05:56Z

Hey @aldur, thanks for taking a stab at this! I'll do a high-level review later today or tomorrow.

src/audit/secrets_outside_environment.rs

tests/integration/test-data/secrets-outside-environment.yml

src/audit/secrets_outside_environment.rs

aldur · 2025-04-10T07:13:39Z

Thanks for the comments! I have addressed them, added a snapshot and had to modify use-trusted-publishing.yml in 6b963b6 for cargo insta test to pass.

I have also some .snap.new files in my cwd, should I add them to this PR?

woodruffw · 2025-04-18T19:38:38Z

I have also some .snap.new files in my cwd, should I add them to this PR?

You should run cargo insta test --review and then accept them, and then check those files in 🙂

aldur · 2025-04-22T08:10:14Z

I have also some .snap.new files in my cwd, should I add them to this PR?

You should run cargo insta test --review and then accept them, and then check those files in 🙂

Done, thank you!

woodruffw · 2025-04-23T22:31:01Z

Thanks @aldur, this is looking really good!

One thing I'm realizing, based on the changes to the other tests: this might be slightly too conservative as a default audit, at least for now, since a lot of people have relatively simple CI/CD setups with 1-2 workflows where an environment wouldn't provide much isolation.

Given that, I'm thinking that it might make sense to mark this as a "pedantic" audit for now, i.e. by calling .persona(Persona::Pedantic) on each finding produced. How would you feel about that? IMO that would allow people to opt into the behavior here while not being too disruptive, and we could make it a "regular" audit in the future (or make it enableable in the regular persona once #617 is fleshed out more).

aldur · 2025-05-05T06:14:52Z

Thanks @aldur, this is looking really good!

One thing I'm realizing, based on the changes to the other tests: this might be slightly too conservative as a default audit, at least for now, since a lot of people have relatively simple CI/CD setups with 1-2 workflows where an environment wouldn't provide much isolation.

Given that, I'm thinking that it might make sense to mark this as a "pedantic" audit for now, i.e. by calling .persona(Persona::Pedantic) on each finding produced. How would you feel about that? IMO that would allow people to opt into the behavior here while not being too disruptive, and we could make it a "regular" audit in the future (or make it enableable in the regular persona once #617 is fleshed out more).

Done! Sorry it took me some time to get back to this :)

woodruffw · 2025-05-05T18:57:47Z

Done! Sorry it took me some time to get back to this :)

No problem at all! I'll do another review tonight.

woodruffw · 2025-05-05T23:03:01Z

src/audit/secrets_outside_environment.rs

+
+audit_meta!(
+    SecretsOutsideEnvironment,
+    "secrets-outside-environment",


I'm trying to make new audit names a bit shorter, so some ideas here:

secret-outside-env

secret-without-env

secret-missing-env

secret-no-env (shortest, but maybe not easy to understand)

Curious if you have any other naming thoughts as well 🙂

Picked number 2 and fixed in 96c90ac.

woodruffw · 2025-05-05T23:09:14Z

src/audit/secrets_outside_environment.rs

+        step: &crate::models::Step<'w>,
+        findings: &mut Vec<crate::finding::Finding<'w>>,
+    ) -> anyhow::Result<()> {
+        if s.contains("secrets") {


I think we need to exclude secrets.github_token here, since that's a "latent" secret that's present regardless of any environment.

Yep that makes sense! Fixed it in b5dab62.

woodruffw · 2025-05-05T23:17:06Z

src/audit/secrets_outside_environment.rs

+                working_directory: _,
+            } => match env {
+                LoE::Expr(e) => {
+                    Self::check_secrets_access(e.as_bare(), step, &mut findings)?;


I think this check needs to be a little more discerning: e needs to be parsed into an Expr, which then needs to be walked recursively to visit each Expr::Context and match that on the secrets namespace.

Without that, we'll end up with (admittedly goofy) false positives like:

with: yolo: ${{ 'secrets' }}

Writing the AST walker is currently a bit annoying/manual; if you want to wait until I add a Visitor API that might make your life a bit easier (I plan on doing that pretty soon) 🙂

I like that! It will also increase the audit confidence. I am ok with waiting for the Visitor API, just give me a ping once ready and I'll put it to work :)

woodruffw · 2025-05-05T23:17:44Z

src/audit/secrets_outside_environment.rs

+                eenv = with;
+            }
+            StepBody::Run {
+                run: _,


Note: I'm okay with skipping run: blocks for an MVP here, but we will need to extract expressions from those and scan for secrets as well at some point.

Makes sense! I naively thought that wouldn't be required, would this be an example?

- name: Clean install dependencies and build run: | npm ci PASSWORD=${{ secrets.password }} npm run build

Yep, exactly 🙂

aldur · 2025-06-17T08:00:56Z

Hi there! Any news on the Visitor API? I can give this another shot and get it merged.

woodruffw · 2025-06-19T20:43:37Z

Hey @aldur, sorry for the delay!

The Visitor API is high on my stack -- it got deferred a bit because of some larger expression API refactoring that needed to be done to enable #240, but now that that's done I'll be working on it. So you should be unblocked here shortly.

(P.S. Sorry for the huge conflict set here -- with any luck most of it can probably be discarded and the snapshot tests can be re-done on top of just the audit changes.)

woodruffw · 2025-07-02T21:42:30Z

I'm done with the expression API refactoring, but I wasn't able to reach a Visitor API design that made a lot of sense -- all of the patterns I tried didn't map super well to our existing use patterns, or could be expressed more easily/cleanly with the existing "match and recurse" style.

Given that, I think we can move forward without blocking on any larger visitor API changes here; the existing APIs should work well enough. Sorry for such a delay on my end.

aldur added 3 commits March 31, 2025 13:45

feat: Audit secrets outside an environment

3e9f0ad

Add TODO

13f9213

Make name more generic

acaaef1

woodruffw reviewed Apr 6, 2025

View reviewed changes

src/audit/secrets_outside_environment.rs Outdated Show resolved Hide resolved

woodruffw reviewed Apr 6, 2025

View reviewed changes

tests/integration/test-data/secrets-outside-environment.yml Outdated Show resolved Hide resolved

woodruffw reviewed Apr 6, 2025

View reviewed changes

src/audit/secrets_outside_environment.rs Outdated Show resolved Hide resolved

aldur added 3 commits April 10, 2025 09:07

Add snapshot test

992b15b

Switch to audit_step and handle expressions

1d5b2df

Add environment to use-trusted-publishing.yml

6b963b6

aldur marked this pull request as ready for review April 10, 2025 07:13

aldur added 4 commits April 22, 2025 09:50

Merge branch 'main' into feat/secrets_outside_env

13c8cf3

Fix merge

2ae5791

Update to Audit from main

cf1b0e5

Update integration tests and snaps

0132617

woodruffw added enhancement New feature or request new-audit New audits labels Apr 23, 2025

aldur force-pushed the feat/secrets_outside_env branch 2 times, most recently from 1048e11 to 0132617 Compare May 5, 2025 06:08

aldur added 2 commits May 5, 2025 08:11

Merge branch 'main' into feat/secrets_outside_env

5a3bfe6

Make this a pedantic audit

f328751

woodruffw reviewed May 5, 2025

View reviewed changes

aldur added 2 commits May 6, 2025 08:20

Rename to secret_without_env

96c90ac

Whitelist secrets.github_token

b5dab62

Uh oh!

feat: Audit secrets outside an environment #637

Are you sure you want to change the base?

feat: Audit secrets outside an environment #637

Uh oh!

Conversation

aldur commented Mar 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

woodruffw commented Mar 31, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aldur commented Apr 10, 2025

Uh oh!

woodruffw commented Apr 18, 2025

Uh oh!

aldur commented Apr 22, 2025

Uh oh!

woodruffw commented Apr 23, 2025

Uh oh!

aldur commented May 5, 2025

Uh oh!

woodruffw commented May 5, 2025

Uh oh!

woodruffw May 5, 2025

Choose a reason for hiding this comment

Uh oh!

aldur May 6, 2025

Choose a reason for hiding this comment

Uh oh!

woodruffw May 5, 2025

Choose a reason for hiding this comment

Uh oh!

aldur May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

woodruffw May 5, 2025

Choose a reason for hiding this comment

Uh oh!

aldur May 6, 2025

Choose a reason for hiding this comment

Uh oh!

woodruffw May 5, 2025

Choose a reason for hiding this comment

Uh oh!

aldur May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

woodruffw Jun 19, 2025

Choose a reason for hiding this comment

Uh oh!

aldur commented Jun 17, 2025

Uh oh!

woodruffw commented Jun 19, 2025

Uh oh!

woodruffw commented Jul 2, 2025

Uh oh!

Uh oh!

aldur commented Mar 31, 2025 •

edited

Loading

aldur May 6, 2025 •

edited

Loading

aldur May 6, 2025 •

edited

Loading