Skip to content

Use SLSA publish action to include verified build information #211

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.

Sign up for GitHub

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

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

Use SLSA publish action to include verified build information #211

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
f858242
Use SLSA publish action to include verified build source
weswigham Jun 14, 2023
b6cb2d2
Add last shell: true
weswigham Jun 14, 2023
eb4e7d6
Fix test shell commands to also work on windows
weswigham Jun 14, 2023
04d5d66
Just stop using relative paths to bin files
weswigham Jun 14, 2023
3cf5427
Use the same npm version across all node versions tested to bin files…
weswigham Jun 14, 2023
0f7429f
Older npm (why do we still test on node 12, again?)
weswigham Jun 14, 2023
51e946d
Use 1.7.0 SHA, add npm run i alias for npm i
weswigham Jun 14, 2023
8ed2bbe
Test in seperate build phase from packaging
weswigham Jun 14, 2023
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
37 changes: 27 additions & 10 deletions .github/workflows/publish.yaml
View file
Open in desktop
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
Copy Markdown

@ianlewis ianlewis Aug 8, 2023

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"
Copy link
Copy Markdown

@ianlewis ianlewis Jun 14, 2023

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
Copy Markdown
Member Author

@weswigham weswigham Jun 14, 2023

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
Comment thread
weswigham marked this conversation as resolved.
Outdated
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
View file
Open in desktop
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
Copy Markdown
Member Author

@weswigham weswigham Jun 14, 2023

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.

Comment thread
weswigham marked this conversation as resolved.
}
}
13 changes: 10 additions & 3 deletions test/runTests.js
View file
Open in desktop
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
Copy Markdown
Member Author

@weswigham weswigham Jun 14, 2023

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