Get NPM signing keys from @sigstore/tuf #647

geigerzaehler · 2025-02-11T10:40:09Z

Instead of hardcoding npm signing keys for verification we get them from sigstore’s TUF repository. This is in line with how npm implements signature verification.

Fixes #616, fixes #612

The implementation is ready to review, @aduh95. The only thing missing is a test for failed signature verification that does use COREPACK_INTEGRITY_KEYS. If you can think of more tests, let me know. I’m also not sure how the test setup works with regards to recording http requests.

Test that signature verification fails if providing an invalid signature for TUF keys.

geigerzaehler · 2025-02-11T13:03:26Z

This change also does not check for key expiry. This requires some more investigation and I’ll tackle this in a follow-up change.

sources/npmRegistryUtils.ts

aduh95 · 2025-02-11T22:48:17Z

This makes the bundle size go from 936.5kb to 2.4mb, do we actually need all that code?

aduh95 · 2025-02-12T07:45:21Z

Maybe we could try patching tuf-js to use undici instead of make-fetch-happen (https://github.com/theupdateframework/tuf-js/blob/0303dab1e09cd2c58b9707c4c2b57566c7df349e/packages/client/src/fetcher.ts#L3)

EDIT: I've tried doing that, if we're fine getting rid of the retry: 2 default option, with the following patch we're down to a 1.2MiB bundle:

diff --git a/dist/fetcher.js b/dist/fetcher.js
index f966ce1bb0cdc6c785ce1263f1faea15d3fe764c..cb361aafe39d48ffb1ce8fb0a909bcb958bec46f 100644
--- a/dist/fetcher.js
+++ b/dist/fetcher.js
@@ -6,7 +6,7 @@ Object.defineProperty(exports, "__esModule", { value: true });
 exports.DefaultFetcher = exports.BaseFetcher = void 0;
 const debug_1 = __importDefault(require("debug"));
 const fs_1 = __importDefault(require("fs"));
-const make_fetch_happen_1 = __importDefault(require("make-fetch-happen"));
+const stream_1 = __importDefault(require("stream"));
 const util_1 = __importDefault(require("util"));
 const error_1 = require("./error");
 const tmpfile_1 = require("./utils/tmpfile");
@@ -61,14 +61,13 @@ class DefaultFetcher extends BaseFetcher {
     }
     async fetch(url) {
         log('GET %s', url);
-        const response = await (0, make_fetch_happen_1.default)(url, {
-            timeout: this.timeout,
-            retry: this.retry,
+        const response = await globalThis.fetch(url, {
+            signal: this.timeout && AbortSignal.timeout(this.timeout),
         });
         if (!response.ok || !response?.body) {
             throw new error_1.DownloadHTTPError('Failed to download', response.status);
         }
-        return response.body;
+        return stream.Readable.fromWeb(response.body);
     }
 }
 exports.DefaultFetcher = DefaultFetcher;

aduh95 · 2025-02-12T08:28:42Z

config.json

-  "keys": {
-    "npm": [
-      {
-        "expires": "2025-01-29T00:00:00.000Z",
-        "keyid": "SHA256:jl3bwswu80PjjokCgh0o2w5c2U4LhQAE57gj9cz1kzA",
-        "keytype": "ecdsa-sha2-nistp256",
-        "scheme": "ecdsa-sha2-nistp256",
-        "key": "MFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAE1Olb3zMAFFxXKHiIkQO5cJ3Yhl5i6UPp+IhuteBJbuHcA5UogKo0EWtlWwW6KSaKoTNEYL7JlCQiVnkhBktUgg=="
-      },
-      {
-        "expires": null,
-        "keyid": "SHA256:DhQ8wR5APBvFHLF/+Tc+AYvPOdTpcIDqOhxsBHRwC7U",
-        "keytype": "ecdsa-sha2-nistp256",
-        "scheme": "ecdsa-sha2-nistp256",
-        "key": "MFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEY6Ya7W++7aUPzvMTrezH6Ycx3c+HOKYCcNGybJZSCJq/fd7Qa8uuAKtdIkUQtQiEKERhAmE5lMMJhP8OkDOa2g=="
-      }
-    ]


Shouldn't we keep this as a fallback?

Not sure. This would only make a difference if fall back to them if we can’t fetch the sigstore keys. I’m a bit worried that this redundancy will obscure problems that, in the end, lead to the same problem as we have now.

This is because the fallback mechanism will break if a new key is added. This will not be announced and it will stay broken unless we maintain it. But we may not notice it is broken because the fallback is not triggered. So it looks like forever maintenance burden that doesn’t actually add that much.

Then again, I might be missing something. So maybe it would help to clarify what concrete problem this would solve

Well we would have tests for it, that's how we detected it this time around, I expect it would not be different next time

Ok. Can you elaborate how we should do the fallback? Should we use the hard-coded keys when the TUF requests fails? Do we want to distinguish between different failures (i.e. network vs verification, etc.)? Or should we include the hard-coded keys in case they are missing from the returned from the TUF client?

IMO the trust hierarchy should be:

env (COREPACK_INTEGRITY_KEYS)

TUF

config.json

We should use the most trusted key set at our disposal, and hard fail only if signature doesn't match / the key is unknown (or known to be expired, but support for key expiry should be a separate PR IMO).
So a network issue should simply fallback to config.json IMO, and not be reported to the user (or rather only with debugUtils.log).

I gave this a bit more thought and I believe the extra keys will decrease the overall security by increasing the attack surface.

If an attacker can compromise either of the keys they can trick into accepting forged signatures.

If a key is compromised, it has to be revoked from both TUF and config.json.

The problem is that there is no trust hierarchy, either we accept signatures from a key or we don’t. Because trust is so important, my recommendation would be to limit the points of trust as much as possible and not include hard-coded keys.

To elaborate a bit on 1: In practice this probably does not make a huge difference because if somebody can compromise the hard-coded key in the source they can most likely also compromise the dependency code in the bundle.

From a usability perspective relying just on TUF is not a huge issue either, I believe. If it is a transient issue (i.e. network, service availability) we can provide a meaningful error an users can always retry later. This happens all the time and any user of npm will be similarly affected. Any other issue is either an attack or a change in the signature mechanism. In the first case, we should fail. In the second case there will be a migration period, again npm also has to phase out the mechanism.

In any case, the user can always workaround the issue by setting COREPACK_INTEGRITY_KEYS.

What do you think? Does this make sense?

2. If a key is compromised, it has to be revoked from both TUF and config.json.

Why would that be the case? It can be revoked only on TUF since it's up the trust hierarchy (that is, until Corepack is updated and config.json contains up-to-date info)

Why would that be the case? It can be revoked only on TUF since it's up the trust hierarchy (that is, until Corepack is updated and config.json contains up-to-date info)

I think we have to clarify first what the different options for the fallback mechanism are:

If the TUF request is successful we use that as the key set, otherwise we use the hard-coded keys

We use the union of the TUF key set and the hard-coded keys. If the TUF request fails we can only use the hard-coded keys.

I don’t know what the process for key revocation is exactly, since it is not documented. But from the implementation in npm I assume a revoked key will be removed from keys.json. If mechanism 2 is implemented we will still use the revoked key. If mechanism 1 is implemented an attacker just needs to interfere with the TUF connection so that it errors. (This is pretty easy with DNS poisoning, TCP reset attck, denial of service.) This will force corepack to downgrade and use the compromised hard-code keys.

I have the feeling that there is a good chance that we’ll make a mistake when implementing a fallback mechanism that leaves the system vulnerable. And the discussion seems to confirm that there are still a lot of open questions around this. I would prefer to follow best practices and rely on established systems instead of implementing a custom system.

Using a union would be a plain bad idea, it is not at all my suggestion. My suggestion is to fallback on the hard coded keys when TUF is not available – if TUF ever goes down, we shouldn't have to make emergency releases of Corepack; let's not replace a single point of failure with another single point of failure.

Check the env

If nothing on the env, check the TUF cache

If it's expired or missing, download an updated version:

If it fails, fallback on the old cache.

If there's no cache, fallback on the hard coded keys.

Ok. I have to say that I disagree with this approach since it weakens security and you haven’t addressed the concerns about possible attacks. But I then again, it is not less secure than the current system. So I’ll defer the decision to you and will implement it as suggested.

geigerzaehler · 2025-02-12T10:29:21Z

EDIT: I've tried doing that, if we're fine getting rid of the retry: 2 default option, with the following patch we're down to a 1.2MiB bundle:

That’s great. Would you like me to add this as a yarn patch? Or do you want to get this into tuf-js?

aduh95 · 2025-02-12T10:39:17Z

EDIT: I've tried doing that, if we're fine getting rid of the retry: 2 default option, with the following patch we're down to a 1.2MiB bundle:

That’s great. Would you like me to add this as a yarn patch? Or do you want to get this into tuf-js?

I think we should add it as a Yarn patch. I can certainly send a PR their way (theupdateframework/tuf-js#849), but given it would be a major change for them, I don't think it would make sense for us to wait on them.

sources/npmRegistryUtils.ts

Instead of hardcoding NPM signing keys for verification we get them from sigstore’s TUF repository. This is in line with how npm implements signature verification. Fixes nodejs#616, nodejs#612

geigerzaehler · 2025-02-13T13:55:18Z

I’ve implemented the test and addressed all the feedback. This is ready for review now

aduh95 · 2025-02-13T15:49:17Z

.yarn/patches/tuf-js-npm-3.0.1-9135d15fbd.patch

+-            timeout: this.timeout,
+-            retry: this.retry,
+        const response = await globalThis.fetch(url, {
+            timeout: this.timeout && AbortSignal.timeout(this.timeout)


Suggested change

+ timeout: this.timeout && AbortSignal.timeout(this.timeout)

+ signal: this.timeout && AbortSignal.timeout(this.timeout)

Fixed in db10011

aduh95 · 2025-02-13T16:00:36Z

As pointed out in theupdateframework/tuf-js#849 (review), we should think about proxy support in our patch (ideally using the same one as Corepack does)

aduh95 · 2025-02-13T16:11:28Z

sources/npmRegistryUtils.ts

+    debugUtils.log(`Using COREPACK_INTEGRITY_KEYS to verify signatures: ${keys.map(k => k.keyid).join(`, `)}`);
+    return keys.map(k => ({
+      keyid: k.keyid,
+      key: crypto.createPublicKey(`-----BEGIN PUBLIC KEY-----\n${k.key}\n-----END PUBLIC KEY-----`,


Creating the keys at this point seems like pure overhead, we only need the one key that matches the keyid in the package metadata (if it exists), why bother?

Makes sense. Fixed in db10011

geigerzaehler · 2025-02-17T12:03:37Z

As pointed out in theupdateframework/tuf-js#849 (review), we should think about proxy support in our patch (ideally using the same one as Corepack does)

Addressed in 3a2115e

README.md

Co-authored-by: Mike McCready <[email protected]>

geigerzaehler · 2025-02-24T10:04:22Z

It looks like we can’t just use replayed responses for TUF request because @sigstore/tuf checks timestamps and those in the recorded responses become stale and the client throws an error. I’m leaning towards not recording TUF requests and rely on network requests in the tests. Alternatively, we could mock the current date. This would be a bit more cumbersome to deal with when requests are re-recorded and would depend on #654. WDYT @aduh95?

OR13 · 2025-02-24T14:19:48Z

Replay with mocked current time seems most aligned with the intention of the tests.

cupofjoakim · 2025-03-18T08:25:30Z

Is this work ongoing or has it stalled?

geigerzaehler · 2025-04-01T07:03:12Z

I’m working on rebasing this and fixing the last issues. I hope to get this done next week.

geigerzaehler force-pushed the signature-keys-tuf branch from e723302 to b1df5fd Compare February 11, 2025 10:50

This comment was marked as resolved.

Sign in to view

OR13 reviewed Feb 11, 2025

View reviewed changes