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.

Sign up for GitHub

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

opendkim: automated key generation (no manual changes for service ini… #29365

Merged
merged 1 commit into from Oct 23, 2017
Merged

opendkim: automated key generation (no manual changes for service ini… #29365

merged 1 commit into from Oct 23, 2017

Conversation

qknight
Copy link
Member

@qknight qknight commented Sep 14, 2017 •
edited

Motivation for this change

the nixcloud.email abstraction requires some changes and in this case we need to automate opendkim.

  • the key is created automatically from the preStart hook

this PR is related to #29366

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • Linux
  • 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 nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

@qknight qknight mentioned this pull request Sep 14, 2017
Closed
8 tasks
@eqyiel
Copy link
Contributor

eqyiel commented Sep 14, 2017

Nice! I bet someone will be glad to not have to work this out for themselves like I did: https://github.com/eqyiel/deployments/blob/master/realms/tsumugi.rkm.id.au/mail-server.nix#L468-L494

fpletz
fpletz approved these changes Sep 15, 2017
View reviewed changes
Copy link
Member

@fpletz fpletz left a comment

Choose a reason for hiding this comment

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

Thanks! Looks fine but look at my small nitpick.

nixos/modules/services/mail/opendkim.nix
@@ -57,9 +59,13 @@ in {
'';
};

keyFile = mkOption {
keyPath = mkOption {
Copy link
Member

Choose a reason for hiding this comment

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

We should add a mkRenamedOptionModule for this change.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, will fix that, thanks for pointing out!

Copy link
Member Author

Choose a reason for hiding this comment

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

fix landed, as you wished.

@fpletz fpletz added this to the 17.09 milestone Sep 15, 2017
@qknight qknight force-pushed the master_mailserver_PRs-opendkim branch from 0a6885d to 00dfa11 Compare September 26, 2017 11:53
@qknight
Copy link
Member Author

qknight commented Oct 21, 2017

@fpletz is the fix sufficient?

@fpletz
Copy link
Member

fpletz commented Oct 23, 2017

Sorry for the delay. Yeah, that's fine! Thanks!

@fpletz fpletz merged commit 61089dd into NixOS:master Oct 23, 2017
@fpletz
Copy link
Member

fpletz commented Oct 23, 2017

Backported to 17.09: b0dba8f

@rvl
Copy link
Contributor

rvl commented Nov 1, 2017

Thanks @qknight, good stuff, useful.

I was previously managing my opendkim key file with NixOps keys, so this change breaks that.

I would suggest that the keyFile option is not an alias but instead triggers an assertion which explains that keys are now generated. Also it might be worth adding a short note to the release notes and backporting that also.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fpletz fpletz fpletz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
17.09
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@qknight @eqyiel @fpletz @rvl