Add zlib 1.2.12 #81
Add zlib 1.2.12 #81
Conversation
|
@Wyverald @meteorcloudy I'm guessing that yanked_versions isn't implemented yet given that this is attempting to build zlib at 1.2.11 after yanking it? |
|
Ah this is because bcr_presubmit.py sees zlib 1.2.11 as having been changed and so tries to build it. Should I be leaving the build files in place for 1.2.11 when yanking it? |
|
as of right now, Bazel itself uses 1.2.11 (https://github.com/bazelbuild/bazel/tree/master/third_party/zlib) Our original vision of yanking is simply that new usages will be disallowed, but the module still remains in the registry for whoever had already been using this version. So what we need to implement in Bazel is the "disallow new usage" part. |
|
Ok - so I've updated the PR now to leave the 1.2.11 build files present to make bcr_presubmit.py happy. However, I have removed 1.2.11 from the versions list and this seems to work ok, packages with a dep on 1.2.11 are resolving to 1.2.12 from what I can see. |
That sounds strange. I'm pretty sure the "versions" field is not read by Bazel at the moment. |
Ah my bad - the repo I was testing it with had an explicit dep on 1.2.12. |
|
But as far as use cases for yanking are concerned that is exactly the behaviour I would like to be able to opt in to - choose the min version that hasn't been yanked. |
| } | ||
| ], | ||
| "versions": [ | ||
| "1.2.11" |
There was a problem hiding this comment.
Removing 1.2.11 will cause other modules that depend on this version to break, we should not do this.
There was a problem hiding this comment.
I'm conflicted here about agreeing with you. From a reproducibility standpoint, definitely, we should not remove existing versions of a package. However in this case, the advice from upstream is that 1.2.12 is a drop in replacement for 1.2.11 that fixes a number of security issues and that 1.2.11 should no longer be used. So with my security hat on I would not agree with this advice and would expect that a dependency on 1.2.11 should in fact resolve to 1.2.12 as this is the minimum version that is 'secure' and that as the module maintainer I have baked in this knowledge into the registry by removing the 1.2.11 package. I think the fact that the end user has bumped the registry commit to use a HEAD that includes this change should mean that they have consciously updated and so should expect that package resolution could have changed, including updates to packages for security fixes. I'm assuming that the current implementation of min version selection in bazel doesn't do this kind of resolution of selecting the minimum version that exists in the registry?
On the bazel slack I suggested this as a way of solving the security type of use case:
one way a yanking like mechanism could be implemented is to introduce a concept like python uses of having a constraints dependency; you could imagine a
bzl_constraint("zlib", min_version="1.2.11")type construct that sets a minimum version. You could then have a security policy module consisting only of bzl_constraint that packages can depend on so you can globally set min versions of packages. You could also have abzl_constraint("zlib", exclude_version="1.2.11")option that would turn off just one version of a particular package.
but I think that the logic in bzlmod should also be extended to do the kind of min version selection I suggested above i.e. a dep on version 1.2.11 should resolve to 1.2.12 or later if a specific version 1.2.11 does not exist in the registry without requiring a module to explicitly depend on 1.2.12 (otherwise a security update needs to review all packages that depend on itself and explicitly update those requirements without a constraint like mechanism I described above.
There was a problem hiding this comment.
And just to be clear here - I deliberately made this change of removing 1.2.11 from versions to invoke this discussion as I don't think the design space has been properly explored yet for deprecating / yanking versions (and the yanked_versions design may even be incorrect/incomplete as a result of this?).
There was a problem hiding this comment.
Thanks for invoking this discussion! I actually had the same hesitation and I agree this problem hasn't been studied throughly. I think this deserves a thread in https://groups.google.com/a/bazel.build/g/external-deps or a short design doc. I can do it and share my thoughts when I have more free time (probably next week), but feel free to start the conversation with the community first.
bzl_constraint("zlib", min_version="1.2.11")
This is actually bazel_dep but without giving visibility to this dependency. It definitely could be (part of) the solution.
There was a problem hiding this comment.
I think you should start a monthly, public engineering meeting for discussions like this. I have found that extremely useful in rules_pkg and rules_license for bringing together contributors and stake holders to refine requirements.
There was a problem hiding this comment.
Sorry for the delay here, I finally had time to start a discussion at #128
This yanks version 1.2.11 due to CVE-2018-25032
|
@meteorcloudy I've added 1.2.11 back into the list of versions on this PR. Given bazel doesn't currently do anything with the yanked_versions field I think its ok to leave it marked as yanked here to show the future intent to actually handle this. I think this PR is good to go though otherwise? |
5 participants
This yanks version 1.2.11 due to CVE-2018-25032