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

syncthing: add support for untrusted devices #205653

Closed

Conversation

zarelit
Copy link
Member

@zarelit zarelit commented Dec 11, 2022

Description of changes

Add support for syncthing untrusted devices as requested in #121286.

Syncthing allows folders to be shared with both untrusted and trusted devices devices at the same time so the PR introduces an option at the folder-level to specify the encryption password for the untrusted devices.

Things done
  • Added a test that actually use the configuration generated by the service module
  • Moved syncthing-related tests to their own directory (they're 4 now)
  • Added encryptionPasswordFiles option at the folder level
  • Double checked that the content of the password files are not leaked through listing of the processes
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • 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/)
  • 23.05 Release Notes (or backporting 22.11 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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/1598

Comment on lines 607 to 610
syncthing = handleTest ./syncthing/syncthing.nix {};
syncthing-init = handleTest ./syncthing/syncthing-init.nix {};
syncthing-relay = handleTest ./syncthing/syncthing-relay.nix {};
syncthing-declarative = handleTest ./syncthing/syncthing-declarative.nix {};
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
syncthing = handleTest ./syncthing/syncthing.nix {};
syncthing-init = handleTest ./syncthing/syncthing-init.nix {};
syncthing-relay = handleTest ./syncthing/syncthing-relay.nix {};
syncthing-declarative = handleTest ./syncthing/syncthing-declarative.nix {};
syncthing = handleTest ./syncthing {};
syncthing-init = handleTest ./syncthing/init.nix {};
syncthing-relay = handleTest ./syncthing/relay.nix {};
syncthing-declarative = handleTest ./syncthing/declarative.nix {};

maybe, not sure

Copy link
Member Author

Choose a reason for hiding this comment

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

it would be nice, now that I think of it I would keep syncthing-relay = handleTest ./syncthing-relay.nix {};
@SuperSandro2000 do you think it would be good to move syncthing relay in the top directory for tests again? syncthing-relay is a software on its own.

@schnusch
Copy link
Contributor

I know it would break a lot of existing configurations, but wouldn't it be nice to turn services.syncthing.folders.<name>.devices into an attrset?

This way the configuration would look like:

{
  services.syncthing.folders = {
    foo = {
      path = "/var/lib/syncthing/foo";
      devices = {
        "b" = {};
        "c" = {};
      };
    };
    bar = {
      path = "/var/lib/syncthing/bar";
      devices = {
        "c".encryptionPasswordFile = "${testPasswordFile}";
      };
    };
  };       
}

To make it backwards compatible or nice for the folder foo it could also support both a list and an attrset.

@Xyz00777
Copy link
Contributor

lassulus and i added a the freeform option and a few other options to syncthing with #226088 is your pr still needed with that? beside of the test edits? would you like to try it please :)

@zarelit
Copy link
Member Author

zarelit commented Apr 30, 2023

@Xyz00777 the core of the PR is to avoid leaking the encryption secret in the nix store so the generated configuration is patched after being generated from the options and before getting applied.

I absolutely agree with the introduction of the settings option but I don't know if I have the time to reshape this PR after the merge of #226088 because it currently have options nested inside the folder structure and they can't be directly rendered as part of settings

@zarelit
Copy link
Member Author

zarelit commented Apr 30, 2023

@Lassulus @Xyz00777 On second sight: I can probably "rebase" this PR on top of syncthing.settings, syncthing is lax about what keys are present in the configuration file and will silently ignore the encryptionPasswordFile with a path in it... I just don't know if it's possible to nest this under syncthing.settings.folders again since it's freeform

@zarelit
Copy link
Member Author

zarelit commented May 17, 2023

Needs deep reworking after merge of #226088 😔 marking as draft

@zarelit zarelit marked this pull request as draft May 17, 2023 14:12
@RusticCraftsman
Copy link

Needs deep reworking after merge of #226088 pensive marking as draft

Any updates on this? What's the state now after that merge? what is needed in order to implement this feature?

@Lassulus
Copy link
Member

Needs deep reworking after merge of #226088 pensive marking as draft

Any updates on this? What's the state now after that merge? what is needed in order to implement this feature?

we are trying to get #233386 merged, so we can continue the other PRs.

@wegank wegank added 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 2.status: merge conflict labels Mar 19, 2024
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 20, 2024
@wegank wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 4, 2024
@mtroberts
Copy link

we are trying to get #233386 merged, so we can continue the other PRs.

I note this has been merged for some time. Any progress on this feature?

@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 12, 2024
@zarelit
Copy link
Member Author

zarelit commented Jul 14, 2024

@mtroberts unfortunately I hadn't the time to work on this. This PR has bit rot and needs rework. I'm closing it for the time being. Feel free to leave a comment or thumbs in the open issue #121286 to signal that you'd like this to be implemented

@zarelit zarelit closed this Jul 14, 2024
@zarelit
Copy link
Member Author

zarelit commented Sep 15, 2024

New one is here: #342138 , thanks @h33p

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.

9 participants