Closed stefano-garzarella closed 10 months ago
cargo audit
hangs indefinitely, reported it here: https://github.com/rust-vmm/rust-vmm-container/pull/89#issuecomment-1771038400
Looking at https://crates.io/crates/cargo-audit they call cargo generate-lockfile
before cargo audit
so it looks like it is a prerequisite (since when, I don't know).
Indeed, locally it fixed the issue, so should we change https://github.com/rust-vmm/rust-vmm-ci/blob/9751aaa0d0706964b1d4a228509a86bc25ffc0e7/.buildkite/test_description.json#L74 prepending cargo generate-lockfile
or cargo build
?
cargo generate-lockfile
. Would committing Cargo.lock
break something else?
Looking at https://crates.io/crates/cargo-audit they call
cargo generate-lockfile
beforecargo audit
so it looks like it is a prerequisite (since when, I don't know).Indeed, locally it fixed the issue, so should we change
https://github.com/rust-vmm/rust-vmm-ci/blob/9751aaa0d0706964b1d4a228509a86bc25ffc0e7/.buildkite/test_description.json#L74 prepending
cargo generate-lockfile
orcargo build
?
Apparently since 4 years ago: https://github.com/rustsec/rustsec/blame/main/cargo-audit/README.md. I think we're running into https://github.com/rustsec/rustsec/pull/1032, which seems to be the commit at which cargo audit
started using the cargo lock (and I guess it does not account for the scenario where it does not exist?). So the generate-lockfile
fixing it makes sense to me, let's add it :)
cargo generate-lockfile
. Would committingCargo.lock
break something else?
The problem I see is that cargo generate-lockfile
also updates the dependencies, so for those crates that have Cargo.lock committed, we're going to audit not the versions we're actually using.
Well here are some solutions I can think of right now:
[ -e Cargo.lock ] || cargo generate-lockfile
, which I don't like because it's a workaround of Cargo.lock missingcargo generate-lockfile --offline
does not update dependencies?
cargo generate-lockfile
. Would committingCargo.lock
break something else?The problem I see is that
cargo generate-lockfile
also updates the dependencies, so for those crates that have Cargo.lock committed, we're going to audit not the versions we're actually using.
Mh, but "versions we're actually using" is not really a concept for a library target, only for a binary. You can build a library against any versions of its dependencies that are semver compatible. How exactly do we actually expect cargo audit to work here? Maybe "fail if any semver allowed version has a known vulnerability" would make sense (so we can prevent downstream user from using our crates together with vulnerable versions of our dependencies, but I don't think we should do this kind of policing - I know some other rust crates recently came under fire for similar things). But I don't think cargo audit does that. I feel like it can only give meaningful information to a binary target, where everything is pinned to a specific patch version, and we can only make sure that we always support the newest versions of our dependencies, to ensure downstream users can always upgrade should vulnerabilities be found :thinking:
@roypat I agree. The Rust Cargo team now recommends libraries pin dependencies by committing the lockfile and has some arguments for it: https://doc.rust-lang.org/nightly/cargo/faq.html#why-have-cargolock-in-version-control
@roypat so we should do something like [ -e Cargo.lock ] && cargo audit
?
@roypat so we should do something like
[ -e Cargo.lock ] && cargo audit
?
I'd be fine with that. When we start checking in Cargo.lock files (which I think we should, @epilys has linked some good arguments. Especially the whole "CI fails because some new version of external dependency got pulled in" thing caused us quite some pain in the past - heck, a different facet of that problem got us into the mess we're in right now!), it'll also start running on those again, for whatever infinitesimal gain that gives us, without us needing to modify the ci package again, so that's a plus. If we really want to main the questionable status quo of "pick random versions of dependencies and then run cargo audit on the result", we can add a || cargo generate-lockfile && cargo audit
to it (or simplify to [ ! -e Cargo.lock ] && cargo generate-lockfile; cargo audit
).
@roypat so we should do something like
[ -e Cargo.lock ] && cargo audit
?I'd be fine with that. When we start checking in Cargo.lock files (which I think we should, @epilys has linked some good arguments. Especially the whole "CI fails because some new version of external dependency got pulled in" thing caused us quite some pain in the past - heck, a different facet of that problem got us into the mess we're in right now!), it'll also start running on those again, for whatever infinitesimal gain that gives us, without us needing to modify the ci package again, so that's a plus. If we really want to main the questionable status quo of "pick random versions of dependencies and then run cargo audit on the result", we can add a
|| cargo generate-lockfile && cargo audit
to it (or simplify to[ ! -e Cargo.lock ] && cargo generate-lockfile; cargo audit
).
I also don't have a strong opinion, perhaps a little more toward the latter possibility (which @epilys also suggested). I found that cargo metadata
generates Cargo.lock if it's not there, or leave what's there, but it seems too hacky to use.
I'll open another PR for this and we can discuss it there. I'll rebase this on that one.
v2:
v3:
v4:
Summary of the PR
This new version contains alsa and pipewire libraries to build vhost-device-sound audio backends.
Requirements
Before submitting your PR, please make sure you addressed the following requirements:
git commit -s
), and the commit message has max 60 characters for the summary and max 75 characters for each description line.unsafe
code is properly documented.