Cargo: Allow publishing the reason for yanking?

Reading the recent discussion about mitigating the impact of changing the layout of network primitives it occurred to me that it could be useful to be able to state the reason for yanking when one is yanking. If we did this in structured format it'd be possible to issue warnings/errors for serious cases.

I imagine three fields: reason kind, summary, link to more info Reason kind would be one of pre-defined values:

  • Unspecified - crates that were published before this was introduced and didn't update the reason
  • Unsoud - either unsound API or invoking UB internally
  • Vulnerable - non-UB security vulnerability (broken crypto etc)
  • API break - accidental semver-breaking change
  • Bug - any other bug in the code that's not a vulnerability nor it's visible from the API

Summary is a short description (up to 100 chars?) like "Relies on internal layout of std types", link is an arbitrary URL that should contain longer explanation and/or discussion.

By default cargo would error if it knows a crate was yanked because of unsoundness or vulnerability, warn in case of bug or a new reason it doesn't know, be silent in case of other reasons.

It'd be possible to allow crates using a whitelist or a command-line argument. It should be also possible to change the yank reason at any time mainly to allow already published crates to have it added but also to allow fixing mistakes.

Interaction with cargo audit

The two main differences vs cargo audit are:

  • Being in cargo by default would provide this for people who don't know about cargo audit or can't install it
  • yank can only be issued by crate maintainer, cargo audit allows others to submit reports, so audit is more powerful but takes more time from their developers to review reports. Author reporting their own crate should not need a review.

Example invocation:

cargo yank --vers 1.2.3 --reason unsound --summary 'Relies on internal layout of std types' --link https://github.com/example/foo/issues/42 foo

If the crate is already yanked the metadata gets updated and cargo prints a message about it.

6 Likes

I think that generally changing/updating metadata (without having to publish a new version) is something that would be nice. But it might require a lot of implementation work, I guess.

Alternatively, maybe surfacing issues from the advisory repository should become part of Cargo?

5 Likes

Rather than needing new API surface in Crates.io for writing (as opposed to reading) the metadata, it could be stored in a Git repository to which crate owners could submit patches, as with RustSec. Of course, this could be slightly more work for the crate owner, and it likely would increase Crates.io's coupling to GitHub, which is not the direction I'd like to see it go, but it would preclude reinventing the wheels of storing this information in a transparent, changelogged manner.

Previous related RFC:

There are also a few previous discussion threads since that RFC was closed. I don’t recall much of the discussion that occurred, it’d probably be good for someone to see if there were specific negatives that would affect this more restrained idea too.

Crates.io already allows writing the yanked state, which is then updated in the index repo. Adding some more fields for this seems pretty trivial.

1 Like

Yanking doesn't have to be related to security. For example, accidental semver violation is a frequent reason for yanking.

I think it would be great to have. It's probably just a matter of making nice pull requests to crates-io and Cargo.

In my view, all of those reasons are gratuitous. They break the ecosystem for no good reason.

Let me reiterate: if the library uses the recommended semver format of dependencies, then you don't need any yanking, just push an updated library version. All dependents will automatically pick it up and use the latest update. If the dependents have locked your version in Cargo.lock, then yanking is useless. They will be able to pull and build your crate regardless of any yanking. So yanking does absolutely nothing good in the majority use cases, but breaks all of the more marginal and complicated use cases where you already are in hot waters. Maybe you're trying to do a git bisect and require building with old version. Maybe you are stuck on an outdated nightly compiler version, can't currently update it, and it doesn't build with new crate versions due to some features removed on stable. Maybe you are trying to test your crate with older or minimal crate dependencies. Maybe you are doing history or security research, and you need to build an outdated version of the library. All of these cases are broken by yanking, all of them don't benefit in the least from the reasons you have listed above.

I would want yanking old (e.g. older than a couple of weeks or months) versions of crates to become entirely impossible. This just gives the maintainers power to break the ecosystem for no good reason. People have already started depending on their crate version, and they can just yank them and break everyone's builds. A rogue maintainer can yank all versions of their crate, effectively crippling everyone depending on them (the Cargo.locked projects will be mostly unaffected, but any project without a lock will be screwed, most importantly any library).

Your proposal to add reasons would just legitimize that harmful practice of yanking because the maintainer has a fit or thinks they know their users' use cases better.

There should be only one reason for yanking: a messed-up release (e.g. releasing a broken API, a critical error, or something that isn't even meant for release). There should be a hard reasonable time limit on yanking for such reasons, and everything else should not be accepted.

1 Like

But if the library accidentally breaks semver, publishing a new version doesn't unbreak semver.

Quoting the semver spec's FAQ:

What do I do if I accidentally release a backwards incompatible change as a minor version?

As soon as you realize that you’ve broken the Semantic Versioning spec, fix the problem and release a new minor version that corrects the problem and restores backwards compatibility. Even under this circumstance, it is unacceptable to modify versioned releases. If it’s appropriate, document the offending version and inform your users of the problem so that they are aware of the offending version.

It is completely reasonable for "document the offending version and inform your users of the problem so that they are aware of the offending version" to include cargo-yanking the broken version. Cargo-yank is explicitly not modifying the existing release, just marking it as broken and excluding it from automatic version selection.

Supplying yank reasons in fact improves the situation over currently, where code authors can and do yank versions for being unfit for their advertised purpose. Specifically, because cargo generate-lockfile could be extended e.g. with --allow-yanked-reason to include yanked versions in automatic version resolution.

What you could request is that there is always a ^-compatible version for yanked versions. If downstream

the situation is exactly the same w.r.t. automatic version resolution as not yanking it. The only difference is that it is visible that the not-latest version is broken and should not be used unless strictly necessary. And when alternative version resolution schemes are used (e.g. -Zminimal-versions), yanking is a strong benefit to remove versions that are not semver compliant.

9 Likes

@8573 I definitely wouldn't want to burden anyone with merging PRs. The yank feature already performs writing, so I see no good reason to not extend it.

@afetisov firstly, yank already exists so the horse is out of the barn. That being said I do think it should have a switch to override safety, but it should be off by default. Rust generally tends to favor safety over convenience of writing.

All dependents will automatically pick it up and use the latest update.

This is not true, locked builds don't do that and we really need the reliability of it. Having a lint "hey this one is vulnerable" helps those who forget to update.

If you're worried that a rogue maintainer will yank every crate you shouldn't have chosen it in the first place and yanking is lesser evil than e.g. backdooring it.

1 Like

I test -Zminimal-versions because I think it is a better route for MSRV in the long-run because otherwise it creeps up as nightly features get adopted and dependencies bump their MSRV. Yanking known-broken versions ensures that this resolution doesn't grab known-broken versions.

6 Likes

This is good and dandy if you actually can grab a compatible version. Whatever tests you run, the #1 issue is to build the project, and with some people just blanket yanking all old versions this is often impossible.

Yes, few people do that, but it just so happens that they are usually the maintainers of the foundational crates used by everyone (looking at you, ring, but you're not the only one).

Blanket yanking certainly sounds egregious. CVEs and unsoundness are certainly causes for yanking though. For ring, it may be that most updates are CVE-related (I don't know; I moved over to smaller crates for my hashing needs myself).

1 Like

If I'm testing that my library is compatible with the API of old versions, then I absolutely don't care about CVEs. If I'm investigating an old issue or bisecting a bug, then I absolutely don't care about CVEs. And, really, if I'm developing a library, then I don't care about CVEs, they are the problem of an end-user binary application, and should be treated at that level.

That's what I'm talking about: someone's niche impractical ideas about what is a published artifact obstruct my entirely unrelated work, and I have no way to circumvent it.

On that point, I would really like cargo to have a flag --shut-up-and-just-pull-my-dependencies to ignore all yanking, but there is even less chance of having it added. Perhaps I should just make a fork.

1 Like

Interesting. I do. If an old series of versions has a streak of known CVEs, why would I care about API compatibility with it? Sure, maybe you don't use the parts of the crate that have the CVE, but with how complicated bill-of-materials is already, I don't know that shading things this finely is anywhere near the level of care. There are already have reports of organizations have started questioning SHA1 usage in Git just to avoid the extra work of "well, does this SHA1 usage matter?" and just preferring "we no longer ship code with this known problem".

To use an analogy: most are still using table saws to attack their security problems. Unit and end-to-end testing would be using chisels. Fuzzing would be some really coarse sandpaper. I suspect we have some time before we're doing using the fine-grained "CVE-not-relevant" proof sandpaper on our supply chains.

4 Likes

I had some specific suggestions around this idea and an integration with RustSec in this post:

Specifically I suggested associating structured metadata (i.e. TOML) with each yank event. Something like this:

reason="security"
description="MsQueue and SegQueue suffer from double-free"

[advisory]
id = "RUSTSEC-2018-0009"
3 Likes

Extendable toml looks good but I'd still like to have some blessed cargo switches for common things (reason, description, link)

The part you're missing is that this reasoning strongly depends on your actual use case. If you're deploying a binary to the end-user, by all means, be most conservative and deploy only known-good dependencies. But relying yanking won't help you with enforcing even minimal standards of quality in that regard. You need to actually search vulnerability databases, test extensively and audit the actual shipped code. CVE or not, most code has bugs, and a logic-only bug which doesn't get a CVE or yank may still cause a disaster in production.

On the other hand, all use cases which happen only on the dev environment (particularly on my machine) don't warrant that level of paranoia, but do benefit strongly from extra flexibility. It's the same as putting #![forbid(warnings)] at the top of your crate, it has no positive effect and is just a pain for your contributors. Actual quality must be enforced in a single place at the point of shipping, anything else is just inviting errors.

When I'm bisecting a bug, do I care about CVEs? No, absolutely not, I have a specific bug that I need to track down, and everything else is just a frustrating obstacle.

If I'm trying to update an old library, do I care about CVEs? No, I don't, I'll deal with those issues when the library is up to date, builds with no issues, and is actually subject to be used in production. In the moment I need to deal with much more important things, like broken and deprecated dependencies, changing APIs, code quality and making the damn thing do its job. I need to be able to run code analysis, I need to be able to run it, including under debuggers and sanitizers. Being unable to resolve the dependencies gives me a ton of headache, and adds nothing to the code quality.

Even more specifically, unless you pin all your dependencies to a specific version, it is impossible to guarantee anything with regards to the absence of bugs.

Arbitrary changes in your dependencies can interact in arbitrary and most unexpected ways, even in simple cases. Rust, with its powerful type system, is much better in that regard than many other languages, but it is nothing but hubris to assume that untested dependencies don't cause issues. You can usually rely on the explicit guarantees of the ecosystem (i.e. that semver keeps the API compatible, most of the time), but anything beyond that is a free for all.