cli / cli

GitHub’s official command line tool
https://cli.github.com
MIT License
36.67k stars 5.6k forks source link

`attestation verify --signer-workflow` always treated as regex #9507

Open ramonpetgrave64 opened 3 weeks ago

ramonpetgrave64 commented 3 weeks ago

Describe the bug

gh --version
gh version 2.55.0 (2024-08-20)
https://github.com/cli/cli/releases/tag/v2.55.0

Currently the gh attestation verify --help for the --signer-workflow option does not seem to suggest that the user input is meant to be treated as a regex, since there is also another --cert-identity-regex option.

--signer-workflow string       Workflow that signed attestation in the format [host/]<owner>/<repo>/<path>/<to>/<workflow>

However I found that if I supplied an incorrect value, the returned error messages suggest the tool will always treat my input as a regex.

And so I've been able to supply regexes to --signer-workflow.

Steps to reproduce the behavior

  1. See my example workflow and download the artifacts and attestations.

  2. Invoke, supplying the incorrect signer branch name

    gh attestation verify slsa3_build_artifact/gundam --bundle slsa3_build_attestation/dl.json --repo ramonpetgrave/github-build-attestations-rw  --signer-workflow "ramonpetgrave/github-build-attestations-rw/.github/workflows/attest-build-provenance-slsa3-rw.yml@refs/heads/main"     
    Loaded digest sha256:5b4167c6bdf2cf66e30ac3af8d63036bda530293e5dd694085f7df9d8d4fa91d for file://slsa3_build_artifact/gundam
    Loaded 1 attestation from slsa3_build_attestation/dl.json
    ✗ Verification failed
    
    Error: verifying with issuer "sigstore.dev": failed to verify certificate identity: no matching CertificateIdentity found, last error: expected SAN value to match regex "^https://github.com/ramonpetgrave/github-build-attestations-rw/.github/workflows/attest-build-provenance-slsa3-rw.yml@refs/heads/main", got "https://github.com/ramonpetgrave/github-build-attestations-rw/.github/workflows/attest-build-provenance-slsa3-rw.yml@refs/heads/dev"
  3. Invoke, supplying a regex that would match multiple different branch names, including the correct branch name.

    ✗ gh attestation verify slsa3_build_artifact/gundam --bundle slsa3_build_attestation/dl.json --repo ramonpetgrave/github-build-attestations-rw  --signer-workflow "ramonpetgrave/github-build-attestations-rw/.github/workflows/attest-build-provenance-slsa3-rw.yml@refs/heads/(main|dev)"
    Loaded digest sha256:5b4167c6bdf2cf66e30ac3af8d63036bda530293e5dd694085f7df9d8d4fa91d for file://slsa3_build_artifact/gundam
    Loaded 1 attestation from slsa3_build_attestation/dl.json
    ✓ Verification succeeded!
    
    sha256:5b4167c6bdf2cf66e30ac3af8d63036bda530293e5dd694085f7df9d8d4fa91d was attested by:
    REPO                                        PREDICATE_TYPE                  WORKFLOW                                                             
    ramonpetgrave/github-build-attestations-rw  https://slsa.dev/provenance/v1  .github/workflows/attest-build-provenance-slsa3-rw.yml@refs/heads/dev
  4. Invoke, supplying an incomplete signer workflow URI, not including the ref, and also cutting off the last few characters of, the workflow's file name.

    gh attestation verify slsa3_build_artifact/gundam --bundle slsa3_build_attestation/dl.json --repo ramonpetgrave/github-build-attestations-rw  --signer-workflow "ramonpetgrave/github-build-attestations-rw/.github/workflows/att"                     
    Loaded digest sha256:5b4167c6bdf2cf66e30ac3af8d63036bda530293e5dd694085f7df9d8d4fa91d for file://slsa3_build_artifact/gundam
    Loaded 1 attestation from slsa3_build_attestation/dl.json
    ✓ Verification succeeded!
    
    sha256:5b4167c6bdf2cf66e30ac3af8d63036bda530293e5dd694085f7df9d8d4fa91d was attested by:
    REPO                                        PREDICATE_TYPE                  WORKFLOW                                                             
    ramonpetgrave/github-build-attestations-rw  https://slsa.dev/provenance/v1  .github/workflows/attest-build-provenance-slsa3-rw.yml@refs/heads/dev

Expected vs actual behavior

With the wording of the documentation, I would not expect user input for --signer-workflow to be treated as a regex, especially since it's possible to supply an incomplete workflow URI: For example: [host/]<owner>/<repo>/<path>/<t, instead of the full [host/]<owner>/<repo>/<path>/<to>/<workflow>.

Instead, it should probably be treated as a full string match, and another future --signer-workflow-regex option should handle regexes.

Logs

Logs supplied above.

andyfeller commented 3 weeks ago

Interesting discovery, @ramonpetgrave64 TIL!

I can see that gh attestation verify creates a regex based on the signer workflow input, which explains how this works as you've described. I don't know if this is an intended but undocumented behavior or something to fix, so I defer to @malancas and @steiza there.

The code in question:

https://github.com/cli/cli/blob/91eb34011c1904c16c0c9ac93471a7f0e81e32a8/pkg/cmd/attestation/verify/policy.go#L109-L129

steiza commented 2 weeks ago

I don't have a strong opinion here. I think it would be okay to update the documentation to clarify that --signer-workflow is handled like a regex, or changing it to be an exact match, and / or adding a --signer-workflow-regex.

For what it's worth, I haven't heard from any users or can't think of any existing use-cases that would need a --signer-workflow-regex, so maybe I have a slight preference for changing --signer-workflow to be an exact match.

ramonpetgrave64 commented 1 week ago

@steiza it’s important that we have the ability to verify the ref of the signer workflow. The regex feature is convenient when the verifier wants to check that the signer workflow comes from a set of known branches or tags that take advantage of GitHub Repository Rules or Protected Branches, such as “/release-*” or “^v\d+.\d+.\d+$” for matching the tag v1.2.3, but not v1.2.3-rc.1.

If we only update the docs for —signer-workflow to say that it’s supposed to be a regex, I’m worried that it won’t be self-explanatory and a user may still expect it to do a full string match.

Speaking of expected refs, I’d also like the CLI to support matching expected source repo refs, for the same reasons I wrote above. I can open a new issue later to discuss that.