-
Notifications
You must be signed in to change notification settings - Fork 180
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
Comments
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.
cf how npm correlates $ 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. |
Thank you so much for the details and explanation! I realized that I can answer one of the questions myself.
The answer is yes, there is still an issue!
Even though the https://registry.npmjs.org/-/npm/v1/keys has been corrected, the latest version of Corepack ( The workarounds are to set the environment variable 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. |
This would be my recommendation. I am not a corepack maintainer though, so it's not my decision. |
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. |
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? |
|
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.
The same key
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 |
(EDIT: some of this is wrong, see messages below!)
TLDR: (I believe that) these signatures only guarantee integrity, and not authenticity.
For reference, NPM loads keys from Sigstore then falls back to the 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. If you still want to pin the npm registry's integrity keys, then I think:
|
I don't think that's true, what is signed is the concatenation of
So, let's not put any safeguards? I really don't follow the logic here
The registry exposes both |
See nodejs/corepack#616 (comment) Signed-off-by: Alexander Trost <galexrt@googlemail.com>
@aduh95 Thanks for catching that! Turns out, this part is also wrong:
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.
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.
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 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. |
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:
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 |
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 |
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. |
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:
|
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.
Easier said than done as we've been discussing in this issue. |
It's good to see that progress is being made to increase the resilience! |
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! |
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. |
I agree it’s be nice to be able to configure from the FS (e.g. something like |
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>
Issue Newly published versions of package managers distributed from npm cannot be installed due to key id mismatch #612 records a disruption to the corepack service after npm registry keys were updated.
Please don't make error of fetching latest version information if packageManager field is specified. #625 describes how a project with an existing
packageManager
record can be affected if a new package manager version is released, signed with a new key, and not used in the project.How can this be prevented in future? I'm not familiar with how the keys and systems for using them work, so this is a question for the experts.
In #612 (comment) @wraithgar wrote
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?
The text was updated successfully, but these errors were encountered: