mozilla / cargo-vet

supply-chain security for Rust
Apache License 2.0
651 stars 43 forks source link

Optional dependencies #537

Open cemoktra opened 1 year ago

cemoktra commented 1 year ago

Hi,

for a project i checked the workload i would have to do an audit and noticed that i should audit a crate (500k lines to audit) that comes in due to an optional/feature dependency. As the feature is not enabled, this crate is obviously not used.

It would be good if such crates would not appear in the list of suggested audits

TitanNano commented 12 months ago

Digging into cargo metatdata I figured out what is the issue here. Cargo does not care about optional sub dependencies and always returns all of them when generating the metadata.

With this minimal cargo file:

[package]
name = "dependency-test"
version = "0.1.0"
edition = "2021"

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[dependencies]
sqlx = { version = "0.7.1", features = ["postgres"], optional = true }

[features]
db = ["dep:sqlx"]

It's easily reproducible that the db feature enables all optional sub dependencies:

$ cargo metadata --features db | jq '.resolve.nodes | map(select(.deps[].name == "sqlx_mysql" or .deps[].name == "sqlx")) | map({ id: .id, features: .features, deps: .deps | map(select(.name == "sqlx_mysql")) })'
[
  {
    "id": "dependency-test 0.1.0 (path+file:///home/jovan/dependency-test)",
    "features": [
      "db"
    ],
    "deps": []
  },
  {
    "id": "sqlx 0.7.1 (registry+https://github.com/rust-lang/crates.io-index)",
    "features": [
      "any",
      "default",
      "json",
      "macros",
      "migrate",
      "postgres",
      "sqlx-macros",
      "sqlx-postgres"
    ],
    "deps": [
      {
        "name": "sqlx_mysql",
        "pkg": "sqlx-mysql 0.7.1 (registry+https://github.com/rust-lang/crates.io-index)",
        "dep_kinds": [
          {
            "kind": null,
            "target": null
          }
        ]
      }
    ]
  },
  {
    "id": "sqlx-macros-core 0.7.1 (registry+https://github.com/rust-lang/crates.io-index)",
    "features": [
      "default",
      "json",
      "migrate",
      "postgres",
      "sqlx-postgres"
    ],
    "deps": [
      {
        "name": "sqlx_mysql",
        "pkg": "sqlx-mysql 0.7.1 (registry+https://github.com/rust-lang/crates.io-index)",
        "dep_kinds": [
          {
            "kind": null,
            "target": null
          }
        ]
      }
    ]
  }
]

while removing the feature gets rid of anything sqlx related:

$ cargo metadata | jq '.resolve.nodes | map(select(.deps[].name == "sqlx_mysql" or .deps[].name == "sqlx")) | map({ id: .id, features: .features, deps: .deps | map(select(.name == "sqlx_mysql")) })'
[]

Unfortunately cargo metadata does not indicate at all which dependencies are being enabled by a feature. Looking at the data in .packages though it should be possible to only consider optional dependencies for a package, if the package has an enabled feature which contains dep:some-package.

// .resolve.nodes
{
  "id": "dependency-test 0.1.0 (path+file:///home/jovan/dependency-test)",
  "features": [
    "db",   // <-- enabled feature
    "sqlx"  // <-- enabled feature
  ],
  "deps": []
},

// .packages
{
    "name": "dependency-test",
    "version": "0.1.0",
    "id": "dependency-test 0.1.0 (path+file:///home/jovan/dependency-test)",
    "license": null,
    "license_file": null,
    "description": null,
    "source": null,
    "dependencies": [
// ... not relevant
    ],
    "targets": [
// ... not relevant
    ],
    "features": {
      "db": [      // <-- is currently enabled
        "sqlx"     // <-- does not have a dep: sub-feature, so not relevant
      ],
      "sqlx": [    // <-- is currently enabled
        "dep:sqlx" // <-- has a dep: sub-feature, this should mark the sqlx dependency as actually active.
      ]
    },
    "manifest_path": "/home/jovan/dependency-test/Cargo.toml",
    "metadata": null,
    "publish": null,
    "authors": [],
    "categories": [],
    "keywords": [],
    "readme": null,
    "repository": null,
    "homepage": null,
    "documentation": null,
    "edition": "2021",
    "links": null,
    "default_run": null,
    "rust_version": null
}
mystor commented 11 months ago

This appears to be caused by an issue with how cargo resolves dependencies in the presence of optional features being conditionally enabled in optional dependencies. If you have a series of crates like:

[package]
name = "a"

[dependencies]
b = { ..., features = ["feat"] }
[package]
name = "b"

[dependencies]
c = { ..., optional = true }

[features]
feat = ["c?/feature"]
[package]
name = "c"

[features]
feature = []

Then the crate c can never be a dependency of a, however it will be included in a's Cargo.lock, and will be listed as an enabled dependency edge within cargo metadata output. I think this might be a bug in cargo, as if you remove the c?/feature, then this doesn't happen.

I'd like to avoid cargo vet getting into the business of interpreting cargo's features or dependency chains, as that's a lot of complexity which would otherwise be unnecessary to add.

The best place to fix this would probably be in Cargo itself. I'm guessing that the crate is appearing as a dependency because the dependency needs to be resolved in order to validate that the named feature is present, and resolving the dependency has the side effect of adding it to the graph in this way, even if it is not used (similar to how dependencies for other platforms are added).

It appears that fixing this within cargo-vet would require us to manually re-resolve dependency and feature requirements from the crate's metadata definition, then match those resolved dependencies against the list of resolved dependencies produced by cargo metadata to find dependencies which should be discarded due to being unreachable.