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
Open
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .github/workflows/CI.yml
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@ jobs:
with:
node-version: ${{ matrix.node-version }}

- name: Update npm
run: npm i -g [email protected]

weswigham marked this conversation as resolved.
Show resolved Hide resolved
- name: Run tests
run: node ./test/runTests.js

Expand Down
37 changes: 27 additions & 10 deletions .github/workflows/publish.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -5,16 +5,33 @@ on:
types: [created]

jobs:
publish-npm:
build:
permissions:
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

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

publish:
needs: [build]
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v2
- uses: actions/setup-node@v1
- name: Set up Node registry authentication
uses: actions/setup-node@64ed1c7eab4cce3362f8c340dee64e5eaeef8f7c # v3.6.0
with:
node-version: 14
registry-url: https://registry.npmjs.org/
- run: npm i
- run: node test/validateModuleExportsMatchCommonJS/index.js
- run: npm publish
env:
NODE_AUTH_TOKEN: ${{secrets.npm_token}}
node-version: 18
registry-url: "https://registry.npmjs.org"

- name: publish
id: publish
uses: slsa-framework/slsa-github-generator/actions/nodejs/publish@4314fec3d06bb217f163b89466dcd34be65b9bf1 # v1.6.0
weswigham marked this conversation as resolved.
Show resolved Hide resolved
with:
access: public
node-auth-token: ${{ secrets.npm_token }}
package-name: ${{ needs.build.outputs.package-name }}
package-download-name: ${{ needs.build.outputs.package-download-name }}
package-download-sha256: ${{ needs.build.outputs.package-download-sha256 }}
provenance-name: ${{ needs.build.outputs.provenance-name }}
provenance-download-name: ${{ needs.build.outputs.provenance-download-name }}
provenance-download-sha256: ${{ needs.build.outputs.provenance-download-sha256 }}
3 changes: 3 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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.

weswigham marked this conversation as resolved.
Show resolved Hide resolved
}
}
2 changes: 1 addition & 1 deletion test/rollup-modules/package.json
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"scripts": {
"test": "../node_modules/.bin/rollup -c rollup.config.js && node build/index.js"
"test": "rollup -c rollup.config.js && node build/index.js"
}
}
15 changes: 11 additions & 4 deletions test/runTests.js
Original file line number Diff line number Diff line change
Expand Up @@ -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).

if (res.error) {
console.error(res.error);
process.exit(res.error.errno || -1);
}
if (res.output) {
console.log(res.output.toString());
}
console.log("Installed");
}

Expand All @@ -37,13 +44,13 @@ for (const test of tests) {
if (pgkJSON.dependencies || pgkJSON.devDependencies) {
const nodeModsInstalled = fs.existsSync(path.join(__dirname, test, "node_modules"));
if (!nodeModsInstalled) {
spawnSync("npm", ["install"], { cwd: path.join(__dirname, test) });
spawnSync("npm", ["install"], { cwd: path.join(__dirname, test), shell: true });
}
}

// Run the test command
const results = spawnSync("npm", ["test"], { cwd: path.join(__dirname, test) });
console.log(results.stdout.toString())
const results = spawnSync("npm", ["test"], { cwd: path.join(__dirname, test), shell: true });
console.log((results.stdout || "").toString())
if (results.status) {
console.log(chalk.bold.red("Error running test: ") + chalk.bold(test))
console.log(results.stderr.toString())
Expand Down
2 changes: 1 addition & 1 deletion test/snowpack-modules/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
"type": "module",

"scripts": {
"test": "../node_modules/.bin/snowpack build; node build/index.js"
"test": "snowpack build && node build/index.js"
},
"engines": {
"node": "14"
Expand Down
2 changes: 1 addition & 1 deletion test/vite/package.json
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"scripts": {
"test": "../node_modules/.bin/vite build && node build/index.js"
"test": "vite build && node build/index.js"
}
}
2 changes: 1 addition & 1 deletion test/webpack-4-modules/package.json
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"scripts": {
"test": "../node_modules/.bin/webpack && node build/main.js"
"test": "webpack && node build/main.js"
}
}