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 yanking guidance to the readme faq #104800

Merged
merged 4 commits into from
Apr 17, 2024

Conversation

LilithHafner
Copy link
Member

Continuation of #102636, taken over at Ian Butterworth's request. New PR so that I can accept review suggestions. cc @oxinabox @ericphanson @vchuravy @Seelengrab

IanButterworth and others added 2 commits April 12, 2024 09:46
Co-authored-by: Lilith Orion Hafner <lilithhafner@gmail.com>
Co-authored-by: Sukera <11753998+Seelengrab@users.noreply.github.com>
README.md Show resolved Hide resolved
README.md Outdated
Comment on lines 305 to 309
There is however, a special category of bugged releases that can not be resolved by having a patch release. These also need to be resolved by
yanking. That special category is when the compat bounds have been set too wide. i.e. say `v2.10.0` was released using a feature not on julia
`v1.6` but the compat entry for julia was not raised in the release. In this case releasing a `v2.10.1` with the corrected julia compat would
not solve the issue as on julia v1.6 Pkg would still resolve the broken `v2.10.0`, and as a minor bump, reverting the code changes would not
be valid in a patch bump.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this sort of too-wide compat be resolved by making a PR to the registry with the correct bounds instead of yanking? After all, the release is (a priori) good, just not for all originally specified versions.

@Seelengrab
Copy link
Contributor

Thanks for taking up the effort!

Copy link
Member

@ericphanson ericphanson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree about the primary purpose of yanking being to make it impossible to install a version. Deleting the version from the registry does that. Yanking has the special property that existing manifests will continue to work. Therefore, I think deleting the version should be used for actively exploited malicious activity.

I think yanking should be used for lesser situations (e.g. in the XZ builds that were yanked, at the time of yanking it was suspected that the backdoor was not present in our builds, so yanking was a less-disruptive first step), or for compat-related issues as detailed here.

BTW: pretty much an aside, but Julia 1.0 does not respect yanks (support was added in v1.1), which is maybe good to know.

@Seelengrab
Copy link
Contributor

Therefore, I think deleting the version should be used for actively exploited malicious activity.

Deleting also has the unintended sideeffect of making the registry (and by extension, registry maintainers/the community) forget that the version was already released previously and subsequently removed. Yes, that information is still technically available in the git history, but that's much less likely to be checked compared to an immediate "hey this was yanked previously".

On the other hand, what exactly is the upside of keeping known-insecure versions installable for existing manifests?

@LilithHafner
Copy link
Member Author

In my very limited experience, adjusting compat bounds on existing versions is pretty hard. But if that can be done easily and reliably then I think it should replace yanking for the second use case. Then Pkg could e.g. print a warning and prompt for confirmation when instantiating a yanked version (IMO there should always be a way to install yanked versions, even if only an UNSAFE_IGNORE_YANKS variable)

@ericphanson
Copy link
Member

Therefore, I think deleting the version should be used for actively exploited malicious activity.

Deleting also has the unintended sideeffect of making the registry (and by extension, registry maintainers/the community) forget that the version was already released previously and subsequently removed. Yes, that information is still technically available in the git history, but that's much less likely to be checked compared to an immediate "hey this was yanked previously".

Given that has never actually happened, that doesn't seem that important? And it could be solved many other ways. We could keep a markdown doc in General with a list of such versions if we ever find one.

On the other hand, what exactly is the upside of keeping known-insecure versions installable for existing manifests?

Huh? I am saying to not do that?


I think you want to use yanking for something other than what it is meant for. Yanking is a Pkg.jl feature that exists in order to make a version installable-from-a-manifest, but not resolvable. Like that is what is was made for. It has special support in Pkg to make this possible. The thing you want, to make something not installable, is 100% possible without special support in Pkg. Namely, to remove it from the registry. The reason yanking exists is because sometimes we want to make something not resolvable but without breaking reproducibility.

@LilithHafner
Copy link
Member Author

