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: retrieve registry keys via TUF #6418

Merged
merged 1 commit into from
May 18, 2023
Merged

Conversation

bdehamer
Copy link
Contributor

@bdehamer bdehamer commented May 2, 2023

Updates the audit signatures command to retrieve the registry keys from the Sigstore TUF repository. The keys are published as a delegated target under the registry.npmjs.org namespace.

The published keys.json uses a slightly different format than the /-/npm/v1/keys endpoint:

{
  "keys": [
    {
      "keyId": "SHA256:jl3bwswu80PjjokCgh0o2w5c2U4LhQAE57gj9cz1kzA",
      "keyUsage": "npm:signatures",
      "publicKey": {
        "rawBytes": "MFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAE1Olb3zMAFFxXKHiIkQO5cJ3Yhl5i6UPp+IhuteBJbuHcA5UogKo0EWtlWwW6KSaKoTNEYL7JlCQiVnkhBktUgg==",
        "keyDetails": "PKIX_ECDSA_P256_SHA_256",
        "validFor": {
          "start": "1999-01-01T00:00:00.000Z"
        }
      }
    },
    {
      "keyId": "SHA256:jl3bwswu80PjjokCgh0o2w5c2U4LhQAE57gj9cz1kzA",
      "keyUsage": "npm:attestations",
      "publicKey": {
        "rawBytes": "MFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAE1Olb3zMAFFxXKHiIkQO5cJ3Yhl5i6UPp+IhuteBJbuHcA5UogKo0EWtlWwW6KSaKoTNEYL7JlCQiVnkhBktUgg==",
        "keyDetails": "PKIX_ECDSA_P256_SHA_256",
        "validFor": {
          "start": "2022-12-01T00:00:00.000Z"
        }
      }
    }
  ]
}

For now, we're transforming this to match the existing key file format used on the registry so that we don't have to make any changes in pacote. In the future we'll update pacote to take advantage of some of the additional metadata available in the newer format.

This new scheme will support third-party registries which may also want to publish their keys to the Sigstore TUF repository, but will fallback to the /-/npm/v1/keys endpoint for backward compatibility.

@bdehamer bdehamer requested a review from a team as a code owner May 2, 2023 23:40
@bdehamer bdehamer requested review from fritzy and feelepxyz and removed request for a team May 2, 2023 23:40
@ljharb
Copy link
Contributor

ljharb commented May 3, 2023

Does this mean that npm audit will now have a runtime dependency on two servers instead of one??

(to be clear, adding an additional network request location to npm feels like a breaking change to me; lots of places that run npm will lock down network traffic to known servers)

@npm-cli-bot
Copy link
Collaborator

npm-cli-bot commented May 3, 2023

no statistically significant performance changes detected

timing results
app-large clean lock-only cache-only cache-only
peer-deps
modules-only no-lock no-cache no-modules no-clean no-clean
audit
show-version run-script
npm@9 43.315 ±1.43 24.451 ±0.18 23.528 ±0.09 26.709 ±0.96 3.468 ±0.03 3.733 ±0.15 3.057 ±0.15 16.761 ±0.46 2.991 ±0.04 4.279 ±0.08 0.448 ±0.00 0.470 ±0.01
#6418 45.772 ±0.36 24.787 ±0.05 24.530 ±0.25 28.190 ±0.75 3.592 ±0.06 3.653 ±0.08 3.046 ±0.05 17.150 ±0.19 3.150 ±0.12 4.489 ±0.32 0.460 ±0.01 0.491 ±0.02
app-medium clean lock-only cache-only cache-only
peer-deps
modules-only no-lock no-cache no-modules no-clean no-clean
audit
show-version run-script
npm@9 30.496 ±0.39 18.543 ±0.22 17.667 ±0.36 18.779 ±0.11 3.246 ±0.03 3.252 ±0.01 3.117 ±0.01 11.924 ±0.12 2.822 ±0.01 3.909 ±0.03 0.450 ±0.02 0.490 ±0.02
#6418 30.496 ±0.93 18.657 ±0.32 17.854 ±0.30 19.178 ±0.21 3.286 ±0.04 3.235 ±0.02 3.190 ±0.06 12.265 ±0.19 2.827 ±0.00 4.189 ±0.42 0.451 ±0.00 0.489 ±0.01

@@ -1486,6 +1539,50 @@ t.test('audit signatures', async t => {
t.matchSnapshot(joinedOutput())
})

t.test('third-party registry with sub-path', async t => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Very nice 👍 would we need to handle a trailing slash on the path? e.g. https://verdaccio-clone.org/npm/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. Will add a test case to make sure that's handled properly.

lib/commands/audit.js Outdated Show resolved Hide resolved
lib/commands/audit.js Outdated Show resolved Hide resolved
@feelepxyz
Copy link
Contributor

adding an additional network request location to npm feels like a breaking change to me; lots of places that run npm will lock down network traffic to known servers)

@ljharb this is good context, makes sense that you might lock down network traffic around npm.

This change should not add any more network requests to audit signatures as we've already implemented attestation verification support in pacote that perform the same network requests there to update the Sigstore trust root (this now includes npm's public keys) 😬

Would have been good to surface this at the time, but I'm also not aware of any complaints around the pacote change so this might mean the audit signatures command still hasn't gotten a lot of usage.

Good to surface all these constraints before we consider turning some of these checks on by default when running npm audit.

@ljharb
Copy link
Contributor

ljharb commented May 3, 2023

People largely don’t run anything that’s not run by default; I’d urge you to consider setting up an npm registry endpoint for this rather than forcing clients to connect independently to sigstore.

@bdehamer bdehamer force-pushed the bdehamer/npm-keys-via-tuf branch from 5f209b9 to cea32f7 Compare May 3, 2023 17:30
@wraithgar wraithgar changed the title chore: retrieve registry keys via TUF fix: retrieve registry keys via TUF May 3, 2023
@bdehamer bdehamer force-pushed the bdehamer/npm-keys-via-tuf branch from cea32f7 to 0910769 Compare May 3, 2023 18:25
@bdehamer
Copy link
Contributor Author

bdehamer commented May 3, 2023

I wonder, would we want to make the same change to pacote so it re-uses the same cache?

@feelepxyz I've got a pacote PR open with the changes necessary to use a consistent TUF cache between the two uses cases.

@bdehamer bdehamer force-pushed the bdehamer/npm-keys-via-tuf branch from 0910769 to 1cb620a Compare May 4, 2023 01:00
Copy link
Contributor

@feelepxyz feelepxyz left a comment

Choose a reason for hiding this comment

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

Awesome work on this!

@ljharb
Copy link
Contributor

ljharb commented May 4, 2023

To reiterate, any CI that’s using something like https://github.com/step-security/harden-runner will immediately fail when npm audit runs (including 400+ of my packages), so this will be a very disruptive breaking change if it’s by default.

@wraithgar
Copy link
Member

Again, this doesn't run on audit. This is part of npm audit signatures.

@ljharb
Copy link
Contributor

ljharb commented May 4, 2023

ahh, thanks, i misunderstood that - I thought this PR was affecting npm audit itself. Appreciate the clarification.

Signed-off-by: Brian DeHamer <bdehamer@github.com>
@wraithgar
Copy link
Member

The pacote PR that added the tuf cache param parsing landed and is published.

@bdehamer
Copy link
Contributor Author

@wraithgar this is ready to go for the next release

@wraithgar wraithgar merged commit 37cc797 into latest May 18, 2023
@wraithgar wraithgar deleted the bdehamer/npm-keys-via-tuf branch May 18, 2023 19:20
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.

5 participants