treewide: add knownVulnerabilities to old libxml2#421204
treewide: add knownVulnerabilities to old libxml2#421204alyssais merged 1 commit intoNixOS:masterfrom
Conversation
alyssais
added
the
1.severity: security
Users should be informed if they're using packages that are deliberately opting in to using old versions of dependencies with vulnerabilities that have otherwise been fixed in Nixpkgs.
alyssais
force-pushed
the
libxml2-knownVulnerabilities
branch
from
656abf4 to
ce31d25
Compare
nixpkgs-ci
bot
added
12.approvals: 1
| libxml2' = libxml2.overrideAttrs (oldAttrs: rec { | ||
| version = "2.13.8"; | ||
| src = fetchurl { | ||
| url = "mirror://gnome/sources/libxml2/${lib.versions.majorMinor version}/libxml2-${version}.tar.xz"; |
There was a problem hiding this comment.
Can also do this
| libxml2' = libxml2.overrideAttrs (oldAttrs: rec { | |
| version = "2.13.8"; | |
| src = fetchurl { | |
| url = "mirror://gnome/sources/libxml2/${lib.versions.majorMinor version}/libxml2-${version}.tar.xz"; | |
| libxml2' = libxml2.overrideAttrs (finalAttrs: oldAttrs: { | |
| version = "2.13.8"; | |
| src = fetchurl { | |
| url = "mirror://gnome/sources/libxml2/${lib.versions.majorMinor version}/libxml2-${finalAttrs.version}.tar.xz"; |
There was a problem hiding this comment.
I think that's orthogonal to this change, as it could have been done originally as well.
nixpkgs-ci
bot
added
12.approvals: 2
wolfgangwalther
left a comment
wolfgangwalther
left a comment
There was a problem hiding this comment.
Instead of replicating this in various places, it'd be easier to maintain with a conditional on the version in the libxml2 package's meta section itself.
It also makes it harder to opt out of it (accidentally or intentionally).
ryand56
added
12.approvals: 3+
|
What about vendored libxml2.so.2 like this? |
Should be investigated but not in scope for this PR. |
There are various ways we could unify this, and I don't want to delay telling users about the deliberate vulnerabilities on painting the rest of the bikeshed. |
|
@wolfgangwalther what do you want to happen here? I'm spending my time and energy cleaning up after a mistake here, I only have so much of that I can dedicate, and it's running pretty thin. Would you like to open a PR after this is merged to unify them? Why is it on me to do additional package cleanup work just because I volunteered to sort the unmarked vulnerabilities? |
|
I'm confused ( 😕 ), because "delaying" seems like a pretty weak argument. If quickness to deliver is the most important metric here, this could have been merged yesterday after the first two approvals already. I gave 👎 because it feels unfair to brush my comment off as bikeshedding. It's clearly not out of scope, because it's about the change you made. And it's also not some minor bikeshedding, because you're fixing something that is already more hacky than we'd like (all the libxml2 overrides) with even more duct tape. Note, that I did not "request changes", so I am not blocking anything. I'd be perfectly happy with a comment along the lines of "yeah, you're right, but I'm not up for it, so I'll proceed as-is". It's just the reasoning and the way your comment came across that I am unhappy with.
I'd like to strongly oppose this. At no time, I asked for neither additional work, nor package cleanup. I only talked about the change you made. You repeated the same |
Ah, that's my fault then. I think your comment was completely reasonable, I just don't think this should need to be blocked on it. I don't think it was bikeshedding to suggest unifying them, but I expected that any attempt to unify them would invite bikeshedding, because there were multiple ways for that to be done, and I don't think security markings should be help up for refactors.
None of this was clear, because all the only communication I had to work with was "👎". In the absence of more information, all I can get from that is that there's some sort of problem with the PR, and I don't understand what it is, which makes me nervous about merging. I'd urge strong caution about using the 👎 reaction for that reason — since it communicates so little except for a negative attitude, it's a recipe for conflict and misunderstanding. (IMO it was a mistake for GitHub to ever add it. It's a rare exception where 👎 can have a positive impact on a conversation.)
To me it wasn't about the change I made — two mistakes had been made here: one was adding the same package override three times, and the other was not listing the vulnerabilities, and I was fixing one of them. We had three instances of the same package before, and three after. Nothing about listing the vulnerabilities makes any future change any harder. But I would very much like to solve this problem, I'd just like to use my energy on a more systemic fix. That's what I'm trying to do in #421201. |
Yes, this was not intended, sorry about that. I got distracted immediately after reacting, before writing up an explanatory comment. When I came back - you had already seen and responded to it. |
|
Let me just say explicitly as well because I realise I didn't include the word in my previous message: I'm sorry for my communication failure here.
|
8 participants
Users should be informed if they're using packages that are deliberately opting in to using old versions of dependencies with vulnerabilities that have otherwise been fixed in Nixpkgs.
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.