feat: support OIDC auth for GitHub Actions/GitLab #6898
Conversation
cometkim
force-pushed
the
npm-trusted-publish
branch
2 times, most recently
from
c8563c6 to
8d89c74
Compare
arcanis
left a comment
arcanis
left a comment
There was a problem hiding this comment.
Thanks! Left a couple of comments
.yarn/versions/9ad4b6ac.yml
Outdated
|
|
||
| declined: | ||
| - "@yarnpkg/plugin-compat" | ||
| - "@yarnpkg/cli" |
| ident, | ||
| otp: this.otp, | ||
| jsonResponse: true, | ||
| allowOidc: Boolean(env.CI && (env.GITHUB_ACTIONS || env.GITLAB)), |
There was a problem hiding this comment.
Do we gain something to exposing that as an option? Shouldn't it be an implementation detail of npmHttpUtils, since only put needs it and it always wants to set it if possible?
There was a problem hiding this comment.
Theoretically, it could be supported anywhere related to the registry if the registry provider wanted it. It's not necessarily coupled with publish or put actions.
Since this essentially involves more than two provider details (registry and runner environment), I didn't think plugin-npm was the ideal place to do it, but I wanted to avoid too many changes here.
Another approach would be to use the getNpmAuthenticationHeader hook; perhaps a plugin for each CI provider would be ideal.
| import {packUtils} from '@yarnpkg/plugin-pack'; | ||
| import {Command, Option, Usage, UsageError} from 'clipanion'; | ||
|
|
||
| const {env} = process; |
There was a problem hiding this comment.
Avoid making an indirection for process.env, it makes it a little more difficult to understand where a value comes from at a glance.
cometkim
force-pushed
the
npm-trusted-publish
branch
from
61877d1 to
c568e5b
Compare
|
@arcanis, btw, do you have any idea why it's not working with |
|
It's a CNAME, yes. I'm not familiar with the oidc protocol but I imagine some certificates may be attached to the hostnames and they didn't bother to do it for the Yarn hostname? Not sure. Unfortunately these days I think GitHub tickets are the only way to get support from the folks handling the npm server 🫤 |
| return null; | ||
|
|
||
| const url = new URL(process.env.ACTIONS_ID_TOKEN_REQUEST_URL); | ||
| url.searchParams.append(`audience`, `npm:${new URL(registry).host}`); |
There was a problem hiding this comment.
I see. When using registry.yarnpkg.com, audience should be replaced with the original (registry.npmjs.com)
|
Ok, now it works without changing the registry URL. |
|
Released in 4.10 - thanks a lot ! |
## What's the problem this PR addresses? At #6898, I made a mistake, making it not work for scoped packages.
## What's the problem this PR addresses? When #6898 added support for OIDC publishing, it copied the getOidcToken function from npm's implementation. However, `ciInfo` was directly replaced with `process.env`, which coincidentally worked fine for GitHub Actions (both the `ciInfo` constant and the environment variable are named `GITHUB_ACTIONS`), but not for GitLab (the `ciInfo` constant is `GITLAB`, but the actual env var is `GITLAB_CI`). ## How did you fix it? Replaced `process.env.GITLAB` with `process.env.GITLAB_CI` ## 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.
2 participants
What's the problem this PR addresses?
Resolves #6831
How did you fix it?
Implementation adapted from https://github.com/npm/cli/blob/7d900c4656cfffc8cca93240c6cda4b441fbbfaa/lib/utils/oidc.js
I'll test publishing with a dummy package.Tested at https://github.com/cometkim/npgi/actions/runs/17703091184/job/50311171040
You can check the published package and the provenance here: https://www.npmjs.com/package/npgi
Note: it doesn't work with the Yarn registry proxy, so it requires settingFixed.publishConfig.registryto"https://registry.npmjs.org"Checklist