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

npm audit and audit-resolve.json #18

Open
wants to merge 8 commits into
base: latest
Choose a base branch
from

Conversation

@naugtur
Copy link

@naugtur naugtur commented Aug 5, 2018

Was: npm audit resolve
Updated to only cover the support for audit-resolve.json file in npm.

Currently implemented in userland here: https://www.npmjs.com/package/npm-audit-resolver

The check-audit command from the above package is providing the exact functionality proposed here.

Other places where this was discussed:

npm/cli#10
https://npm.community/t/interactive-tool-to-manage-audit-findings-npm-audit-resolve/197

accepted/0003-interactive-audit-resolver.md Outdated Show resolved Hide resolved
### resolutions
- **fix** runs the fix proposed in audit action and marks the issue as fixed in `audit-resolv.json`. On each subsequent run of `npm audit resolve` if the issue comes up again, the user is also warned that the problem was supposed to be fixed already.
- **ignore** marks the issue as ignored and `npm audit` will no longer fail because of it (but should display a single line warning about the issue being ignored). If a new vulnerability is found in the same dependency, it does not get ignored. If another dependency is installed with the same vulnerability advisory id it is not ignored. If the same package is installed as a dependency to another dependency (different path) it is not ignored.
- **postpone** marks the issue as postponed to a timestamp 24h in the future. Instead of ignoring an issue permanently just to make a build pass, one can postpone it when in rush, but make it show up as a CI faiure on the next working day.

This comment has been minimized.

@ljharb

ljharb Aug 5, 2018
Contributor

The next working day is often more than 24 hours later; is this just meant as an example?

This comment has been minimized.

@naugtur

naugtur Aug 5, 2018
Author

Yes. I didn't intend it to count in working days.

If noone's at work and the build turns red, does it make a sound?

Why is postpone useful at all? It's designed to build a secure development culture where one didn't yet form. Without it, a developer under time pressure would mark an issue as ignored with intention to un-mark it later.
While shipping with a known vulnerability is a bad practice, NPM's mission with the community should be to empower people to build more secure products and trust their skill and understanding of their project's particular needs. We should also aspire to help teams introduce more secure workflows effortlessly, so letting a build pass without risking compromising security long-term is a win.

Remove is only useful as a convenience. Imagine a developer introducing `npm audit` and having to go through tens of issues. If they notice one of the first issues is caused by a dependency they no longer use, instead of remembering to clean it up later, they can choose this option.

This comment has been minimized.

@ljharb

ljharb Aug 5, 2018
Contributor

Presumably this could also narrow the list of action items, if some of them are from transitive deps of the removed item?

This comment has been minimized.

@naugtur

naugtur Aug 5, 2018
Author

In my experience audit grouped these together nicely, so when I choose remove it's unlikely the dependency or any of its children would come up again.

But we could easily run audit again after the remove command and thanks to audit-resolv being written after each decision, it'd automatically continue from the same point on the list.

@CADBOT
Copy link

@CADBOT CADBOT commented Sep 28, 2018

What's the latest on this? Would be great to have something to replace .nsprc/exceptions from nsp.

@naugtur
Copy link
Author

@naugtur naugtur commented Sep 28, 2018

I don't really know what's next other than moving my repo to npm org on GitHub.
There's no disagreement on anything here.

I have more free time than usual for the next few days so I'd be happy to move this forward too.

But you can use it now. Install npm-audit-resolver. I'm using it at work with my team for a while now

### resolutions
- **fix** runs the fix proposed in audit action and marks the issue as fixed in `audit-resolv.json`. On each subsequent run of `npm audit resolve` if the issue comes up again, the user is also warned that the problem was supposed to be fixed already.
- **ignore** marks the issue as ignored and `npm audit` will no longer fail because of it (but should display a single line warning about the issue being ignored). If a new vulnerability is found in the same dependency, it does not get ignored. If another dependency is installed with the same vulnerability advisory id it is not ignored. If the same package is installed as a dependency to another dependency (different path) it is not ignored.
- **postpone** marks the issue as postponed to a timestamp 24h in the future. Instead of ignoring an issue permanently just to make a build pass, one can postpone it when in rush, but make it show up as a CI faiure on the next working day.

This comment has been minimized.

@evilpacket

evilpacket Nov 6, 2018

How do we handle the situation in which somebody keeps hitting snooze (postpone) on a vulnerability. Do we really have to rely on git commit history to see this? Should we track all history not just the current state / decisions? This adds complexity that we maybe don't need but also could be a useful audit trail?

This comment has been minimized.

@naugtur

naugtur Nov 7, 2018
Author

I thought about a few cases of audit trail with similar conclusions. IMHO the benefit of audit-resolv file's simplicity outweighs the inconvenience of using git history. Also, people are quite familiar with git so the less we do on our end, the less learning they have to do.

I'd like to avoid the file growing to overwhelming lengths, but other than that I'm fine with adding more capability to this file format if it's useful for the server side ignores etc.

This comment has been minimized.

@naugtur

naugtur Dec 4, 2018
Author

Leaving only the 24h option it should be annoying enough for most reasonable people to eventually fix. :)
Unless someone gets a job postponing vulnerabilities every morning before the team comes in.

With the audit-resolve format change I just pushed we can also add a count${dependency path}er. I'd consider that a nice touch, not the core functionality though.

This comment has been minimized.

@naugtur

naugtur Jul 7, 2019
Author

Update - instead of letting people postpone for more than 24h, due to a popular demand, I got an option in to make ignores expire. So if you decide to ignore something because there's no fix and it doesn't seem accessible from the code that depends on it, it's possible to make it expire.

Added expiresAt field to the decision object in resolve file

`npm audit resolve` runs audit and iterates over all actions from the output with a prompt for a decision on each.
If fix is available, it's offered as one of the options to proceed. Other options include: ignore, remind later and delete.

All decisions are stored in `audit-resolv.json` file as key-value, where key is `${dependency path}|${advisory id}` and value is a structure specific to the resolution.

This comment has been minimized.

@evilpacket

evilpacket Nov 6, 2018

Can you document the format of this file? Also adding in the option to have a reason for each decision.

This comment has been minimized.

@evilpacket

evilpacket Nov 6, 2018

Another though is that the audit-resolve.json file should not be published as part of a module publication. We will want to assert exceptions outside of the package.json and update those exceptions outside of the normal publication lifecycle.

This comment has been minimized.

@naugtur

naugtur Nov 7, 2018
Author

Will update the RFC to contain the file format spec

This comment has been minimized.

@naugtur

naugtur Dec 4, 2018
Author

only the file next to top level package.json should be taken into account at all (and that's how it's implemented in npm-audit-resolver)

This comment has been minimized.

@naugtur

naugtur Mar 27, 2019
Author

After some tweaking I updated the RFC to include a precise format definition. Same format definition is used in a branch where I'm working on refactoring.

npm-audit-resolver package after the refactoring should be trivial to integrate into npm audit for the purpose of respecting decisions made in audit-resolve.json

This comment has been minimized.

@evilpacket
Copy link

@evilpacket evilpacket commented Nov 6, 2018

In general I'm very excited about the potential of the npm audit resolve feature. I think some work needs to be done to really make it a good interactive experience presenting the information in the best way possible for users.

@zkat
Copy link
Contributor

@zkat zkat commented Feb 7, 2019

(Heads up, we are tracking this internally and it's something we intend to get to, but I can't give you an exact timeline right now. Thanks for your patience.)

@naugtur
Copy link
Author

@naugtur naugtur commented Feb 11, 2019

If you want to consider one bigger push to move this forward, my family is out for winter vacation 25-28Feb while I stay at work, so I'll have unusually many hours in the evenings in that period.

@ersel
Copy link

@ersel ersel commented Feb 19, 2019

This would be an awesome addition to npm CLI. We used to run npm audit on CI with a custom script outlined here.

