1907810 - cargo vet requires stronger vetting requirement for test-only dependency

Status: UNCONFIRMED

(Developer Infrastructure :: Mach Vendor & Updatebot, defect)

Description

[Max Inden]

By default cargo vet will enable all features on all crates, no matter whether they are used or not. See the cargo vet docs for details. This is problematic where an unvetted optional disabled dependency is included in the vetting process, even though it isn't used.

Example:

• Crate A is used.
• Crate A has an optional dependency on crate B. The feature is disabled and thus crate B is not used.
• Crate B is not vetted.
• Vetting of crate A will fail as cargo vet will enable all features on crate A and thus include unvetted crate B.

A concrete failure is described in detail in https://phabricator.services.mozilla.com/D212959#inline-1194604.

One possible solution is to override the default by passing --no-all-features. I will propose this as a Phabricator Patch.

Comment 1

[Max Inden]

Attachment: WIP: Bug 1907810 - use cargo vet with --no-all-features

By default cargo vet will enable all features on all crates, no
matter whether they are used or not. This is problematic where an
unvetted optional disabled dependency is included in the vetting
process, even though it isn't used.

Example:

• Crate A is used.
• Crate A has an optional dependency on crate B. The feature is
  disabled and thus crate B is not used.
• Crate B is not vetted.
• Vetting of crate A will fail as cargo vet will enable all features
  on crate A and thus include unvetted crate B.

Unless the user specifies otherwise, override the default, using
--no-all-features instead.

Comment 2

[Max Inden]

@nika and @glandium would you mind taking a look or redirecting me to the right person to talk to?

Comment 3

[Nika Layzell [:nika] (ni? for response)]

Cargo only enables features for crates within the local workspace, which is desirable. We have many features which can be turned on from our build system (https://searchfox.org/mozilla-central/rev/54a5c4f14f3eb514a29e0cebcb5a095144bcd450/toolkit/library/rust/shared/Cargo.toml#126-156), and we want to be vetting each of these features and any dependencies which they might pull in.

In the case of the bug you mention in comment 0, I think that the dependency is being pulled in as-expected. It appears that the crate test/http3server depends on neqo-udp, with the tokio feature enabled:

# TODO: depend on /mozilla not /mxinden
neqo-udp = { tag = "v0.7.9", git = "https://github.com/mozilla/neqo", default-features = false, features = ["tokio"] }

That crate is in the workspace dependency graph, so the feature is pulled in. I believe that feature is being enabled because it is being used, not because of --all-features.

Comment 4

[Max Inden]

Sorry for the confusion, I posted a simplified version above, ignoring the different vetting levels, i.e. safe-to-run and safe-to-deploy.

http3server imports neqo-udp with feature tokio. That is correct. Though http3server is a testing binary only. Thus, to my understanding needs onlysafe-to-runand thustokioonly needssafe-to-run`.

neqo_glue is used in Firefox directly. Thus it needs safe-to-deploy. But it does not depend on tokio.

Thus, tokio needs safe-to-run only. But cargo vet requires safe-to-deploy.

Does the above make sense?

Comment 5

[Nika Layzell [:nika] (ni? for response)]

I don't believe this patch will make any difference to that situation. Even if we disable --all-features, the situation with the tokio dependency having higher-than-expected audit requirements due to it being depended on via 2 different dependency paths will still be there. If you are seeing an improvement here, it's probably because disabling --all-features is causing some crates which need to be audited to no longer need to be audited. (that being said, running cargo metadata without --all-features on the patch you posted still shows a tokio dependency from neqo-udp).

Looking at the output from cargo metadata, it doesn't seem like cargo distinguishes between these two dependency paths when resolving. In the resolve.nodes list after the patch you linked above, there's a single "git+https://www.github.com/mozilla/neqo.git?branch=main#neqo-udp@0.7.9" node which declares as a dependency "registry+https://github.com/rust-lang/crates.io-index#tokio@1.29.1", and notes that the "tokio" feature has been enabled.

I'm guessing this is because cargo's resolver generally does unify. In the case where there were two dependency paths leading to the neqo-udp crate within a single build target, we would want to consider features enabled under the other path as well. There also doesn't appear to be any mechanism to run cargo metadata for only a single target and not the entire workspace, so we couldn't manually discover every crate which could be built and run dependency resolution for them independently.

I don't think there's really a way for cargo-vet to handle this situation without manually re-implementing cargo's feature resolution algorithms, which is something we've generally tried to avoid doing, because it's likely we'd not perfectly match cargo's behaviour, and re-implementing everything is pretty gross. In addition, if cargo ever changes how feature resolution is handled, things would break unless we kept up with implementing these new features-features.

I've filed https://github.com/mozilla/cargo-vet/issues/626 on cargo-vet about this, but I'm not sure there's much we can do about it.

Comment 6

[Mike Hommey [:glandium]]

While it is not done automatically at the moment, we also do unify features through the workspace-hack crate, which means that we /would/ end up with tokio in gkrust via neqo-udp eventually.

Comment 7

[Max Inden]

Thank you for the analysis!

I am new to Mozilla's dependency management process. How do you suggest we proceed with https://phabricator.services.mozilla.com/D212959?

Current cargo vet output:

➜  mozilla-unified git:(quic-rust-udp-io) ✗ ./mach cargo vet
[INFO] Using /home/mxinden/code/mozilla.org/mozilla-unified/.cargo/config.toml.
Vetting Failed!

4 unvetted dependencies:
  tokio:1.29.1 missing ["safe-to-deploy"]
  tracing:0.1.37 missing ["safe-to-deploy"]
  tracing-attributes:0.1.24 missing ["safe-to-deploy"]
  tracing-core:0.1.30 missing ["safe-to-deploy"]

recommended audits for safe-to-deploy:
    Command                                      Publisher   Used By                                   Audit Size
    cargo vet inspect tracing-attributes 0.1.24  hawkw       tracing                                   5025 lines
    cargo vet inspect tracing-core 0.1.30        hawkw       tracing                                   7032 lines
    cargo vet inspect tracing 0.1.34             hawkw       h2, warp, hyper, quinn-udp, and 2 others  11016 lines
    cargo vet inspect tokio 1.29.1               carllerche  h2, warp, hyper, neqo-bin, and 7 others   112764 lines
      NOTE: this project trusts Carl Lerche (carllerche) - consider cargo vet trust tokio carllerche

estimated audit backlog: 135837 lines

• tracing*
  • Is needed by quinn-udp, a dependency of neqo-udp, the core of https://bugzilla.mozilla.org/show_bug.cgi?id=1901292, a Rust wrapper around GSO, GRO, recvmmsg, ...
  • Is audited for safe-to-run.
  • Some audits are for previous versions. That said, I assume the diff is minimal.
  • tracing-attribute won't be needed in future versions.
  • Who would need to be involved in a safe-to-deploy audit of tracing*?
• tokio
  • Is only needed for http3server, i.e. safe-to-run. safe-to-deploy requirement is false, see above.
  • Is audited for safe-to-run via audit import from Google.
  • Authored by carllerche, where "this project trusts Carl Lerche (carllerche) - consider cargo vet trust tokio carllerche".
  • Can the safe-to-run audit be extended to a safe-to-deploy audit?
  • Worst case, I can split neqo-udp into two crates, duplicating its logic once with and once without tokio.

Again, thank you for your help!

Comment 8

[Mike Hommey [:glandium]]

It looks like from a quick glance at the neqo-udp source, that it should be possible to split the tokio specific parts into a separate crate. That would solve your problem and comment 6.

Comment 9

[Max Inden]

It looks like from a quick glance at the neqo-udp source, that it should be possible to split the tokio specific parts into a separate crate. That would solve your problem and comment 6.

Sounds good. https://github.com/mozilla/neqo/pull/1988 splits the tokio specific parts into a separate crate. Thus resolving the need for an audit of tokio.

---

This leaves us with the missing audit of tracing only.

Mike, since you did the safe-to-run audit for tracing, what would be involved in increasing this to a safe-to-deploy audit?

An alternative route would be to ask upstream quinn-udp to drop the dependency on tracing, or hide it behind a feature flag, falling back to log. Though that will likely delay https://bugzilla.mozilla.org/show_bug.cgi?id=1901292 significantly.

Comment 10

[Mike Hommey [:glandium]]

The tracing audit I did was limited to version updates. Bumping that to safe-to-deploy would be a little too much. It would have to go through a full audit.

Comment 11

[Max Inden]

Thanks Mike.

I proposed dropping tracing on upstream quinn-udp: https://github.com/quinn-rs/quinn/pull/1921

In case that pull request is not accepted, we will either need to fork quinn-udp or go through the full audit process for the tracing crate.

Comment 12

[Max Inden]

For the record, upstream quinn-udp accepted a refined patch: https://github.com/quinn-rs/quinn/pull/1923/.

Thus https://bugzilla.mozilla.org/show_bug.cgi?id=1901295 is no longer blocked on a tracing nor a tokio audit.

Thank you all for the help.

I assume we keep this Bug open to track the general issue of cargo vet requiring stronger vetting requirement for test-only dependency.