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

Saveguards for keyid mismatch #616

Open
MikeMcC399 opened this issue Jan 29, 2025 · 21 comments
Open

Saveguards for keyid mismatch #616

MikeMcC399 opened this issue Jan 29, 2025 · 21 comments

Comments

@MikeMcC399
Copy link
Contributor

MikeMcC399 commented Jan 29, 2025


In #612 (comment) @wraithgar wrote

https://registry.npmjs.org/-/npm/v1/keys should now be returning two keys. During key rotation the old one was omitted by mistake and has since been re-added.

Even if a mistake hadn't been made, would there still have been an issue needing keys urgently updating in Corepack?

Does Corepack need to know in advanced if npm registry keys on the endpoint https://registry.npmjs.org/-/npm/v1/keys (see https://docs.npmjs.com/about-registry-signatures) are going to be changed?

If npm registry keys are changed, is there any advanced notice from the npm registry of a plan to change?

Is there any notification that npm registry keys have changed, at the time of change or does the endpoint need to be polled by Corepack regularly?

Can Corepack ensure that the version bundled in Node.js will always have a set of keys which can be used without error?


Also: the more fundamental question of whether Corepack should have a copy of the npm registry keys with the risk of them becoming out of date? Could Corepack simply use the live keys on https://registry.npmjs.org/-/npm/v1/keys instead of keeping a copy in the repo?

@wraithgar
Copy link

If npm registry keys are changed, is there any advanced notice from the npm registry of a plan to change?

There shouldn't be. The values of the keys aren't something corepack needs to know beforehand. If a local reference is needed they can be cached. The response has http caching headers that should inform clients how long to cache.

expires is simply used to denote the latest date that a published package should have been signed by a given key.
keyid is the id used to correspond a package's signature with a key for signature verification. Both are present in registry results, based on when the package was published.

cf how npm correlates keyid and validates expires, and how corepack correlates keyid (it does not appear that corepack validates the expires attribute of a key)

$ npm view tailwindcss dist.signatures
{
  sig: 'MEUCIQCxnGQkT4CGZFjUwh22KPbzju3jG5+gjSTF1Y9kex1mbQIgUfmmePbMOC0Iv7GsQRPM3a+2Q8Jv5JqgYj4dKc9pDW8=',
  keyid: 'SHA256:DhQ8wR5APBvFHLF/+Tc+AYvPOdTpcIDqOhxsBHRwC7U'
}
$ npm view npm dist.signatures
{
  sig: 'MEYCIQCdbIJJFvhCU6D3kO7inpSRAXC930c2CLz8JqtNeP5D+QIhAJBhj+7nJMlGtsx1MqETvG5+h6TMc5ZiUo0/MUtjHm3e',
  keyid: 'SHA256:jl3bwswu80PjjokCgh0o2w5c2U4LhQAE57gj9cz1kzA'
}

The endpoint has always returned an array in anticipation of key rotation. Old keys will remain in the returned data set for as long as there are artifacts in the registry that are signed by those keys.

Trying to keep the contents of that endpoint in the source code in a way that's decoupled from the source (i.e. not an http response cache) seems like it's guaranteed to have problems when those contents change. The current setup also prevents other registries from being used, as it appears to be indexed by package name, not by registry hostname.

@MikeMcC399
Copy link
Contributor Author

MikeMcC399 commented Jan 29, 2025

@wraithgar

Thank you so much for the details and explanation!

I realized that I can answer one of the questions myself.

Even if a mistake hadn't been made, would there still have been an issue needing keys urgently updating in Corepack?

The answer is yes, there is still an issue!

pnpm@10.0.0 was signed with the older key (which expired earlier today 2025-01-29T00:00:00.000Z, meaning it shouldn't get used for signing any new packages)

$ npm view pnpm@10.0.0 dist.signatures
{
  sig: 'MEUCIBhQnfDt9V8tw3FnrqHdMokyqJJtYX7HhR5NxvfggwP/AiEAiQQ74inA/JVI5IHN0piTLb2LhSUPJAkYYsGgM8DTrCI=',
  keyid: 'SHA256:jl3bwswu80PjjokCgh0o2w5c2U4LhQAE57gj9cz1kzA'
}

pnpm@10.1.0 was signed with the new key. I assume that currently any future pnpm or Yarn releases will be using this key until it itself is rotated and set to expire.

$ npm view pnpm@10.1.0 dist.signatures
{
  sig: 'MEUCIQDlkgmNyZjT7KUY8AO6jH7Gs3fyiXG8nbTnuLbd8fOS2AIgXyJ6SaYhumMFzUYQAZPJGhsnlaD5N0X2MZsbG+eS/Xo=',
  keyid: 'SHA256:DhQ8wR5APBvFHLF/+Tc+AYvPOdTpcIDqOhxsBHRwC7U'
}

Even though the https://registry.npmjs.org/-/npm/v1/keys has been corrected, the latest version of Corepack ( 0.30.0 ) distributed with Node.js fails to be able to install pnpm@10.1.0.

The workarounds are to set the environment variable COREPACK_INTEGRITY_KEYS=0, to manually update to corepack@0.31.0 or to install it using knowledge of the release's hash.

Any other pnpm or Yarn package published to the npm registry from today onwards is going to potentially hit this key mismatch issue.

If I have understood this correctly it would mean that Corepack would need to be enhanced to always use the live keys on https://registry.npmjs.org/-/npm/v1/keys and not use its own stored keys which could become out of date.

@wraithgar
Copy link

If I have understood this correctly it would mean that Corepack would need to be enhanced to always use the live keys on https://registry.npmjs.org/-/npm/v1/keys and not use its own stored keys which could become out of date.

This would be my recommendation. I am not a corepack maintainer though, so it's not my decision.

@aduh95
Copy link
Contributor

aduh95 commented Jan 29, 2025

Wouldn't making a HTTP call defeat the purpose of the signature verification? If the purpose of the signature is to have a safeguard in case someone makes a MitM attack on the registry, relying on the registry to get the keys would defeat that purpose. I'm not sure we can avoid shipping a list of trusted keys.

@MikeMcC399
Copy link
Contributor Author

@wraithgar

Are you able to predict how often keys in https://registry.npmjs.org/-/npm/v1/keys are likely to be rotated and therefore what the risk of Corepack breaking again would be if Corepack does not address this issue?

@MikeMcC399
Copy link
Contributor Author

@MikeMcC399
Copy link
Contributor Author

MikeMcC399 commented Feb 2, 2025

Looking back in the history, for instance of npm releases, the earliest release shown on https://www.npmjs.com/package/npm is https://www.npmjs.com/package/npm/v/1.1.25 published 12 years ago.

$ npm view npm@1.1 dist.signatures.keyid
npm@1.1.25 'SHA256:jl3bwswu80PjjokCgh0o2w5c2U4LhQAE57gj9cz1kzA'
npm@1.1.70 'SHA256:jl3bwswu80PjjokCgh0o2w5c2U4LhQAE57gj9cz1kzA'
npm@1.1.71 'SHA256:jl3bwswu80PjjokCgh0o2w5c2U4LhQAE57gj9cz1kzA'

The same key jl3bwswu80PjjokCgh0o2w5c2U4LhQAE57gj9cz1kzA was used all these years until npm@11.1.0 was published on Jan 29, 2025:

$ npm view npm@11 dist.signatures.keyid
npm@11.0.0 'SHA256:jl3bwswu80PjjokCgh0o2w5c2U4LhQAE57gj9cz1kzA'
npm@11.1.0 'SHA256:DhQ8wR5APBvFHLF/+Tc+AYvPOdTpcIDqOhxsBHRwC7U'

If the signing keys on https://registry.npmjs.org/-/npm/v1/keys in future are only going to be rotated every 12 years, then this issue does not represent a practical risk for Corepack users.

If on the other hand there is a new process to rotate the keys more often, then this could represent a risk for Corepack users experiencing a time window where new keys have not propagated through a new Corepack release and distribution. During this time packages signed with a new key would fail unless additional steps are taken by users to disable integrity checks or add a hash in corepack commands.

@LucienLeMagicien
Copy link

LucienLeMagicien commented Feb 2, 2025

Wouldn't making a HTTP call defeat the purpose of the signature verification? If the purpose of the signature is to have a safeguard in case someone makes a MitM attack on the registry, relying on the registry to get the keys would defeat that purpos. I'm not sure we can avoid shipping a list of trusted keys.

(EDIT: some of this is wrong, see messages below!)
I am not a security professional. However, I am not sure this attack vector (an attacker manages to MitM/impersonate registry.npmjs.org) is relevant in this situation because:

  • The documentation says that this is specifically geared against malicious proxies or mirrors doing MitM. It dosn't say that it is geared against a direct HTTPS MitM on the registry, or a malicious registry.
    (To a lesser extent, I think it is also geared against local cache poisoning. This is for example relevant when using Yarn zero-install: you have to trust that whoever can write to your repo isn't uploading bad packages, or validate it in CI. See the v3 doc and yarn v4's hardened mode.)
  • This signature verification mechanism has no way of knowing whether or not the keys are "trusted", outside of the facts that they come from registry.npmjs.org and trusting TLS.
    (Compare with TLS certs or Debian/Arch/RedHat packages: they have a "chain of trust" or "root of trust" where a trust anchor has signed these keys. The local package manager can verify that new keys are trusted (signed) by a key it already trusts.)
  • If someone manages to MitM between you and the registry over HTTPS, you have bigger problems:
    • It means they managed to somehow trick your system/your CI into trusting a malicious cert (malicious root CA, TLS downgrade attack, ...)
    • Even if the keys were pinned so that Corepack wasn't fetching them via HTTPS, since the attacker controls what you receive they could simply send you data for another package entirely: by simply uploading a malicious package to NPM, they can get a package with a valid signature that Corepack would accept. The only way to guard against this would be to instead pin the hash of each package version.
    • Even if you pin the hash yourself (like how @MikeMcC399 explained here: Newly published versions of package managers distributed from npm cannot be installed due to key id mismatch #612 (comment)) so that you are sure that the package you receive is a trusted one, I think npm itself (and I assume other package managers) does not pin the registry's keys and would trust what it gets over HTTPS and the attack would go through.
      Trying to have Corepack act as a bonus rogue-key-detector doesn't make sense to me.
  • If someone manages to hack into registry.npmjs.org, we have bigger problems.

TLDR: (I believe that) these signatures only guarantee integrity, and not authenticity.
This is instead where projects like TUF and Sigstore fit:

For reference, NPM loads keys from Sigstore then falls back to the /-/npm/v1/keys endpoint here:
https://github.com/npm/cli/blob/593c84921b0df963cef2ca7b13e44acc20cbd558/lib/utils/verify-signatures.js#L174
(Note that I don't know how to use TUF / Sigstore but I believe this is the relevant part. I also haven't looked into how other package managers get their keys.)


Given this, it makes sense to me that Corepack would retrieve the keys via HTTPS to verify download integrity. I believe that Sigstore is what you're looking for if you want to make sure that the package managers that Corepack downloads are the ones released by a trusted source.
Again though, I am not a security professional so hopefully someone more apt can chip in.

If you still want to pin the npm registry's integrity keys, then I think:

  • This project should have a scheduled job to see if the endpoint returns a new key and notify a human. The job should at least run weekly, given the endpoint's cache headers.
  • If this error happen again, Corepack should query the endpoint anyway to see if there is a new key and prompt the user to pass a flag to trust the new keys if one matches, and/or instruct them on how to set the hash for their package manager in their package.json (I believe this is what this issue is for Expand packageManager description with allowed & recommended hash values #620).

@aduh95
Copy link
Contributor

aduh95 commented Feb 2, 2025

  • Even if the keys were pinned so that Corepack wasn't fetching them via HTTPS, since the attacker controls what you receive they could simply send you data for another package entirely: by simply uploading a malicious package to NPM, they can get a package with a valid signature that Corepack would accept. The only way to guard against this would be to instead pin the hash of each package version.

I don't think that's true, what is signed is the concatenation of ${packageName}@${version}:${integrity}, so it would only work if the package name and version matches.

  • If someone manages to hack into registry.npmjs.org, we have bigger problems.

So, let's not put any safeguards? I really don't follow the logic here

TLDR: (I believe that) these signatures only guarantee integrity, and not authenticity.

The registry exposes both dist.integrity (and dist.shasum) to validate the integrity, and dist.signatures to validate the authenticity.

@LucienLeMagicien
Copy link

LucienLeMagicien commented Feb 3, 2025

@aduh95 Thanks for catching that! Turns out, this part is also wrong:

I think npm itself (and I assume other package managers) does not pin the registry's keys and would trust what it gets over HTTPS and the attack would go through.

At least in CI, npm (and other package managers) would have the package hash in the lockfile, so (even if signatures didn't include the package name/version as I initially thought) it would still not go through since npm would see it's not receiving the right packages.

So, let's not put any safeguards? I really don't follow the logic here

Because of my erroneous understanding, I thought that these safeguards wouldn't actually guard against anything and that this wasn't part of the threat model.
I see now that it would instead mean Corepack would be the only tool that wouldn't detect it in CI, since it doesn't have an equivalent to lockfiles (unless the package's hash is pinned - or if it ships the keys to verify the signatures as it does now).

The registry exposes both dist.integrity (and dist.shasum) to validate the integrity, and dist.signatures to validate the authenticity.

Right, I meant "package authenticity" (as in build provenance) rather than "signature authenticity" here. From what I understand, this signature only allows to verify that the one who has the key is also the one that sends the package (both signature and integrity can be forged if someone impersonates the registry - this is meant to catch malicious mirrors/proxies).
Given that Corepack currently pins the keys, here this effectively verifies that registry.npm.org isn't being impersonated.

I meant to say that I thought Corepack should rely on provenance statements instead(?), but it seems neither npm, yarn or pnpm currently publish it anyway.

Given this, I believe you're right that Corepack can't avoid shipping a list of trusted keys currently.

@aduh95
Copy link
Contributor

aduh95 commented Feb 3, 2025

Some shower thoughts on this topic: we technically could have a trusted public/private key pair that would allow Corepack maintainers to sign registry keys without needing to release a new version every time a registry rotates its keys. However this creates additional challenges such as:

  • how do we generate the private key and store it without introducing more vulnerabilities (not the hardest problem, but still).
  • how do we deal with that private key leaking.
  • how do we protect Corepack user from a maintainer (either mistakenly or maliciously) signing a key that should not be trusted.

tbh I don't know if that would be worth the effort, I suspect most folks would be perfectly fine trusting only HTTPS (and therefor be confortable setting COREPACK_INTEGRITY_KEYS to 0 in their env, the signature verification is only a redundancy for TLS), and while the transition period is annoying (from the time the new registry key is released, and the time the version of Corepack that integrates it is deployed everywhere), hopefully it's rather rare.

@LucienLeMagicien
Copy link

Yeah I thought so as well, it scratches the engineer's itch but it doesn't seem worth it.

I still think there should be a scheduled CI job to look for key updates (like how it's done for latest versions in .github/workflows/sync.yml), and that Corepack should at least display a better error with a list of workarounds when this particular thing happens, and ideally even gracefully recover from it (using a known-good version if the user didn't pin a version?)

@wraithgar
Copy link

wraithgar commented Feb 3, 2025

TLDR: (I believe that) these signatures only guarantee integrity, and not authenticity.
This is instead where projects like TUF and Sigstore

The npm cli uses TUF to fetch the keys. It also falls back to a direct request from the registry if that fails, and perhaps a warning should be logged when this happens.

@MikeMcC399
Copy link
Contributor Author

Is it possible for npm to estimate how often they expect to rotate keys, or is this simply an unknown?

There is significant lead time for a Corepack release to be updated everywhere it is used. The issue is not resolved simply by quickly releasing a new version of Corepack. A scenario of an estimated 4 to 8 weeks needs to be covered where the Corepack version in use lags behind the latest release.

Corepack needs to be resilient by default if npm rotates keys (again).

Some factors affecting change and rollout:

  • npm Cache-control max-age 7 days
  • Corepack release
  • Node.js Current release
  • Node.js LTS release (2 weeks additional after Current release)
  • CI provider delay such as GitHub Actions or CircleCI
  • Docker builds
  • End-user CI configuration updates

@aduh95
Copy link
Contributor

aduh95 commented Feb 4, 2025

Worth noting that as long as the next key rotation happens after the fix for #625 is released and deployed everywhere, the consequences should be not as dramatic next time around, i.e. only folks who need to use a latest/non-pinned version would be affected instead of everyone.

Corepack needs to be resilient by default if npm rotates keys (again).

Easier said than done as we've been discussing in this issue.

@MikeMcC399
Copy link
Contributor Author

@aduh95

It's good to see that progress is being made to increase the resilience!

@wraithgar
Copy link

Is it possible for npm to estimate how often they expect to rotate keys, or is this simply an unknown?

This is an unknown currently.

@MikeMcC399
Copy link
Contributor Author

@MikeMcC399
Copy link
Contributor Author

@wraithgar

Is it possible for npm to estimate how often they expect to rotate keys, or is this simply an unknown?

This is an unknown currently.

I appreciate your candid response! If npm could at least hold back on any further key rotation until the proposed enhancements mentioned here are implemented and rolled out, that would be helpful!

@zkochan
Copy link

zkochan commented Feb 4, 2025

ok, I understand that fetching this via HTTP is problematic but could it be loaded from the filesystem? In that case users could download it themselves and commit to the repository.

@aduh95
Copy link
Contributor

aduh95 commented Feb 4, 2025

I agree it’s be nice to be able to configure from the FS (e.g. something like .corepack.env), but unfortunately support for it would be lacking on v18.x.

pranavrajs pushed a commit to chatwoot/chatwoot that referenced this issue Feb 5, 2025
So, a while back Circle CI builds and Heroku builds started to fail. 

From all the threads I read, it seems like the [npm registry rotated
it's signing
keys](pnpm/pnpm#9014 (comment))
New pnpm versions were signed with the new key. Corepack, however,
bundles a static set of trusted keys (from Node’s release), so it
continued verifying signatures only against the old key. When it
encountered packages signed with the new key, Corepack’s integrity check
failed with “Cannot find matching keyid” errors.This mismatch caused
Corepack’s integrity check to fail with “Cannot find matching keyid”
errors.

Workarounds include the following

1. Updating Corepack (to 0.31.0), they [upgraded their
package](https://github.com/nodejs/corepack/releases/tag/v0.31.0) to
include the new integrity check keys. But we seldom control what's going
on with the CI, also, updating this across our scripts is going to be a
painful task. Besides Heroku has [made some
fixes](heroku/buildpacks-nodejs#1010) around
this
2. Disabling integrity checks 🔥 #YOLO
3. Pinning `pnpm` to older versions, or pinning it to a newer version
with the checksum in place.

Doing the third one here, running `corepack use pnpm@9.15.5` fixes this,
[ref](pnpm/pnpm#9014 (comment))

We can get rid of this over time as CDN caches used by build systems are
refreshed. But the change in this PR is not disruptive in anyway, only
rigidly secure.

Fixes: #10832

---

Here are the threads to follow 

- pnpm/pnpm#9014
- pnpm/pnpm#9029
- nodejs/corepack#612
- nodejs/corepack#616
- heroku/buildpacks-nodejs#1010

---------

Co-authored-by: Vishnu Narayanan <vishnu@chatwoot.com>
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

5 participants