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

Determine plan for auditing Rust dependencies #6500

Closed
legoktm opened this issue Jul 21, 2022 · 9 comments · Fixed by #6981
Closed

Determine plan for auditing Rust dependencies #6500

legoktm opened this issue Jul 21, 2022 · 9 comments · Fixed by #6981
Assignees

Comments

@legoktm
Copy link
Member

legoktm commented Jul 21, 2022

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
Copy link
Member Author

legoktm commented Aug 1, 2022

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).

@ghost ghost self-assigned this Sep 13, 2022
legoktm added a commit that referenced this issue Jan 30, 2023
Since we only target Linux x86_64 machines, I ignored all the obvious
Windows/WASM/Redox dependencies. In the future it seems like we should
be able to set an explicit target for this (mozilla/cargo-vet#63).

We import the Firefox audits to help reduce review load; the SecureDrop
security model relies on Firefox already via Tor Browser.

Refs #6500
@legoktm
Copy link
Member Author

legoktm commented Jan 30, 2023

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 mozilla/cargo-vet#63 (comment). That ticket has some suggestions on how upstream can implement some form of target limiting.

@legoktm
Copy link
Member Author

legoktm commented Jan 30, 2023

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

  • 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.
  • 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.

legoktm added a commit that referenced this issue May 15, 2023
Since we only target Linux x86_64 machines, I ignored all the obvious
Windows/WASM/Redox dependencies. In the future it seems like we should
be able to set an explicit target for this (mozilla/cargo-vet#63).

We import the Firefox audits to help reduce review load; the SecureDrop
security model relies on Firefox already via Tor Browser.

Refs #6500
legoktm added a commit that referenced this issue May 16, 2023
Since we only target Linux x86_64 machines, I ignored all the obvious
Windows/WASM/Redox dependencies. In the future it seems like we should
be able to set an explicit target for this (mozilla/cargo-vet#63).

We import the Firefox audits to help reduce review load; the SecureDrop
security model relies on Firefox already via Tor Browser.

Refs #6500
@zenmonkeykstop zenmonkeykstop moved this to Cycle Backlog in SecureDrop dev cycle May 19, 2023
legoktm added a commit that referenced this issue Jun 5, 2023
Since we only target Linux x86_64 machines, I ignored all the obvious
Windows/WASM/Redox dependencies. In the future it seems like we should
be able to set an explicit target for this (mozilla/cargo-vet#63).

We import the Firefox audits to help reduce review load; the SecureDrop
security model relies on Firefox already via Tor Browser.

Refs #6500
legoktm added a commit that referenced this issue Jun 5, 2023
Since we only target Linux x86_64 machines, I ignored all the obvious
Windows/WASM/Redox dependencies. In the future it seems like we should
be able to set an explicit target for this (mozilla/cargo-vet#63).

We import the Firefox audits to help reduce review load; the SecureDrop
security model relies on Firefox already via Tor Browser.

Refs #6500
@legoktm
Copy link
Member Author

legoktm commented Jun 6, 2023

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

legoktm added a commit that referenced this issue Jun 13, 2023
Since we only target Linux x86_64 machines, I ignored all the obvious
Windows/WASM/Redox dependencies. In the future it seems like we should
be able to set an explicit target for this (mozilla/cargo-vet#63).

We import the Firefox audits to help reduce review load; the SecureDrop
security model relies on Firefox already via Tor Browser.

Refs #6500
@zenmonkeykstop zenmonkeykstop moved this from Cycle Backlog to Ready to go in SecureDrop dev cycle Jun 20, 2023
@cfm
Copy link
Member

cfm commented Jun 21, 2023

@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: #6500 (comment) (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:

@cfm cfm moved this from Ready to go to In Progress in SecureDrop dev cycle Jun 21, 2023
@legoktm
Copy link
Member Author

legoktm commented Jun 22, 2023

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
Copy link

bholley commented Jun 26, 2023

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
Copy link
Contributor

zenmonkeykstop commented Sep 22, 2023

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 added a commit that referenced this issue Oct 10, 2023
cargo vet is a tool designed by Mozilla to record audits of Rust
dependencies, and it matches nicely with the philosophy of our diff
review system for Python. It can automatically present diffs and
verify everything has been checked.

== Trust ==

To reduce the number of audits we have to do, we trust the following
organizations:

* Bytecode Alliance (WASM/WASI)
* Google
* Internet Security Research Group (Let's Encrypt, etc.)
* Mozilla
* Zcash

We also trust a number of individual developers, because they are
members of the Rust Project and also trusted by one of the above
organizations we trust. We also trust the two Sequoia-OpenPGP team
members who release things.

All of the individual trust markers have an expiry date to remind us to
re-evalutate trustworthiness every so often.

== Exemptions ==

There are a number of dependencies that appear in our tree but are not
used on Linux x86_64 so we can ignore them entirely. These are marked in
config.toml with a policy stanza that has an empty criteria block. These
crates have been identified manually, in the future cargo-vet will
hopefully let us specify specific targets we care about and take care of
it automatically.

The remaining exemptions in config.toml have not been reviewed yet; we
can incrementally chip away at them.

== Signing ==

Unlike diff reviews, there is no PGP signing of this file. Because these
are committed into the Git repository directly, we can rely on that as a
measure of trust (unlike random wiki pages).

== CI ==

CI verifies that all dependencies have either been reviewed or exempted,
so there's no need for manual tracking in PR descriptions. Upstream
provides a GitHub Actions template that we use most of.

Fixes #6500.
legoktm added a commit that referenced this issue Oct 10, 2023
cargo vet is a tool designed by Mozilla to record audits of Rust
dependencies, and it matches nicely with the philosophy of our diff
review system for Python. It can automatically present diffs and
verify everything has been checked.

== Trust ==

To reduce the number of audits we have to do, we trust the following
organizations:

* Bytecode Alliance (WASM/WASI)
* Google
* Internet Security Research Group (Let's Encrypt, etc.)
* Mozilla
* Zcash

We also trust a number of individual developers, because they are
members of the Rust Project and also trusted by one of the above
organizations we trust. We also trust the two Sequoia-OpenPGP team
members who release things.

All of the individual trust markers have an expiry date to remind us to
re-evalutate trustworthiness every so often.

== Exemptions ==

There are a number of dependencies that appear in our tree but are not
used on Linux x86_64 so we can ignore them entirely. These are marked in
config.toml with a policy stanza that has an empty criteria block. These
crates have been identified manually, in the future cargo-vet will
hopefully let us specify specific targets we care about and take care of
it automatically.

The remaining exemptions in config.toml have not been reviewed yet; we
can incrementally chip away at them.

== Signing ==

Unlike diff reviews, there is no PGP signing of this file. Because these
are committed into the Git repository directly, we can rely on that as a
measure of trust (unlike random wiki pages).

== CI ==

CI verifies that all dependencies have either been reviewed or exempted,
so there's no need for manual tracking in PR descriptions. Upstream
provides a GitHub Actions template that we use most of.

Fixes #6500.
@legoktm
Copy link
Member Author

legoktm commented Oct 10, 2023

We have some discussion re: signing policy at #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 #6981 if you're interested in taking a look.

@legoktm legoktm added this to the SecureDrop 2.7.0 milestone Oct 10, 2023
legoktm added a commit that referenced this issue Oct 12, 2023
cargo vet is a tool designed by Mozilla to record audits of Rust
dependencies, and it matches nicely with the philosophy of our diff
review system for Python. It can automatically present diffs and
verify everything has been checked.

== Trust ==

To reduce the number of audits we have to do, we trust the following
organizations:

* Bytecode Alliance (WASM/WASI)
* Google
* Internet Security Research Group (Let's Encrypt, etc.)
* Mozilla
* Zcash

We also trust a number of individual developers, because they are
members of the Rust Project and also trusted by one of the above
organizations we trust. We also trust the two Sequoia-OpenPGP team
members who release things.

All of the individual trust markers have an expiry date to remind us to
re-evalutate trustworthiness every so often. For now we've set the
expiry to 6 months as we're still getting familiar with the system.

== Exemptions ==

There are a number of dependencies that appear in our tree but are not
used on Linux x86_64 so we can ignore them entirely. These are marked in
config.toml with a policy stanza that has an empty criteria block. These
crates have been identified manually, in the future cargo-vet will
hopefully let us specify specific targets we care about and take care of
it automatically.

The remaining exemptions in config.toml have not been reviewed yet; we
can incrementally chip away at them.

== Signing ==

Unlike diff reviews, there is no PGP signing of this file. Because these
are committed into the Git repository directly, we can rely on that as a
measure of trust (unlike random wiki pages).

== CI ==

CI verifies that all dependencies have either been reviewed or exempted,
so there's no need for manual tracking in PR descriptions. Upstream
provides a GitHub Actions template that we use most of.

Fixes #6500.
@github-project-automation github-project-automation bot moved this from In Progress to Done in SecureDrop dev cycle Oct 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants