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

Jump to bottom

nixos/opendkim: systemd sandbox #93314

Merged
merged 3 commits into from Sep 5, 2020
Merged

nixos/opendkim: systemd sandbox #93314

merged 3 commits into from Sep 5, 2020

Conversation

tnias
Copy link
Contributor

@tnias tnias commented Jul 16, 2020

Motivation for this change

Add systemd service sandbox features.

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/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

cc opendkim maintainer @abbradar

@hmenke hmenke mentioned this pull request Jul 17, 2020
Draft
11 tasks
@symphorien
Copy link
Member

CC @nlewo : this breaks simple nixos mailserver, because it uses PreStart to generate the key apparently.

relevant log:

server # 2020-08-08T21:37:56.922138+00:00 server systemd[1]: Starting OpenDKIM signing and verification daemon...
server # [   19.490661] systemd[1]: Starting OpenDKIM signing and verification daemon...
server # [   20.799512] opendkim-pre-start2020-08-08T21:37:58.233772+00:00 server opendkim-pre-start[866]: .opendkim-genkey-wrapped: openssl exited with status %d
server # [866]: .opendkim-genkey-wrapped: openssl exited with status %d
server # 2020-08-08T21:37:58.243123+00:00 server opendkim-pre-start[866]: 1
server # [   20.810270] opendkim-pre-start[866]: 1
server # 2020-08-08T21:37:58.256087+00:00 server systemd[1]: opendkim.service: Control process exited, code=exited, status=1/FAILURE
server # [   20.823766] systemd[1]: opendkim.service: Control process exited, code=exited, status=1/FAILURE
server # 2020-08-08T21:37:58.261531+00:00 server systemd[1]: opendkim.service: Failed with result 'exit-code'.
server # [   20.835354] systemd[1]: opendkim.service: Failed with result 'exit-code'.
server # 2020-08-08T21:37:58.272523+00:00 server systemd[1]: Failed to start OpenDKIM signing and verification daemon.
server # [   20.846334] systemd[1]: Failed to start OpenDKIM signing and verification daemon.

@nlewo
Copy link
Member

nlewo commented Aug 13, 2020 •
edited

@tnias I think we need to add a ReadWritePaths = [cfg.keyPath]. The current implementation is working because the default value of keyPath is /var/lib/opendkim/keys which is in the default systemd stateDir. But if we set keyPath to something else, the preStart script could then not write the key.
@symphorien If cfg.keyPath is a "readWrite" path, I think this should work for SNM.

@nlewo
Copy link
Member

nlewo commented Aug 23, 2020

@tnias what do you think about #93314 (comment)?

@tnias
Copy link
Contributor Author

tnias commented Aug 23, 2020

@nlewo sorry, forgot to respond. Sounds good. 👍 Added it as a new commit to this PR.

@symphorien
Copy link
Member

simple-nixos-mailserver tests now pass with this diff:

diff --git a/mail-server/opendkim.nix b/mail-server/opendkim.nix
index d381519..6fd0bef 100644
--- a/mail-server/opendkim.nix
+++ b/mail-server/opendkim.nix
@@ -56,6 +56,7 @@ in
       services.opendkim = {
         enable = true;
         selector = cfg.dkimSelector;
+        keyPath = cfg.dkimKeyDirectory;
         domains = "csl:${builtins.concatStringsSep "," cfg.domains}";
         configFile = pkgs.writeText "opendkim.conf" (''
           Canonicalization relaxed/simple

@symphorien
Copy link
Member

I also ran simple-nixos-mailserver's tests with success on top of the merge of this PR and #93293 #93277 and #93305

@symphorien
Copy link
Member

These PRs probably deserve (succinct) release notes.

@nlewo
Copy link
Member

nlewo commented Aug 31, 2020

These PRs probably deserve (succinct) release notes.

Yep, @tnias could you please add a sentence saying opendkim is now sanboxed in the section "Other Notable Changes" of the 20.09 release notes?

@tnias
Copy link
Contributor Author

tnias commented Sep 3, 2020

Yes, will add it.

@tnias
Copy link
Contributor Author

tnias commented Sep 3, 2020

@nlewo I rebased the patches against current master, as to not interfere with other releasenote changes.

@nlewo
Copy link
Member

nlewo commented Sep 5, 2020

Thanks!

@nlewo nlewo merged commit d65002a into NixOS:master Sep 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
6.topic: nixos 8.has: changelog 8.has: documentation 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 10.rebuild-linux: 1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@tnias @symphorien @nlewo