How does this sound? (up to definitions of "very bad" and "very very bad")

Compat bounds were too wide => PR General to retroactively adjust compat bounds
Very bad release, worse than any ordinary bug => Yank
Very very bad release e.g. actively exploited security vulnerability => Delete from registry

@ericphanson
Copy link
Member

That sounds reasonable to me. I think yanking is also ok for trying to address compat issues, given the registries compat format is undocumented, we have no tooling setup to adjust bounds that I know of, and we don’t have good CI checks to verify it is correct. IMO if we had some of those things we could prefer adjusting bounds over yanking, but given that we don’t, manually adjusting bounds is error prone and requires careful review, while yanking is straight forward.

@LilithHafner
Copy link
Member Author

Okay. Once #104849 is in better shape we can change the guidance to what I recommended above, and until then also recommend yanking for the too-wide compat bounds case.

@Seelengrab
Copy link
Contributor

Seelengrab commented Apr 13, 2024

Huh? I am saying to not do that?

Both ]add and ]instantiate are an act of installing packages. Instantiating old manifests that have vulnerable versions is IMO just as bad as adding that version to new manifests. I'm proposing we treat these two cases the same, in order to protect the users of the registry from unknowingly installing vulnerable versions, both through instantiation as well as adding. No one is going to actively check their old manifests for security vulnerabilities; thus, this should be something that's handled directly by the registry.

Very bad release, worse than any ordinary bug => Yank

I think this is not precise enough. What exactly is "worse than an ordinary bug"? In what situation would a package developer/registry maintainer choose a yank over a straight-up deletion of a version, if the yank still permits the chance that someone gets a horribly broken version?

Very very bad release e.g. actively exploited security vulnerability => Delete from registry

How are we going to keep record of previously deleted versions, to prevent double registrations? Are we actively going to check the git history of every package?


I know that we don't have tooling & documentation around the Registry.toml format and adjusting compat bounds. This is an opportunity to remedy that, since those are quite simply operations that occur during regular maintenance of the registry.

@LilithHafner
Copy link
Member Author

I put my suggestion, with revisions due to @ericphanson's observation that editing compat bounds is harder than yanking, into prose in the latest commit.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Co-authored-by: Eric Hanson <5846501+ericphanson@users.noreply.github.com>
@ericphanson
Copy link
Member

I think this is not precise enough. What exactly is "worse than an ordinary bug"?

I agree this is imprecise. But I think having something here is better than nothing, so let's merge this and if someone proposes more precise language in another PR we can consider that there.

In what situation would a package developer/registry maintainer choose a yank over a straight-up deletion of a version, if the yank still permits the chance that someone gets a horribly broken version?

From the perspective of the registry, it's basically "fine" to get horribly broken code. That is the ordinary course of business when using code/the registry, and there is an ordinary, regularly-exercised fix: update to a non-broken version. We would never delete a version due to the brokenness of the code. If it is doing the wrong thing, and we delete the bad version, how can you compare the old vs new to understand how much the bug affected your downstream application?

How are we going to keep record of previously deleted versions, to prevent double registrations? Are we actively going to check the git history of every package?

I think we can cross that bridge when we get to it. Hopefully not for a long time.

I know that we don't have tooling & documentation around the Registry.toml format and adjusting compat bounds. This is an opportunity to remedy that, since those are quite simply operations that occur during regular maintenance of the registry.

Is this more of an opportunity than any other time? The problem has been known for years. I'm not going to block merging this helpful advice which is fit for the current status quo in favor of waiting for something that may or may not happen.

@ericphanson ericphanson merged commit ce893bf into JuliaRegistries:master Apr 17, 2024
10 checks passed
@LilithHafner LilithHafner deleted the lh/ib/yank_guidance branch April 17, 2024 16:26
@LilithHafner
Copy link
Member Author

@StefanKarpinski, would you like us to reconsider this decision and/or revert this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants