GitHub OIDC: validate job_workflow_ref

[woodruffw]

Also adds a bunch of counterexample tests to be certain.

[woodruffw]

This is more or less the same as the original implementation, except that we now use a strict regex (re.match) instead of str.startswith as our predicate. That should eliminate the possibility for potential prefix confusion by a malicious user, although said confusion would still be outside of our threat model (since it assumes CI compromise).

[ewjoachim]

Hm, I've left a comment and github is not showing it to me. I wonder if github lost my comment. Let me know if this is the first comment you see of me in this PR.

Edit: ah here it is. Github is insisting on hiding it from me for some reason. #11263 (comment)

Anyway, the discussion we oroginally had was like this: #10753 (comment)

And I'm not seeing the same things as we discussed, but maybe I'm misunderstanding the goal of this PR

[woodruffw]

And I'm not seeing the same things as we discussed, but maybe I'm misunderstanding the goal of this PR

Yeah, sorry if this was unclear: the goal here is not to support reusable workflows. We're only using job_workflow_ref to verify the workflow filename (and prevent any potential workflow filename confusion).

The reason we're doing this is because the separate workflow claim is not the workflow filename: it's the name: specified in the workflow file. Multiple workflow files can share the same name and we'd like each publisher/provider registration to be limited to a single workflow file, so job_workflow_ref is our only verification route.

[di]

This LGTM but want to make sure @ewjoachim's concerns have been addressed before merging.

[ewjoachim]

Ok so there's one question left from my other comment:

What happens if there's an @ in the branch/tag name ?

I believe this will confuse rsplit.

[woodruffw]

Ok so there's one question left from my other comment:

What happens if there's an @ in the branch/tag name ?

I believe this will confuse rsplit.

Yeah, I think you're right. I don't have a clean solution for this, then -- we could do an unambiguous parse by referencing the ref claim for the suffix, but that causes an abstraction leak (claim verification depending on other claims as inputs) that makes our verification step much more manual (and therefore user-error prone).

All of this is technically outside of our threat model (#10644), since the attack here assumes that the attacker manages to fool the repository owner into introducing a malicious workflow. Thoughts on just reverting back to the startswith or regex check?

[woodruffw]

Okay, this should do the trick: we're now using the ref claim to complement the job_workflow_ref check. This prevents any sort of prefix or suffix sneakiness, since the verification is now an exact string comparison for the pattern:

{workflow_path}@{ref}

...where {workflow_path} is computed solely from trusted setup data and {ref} is a verified claim containing a git-style ref.

[di]

Will let @ewjoachim re-review as well.

[ewjoachim]

Haha, I'm sorry for taking nice code away and forcing you to write less nice code :D (blame GitHub for mixing parts of strings together in a bigger string without any kind of escaping and expecting us to parse the bits ourselves)

Seems good now :)

$ python -m this | grep amb
In the face of ambiguity, refuse the temptation to guess.

[di]

Much appreciated, @ewjoachim 🙂

[woodruffw]

Haha, I'm sorry for taking nice code away and forcing you to write less nice code :D (blame GitHub for mixing parts of strings together in a bigger string without any kind of escaping and expecting us to parse the bits ourselves)

It's greatly appreciated! Precision is a virtue, especially in anything remotely involving security 🙂