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

mautrix-{meta,signal,whatsapp}: build with goolm #337571

Merged
merged 1 commit into from
Sep 1, 2024

Conversation

gmacon
Copy link
Contributor

@gmacon gmacon commented Aug 26, 2024

Description of changes

After olm gained knownVulnerabilities in #334638, build these bridges using the pure-Go goolm library instead of libolm bindings. I also tried converting mautrix-discord, but it failed to build for what appears to be an upstream reason, but I saw no reason to block the other three. The other mautrix-* bridges in nixpkgs are Python bridges, and I didn't look into building those without olm.

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.

@emilazy
Copy link
Member

emilazy commented Aug 27, 2024

Tagging @sumnerevans here as I expect he’ll have opinions about this – per #336052 (comment) apparently goolm has known issues with these bridges.

@niklaskorz
Copy link
Contributor

I think this should be an option for now, i.e., add it as a package argument so it's easy to override. I'm impartial about which to make the default (libolm has the vulnerability, goolm is potentially not adhering to the spec yet and hasn't been audited).

@gmacon
Copy link
Contributor Author

gmacon commented Aug 27, 2024

I think this should be an option for now, i.e., add it as a package argument so it's easy to override.

Do you have any preferences about what shape the option should take? Is it withGoolm ? false?

@sumnerevans
Copy link
Contributor

We (Beeper) tried to use goolm in our next-generation Android client and ran into weird issues. It's not entirely clear whether the issues were due to goolm itself or the way that we were using the library. We ended up just using libolm to reduce the number of variables that could be causing our issues.

I definitely think compilation with goolm should be added as a disabled option for now. Selfishly, it would be great if some people started testing it out more in production and reported any issues with it.

@dotlambda
Copy link
Member

Selfishly, it would be great if some people started testing it out more in production and reported any issues with it.

I doubt any of those issues would be fixed.
The upstream repo has only seen a total of 16 commits by a single author.
There's also nothing that indicates it's more secure than libolm. Rather the opposite.

@emilazy
Copy link
Member

emilazy commented Aug 27, 2024

Mautrix uses their own fork.

@gmacon
Copy link
Contributor Author

gmacon commented Aug 28, 2024

Ok, here we go. OfBorg and Hydra won't build these as-is, but I tested the builds with commands like

nix build --impure --expr 'let p = import ./. {}; in p.mautrix-whatsapp.override { withGoolm = true; }'

I intend to use these options for my self-hosted Beeper bridges, and I'll report any issues I run into.

@gmacon gmacon added the backport release-24.05 Backport PR automatically label Aug 28, 2024
Copy link
Contributor

@niklaskorz niklaskorz left a comment

Choose a reason for hiding this comment

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

Tested the whatsapp and signal bridges, both seem to work just fine with goolm enabled

Edit: looks like replies on the whatsapp bridge are broken with goolm
Edit 2: same on the Signal bridge, all incoming messages are broken with goolm. Nonetheless I'm not opposed to merging this as it's opt-in right now.

Copy link
Member

@Ma27 Ma27 left a comment

Choose a reason for hiding this comment

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

Given the current issues with this flag we should add a warning as comment above. Given that these flags aren't discoverable at all, you'll see the warning when you learn about the flag (by reading the expression).

@gmacon
Copy link
Contributor Author

gmacon commented Aug 29, 2024

How about:

This option enables the use of the goolm library instead of libolm for end-to-end encryption. Using goolm is not officially supported by the mautrix developers, but they are interested in people trying it out and reporting any issues they run into.

@sumnerevans , I'm particularly interested in your feedback here to make sure that I don't misrepresent what Beeper wants to promise/request.

@gmacon gmacon marked this pull request as draft August 29, 2024 22:50
@sumnerevans
Copy link
Contributor

We marked it as experimental as of https://github.com/mautrix/go/releases/tag/v0.17.0, so maybe:

This option enables the use of an experimental pure-Go implementation of the Olm protocol instead of libolm for end-to-end encryption. Using goolm is not recommended by the mautrix developers, but they are interested in people trying it out in non-production-critical environments and reporting any issues they run into.

Given the issues that people have already mentioned, I think we should warn people about using this in production-critical environments.

After olm gained knownVulnerabilities in NixOS#334638, allow building these
bridges using the pure-Go goolm library instead of libolm bindings.
@gmacon gmacon marked this pull request as ready for review August 30, 2024 21:52
@gmacon gmacon requested a review from Ma27 August 30, 2024 21:52
@ofborg ofborg bot requested a review from niklaskorz August 30, 2024 22:33
Copy link
Member

@Ma27 Ma27 left a comment

Choose a reason for hiding this comment

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

Approach is OK for me.
Haven't tried the goolm variant out so far, but @sumnerevans having approved this is OK change-wise, so let's merge.

@Ma27 Ma27 merged commit ac0aa32 into NixOS:master Sep 1, 2024
27 of 28 checks passed
Copy link
Contributor

github-actions bot commented Sep 1, 2024

Successfully created backport PR for release-24.05:

@gmacon gmacon deleted the mautrix-goolm branch September 2, 2024 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants