Persisting credentials on boolean GitHub expressions

[nicholasjng]

Hey, I got a small follow-up to the issue I posted today.

I merged the audit fix for template expansions in our docs action (everything was green) and ended up with red CI only after the push on main.

Why? Well, we build our docs on every PR, but deploy them, which involves creating a commit in our docs branch in CI using credentials, only on pushes to main, i.e. when ${{ github.ref == 'refs/heads/main' }}.

This isn't supported by zizmor, which requires that any value other than false for persist-credentials must be "true". But it's technically less broad than just rolling with "true" - in fact, the latter would mean persisted credentials in ~50% more jobs, assuming every PR gets merged.

FWIW, I think the current solution is really good from both user and dev perspectives, because it forces you to think about the ramifications of persisting credentials, and it doesn't try to accommodate every sidestep like what I just presented. But I'm wondering what your verdict is on a case like this?

[woodruffw]

This is a great question, thank you!

Yeah, this points to a basic (but significant) limitation in the expressivity of zizmor's analyses: we often run into states where a value is T | expression, where expression is something potentially non-trivial.

Some of the audits specialize this a bit (in template-injection for example, we walk the expression's AST to figure out if it's "safe" to expand), but other audits (like artipacked) simply give up and either ignore or blindly flag something depending on the context:

zizmor/src/audit/artipacked.rs

Lines 72 to 82 in b0c6f34

	match with.get("persist-credentials") {
	Some(EnvValue::Boolean(false)) => continue,
	Some(EnvValue::Boolean(true)) => {
	// If a user explicitly sets `persist-credentials: true`,
	// they probably mean it. Only report if in auditor mode.
	vulnerable_checkouts.push((step, Persona::Auditor))
	}
	// TODO: handle expressions and literal strings here.
	// persist-credentials is true by default.
	_ => vulnerable_checkouts.push((step, Persona::default())),
	}

I've been thinking about ways to improve this, but unfortunately many of them are hard to generalize: it'd be great to consider persist-credentials: ${{ github.ref == 'refs/heads/main' }} safe, but this requires us to know that refs/heads/main is really the main branch (which it frequently is, but not always!). We'd also need to handle non-trivial expressions like ${{ github.ref == 'refs/heads/main' && github.actor == 'foo' }} which require lots of external knowledge to make a security decision 🙂

In the mean time, there are probably a couple of things that could be done here

1. When the persist-credentials: value is an expression, we could emit lower-confidence findings, or maybe mark them as pedantic.
2. You can use an # zizmor: ignore[artipacked] rule to suppress this, since it's a false positive in this case. That's not very satisfying, I admit.

TL;DR: this is something I'm thinking about, and I have some ideas for improving the overall precision of our expression handling. Unfortunately however I think it's going to be impossible to generalize with zizmor's architectural assumptions (namely that everything can run offline, and that we don't necessarily know things like trusted actors, default branches, etc.).

[nicholasjng]

All good. I think this is a good place for zizmor: ignore, since the alternative is some pretty hefty engineering work like you mention. Thanks!