-
Notifications
You must be signed in to change notification settings - Fork 45
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
fix: bug to support rotated keys in signature/attestation audit #338
Conversation
65639f0
to
f3723d4
Compare
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() |
@wraithgar thanks for review, applied suggestions. I think there might be some unrelated test failures due to changed digests? |
There was a problem hiding this 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 👍
The failing test is most likely a change node made to a compression setting. It affects the cli and probably other tests too. |
*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
Context
npm audit signatures
performs 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
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 thedist
object but this is a pretty big effort.