signal-desktop: preload sqlcipher library from nixpkgs

[andresilva]

Motivation for this change

Fix #124901.

Preloading the packaged node sqlcipher lib was leading to errors like:

/bin/sh: symbol lookup error: /nix/store/igb91vpsqwpiw9kpppjhxhhlk4q4b4s5-signal-desktop-5.10.0/lib/Signal/resources/app.asar.unpacked/node_modules/better-sqlite3/build/Release/better_sqlite3.node: undefined symbol: node_module_register

which prevented xdg-open calls to work and in turn having links fail to open in the browser. Preloading instead the sqlcipher library from the nixpkgs repo fixes the issue, and I could still open my existing database so I presume encryption is working.

I also needed to add json and fts5 feature support to the sqlcipher library since Signal uses these (these and more are included with the vanilla sqlite package).

Things done

• Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
• Built on platform(s)
  • NixOS
  • macOS
  • other Linux distributions
• Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
• Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
• Tested execution of all binary files (usually in ./result/bin/)
• 21.11 Release Notes (or backporting 21.05 Relase 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.

[primeos]

+1 for the idea but we should think this through first. IIRC we initially decided against using SQLCipher from Nixpkgs due to concerns about compatibility (hopefully this shouldn't be an issue but I haven't looked into it - would be welcome if you have time to look into that and could provide some references) and sticking with upstream's decisions. That said better_sqlite3.node is far from ideal...

Also: I'd like to have a dedicated maintainer for sqlcipher in Nixpkgs. I can (co-)maintain it but I'd like to have a second maintainer, if possible.

[andresilva]

@primeos I looked into it a bit more now. Signal maintains their own fork of better-sqlite3, the only substantial change is this commit signalapp/better-sqlite3@5acdaf9 where they replace the sqlite source code with the sqlcipher source. I've downloaded the sqlcipher source tarball they have there:

21451:#define CIPHER_VERSION_NUMBER 4.4.3

This is the same version as the sqlcipher package in nixpkgs. Apart from that their build has more features enabled than we're currently using for the sqlcipher package.

Theirs:

    'SQLITE_LIKE_DOESNT_MATCH_BLOBS',
    'SQLITE_THREADSAFE=2',
    'SQLITE_USE_URI=0',
    'SQLITE_DEFAULT_MEMSTATUS=0',
    'SQLITE_OMIT_DEPRECATED',
    'SQLITE_OMIT_GET_TABLE',
    'SQLITE_OMIT_TCL_VARIABLE',
    'SQLITE_OMIT_PROGRESS_CALLBACK',
    'SQLITE_OMIT_SHARED_CACHE',
    'SQLITE_TRACE_SIZE_LIMIT=32',
    'SQLITE_DEFAULT_CACHE_SIZE=-16000',
    'SQLITE_DEFAULT_FOREIGN_KEYS=1',
    'SQLITE_DEFAULT_WAL_SYNCHRONOUS=1',
    'SQLITE_ENABLE_COLUMN_METADATA',
    'SQLITE_ENABLE_UPDATE_DELETE_LIMIT',
    'SQLITE_ENABLE_STAT4',
    'SQLITE_ENABLE_FTS5',
    'SQLITE_ENABLE_JSON1',
    'SQLITE_ENABLE_RTREE',
    'SQLITE_INTROSPECTION_PRAGMAS',

    # SQLCipher-specific options
    'SQLITE_HAS_CODEC',
    'SQLITE_TEMP_STORE=2',
    'SQLITE_SECURE_DELETE',

Ours:

    "-DSQLITE_ENABLE_COLUMN_METADATA=1"
    "-DSQLITE_ENABLE_FTS5"
    "-DSQLITE_ENABLE_JSON1"
    "-DSQLITE_ENABLE_UNLOCK_NOTIFY=1"
    "-DSQLITE_HAS_CODEC"
    "-DSQLITE_SECURE_DELETE=1"

I have been using Signal for the past hours and everything seems to be working fine so far.

Also: I'd like to have a dedicated maintainer for sqlcipher in Nixpkgs. I can (co-)maintain it but I'd like to have a second maintainer, if possible.

I have never used sqlcipher and only looked into it now to fix this Signal issue. I am OK with being added as co-maintainer but just wanted to let you know I barely know anything about these packages :P

[andresilva]

@primeos I opted to not change the sqlcipher package directly and instead override it for signal to make sure we can use the exact same build features as the upstream signal npm package. I think this shouldn't cause any trouble now, we're building the same source with the same features enabled, we just need to keep an eye on upstream for changes going forward unfortunately.

[andresilva]

@primeos Any thoughts on this? I'm using this since I opened the PR, and I've been updating it locally with the new Signal versions. No issues so far.

[primeos]

@andresilva thanks for the ping and sorry about my delayed reply. I'm really upset about the way upstream handles this and was hoping that a proper upstream fix would be merged (IIRC signalapp/better-sqlite3#3 should fix it upstream). Anyway, I like the current approach and using better_sqlite3.node was a horrible hack in the first place (which I don't like either). It's just a bit annoying that we cannot easily ensure/guarantee that signal-sqlcipher doesn't diverge from signalapp/better-sqlite3 in a meaningful way, which is my main concern (but at least it should be fine as long as the major SQLite version matches and the VM test should help a bit as well).

I'll probably merge this within the next 7 days but I haven't completely made up my mind yet (usually things are much simpler to decide but in this case I'm still traumatized by the past issues due to stupid decisions by Signal-Desktop...).

[andresilva]

Updated the PR to fix conflicts. FWIW I have subscribed to notifications on the upstream https://github.com/signalapp/better-sqlite3 repo and will keep an eye for changes there.

[emptyflask]

Thanks for fixing this, it's been bugging me for months!

[SuperSandro2000]

@primeos if you haven't changed your mind about this PR I would like to merge it.

[primeos]

@SuperSandro2000 yes, absolutely. Feel free to merge it, I haven't changed my mind. I just seem to be way too incompetent to remember to finally merge this PR... :o (sorry, again, @andresilva - I'm still adjusting to working full-time and getting around to my backlog of TODOs).

With signalapp/better-sqlite3#3 finally merged we hopefully shouldn't need the LD_PRELOAD hack for long anyway.

[primeos]

I just gave it a very brief test as well while updating to version 5.22.0. LGTM so let's finally get it merged - at last... :)

Huge thanks @andresilva! For the fix, the fast and detailed responses, and the testing.