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

downloads package managers only from npm by default #495

Open
mcollina opened this issue Jun 10, 2024 · 15 comments
Open

downloads package managers only from npm by default #495

mcollina opened this issue Jun 10, 2024 · 15 comments

Comments

@mcollina
Copy link
Member

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
Copy link
Contributor

aduh95 commented Jun 10, 2024

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
Copy link
Member

styfle commented Jun 10, 2024

For more context, I created the original issue #354 after the Node.js TSC meeting because some expressed that they didn't like the idea of corepack gatekeeping.

@mcollina
Copy link
Member Author

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
Copy link
Contributor

aduh95 commented Jun 11, 2024

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
Copy link
Member Author

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
Copy link
Contributor

aduh95 commented Jun 11, 2024

Oh sorry for the confusion, I thought you were referring to the feature added in #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)

@aduh95 aduh95 changed the title downloads package managers only from npm downloads package managers only from npm by default Jun 11, 2024
@mcollina
Copy link
Member Author

I agree

@merceyz
Copy link
Member

merceyz commented Jun 11, 2024

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
Copy link
Member Author

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

@mhdawson
Copy link
Member

@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
Copy link
Member Author

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
Copy link
Contributor

aduh95 commented Jun 13, 2024

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
Copy link
Member Author

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
Copy link
Contributor

arcanis commented Jun 13, 2024

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
Copy link
Member

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?

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

7 participants