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

Bot user for nodejs/corepack #660

Closed
arcanis opened this issue Feb 10, 2022 · 23 comments
Closed

Bot user for nodejs/corepack #660

arcanis opened this issue Feb 10, 2022 · 23 comments

Comments

@arcanis
Copy link

arcanis commented Feb 10, 2022

I'd like to automate Corepack so that it'd automatically be updated with new package manager information, and republished. The setup I have in mind:

  • Every day, a workflow checks what are the latest npm/pnpm/Yarn releases, updates the config file, and opens a PR
  • On each commit, check if the package.json version is on the npm registry; if it isn't, publish it

This would require the following:

  • Adding a npm publish token into the Corepack repository
  • (optional) Transfering the corepack package to ~nodejs-foundation
  • (optional) Adding a GH bot user token into the Corepack repository, for the workflow to open PRs

Ref nodejs/corepack#90

@aduh95
Copy link
Contributor

aduh95 commented Feb 10, 2022

/cc @nodejs/tsc

@mcollina
Copy link
Member

I do not think this is a great idea.
One of the main reason we adopted corepack was to avoid work in nodejs/node to support multiple package managers. There is a measurable amount of traffic on our H1 due to vulnerabilities in the npm dependencies, and my goal is to not increase it.

I propose that the config file include only the major version of each package manager. This will ensure that for a specific version of Node.js there won't be any breaking change.
(I consider the drop of a Node.js version a breaking change).
Then we will update the major version of each package manager whenever we ship a new major release of Node.js.

@arcanis
Copy link
Author

arcanis commented Feb 11, 2022

Limiting updates to major is a good idea, but it's tangential to having an automation - we'll still want to bump the minor / patch releases from the same major.

I propose that the config file include only the major version of each package manager. This will ensure that for a specific version of Node.js there won't be any breaking change.

The config file contains exact versions so as one Corepack / Node version always provides the exact same default pnpm / Yarn versions, mirroring the way you currently distribute npm.

@mcollina
Copy link
Member

Limiting updates to major is a good idea, but it's tangential to having an automation - we'll still want to bump the minor / patch releases from the same major.

No. I'm proposing to avoid the problem altogether and just specify the major version.

The config file contains exact versions so as one Corepack / Node version always provides the exact same default pnpm / Yarn versions, mirroring the way you currently distribute npm.

I would note that having a config file with the exact versions defeats one of the purposes of corepack.

I'm proposing a change so we can actually avoid the problem completely. Given corepack download the package managers dynamically, we should not have users install versions with known vulnerabilities or even bugs if a fix is available.

I do not think corepack should mirror what Node does with npm because we ship npm in source tree. This is a source of frequent reported vulnerabilities for Node.js (false positeves, usually RedDoS).

@arcanis
Copy link
Author

arcanis commented Feb 11, 2022

I'm proposing a change so we can actually avoid the problem completely. Given corepack download the package managers dynamically, we should not have users install versions with known vulnerabilities or even bugs if a fix is available.

Dynamic installs are a large topic, with some people seing them as a pro in terms of security (always up-to-date) while others see it as a con (faker.js situation). The current design avoids this problem by ensuring that the default versions are static (even if the binaries are lazy loaded), which matches the existing status quo.

In my opinion, changing to dynamic versioning is a separate topic that's worthy to be debated on its own (and if everyone agrees to the change then so be it, I personally don't have strong feelings either way), but that shouldn't block improvements on the current design in the meantime.

@aduh95
Copy link
Contributor

aduh95 commented Feb 11, 2022

I'm +1 changing the default of Corepack to always be the most up-to-date semver-minor/semver-patch of all supported package managers. IMHO being up-to-date is much preferable than keeping an insecure version forever.

I'm +1 having a bot that opens a PR to nodejs/corepack every time a new major version of npm or pnpm is released, and that opens a PR to nodejs/node every time a new version of Corepack is released.

@BethGriggs
Copy link
Member

+1 to @mcollina's suggestion.

I completely missed the fact that Corepack maintains a config file with specific versions in the original discussion. Even with automation to create the PRs to update Corepack, that means Corepack puts extra burden on the project to ship new releases any time one of the three package managers upgrade their defaults in the Corepack config.

To add, I struggled to find the config file in Node.js core as it's packed 14000 lines into corepack.js (thanks for the pointer to that, @richardlau).

@mhdawson
Copy link
Member

I agree with the sentiments in the last few comments. From my perspective the only reason some of us were willing to pull in corepack is because it avoided pulling the package managers into Node.js itself. If it's effectively the same thing and forces Node.js to do security updates when an update is needed in one of the package managers we just don't have the bandwidth to support that that (we already struggle with the CVEs etc. that we have to handle from including npm).

@aduh95
Copy link
Contributor

aduh95 commented Feb 16, 2022

On the TSC meeting it was decided to go on with adding an automation that opens a PR to nodejs/node every time a new version of Corepack is released. We have already a workflow in the core repo to update dependencies: https://github.com/nodejs/node/blob/HEAD/.github/workflows/tools.yml.
@arcanis would you like to open a PR to add a step on this workflow that checks for Corepack updates?

@arcanis
Copy link
Author

arcanis commented Feb 16, 2022

Sure, will do 👍

@arcanis
Copy link
Author

arcanis commented Feb 23, 2022

Opened nodejs/node#42090 to auto-update Corepack in Node. However my initial points were also to allow automated releases in the Corepack repository; is it possible?

Even if Corepack switches to dynamic versions, I'd still prefer to have some repository workflow for Corepack releases, and avoid having to release it by running commands from my own laptop.

@mhdawson
Copy link
Member

(optional) Transfering the corepack package to ~nodejs-foundation

I don't think that makes sense. None of the packages are owned by the user, instead we add the nodejs-foundation as a backup user so that if the prior collaborators are no longer around we can add new ones. We have talked about creating an org, and in that case it might make more sense. Since you indicated this was optional, it is probably best to handle/discuss this aspect as a follow on task to avoid blocking the key request.

I agree the concept of having a publishing flow that does not require an individual involvement makes sense. It is something we might like for node-addon-api as well (although not a big deal there as our releases are not too often).

Adding a npm publish token into the Corepack repository

My main question is with respect to 2fa/publishing. I think it's more complicated than just adding a token to the repo. @dominykas what would you suggestion on this front?

@mmarchini
Copy link
Contributor

According to our policy, we must add the nodejs-foundation user as admin for any project maintained package:

Create a user called nodejs-foundation who we always add as one of the collaborators with admin rights and for which the password is maintained by the Build Working Group.

So if that's not the case yet, this is the first step. I'm almost sure we have used this same user to automate other packages that have automated release. If not, we really need to settle on this, adding user-created automated tokens is not the way to go for a project of our size.

I'm +1 on automating the release flow for corepack by the way (I think we should do that for all our npm packages as part of our policy).

@mhdawson
Copy link
Member

If not, we really need to settle on this, adding user-created automated tokens is not the way to go for a project of our size.

I agree that user-created tokens should not be used for automation of publishing. I don't think we have automated publishing yet. There was also a suggestion that we should change the name of the nodejs-foundation user. This might be the right time to create a new user or org and start by using it for automation.

@targos
Copy link
Member

targos commented Feb 28, 2022

This might be the right time to create a new user or org and start by using it for automation.

+1. This would help unblocking nodejs/node-core-utils#511

@arcanis
Copy link
Author

arcanis commented Apr 7, 2022

What are the next steps regarding the publish bot user?

@targos
Copy link
Member

targos commented Apr 8, 2022

I guess it would be to create an automation token for the nodejs-foundation user and add it as a secret to the nodejs/corepack repo (like #618)?

@aduh95
Copy link
Contributor

aduh95 commented May 15, 2022

@mhdawson can you take care of the token creation please?

  1. Go to https://www.npmjs.com/settings/nodejs-foundation/tokens/new
  2. Enter the nodejs-foundation account's password
  3. Name: NPM_TOKEN secret to publish corepack ?
    Type: "Automation"
    Click "Generate Token"
  4. Copy the generated token (it starts with "npm_")
  5. Go to https://github.com/nodejs/corepack/settings/secrets/actions/new
  6. Name: NPM_TOKEN
    Value: the token you copied at 4.
    Click "Add secret"

@arcanis can you transfer the npm package ownership to nodejs-foundation please?

@mhdawson
Copy link
Member

@aduh95 done as requested above

@arcanis
Copy link
Author

arcanis commented May 17, 2022

@arcanis can you transfer the npm package ownership to nodejs-foundation please?

@aduh95 I invited nodejs-foundation to be an admin, feel free to remove me once you have confirmed it works fine on your end

@aduh95
Copy link
Contributor

aduh95 commented Jun 29, 2022

@mhdawson Can you make sure that nodejs-foundation is the only account that has release permission on npm please?

@mhdawson
Copy link
Member

Ok removed @arcanis as so the only maintainer is nodejs-foundation.

@aduh95 I assume I should also set the publishing access to be "Require two-factor authentication or automation tokens" right?

@aduh95
Copy link
Contributor

aduh95 commented Jun 30, 2022

I assume I should also set the publishing access to be "Require two-factor authentication or automation tokens" right?

That's correct.

Ok removed @arcanis as so the only maintainer is nodejs-foundation.

Thanks a lot! I assume the issue can now be closed :)

@arcanis arcanis closed this as completed Jun 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants