Feature: filter returned vulnerabilites and warnings to subtree of a … #862

dkcumming · 2023-04-11T10:18:43Z

…target package

Passing -t followed by the name of a target package omits any vulnerabilities or warnings found that are not from packages in the subgraph formed from the target node. This is achieved by a depth first search for the vulnerable packages, from the target node.

Currently misses for the target name, and multiple targets returned for the target name are handled naively by ignoring flag and continuing with the full graph.

If -t is not provided, no change to complexity.
Providing -t increases complexity by $O(P \times (V' + E'))$ where $V'$ is the vertices in the subgraph (packages), $E'$ is the edges in the subgraph (dependencies), $P$ is the sum of all vulnerabilites and warnings found from the audit.

Shnatsel · 2023-04-11T14:54:30Z

Thank you for the PR!

Could you clarify why such a feature is desirable? This introduces additional complexity to the codebase, and I'm not sure what are the use cases for it.

If we do decide to merge this, I'd also expect all TODOs to be done and tests to be added.

tarcieri · 2023-04-11T20:57:27Z

We'd previously discussed this a bit over email, although I thought the intent was to enable it at the library-level, not necessarily something that would be in cargo-audit

tarcieri · 2023-04-11T20:59:32Z

rustsec/src/report.rs

-    /// Generate a report for the given advisory database and lockfile
-    pub fn generate(db: &Database, lockfile: &Lockfile, settings: &Settings) -> Self {
+    /// Generate a report for the given advisory database and lockfile, optional narrowing of subgraph from target package
+    pub fn generate(db: &Database, lockfile: &Lockfile, settings: &Settings, tree:&dependency::Tree, target:Option<&Package>) -> Self {


There are already two types for carrying additional state so you don't have to add parameters to this method: Settings and its Settings::query field which contains a database::Query.

I would think you could access the dependency tree via the Lockfile, and this feature could potentially set some option on the database::Query.

Yes, this makes more sense. I will do a refactor using Query

dkcumming · 2023-04-16T11:49:44Z

Hi @Shnatsel, the motivation for the feature that auditing large packages can result in a large amount of vulnerabilities being output. Sometimes it is beneficial to isolate a particular part and see what vulnerabilities exist for that component of the package. Previously when auditing a large code base I was using cargo audit in combination with cargo tree to determine if the returned vulnerabilities were dependent in the subtree of a target package, but it felt appropriate to add the feature to cargo audit directly.

…target package Passing `-t` followed by the name of a target package omits any vulnerabilities or warnings found that are not from packages in the subgraph formed from the target node. This is achieved by a depth first search for the vulnerable packages, from the target node. Currently misses for the target name, and multiple targets returned for the target name are handled naively by ignoring flag and continuing with the full graph. If `-t` is not provided, no change to complexity. Providing `-t` increases complexity by $O(P \times (V' + E'))$ where $V'$ is the vertices in the subgraph (packages), $E'$ is the edges in the subgraph (dependencies), $P$ is the sum of all vulnerabilites and warnings found from the audit.

…s` occurs in `query_vulnerabilities`. `Warnings` that are `Yanked` are still occuring in `check_for_yanked`.

- Accepts target package identifier, and validates correct name, version, source. - Filters packages in lockfile by the name, version, source looking for unique package matching target. - If no package matches, the identifier is retuned with the error. - If multiple packages could match the identifier, then the provided identifier and the possible targets are returned with the error. - If a unique target is found the package is returned.

dkcumming · 2023-05-05T09:32:15Z

Hi @Shnatsel @tarcieri,
Sorry for the long time between commits. I have had other work. I implemented some solutions to the requested changes, however this push is to get some feedback from you. Some details of this push are:

Features:

Refactored the parameters to make use of query.
Added feedback to user if target package doesn't exist, or multiple packages match the identifier. I had a look at cargo tree -p package for this as they had nice feedback for this. I pulled and edited some of the code (credited), extending the functionality useful for this case and stripping what wasn't relevant. But as it is now, The user provides a package identifier as a combination of name, version, url.
If no package matches the identifier then the user receives
error: No target package found matching identifier: not-there
And if multiple packages match the identifier then the user receives
```
error: Multiple packages found matching identifier: hydra-dx-math
These packages were found that could match:
    https://crates.io/foo#[email protected]
    https://crates.io/foo#[email protected]
```

These strings that are returned are able to be copy pasted for the next cargo audit -t command and will parse correctly.

Known Problems:

The location of the files is not in final place. As I was moving them I thought it better to get some feedback from you.
Formatting of error messages can be improved, just wanted to see if you had anything to say on this.
I expect there is some sub optimal rust code in here, I am not an expert rust developer by any means, so feedback is appreciated.
The cargo tree -p method does not support if people screw up the versioning. For example if name, version, url are the same but the commit at url is different. I haven't looked into it deeper if that is intended or not.
The tree does not print from the target package node. I missed this as I only use the json implementation, but I will implement it.

Let me know what you think when you get some time. Thank you.

dkcumming · 2023-05-05T09:55:47Z

cargo-audit/src/lockfile.rs

+
+// ! Package identifiers.
+// !
+// ! Adapted from Cargo's `package_id_spec.rs`:
+// !
+// ! <https://github.com/rust-lang/cargo/blob/master/src/cargo/core/package_id_spec.rs>
+// !
+// ! Copyright (c) 2014 The Rust Project Developers
+// ! Licensed under the same terms as the `cargo-lock` crate: Apache 2.0 + MIT


This does not belong here, but I was wondering what structure you would recommend.

Running `cargo audit` alone, or with `-t multi-vulns` will show both vulnerabilities. Running `cargo audit` with `-t base64` will show the `base64` vulnerability, and omit the `chrono` branch with `time` vulnerability. Running `cargo audit` with `-t chrono`, or `-t time` will show the `time` vulnerability, and omit the `base64` vulnerability.

dkcumming · 2023-05-22T12:02:35Z

Further progression. I refactored and added test cases to show the restriction of output for different branches. These can be run in the from the test directory like so:

No filtering
multi_vulns$ ../../../../target/debug/cargo-audit audit -t multi_vuln or
multi_vulns$ ../../../../target/debug/cargo-audit audit
-Filter base64 branch
multi_vulns$ ../../../../target/debug/cargo-audit audit -t base64
-Filter chrono branch
multi_vulns$ ../../../../target/debug/cargo-audit audit -t chrono or
multi_vulns$ ../../../../target/debug/cargo-audit audit -t time (for the time dependency vuln directly)

Previously I thought there was an issue with the layout when printing the tree "The tree does not print from the target package node. I missed this as I only use the json implementation, but I will implement it.". After using this a few times I realised the current output is correct as is.

@Shnatsel @tarcieri
Are you able to provide feedback, or what you would need to see for a merge?
This change has been very useful for RV with audits, and we believe is likely useful to others who use cargo audit, it would be great to make it available.

tarcieri · 2023-05-22T13:00:28Z

I worked through reviewing a rather large backlog of PRs this weekend and didn't quite get to this one. Maybe next weekend.

dkcumming · 2023-05-23T01:39:43Z

No problem, I see that some CI tests failed. Looks like related to the lockfile. Is that related to anything on my end that I need to address?

tarcieri · 2023-05-23T20:21:02Z

You can try rebasing

dkcumming · 2023-05-24T01:46:04Z

I synced the main branch with upstream/main. The feature branch was already merged with the upstream/main. Is this what you meant?

ACassimiro · 2023-06-29T13:08:43Z

@Shnatsel @tarcieri I would just like to leave a ping here and say that this was a really helpful feature for going through specific packages in a huge repository. It saved me quite a bit of effort.

tarcieri · 2023-06-29T17:20:08Z

I should have some time this weekend to take a look

dkcumming · 2023-06-30T01:27:25Z

@tarcieri If there would be any benefit from a meeting to go over anything, I'm available pretty much anytime weekend or weekday for a meeting.

tarcieri · 2023-07-01T21:47:08Z

@dkcumming can you rebase? that should fix the test failures

dkcumming · 2023-07-01T22:09:09Z

@tarcieri Should be updated now

tarcieri · 2023-07-01T22:27:49Z

@dkcumming looks like there's a test failure

…-audit-extension into feature-target-package

dkcumming · 2024-10-27T18:46:01Z

@Shnatsel @tarcieri I haven't actually needed to use this feature on any projects since I have only been working with smaller trees since I did this. But I had some spare time this weekend to go and fix the problems that were on CI. If I get time another weekend I might add some test cases, but since I haven't needed this feature I am just going to put that on hold for now.

Shnatsel · 2024-10-27T22:56:06Z

Thank you for updating the PR!

I admit I am hesitant to merge this, because we are struggling to maintain the code for the basic functionality as it is. Taking on more complex features would make it even harder, and is probably not something we should be doing at this stage, unless someone is willing to provide funding to the project. @tarcieri WDYT?

tarcieri · 2024-10-28T16:30:50Z

I think my general concern remains that this contains logic in cargo-audit that would probably make more sense in cargo-lock and rustsec

tarcieri reviewed Apr 11, 2023

View reviewed changes

dkcumming added 5 commits May 1, 2023 20:15

Refactored so that dfs filtering for Vulnerabilities and `Warning…

d1a9541

…s` occurs in `query_vulnerabilities`. `Warnings` that are `Yanked` are still occuring in `check_for_yanked`.

removed debugging print statements

f0121a9

Merge remote-tracking branch 'upstream/main' into feature-target-package

6bc1087

dkcumming force-pushed the feature-target-package branch from 4360c3d to 6bc1087 Compare May 5, 2023 09:21

dkcumming commented May 5, 2023

View reviewed changes

dkcumming added 3 commits May 22, 2023 19:45

Refactor of PackageIdSpec.

f0b03ca

Merge remote-tracking branch 'upstream/main' into feature-target-package

6eb2929

Merge remote-tracking branch 'upstream/main' into feature-target-package

721fdce

dkcumming added 3 commits October 27, 2024 13:49

Merge branch 'main' into feature-target-package

f8b2af8

Correct feature flags for pet-graph / "dependency-tree"

02ff386

Fixing clippy

cb8384f

dkcumming added 5 commits October 27, 2024 17:37

Fixing rust fmt

2fa69f6

Fixing imports of dependencies for cfg(feature = "dependency-tree")

c2c87df

Fixed bug with feature flags

c043282

Fixed bug with feature flags

c7c40ce

Merge branch 'feature-target-package' of github.com:dkcumming/rustsec…

de87b9a

…-audit-extension into feature-target-package

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: filter returned vulnerabilites and warnings to subtree of a … #862

Feature: filter returned vulnerabilites and warnings to subtree of a … #862

dkcumming commented Apr 11, 2023

Shnatsel commented Apr 11, 2023

tarcieri commented Apr 11, 2023

tarcieri Apr 11, 2023

dkcumming Apr 16, 2023

dkcumming commented Apr 16, 2023

dkcumming commented May 5, 2023 •

edited

Loading

dkcumming May 5, 2023

dkcumming commented May 22, 2023

tarcieri commented May 22, 2023

dkcumming commented May 23, 2023

tarcieri commented May 23, 2023

dkcumming commented May 24, 2023

ACassimiro commented Jun 29, 2023

tarcieri commented Jun 29, 2023

dkcumming commented Jun 30, 2023

tarcieri commented Jul 1, 2023

dkcumming commented Jul 1, 2023

tarcieri commented Jul 1, 2023

dkcumming commented Oct 27, 2024

Shnatsel commented Oct 27, 2024

tarcieri commented Oct 28, 2024

Feature: filter returned vulnerabilites and warnings to subtree of a … #862

Are you sure you want to change the base?

Feature: filter returned vulnerabilites and warnings to subtree of a … #862

Conversation

dkcumming commented Apr 11, 2023

Shnatsel commented Apr 11, 2023

tarcieri commented Apr 11, 2023

tarcieri Apr 11, 2023

Choose a reason for hiding this comment

dkcumming Apr 16, 2023

Choose a reason for hiding this comment

dkcumming commented Apr 16, 2023

dkcumming commented May 5, 2023 • edited Loading

dkcumming May 5, 2023

Choose a reason for hiding this comment

dkcumming commented May 22, 2023

tarcieri commented May 22, 2023

dkcumming commented May 23, 2023

tarcieri commented May 23, 2023

dkcumming commented May 24, 2023

ACassimiro commented Jun 29, 2023

tarcieri commented Jun 29, 2023

dkcumming commented Jun 30, 2023

tarcieri commented Jul 1, 2023

dkcumming commented Jul 1, 2023

tarcieri commented Jul 1, 2023

dkcumming commented Oct 27, 2024

Shnatsel commented Oct 27, 2024

tarcieri commented Oct 28, 2024

dkcumming commented May 5, 2023 •

edited

Loading