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

{segger-jlink,nrfconnect,nrf-command-line-tools}: drop #353880

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

emilazy
Copy link
Member

@emilazy emilazy commented Nov 5, 2024

This is a proprietary package that depends on the vulnerability‐ridden Qt 4, and was removed along with Qt 4 in 2022. It was re‐added at the beginning of the year, but upstream is unwilling to upgrade their Qt version and as it is proprietary software we have no way to patch it on our end.

I think that it would be best not to ship this in 24.11. While it’s clearly useful software, we made the deliberate decision to remove Qt 4 seven years after it reached end of life, and since this software is seemingly doomed to eternal knownVulnerabilities I think that it does not meet our standards for inclusion. This would be better maintained in a third‐party repository, as suggested by the maintainer in #214195 (comment).

Reverts: #255185
See: #214195 (comment)

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

Depends on `segger-jlink`.

This reverts commit 9058bc4.
Depends on `segger-jlink`.

This reverts commit c2f2509.
This is a proprietary package that depends on the
vulnerability‐ridden Qt 4, and was removed along with Qt 4 in
2022. It was re‐added at the beginning of the year, but upstream
is unwilling to upgrade their Qt version and as it is proprietary
software we have no way to patch it on our end.

I think that it would be best not to ship this in 24.11. While it’s
clearly useful software, we made the deliberate decision to remove Qt 4
seven years after it reached end of life, and since this software is
seemingly doomed to eternal `knownVulnerabilities` I think that it does
not meet our standards for inclusion. This would be better maintained in
a third‐party repository, as suggested by the maintainer in
<NixOS#214195 (comment)>.

This reverts commit d5fd263.
@emilazy
Copy link
Member Author

emilazy commented Nov 5, 2024

Ah, I just saw #319342. @h7x4 I guess the headless variants of these tools do not require Qt 4, then? We could simply drop the bundled Qt 4 and only ship the headless versions, I think, especially if that means we can keep the CLI tools.

@teburd
Copy link
Contributor

teburd commented Nov 5, 2024

Please, stop removing these incredibly useful tools.

@StarGate01
Copy link
Member

StarGate01 commented Nov 5, 2024

I do not agree on the removal (yet). The QT4 "vulnerability" is opt-in, the user has to specify

config = {
              allowUnfree = true;
              segger-jlink.acceptLicense = true;
              permittedInsecurePackages = [
                "segger-jlink-qt4-796s"
              ];
            };

Which, in my opinion, is an adequate safeguard.

The Segger JLink tools and those which depend on it are industry-standard tools and required by engineers around the globe to do their job. I stand by my comment in #214195 (comment) , "It would be nice for NixOS to evolve from a project for ambitions hobbyists into a system which supports real-world software.".

Ideally, we would get Segger to upgrade their stack to a newer version of QT.

Also, re: headless versions - even the core JLinkExe needs QT4 to display a license prompt window.

@emilazy
Copy link
Member Author

emilazy commented Nov 5, 2024

I agree that they’re useful, but all software in the tree poses a maintenance burden and a potential risk to users, and we have to consider those factors as well; keeping around software with CVEs from 2009 and upstreams that don’t care about that isn’t conducive to maintaining a healthy and cohesive package set. If upstream update to a supported Qt that would be great and I’d happy to see the package return. In the meantime, I have no desire to stop people using this software, but I think a third‐party repository is the best way to handle the situation; Nix makes it easy to package and use out‐of‐tree software even if it doesn’t meet Nixpkgs standards for whatever reason.

Also, re: headless versions - even the core JLinkExe needs QT4 to display a license prompt window.

The segger-jlink-headless package doesn’t depend on Qt at all; are you saying it’s currently broken?

@StarGate01
Copy link
Member

StarGate01 commented Nov 5, 2024

segger-jlink-headless

My info might be outdated, the author of the headless variants is @h7x4 . They might also comment on this approach.

@teburd
Copy link
Contributor

teburd commented Nov 5, 2024

upstreams that don’t care about that isn’t conducive to maintaining a healthy and cohesive package set. If upstream update to a supported Qt that would be great and I’d happy to see the package return. In the meantime, I have no desire to stop people using this software, but I think a third‐party repository is the best way to handle the situation; Nix makes it easy to package and use out‐of‐tree software even if it doesn’t meet Nixpkgs standards for whatever reason.

What maintenance burden? @StarGate01 has done a great job of maintaining these packages. More energy seems to spent trying to remove this useful set of programs than letting maintenance of them falling to the people wanting to do the work.

I like NixOS, but this is exactly the sort of thing that pushes me away from it.

@emilazy
Copy link
Member Author

emilazy commented Nov 5, 2024

letting maintenance of them falling to the people wanting to do the work

In an ideal world, that’d be how it works. In practice, however, any package unavoidably places burdens upon the Nixpkgs maintainers:

  1. When a breaking change is made – say, a Nixpkgs interface needs changing, or a commonly‐used library is updated to an incompatible version or removed – the burden often falls on the person making the change to ensure that downstream packages continue working and fix them if necessary. (This also results in the phenomenon of packages that are effectively unmaintained and perhaps unused but are kept going by uninterested maintainers who are effectively obligated to tend to them as part of tree‐wide changes like Nixpkgs interface improvements or compiler upgrades. I’m not saying that’s what’s happening here, of course, but I think it’s helpful behind‐the-scenes perspective to counter the impression that packages don’t place any burden on anyone but their direct maintainers, and that packages just keep working without having work put into them.)

  2. Users who have problems with packages will report problems to Nixpkgs support channels and GitHub issues and this often results in unrelated maintainers having to triage the problem to the package maintainers.

The benefit of a third‐party repository is that it’s clear where responsibility for dealing with upstream Nixpkgs changes lies and it’s clear where users should go for support, and it avoids giving the impression that we as a project can support Qt 4 software and might accept more of it, when we decided explicitly to remove it and everything using it after it had been end‐of‐life for seven years. In general, we are pretty permissive about our package inclusion standards, but we already don’t have enough resources to adequately maintain the package set we have, and we have to draw a line somewhere; packages that rely on end‐of‐life versions of libraries that are subject to severe known security vulnerabilities and have no clear path away from that state are one of the lines we draw. When that overlaps with software that is important and useful, distributing the effort and making it clear what’s official is the recourse we have. The usual disadvantages of moving things out‐of‐tree like losing automatic cached binary builds don’t apply here, since we don’t build non‐free and insecure packages anyway.

That said, I’d certainly like to hear about the headless variant, since if it would be possible to keep the CLI tools in‐tree and only the things that actually depend on Qt 4 need moving out that would be better all around.

Copy link
Contributor

@SigmaSquadron SigmaSquadron left a comment

Choose a reason for hiding this comment

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

Shipping 24.11 with Qt 4 of all things would be terrible. As mentioned on Matrix, Nixpkgs should not be expected to maintain software from upstreams that are clearly uninterested in shipping an up-to-date product.

I do believe separating this into a third-party repository would not only be a net positive for NixOS, but also to users of this package, as the current UX requires you to allow unfree and insecure packages, which is not something direct consumers of Nixpkgs should need to do.

Copy link
Contributor

Choose a reason for hiding this comment

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

The other files in this directory should also be deleted.

@SigmaSquadron SigmaSquadron added 12.approvals: 1 This PR was reviewed and approved by one reputable person 1.severity: security Issues which raise a security issue, or PRs that fix one labels Nov 5, 2024
@SigmaSquadron
Copy link
Contributor

SigmaSquadron commented Nov 5, 2024

I like NixOS, but this is exactly the sort of thing that pushes me away from it.

I would like to note that NixOS would never prevent you from using this sort of software; it'd be trivial to add a flake or simply pull in a third-party derivation as a tarball. Nixpkgs, on the other hand, should have a bare minimum entry bar for packages, and packages that actively implement security vulnerabilities shouldn't make the cut - the top-level is supposed to be a curated package set, after all.

It's also important to remember that every other distribution has a much more restrictive set of rules for acceptable packages. The fact that this existed for so long in Nixpkgs while other distributions would have deleted it is quite alarming.

@h7x4
Copy link
Member

h7x4 commented Nov 5, 2024

Hello, thanks for bringing this up. Sorry about all the surprise backlash 😅

I'm also a bit conflicted about dropping these packages. While it is using QT4 with ugly CVEs, I do believe we are doing an okay enough job at both warning the user about the potential risks and at maintaining the package. This piece of software is probably used by tens of thousands of developers every day at this point, even more so in CI. Removing it would lose a significant selling point of nixpkgs/NixOS for the entirety of the embedded ecosystem. A lot of people depend on this being available to get their work done, and NixOS would be a no-go if the package is not available. With the added complexity and discoverability issues that comes with out-of-tree packaging, not to mention having to trust the owner of the repo, I think it would be a dealbreaker for many new people that are still just considering NixOS.

I'm happy to make the warnings even more severe and annoying for both for users and nixpkgs developers. Maybe also make a big capital comment telling people not to go include QT4 packages for any reason, and note why it's a special exception. But I do not think the lessened security risk and maintenance cost that comes with the removal outweighs the amount of people who use this package on a daily basis.

letting maintenance of them falling to the people wanting to do the work

In an ideal world, that’d be how it works. In practice, however, any package unavoidably places burdens upon the Nixpkgs maintainers

I realize maintenance isn't as easy as "if the people in the maintainer list upgrades it every once in a while, it should be fine!", but the package currently has enough maintainers that at least one of us should be able to address any concerns in PRs that might affect the package. The ownership issue is prevalent in the entirety of nixpkgs, and I don't think it's fair to drop the package on this basis. The package is not particularly more expensive in terms of maintenance cost compared to any other package, especially with active maintainers.

Also, re: headless versions - even the core JLinkExe needs QT4 to display a license prompt window.

The segger-jlink-headless package doesn’t depend on Qt at all; are you saying it’s currently broken?

JLinkExe, also known as J-Link Commander, is an interactive CLI program only meant for interactive use. It is not part of segger-jlink-headless, which was split off only so we can link against libjlinkarm.so more easily. Even if the QT-dependent variant is removed, this should still be fine (i.e. nrfconnect and nrf-command-line-tools shouldn't need to be removed).

[...] I think that it does not meet our standards for inclusion.

[...] doesn’t meet Nixpkgs standards for whatever reason.

Do you mind if I ask about this? When the package was re-added I was under the impression that the package fully met nixpkgs standards just by being properly marked as insecure and proprietary. If we are to disallow insecure software to exist within the tree, I really think it should be added to https://github.com/NixOS/nixpkgs/blob/master/pkgs/README.md#quick-start-to-adding-a-package or somewhere else visible. Possibly even remove the possibility of marking a package as insecure in favor of dropping it, to ensure no more people make the same misunderstanding.

I like NixOS, but this is exactly the sort of thing that pushes me away from it.

Please don't use such an inflammatory tone. Discussion is not sustainable with strong negative emotions.

@SigmaSquadron
Copy link
Contributor

SigmaSquadron commented Nov 5, 2024

I'm happy to make the warnings even more severe and annoying for both for users and nixpkgs developers. Maybe also make a big capital comment telling people not to go include QT4 packages for any reason, and note why it's a special exception. But I do not think the lessened security risk and maintenance cost that comes with the removal outweighs the amount of people who use this package on a daily basis.

The ideal solution is to do the exact opposite: users should have as little trouble as possible installing the packages they need, but Nixpkgs forces unfree and insecure packages to have gates, thus making a third-party repo with these warnings disabled an appealing option, despite the lower discoverability.

It could also be argued that Nixpkgs' discoverability isn't as good as it's claimed to be, considering this package was left alone for months before someone noticed that it contains Qt 4. There are plenty of packages in the tree that are just forgotten.

@SigmaSquadron

This comment was marked as duplicate.

@h7x4
Copy link
Member

h7x4 commented Nov 5, 2024

The ideal solution is to do the exact opposite: users should have as little trouble as possible installing the packages they need, but Nixpkgs forces unfree and insecure packages to have gates, thus making a third-party repo with these warnings disabled an appealing option, despite the lower discoverability.

This doesn't make much sense to me. I value discoverability over ease of use. This ideal solution is just as subjectively ideal as the current implementation. We would also still need to do something in the third-party repo to ensure the user explicitly accepts the license, as the user needs to be aware of it before downloading the software.

It could also be argued that Nixpkgs' discoverability isn't as good as it's claimed to be, considering this package was left alone for months before someone noticed that it contains Qt 4. There are plenty of packages in the tree that are just forgotten.

Several people already knew the package existed, but deemed it acceptable. I don't think it's an issues with discoverability in this case, rather with guideline clarity.

@SigmaSquadron
Copy link
Contributor

I value discoverability over ease of use. This ideal solution is just as subjectively ideal as the current implementation.

Fair enough! My main fear is that including "UX exceptions" such as SEGGER software in Nixpkgs could cause users to get the wrong idea of how Nixpkgs should be used. New users would be more inclined to enable broken or vulnerable software system-wide if they saw that there is actively maintained software gated behind the warnings, thus rendering them useless.

@h7x4
Copy link
Member

h7x4 commented Nov 5, 2024

I can see where you're coming from. As I mentioned earlier, I would be happy to try to work out some warnings for nixpkgs developers not to copy from this example, as well as more severe warnings for the user regarding both legality and security issues. I can't really think of anything better than eval warnings and comments in the file for now, but I'm open for ideas. An update to the README regarding insecure packages would be good as well.

The current system for accepting the license is also not really optimal. The reason behind the license-check-in-config thing is that unlike many other pieces of proprietary software that lets you accept the license when you boot the program, this requires you to read and accept the license before you download the program. We could have used the requireFile approach, but since there is no username and password involved this seems (to me) like an better middleground. Even if the QT4 dependent part is removed, this part might warrant a warning comment not to copy the pattern just for any reason as well.

@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Nov 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.severity: security Issues which raise a security issue, or PRs that fix one 2.status: merge conflict This PR has merge conflicts with the target branch 6.topic: policy discussion 12.approvals: 1 This PR was reviewed and approved by one reputable person
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants