nodejs / corepack

Zero-runtime-dependency package acting as bridge between Node projects and their package managers
MIT License
2.3k stars 145 forks source link

downloads package managers only from npm by default #495

Open mcollina opened 3 weeks ago

mcollina commented 3 weeks ago

corepack supports downloading the package managers from a source that is not npm. I think this is a significant security risk for our users and we should only download them from npm (or another 3rd party registry, not just a URL).

aduh95 commented 3 weeks ago

The npm client also supports downloading from arbitrary URLs, if npm doesn't impose that restriction, it would be weird for Corepack to do IMO.

styfle commented 3 weeks ago

For more context, I created the original issue https://github.com/nodejs/corepack/issues/354 after the Node.js TSC meeting because some expressed that they didn't like the idea of corepack gatekeeping.

mcollina commented 3 weeks ago

The npm client does not download & execute libraries from the internet without telling its users. The gates here should be way tighter to guarantee the basic security of our users.

aduh95 commented 3 weeks ago

The npm client does not download & execute libraries from the internet without telling its users. The gates here should be way tighter to guarantee the basic security of our users.

? That does not seem to be the case

$ mkdir repro && (cd repro && echo '{"dependencies":{"yarn":"https://registry.yarnpkg.com/yarn/-/yarn-1.22.22.tgz"}}' > package.json && npm i) && rm -r repro

added 1 package, and audited 2 packages in 2s

found 0 vulnerabilities

No warnings at all, unless it's disabled in my env for some reason. What warning are you seeing?

mcollina commented 3 weeks ago

There are no warnings, but that is ok because the end user asked for it.

Running yarn after a corepack enable is absolutely not transparent to the end user. The npm registry acts as a 3rd party that give an extensive list of guarantees. Those guarantees are not available if a project is self-hosting their deps. The user would expect to have the same guarantees as npm.

aduh95 commented 3 weeks ago

Oh sorry for the confusion, I thought you were referring to the feature added in https://github.com/nodejs/corepack/pull/359. In that case, that's up to the Yarn team (cc @arcanis). From Corepack side, we could define requirements for what would be acceptable for a default registry – ideally we would not restrict it to npm public registry, but instead define a set of requirements (IIRC we discussed accepting only registries with an audited a signature process)

mcollina commented 3 weeks ago

I agree

merceyz commented 3 weeks ago

Running yarn after a corepack enable is absolutely not transparent to the end user.

What do you mean?

For whatever it's worth, the user is prompted if they want to download the package manager and where it's from:

$ docker run --rm -it node:22 bash
$ corepack enable
$ yarn
! Corepack is about to download https://registry.yarnpkg.com/yarn/-/yarn-1.22.22.tgz
? Do you want to continue? [Y/n]
mcollina commented 3 weeks ago

Would the user know the risk this poses? That's my point. The message seems official and safe enough.

mhdawson commented 2 weeks ago

@mcollina would adding to the wording help? Just for the sake of discussion would something like the following address your concern? The main reason I ask is to understand if its around the current message or something deeper?

! Corepack is about to download https://registry.yarnpkg.com/yarn/-/yarn-1.22.22.tgz. This is not part of the Node.js project and it's up to you in order to decide if this is safe to use. ? Do you want to continue? [Y/n]

mcollina commented 2 weeks ago

I think the messaging would help, but it's not enough. We should ask more guarantees from packages that don't want to publish their package managers to npm.

I think their package manager should:

  1. be signed
  2. verifiable on download
  3. the verifying key should either be part of corepack or downloaded from a known keychain. Sigstore might also be an option.

I'm worried that a domain takeover in 2-3 years might cause issues.

Considering that the yarn registry is currently a domain alias to npm, I think they could just switch to using npm there.

(On a side note, yarn is signing their release with gpg, so I think this could easy be done).


I also think that we should do this for all package managers, even those published to npm. The risk involved in running stuff almost automatically from the internet likes deserves this.

aduh95 commented 2 weeks ago

I also think that we should do this for all package managers, even those published to npm. The risk involved in running stuff almost automatically from the internet likes deserves this.

Isn't it already the case? Packages that are published on the npm public registry are signed by npm, and Corepack verifies the signature if the user did not provide a hash. What more would you like to see?

(On a side note, yarn is signing their release with gpg, so I think this could easy be done).

Verifying PGP signature would require quite a lot of work – and probably a significant bundle size increase – on the Corepack side. I'd much rather prefer if they'd use an ECC signature as npm public registry does nowdays.

mcollina commented 2 weeks ago

Isn't it already the case? Packages that are published on the npm public registry are signed by npm, and Corepack verifies the signature if the user did not provide a hash. What more would you like to see?

This is sufficient. However, it does not certify that the bundle comes from the maintainers. I would generically prefer that (even for other modules that I download from npm). Using provenance/sigstore is kind of great, too.

Anyhow, this is wishlist thing. My main concern is into having non-npm URLs in the default list.

arcanis commented 2 weeks ago

My main concern is into having non-npm URLs in the default list.

Guaranteeing that the build got authored by a known maintainer is a valid concern, but the place where those builds come from doesn't seem relevant once this check has been performed.

RedYetiDev commented 2 weeks ago

I'm worried that a domain takeover in 2-3 years might cause issues. ... My main concern is into having non-npm URLs in the default list.

If an attacker takes over the NPM domain, wouldn't it have the same implications, what's so special about non-NPM urls?