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

Add workflow for dependabot and cargo vet #537

Merged
merged 2 commits into from
Oct 26, 2023
Merged

Conversation

jeffcharles
Copy link
Collaborator

Description of the change

Fixes #531. Adds a workflow to run cargo vet, trust our trusted publishes, and run cargo vet regenerate exemptions on pushes to branches used by Dependabot for cargo.

I don't think this introduces a security vulnerability because it should only run on branches in the javy repository and branches on forks would have the logic run in the branch on their fork.

Why am I making this change?

Most of our Dependabot PRs are failing due to cargo vet --locked failing and this should address that.

Checklist

  • I've updated the relevant CHANGELOG files if necessary. Changes to javy-cli and javy-core do not require updating CHANGELOG files.
  • I've updated the relevant crate versions if necessary. Versioning policy for library crates
  • I've updated documentation including crate documentation if necessary.

- run: cargo vet trust --all Amanieu
continue-on-error: true

- run: cargo vet regenerate exemptions
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't feel comfortable us blindly doing this, for several reasons:

  • cargo-vet's docs explicitly recommend doing this as a last resort:

    The most aggressive solution is to run cargo vet regenerate exemptions which will add whatever exemptions necessary to make check pass (and remove uneeded ones). Ideally you should avoid doing this and prefer adding audits, but if you've done all the audits you plan on doing, that's the way to finish the job.

  • Even though this is probably the path of least resistance, following this approach will potentially reduce the possibility of allowing us to shrink our exemptions list, which is what I personally think we should be going after: every time a dependency is updated, we could (and probably should) check if audits or trust criteria for a particular crate has changed in the other supply chains that we import, for example, I noticed that we have exemptions for clap, but Wasmtime trusts it, is there a good reason for us not to trust it as well? This also seems to be a side effect of us relying mostly on cargo vet --locked and perhaps not running cargo vet (without --locked), which as per the docs, will fetch the imports and give suggestions, according to the imports, for example if I remove clap's exemption from our list and run cargo vet I get the following suggestion to trust epage

1 unvetted dependencies:
  clap:2.34.0 missing ["safe-to-deploy"]

recommended audits for safe-to-deploy:
    Command                        Publisher  Used By                  Audit Size
    cargo vet inspect clap 2.34.0  kbknapp    criterion and structopt  25252 lines
      NOTE: mozilla and bytecode-alliance trust Ed Page (epage), who published another version of this crate - consider cargo vet trust clap epage

clap is just one example, but I think that if we don't keep regenerating the exemptions, we'd be able to get rid of other exemptions in the exemptions list by relying on all the delta audits that are already published.

Copy link
Collaborator Author

@jeffcharles jeffcharles Oct 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll remove this line. I don't know how to automate adding additional trusted users given cargo vet doesn't appear to have a stable output format I can use a readily available parser on.

This also seems to be a side effect of us relying mostly on cargo vet --locked and perhaps not running cargo vet (without --locked), which as per the docs, will fetch the imports and give suggestions, according to the imports, for example if I remove clap's exemption from our list and run cargo vet I get the following suggestion to trust epage

Yeah I already run cargo vet without the --locked parameter whenever I make a dependency change and add trusted publishers if our imports also trust the author.

What happened in clap's case is it's owned by multiple users. epage published a different version of the crate. So running cargo vet trust --all epage would not result in the crate being trusted. We have to run cargo vet trust clap epage because epage published a different version but we're vouching that we trust the version we're using (I'm not clear on why we specify the user in this case).

Should we adopt the policy that if a publisher trusted by one of our imports has published a different version of the crate, we should trust the version of the crate we're using if it's been published by a different publisher who is not trusted by any of our imports?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh that's right, if I trust epage and run cargo-vet after (having removed the exemption for clap) -- I still get a warning about our current version.

Should we adopt the policy that if a publisher trusted by one of our imports has published a different version of the crate, we should trust the version of the crate we're using if it's been published by a different publisher who is not trusted by any of our imports?

I'd prefer if in this case we still require an audit/exemption since even though we assume that it's probably fine, we are trusting explicitly what a particular user publishes. Either that or updating to a dependency published by a trusted user. Like in the case of clap, if we update to a newer version, we can get rid of the exemption. (Sidenote: I'm curious why dependabot hasn't updated clap, we're in a old version)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer if in this case we still require an audit/exemption since even though we assume that it's probably fine, we are trusting explicitly what a particular user publishes.

Agreed!

I'm curious why dependabot hasn't updated clap, we're in a old version

clap is a transitive dependency used by criterion and structop. structopt depends on version 2.x of clap which pins it to a 2.x version in our project. structop is in maintenance mode so they're likely not updating their dependencies regularly. If we want to use a newer version of clap we should complete #321.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah that explains it -- I thought we had migrated.

@jeffcharles jeffcharles merged commit 4956c74 into main Oct 26, 2023
14 checks passed
@jeffcharles jeffcharles deleted the jc.dependabot-cargo-vet branch October 26, 2023 14:28
jeffcharles added a commit that referenced this pull request Oct 26, 2023
jeffcharles added a commit that referenced this pull request Oct 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GitHub Action for Dependabot PRs to run cargo vet
2 participants