Recording violations with non-binary audit criteria

[djkoloski]

On Fuchsia, we'd like to record crate audits for UB risk on a scale from 0 (no unsafe code) to 4 (hard to avoid UB). We have that set up like this:

[criteria.ub-risk-0]
description = "..."
implies = "ub-risk-1"

[criteria.ub-risk-1]
description = "..."
implies = "ub-risk-2"

[criteria.ub-risk-2]
description = "..."
implies = "ub-risk-3"

# ...

Which works well, we can record our audits by picking the strictest criteria that a crate satisfies. However, this makes it tricky to record violations:

# `foo` is ub-risk-2, which implies ub-risk-3 or more and NOT ub-risk-1 or less
[[audits.foo]]
criteria = "ub-risk-2"

[[audits.foo]]
violation = "ub-risk-1"

[[audits.foo]]
violation = "ub-risk-0"

Right now, it looks like we'll have to record violations at every lower ub-risk- level than the one we signed off on. Ideally, we'd be able to opt into contrapositivity with implies: "ub-risk-0 implies ub-risk-1" also means that "not ub-risk-1 implies not ub-risk-0". That would bring us down to needing a single extra violation, which would be easier to handle.

If we could do that, then it would also be helpful for us to record the violation along with the criteria like:

[[audits.foo]]
criteria = "ub-risk-2"
violation = "ub-risk-1"

[bholley]

Ah interesting. So the idea here is that cargo-vet should apply modus tollens on implies relationships, and generate implicit violations for implying criteria? That seems sensible: a safe-to-run violation should imply a safe-to-deploy violation as well. I actually would have thought we did that already, though honestly we've never used violation much.

I imagine it should also be pretty straightforward to allow criteria and violation to exist in the same audit entry.

@mystor WDYT?

[djkoloski]

I did some testing and it seems that safe-to-deploy and safe-to-run already have a modus tollens relationship:

first/supply-chain/audits.toml

[[audits.byteorder]]
who = "David Koloski <djkoloski@gmail.com>"
criteria = "safe-to-run"
violation = "^1.4.3"

second/Cargo.toml

[package]
name = "second"
version = "0.1.0"
edition = "2021"

[dependencies]
byteorder = "=1.4.3"

second/supply-chain/config.toml

[imports.first]
url = "http://localhost:8000/supply-chain/audits.toml"

[policy.byteorder]
criteria = "safe-to-deploy"

[[exemptions.byteorder]]
version = "1.4.3"
criteria = "safe-to-deploy"

This yields the error we'd expect:

$ cargo vet check
Violations Found!
  byteorder:1.4.3
    the exemption 1.4.3
      criteria: ["safe-to-deploy"]
    conflicts with foreign (first) violation against ^1.4.3
      criteria: ["safe-to-run"]
      who: David Koloski <djkoloski@gmail.com>

However, swapping these for custom criteria:

first/supply-chain/audits.toml:

[criteria.safe-to-deploy-2]
description = "safe-to-deploy"
implies = "safe-to-run-2"

[criteria.safe-to-run-2]
description = "safe-to-run-2"

[[audits.byteorder]]
who = "David Koloski <djkoloski@gmail.com>"
criteria = "safe-to-run-2"
violation = "^1.4.3"

second/supply-chain/audits.toml:

[criteria.safe-to-deploy-2]
description = "safe-to-deploy"
implies = "safe-to-run-2"

[criteria.safe-to-run-2]
description = "safe-to-run-2"

second/supply-chain/config.toml:

[imports.first]
url = "http://localhost:8000/supply-chain/audits.toml"

[policy.byteorder]
criteria = "safe-to-deploy-2"

[[exemptions.byteorder]]
version = "1.4.3"
criteria = "safe-to-deploy-2"

Passes the check:

$ cargo vet check
Vetting Succeeded (1 exempted)

But also unexpectedly passes if the criteria is set to safe-to-run-2 as well. Perhaps a bug? I'll try and minimize.

[mystor]

This error with safe-to-deploy-2 and safe-to-run-2 is happening because unlike the built-in criteria, custom criteria are not identity-mapped into the local namespace by default. You need to add a [imports.first.criteria-map] table mapping the criteria to local criteria in order to catch the relevant errors. Something like this could work:

[imports.first]
url = "http://localhost:8000/supply-chain/audits.toml"

[imports.first.criteria-map]
safe-to-deploy-2 = "safe-to-deploy-2"
safe-to-run-2 = "safe-to-run-2"

You can also use this to change how criteria are mapped for the default criteria, though there is an identity mapping by-default.

In terms of your original request around combining violations and audits into a single criteria, this would be potentially possible, but not the easiest to implement, as we currently always require only one key, and don't allow combo entries like this. If we did add something like that, it'd also be odd as the violation field for most violation entries is a VersionReq to specify a range of versions, rather than a criteria to add a violation for. Overloading the meaning like that feels a bit confusing.

It would be interesting to learn more about how you want to use violations for these ub criteria. Right now our story around violations is one of the weakest stories (as others haven't had a use for them yet, so they haven't been exercised much), and it'd be good to learn more about how people who want to use them expect them to act.

[bholley]

If I had to guess, I might interpret the goal here to be to affirmatively record the discovered UB issues and ensure someone else doesn't come along and patch the audit hole with a less-careful review. If that's the case though, you probably also want to do the work to specify a full version range rather than limiting the violation to the precise version you audited. Otherwise, it won't apply to the next trivial point release, even though the issue is almost certainly still there.

[djkoloski]

Our goal is basically just to catch inconsistencies, and catch them eagerly. If we didn't use violations when recording these kinds of continuous criteria, we might have one of our imports provide (e.g.) ub-risk-2 for something that we assigned ub-risk-3. If there were some alternative way to enforce that criteria, that would also work for us. It's also nice that we can catch violations eagerly even while a crate is exempted.

One additional note is that it's currently pretty painful to reverse-engineer violation version ranges. We usually sit down with a specific version of a crate, review the code, and find some issues. To record a violation properly, we then additionally need to either:

• Find their repo and dig through their git history (this only works if they don't do a lot of branching)
• Pull down versions of their crate and bisect to find when the violation got introduced (annoying / a lot of work)

I think it would be helpful to provide some tooling for bisecting violations, and it's a superset of what inspect does right now. Does that sound reasonable? I can split that into a separate issue as well.

[bholley]

That bisection facility seems reasonable to add, feel free to file an issue for it.

I'm also curious as to whether eagerly determining the violation range is the optimal process for you, or whether that's just an artifact of the cargo-vet workflow which we could consider improving. It seems to me that it might be enough to say "at least some versions of crate foo are ub-risk-3, including version X we considered importing" — and then defer the question of whether the problem exists in version Y to the moment where somebody actually tries to import version Y. Would this be preferable?

[djkoloski]

Hm, I think that would actually be very helpful. If we could just say "a violation was introduced at some point between prior review and current review", then that would be sufficient for us. Maybe that could be used to prioritize which crates to audit using cargo vet suggest? I think doing the version bisecting upfront doesn't do us too much good unless we plan to use some intermediate crates.