fix: bug to support rotated keys in signature/attestation audit#338
Merged
wraithgar merged 1 commit intoDec 1, 2023
Merged
Conversation
65639f0 to
f3723d4
Compare
wraithgar
reviewed
Nov 30, 2023
Contributor
|
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() |
Contributor
Author
|
@wraithgar thanks for review, applied suggestions. I think there might be some unrelated test failures due to changed digests? |
bdehamer
approved these changes
Nov 30, 2023
Contributor
bdehamer
left a comment
There was a problem hiding this comment.
One question, but looks good otherwise 👍
Contributor
|
The failing test is most likely a change node made to a compression setting. It affects the cli and probably other tests too. |
wraithgar
reviewed
Nov 30, 2023
*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>
4e044e3 to
58defea
Compare
bdehamer
approved these changes
Dec 1, 2023
wraithgar
approved these changes
Dec 1, 2023
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
npm audit signaturesperforms the registry signature and sigstore attestation bundle verification inpacote.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
integratedTimefor 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
timeset 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
signedAttimestamp in thedistobject but this is a pretty big effort.