nheko: clean up #341831

rnhmjoj · 2024-09-14T13:27:40Z

Description of changes

Clean up nheko and related packages. ~~Most importantly, fix the build failure caused by the olm deprecation (#338006).~~

Things done

Add a 👍 reaction to pull requests you find important.

cc: @emilazy

Ma27 · 2024-09-14T14:38:46Z

pkgs/by-name/nh/nheko/package.nix

+      lmdb
+      mtxclient
+      nlohmann_json
+      # See discussion at https://github.com/Nheko-Reborn/nheko/issues/1786


If I read this correctly, the author considers the entire issue to not be a big issue.
This contradicts the decision we took, i.e. marking olm as insecure.

I'm not sure if we want to revert this on a per-package basis given on what upstream's opinion on the olm topic is.

cc @emilazy @NixOS/security for opinions.

Well, I think it makes sense to have that warning:

olm is not used exclusively for Matrix: though unlikely, other application may be vulnerable.

the library is now unmaintained upstream.

The referenced issue sounds a lot like wishful thinking and hoping for a great future, where some kind soul forks and maintains libolm. Or were you referencing something specific in that issue?

I don't think it is reasonable to override knownVulnerabilities, unless the upstream says they are not affected by the issues that affect libolm.

unless the upstream says they are not affected by the issues that affect libolm.

I'm referencing this comment in particular: Nheko-Reborn/nheko#1786 (comment) (deepbluev7 is one of the main dev of Nheko). They are saying it's ok for their use case, so I think it's stupid to mark it as unsafe on the sole basis of a CVE about some theoretical attack.

More importantly, this decision broke an application that people rely for communications on a daily basis without any notice. Worse, it results in an obscure error message that doesn't even refer to Nheko when updating your system.

It doesn't break anything to provide a detailed and balanced warning that specifically refers to the library's use in Matrix, points to upstream advisories, and tells the user exactly how to override it. The advisory was discussed extensively on the original PR. There's no argument for overriding this individually at the client level and it's hard not to see this as an inappropriate attempt to revert the hard-won consensus on that PR through the back door.

To be clear: it was the express and specific intent to show this advisory to people using Matrix clients that rely on this library with known weaknesses and an upstream expressing that there will be no maintenance or fixes and that the library should not be used. The display of it to users of those clients is the point, not an unintended side effect. Nothing is broken about keeping users informed and the ability to override the warning and the choice to leave that decision to end users is deliberate.

For context and with extensive discussion: #334638

It doesn't break anything to provide a detailed and balanced warning that specifically refers to the library's use in Matrix, points to upstream advisories, and tells the user exactly how to override it.

Yes it does, add Nheko to your system and try to rebuild it: it fails. The message is bad because it does not refers explicitely to which package is causing it (this is a more general UX problem in NixOS, though). It is also borderline incomprehensible for the average user (you need to know that your chat uses a protocol called Matrix, that Matrix uses an end-to-end encryption thing called olm, and you need to know that a side-channel is a bad thing).

I'm not saying this because I'm angry with you or anything: I just had friends asking me what the error meant and what to do.

you need to know that your chat uses a protocol called Matrix, that Matrix uses an end-to-end encryption thing called olm, and you need to know that a side-channel is a bad thing

If you're that much of a non-technical user, you shouldn't nixos-rebuild switch a system.

Using NixOS does not imply knowledge of the internals of a chat program or cryptography.

rnhmjoj · 2024-09-14T19:22:54Z

Removed the controversial overrides and added an exception in pkgs/top-level/release-lib.nix.
I guess this is what @emilazy is proposing we should be doing instead.

emilazy · 2024-09-15T13:04:16Z

I believe the Hydra override may also need to go in release-python.nix, although I’m not sure. I think we should split it into another PR, although I’ll emphasize again that I’m not at all opposed to it myself. (I just think that it might require more discussion from the security team and release managers than the rest of this PR, since AFAICT it’s applied very sparingly, and I wouldn’t want this otherwise‐uncontroversial PR to be held up by that, especially since it’s not specific to Nheko; see #233024, #273702, and #322068 for the last (and only?) time I know it was used.)

About the vulnerability message: I’d be open to ways to make it more accessible and less technical, but it is a difficult situation. The issues with libolm are highly controversial, with multiple security researchers, the Matrix.org libolm upstream, client developers, and Matrix fans all having wildly different and contradictory views about the topic. A lot of that was discussed in the original PR.

It’s been very unpleasant to deal with as a downstream, where my priority is making sure users are informed and warned about potential security vulnerabilities and maintenance issues in libraries they may be depending on for security and privacy, and it took a lot of work to write a message that was neutral and pointed to various resources on the topic, including the CVEs issued for the flaws, extensive descriptions of the problems in the code, and statements from upstream. I think we reached about as much consensus on it as any action on this topic could hope to gather.

The stress of dealing with remediating the situation and trying to find a way forward that would satisfy a large subset of parties, while the PR was being canvassed beyond the NixOS community, may have made me more snappy than I should have been in response to this one, especially if you hadn’t seen the full context of the prior discussion, so I’m sorry for that.

If we could make the message less technical and easier to understand for end users, I would appreciate and support that. But I found it very difficult to do more than neutrally describe a nuts‐and‐bolts overview, because interpreting it further risks making editorial statements on the matter that are inevitably going to be even more controversial than the message already is.

Hopefully the client ecosystem will move to vodozemac sooner rather than later and we won’t have to worry about it any more…

rnhmjoj · 2024-09-15T18:07:16Z

I believe the Hydra override may also need to go in release-python.nix, although I’m not sure.

There is at least some olm bindings for python, so yes it's worth looking into.

I think we should split it into another PR, [...] and I wouldn’t want this otherwise‐uncontroversial PR to be held up by that

Uhm, there's not much point in having these minor changes if Nheko remains quite inaccessible. I think they can wait.

although I’ll emphasize again that I’m not at all opposed to it myself.

Ok, understood.

About the vulnerability message: I’d be open to ways to make it more accessible and less technical, but it is a difficult situation.

I wouldn't change the message now, it's probably too late and we risk creating more confusion.

It’s been very unpleasant to deal with as a downstream, where my priority is making sure users are informed and warned about potential security vulnerabilities and maintenance issues in libraries they may be depending on for security and privacy

Since the issue is very technical, I would have preferred if the security team had made a stronger decision: either disregarding the vulnerability reports or marking individual packages as unsafe. But if even the "experts" are not agreeing on this issue, I can't blame you.

may have made me more snappy than I should have been in response to this one, especially if you hadn’t seen the full context of the prior discussion, so I’m sorry for that.

I understand, no problem.

Hopefully the client ecosystem will move to vodozemac sooner rather than later and we won’t have to worry about it any more…

Yes, I think most already did, unfortunately that's not the case for Nheko.

fabianhjr

Change review to neutral comment, didn't read all the previous threads before reviewing/commenting.

rnhmjoj · 2024-10-07T06:23:10Z

pkgs/top-level/release-lib.nix

@@ -34,7 +34,20 @@ let
    systems
    ;

-  pkgs = packageSet (recursiveUpdate { inherit system; config.allowUnsupportedSystem = true; } nixpkgsArgs);
+  allNixpkgsArgs = lib.recursiveUpdate


This should cover all the release-*.nix files providing their own nixpkgsArgs.

emilazy · 2024-10-11T04:04:32Z

I think the Hydra commit should go in its own PR so it can be reviewed separately by the security and release teams rather than being folded into a clean‐up. It’s also not totally clear to me where the allowUnfree and inHydra settings are coming from. The rest of the PR LGTM.

emilazy · 2024-10-11T04:04:40Z

Gah, mispress, sorry.

rnhmjoj · 2024-10-11T06:27:59Z

I think the Hydra commit should go in its own PR

All right, I'll create another PR.

It’s also not totally clear to me where the allowUnfree and inHydra settings are coming from. The rest of the PR LGTM.

It was already in the default nixpkgsArgs ? { config = { allowUnfree = false; inHydra = true; }; }.

rnhmjoj · 2024-10-11T10:45:06Z

I've dropped the hydra commit from this PR. See #347899.

rnhmjoj requested review from fpletz, Ekleog and pstn September 14, 2024 13:27

ofborg bot added 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 labels Sep 14, 2024

Ma27 reviewed Sep 14, 2024

View reviewed changes

rnhmjoj force-pushed the pr-nheko branch from b3b58c4 to 6d1a14e Compare September 14, 2024 19:21

emilazy mentioned this pull request Sep 15, 2024

olm: update vulnerability description #338006

Merged

13 tasks

fabianhjr reviewed Sep 16, 2024

View reviewed changes

rnhmjoj force-pushed the pr-nheko branch 2 times, most recently from 739055a to f716d5c Compare October 7, 2024 06:21

rnhmjoj commented Oct 7, 2024

View reviewed changes

rnhmjoj force-pushed the pr-nheko branch from f716d5c to b802cf4 Compare October 7, 2024 06:24

rnhmjoj requested a review from emilazy October 10, 2024 10:30

emilazy closed this Oct 11, 2024

emilazy reopened this Oct 11, 2024

rnhmjoj added 6 commits October 11, 2024 12:40

coeurl: 0.3.0 -> 0.3.1

a9fc9e8

coeurl: move to by-name and reformat

9ed27f0

nheko: add rnhmjoj as maintainer

f69e612

mtxclient: add rnhmjoj as maintainer

9f6acae

mtxclient: move to by-name and reformat

1b016ab

nheko: move to by-name and reformat

4312e5a

rnhmjoj force-pushed the pr-nheko branch from b802cf4 to 4312e5a Compare October 11, 2024 10:40

rnhmjoj mentioned this pull request Oct 11, 2024

release-lib: add exception to build olm on hydra #347899

Merged

13 tasks

emilazy approved these changes Oct 11, 2024

View reviewed changes

emilazy merged commit 8d4e086 into NixOS:master Oct 11, 2024
32 of 33 checks passed

rnhmjoj deleted the pr-nheko branch November 29, 2024 10:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nheko: clean up #341831

nheko: clean up #341831

rnhmjoj commented Sep 14, 2024 •

edited

Loading

Ma27 Sep 14, 2024

rnhmjoj Sep 14, 2024 •

edited

Loading

mweinelt Sep 14, 2024

rnhmjoj Sep 14, 2024 •

edited

Loading

emilazy Sep 14, 2024

emilazy Sep 14, 2024

emilazy Sep 14, 2024

rnhmjoj Sep 14, 2024 •

edited

Loading

Ma27 Sep 14, 2024

rnhmjoj Sep 14, 2024

rnhmjoj commented Sep 14, 2024

emilazy commented Sep 15, 2024 •

edited

Loading

rnhmjoj commented Sep 15, 2024

fabianhjr left a comment •

edited

Loading

rnhmjoj Oct 7, 2024

emilazy commented Oct 11, 2024 •

edited

Loading

emilazy commented Oct 11, 2024

rnhmjoj commented Oct 11, 2024

rnhmjoj commented Oct 11, 2024 •

edited

Loading

nheko: clean up #341831

nheko: clean up #341831

Conversation

rnhmjoj commented Sep 14, 2024 • edited Loading

Description of changes

Things done

Choose a reason for hiding this comment

rnhmjoj Sep 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnhmjoj Sep 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnhmjoj Sep 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnhmjoj commented Sep 14, 2024

emilazy commented Sep 15, 2024 • edited Loading

rnhmjoj commented Sep 15, 2024

fabianhjr left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emilazy commented Oct 11, 2024 • edited Loading

emilazy commented Oct 11, 2024

rnhmjoj commented Oct 11, 2024

rnhmjoj commented Oct 11, 2024 • edited Loading

rnhmjoj commented Sep 14, 2024 •

edited

Loading

rnhmjoj Sep 14, 2024 •

edited

Loading

rnhmjoj Sep 14, 2024 •

edited

Loading

rnhmjoj Sep 14, 2024 •

edited

Loading

emilazy commented Sep 15, 2024 •

edited

Loading

fabianhjr left a comment •

edited

Loading

emilazy commented Oct 11, 2024 •

edited

Loading

rnhmjoj commented Oct 11, 2024 •

edited

Loading