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

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

Open
retep998 opened this issue Jun 26, 2017 · 31 comments
Open
Labels
A-dependency-resolution Area: dependency resolution and the resolver A-lockfile Area: Cargo.lock issues A-yanked Area: yanked dependencies C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` E-medium Experience: Medium S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review

Comments

@retep998
Copy link
Member

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
Copy link
Member

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.

@carols10cents carols10cents added A-dependency-resolution Area: dependency resolution and the resolver C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` labels Aug 30, 2017
@pietroalbini
Copy link
Member

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
Copy link

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
Copy link

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
Copy link
Member

@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: #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
Copy link
Contributor

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

@dpc
Copy link
Contributor

dpc commented Oct 13, 2019

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
Copy link
Member

dtolnay commented Nov 3, 2021

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

&& (!s.is_yanked() || self.yanked_whitelist.contains(&s.package_id()))

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
Copy link

Lindenk commented Aug 29, 2022

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
Copy link

U007D commented Dec 20, 2022

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
Copy link
Member

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
Copy link

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 weihanglo added the S-needs-team-input Status: Needs input from team on whether/how to proceed. label Jun 8, 2023
@weihanglo
Copy link
Member

Triage:

Glad we have many use cases so far!

I believe this needs inputs from Cargo team to determine:

  • Whether we want to have such a mechanism to bypass yanking protection.
  • If so, how should the guardrail look like to prevent from easy copy-pasting for using yanked protection.

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

@rustbot rustbot added A-lockfile Area: Cargo.lock issues A-yanked Area: yanked dependencies E-medium Experience: Medium labels Jun 8, 2023
@dan-da
Copy link

dan-da commented Sep 8, 2023

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
Copy link
Contributor

epage commented Sep 8, 2023

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
Copy link

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 1), patching cargo would probably be more convenient.

Footnotes

  1. 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
Copy link
Member

dtolnay commented Dec 13, 2023

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 [email protected]

(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 #4225 (comment).

@epage
Copy link
Contributor

epage commented Dec 13, 2023

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
Copy link
Contributor

dpc commented Dec 13, 2023

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
Copy link
Contributor

epage commented Dec 19, 2023

We talked about this in the cargo team meeting

Motivations

  • Crater being able to re-generate lockfiles that depend on yanked dependencies (at least at one point, unsure if its still the case)
  • Projects building old versions of libraries before they checked in their lockfile
  • Package authors that aggressively yank and users need a short-term access to a yanked version
  • ... many other cases we haven't predicted

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

  • --precise <yanked> with a warning was discussed and this is our preferred option. --precise requires a full version number, so users are likely doing this with full knowledge. The warning helps make sure that is the case.
  • Alt: --precise <yanked> --yanked (name to be bikeshedded). This is inconsistent with our other proposals for --precise that say "trust the user" and those proposals generally have the extra flag be used without --precise (cargo update --breaking, cargo update --prelease).
  • Alt: Prompt the user. Adding a prompt to a command that generally doesn't have one is a pain for automation and an annoying stutter for users. Usually these are best when there are side effects that can't be undone. For example, it would have been great for cargo publish (or dry-run by default) but its too late to change that

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

  • Community norms have been established and we need less guard rails
  • The uses of cargo yank and the reasons people want access to yanked weren't originally predicted in the design
  • The original yanked feature was an MVP and needed extension

@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
Copy link
Member

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 epage added S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review and removed S-needs-team-input Status: Needs input from team on whether/how to proceed. labels Dec 19, 2023
@epage
Copy link
Contributor

epage commented Dec 19, 2023

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:

  • You are welcome to reach out on zulip or join office hours for advice. See also contrib docs
  • 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.
  • We can either insta-stable this which will make the bar for merging the PR higher or put it behind an unstable feature (-Zunstable-options?) which would allow iterating on this over several PRs and allow for people to test it before we hold an FCP

@Eh2406
Copy link
Contributor

Eh2406 commented Dec 20, 2023

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

&& (!s.is_yanked() || self.yanked_whitelist.contains(&s.package_id()))
to add a || req.is_precise(). (And then add tests.)

@weihanglo
Copy link
Member

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

@weihanglo
Copy link
Member

#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
Copy link
Member

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:

  • 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? Otherwise, the logic for generating the placeholder project could get quite complex, and we might just wait to use this feature until our oldest supported Rust was branched after nightly-2024-02-06.
  • 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?

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
Copy link
Contributor

epage commented Feb 6, 2024

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
Copy link
Member

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
Copy link
Contributor

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

@epage
Copy link
Contributor

epage commented May 6, 2024

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

@torhovland
Copy link
Contributor

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

bors added a commit that referenced this issue May 29, 2024
feat: stabilize `cargo update --precise <yanked>`

### What does this PR try to resolve?

Stabilize `cargo update --precise <yanked>`.

The cargo team has discussed the stabilization in the meeting today.
The interface of this is quite small and not as controversial as `--precise <pre-release>`,
since there is no version requirement operators involved.
We'd like to move forward and stabilize this part.

Note that `--precise <yanked>` allows using yanked version only for the specified package,
We leave the cascading allowing yanked versions (e.g. <#4225 (comment)>) as a future extension.

### How should we test and review this PR?

Check if any adjustment needed for warnings and CLI help text.

### Additional information

cc <#4225>.
bors added a commit that referenced this issue May 29, 2024
feat: stabilize `cargo update --precise <yanked>`

### What does this PR try to resolve?

Stabilize `cargo update --precise <yanked>`.

The cargo team has discussed the stabilization in the meeting today.
The interface of this is quite small and not as controversial as `--precise <pre-release>`,
since there is no version requirement operators involved.
We'd like to move forward and stabilize this part.

Note that `--precise <yanked>` allows using yanked version only for the specified package,
We leave the cascading allowing yanked versions (e.g. <#4225 (comment)>) as a future extension.

### How should we test and review this PR?

Check if any adjustment needed for warnings and CLI help text.

### Additional information

cc <#4225>.
bors added a commit that referenced this issue May 29, 2024
feat: stabilize `cargo update --precise <yanked>`

### What does this PR try to resolve?

Stabilize `cargo update --precise <yanked>`.

The cargo team has discussed the stabilization in the meeting today.
The interface of this is quite small and not as controversial as `--precise <pre-release>`,
since there is no version requirement operators involved.
We'd like to move forward and stabilize this part.

Note that `--precise <yanked>` allows using yanked version only for the specified package,
We leave the cascading allowing yanked versions (e.g. <#4225 (comment)>) as a future extension.

### How should we test and review this PR?

Check if any adjustment needed for warnings and CLI help text.

### Additional information

cc <#4225>.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-dependency-resolution Area: dependency resolution and the resolver A-lockfile Area: Cargo.lock issues A-yanked Area: yanked dependencies C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` E-medium Experience: Medium S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review
Projects
None yet
Development

No branches or pull requests