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

feat: add npm audit signatures #4827

Merged
merged 2 commits into from
Jul 11, 2022
Merged

Conversation

feelepxyz
Copy link
Contributor

@feelepxyz feelepxyz commented Apr 29, 2022

Implements RFC: Improve signature verification

Adds a new sub-command to audit: npm audit signatures (following npm audit licenses)

This command will verify registry signatures stored in the packument against a public key on the registry.

It supports:

  • Any registry that implements host/-/npm/v1/keys endpoint and provides signatures in the packument dist object
  • Validates public keys are not expired
  • Errors when encountering packages with missing signatures when the registry returns keys at host/-/npm/v1/keys
  • Errors when encountering invalid signatures
  • json/human format output

lib/commands/audit.js Outdated Show resolved Hide resolved
lib/commands/audit.js Show resolved Hide resolved
lib/utils/verify-signatures.js Outdated Show resolved Hide resolved
lib/utils/verify-signatures.js Outdated Show resolved Hide resolved
lib/utils/verify-signatures.js Outdated Show resolved Hide resolved
@feelepxyz
Copy link
Contributor Author

@wraithgar @ljharb thanks for the quick feedback, much appreciated! 🙇 I'm away next week so won't be getting to this until I'm back.

@wraithgar
Copy link
Member

@feelepxyz that's fine, it'll give us enough time to react to this and hopefully move some of this logic into pacote proper.

@feelepxyz
Copy link
Contributor Author

feelepxyz commented Apr 30, 2022 via email

@feelepxyz feelepxyz force-pushed the validate-registry-signatures branch 2 times, most recently from a7c78e7 to 0def984 Compare May 12, 2022 15:10
lib/commands/audit.js Outdated Show resolved Hide resolved
lib/commands/audit.js Outdated Show resolved Hide resolved
lib/commands/audit.js Outdated Show resolved Hide resolved
lib/commands/audit.js Outdated Show resolved Hide resolved
lib/commands/audit.js Outdated Show resolved Hide resolved
lib/commands/audit.js Outdated Show resolved Hide resolved
lib/commands/audit.js Outdated Show resolved Hide resolved
lib/commands/audit.js Outdated Show resolved Hide resolved
@feelepxyz feelepxyz force-pushed the validate-registry-signatures branch from 09a73ed to e67e5db Compare May 23, 2022 10:07
feelepxyz added a commit to npm/pacote that referenced this pull request May 23, 2022
Raise an error when the package has a signature, `verifySignatures` is true and there are no registry keys configured.

Updated the error message and fixed the undefined name@version in the error message to match the test cases here:
npm/cli#4827

Possible anti-pattern: I've attached a bunch of error attributes if the signature is invalid so we can present this in the json output to ease debugging.
@feelepxyz feelepxyz marked this pull request as ready for review May 23, 2022 14:25
@feelepxyz feelepxyz requested a review from a team as a code owner May 23, 2022 14:25
feelepxyz added a commit to npm/pacote that referenced this pull request May 23, 2022
Raise an error when the package has a signature, `verifySignatures` is true and there are no registry keys configured.

Updated the error message and fixed the undefined name@version in the error message to match the test cases here:
npm/cli#4827

Possible anti-pattern: I've attached a bunch of error attributes if the signature is invalid so we can present this in the json output to ease debugging.
@feelepxyz feelepxyz requested a review from wraithgar May 23, 2022 18:18
@feelepxyz
Copy link
Contributor Author

@wraithgar I think this is ready for more thorough review now, I've updated to use your pacote changes, but pending these changes in my pacote PR 🙇

Copy link
Contributor

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

What does this command do with -g? What should it do? Should it error out, or is there something useful it can do?

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

What does this command do with -g? What should it do? Should it error out, or is there something useful it can do?

Good shout, currently it just ignores it and runs on the current install but maybe better to error unless it should work? I have never really used global so not sure what makes sense.

@ljharb
Copy link
Contributor

ljharb commented May 23, 2022

Are the signatures being audited in the installed package.json, or in the lockfile? Global installs don't have a lockfile.

@feelepxyz
Copy link
Contributor Author

Are the signatures being audited in the installed package.json, or in the lockfile? Global installs don't have a lockfile.

The ones installed from the lockfile. I've added an error case if running with the global flag, looks like this is done for a few commands already. Once we roll this into npm install we can also do this for global dep installs.

@feelepxyz feelepxyz requested review from nlf and darcyclarke May 24, 2022 09:42
@feelepxyz feelepxyz force-pushed the validate-registry-signatures branch from 76510af to 0dc9964 Compare May 26, 2022 09:51
lib/commands/audit.js Outdated Show resolved Hide resolved
test/lib/commands/audit.js Outdated Show resolved Hide resolved
lib/commands/audit.js Outdated Show resolved Hide resolved
@wraithgar
Copy link
Member

@feelepxyz let me know when you're done with this iteration of the thread and I'll fix the package/lock conflicts.

@feelepxyz
Copy link
Contributor Author

@feelepxyz let me know when you're done with this iteration of the thread and I'll fix the package/lock conflicts.

Nice one, I'm all done 👍

@wraithgar wraithgar force-pushed the validate-registry-signatures branch 2 times, most recently from dcabb3c to f488165 Compare June 23, 2022 15:47
@wraithgar
Copy link
Member

@feelepxyz all done. How do you feel about squashing the commits in this PR while we wait for it to land? We're likely gonna have at least one more conflict w/ the package lock before we can land this and it'd be way easier to suss out if we hit "reset" and squash the commits so far.

@feelepxyz
Copy link
Contributor Author

feelepxyz commented Jun 23, 2022 via email

@wraithgar
Copy link
Member

Yes sounds good, happy to squash the commits 👌

Great! The branch is all yours.

@feelepxyz feelepxyz force-pushed the validate-registry-signatures branch 3 times, most recently from 32bdfca to 48e1b44 Compare June 28, 2022 14:39
@wraithgar wraithgar force-pushed the validate-registry-signatures branch from 48e1b44 to b51a9f7 Compare June 28, 2022 15:50
Implemenents [RFC: Improve signature verification](npm/rfcs#550)

Adds a new sub-command to `audit`: `npm audit signatures` (following [`npm audit licenses`](#3452))

This command will verify registry signatures stored in the packument against a public key on the registry.

Supporting:
- Any registry that implements `host/-/npm/v1/keys` endpoint and provides `signatures` in the packument `dist` object
- Validates public keys are not expired
- Errors when encountering packages with missing signatures when the registry returns keys at `host/-/npm/v1/keys`
- Errors when encountering invalid signatures
- Output: json/human formats

Co-authored-by: Michael Garvin <wraithgar@github.com>
@feelepxyz feelepxyz force-pushed the validate-registry-signatures branch from b51a9f7 to 3ae53e4 Compare June 30, 2022 07:50
@wraithgar wraithgar self-requested a review June 30, 2022 14:23
Copy link
Member

@wraithgar wraithgar left a comment

Choose a reason for hiding this comment

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

Let's do this.

@wraithgar
Copy link
Member

docs are preliminary, @feelepxyz can expand on them in another PR.

@wraithgar wraithgar merged commit f032e1c into latest Jul 11, 2022
@wraithgar wraithgar deleted the validate-registry-signatures branch July 11, 2022 17:49
@wraithgar wraithgar mentioned this pull request Jul 13, 2022
feelepxyz added a commit that referenced this pull request Jul 14, 2022
Update command documentation for `npm audit signatures` added in this PR:
#4827
fritzy pushed a commit that referenced this pull request Jul 20, 2022
fix: Update docs for audit signatures cmd

Update command documentation for `npm audit signatures` added in this PR:
#4827
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.

6 participants