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

[chore]: import audits to cargo vet #1272

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

MCJOHN974
Copy link
Collaborator

@MCJOHN974 MCJOHN974 commented Jan 27, 2025

Description

I did some googling and found that cargo vet do not automatically do any auditing. Instead, on cargo vet init it marks all dependencies as "exempted". To shrink size of "exempted" set and move crates to "audited" status there are basically two ways:

  • Do audit by ourselves
  • Import audits

In this PR I import audits provided by some big parties like Google, Mozilla, Bytecode Alliance, etc. (Actually I just copied imports used in cargo vet github repo. This change moves around 100 dependencies from "exempted" to "audited" status.

However, there is still 600 exempted dependencies. We can add some more audit imports, and shrink this size, but I don't believe we can push it down to zero. And I don't think we need to do this, security is never about "build 100% secure code". It always about "let's add some more 9s to our 99.9999% secure code".

Changes

Adds audit imports to cargo vet and updates cargo vet lockfile.

Testing Information

No testing needed, once CI is green it's fine.

Checklist:

  • I have performed a self-review of my code
  • My changes generate no new warnings
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@MCJOHN974 MCJOHN974 marked this pull request as ready for review January 27, 2025 09:47
@MCJOHN974 MCJOHN974 requested a review from cylewitruk January 27, 2025 09:47
@cylewitruk
Copy link
Member

cylewitruk commented Jan 27, 2025

@MCJOHN974 I actually spoke with @fabergat about this last night saying that I think we should do this to help reduce the diffs, so I'm for it. But let's get the others' opinions as well -- @djordon @matteojug

Copy link
Collaborator

@matteojug matteojug left a comment

Choose a reason for hiding this comment

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

I'm okay with adding trusted audit sources, I'm just not sure if embark and isrg qualifies as such.

@MCJOHN974
Copy link
Collaborator Author

MCJOHN974 commented Jan 28, 2025

I'm okay with adding trusted audit sources, I'm just not sure if embark and isrg qualifies as such.

@matteojug I just copied list of trusted sources from config.toml in cargo vet repo: https://github.com/mozilla/cargo-vet/blob/main/supply-chain/config.toml

From my opinion while working in this direction we should first try to move as much deps as possible from "exempted" to "audited" status and then try to switch from less trusted audit to more trusted.

Copy link
Collaborator

@matteojug matteojug left a comment

Choose a reason for hiding this comment

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

For my opinion working on this direction we should first try to move as much deps as possible from "exempted" to "audited" status and then try to switch from less trusted audit to more trusted.

Yeah good point, better to have someone saying they audited them than just add exceptions.
Approving, but let's wait to hear also from @djordon . Also, you should probably rebase this after the recent #1271.

@cylewitruk
Copy link
Member

For my opinion working on this direction we should first try to move as much deps as possible from "exempted" to "audited" status and then try to switch from less trusted audit to more trusted.

Yeah good point, better to have someone saying they audited them than just add exceptions. Approving, but let's wait to hear also from @djordon . Also, you should probably rebase this after the recent #1271.

Yeah I agree with this logic as well -- exemptions give us nothing.

There is the small thing that if it is trusted then it's not going to show up as a diff to give us the opportunity to audit ourselves. But tbh I think that without the trusted imports, the chances of us choosing audit vs. exempt become more slim due to the sheer volume of diffs.

Copy link
Collaborator

@djordon djordon left a comment

Choose a reason for hiding this comment

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

Yeah these changes make sense to me.

The main question I have is what our policy should be for importing an organizations audits. It appears quite consequential, since their mistakes imply a false confidence of our supply chain. Let's chat offline about this and figure out how we want to handle these things.

Some proposed rules:

  • Three engineers must agree on importing an organizations audits.
  • Imports of a highly trusted organization are trusted.
  • Consensus must be reached on designation of a highly trusted organizations.

Let's all chat about this first before merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: In Review
Development

Successfully merging this pull request may close these issues.

4 participants