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

nixos/syncthing: define and handle encryptionPassword option #342138

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions nixos/doc/manual/release-notes/rl-2411.section.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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.


## Detailed migration information {#sec-release-24.11-migration}

### `sound` options removal {#sec-release-24.11-migration-sound}
Expand Down
104 changes: 95 additions & 9 deletions nixos/modules/services/networking/syncthing.nix
Original file line number Diff line number Diff line change
Expand Up @@ -34,12 +34,22 @@ let
The options services.syncthing.settings.folders.<name>.{rescanInterval,watch,watchDelay}
were removed. Please use, respectively, {rescanIntervalS,fsWatcherEnabled,fsWatcherDelayS} instead.
'' {
devices = map (device:
if builtins.isString device then
{ deviceId = cfg.settings.devices.${device}.id; }
devices = let
folderDevices = folder.devices;
in
if builtins.isList folderDevices then
map (device:
if builtins.isString device then
{ deviceId = cfg.settings.devices.${device}.id; }
else
device
Copy link
Contributor

Choose a reason for hiding this comment

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

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)

) folderDevices
else if builtins.isAttrs folderDevices then
mapAttrsToList (deviceName: deviceValue:
deviceValue // { deviceId = cfg.settings.devices.${deviceName}.id; }
) folderDevices
else
device
) folder.devices;
throw "Invalid type for devices in folder '${folderName}'; expected list or attrset.";
}) (filterAttrs (_: folder:
folder.enable
) cfg.settings.folders);
Expand Down Expand Up @@ -103,9 +113,66 @@ let
# don't exist in the array given. That's why we use here `POST`, and
# only if s.override == true then we DELETE the relevant folders
# afterwards.
(map (new_cfg: ''
curl -d ${lib.escapeShellArg (builtins.toJSON new_cfg)} -X POST ${s.baseAddress}
''))
(map (new_cfg:
let
isSecret = attr: value: builtins.isString value && attr == "encryptionPassword";
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand why we're checking if value is a string here. It can only be str or null, right? We should just check if it's null.


resolveSecrets = attr: value:
if builtins.isAttrs value then
# Attribute set: process each attribute
builtins.mapAttrs (name: val: resolveSecrets name val) value
else if builtins.isList value then
# List: process each element
map (item: resolveSecrets "" item) value
else if isSecret attr value then
# String that looks like a path: replace with placeholder
let
varName = "secret_${builtins.hashString "sha256" value}";
in
"\${${varName}}"
else
# Other types: return as is
value;

# Function to collect all file paths from the configuration
collectPaths = attr: value:
if builtins.isAttrs value then
concatMap (name: collectPaths name value.${name}) (builtins.attrNames value)
else if builtins.isList value then
concatMap (name: collectPaths "" name) value
else if isSecret attr value then
[ value ]
else
[];

# Function to generate variable assignments for the secrets
generateSecretVars = paths:
concatStringsSep "\n" (map (path:
let
varName = "secret_${builtins.hashString "sha256" path}";
in
''
if [ ! -r ${path} ]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

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

echo "${path} does not exist"
exit 1
fi
${varName}=$(<${path})
''
) paths);

resolved_cfg = resolveSecrets "" new_cfg;
secretPaths = collectPaths "" new_cfg;
secretVarsScript = generateSecretVars secretPaths;

jsonString = builtins.toJSON resolved_cfg;
escapedJson = builtins.replaceStrings ["\""] ["\\\""] jsonString;
Copy link
Contributor

Choose a reason for hiding this comment

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

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!)

in
''
${secretVarsScript}

curl -d "${escapedJson}" -X POST ${s.baseAddress}
''
))
(lib.concatStringsSep "\n")
]
/* If we need to override devices/folders, we iterate all currently configured
Expand Down Expand Up @@ -378,11 +445,30 @@ in {
};

devices = mkOption {
type = types.listOf types.str;
type = types.oneOf [
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

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

encryptionPassword = mkOption {
Copy link
Contributor

Choose a reason for hiding this comment

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

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 {

type = types.nullOr types.str;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

Suggested change
type = types.nullOr types.str;
type = types.nullOr types.path;

We should also add an assertion like this one in step-ca to make sure people don't accidentally leak the password into their store.

(I was sure we had a type that would enforce this, but AFAICT, we don't. I've sent out a PR implementing a types.pathNotInStore here: #373287)

default = null;
description = ''
Path to encryption password. If set, the file will be read during
service activation, without being embedded in derivation.
'';
};
};
}))
)];
default = [];
description = ''
The devices this folder should be shared with. Each device must
be defined in the [devices](#opt-services.syncthing.settings.devices) option.

Either a list of strings, or an attribute set, where keys are defined in the
[devices](#opt-services.syncthing.settings.devices) option, and values are
device configurations.
'';
};

Expand Down
9 changes: 5 additions & 4 deletions nixos/tests/all-tests.nix
Original file line number Diff line number Diff line change
Expand Up @@ -941,10 +941,11 @@ in {
switchTestNg = handleTest ./switch-test.nix { ng = true; };
sx = handleTest ./sx.nix {};
sympa = handleTest ./sympa.nix {};
syncthing = handleTest ./syncthing.nix {};
syncthing-no-settings = handleTest ./syncthing-no-settings.nix {};
syncthing-init = handleTest ./syncthing-init.nix {};
syncthing-many-devices = handleTest ./syncthing-many-devices.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 {};
Comment on lines +944 to +948
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

syncthing-relay = handleTest ./syncthing-relay.nix {};
sysinit-reactivation = runTest ./sysinit-reactivation.nix;
systemd = handleTest ./systemd.nix {};
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import ./make-test-python.nix ({ lib, pkgs, ... }: {
import ../make-test-python.nix ({ lib, pkgs, ... }: {
name = "syncthing";
meta.maintainers = with pkgs.lib.maintainers; [ chkno ];

Expand Down
120 changes: 120 additions & 0 deletions nixos/tests/syncthing/folders.nix
Original file line number Diff line number Diff line change
@@ -0,0 +1,120 @@
import ../make-test-python.nix (
{ lib, pkgs, ... }:
let
genNodeId =
name:
pkgs.runCommand "syncthing-test-certs-${name}" { } ''
mkdir -p $out
${pkgs.syncthing}/bin/syncthing generate --config=$out
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems a bit odd to me for us to be doing something non-deterministic here (I imagine that syncthing generate rolls dice under the hood).

That said, I don't know if this concretely is a problem. Just couldn't help but comment and wonder.

${pkgs.libxml2}/bin/xmllint --xpath 'string(configuration/device/@id)' $out/config.xml > $out/id
'';
idA = genNodeId "a";
idB = genNodeId "b";
idC = genNodeId "c";
testPasswordFile = pkgs.writeText "syncthing-test-password" "it's a secret";
in
{
name = "syncthing";
meta.maintainers = with pkgs.lib.maintainers; [ zarelit ];
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know what the proper etiquette is here for adding other people to new tests. @zarelit, is this OK?


nodes = {
a = {
services.syncthing = {
enable = true;
openDefaultPorts = true;
cert = "${idA}/cert.pem";
key = "${idA}/key.pem";
settings = {
devices.b.id = lib.fileContents "${idB}/id";
devices.c.id = lib.fileContents "${idC}/id";
folders.foo = {
path = "/var/lib/syncthing/foo";
devices = [ "b" ];
};
folders.bar = {
path = "/var/lib/syncthing/bar";
devices.c.encryptionPassword = "${testPasswordFile}";
};
};
};
};
b = {
services.syncthing = {
enable = true;
openDefaultPorts = true;
cert = "${idB}/cert.pem";
key = "${idB}/key.pem";
settings = {
devices.a.id = lib.fileContents "${idA}/id";
devices.c.id = lib.fileContents "${idC}/id";
folders.foo = {
path = "/var/lib/syncthing/foo";
devices = [ "a" ];
};
folders.bar = {
path = "/var/lib/syncthing/bar";
devices.c.encryptionPassword = "${testPasswordFile}";
};
};
};
};
c = {
services.syncthing = {
enable = true;
openDefaultPorts = true;
cert = "${idC}/cert.pem";
key = "${idC}/key.pem";
settings = {
devices.a.id = lib.fileContents "${idA}/id";
devices.b.id = lib.fileContents "${idB}/id";
folders.bar = {
path = "/var/lib/syncthing/bar";
devices = [
"a"
"b"
];
type = "receiveencrypted";
};
};
};
};
};

testScript = ''
start_all()

a.wait_for_unit("syncthing.service")
b.wait_for_unit("syncthing.service")
c.wait_for_unit("syncthing.service")
a.wait_for_open_port(22000)
b.wait_for_open_port(22000)
c.wait_for_open_port(22000)

# Test foo

a.wait_for_file("/var/lib/syncthing/foo")
b.wait_for_file("/var/lib/syncthing/foo")

a.succeed("echo a2b > /var/lib/syncthing/foo/a2b")
b.succeed("echo b2a > /var/lib/syncthing/foo/b2a")

a.wait_for_file("/var/lib/syncthing/foo/b2a")
b.wait_for_file("/var/lib/syncthing/foo/a2b")

# Test bar

a.wait_for_file("/var/lib/syncthing/bar")
b.wait_for_file("/var/lib/syncthing/bar")
c.wait_for_file("/var/lib/syncthing/bar")

a.succeed("echo plaincontent > /var/lib/syncthing/bar/plainname")

# 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")
Copy link
Contributor

Choose a reason for hiding this comment

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

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)


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

Choose a reason for hiding this comment

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

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")

'';
}
)
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import ./make-test-python.nix ({ lib, pkgs, ... }: let
import ../make-test-python.nix ({ lib, pkgs, ... }: let

testId = "7CFNTQM-IMTJBHJ-3UWRDIU-ZGQJFR6-VCXZ3NB-XUH3KZO-N52ITXR-LAIYUAU";

Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import ./make-test-python.nix ({ lib, pkgs, ... }:
import ../make-test-python.nix ({ lib, pkgs, ... }:

# This nixosTest is supposed to check the following:
#
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import ./make-test-python.nix ({ lib, pkgs, ... }: {
import ../make-test-python.nix ({ lib, pkgs, ... }: {
name = "syncthing";
meta.maintainers = with pkgs.lib.maintainers; [ chkno ];

Expand Down