-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
feat(plugin-npm): add npm provenance support #6750
Conversation
Thanks @GauBen for looking into this! I'm interested in supporting provenance, however I'm concerned about the number of dependencies this brings in. We try not to rely on the npm libraries, and here we're adding a bunch of them for a small feature. Is sigstore well-speced enough that we can afford to just call whatever endpoints are needed ourselves? Or is the sdk a black box we can't avoid? |
Hi Arcanis, thanks for taking the time to review this quickly! I'm mildly annoyed too by the fact that we need 90+ packages to sign a .tgz archive. After a quick investigation, most of these libraries are pulled by make-fetch-happen. To continue, I'd suggest opening a PR on tuf-js to remove make-fetch-happen from the dependencies and make it a BYOF (bring your own fetch) library. We can speed up the process by depending on the forked version until it is merged. Edit: Node maintainer Antoine du Hamel opened theupdateframework/tuf-js#849 two months ago |
Ok I did something interesting:
It might not be the best way to do it, but we can continue from here, what do you think? |
@GauBen, btw, thank you very much for this brilliant PR. |
I measured the weight of the feature:
If the hack is ok (or can be improved), we can go with sigstore
I don't know, and I'm not really motivated to go through it. However, if @lsd-cat agrees, we can adapt their "webcat" project to fit our needs. Does it support the in-toto statements used by npm provenance? Update: I'm too tired for that, there are more packages than |
The project is MIT licensed and I'm happy with any use, though it is really rudimentary with the purpose to support just a basic set of Sigstore functionalities. TUF is a bit battle tested, as it was adapted through multiple root updates. Sigstore has just a few signing/hash algorithms tested, but adding support there should be trivial. It expects a bundle, and verifies the signing cert, the SCT, the signature over the artifact and the Rekor promise, but not the inclusion proof if I recall correctly. I'm happy to provide support in case of problems, but I can't promise it. There is no support for in-toto statements. None of them has really any kind of automated testing yet. It is likely that we will continue the development of WEBCAT in the next months, so if there is community interests in reusing the sigstore/tuf implementations there, perhaps we could collaborate on them? Our main requirements would be to keep them browser native and without runtime dependencies. |
A reasonable test would be to run both:
And see what works and what does not. I did not use tuf-conformance before because the tests check for files, and in the browser environment I'm using a different type of storage, but we can probably make that a build time preference and see if we pass a reasonable amount of tests or what are the issues? |
6fa6f78
to
ea69525
Compare
Thanks a lot for your detailed answer!
It would be nice to make it runtime-agnostic (for Node.js consumers too), but that might be out of the scope of your project.
I have no idea if there is interest past this PR, IIRC pnpm uses npm to publish, so all JS package managers would use sigstore under the hood. There is nothing the prevents Yarn to revisit the provenance implementation once WEBCAT is ready though. A good night of sleep gave me a less dumb idea, let's try that instead Edit: Indeed, we have provenance support with only 10 new packages! (down from 90 new packages 👀) |
@GauBen, I don't know about the Yarn command, but for me personally, it's a pretty important functionality 💜 Publishing workspace to NPM with Yarn is a true pleasure - just one line: yarn workspaces foreach --recursive npm publish and I wouldn't want to change anything about this elegance, except to add the I've tried a combination of I've been looking forward to the Yarn provenance releases, and it would be great if it worked out! Thanks for your contribution! |
Thank you @abatalov-walletteam and @radist2s for your kind comments (you seem to be only one person but I don't want te be rude) Any chance you try this PR on your repo and confirm it works? Run
Thank you again Edit: confirmed to work on GitHub for the latest commit |
This comment was marked as resolved.
This comment was marked as resolved.
Hey @GauBen, just wanted to confirm — everything works great! ✅ I tested publishing with Thanks again for implementing this — really appreciate it! |
Great work you've done so far on this PR! Just thought to ask if support for third-party package publishing tools is planned, or would it only be the CLI flag? For example, |
@SerenModz21 Consider it done! I added support (and documented):
@arcanis I set |
@GauBen Amazing, thank you! |
That sounds good to me - I'd have liked to have some tests though, as I'm refactoring various parts of Yarn and I'm concerned I could accidentally break this in the future. Do you think you could look at that as a follow-up? For example by copying a publish test and validating the provenance in the test server if the package name is |
I'll give it a shot, but I'd probably need to update workflow files (to provide |
Can't we sign with a bogus certificate, just for testing? (not familiar with sigstore at all!) In the meantime let's merge this iteration |
## What's the problem this PR addresses? Follow-up #6750: enable provenance statements for Yarn ## How did you fix it? <!-- A detailed description of your implementation. --> Added `permissions: id-token: write` to publication workflows ## Checklist <!--- Don't worry if you miss something, chores are automatically tested. --> <!--- This checklist exists to help you remember doing the chores when you submit a PR. --> <!--- Put an `x` in all the boxes that apply. --> - [x] I have read the [Contributing Guide](https://yarnpkg.com/advanced/contributing). <!-- See https://yarnpkg.com/advanced/contributing#preparing-your-pr-to-be-released for more details. --> <!-- Check with `yarn version check` and fix with `yarn version check -i` --> - [x] I have set the packages that need to be released for my changes to be effective. <!-- The "Testing chores" workflow validates that your PR follows our guidelines. --> <!-- If it doesn't pass, click on it to see details as to what your PR might be missing. --> - [x] I will check that all automated PR checks pass before the PR gets reviewed.
## What's the problem this PR addresses? <!-- Describe the rationale of your PR. --> <!-- Link all issues that it closes. (Closes/Resolves #xxxx.) --> Follow-up #6750: add tests ## How did you fix it? <!-- A detailed description of your implementation. --> New npm publish acceptance tests that verifies the provenance statement validity ## Checklist <!--- Don't worry if you miss something, chores are automatically tested. --> <!--- This checklist exists to help you remember doing the chores when you submit a PR. --> <!--- Put an `x` in all the boxes that apply. --> - [ ] I have read the [Contributing Guide](https://yarnpkg.com/advanced/contributing). <!-- See https://yarnpkg.com/advanced/contributing#preparing-your-pr-to-be-released for more details. --> <!-- Check with `yarn version check` and fix with `yarn version check -i` --> - [ ] I have set the packages that need to be released for my changes to be effective. <!-- The "Testing chores" workflow validates that your PR follows our guidelines. --> <!-- If it doesn't pass, click on it to see details as to what your PR might be missing. --> - [ ] I will check that all automated PR checks pass before the PR gets reviewed. --------- Co-authored-by: Maël Nison <[email protected]>
- run: yarn config set npmAuthToken '${{ secrets.NPM_TOKEN }}' | ||
- run: yarn publish --provenance --tolerate-republish |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the actual recommendation? Shouldn't it be using an env variable instead?
- run: yarn config set npmAuthToken '${{ secrets.NPM_TOKEN }}' | |
- run: yarn publish --provenance --tolerate-republish | |
- run: yarn publish --provenance --tolerate-republish | |
env: | |
YARN_NPM_AUTH_TOKEN: ${{ secrets.NPM_TOKEN }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, it feels cleaner and more self-contained
What's the problem this PR addresses?
Hi! I added support for provenance to
yarn npm publish
.Closes #5430
How did you fix it?
Adapted code from npm to produce a provenance signature in supported CI environment.
Checklist
Next steps