Skip to content

Commit

Permalink
fix: bug to support rotated keys in signature/attestation audit (#338)
Browse files Browse the repository at this point in the history 
*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 <philip@mailharrison.com>
  • Loading branch information
@feelepxyz
feelepxyz committed Dec 1, 2023
1 parent 9e9e187 commit 0c96b9e
Show file tree
Hide file tree
Showing 2 changed files with 100 additions and 6 deletions.
32 changes: 28 additions & 4 deletions lib/registry.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand Down Expand Up @@ -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,
Expand All @@ -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) {
Expand Down Expand Up @@ -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} ` +
Expand Down Expand Up @@ -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} ` +
Expand Down
74 changes: 72 additions & 2 deletions test/registry.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand All @@ -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')
Expand Down Expand Up @@ -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/sigstore@0.4.0')
.reply(200, JSON.parse(fixture))

const f = new MockedRegistryFetcher('sigstore@0.4.0', {
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')
Expand Down

0 comments on commit 0c96b9e

Please sign in to comment.