nixos/syncthing: define and handle encryptionPassword option #342138

h33p · 2024-09-15T20:16:06Z

Description of changes

Redefine devices to accept either a list of strings, or an attrset.
If attrset, allow specifying encryptionPassword as a reference to password to use for device.
Modify activation script to read all encryption passwords and concatinate them to JSON without hitting nix store.

cc @kira-bruneau @WillPower3309 @bjornfor

Things done

Built on platform(s)
- x86_64-linux
- aarch64-linux
- x86_64-darwin
- aarch64-darwin
Tested, as applicable:
- NixOS test(s) (look inside nixos/tests)
- and/or package tests
- or, for functions and "core" functionality, tests in lib/tests or pkgs/test
- made sure NixOS tests are linked to the relevant packages
Tested basic functionality of all binary files (usually in ./result/bin/)
24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
- (Module updates) Added a release notes entry if the change is significant
Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

Rewrite the syncthing config update script to embed secrets into the json request. Specifically, we handle the `encryptionPassword` secret. With this code, the user can embed path to the encrpyption password for a given device the folder is shared with, and have it loaded in, without touching the nix store.

- Change `folder.devices` type into `oneOf [(listOf str) (attrsOf (submodule { ... }))]`. - Expose `encryptionPassord` within the attrSet of the devices option. This allows the user to set the encrpyption password use to share the folder's data with. We do this by file path, as opposed to string literal, because we do not want to embed the encrpyption password into the nix store.

jfly

Great work here. I'd really love to have this feature!

jfly · 2025-01-12T19:50:37Z

nixos/doc/manual/release-notes/rl-2411.section.md

@@ -518,6 +518,8 @@
 - `nix.channel.enable = false` no longer implies `nix.settings.nix-path = []`.
  Since Nix 2.13, a `nix-path` set in `nix.conf` cannot be overriden by the `NIX_PATH` configuration variable.

+- `services.syncthing` now accepts an `attrSet` for `folders[<folder>].devices`, allowing to set `encryptionPassword` file for a device.


nit: Is the "official" term actually attrset? I see that on nix.dev, at least: https://nix.dev/tutorials/packaging-existing-software.html (emphasis mine):

Only add the top-level xorg derivation to the input attrset, rather than the full xorg.libX11, as the latter would cause a syntax error.

jfly · 2025-01-12T19:56:07Z

nixos/modules/services/networking/syncthing.nix

@@ -378,11 +445,30 @@ in {
                  };

                  devices = mkOption {
-                    type = types.listOf types.str;
+                    type = types.oneOf [


nit: I think it would be slightly more convenient to restructure this as a listOf (oneOf [str (submodule ...)]. Then people would be able to intermix string devices alongside structured devices with encryption passwords. For example:

devices = [ "deviceId1" { deviceId = "deviceId2"; encryptionPassword = "/path/to/secret"; } ];

Edit: Just saw your comment here:

Not sure how applicable it is to change the type of devices, but the current way where it's a list of strings is relatively clunky and differs from syncthing's structure. But that can be changed.

That's fair. To address my concern I propose that we deprecate the old way with the intent of moving entirely to an attrset in the future. We don't have to do the deprecation in this PR, though.

jfly · 2025-01-12T19:56:54Z

nixos/modules/services/networking/syncthing.nix

+                      (types.attrsOf (types.submodule ({ name, ... }: {
+                        freeformType = settingsFormat.type;
+                        options = {
+                          encryptionPassword = mkOption {


nit: There are probably counterexamples out there, but I've always seen these types of settings with a "File" suffix:

Suggested change

encryptionPassword = mkOption {

encryptionPasswordFile = mkOption {

jfly · 2025-01-12T19:59:25Z

nixos/modules/services/networking/syncthing.nix

+                      (types.listOf types.str)
+                      (types.attrsOf (types.submodule ({ name, ... }: {
+                        freeformType = settingsFormat.type;
+                        options = {


nit: We should add deviceId here as well. I'm pretty sure it counts as a "valuable option" (to use the terminology from RFC42)

jfly · 2025-01-12T20:01:20Z

nixos/modules/services/networking/syncthing.nix

+          if builtins.isString device then
+            { deviceId = cfg.settings.devices.${device}.id; }
+          else
+            device


I don't understand how we'd ever end up here. With the code in this PR, if devices is a list, then isn't it guaranteed to be a list of strs? (I am proposing changing that below)

jfly · 2025-01-12T22:13:26Z

nixos/tests/syncthing/folders.nix

+
+      # B should be able to decrypt, check that content of file matches
+      b.wait_for_file("/var/lib/syncthing/bar/plainname")
+      b.succeed("grep plaincontent /var/lib/syncthing/bar/plainname")


aPerhaps a bit stricter?

Suggested change

b.succeed("grep plaincontent /var/lib/syncthing/bar/plainname")

assert "plaincontent" == b.succeed("cat /var/lib/syncthing/bar/plainname")

(not sure if there will be a trailing newline in there or not)

jfly · 2025-01-12T22:14:28Z

nixos/tests/syncthing/folders.nix

+      b.succeed("grep plaincontent /var/lib/syncthing/bar/plainname")
+
+      # Bar on C is untrusted, check that content is not in cleartext
+      c.fail("grep -R plaincontent /var/lib/syncthing/bar")


This could fail if the file doesn't exist. I'd suggest something like this:

Suggested change

c.fail("grep -R plaincontent /var/lib/syncthing/bar")

assert "plaincontent" not in c.succeed("cat /var/lib/syncthing/bar")

jfly · 2025-01-12T22:16:26Z

nixos/modules/services/networking/syncthing.nix

+            secretVarsScript = generateSecretVars secretPaths;
+
+            jsonString = builtins.toJSON resolved_cfg;
+            escapedJson = builtins.replaceStrings ["\""] ["\\\""] jsonString;


Why aren't we using escapeShellArg anymore?

edit: Nevermind, I think I see now. We're generating a JSON string that happens to have interpolatable bash variables in it. This feels problematic to me. Is there a chance there are other $ variables just randomly in our resulting JSON, and letting bash try to treat them as variables is going to do the wrong thing (either error out, or worse, silently return an empty string)?

I experimented with a different approach that doesn't rely upon bash string interpolation. What do you think of this? e2ce674 (I've confirmed that nix-build --attr nixosTests.syncthing-folders still passes with this change. Thanks for writing the test!)

jfly · 2025-01-12T22:20:11Z

nixos/tests/all-tests.nix

+  syncthing = handleTest ./syncthing/default.nix {};
+  syncthing-no-settings = handleTest ./syncthing/no-settings.nix {};
+  syncthing-init = handleTest ./syncthing/init.nix {};
+  syncthing-many-devices = handleTest ./syncthing/many-devices.nix {};
+  syncthing-folders = handleTest ./syncthing/folders.nix {};


These renames are a nice refactor to do, but they're also conflict prone (I think it's the reason this PR is currently conflicting). Suggestion: do this in a separate, simpler PR that can get merged quickly.

jfly · 2025-01-12T22:23:20Z

nixos/modules/services/networking/syncthing.nix

+                  varName = "secret_${builtins.hashString "sha256" path}";
+                in
+                  ''
+                    if [ ! -r ${path} ]; then


Couldn't path have spaces in it?

$ test -r /foo bar test: too many arguments $ test -r "/foo bar"

Quotes seem necessary to me:

Suggested change

if [ ! -r ${path} ]; then

if [ ! -r "${path}" ]; then

jfly · 2025-02-19T17:49:25Z

I've rebased the changes in the PR and implemented changes according to my feedback above in #383442

github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Sep 15, 2024

h33p mentioned this pull request Sep 15, 2024

feature request: syncthing: add support for untrusted folders #121286

Open

NixOSInfra added the 12. first-time contribution This PR is the author's first one; please be gentle! label Sep 15, 2024

zarelit mentioned this pull request Sep 15, 2024

syncthing: add support for untrusted devices #205653

Closed

12 tasks

ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Sep 15, 2024

h33p added 2 commits September 19, 2024 16:16

h33p force-pushed the 121286_syncthing_encryption_password branch from 9540845 to 0bc8c16 Compare September 19, 2024 15:22

h33p added 3 commits September 19, 2024 16:35

nixosTests.syncthing: move tests to their own directory

3dcbfdc

nixosTests.syncthing: define test for declarative folders

137b008

nixosTests.syncthing: create encrypted device test

3c04dff

h33p force-pushed the 121286_syncthing_encryption_password branch from 494338f to 3c04dff Compare September 30, 2024 09:38

doc: 24.11: release note for syncthing changes

3018b7d

github-actions bot added 8.has: documentation This PR adds or changes documentation 8.has: changelog labels Sep 30, 2024

h33p marked this pull request as ready for review September 30, 2024 09:49

h33p changed the title ~~syncthing: define and handle encryptionPassword option~~ nixos/syncthing: define and handle encryptionPassword option Sep 30, 2024

wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Nov 10, 2024

jfly reviewed Jan 13, 2025

View reviewed changes

jfly mentioned this pull request Feb 19, 2025

nixos/syncthing: define and handle encryptionPassword option #383442

Open

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nixos/syncthing: define and handle encryptionPassword option #342138

nixos/syncthing: define and handle encryptionPassword option #342138

h33p commented Sep 15, 2024 •

edited

Loading

jfly left a comment

jfly Jan 12, 2025

jfly Jan 12, 2025

jfly Jan 12, 2025

jfly Jan 12, 2025

jfly Jan 12, 2025

jfly Jan 12, 2025

jfly Jan 12, 2025

jfly Jan 12, 2025

jfly Jan 12, 2025

jfly Jan 12, 2025

jfly commented Feb 19, 2025

	encryptionPassword = mkOption {
	encryptionPasswordFile = mkOption {

	b.succeed("grep plaincontent /var/lib/syncthing/bar/plainname")
	assert "plaincontent" == b.succeed("cat /var/lib/syncthing/bar/plainname")

	c.fail("grep -R plaincontent /var/lib/syncthing/bar")
	assert "plaincontent" not in c.succeed("cat /var/lib/syncthing/bar")

nixos/syncthing: define and handle encryptionPassword option #342138

Are you sure you want to change the base?

nixos/syncthing: define and handle encryptionPassword option #342138

Conversation

h33p commented Sep 15, 2024 • edited Loading

Description of changes

Things done

jfly left a comment

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jfly commented Feb 19, 2025

h33p commented Sep 15, 2024 •

edited

Loading