Use SLSA publish action to include verified build information by weswigham · Pull Request #211 · microsoft/tslib

Wesley Wigham (weswigham) · 2023-06-14T20:16:02Z

cc Ian Lewis (@ianlewis) if this looks about right to you - obviously aren't really able to test this completely outside of an actual github action.

Wesley Wigham (weswigham) · 2023-06-14T20:16:39Z

 "./": "./"
+ },
+ "scripts": {
+ "test": "node ./test/runTests.js && node test/validateModuleExportsMatchCommonJS/index.js"


Previously the publish skipped the runTests.js part, but I don't see the harm in including it - it's fast.

Wesley Wigham (weswigham) · 2023-06-14T20:17:46Z

 if (!filesInTest.includes("node_modules")) {
 console.log("Installing Deps...");
- spawnSync("npm", ["install"], { cwd: __dirname });
+ const res = spawnSync("npm", ["install"], { cwd: __dirname, shell: true });


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

… are loaded in the same way

Ian Lewis (ianlewis)

LGTM, Just added a couple comments.

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

Ian Lewis (ianlewis) · 2023-06-14T22:39:28Z

+ 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"


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.

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

Wesley Wigham (weswigham) · 2023-06-14T23:39:57Z

Ian Lewis (@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?

Ian Lewis (ianlewis) · 2023-06-15T00:06:43Z

Ian Lewis (@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.

Jake Bailey (jakebailey) · 2023-07-07T06:49:04Z

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

Ian Lewis (ianlewis) · 2023-07-10T07:23:39Z

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

Jake Bailey (jakebailey) · 2023-08-05T03:01:09Z

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 😄

Ian Lewis (ianlewis) · 2023-08-08T23:16:38Z

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.

Ian Lewis (ianlewis) · 2023-08-08T23:17:12Z

+ 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


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

Use SLSA publish action to include verified build source
f858242

Wesley Wigham (weswigham) requested review from Ryan Cavanaugh (RyanCavanaugh) and Ron Buckton (rbuckton)June 14, 2023 20:16

Wesley Wigham (weswigham) commented Jun 14, 2023
View reviewed changes

Wesley Wigham (weswigham) added 2 commits June 14, 2023 13:19

Add last shell: true
b6cb2d2

Fix test shell commands to also work on windows
eb4e7d6

Wesley Wigham (weswigham) commented Jun 14, 2023
View reviewed changes

Comment threadtest/snowpack-modules/package.json Outdated

Wesley Wigham (weswigham) added 3 commits June 14, 2023 13:34

Just stop using relative paths to bin files
04d5d66

Use the same npm version across all node versions tested to bin files…
3cf5427
… are loaded in the same way

Older npm (why do we still test on node 12, again?)
0f7429f

Ron Buckton (rbuckton) reviewed Jun 14, 2023
View reviewed changes

Comment thread.github/workflows/CI.yml

Ian Lewis (ianlewis) reviewed Jun 14, 2023
View reviewed changes

Wesley Wigham (weswigham) added 2 commits June 14, 2023 16:31

Use 1.7.0 SHA, add npm run i alias for npm i
51e946d

Test in seperate build phase from packaging
8ed2bbe

Ian Lewis (ianlewis) reviewed Aug 8, 2023
View reviewed changes

Diogo Teles Sant'Anna (diogoteles08) mentioned this pull request Oct 24, 2023
Suggest an dependabot to keep GitHub Actions updated #225
Closed

	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

Uh oh!

Conversation

Wesley Wigham (weswigham) commented Jun 14, 2023

Uh oh!

Wesley Wigham (weswigham)Jun 14, 2023

Choose a reason for hiding this comment

Uh oh!

Wesley Wigham (weswigham)Jun 14, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Ian Lewis (ianlewis) left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Ian Lewis (ianlewis)Jun 14, 2023

Choose a reason for hiding this comment

Uh oh!

Wesley Wigham (weswigham)Jun 14, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Wesley Wigham (weswigham) commented Jun 14, 2023

Uh oh!

Ian Lewis (ianlewis) commented Jun 15, 2023

Uh oh!

Jake Bailey (jakebailey) commented Jul 7, 2023

Uh oh!

Ian Lewis (ianlewis) commented Jul 10, 2023

Uh oh!

Jake Bailey (jakebailey) commented Aug 5, 2023

Uh oh!

Ian Lewis (ianlewis) commented Aug 8, 2023

Uh oh!

Ian Lewis (ianlewis)Aug 8, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants