Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature: only accept reusable workflow pinned by version #12

Open
Tracked by #30
laurentsimon opened this issue Mar 29, 2022 · 16 comments
Open
Tracked by #30

Feature: only accept reusable workflow pinned by version #12

laurentsimon opened this issue Mar 29, 2022 · 16 comments
Labels
type:feature New feature request

Comments

@laurentsimon
Copy link
Contributor

laurentsimon commented Mar 29, 2022

The reusable workflow can be pinned by hash, version or tag in general.

However:

  1. Pinned by hash makes it pretty hard to retrieve the branch during verification.
  2. Pinned by branch (like main) should be discouraged.

The OIDC token (which we use to retrieve the workflow identity/pin) currently does not report the version/branch used. So for now we will only accept version/tag pinning during verification. Once GitHub adds support, we can accept hash pins.

@laurentsimon laurentsimon changed the title Feature: only accept version tag for the reusable workflow Feature: only accept reusable workflow pinned by version Mar 30, 2022
@naveensrinivasan
Copy link
Contributor

Pinned by hash makes it pretty hard to retrieve the branch during verification.

@laurentsimon Can you please explain why so? A specific example would be helpful to understand. Thanks

@laurentsimon
Copy link
Contributor Author

Well, sort of what I described. The OIDC currently does not contain all the information we'd like and only contains trusted-workflow.yml@XXX where XXX is how the user pinned the trusted builder.
If users use a hash, the verifier would need to use GitHub API to match it to a branch and verify it's the main branch.
If they use a branch... well.. they should not use the main branch directly because we may be breaking things at head.
So the only viable solution today is to ask users to pin by version: we, the maintainers, should be only releasing branches from the main branch, so if they trust us this works.

Later when GitHub adds support in the OIDC token to indicate the branch and hash of the trusted builder, we'll be able to let users pin by hash and we'll be able to verify the hash corresponds to the main branch via the information in the certificate (see https://github.com/slsa-framework/slsa-github-generator-go/blob/main/images/cert.svg) which is a mere copy of the information in the OIDC token.

Does this help?

@MarkLodato
Copy link
Member

@laurentsimon The verifier could have a list of acceptable hashes, either hard-coded or via some signed mechanism (TUF? attestation?) This avoids the need for us to trust the protection of tags on our repo.

@laurentsimon
Copy link
Contributor Author

You're right, we could do that. There are going to be quite a few generators though, and we'll have to be sure to add new hashes after each release. This is do-able, just a little more error prone for us. Some builders may be "out of our control", for example pipy seem interested in owning their builder.

It may become useful to have a builder's version, since new versions may have new features or breaking changes. (We can map each tag to a version, though).

Using version/tag pinning seems simpler to start with (due to the OIDC token limitation). We could later introduce support for hashes. If we introduce hashes right now, it may be harder to take it way from users if it becomes untenable to maintain.

I'm not against doing it, mostly trying to weigh the pros and cons.

Wdut?

/cc @asraa

@MarkLodato
Copy link
Member

I think we have the following considerations:

  • Ease of implementation on our side:
    • Branch: easy, just check for main
    • Tag: easy, just check for some pattern, e.g. v*
    • Hash: hard, need to maintain a hardcoded allowlist OR have some infrastructure to keep the list of "trusted" hashes up-to-date (signing, TUF, etc.)
  • Ease of use on caller's side:
    • Branch: easy, stays updated
    • Tag: medium-to-easy, dependabot keeps it up-to-date
    • Hash: medium, dependabot keeps it up-to-date, but it's less readable
  • Stability for the caller:
    • Branch: poor, API can change underneath the user, breaking a build
    • Tag: medium-to-good (tags are mutable, but we shouldn't modify them)
    • Hash: good (immutable)
  • Insider risk of someone with slsa-framework write access pushing a bad release:
    • branch: medium, branch protection w/ two party review mostly covers this, but still a risk of two people pushing a bad commit that gets auto-accepted everywhere
    • tag: poor, anyone with correct permissions can push arbitrary things to the tag and mutate/delete tags, with no logs
    • hash: good, immutable - this is what GitHub recommends for security reasons

@laurentsimon
Copy link
Contributor Author

+1, good summary.

@laurentsimon
Copy link
Contributor Author

Enforcing pinning by version is done in #63
Keeping this issue open for now.

@joshuagl
Copy link
Member

joshuagl commented Jul 5, 2022

I just discovered that GitHub now supports tag protection rules:

Only users with admin or maintain permissions in the repository will be able to create protected tags, and only users with admin permissions in the repository will be able to delete protected tags.

It's not clear from the docs what permissions are required to mutate/overwrite a tag, but this may mitigate some of the insider risk of tags?

@ianlewis
Copy link
Member

ianlewis commented Jul 5, 2022

I just discovered that GitHub now supports tag protection rules:

Only users with admin or maintain permissions in the repository will be able to create protected tags, and only users with admin permissions in the repository will be able to delete protected tags.

It's not clear from the docs what permissions are required to mutate/overwrite a tag, but this may mitigate some of the insider risk of tags?

Yeah, we should enable that so that it's not a risk for folks who are simply contributors. I just added a tag rule for all tags ("*").

@datosh
Copy link

datosh commented Oct 17, 2022

I just integrated the generator into a project that is also using Renovate with the pinning rule enforced. Manual overwrite for slsa generator package would look like this:

  "packageRules": [
    {
      "matchManagers": ["github-actions"],
      "matchPackageNames": ["slsa-framework/slsa-github-generator"],
      "pinDigests": false
    }
  ]

Would it be helpful to document this somewhere for users of the generator? Any recommendations for a location? Seems a bit too much detail for the README.

@laurentsimon
Copy link
Contributor Author

Great idea @datosh

How about linking to your renovatebot config in the generator repo in this section https://github.com/slsa-framework/slsa-github-generator#referencing-slsa-builders-and-generators?

We can create a RENOVATEBOT.md to link to in the same repo? Wdut?

@datosh
Copy link

datosh commented Oct 18, 2022

Thanks for the quick response @laurentsimon. I put together a quick PR. What do you think?

How about linking to your renovatebot config in the generator repo

Do you mean linking to the repository where I actually use slsa generator to show a live working example? I can add the link, just wasn't sure if that's what you mean.

@ianlewis ianlewis added the type:feature New feature request label Nov 25, 2022
@stevehipwell
Copy link

  • Ease of use on caller's side:
    • Hash: medium, dependabot keeps it up-to-date, but it's less readable

@MarkLodato I think this might have changed to medium-to-easy since Dependabot now supports keeping a version comment up to date as part up updating digests?

@MarkLodato
Copy link
Member

@MarkLodato I think this might have changed to medium-to-easy since Dependabot now supports keeping a version comment up to date as part up updating digests?

👍

@jul-sh
Copy link

jul-sh commented Apr 17, 2024

The reusable workflow can be pinned by hash, version or tag in general.

Pinning by hash does not appear to be functional, hence one can only depend on released tags. As mentioned before in this thread that doesn't seem ideal for defense in depth.

It's also an issue for bug fixes though! In our case provenance generation was blocked by a breaking bug (slsa-framework/slsa-github-generator#3571). The fix is merged, but we are unable to use them, since it's not yet included in a release tag.

Supporting pinning by hash would solve that problem (slsa-framework/slsa-github-generator#3573).

@laurentsimon
Copy link
Contributor Author

@ramonpetgrave64 let's work on a relaese

ramonpetgrave64 pushed a commit that referenced this issue Apr 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:feature New feature request
Projects
None yet
Development

No branches or pull requests

8 participants