Recently we've switched to @naugtur 's npm-resolve-audit module by installing it as a dev dependency and adding a script to run node node_modules/npm-audit-resolver/check.js

Thanks for building this tool @naugtur !

@naugtur
Copy link
Author

@naugtur naugtur commented Feb 19, 2019

@ersel if you're running it inside an npm script, you can use it as a command check-audit and it resolves from dependencies. Otherwise I'd go with npx -p npm-audit-resolver check-audit - feels a little cleaner and you don't need to install resolver as a dependency - npx will download and cache it. (npx is shipped with npm so no need to install it)

Check out my slides on using audit in CI (and other things) https://naugtur.pl/pres3/securedev/#/18

@naugtur
Copy link
Author

@naugtur naugtur commented Mar 27, 2019

I've updated the RFC and I'm working on a refactoring to separate the core features from npm-related and interactive ones.

Could we agree on making npm audit use audit-resolve.json being the first step?
That way people could produce the file manually or with the interactive resolver, but the means of ignoring a package would be there.

@naugtur
Copy link
Author

@naugtur naugtur commented Jul 7, 2019

Hi,

I'm finishing the refactoring I mentioned before.
My suggestion to proceed now is that npm (and potentially other package managers) can use the core of npm-audit-resolver to include facts from audit-reslve.json when checking the repository with npm audit command - therefore skipping issues that have been addressed before.
The interactive part can (initially or forever) remain in the separate package with potential competitors, while the spec for audit-resolve.json becomes something we can slowly standardize.

I will open another PR to npm some time soon introducing the line to include decisions from audit-resolve.json in the npm audit logic.

Edit:
I should probably let you know @zkat @evilpacket

@naugtur naugtur changed the title npm audit resolve npm audit and audit-resolve.json Jul 17, 2020
@Den-dp
Copy link

@Den-dp Den-dp commented Jul 23, 2020

https://www.npmjs.com/package/audit-ci is another pretty popular alternative solution for this problem. Dropping it here to give more info for inspiration.

@naugtur
Copy link
Author

@naugtur naugtur commented Jul 24, 2020

Thanks for bringing it up @Den-dp
The important difference here is audit-ci is only operating on risk levels or advisory allowlist.

audit-resolve.json file is encoding precisely which items were ignored so if you ignore an unreachable ReDos in a sub dependency somewhere in your web app, you still get a notification if ReDos is found in your server framework used.

audit-resolve-core package (work in progress) is meant for embedding and audit-ci could easily switch to using that at any point btw.

@nierob
Copy link

@nierob nierob commented Jul 24, 2020

Lately I migrated to npm-audit-resolver, it works Ok and fulfils my needs. Up to now, I have stumbled only on one issue; generated resolve file can be big (lodash case…). As the tool allows to mark every usage per package per CVE, it creates quite redundant list. One could softly compress it by expressing “ignore all usages” per CVE per package (maybe limited to specific versions). Anyway it is just optimisation that would make it a bit more git friendly.

The important difference here is audit-ci is only operating on risk levels or advisory allowlist.

As a user I can confirm that "risk level" is not giving enough flexibility. One really wants to specify at least advisory/package configuration.

My personal opinion is that npm audit should not take part in blocking CI, unless a newly introduced dependencies caused errors, for example in checking PR that changes dependencies. It should be executed regularly on a side, preferably trying to commit a patch like result of npm audit fix or if it is not possible create a bug report. The main issue here is that placing it in blocking CI causes the same negative effects as flaky tests; integrations are broken because of “unknown” / “unrelated” reasons, nobody owns the problem because it is injected into middle of an unrelated process.

@naugtur
Copy link
Author

@naugtur naugtur commented Jul 24, 2020

Thanks for mentioning compression. Although the goal is to be very explicit not to invite future occurrences, you got me thinking. The list can be compressed by putting a wildcard in the path. So each occurrence of a package with certain advisory anywhere in dependency tree of one specific direct dependency could be collapsed into one.

