Skip to content
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

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

Merged

Conversation

feelepxyz
Copy link
Contributor

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.

@feelepxyz feelepxyz requested a review from a team as a code owner November 30, 2023 12:28
@feelepxyz feelepxyz changed the title Fix bug to support rotated keys in signature/attestation audit fix: bug to support rotated keys in signature/attestation audit Nov 30, 2023
@feelepxyz feelepxyz force-pushed the feelepxyz/support-rotated-public-signing-keys branch from 65639f0 to f3723d4 Compare November 30, 2023 12:34
lib/registry.js Outdated Show resolved Hide resolved
@wraithgar
Copy link
Member

wraithgar commented Nov 30, 2023

I can't recommend changes outside of your diff so I'll just put it here:

  async manifest () {
    if (this.package) {
      return this.package
    }

    if (this.opts.verifySignatures) {
       this.fullMetadata = true
    }


    const packument = await this.packument()

@feelepxyz
Copy link
Contributor Author

@wraithgar thanks for review, applied suggestions. I think there might be some unrelated test failures due to changed digests?

@wraithgar wraithgar self-assigned this Nov 30, 2023
Copy link
Contributor

@bdehamer bdehamer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One question, but looks good otherwise 👍

lib/registry.js Outdated Show resolved Hide resolved
@wraithgar
Copy link
Member

The failing test is most likely a change node made to a compression setting. It affects the cli and probably other tests too.

lib/registry.js Outdated Show resolved Hide resolved
*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 install loop.

Signed-off-by: Philip Harrison <philip@mailharrison.com>
@feelepxyz feelepxyz force-pushed the feelepxyz/support-rotated-public-signing-keys branch from 4e044e3 to 58defea Compare December 1, 2023 09:58
@wraithgar wraithgar merged commit 0c96b9e into npm:main Dec 1, 2023
14 of 15 checks passed
@github-actions github-actions bot mentioned this pull request Dec 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants