search

freedomofpress / securedrop

GitHub repository for the SecureDrop whistleblower platform. Do not submit tips here!
https://securedrop.org/
Other
3.63k stars 686 forks source link

Determine plan for auditing Rust dependencies #6500

Closed legoktm closed 1 year ago

legoktm commented 2 years ago

As part of using Sequoia, we'll start pulling in Rust dependencies (161 crates by my current count!). The process for performing "diff reviews" of Python dependencies is documented on the wiki.

We may want to take advantage of some Rust tools that allow for sharing audit status like:

legoktm commented 2 years ago

I reviewed both the cargo-crev and cargo-vet documentation, and while it seems less mature, my preference is for cargo-vet.

cargo-crev is designed for individuals doing reviews, each person gets a crev ID and hosts their own crev proof repository. While cargo-vet seems more aligned with trusting orgs for reviews.

cargo-crev is built on the web of trust, so when you trust person, you now implicitly trust all the other people they trust. I think that's fine for casual projects, but I think we're a lot more paranoid than that! I imagine we would trust the audits done by Mozilla/Firefox since if we don't trust them we're already pwned, but not necessarily every person who someone who works on Firefox trusts.

And ultimately cargo-vet seems conceptually simpler, there's no cryptography involved or keys or IDs, it just relies on git repos (so HTTPS).

legoktm commented 1 year ago

I tried out cargo vet today, it seems to work pretty nicely. The main "issue" I ran into was that we are deploying to a very specific platform ("target" in Rust lingo), Linux x86_64. There are a bunch of Windows/WASM/Redox dependencies in the vet list that we don't actually use that I filtered out manually based on https://github.com/mozilla/cargo-vet/issues/63#issuecomment-1382836458. That ticket has some suggestions on how upstream can implement some form of target limiting.

legoktm commented 1 year ago

Thinking closely about the Python diff review workflow, there are two steps missing:

legoktm commented 1 year ago

I think we're mostly set on going forward with cargo vet, but FWIW Google has also adopted it: https://opensource.googleblog.com/2023/05/open-sourcing-our-rust-crate-audits.html

cfm commented 1 year ago

@legoktm, thanks for your research and recommended links here. I've read the cargo-vet manual and like both the trust model it implements and the review ergonomics it offers.

Re: https://github.com/freedomofpress/securedrop/issues/6500#issuecomment-1409539888 (modulo my conversion to ordered list :-), the cargo-vet team has addressed both of these topics in mozilla/cargo-vet#449:

Thinking closely about the Python diff review workflow, there are two steps missing:

  1. We don't verify that the version we review matches checksum of what is included in Cargo.lock. This probably needs to be fixed upstream.

Their claim:

Cargo-vet primarily operates on releases from crates.io, for which digests are published in the public append-only crates.io registry index. In other words, the index maps (crate, version) to digest, and so by depending on the index, cargo-vet can map that tuple to digest without including the digest inline.

I agree with you that this still requires trusting cargo (as we do for pip) and crates.io and the index (as we do not do for pypi.org). However, it appears to be a deliberate design decision.

  1. There's no opportunity to PGP sign the review. If we consider this to be a requirement, I would suggest we just mandate GPG-signing of commits that add new audits.

Their claim:

Cargo-vet operates at the granularity of organizations/projects rather than individuals. Organizations can (and sometimes do) cryptographically bind audits to authors (by requiring them to sign the relevant git commit) but cargo-vet operates only on the audit file, not any VCS history that may have been involved in creating it.

To feel comfortable with this, I think we'd want to upgrade our "commits should be signed" to "commits MUST be signed", enforced by CI.

See also:

legoktm commented 1 year ago

To feel comfortable with this, I think we'd want to upgrade our "commits should be signed" to "commits MUST be signed", enforced by CI.

If we want to enforce it by CI (a good idea IMO), then I think we should just sign the relevant files, similar to how we sign sha256sums in securedrop-builder, so we're not dependent on tracking the entire git history to ensure it hasn't been touched by an unsigned commit, we can just check the latest state.

bholley commented 1 year ago

Not that I'm any kind of stakeholder here, but it seems to me that if you consider commit signing important enough to enforce for third-party code integrity, it would make sense to enforce it for first-party code as well.

zenmonkeykstop commented 1 year ago

Generating signed hashes for the audit files seems like an extra hurdle, I'd be in favour of adding an overall commit signing policy - only downside I can see is inconvenience/barrier to entry.

Failing that, a more lightweight approach would be to define code ownership for said files and ensuring the owner group is a) signing their commits and b) comfortable reviewing changes to the cargo vet audits.

legoktm commented 1 year ago

We have some discussion re: signing policy at https://github.com/freedomofpress/securedrop/issues/6943. Ultimately since these audits are all being stored in the Git history and attributable, I don't think we need to require PGP signatures (in contrast our diff reviews are posted on wiki pages without any pre-review).

Also, @bholley, thanks for commenting and your work on cargo-vet :) I've put up our initial configuration at https://github.com/freedomofpress/securedrop/pull/6981 if you're interested in taking a look.