-
-
Notifications
You must be signed in to change notification settings - Fork 15k
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
base: master
Are you sure you want to change the base?
Changes from all commits
b5e81d5
0bc8c16
3dcbfdc
137b008
3c04dff
3018b7d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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 | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||||||
) 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); | ||||||
|
@@ -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"; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand why we're checking if |
||||||
|
||||||
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 | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Couldn't $ test -r /foo bar
test: too many arguments
$ test -r "/foo bar" Quotes seem necessary to me:
Suggested change
|
||||||
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; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why aren't we using 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 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 |
||||||
in | ||||||
'' | ||||||
${secretVarsScript} | ||||||
|
||||||
curl -d "${escapedJson}" -X POST ${s.baseAddress} | ||||||
'' | ||||||
)) | ||||||
(lib.concatStringsSep "\n") | ||||||
] | ||||||
/* If we need to override devices/folders, we iterate all currently configured | ||||||
|
@@ -378,11 +445,30 @@ in { | |||||
}; | ||||||
|
||||||
devices = mkOption { | ||||||
type = types.listOf types.str; | ||||||
type = types.oneOf [ | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 devices = [
"deviceId1"
{
deviceId = "deviceId2";
encryptionPassword = "/path/to/secret";
}
]; Edit: Just saw your comment here:
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 = { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: We should add |
||||||
encryptionPassword = mkOption { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
|
||||||
type = types.nullOr types.str; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit:
Suggested change
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 |
||||||
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. | ||||||
''; | ||||||
}; | ||||||
|
||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 {}; | ||
|
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 | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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 ]; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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") | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. aPerhaps a bit stricter?
Suggested change
(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") | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
|
||||||
''; | ||||||
} | ||||||
) |
There was a problem hiding this comment.
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):