Figure out how to allow conformance tests on third-party PRs

[woodruffw]

The conformance testsuite currently relies on a GitHub Actions OIDC credential, obtained by asking for the id-token: write permission.

Unfortunately (but understandably), this permission can't be given to a repository's (untrusted) third-party forks: forks can only receive id-token: read, which doesn't allow access to the OIDC credential.

As such, the conformance testsuite currently fails when it's triggered by a PR coming from a third-party fork:

=================================== FAILURES ===================================
_______________________________ test_sign_verify _______________________________

client = <test.client.SigstoreClient object at 0x7f6153166310>

    def test_sign_verify(client: SigstoreClient) -> None:
        """
        A basic test that signs and verifies an artifact for a given Sigstore
        client.
        """
        artifact_path = Path("artifact.txt")
        signature_path = Path("artifact.txt.sig")
        certificate_path = Path("artifact.txt.crt")

        assert artifact_path.exists()
        assert not signature_path.exists()
        assert not certificate_path.exists()

        # Sign the artifact.
>       client.sign(artifact_path, signature_path, certificate_path)

/home/runner/work/_actions/trailofbits/sigstore-conformance/4b6b3c2877f7fd629d33c654b7[51](https://github.com/sigstore/sigstore-python/actions/runs/3649487017/jobs/6164227325#step:5:54)43ecfcc68e2b/test/test_sign_verify.py:20: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
/home/runner/work/_actions/trailofbits/sigstore-conformance/4b6b3c2877f7fd629d33c654b75143ecfcc68e2b/test/client.py:[53](https://github.com/sigstore/sigstore-python/actions/runs/3649487017/jobs/6164227325#step:5:56): in sign
    self.run(
/home/runner/work/_actions/trailofbits/sigstore-conformance/4b6b3c2877f7fd629d33c6[54](https://github.com/sigstore/sigstore-python/actions/runs/3649487017/jobs/6164227325#step:5:57)b75143ecfcc[68](https://github.com/sigstore/sigstore-python/actions/runs/3649487017/jobs/6164227325#step:5:71)e2b/test/client.py:35: in run
    subprocess.run(
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

input = None, capture_output = False, timeout = None, check = True
popenargs = (['/home/runner/work/sigstore-python/sigstore-python/test/integration/sigstore-python-conformance', 'sign', '--signature', PosixPath('artifact.txt.sig'), '--certificate', PosixPath('artifact.txt.crt'), ...],)
kwargs = {'stderr': -2, 'stdout': -1, 'text': True}
process = <Popen: returncode: 1 args: ['/home/runner/work/sigstore-python/sigstore-pyt...>
stdout = "\nInsufficient permissions for GitHub Actions workflow.\n\nThe most common reason for this is incorrect\nconfiguratio...gnature', 'artifact.txt.sig', '--certificate', 'artifact.txt.crt', 'artifact.txt']' returned non-zero exit status 1.\n"
stderr = None, retcode = 1

Logs: https://github.com/sigstore/sigstore-python/actions/runs/3649487017/jobs/6164227325

Consumers of the test suite can hack around this by disabling the testsuite on third-party forks, but that isn't ideal in the long term (since it means that third-party contributors are receiving less test coverage than first-party ones).

Instead, we should figure out a secure way to share an OIDC credential with the conformance testsuite, even when it's coming from an untrusted fork. One possible way to do that would be to leverage the fact that the conformance suite should always be pinned to a hash that we know ahead of time.

[di]

Some other workarounds that come to mind:

• if we require a GitHub OIDC token, we could set up an action in another repository with a sufficient blast radius that generates a useless OIDC token on-demand and makes it available to this test suite when the workflow is triggered, which could then be injected into the test environment
• If we just require a valid OIDC token, we could set up a lightweight service on GCP that returns an OIDC token for the service account the service is running as upon request (again, with a sufficient blast radius).

One possible way to do that would be to leverage the fact that the conformance suite should always be pinned to a hash that we know ahead of time.

Not sure I follow the line of thought here, how does this relate?

[woodruffw]

Yep! Something like that is close to what I was thinking 🙂

Not sure I follow the line of thought here, how does this relate?

I hadn't fully thought it through, but it was connected to reducing the blast radius: I was thinking that we could set an OIDC token as an input to the workflow, but only if the workflow is pinned to a hash that we recognize. But I hadn't fully thought that through yet, and maybe it's sufficient to just have a dedicated "useless" OIDC identity that'll sign for anything.

[asraa]

I wonder if you can also use a pull_request_target action to access id-token write? You would need to protect against mis-use by only allowing triggering via an approval label.

EDIT: Check this out https://securitylab.github.com/research/github-actions-preventing-pwn-requests/. I think you can. pull_request_target can allow write access.

[di]

Oh, interesting! I wonder if that would give it an identity of the conformance test suite, or of the parent repository. If it's the latter, I think I'd still be wary here of making that identity available for signing, since we also use it to sign builds/releases.

[woodruffw]

TIL, thanks @asraa! Yeah, we should see which identity this ends up giving it. I can do a quick PR to test this out!

[tetsuo-cpp]

Oh, interesting! I wonder if that would give it an identity of the conformance test suite, or of the parent repository. If it's the latter, I think I'd still be wary here of making that identity available for signing, since we also use it to sign builds/releases.

Ok, I did an experiment to figure out what identity is being used.

The test PR is here: https://github.com/tetsuo-cpp/sigstore-conformance/pull/1. This is a bit of a mind bend, but I had to make a PR from the trailofbits repo to my own fork because the target branch needs to have the pull_request_target configuration in it in order for this to work (and obviously I didn't want to commit that into our real main). In this PR, it used https://github.com/tetsuo-cpp/sigstore-conformance/.github/workflows/conformance.yml@refs/pull/1/merge. So in a normal situation where this action gets used in a workflow for a client (say Cosign), it would use the sigstore/cosign identity rather the conformance test suite.

I'd say there's a silver lining here in that the identity always gets the GITHUB_REF appended to it. So even if this identity was misused via a malicious third party PR, the identity would be different from the one triggered by a release (https://github.com/sigstore/sigstore-python/.github/workflows/conformance.yml@release vs https://github.com/sigstore/sigstore-python/.github/workflows/conformance.yml@refs/pull/1/merge).

[tetsuo-cpp]

I think there are two main ways of getting this to work.

The first would be feeding in an external token to the test suite as @di described above. We can figure out the actual source of the token later whether that involves polling some external service, or whether we have an Action that generates one from another repository. Pros would be that the client's identity can't be misused by 3rd party pull request and cons would be added complexity and potential flakiness due to using one token for the entire test suite (I can imagine a scenario where the "generate token" step gets scheduled well before the tests start running and the token ends up expiring in the middle of the test execution).

The second solution is to advise users to use pull_request_target in their workflow and provide guidance on tweaking their Actions settings to ensure that PRs from forks always require approval to run (I believe the default setting is to require approval for a user's first contribution) so we don't have an issue where a 3rd party PR ends up misusing our identity.

My view is that we should advise users to use pull_request_target and provide guidance on how to do this safely. It streamlines implementation and means that we can do the obvious and simple thing in the test suite (that is, to use ambient credentials) without tying ourselves in knots trying to generate tokens in different places and pass them into the test suite. Asking clients to make all 3rd party contributions require approval to run seems like a reasonable ask and all Sigstore projects should already be doing that. And in the case that a client configures this wrong and we have a 3rd party PR that uses the identity to sign a faulty artifact, that's not great but I don't see it having a material impact because the identity available in a PR is distinctly different to the release identity.

@di @woodruffw @asraa, what are your thoughts on this?

[woodruffw]

Thanks for the detailed writeup!

I agree with your suggestion: pull_request_target keeps the potential failure modes (in terms of reliability) to a minimum, and keeps integration simple. The fact that the workflow ref is always distinct from a release's workflow ref is a nice cherry on top as well.

I think we should go forward with this, but make sure that we document the caveats:

1. Using pull_request_target means that the action runs in the context of the "base" repo, which in turn means that maintainers have to be consistently vigilant about which PRs they approve to run.
2. In terms of limiting the area of effect, repositories that use this action should configure appropriate actions policies (like always requiring approval for third-party PRs).
3. We should use (and enforce?) the "label" trick, as documented in that GitHub blog post about mitigating the risk of "pwn requests". In particular, we should have our action test its own executing context to make sure it was triggered by a label, and not by a "bare" pull_request_target trigger.
   1. As a subcomponent of this, we should advise consumers of this action that using the conformance suite changes the potential privileges of their "triage" members -- repo members with "triage" rights can assign labels, meaning that a triager could potentially trigger a malicious third party workflow.

[asraa]

Thanks for setting up the test demo, so I got a nice TIL from this too :)

We should use (and enforce?) the "label" trick, as documented in that GitHub blog post about mitigating the risk of "pwn requests". In particular, we should have our action test its own executing context to make sure it was triggered by a label, and not by a "bare" pull_request_target trigger.

But +1. I would actually recommend that the label be applied each time, rather than approving a 3rd party contributor once (and for all). Just in case there's a mistake in the PR, or some danger, or some account takeover type of issue.

repo members with "triage" rights can assign labels, meaning that a triager could potentially trigger a malicious third party workflow.

Nice point.

[di]

Just want to circle back to my original question before we dive into this: do we actually need a GitHub OIDC token for these tests? Or do we just need a valid OIDC token, and we're using the ambient token because it's easiest?

If it's the latter, I think pulling a test token from somewhere else would be preferable both for UX for maintainers reviewing PRs, and for isolation of the repo's identity.

[woodruffw]

Just want to circle back to my original question before we dive into this: do we actually need a GitHub OIDC token for these tests? Or do we just need a valid OIDC token, and we're using the ambient token because it's easiest?

Yeah, we're using the ambient token because it's easiest, both to obtain and to use in verification tests (since the conformance suite is Actions based, we can always recompute the expected identity and other claims directly from the runner envirionment).

We could refactor the conformance suite to make it generic over any OIDC token, but we'll need to remove assumptions like these:

https://github.com/trailofbits/sigstore-conformance/blob/acea918a74aa667be0286fb9a0e2bb173bd7c4a5/test/client.py#L4-L11

[woodruffw]

But +1. I would actually recommend that the label be applied each time, rather than approving a 3rd party contributor once (and for all). Just in case there's a mistake in the PR, or some danger, or some account takeover type of issue.

Yep! I listed this under point (2) 🙂

[woodruffw]

Hmm, I just realized another possible option: reusable workflows might be able to access the caller workflow's state, so it's possible they can access the OIDC credential as well. I can experiment with that.

[woodruffw]

I played around with reusable workflows a bit, and I don't think they'll do what we need -- they can only be used as full-fledged jobs, not steps, meaning that they can't be composed with the steps that each Sigstore client implementation needs to set up its own environment, build, etc.

[woodruffw]

I'm trying another approach now, where I use a reusable workflow to grab an OIDC token and then forward that token (via an output) to the calling workflow.

[woodruffw]

I'm trying another approach now, where I use a reusable workflow to grab an OIDC token and then forward that token (via an output) to the calling workflow.

I'm having trouble getting this to work, but for reasons that I don't understand. Here's the reusable workflow, which exists only to obtain an OIDC token:

name: Reusable sigstore-conformance workflow

permissions:
  # Needed to access the workflow's OIDC identity.
  id-token: write

on:
  workflow_call:
    outputs:
      oidc-token:
        description: "An OIDC JWT for the conformance testsuite"
        value: ${{ jobs.get-token.outputs.oidc-token }}

jobs:
  get-token:
    runs-on: ubuntu-latest
    outputs:
      oidc-token: ${{ steps.get-token.outputs.oidc-token }}
    steps:
      - name: get-token
        id: get-token
        run: |
          token=$(curl -H \
            "Authorization: bearer ${ACTIONS_ID_TOKEN_REQUEST_TOKEN}" \
            "${ACTIONS_ID_TOKEN_REQUEST_URL}&audience=sigstore" | \
          jq -r .value)

          echo "oidc-token=${token}" >> "${GITHUB_OUTPUT}"

and then the calling workflow, which should be granting the correct permissions (since it sets id-token: write, without failing):

jobs:
  get-token:
    permissions:
      id-token: write
    uses: trailofbits/sigstore-conformance/.github/workflows/reusable.yml@ww/reusable

  get-token-test:
    runs-on: ubuntu-latest
    needs: get-token
    steps:
      - run: echo ${{ needs.get-token.outputs.oidc-token }}

however, this fails because the OIDC token URL expands to nothing, per the logs:

Run token=$(curl -H \
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed

  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
curl: (6) Could not resolve host: &audience=sigstore

Logs: https://github.com/trailofbits/sigstore-conformance/actions/runs/3660055567/jobs/6186756943

This is a bit of a mystery to me: my reading of GitHub's OIDC and reusable workflow documentation suggests that this should work, or at least fail with a permissions error rather than silently expanding to nothing.

[woodruffw]

To clarify: my reading of GitHub's docs is that the reusable workflow here should produce an OIDC token corresponding to trail-of-forks/sigstore-conformance, with a job_workflow_ref linking back to the repo that the workflow + ref is actually defined in (trailofbits/sigstore-conformance@ww/reusable).

[woodruffw]

Experimented some more, and it looks like reusable workflows can access the OIDC token, just not from a fork. So this is a fundamental restriction on forks.

[tetsuo-cpp]

@di @woodruffw @asraa

As I've been integrating the conformance suite into sigstore-java, we've noticed that the labelling mechanism is a bit clunky.

To recap, there's a bit of redundancy going on here. The basic idea is that we want to avoid having malicious 3rd party PRs having access to the client repo's OIDC token. We currently have some guidance here which explains that we should set our CI to require approval for PRs from forks. However, since there's no way to enforce this setting from our action, we decided to also add the labelling mechanism so that the action is safe by default.

We covered this earlier in this thread, however it's important to note that the identity that is assumed in a PR is distinctly different to the release identity so there's no way for a malicious PR to use the identity to sign something that would pass a release validation. While it might feel icky to leave this unsafe by default, if we don't have a concrete explanation on how this can be abused, I think there's an argument for removing the labelling mechanism and relying on users to just set the CI to require approval from forks. Yes it's unsafe by default but, even if the CI is misconfigured, it doesn't seem like this would be catastrophic (famous last words 😆). @vlsi makes some good points to that effect in https://github.com/sigstore/sigstore-java/pull/251.

What do you think about this?

[woodruffw]

This is tricky -- the current flow is pretty clunky, but IMO there are three major risks with removing the label requirement:

1. We've removed a layer of defense against repo misconfiguration: in the current state of affairs, a maintainer still has to label the PR before the action accepts it, even if the repo has been misconfigured to trust third-party PRs. This (IMO) is still desirable, since it means that we can provide some degree of protection to downstream users who otherwise ignore our guidance.
2. You're right that the claims in the OIDC token are scoped such that an attacker would be unlikely to find them useful for Sigstore. However, there's still an impersonation risk with other OIDC consumers: pull_request_target means that a malicious PR can run arbitrary code and therefore request arbitrary additional OIDC tokens, including ones with aud claims other than sigstore. This in turn might allow impersonation on other services, since some services (like AWS) may just be configured to trust valid OIDC tokens with a matching aud claim, without additional claim constraints.
3. Even outside of the OIDC context, pull_request_target is dangerous by design and its use is discouraged by GitHub. Even if an attacker can't convince Sigstore or another service to use the OIDC tokens it can access, they can still (1) exfiltrate other workflow secrets, (2) perform whatever actions the default GITHUB_TOKEN permissions allow, and (3) generally impersonate the target repository.

I don't think we have to stick with labels as the gatekeeping mechanism, but we should definitely retain something. One thought: maybe we can reuse the mechanism that we currently use for GCB runs on sigstore-python? The runner there currently listens for a /gcbrun comment from a maintainer, so maybe we could do something similar here.

[woodruffw]

Another aspect of this: the current labeling mechanism is a little fiddly: it doesn't automatically re-queue the conformance suite for running after a PR changes, and it doesn't automatically accept PRs from maintainers (which I incorrectly attempted to resolve in #62). We should figure out resolutions to both of those as part of a more permanent solution here.

[haydentherapper]

Cc @laurentsimon, you might find this thread interesting

[vlsi]

exfiltrate other workflow secrets, (2) perform whatever actions the default GITHUB_TOKEN permissions allow, and (3) generally impersonate the target repository

Well, with permissions: id-token: write they can not do much. They can only request OIDC token, and they have no access to the secrets

[woodruffw]

From https://docs.github.com/en/actions/security-guides/automatic-token-authentication#how-the-permissions-are-calculated-for-a-workflow-job: they can do whatever the default permissions allow, and orgs are able to configure those permissively. In other words, we can't trust permissions: id-token: write to drop all permissions other than id-token, only to drop whatever permissions aren't configured by the org. This is arguably not our problem (the org/repo is in a vulnerable state of their own doing), but IMO it's something we should provide defense for since we're explicitly asking them to use an unsafe workflow.

From https://securitylab.github.com/research/github-actions-preventing-pwn-requests/: all pull_request_target workflows have access to the first-party repo's workflow secrets. Unless I'm missing something, there's no permission scoping that changes this.

[vlsi]

Have you actually tried "organization defaults"?

See https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#permissions

all of those that are not specified are set to none

I believe the defaults have no power here. All of those that are not specified are set to none.

[vlsi]

Yet another possibility might be testing within "fork Action". In other words, let users to perform OIDC tests in their forks and propagate the results somehow to the upstream. For instance we could propagate the link from Actions in fork repository as a comment in the upstream PR.

[laurentsimon]

Other things to think about:

1. pull_request_target have access to GH encrypted secrets
2. There's a race condition when you apply a label. You're never guaranteed that the label is applied on the commit sha the reviewer reviewed... the PR author could have pushed a malicious commit before the reviewer added a label. Ideally you need a comment like /ok-to-run <reviewed-sha> to trigger the pre-submit

[woodruffw]

2. There's a race condition when you apply a label. You're never guaranteed that the label is applied on the commit sha the reviewer reviewed... the PR author could have pushed a malicious commit before the reviewer added a label. Ideally you need a comment like /ok-to-run <reviewed-sha> to trigger the pre-submit

Yep, this is one of the things I'd like this workflow to be resilient against. I like the /ok-to-run ... idea!

Edit: It looks like the relevant GitHub Action event for this is issue_comment, but we probably won't be able to use that directly (since we need the pull_request_target context for OIDC access). Going with this approach will probably require us to use the GitHub API directly to retrieve the PR's comments, check for a matching one from a permissioned user, etc.

[vlsi]

pull_request_target have access to GH encrypted secrets

It has not provided permissions is present in the workflow yml

[laurentsimon]

To ensure to comment author is a maintainer, you can check the author_association in the GH context. You could also hardcode author_id, login in the workflow, but these are different in a fork repo (if you expect some users may want to accept PRs on their forks, it would not work.. but maybe we don't care about this use case)

[laurentsimon]

pull_request_target have access to GH encrypted secrets

It has not provided permissions is present in the workflow yml

you don't need permissions to access encrypted secrets. They are available in a pull_request_target event

[woodruffw]

Have you actually tried "organization defaults"?

No, I've just read the docs and interpreted them. I could be wrong in my interpretation, but it's one of several sufficient reasons why we want some kind of additional check here.

Yet another possibility might be testing within "fork Action". In other words, let users to perform OIDC tests in their forks and propagate the results somehow to the upstream.

Do you have a resource for this? This sounds like an approach we should look into, if there's an ergonomic way to do it.

[laurentsimon]

Letting users perform the OIDC tests in their fork seems dangerous for security reasons: if the run happens in the runners / workflows they own, how will you know they ran the test you expect them to? How do you trust the results? I think you'd have to come up with a way to verify what they actually ran (based on hash commit of their code and verify it's the same as what the reviewer reviewed).

If we want to offload the run to another repo, an alternative could be to offload it to a repo we own. So you could have something along the lines of:

1. User submits a PR on this repo
2. pre-submit workflow posts a request on a another repo we own (ideally a different org to prevent access to secrets - unless we're certain no org secrets are available to the repo). The PR could have a description that says "hash commit / ref / repo-name")
3. the other repo checks out the repo / ref / commit and runs the tests, then posts a comment back on the original PR in this repo
4. The pre-submit in this repo verifies the bot's results (signature, actor_id, etc)

If we only expect this repo to accept PRs, and not the forked repo, this would work. Clearly requires some engineering :)

[woodruffw]

Hmm -- I'm personally okay with not offloading it in the interest of simplicity, so long as we retain that comment flow to make sure maintainers explicitly opt into testing against a specific non-symbolic ref.

Here's what I'm thinking:

1. We retain the current pull_request_target flow, but without the labeled trigger (since this is finnicky anyways);
2. Instead of a labeled check, we use the GitHub API to scan the associated PR's comments, looking for a comment from an authorized user that matches /ok-to-run <ref>, where ref is the concrete GITHUB_SHA for the run;
3. If we find a match, we let it go through. Otherwise, we fail.

(Steps (2) and (3) would be skipped for first-party PRs entirely.)

This has the advantage of working with forked repos as well, and requires scaffolding on our side (no extra org, no remote workflows). OTOH it retains the fundamental danger of pull_request_target and requires maintainers to understand that they're responsible for carefully reviewing code before submitting it for conformance testing.

[tetsuo-cpp]

I'd also rather keep things simple if we can.

The thing that prompted me to re-open this issue was the fact that the "safe to test" label is required even for non-fork PRs as well as the general jankiness of the mechanism (you need to remove and add the "safe to test" label to test new commits, race condition, etc).

I think @woodruffw's suggestion addresses these.

[vlsi]

No, I've just read the docs and interpreted them. I could be wrong in my interpretation, but it's one of several sufficient reasons why we want some kind of additional check here

The documentation clearly says: "all of those that are not specified are set to none" I have no idea how can you read that it as "permissions can be overridden at the organization level". If workflow level specifies something, then all the rest are set to none.

[tetsuo-cpp]

you don't need permissions to access encrypted secrets. They are available in a pull_request_target event

The docs seem to support this comment from @laurentsimon in the "Warning" block here: https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#pull_request_target.

Given that this is the case, I think that regardless of this discussion about organisation defaults and how they affect permissions, we need to protect the pull_request_target runs behind a check.

@vlsi What do you think about the approach outlined at https://github.com/trailofbits/sigstore-conformance/issues/55#issuecomment-1366162953? I think the advantages of this approach over the current approach are:

• We can restrict it to only require comments for third-party PRs, so it shouldn't affect your day to day development on sigstore-java much at all.
• Less jankiness since we're not using labels. At the moment, if you want to rerun conformance testing for a new commit, you need to re-add the label. That means that for the times that you do get a third party PR, the approval mechanism will be less painful than it is now.

[haydentherapper]

Have we considered making these tests run nightly rather than on each pull request? That would mitigate concerns of using pull_request_target since you'd only run against main.

[woodruffw]

Have we considered making these tests run nightly rather than on each pull request? That would mitigate concerns of using pull_request_target since you'd only run against main.

That would work! The only downside there would be that the feedback loop for regressions would be a little bit longer, but perhaps not meaningfully so (and not importantly so, since hopefully nobody is using nightly builds of the various Sigstore clones).

I'll defer to @di on that -- IMO it might make sense as the "default" deployment, but we might still want to offer guidance/functionality for Sigstore clients that wish to run the conformance suite on every PR.

[di]

The general goal is to catch regressions before they're merged, not released, so I think we should prefer them to run on PRs.

[tetsuo-cpp]

@woodruffw @di @laurentsimon

I sat down to figure out what this is going to look like. Turns out there's a bit more complexity than appears at first glance.

Our current design relies in pull_request_target with the labelled trigger type. One problem I see is that it doesn't seem like pull_request_target supports anything to do with comments. This is important because the run is not going to be triggered off pull request activity in the third-party case. The workflow will look like:

• Outside contributor pushes commit sha0.
• Collaborator comments /confrun sha0.
• Trigger conformance test run.

The way to trigger off comments is to use issue_comment and check whether github.event.issue.pull_request is set. Of course, this doesn't give us the permissions we need for the OIDC token.

One option would be to have a workflow that runs off issue_comment that does the comment check and then have a workflow_run trigger to do the bulk of the work that requires the OIDC token. The problem I see with this would be we don't have any control over this in the action itself. So we'll need to push this configuration down into each client repo and provide guidance on how to configure it correctly.

To my understanding, users would need to have workflows that look like this:

check-conformance.yml

# This workflow checks whether the conformance test is allowed to execute.
name: Check Conformance Permissions

on:
  issue_comment

jobs:
  check-conformance-permissions:
    runs-on: ubuntu-latest
    steps:
      # We can add another mode to this action that checks if we're running in a pull request and the author isn't a collaborator.
      # We can then use the GitHub API to scan the comments for a comment from a collaborator.
      # If there is no such comment, this job should fail.
      - uses: trailofbits/sigstore-conformance
        with:
          check-permissions: true 

conformance.yml

# This workflow actually executes the conformance tests.
name: Run Conformance Tests

on:
  push:
    branches:
      - main
  workflow_run:
    workflows: ["Check Conformance Permissions"]
    types:
      - completed

  jobs:
    conformance:
      # This is the default mode which runs the conformance suite.
      - uses: trailofbits/sigstore-conformance
        with:
          entrypoint: sigstore

I'm sure there are things I've left out and syntax errors, etc but that's the general gist of it.

I think it'd be pretty ugly if we forced this configuration into each client. I'm beginning to think that using ambient credentials might be more trouble than it's worth and it might be easier to get tokens from elsewhere as suggested by @di here: https://github.com/trailofbits/sigstore-conformance/issues/55#issuecomment-1344453466.

[vlsi]

For reference, pull_request_target jobs have access to secrets even in the case the workflow has permissions: id-token: write.

Even though the documentation says "all of those that are not specified are set to none" (see https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#permissions), the actual implementation allows for secret exfiltrating.

See my test in https://github.com/vlsi/pull_request_target_test/pull/1/checks#step:2:8

[woodruffw]

Thanks for the writeup @tetsuo-cpp!

The way to trigger off comments is to use issue_comment and check whether github.event.issue.pull_request is set. Of course, this doesn't give us the permissions we need for the OIDC token.

Hmm, is this true even when the comment's author association (author_association) indicates that they're an owner/maintainer for the repo? I would hope that being an owner/maintainer means that issue_comment events get the right permissions, but it's definitely a weird case.

I think the above is worth testing, but overall I'm in agreement that needing a separate "check" workflow is too complicated to be a reasonable imposition on Sigstore clients. Running with a "throwaway" ambient OIDC credential would be better, per @di's suggestion.

[tetsuo-cpp]

It seems that while we can get this to work by triggering off issue_comment, there is a bit of jankiness involved. As per the discussion at https://github.com/tetsuo-cpp/sigstore-conformance/pull/2#issuecomment-1408885737, I'm going to look into having a lightweight service that produces OIDC token just for the test suite.

@di, regarding your comment at https://github.com/sigstore/sigstore-conformance/issues/55#issuecomment-1342918955.

Having a separate repo sounds a bit tricky. There are some ways to trigger workflows in other repositories. So a client's repo would trigger the "generate token" job in our special repo, but we'd have to do something similar to give the token back and much of this configuration would need to be in the client's workflow.

A similar idea would be to have a Google Cloud Build job that simply prints the host's ambient credential out or stores it in some kind of build artifact. We could then use this in the test suite. Similarly, most of this can't be baked into the action itself so we'll be pushing more configuration into client repos.

The second idea you suggested sounds more convenient on the client side. I'm assuming I can just write a small server that's always online and the conformance test suite just sends some kind of request and gets a token back. We should just be able to do this for each request. Perhaps we need to protect it somehow so it's not spammable? There's a bit more to think about as we need to figure out what GCP account it belongs to and who has the keys to it since this will have to live entirely outside of GitHub.

[di]

I'm assuming I can just write a small server that's always online and the conformance test suite just sends some kind of request and gets a token back.

Yep, exactly. My plan is to the ambient-token-grabbing functionality out of sigstore-python into a reusable library, and just do that.

[tetsuo-cpp]

@di I'm going to focus on some other things while you're working on the server + separating out ambient token functionality into another package. Let me know if you need a hand with any of that.

[di]

We've got the reusable library (https://github.com/di/id), working on standing up the lightweight service now.

[di]

We could probably also just use this instead: https://github.com/chainguard-dev/justtrustme

[loosebazooka]

Don't think fulcio would accept these tokens?

[tetsuo-cpp]

There's been some discussions since the last post here so I'll leave an update explaining.

@di did a some investigation on running a lightweight service on GCP but ran into some issues so we decided the best course of actions might be to have a dedicated GitHub repo to generate OIDC tokens and then trigger the workflow from each client repo.

Today, I had a look into this approach. I think this problem suffers from the same issue as just using the ambient OIDC token. According to this comment, you need a personal access token in order to make a workflow dispatch request to the dedicated OIDC repo. This can be a read-only access token that only has access to public repositories (minimal privilege), however it's still a secret that needs to be exposed to the workflow and would require pull_request_target to use in a third-party PR. So this brings us back to square one again...

I suppose I could just have the OIDC job run by itself every 15 min or so, regardless of whether anyone requested it, and then have the conformance test suite grab the latest generated token. Given how lightweight the process of grabbing the token is, it probably won't be an issue.

@di What do you think about that?