fix: bug to support rotated keys in signature/attestation audit (#338)

*Context*

`npm audit signatures` performs the registry signature and sigstore
attestation bundle verification in `pacote`.

The current code checks if the public key from the tuf trust root keys
target has expires set to in the past:
https://github.com/npm/pacote/blob/main/lib/registry.js#L174-L175

If we decide to rotate signing keys and add expires to a old public key,
verification will always fail saying the key for old packages has
expired.

This means we can't rotate signing keys for npm at the moment!

*Solution*

Check public key expiry against either `integratedTime` for attestations
or the publish time for registry signatures.

This allows us to rotate a key, setting expiry to after all packages
that where signed with that key where published.

Complication: some really old npm packages don't have `time` set so we
need some kind of cutoff date for these packages.

Having time in the packument also requires the npm/cli to fetch the full
manifest, not the minified packument that does not contain time.

This will restrict usage in the npm/cli install loop.

Some other solutions I considered where backfilling packages with a
`signedAt` timestamp in the `dist` object but this is a pretty big
effort.

Signed-off-by: Philip Harrison <[email protected]>

Author: feelepxyz

lib/registry.js

@@ -14,6 +14,10 @@ const sigstore = require('sigstore')
 const corgiDoc = 'application/vnd.npm.install-v1+json; q=1.0, application/json; q=0.8, */*'
 const fullDoc = 'application/json'
 
+// Some really old packages have no time field in their packument so we need a
+// cutoff date.
+const MISSING_TIME_CUTOFF = '2015-01-01T00:00:00.000Z'
+
 const fetch = require('npm-registry-fetch')
 
 const _headers = Symbol('_headers')
@@ -115,6 +119,13 @@ class RegistryFetcher extends Fetcher {
       return this.package
     }
 
+    // When verifying signatures, we need to fetch the full/uncompressed
+    // packument to get publish time as this is not included in the
+    // corgi/compressed packument.
+    if (this.opts.verifySignatures) {
+      this.fullMetadata = true
+    }
+
     const packument = await this.packument()
     let mani = await pickManifest(packument, this.spec.fetchSpec, {
       ...this.opts,
@@ -124,6 +135,12 @@ class RegistryFetcher extends Fetcher {
     mani = rpj.normalize(mani)
     /* XXX add ETARGET and E403 revalidation of cached packuments here */
 
+    // add _time from packument if fetched with fullMetadata
+    const time = packument.time?.[mani.version]
+    if (time) {
+      mani._time = time
+    }
+
     // add _resolved and _integrity from dist object
     const { dist } = mani
     if (dist) {
@@ -171,8 +188,10 @@ class RegistryFetcher extends Fetcher {
                   'but no corresponding public key can be found'
               ), { code: 'EMISSINGSIGNATUREKEY' })
             }
-            const validPublicKey =
-              !publicKey.expires || (Date.parse(publicKey.expires) > Date.now())
+
+            const publishedTime = Date.parse(mani._time || MISSING_TIME_CUTOFF)
+            const validPublicKey = !publicKey.expires ||
+              publishedTime < Date.parse(publicKey.expires)
             if (!validPublicKey) {
               throw Object.assign(new Error(
                   `${mani._id} has a registry signature with keyid: ${signature.keyid} ` +
@@ -254,8 +273,13 @@ class RegistryFetcher extends Fetcher {
                 ), { code: 'EMISSINGSIGNATUREKEY' })
               }
 
-              const validPublicKey =
-                !publicKey.expires || (Date.parse(publicKey.expires) > Date.now())
+              const integratedTime = new Date(
+                Number(
+                  bundle.verificationMaterial.tlogEntries[0].integratedTime
+                ) * 1000
+              )
+              const validPublicKey = !publicKey.expires ||
+                (integratedTime < Date.parse(publicKey.expires))
               if (!validPublicKey) {
                 throw Object.assign(new Error(
                   `${mani._id} has attestations with keyid: ${keyid} ` +

test/registry.js

@@ -186,13 +186,13 @@ t.test('verifySignatures valid signature', async t => {
   t.ok(mani._integrity)
 })
 
-t.test('verifySignatures expired signature', async t => {
+t.test('verifySignatures expired key', async t => {
   const f = new RegistryFetcher('@isaacs/namespace-test', {
     registry,
     cache,
     verifySignatures: true,
     [`//localhost:${port}/:_keys`]: [{
-      expires: '2010-01-01',
+      expires: '2010-01-01T00:00:00.000Z',
       keyid: 'SHA256:jl3bwswu80PjjokCgh0o2w5c2U4LhQAE57gj9cz1kzA',
       keytype: 'ecdsa-sha2-nistp256',
       scheme: 'ecdsa-sha2-nistp256',
@@ -210,6 +210,45 @@ t.test('verifySignatures expired signature', async t => {
   )
 })
 
+t.test('verifySignatures rotated keys', async t => {
+  const f = new RegistryFetcher('@isaacs/namespace-test', {
+    registry,
+    cache,
+    verifySignatures: true,
+    [`//localhost:${port}/:_keys`]: [{
+      expires: '2020-06-28T18:46:27.981Z', // Expired AFTER publish time: 2019-06-28T18:46:27.981Z
+      keyid: 'SHA256:jl3bwswu80PjjokCgh0o2w5c2U4LhQAE57gj9cz1kzA',
+      keytype: 'ecdsa-sha2-nistp256',
+      scheme: 'ecdsa-sha2-nistp256',
+      // eslint-disable-next-line max-len
+      key: 'MFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAE1Olb3zMAFFxXKHiIkQO5cJ3Yhl5i6UPp+IhuteBJbuHcA5UogKo0EWtlWwW6KSaKoTNEYL7JlCQiVnkhBktUgg==',
+      // eslint-disable-next-line max-len
+      pemkey: '-----BEGIN PUBLIC KEY-----\nMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAE1Olb3zMAFFxXKHiIkQO5cJ3Yhl5i6UPp+IhuteBJbuHcA5UogKo0EWtlWwW6KSaKoTNEYL7JlCQiVnkhBktUgg==\n-----END PUBLIC KEY-----',
+    }, {
+      expires: null,
+      keyid: 'SHA256:123',
+      keytype: 'ecdsa-sha2-nistp256',
+      scheme: 'ecdsa-sha2-nistp256',
+      // eslint-disable-next-line max-len
+      key: '123',
+      // eslint-disable-next-line max-len
+      pemkey: '-----BEGIN PUBLIC KEY-----\n123\n-----END PUBLIC KEY-----',
+    }],
+  })
+  const mani = await f.manifest()
+  t.match(mani, {
+    // eslint-disable-next-line max-len
+    _integrity: 'sha512-5ZYe1LgwHIaag0p9loMwsf5N/wJ4XAuHVNhSO+qulQOXWnyJVuco6IZjo+5u4ZLF/GimdHJcX+QK892ONfOCqQ==',
+    _signatures: [
+      {
+        keyid: 'SHA256:jl3bwswu80PjjokCgh0o2w5c2U4LhQAE57gj9cz1kzA',
+        // eslint-disable-next-line max-len
+        sig: 'MEQCIHXwKYe70+xcDOvFhM1etZQFUKEwz9VarppUbp5/Ie1+AiAM7aZcT1a2JR0oF/XwjNb13YEHwiagnDapLgYbklRvtA==',
+      },
+    ],
+  })
+})
+
 t.test('verifySignatures invalid signature', async t => {
   tnock(t, 'https://registry.npmjs.org')
     .get('/abbrev')
@@ -837,6 +876,37 @@ t.test('verifyAttestations no valid key', async t => {
   )
 })
 
+t.test('verifyAttestations rotated key', async t => {
+  const fixture = fs.readFileSync(
+    path.join(__dirname, 'fixtures', 'sigstore/valid-attestations.json'),
+    'utf8'
+  )
+
+  tnock(t, 'https://registry.npmjs.org')
+    .get('/-/npm/v1/attestations/[email protected]')
+    .reply(200, JSON.parse(fixture))
+
+  const f = new MockedRegistryFetcher('[email protected]', {
+    registry: 'https://registry.npmjs.org',
+    cache,
+    verifyAttestations: true,
+    [`//registry.npmjs.org/:_keys`]: [{
+      expires: '2023-04-01T00:00:00.000Z', // Rotated AFTER integratedTime 2023-01-11T17:31:54.000Z
+      keyid: 'SHA256:jl3bwswu80PjjokCgh0o2w5c2U4LhQAE57gj9cz1kzA',
+      keytype: 'ecdsa-sha2-nistp256',
+      scheme: 'ecdsa-sha2-nistp256',
+      // eslint-disable-next-line max-len
+      key: 'MFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAE1Olb3zMAFFxXKHiIkQO5cJ3Yhl5i6UPp+IhuteBJbuHcA5UogKo0EWtlWwW6KSaKoTNEYL7JlCQiVnkhBktUgg==',
+      // eslint-disable-next-line max-len
+      pemkey: '-----BEGIN PUBLIC KEY-----\nMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAE1Olb3zMAFFxXKHiIkQO5cJ3Yhl5i6UPp+IhuteBJbuHcA5UogKo0EWtlWwW6KSaKoTNEYL7JlCQiVnkhBktUgg==\n-----END PUBLIC KEY-----',
+    }],
+  })
+
+  const mani = await f.manifest()
+  t.ok(mani._attestations)
+  t.ok(mani._integrity)
+})
+
 t.test('verifyAttestations no registry keys at all', async t => {
   tnock(t, 'https://registry.npmjs.org')
     .get('/sigstore')