mozilla / cargo-vet

supply-chain security for Rust
Apache License 2.0
621 stars 43 forks source link

Clarify which audits are recommended when `X` `implies` `Y` criteria #613

Closed anforowicz closed 1 month ago

anforowicz commented 1 month ago

Context

In my experience, engineers performing semi-automated crate updates are sometimes confused which criteria to audit for. For example, an engineer may see the following message from cargo vet:

$ tools/crates/create_update_cl.py --no-upload single skrifa
...Updating skrifa@0.19.0 to a newer version...
$ tools/crates/run_cargo_vet.py check
...Vetting Failed!

1 unvetted dependencies:
  skrifa:0.19.1 missing ["safe-to-deploy", "crypto-safe", "ub-risk-2"]

recommended audits for safe-to-deploy, crypto-safe, ub-risk-2:
    Command                              Publisher  Used By   Audit Size
    cargo vet diff skrifa 0.19.0 0.19.1  rsheeter   chromium  12 files changed, 1317 insertions(+), 220 deletions(-)

estimated audit backlog: 1537 lines

Use |cargo vet certify| to record the audits.

The crate has been previously audited as does-not-implement-crypto and ub-risk-0 and the delta doesn't change those properties. But cargo vet's message recommends auditing for crypto-safe and ub-risk-2 instead (which have a transitive implies relationship: ub-risk-0 implies ub-risk-1 which implies ub-risk-2; and does-not-implement-crypto implies crypto-safe - see here and here).

To reduce the confusion, we've tried to cover this in our documentation:

But, it would be great if cargo vet's output could also somehow be tweaked to avoid this kind of confusion (since the doc guidance is easy to miss, but the output of cargo vet is looked at each time a crate is updated). Can we discuss if and how cargo vet's output could be changed?

FWIW this confusion (in light of does-not-implement-crypto vs crypto-safe) has also been touched upon in https://github.com/mozilla/cargo-vet/issues/417 but that other issue focuses more on delta audit workflow surfacing the properties of the baseline audit.

Concrete proposal

I would like to propose changing this part of the output:

```
recommended audits for safe-to-deploy, crypto-safe, ub-risk-2:
```

Into:

```
recommended audits for safe-to-deploy, crypto-safe (or does-not-implement-crypto), ub-risk-2 (or ub-risk-0, or ub-risk 1):
```

It seems possible to derive the new text based on the implies relationship that cargo vet already recognizes between the audit criteria.

I haven't yet tried looking at cargo vet's source code to see where such change could be made. Obviously we wouldn't want to change all criteria => string conversions - hopefully it won't be too difficult to identify which ones can use the new, expanded wording (?).

WDYT?

mystor commented 1 month ago

The string used for the "recommended audits for" printout is populated in a pretty simple way, by iterating the set of failed criteria for the revision, picking a minimal set of criteria which can be used to satisfy it, and concatenating them together into a string:

https://github.com/mozilla/cargo-vet/blob/11690dff98669f23b45c6f8bdb7ab39c0bb7548f/src/resolver.rs#L1982-L1986

It it was changed here, I believe this would only impact that printout and the suggestion list in the JSON output, but wouldn't impact things like what criteria are selected by default when running cargo vet certify. I've thrown together a PR which implements it as it's fairly straightforward (#614).

For most consumers of cargo vet who don't use custom criteria, this would only impact suggested audits for safe-to-run which would now read something like:

recommended audits for safe-to-run (or safe-to-deploy):

That seems fairly reasonable to me unless you have fairly complex implies criteria graphs.