That would limit the number of entities while not breaking the security promises.

"decision": "RESOLUTION_TYPE",
"reason": "Reason provided by the person making the decision (optional)",
"madeAt": timestamp
"expiresAt": timestamp

This comment has been minimized.

@dominykas

dominykas Jul 30, 2020

Can this please also (only?) support a human readable ISO date string?

Use case: extending the expiry time without using any tooling (an extremely common operation, from my experience with .snyk policy files...)

This comment has been minimized.

@naugtur

naugtur Jul 30, 2020
Author

Personally I believe it should not be limited to ISO string because of the ending I always mess up.
2020-09-21 should also work if we want to cater to humans :)

Next iteration of the core package that reads and writes the resolve file will have that.

Thanks for the data point!

@nierob
Copy link

@nierob nierob commented Aug 5, 2020

Thanks for mentioning compression. Although the goal is to be very explicit not to invite future occurrences, you got me thinking. The list can be compressed by putting a wildcard in the path. So each occurrence of a package with certain advisory anywhere in dependency tree of one specific direct dependency could be collapsed into one.

That would limit the number of entities while not breaking the security promises.

It depends what kind of use case you want to solve. For me, partially ignoring issues doesn't make sense because it would still block CI. That means that path as such is useful when deciding if an issue can be ignored, later on it is not needed. That is because the final decision is binary; "do I accept this package or not". So instead of path I would use package version:

 {
   "version": 1,
   "decisions": {
     "ADVISORY_NUMBER": [
        {
           "AFFECTED_PACKAGE_NAME_AND_SHA1_FROM_LOCK_FILE":{
             "decision": "RESOLUTION_TYPE",
...

If one update package version, technically the security analysis needs to be re-done, from scratch. If you put path wildcard, this situation will likely not be detected.

@naugtur
Copy link
Author

@naugtur naugtur commented Aug 5, 2020

@nierob that doesn't cover when a package from your dev deps gets ignored because the vulnerable cod is obviously unreachable, but then it resurfaces as a prod dependency pulled by something else and you're busted.

On the app level (the resolver app that asks you what to do) the decision can be aggregated, but the stuff written in audit-resolve.json needs to be a snapshot of where the package was found at the time you decided to ignore it.

And it's not affected by npm's deduplication, that happens on a differnt leve, so the entries in package-lock are not moved around.

@nierob
Copy link

@nierob nierob commented Aug 5, 2020

@nierob that doesn't cover when a package from your dev deps gets ignored because the vulnerable cod is obviously unreachable, but then it resurfaces as a prod dependency pulled by something else and you're busted.

True. With the current config definition, the opposite is also true. One can update package version and previously unreachable code path may became reachable.

You convinced me that it needs to contain all three; path, package version and vulnerability id :-) In such case no much can be done regarding the file size. Maybe path prefixes can be used... Still I would discuss that as a format update, in a different RFC, it looks like a side-track.

The implementation should introduce support for reading and applying decisions from `audit-resolve.json` file

Features:
- Let users save tecisions about vulnerabilities and change `npm audit` behavior to accomodate that.

This comment has been minimized.

@nfriedly

nfriedly Sep 10, 2020

Was "tecisions" supposed to say "decisions" ?

This comment has been minimized.

@naugtur

naugtur Sep 11, 2020
Author

That's definitely one of the possibilities.

@jfaylon
Copy link

@jfaylon jfaylon commented Dec 9, 2020

Please allow this this for the open source libraries to have time to fix the vulnerabilities. We're using npm audit for our systems for security and 1 security advisory can block the entire deployment for us. If we have the ignore for x number of days/hours to give ample time for the libraries/dependencies to update their libraries, that would be great.

@naugtur
Copy link
Author

@naugtur naugtur commented Dec 9, 2020

@jfaylon There's a collaboration space being set up under the OpenJS Foundation to tackle this.

For now you can use https://www.npmjs.com/package/npm-audit-resolver for your vulnerability management. My team's been doing that for a few years now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet