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

Use SLSA publish action to include verified build information #211

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

weswigham
Copy link
Member

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.

@@ -43,5 +43,8 @@
},
"./*": "./*",
"./": "./"
},
"scripts": {
"test": "node ./test/runTests.js && node test/validateModuleExportsMatchCommonJS/index.js"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 });
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link

@ianlewis ianlewis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, Just added a couple comments.

I can't speak to some of the other changes in runTests.js and CI.yml though.

.github/workflows/publish.yaml Outdated Show resolved Hide resolved
actions: read # For getting workflow run info.
uses: slsa-framework/slsa-github-generator/.github/workflows/[email protected]
with:
run-scripts: "i, test"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

package.json Show resolved Hide resolved
@weswigham
Copy link
Member Author

@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 run-scripts argument to the builder?

@ianlewis
Copy link

@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 run-scripts argument to the builder?

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.

@jakebailey
Copy link
Member

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

@ianlewis
Copy link

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.
slsa-framework/slsa-github-generator#2359

@jakebailey
Copy link
Member

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 😄

@ianlewis
Copy link

ianlewis commented Aug 8, 2023

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/[email protected]
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

v1.8.0 fixes some issues with unscoped packages.

Suggested change
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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generate SLSA Build L3 provenance
4 participants