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
37 changes: 27 additions & 10 deletions .github/workflows/publish.yaml
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/builder_nodejs_slsa3.yml@v1.7.0
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
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
}
}
13 changes: 10 additions & 3 deletions test/runTests.js
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())
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