search

slsa-framework / slsa-github-generator

Language-agnostic SLSA provenance generation for Github Actions
Apache License 2.0
421 stars 129 forks source link

[feature][nodejs] Support sha1 or tag for building #2138

Open laurentsimon opened 1 year ago

laurentsimon commented 1 year ago

See an example at https://github.com/npm/node-semver/blob/main/.github/workflows/release.yml#L315-L342

ianlewis commented 1 year ago

This example doesn't look to me to be anything unique to me. Was there something specific you meant?

This could be done via a run script.

npm i --prefer-online --no-fund --no-audit -g npm@latest

I'm not sure why they do this. IIUC is exactly what setup-node does except that it sets npm to read from the NODE_AUTH_TOKEN environment variable rather than setting a static value. 

npm config set '//registry.npmjs.org/:_authToken'=\${PUBLISH_TOKEN}

They do a checkout a ref from earlier in the workflow so maybe that's something we do need to support. This seems like a similar use case to JReleaser where they create a new commit for the release and check that out.

While I don't really want to require that folks change their workflows, I feel like the proper thing to do is actually trigger a separate workflow run based on that commit. This is like what changesets/action does. It creates a PR for the release which can trigger its own workflows when it's merged. We've already encountered some difficulties with verification (e.g. https://github.com/slsa-framework/slsa-verifier/issues/599)

ianlewis commented 1 year ago

Ah I see, as far as the install goes, it kind of looks like they were installing the latest npm just so they could use the --provenance flag but I'm still not sure why they didn't just rely on setup-node to do the setup for them?

laurentsimon commented 1 year ago

Sorry, I should have been clearer. The line to look at is https://github.com/npm/node-semver/blob/main/.github/workflows/release.yml#L330. It's using a tag that seems to be created in an earlier job https://github.com/npm/node-semver/blob/main/.github/workflows/release.yml#LL60C28-L60C28 (I've not verified the end-to-end call, though), instead of using the tag / sha from the GitHub event. (Note: the event is workflow_dispatch, so there is not tag by default)

ianlewis commented 1 year ago

Yeah, I noticed some workflows do this. I was kind of hoping we could get folks to adopt a model where they don't create new commits and build from them in the same workflow run.

Rather, they could create a PR for the release and/or trigger a new workflow run for the new commit. This is what the changesets tool does. Example PR: https://github.com/sigstore/sigstore-js/pull/489

laurentsimon commented 1 year ago

Agreed no pushing directly to the repo is better. I suspect it's more work for users to change their setup, and it will be an adoption problem. But we'll see.

ianlewis commented 1 year ago

Agreed no pushing directly to the repo is better. I suspect it's more work for users to change their setup, and it will be an adoption problem. But we'll see.

Yeah, I suppose we will need to support it at some point but I'd like folks to not do it if possible.