feat: add verifySignatures to registry.manifest (#170)

wraithgar · wraithgar · commit 4401c5863302 · 2022-05-17T11:42:31.000-07:00
diff --git a/README.md b/README.md
@@ -168,6 +168,10 @@ resolved, and other properties, as they are determined.
   is unlikely to change in the span of a single command.
 * `silent` A boolean that determines whether the banner is displayed
   when calling `@npmcli/run-script`.
+* `verifySignatures` A boolean that will make pacote verify the
+    integrity signature of a manifest, if present.  There must be a
+    configured `_keys` entry in the config that is scoped to the
+    registry the manifest is being fetched from.
 
 
 ### Advanced API
diff --git a/lib/registry.js b/lib/registry.js
@@ -7,15 +7,14 @@ const npa = require('npm-package-arg')
 const rpj = require('read-package-json-fast')
 const pickManifest = require('npm-pick-manifest')
 const ssri = require('ssri')
+const crypto = require('crypto')
 
 // Corgis are cute. 🐕🐶
 const corgiDoc = 'application/vnd.npm.install-v1+json; q=1.0, application/json; q=0.8, */*'
 const fullDoc = 'application/json'
 
 const fetch = require('npm-registry-fetch')
 
-// TODO: memoize reg requests, so we don't even have to check cache
-
 const _headers = Symbol('_headers')
 class RegistryFetcher extends Fetcher {
   constructor (spec, opts) {
@@ -39,28 +38,30 @@ class RegistryFetcher extends Fetcher {
     this.packumentUrl = removeTrailingSlashes(this.registry) + '/' +
       this.spec.escapedName
 
+    const parsed = new URL(this.registry)
+    const regKey = `//${parsed.host}${parsed.pathname}`
+    // unlike the nerf-darted auth keys, this one does *not* allow a mismatch
+    // of trailing slashes.  It must match exactly.
+    if (this.opts[`${regKey}:_keys`]) {
+      this.registryKeys = this.opts[`${regKey}:_keys`]
+    }
+
     // XXX pacote <=9 has some logic to ignore opts.resolved if
     // the resolved URL doesn't go to the same registry.
     // Consider reproducing that here, to throw away this.resolved
     // in that case.
   }
 
-  resolve () {
-    if (this.resolved) {
-      return Promise.resolve(this.resolved)
-    }
-
-    // fetching the manifest sets resolved and (usually) integrity
-    return this.manifest().then(() => {
-      if (this.resolved) {
-        return this.resolved
-      }
-
+  async resolve () {
+    // fetching the manifest sets resolved and (if present) integrity
+    await this.manifest()
+    if (!this.resolved) {
       throw Object.assign(
         new Error('Invalid package manifest: no `dist.tarball` field'),
         { package: this.spec.toString() }
       )
-    })
+    }
+    return this.resolved
   }
 
   [_headers] () {
@@ -87,91 +88,127 @@ class RegistryFetcher extends Fetcher {
     // npm-registry-fetch the packument
     // set the appropriate header for corgis if fullMetadata isn't set
     // return the res.json() promise
-    const p = fetch(this.packumentUrl, {
-      ...this.opts,
-      headers: this[_headers](),
-      spec: this.spec,
-      // never check integrity for packuments themselves
-      integrity: null,
-    }).then(res => res.json().then(packument => {
+    try {
+      const res = await fetch(this.packumentUrl, {
+        ...this.opts,
+        headers: this[_headers](),
+        spec: this.spec,
+        // never check integrity for packuments themselves
+        integrity: null,
+      })
+      const packument = await res.json()
       packument._cached = res.headers.has('x-local-cache')
       packument._contentLength = +res.headers.get('content-length')
       if (this.packumentCache) {
         this.packumentCache.set(this.packumentUrl, packument)
       }
       return packument
-    })).catch(er => {
+    } catch (err) {
       if (this.packumentCache) {
         this.packumentCache.delete(this.packumentUrl)
       }
-      if (er.code === 'E404' && !this.fullMetadata) {
-        // possible that corgis are not supported by this registry
-        this.fullMetadata = true
-        return this.packument()
+      if (err.code !== 'E404' || this.fullMetadata) {
+        throw err
       }
-      throw er
-    })
-    if (this.packumentCache) {
-      this.packumentCache.set(this.packumentUrl, p)
+      // possible that corgis are not supported by this registry
+      this.fullMetadata = true
+      return this.packument()
     }
-    return p
   }
 
-  manifest () {
+  async manifest () {
     if (this.package) {
-      return Promise.resolve(this.package)
+      return this.package
     }
 
-    return this.packument()
-      .then(packument => pickManifest(packument, this.spec.fetchSpec, {
-        ...this.opts,
-        defaultTag: this.defaultTag,
-        before: this.before,
-      }) /* XXX add ETARGET and E403 revalidation of cached packuments here */)
-      .then(mani => {
-        // add _resolved and _integrity from dist object
-        const { dist } = mani
-        if (dist) {
-          this.resolved = mani._resolved = dist.tarball
-          mani._from = this.from
-          const distIntegrity = dist.integrity ? ssri.parse(dist.integrity)
-            : dist.shasum ? ssri.fromHex(dist.shasum, 'sha1', { ...this.opts })
-            : null
-          if (distIntegrity) {
-            if (!this.integrity) {
-              this.integrity = distIntegrity
-            } else if (!this.integrity.match(distIntegrity)) {
-              // only bork if they have algos in common.
-              // otherwise we end up breaking if we have saved a sha512
-              // previously for the tarball, but the manifest only
-              // provides a sha1, which is possible for older publishes.
-              // Otherwise, this is almost certainly a case of holding it
-              // wrong, and will result in weird or insecure behavior
-              // later on when building package tree.
-              for (const algo of Object.keys(this.integrity)) {
-                if (distIntegrity[algo]) {
-                  throw Object.assign(new Error(
-                    `Integrity checksum failed when using ${algo}: ` +
-                    `wanted ${this.integrity} but got ${distIntegrity}.`
-                  ), { code: 'EINTEGRITY' })
-                }
-              }
-              // made it this far, the integrity is worthwhile.  accept it.
-              // the setter here will take care of merging it into what we
-              // already had.
-              this.integrity = distIntegrity
+    const packument = await this.packument()
+    const mani = await pickManifest(packument, this.spec.fetchSpec, {
+      ...this.opts,
+      defaultTag: this.defaultTag,
+      before: this.before,
+    })
+    /* XXX add ETARGET and E403 revalidation of cached packuments here */
+
+    // add _resolved and _integrity from dist object
+    const { dist } = mani
+    if (dist) {
+      this.resolved = mani._resolved = dist.tarball
+      mani._from = this.from
+      const distIntegrity = dist.integrity ? ssri.parse(dist.integrity)
+        : dist.shasum ? ssri.fromHex(dist.shasum, 'sha1', { ...this.opts })
+        : null
+      if (distIntegrity) {
+        if (this.integrity && !this.integrity.match(distIntegrity)) {
+          // only bork if they have algos in common.
+          // otherwise we end up breaking if we have saved a sha512
+          // previously for the tarball, but the manifest only
+          // provides a sha1, which is possible for older publishes.
+          // Otherwise, this is almost certainly a case of holding it
+          // wrong, and will result in weird or insecure behavior
+          // later on when building package tree.
+          for (const algo of Object.keys(this.integrity)) {
+            if (distIntegrity[algo]) {
+              throw Object.assign(new Error(
+                `Integrity checksum failed when using ${algo}: ` +
+                `wanted ${this.integrity} but got ${distIntegrity}.`
+              ), { code: 'EINTEGRITY' })
             }
           }
         }
-        if (this.integrity) {
-          mani._integrity = String(this.integrity)
-          if (dist.signatures) {
+        // made it this far, the integrity is worthwhile.  accept it.
+        // the setter here will take care of merging it into what we already
+        // had.
+        this.integrity = distIntegrity
+      }
+    }
+    if (this.integrity) {
+      mani._integrity = String(this.integrity)
+      if (dist.signatures) {
+        if (this.opts.verifySignatures) {
+          if (this.registryKeys) {
+            // validate and throw on error, then set _signatures
+            const message = `${mani._id}:${mani._integrity}`
+            for (const signature of dist.signatures) {
+              const publicKey = this.registryKeys.filter(key => (key.keyid === signature.keyid))[0]
+              if (!publicKey) {
+                throw Object.assign(new Error(
+                  `${mani._id} has a signature with keyid: ${signature.keyid} ` +
+                  'but no corresponding public key can be found.'
+                ), { code: 'EMISSINGSIGNATUREKEY' })
+              }
+              const validPublicKey =
+                !publicKey.expires || (Date.parse(publicKey.expires) > Date.now())
+              if (!validPublicKey) {
+                throw Object.assign(new Error(
+                  `${mani._id} has a signature with keyid: ${signature.keyid} ` +
+                  `but the corresponding public key has expired ${publicKey.expires}`
+                ), { code: 'EEXPIREDSIGNATUREKEY' })
+              }
+              const verifier = crypto.createVerify('SHA256')
+              verifier.write(message)
+              verifier.end()
+              const valid = verifier.verify(
+                publicKey.pemkey,
+                signature.sig,
+                'base64'
+              )
+              if (!valid) {
+                throw Object.assign(new Error(
+                  'Integrity checksum signature failed: ' +
+                  `key ${publicKey.keyid} signature ${signature.sig}`
+                ), { code: 'EINTEGRITYSIGNATURE' })
+              }
+            }
             mani._signatures = dist.signatures
           }
+          // if no keys, don't set _signatures
+        } else {
+          mani._signatures = dist.signatures
         }
-        this.package = rpj.normalize(mani)
-        return this.package
-      })
+      }
+    }
+    this.package = rpj.normalize(mani)
+    return this.package
   }
 
   [_tarballFromResolved] () {
diff --git a/test/registry.js b/test/registry.js
@@ -150,6 +150,132 @@ t.test('provide matching integrity, totes ok, includes signature', async t => {
   })
 })
 
+t.test('verifySignatures valid signature', async t => {
+  const f = new RegistryFetcher('@isaacs/namespace-test', {
+    registry,
+    cache,
+    verifySignatures: true,
+    [`//localhost:${port}/:_keys`]: [{
+      expires: null,
+      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-----',
+    }],
+  })
+  return f.manifest().then(mani => {
+    t.ok(mani._signatures)
+    t.ok(mani._integrity)
+  })
+})
+
+t.test('verifySignatures expired signature', async t => {
+  const f = new RegistryFetcher('@isaacs/namespace-test', {
+    registry,
+    cache,
+    verifySignatures: true,
+    [`//localhost:${port}/:_keys`]: [{
+      expires: '2010-01-01',
+      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-----',
+    }],
+  })
+  return t.rejects(
+    f.manifest(),
+    {
+      code: 'EEXPIREDSIGNATUREKEY',
+    }
+  )
+})
+
+t.test('verifySignatures invalid signature', async t => {
+  tnock(t, 'https://registry.npmjs.org')
+    .get('/abbrev')
+    .reply(200, {
+      _id: 'abbrev',
+      _rev: 'deadbeef',
+      name: 'abbrev',
+      'dist-tags': { latest: '1.1.1' },
+      versions: {
+        '1.1.1': {
+          name: 'abbrev',
+          version: '1.1.1',
+          dist: {
+            // eslint-disable-next-line max-len
+            integrity: 'sha512-nne9/IiQ/hzIhY6pdDnbBtz7DjPTKrY00P/zvPSm5pOFkl6xuGrGnXn/VtTNNfNtAfZ9/1RtehkszU9qcTii0Q==',
+            shasum: 'f8f2c887ad10bf67f634f005b6987fed3179aac8',
+            tarball: 'https://registry.npmjs.org/abbrev/-/abbrev-1.1.1.tgz',
+            signatures: [
+              {
+                keyid: 'SHA256:jl3bwswu80PjjokCgh0o2w5c2U4LhQAE57gj9cz1kzA',
+                sig: 'nope',
+              },
+            ],
+          },
+        },
+      },
+    })
+
+  const f = new RegistryFetcher('abbrev', {
+    registry: 'https://registry.npmjs.org',
+    cache,
+    verifySignatures: true,
+    [`//registry.npmjs.org/:_keys`]: [{
+      expires: null,
+      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-----',
+    }],
+  })
+  return t.rejects(
+    f.manifest(),
+    {
+      code: 'EINTEGRITYSIGNATURE',
+    }
+  )
+})
+
+t.test('verifySignatures no valid key', async t => {
+  const f = new RegistryFetcher('@isaacs/namespace-test', {
+    registry,
+    cache,
+    verifySignatures: true,
+    [`//localhost:${port}/:_keys`]: [{
+      keyid: 'someotherid',
+    }],
+  })
+  return t.rejects(
+    f.manifest(),
+    {
+      code: 'EMISSINGSIGNATUREKEY',
+    }
+  )
+})
+
+t.test('verifySignatures no registry keys at all', async t => {
+  const f = new RegistryFetcher('@isaacs/namespace-test', {
+    registry,
+    cache,
+    verifySignatures: true,
+    [`//localhost:${port}/:_keys`]: null,
+  })
+  return f.manifest().then(mani => {
+    t.notOk(mani._signatures)
+  })
+})
+
 t.test('404 fails with E404', t => {
   const f = new RegistryFetcher('thing-is-not-here', { registry, cache })
   return t.rejects(f.resolve(), { code: 'E404' }).then(() =>
