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

implement imports.exclude #80

Closed
bholley opened this issue May 16, 2022 · 6 comments · Fixed by #310
Closed

implement imports.exclude #80

bholley opened this issue May 16, 2022 · 6 comments · Fixed by #310
Labels
p1 do now

Comments

@bholley
Copy link
Collaborator

bholley commented May 16, 2022

At present cargo-vet largely punts on conflict handling, we should define it.

Generally speaking, audits are additive and can't generate conflicts. However, it can happen if the range provided in a violation entry intersects a leaf in the audit version graph. This basically means that two parties explicitly disagree on whether the criteria are met for that version. Examples might include:

  1. Somebody missed a problem when certifying an audit.
  2. Somebody provided an overly-broad violation range.
  3. Two parties fundamentally disagree on whether there's a problem.

I think (1) and (2) are the most likely cases, and so I think we want the default behavior to result in a discussion and ideally a consensus resolution. However, we can't have the tool entirely fall apart while that discussion is happening, or in the actual case of (3). So we need some mechanism to immediately resolve the conflict locally.

My spitball suggestion here is to add an exclude field to imports table entries, which lists a set of crates for which audits are dropped for that import. That strikes me as a lightweight mechanism to resolve conflicts in either direction (local violation + foreign certification, or vice versa), while also ensuring that any such conflict is surfaced for manual inspection.

@tmandry wdyt?

@Gankra
Copy link
Contributor

Gankra commented May 16, 2022

I think I've been picturing a local override more as a flag only local audits can have with an extra flag for "and i mean it", ala !important in CSS, but maybe that's overly broad?

The flipside over-engineered version of this would be generating a uuid for each forbid entry so people can explicitly name it.

@bholley
Copy link
Collaborator Author

bholley commented May 16, 2022

Yeah, my thinking was that being able to selectively reject imported audits as proposed above is a simple safety valve that doesn't complicate the model or lead to surprising results.

@tmandry
Copy link

tmandry commented May 16, 2022

One case where I could see fundamental disagreements cropping up are where we have unsafe code that is technically undefined behavior but also unlikely to ever be a problem. We come across this fairly often. (recent example)

In any case:

A problem I see with the model being proposed here is that a trusted upstream audit source can suddenly cause cargo vet to start failing without any changes, which is not nice if it's running in CI. Off the top of my head there are a few approaches I see that could work...

  • Warn when there's a conflict, and ask the user to resolve it somehow to remove the warning, but don't fail
  • Vendor upstream audits with a command that updates them periodically. These vendored files then become the source of truth until they are updated again.

@Gankra
Copy link
Contributor

Gankra commented May 16, 2022

Yes there is already a "lockfile" for foreign audits -- imports.lock, one of the 3 files managed by cargo vet and which is already minimally implemented. It's just a cache of all the foreign files, nothing fancy.

The expected workflow is that CI will run with cargo vet --locked and therefore not update the lockfile at all. That said, because the stakes are a lot lower we currently always try to update the lockfile when cargo vet is run without --locked, unlike Cargo.lock, which tries to stay static more aggressively. This is presumably a good thing because you do want to know if one of your cohorts notices a serious issue and forbids something. It's just a question of what you do once you learn about that.

@bholley bholley changed the title Conflict Handling implement imports.exclude May 17, 2022
@bholley
Copy link
Collaborator Author

bholley commented May 17, 2022

I added exclude to the book, morphing this issue to be about implementing.

@bholley bholley added the p2 do soon label May 17, 2022
@Gankra Gankra added p1 do now and removed p2 do soon labels Jun 30, 2022
@mystor
Copy link
Collaborator

mystor commented Jul 11, 2022

I think this will be slightly easier to implement in a clean way once we've also fixed #238, as that way we can more easily track which source each imported entry comes from and do these exclusions locally after the lock has been updated.

I imagine we'll want to have the excluded entries downloaded and in the lockfile to keep formatting and error reporting anyway.

mystor added a commit to mystor/cargo-vet that referenced this issue Aug 25, 2022
This is a fairly straightforward implementation of imports.exclude. The
configuration option is already documented in the book. After changing
this configuration value, you'll need to update imports.lock by running
a successful `cargo vet` without `--locked` (or by running `cargo vet
regenerate imports`).

I also added some validation to help catch outdated `imports.lock` files
when running with `--locked`. These will never fire when unlocked, as
`imports.lock` will be automatically updated in those cases.

Fixes mozilla#80
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p1 do now
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants