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

Audit requirements may be overly strong when dealing with multiple root crates in a workspace #626

Open
mystor opened this issue Jul 15, 2024 · 2 comments

Comments

@mystor
Copy link
Collaborator

mystor commented Jul 15, 2024

This was noticed in https://bugzilla.mozilla.org/show_bug.cgi?id=1907810, specifically because of the changes in https://phabricator.services.mozilla.com/D212959#inline-1194604.

If we have multiple crates in the local workspace which have different audit requirements, it is possible for an overly strong requirement to be placed on an indirect dependency.

Specifically if we have a dependency graph like the following, where W1 and W2 are toplevel workspace crates, D1 and D2 are third-party dependencies, where D1 optionally depends on D2. W1 does not enable this dependency but W2 does. If W1 has stronger audit requirements than W2 does, those stronger requirements can end up applying to D2, as the two D1 nodes are unified when running cargo metadata to have the combined feature set.

W1[req:safe-to-deploy] -> D1[features:]
W2[req:safe-to-run] -> D1[features:D2] -> D2

(D2 should require "safe-to-run", but will instead require "safe-to-deploy")

Unfortunately, I don't think that the output of cargo metadata really provides a way to solve this ambiguity and get what the resolution would be for each workspace crate independently, such that we could treat the two D1 dependencies as separate "nodes", to not propagate the same audit requirements to their dependencies.

It might be possible if we didn't trust the dependency resolution done by cargo and implemented feature resolution ourselves, but that seems like it'd be both a lot of work, and likely to break as cargo updates and changes how feature resolution is handled.

@afranchuk
Copy link
Collaborator

afranchuk commented Jul 15, 2024

I've experimented a bit and it looks like using cargo tree --format "{p} {f}" --package CRATE will show the correct subset of features and thus dependent crates. Unfortunately, though, cargo tree doesn't support an output format more easily ingested by programs. It'd be cool if cargo metadata supported a --package flag (like many other commands do).

@mystor
Copy link
Collaborator Author

mystor commented Jul 15, 2024

Unfortunately it does look like if you run the cargo tree without a specific --package flag that it'll still unify features, as the in the example from that bug, the neqo-udp crate still has a tokio dependency even under the gkrust crate unless you explicitly scope to only the gkrust package.

Does seem like cargo metadata would need a --package flag and we'd need a reliable way to enumerate all packages within the workspace so that we could do separate metadata runs for each one. On top of that we'd also need to tweak the graph building for the resolver and such so that dependency lists and inherited audit requirements could be dependent on which package the dependency graph is coming from.

mxinden added a commit to mxinden/neqo that referenced this issue Jul 17, 2024
`neqo-udp` is used in Firefox. Firefox uses `cargo vet` to audit its
dependencies. `tokio`, the dependency of `neqo-udp`, is not audited as
`safe-to-deploy`. `cargo vet` will require `safe-to-deploy` for `tokio` even
when behind a feature flag.

To work around this limitation in `cargo vet`, drop `tokio` dependency in
`neqo-udp`, moving `tokio` specific logic into `neqo-bin`.

See details in:

- mozilla/cargo-vet#626
- https://bugzilla.mozilla.org/show_bug.cgi?id=1907810
- https://phabricator.services.mozilla.com/D212959#inline-1194604
mxinden added a commit to mxinden/neqo that referenced this issue Jul 17, 2024
`neqo-udp` is used in Firefox. Firefox uses `cargo vet` to audit its
dependencies. `tokio`, the dependency of `neqo-udp`, is not audited as
`safe-to-deploy`. `cargo vet` will require `safe-to-deploy` for `tokio` even
when behind a feature flag.

To work around this limitation in `cargo vet`, drop `tokio` dependency in
`neqo-udp`, moving `tokio` specific logic into `neqo-bin`.

See details in:

- mozilla/cargo-vet#626
- https://bugzilla.mozilla.org/show_bug.cgi?id=1907810
- https://phabricator.services.mozilla.com/D212959#inline-1194604
github-merge-queue bot pushed a commit to mozilla/neqo that referenced this issue Jul 17, 2024
* chore(neqo-udp): drop tokio dependency

`neqo-udp` is used in Firefox. Firefox uses `cargo vet` to audit its
dependencies. `tokio`, the dependency of `neqo-udp`, is not audited as
`safe-to-deploy`. `cargo vet` will require `safe-to-deploy` for `tokio` even
when behind a feature flag.

To work around this limitation in `cargo vet`, drop `tokio` dependency in
`neqo-udp`, moving `tokio` specific logic into `neqo-bin`.

See details in:

- mozilla/cargo-vet#626
- https://bugzilla.mozilla.org/show_bug.cgi?id=1907810
- https://phabricator.services.mozilla.com/D212959#inline-1194604

* Fix clippy & doc

@mxinden please double-check

---------

Co-authored-by: Lars Eggert <lars@eggert.org>
github-merge-queue bot pushed a commit to mozilla/neqo that referenced this issue Jul 17, 2024
* chore(neqo-udp): drop tokio dependency

`neqo-udp` is used in Firefox. Firefox uses `cargo vet` to audit its
dependencies. `tokio`, the dependency of `neqo-udp`, is not audited as
`safe-to-deploy`. `cargo vet` will require `safe-to-deploy` for `tokio` even
when behind a feature flag.

To work around this limitation in `cargo vet`, drop `tokio` dependency in
`neqo-udp`, moving `tokio` specific logic into `neqo-bin`.

See details in:

- mozilla/cargo-vet#626
- https://bugzilla.mozilla.org/show_bug.cgi?id=1907810
- https://phabricator.services.mozilla.com/D212959#inline-1194604

* Fix clippy & doc

@mxinden please double-check

---------

Co-authored-by: Lars Eggert <lars@eggert.org>
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

No branches or pull requests

2 participants