Use SLSA publish action to include verified build information #211
Conversation
| @@ -43,5 +43,8 @@ | |||
| }, | |||
| "./*": "./*", | |||
| "./": "./" | |||
| }, | |||
| "scripts": { | |||
| "test": "node ./test/runTests.js && node test/validateModuleExportsMatchCommonJS/index.js" | |||
There was a problem hiding this comment.
Previously the publish skipped the runTests.js part, but I don't see the harm in including it - it's fast.
| @@ -14,7 +14,14 @@ const tests = filesInTest | |||
| // Support setting up the test node modules | |||
| if (!filesInTest.includes("node_modules")) { | |||
| console.log("Installing Deps..."); | |||
| spawnSync("npm", ["install"], { cwd: __dirname }); | |||
| const res = spawnSync("npm", ["install"], { cwd: __dirname, shell: true }); | |||
There was a problem hiding this comment.
At least on my machine, you can't actually find npm unless the shell: true command is passed so this actually checks the system PATH for npm (and, without the extra logging below, this failure was completely silent).
.github/workflows/publish.yaml
Outdated
| actions: read # For getting workflow run info. | ||
| uses: slsa-framework/slsa-github-generator/.github/workflows/builder_nodejs_slsa3.yml@v1.7.0 | ||
| with: | ||
| run-scripts: "i, test" |
There was a problem hiding this comment.
Running test in the builder is ok, but we only support running it once and only in an ubuntu-latest runner. I assume this is ok based on your previous workflow?
In general though I think we are actually going to lean towards projects running tests outside the builder since that way they can support multiple-node versions, different runners etc. The tests also can't interfere with the build that way. In that case you probably could get away without any run-scripts at all. As you mentioned on the issue, the security benefit is indeed a bit nuanced in your case but I think there is still some benefit to creating the package archive in a traceable way separately from publish.
There was a problem hiding this comment.
I assume this is ok based on your previous workflow?
Aye, we run the full matrix on normal CI every commit, we just like to make sure an obviously broken build isn't published when we cut a release :)
|
@ianlewis you mean you wanna recommend something like what I have now, going forward, for the install & test phases or a build, rather than using the |
tbh, It's not a big deal either way. You could do it the way you have it for convenience's sake if that makes sense for you. There just isn't really a specific reason to run the tests in the builder. |
|
I tested this out on hereby as a low-risk test before we do this to critical packages, and unfortunately it didn't seem to work: https://github.com/jakebailey/hereby/actions/runs/5483129736/jobs/9989176122#step:11:404 |
Yes, this is a bug. Will fix ASAP. |
|
With 1.8.0 out, I got my first working SLSA level 3 build: https://github.com/jakebailey/hereby/actions/runs/5768389689 Feel free to copy my (now working) pipeline for this, though we'll have to get it into everyone's heads to stop doing local releases 😄 |
Thanks! I'm glad things are working for you now. Hopefully v1.8.0 allows us to move forward. |
| id-token: write # For signing | ||
| contents: read # For repo checkout. | ||
| actions: read # For getting workflow run info. | ||
| uses: slsa-framework/slsa-github-generator/.github/workflows/builder_nodejs_slsa3.yml@v1.7.0 |
There was a problem hiding this comment.
v1.8.0 fixes some issues with unscoped packages.
| uses: slsa-framework/slsa-github-generator/.github/workflows/builder_nodejs_slsa3.yml@v1.7.0 | |
| uses: slsa-framework/slsa-github-generator/.github/workflows/builder_nodejs_slsa3.yml@v1.8.0 |
Fixes #210
cc @ianlewis if this looks about right to you - obviously aren't really able to test this completely outside of an actual github action.