rust-lang / cargo

The Rust package manager
https://doc.rust-lang.org/cargo
Apache License 2.0
12.29k stars 2.32k forks source link

Add flag to allow Cargo to create a lock file that depends on yanked crates #4225

Open retep998 opened 7 years ago

retep998 commented 7 years ago

Currently if you have a Cargo.lock that depends on yanked crates you are able to build the project just fine as the yanked crates still exist. However there is currently no way to create a Cargo.lock if the only versions of some dependency that satisfies version constraints are yanked, aside from creating a Cargo.lock by hand which is a really poor solution.

I propose a new flag that is added to Cargo commands capable of creating a Cargo.lock which will allow Cargo to add dependencies on yanked crates if it has no other option.

Inspired by https://users.rust-lang.org/t/crates-io-disable-yanking-crates-that-have-dependant-crates/11492

carols10cents commented 6 years ago

I'm mostly against this because it defeats the whole purpose of yanking (which could have been done because of malicious code or a security problem). I also think this could at least be prototyped in an external tool first, and we could get a better idea of how often this is actually needed and whether this is the solution to the problem that we want to bake into cargo.

Leaving this open for others' thoughts, though.

pietroalbini commented 6 years ago

Having a flag like this (even perma-unstable) would be really nice for Crater: it regenerates lockfiles every run, so if a crate depends on yanked ones it can't be built anymore.

briansmith commented 5 years ago

I'm mostly against this because it defeats the whole purpose of yanking (which could have been done because of malicious code or a security problem)

Consider the ring crate, version 0.12.x. Are there security problems with version 0.12 that are fixed in later versions? Nobody knows! Because nobody knows, it's not reasonable to leave that version un-yanked. But, yanking it breaks lots of stuff, even though that's not the intention.

When I say "Nobody knows!" I mean that! I have completely forgotten what was in ring 0.12.x compared to later versions, and I am the crate author! We can't be expected to keep track of security issues in old versions indefinitely.

sbditto85 commented 5 years ago

Speaking of ring 0.12.x i have a bin that uses biscuit 0.0.8 which depends on ring 0.12.x ... because of that i cannot add any other non-related crates (like hex which has no deps) because that would be creating a new lock file. We are currently working on updating deps etc, but it really leaves me high and dry trying to add a quick feature we need. It would be nice to be able to include ring 0.12.x in my cargo.toml file with a flag that says "yeah i know its yanked but I accept any consequences until i can upgrade"

carols10cents commented 5 years ago

@sbditto85 If you had a lockfile containing ring 0.12, you should be able to upgrade unrelated crates without affecting the locked dependency on ring, but there's currently a bug in cargo affecting resolution in a way that breaks this: https://github.com/rust-lang/cargo/issues/6609 A workaround until that gets fixed is to use your VCS to check out the changes to the lockfile having to do with ring and biscuit, and keep the changes to the crates you're adding, like hex.

birkenfeld commented 4 years ago

Another valid use case that came up on Reddit: allowing people to do git bisect.

dpc commented 4 years ago

Another use case: I am writting a fuzzer. I want to temporarily run it against a affected crate version to see if it would catch the problem.

dtolnay commented 2 years ago

Workaround for those landing on this thread needing to do this: I commented the following line:

https://github.com/rust-lang/cargo/blob/8412d308730f1932c4130d3b28ced0e22ecb2ddb/src/cargo/sources/registry/mod.rs#L790

then compiled Cargo using cargo build --release, and then was able to generate a working lockfile using /path/to/cargo/target/release/cargo generate-lockfile. Once the yanked crate is in the lockfile, any other Cargo will be able to build it and you no longer need the undermined build.

Lindenk commented 1 year ago

Imo it's insane we need to patch Cargo to build old projects. The notion of yanking packages on crates.io clearly needs revision

Security is secondary to backwards compatibility and working code

U007D commented 1 year ago

I am running into an issue with this as I type this. I am doing a security review for code at ${WORK} and cannot do an analysis because the build is failing due to dependencies of dependencies of dependencies (i.e. not code we wrote) having been yanked.

I care deeply about security (it's an essential part of my job), but I wanted to point out that it's a false dichotomy to suggest either we're secure by not supporting downloading of yanked crates (for any reason) or we throw caution to the wind by providing a mechanism to support this. I believe the reality is more nuanced.

One simple solution is to require a verbose flag along the lines of --i-understand-this-action-may-have-security-implications or some such to make using such a feature both unergonomic and the risk self-documenting.

I just thought I'd weigh in on this, as I still need a solution to this--it's blocking our analysis and hence our product release. I'm off to synthetically generate a Cargo.lock file as a workaround.

obi1kenobi commented 1 year ago

This would also be somewhat useful to cargo-semver-checks.

By far the easiest way to generate the rustdoc JSON data cargo-semver-checks needs is to declare a dependency on a specific version of a crate, and then ask for that dependency's rustdoc. This obviously requires being able to generate a lockfile including that specific version.

The security risk here is minimized (though not entirely eliminated) since cargo-semver-checks never runs the underlying crate, it just generates a machine-readable API description. Unless the crate included a vulnerability or exploit that could be triggered on the equivalent of a cargo check (which isn't common at all), this should be safe to do.

823984418 commented 1 year ago

Just for the sake of problem-solving, if someone still cannot build a project with missing Cargo.lock files in the future, they can consider doing so:

  1. Try building a project once to generate a Cargo.lock file
  2. Visit https://github.com/rust-lang/crates.io-index
  3. Index manually to find the corresponding dependencies
  4. Copy the cksum of required version
  5. Add the following content to the Cargo.lock file
  6. Try building again
[[package]]
name = {{name}}
version = {{vers}}
checksum = {{cksum}}

If all goes well, cargo will automatically fill in the gaps. Good luck!


The above does not contain any express or implied use of the yanked dependencies.

weihanglo commented 1 year ago

Triage:

Glad we have many use cases so far!

I believe this needs inputs from Cargo team to determine:

In terms of implementation, this shouldn't be too hard but still need a considerable time for discussions. So labelling as:

@rustbot label +E-medium +S-needs-team-input +A-yanked +A-lockfile

dan-da commented 10 months ago

I am presently trying to build a 4 year old crypto crate in order to bring it up to date and extend it. I am running into dependency hell with multiple yanked crates just trying to get an initial build working. Cargo is making my [volunteer] job much harder than it needs to be.

This is madness and I cannot believe anyone ever thought it a good idea to intentionally prevent cargo from building crates.

A warning that a crate has been deprecated/yanked would be fine and good. Or a flag to override the yank.

I saw in a reddit post that Microsoft intentionally did not permit yanking of libs in their .Net registry for exactly this reason, to avoid dependency hell.

I am sitting here mouth open in amazement that this situation has persisted to this day when people have been raising it as an issue since at least 2017.

Reddit discussion 4 years ago: Rust Crypto Developers: Please stop the yanking.

There are any number of solutions that would be better than, hey let's just break everyone's build that depends on this.

wow.

epage commented 10 months ago

Keep in mind that we have a lot of issues that exist for a long time. We are a small group of people with limited time trying to tackle a large set of problems. We also have Cargo.lock which should mitigate problems with yank and we've updated our guidance on Cargo.lock for libs.

btw https://internals.rust-lang.org/t/suggestion-cargo-yank-is-a-misfeature-and-should-be-deprecated-and-eventually-removed/18486 is also a relevant thread

We also have the alternatives of

ruifengx commented 7 months ago

Apparently, cargo does not actually require checksums in the lockfile to proceed. So as a quick workaround, there is no need to look up checksums in crates.io index, all we need to do is add the following in the lockfile:

[[package]]
name = "package-name"
version = "expected-version"

And run cargo build. After that, cargo will figure out everything. It still requires human intervention, but at least it is not that complicated. However, for automated scripts (e.g., git bisect for old projects which do not have their Cargo.lock checked in [^note]), patching cargo would probably be more convenient.

[^note]: It is already a fact that a lot of library crates do not have their Cargo.lock checked in. The new recommendation might help the future, but it will not fix the history.

dtolnay commented 6 months ago

Would someone be willing to write a third-party Cargo subcommand to serve this use case? I imagine something like this: suppose the crate yankedcrate version 3.3.3 is yanked but we need to depend on it. The caller would make sure yankedcrate is listed among the dependencies in their Cargo.toml with a version requirement that is compatible with 3.3.3, then run:

$ cargo give-me-yanked yankedcrate@3.3.3

(Syntax similar to cargo add.)

The subcommand would do the following:

  1. Find the right Cargo.lock using cargo locate-project --workspace
  2. Blindly append the following to Cargo.lock:
    [[package]]
    name = "yankedcrate"
    version = "3.3.3"
    source = "registry+https://github.com/rust-lang/crates.io-index"
  3. Run cargo tree >/dev/null

I have found that this sequence results in Cargo repairing the lockfile with all the correct checksum and dependencies for the yanked crate, most of the time, and does not require a modified build of cargo like https://github.com/rust-lang/cargo/issues/4225#issuecomment-958659507.

epage commented 6 months ago

FYI rust-lang/rfcs#3493 proposes shifting the filtering-out of pre-releases from semver::VersionReq to the resolver by treating them like yanked but with cargo update --precise being able to force the selection of a pre-release (cargo team seems to be leaning in favor of that RFC). Likely the trivial implementation would make cargo update --precise just work with yanked as well and we'd have to do extra work to avoid it. I wonder if we should just make --precise work with yanked now. It doesn't help when you have dependency trees that can only be satisfied by allowing yanked but its a first step. It also aligns with the approved-but-not-implemented #12425 which has allows --precise to force dependency versions to work even if changing Cargo.toml is needed.

dpc commented 6 months ago

Would someone be willing to write a third-party Cargo subcommand to serve this use case?

I don't it's a worthwhile endeavor given the manual procedure seems easy enough and situations where one needs it are rare.

While I completely understand the time is scarce, it might not be a priority, it might require technical changes, etc. I think this issue is on a more fundamental and higher level:

While the yanking system and cargo preventing people from accidentally using releases assumed to be bad/insecure/broken is awesome, at the end of the day users are not little babies and thus need to be assumed to have the ownership of their software and decisions they make.

Ideally cargo should have a way to introduce a dependency on a yanked crate that (like everything else) has a good UX instead of having to google workaround and hacking lock file. It doesn't need to make anyone less safe - quite the opposite - it's an opportunity to educate and warn the user, eg. by using appropriately named options --dangerous-allow-yanked (or something like it) and displaying some kind of a warning.

epage commented 6 months ago

We talked about this in the cargo team meeting

Motivations

The happy path should be to use unyanked packages. Using a yanked package should require users going out of their way. However, unlike the current workarounds, it should be discoverable.

Potential solutions

We suspect it is fine to make a change because one or more of

@alexcrichton we were interested in some historical input on this in case we are missing anything which would make us feel more confident in moving forward with this.

alexcrichton commented 6 months ago

I'd very much agree with all the bullets under your comment:

We suspect it is fine to make a change because one or more of

I think every one of those points is correct and strong motivation for extending the yank feature beyond the bare-bones MVP it represents today. Yanking was originally thought of "well we gotta have something" and never got around to actually filling it out. I think it'd be a great idea to have an opt-in way of depending on yanked items for all the reasons you mention.

epage commented 6 months ago

Thanks alex!

I've marked this as Accepted for the proposed solution of --precise implicitly allowing yanked but warning users when a yanked package is selected.

Notes for implementer:

Eh2406 commented 6 months ago

The one caution I'd give is that --precise isn't strictly a version (iirc it can also be a commit sha) and you likely don't know the Source of the selected package ahead of time which might be barriers to the naive approach of just putting it in the allowlist for yanked packages.

I think only a index dependency can be yanked. So if --precise is a sha, we do not need to worry about doing a yank check. That being said the implementation that comes to my mind is to change this line https://github.com/rust-lang/cargo/blob/69255bb10de7f74511b5cef900a9d102247b6029/src/cargo/sources/registry/mod.rs#L790 to add a || req.is_precise(). (And then add tests.)

weihanglo commented 5 months ago

Just posted a PR #13333, since I want this feature for myself 🤪

weihanglo commented 5 months ago

13333 has been available since the nightly-2024-02-06 toolchain. For example, you can use it like:

cargo +nightly -Zunstable-options update --precise 1.16.0 tokio

For those subscribing to the thread, we would appreciate receiving feedback from you. Myself plan to stabilize it after a short period if no bug is introduced.

obi1kenobi commented 5 months ago

I think the invocation you posted looks good, and I'm in favor of shipping it. It's possible it's not a complete solution, and I have a couple of questions. (Sorry I couldn't jump in sooner, I just got home from giving a talk at FOSDEM.)

To generate rustdoc JSON for a historical crate version, cargo-semver-checks generates a "placeholder project" whose Cargo.toml project contains a dependency line like crate-to-check = "=1.2.3". It then executes a cargo doc command, and that command implicitly generates a lockfile in the process. That lockfile generation currently fails if 1.2.3 is a yanked version of that crate.

My understanding is that with this new feature, we would run cargo -Zunstable-options update --precise 1.2.3 crate-to-check before our cargo doc command. Is this correct?

A few follow-up questions:

The latter question is important because sometimes the yanked crates are indirect dependencies, and c-s-c needs a way to say "all yanked dependencies are fine if they are the only valid solution" instead of opting into a single yanked crate version at a time.

epage commented 5 months ago

My understanding is that with this new feature, we would run cargo -Zunstable-options update --precise 1.2.3 crate-to-check before our cargo doc command. Is this correct?

Unsure if it will fail still. You might need to loosen the bounds so a different version gets picked and then run cargo update.

For your situation, if we made yanked packages generally accessible but not-preferred (like I propose for MSRV-incompatible), then that version requirement will work without any extra steps. I suspect we might go in this direction one day but the current implementation is a smaller step

c-s-c currently supports Rust 1.70+. Is there a good way to detect whether -Zunstable-options update --precise can update to a yanked version on the currently-installed Rust?

There is not

Does --precise give permission to use a yanked version specifically for the named crate, or also for other crates as necessary? For example, if crate-to-check v1.2.3 is yanked and depends on other-crate = "=0.1.2" that is also yanked, will cargo -Zunstable-options update --precise 1.2.3 crate-to-check succeed in generating a lockfile?

Only for the named crate. The alternative solution I mentioned earlier would allow these other cases to work.

obi1kenobi commented 5 months ago

Makes sense, thanks! I remain in favor of the feature as-is on current nightly. Based on the above, it doesn't currently solve the c-s-c use case and I'll continue keeping an eye for future developments.

Thanks again!

torhovland commented 2 months ago

It looks to me like this issue can be closed now.

epage commented 2 months ago

That depends. There are still cases where you can't tell cargo to depend on yanked crates.

torhovland commented 2 months ago

What do you think is needed in order for this issue to be closed, then?