nixos/knot: add keyFiles option #79266

Mic92 · 2020-02-05T14:28:59Z

Add an option to include configuration not in the nix store.
The use case for this are nixops like tools where the configuration
is not world-readable. Because of knot's DynamicUser we need to
install those files with the correct permission on startup into /var/lib/knot.

Motivation for this change

Things done

Mic92 · 2020-02-05T14:32:33Z

Because of DynamicUser the permission management is not yet very straight forward:

services.knot = {
  enable = true;
  checkConfig = false;
  extraConfig = ''
    include: /var/lib/knot/knot-he-key.conf
  '';
};
systemd.services.knot = {
  serviceConfig.PermissionsStartOnly = true;
  preStart = ''
    install -m700 --owner $USER /var/src/secrets/knot-he-key.conf /var/lib/knot/knot-he-key.conf
  '';
};

Maybe rather than checkConfig I could also add an option to add custom secret files?

mweinelt

I understand your use case and the change is fine with me.

vcunat · 2020-02-05T15:37:09Z

I haven't really thought about best practices for managing these secrets (for knot service).

Mic92 · 2020-02-05T15:58:41Z

I though about adding an systemd.services.knot.keyFiles = [ "/path/to/tsig.conf" ];.
Those would be copied to /var/lib/knot/keys.d and than included in knot's default configuration.

vcunat · 2020-02-05T16:03:41Z

I wonder if the risk of omitting the quotation marks is too high (leaking secrets by accident) 🤔

EDIT: I expect such mistakes can be detected in eval-time by some type-checking.

Mic92 · 2020-02-05T16:10:58Z

I wonder if the risk of omitting the quotation marks is too high thinking

Usually when using nixops is using something like /run/keys, which is likely to fail on the developer machine. In theory all passwordFile options in nixos could be accidentally miss-used like that. However I see a fix for that outside of scope of this PR. This would be solved by a linter or custom nixos type option that check if the path is in the nix store.

symphorien · 2020-02-05T19:56:20Z

About passwordFile like options: #78640

Mic92 · 2020-02-08T08:16:31Z

With the latest commit knot now can do automatic DNSSEC signing.

Mic92 · 2020-02-08T10:24:55Z

nixos/modules/services/networking/knot.nix

@@ -5,14 +5,16 @@ with lib;
 let
  cfg = config.services.knot;

-  configFile = pkgs.writeText "knot.conf" cfg.extraConfig;
-  socketFile = "/run/knot/knot.sock";
+  configFile = pkgs.writeTextFile {


I tried to convert nix attrsets to yaml using remarshal. However the yaml subset chosen by knot is too restrictive for that (i.e. expect keys to appear in a certain order).

Have you seen this thread? If a few people tries this out in practice, I suppose we could make it available as another optional attribute.

I'm not aware of keys being expected in particular order, though (perhaps I misunderstand). In any case the yaml is stricter than what I see usually (and new yaml is even a super-set of json IIRC).

I had an issue with an acl rule where the id key was not the first key, which lead to an error. This broke already in the nixos test. Anyway this PR should not interfere with migrating to a different contribute type...

Oh, that's a known restriction, though not by me until now.

I think I have an idea how to approach this; hopefully I can get it to work soon. I'll link from here when that happens.

It didn't really go the way I imagined, but let's continue on #81460

Mic92 · 2020-02-08T10:26:27Z

Instead of adding an option to disable configuration checks I added a keyFiles option that implicitly disable configuration checks when used. This also gets permissions right in combination with knot's DynamicUser option.

Mic92 · 2020-02-08T18:26:09Z

Here is a real-world example: https://github.com/Mic92/dotfiles/blob/34a97d3292b833fbad8f716f9c37a28eb4121eb1/nixos/eve/modules/knot/default.nix

Also the nixos test covers the new feature as well.

Otherwise knot tries to write to non-writable directories. This for example breaks dnssec signing. While it's possible to overwrite these path in the configuration, having a sane defaults is nicer.

This makes it hard to include secret files. Also using tools like keymgr becomes harder.

This useful to include tsig keys using nixops without adding those world-readable to the nix store.

Mic92 · 2020-02-12T16:39:10Z

I dropped DynamicUser. Also because of #79928

Mic92 · 2020-02-13T10:56:01Z

@ju1m If you would have a short look at it and give me a thumbs up, I would go forward and merge it!

nixos/knot: add keyFiles option (cherry picked from commit 466c1df)

ofborg bot added 6.topic: nixos 8.has: module (update) labels Feb 5, 2020

Mic92 requested review from mweinelt and vcunat February 5, 2020 14:32

ofborg bot added 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 labels Feb 5, 2020

mweinelt approved these changes Feb 5, 2020

View reviewed changes

vcunat approved these changes Feb 5, 2020

View reviewed changes

ofborg bot requested a review from vcunat February 8, 2020 08:34

ofborg bot added 10.rebuild-darwin: 1-10 and removed 10.rebuild-darwin: 0 labels Feb 8, 2020

Mic92 force-pushed the knot branch from 713bfcb to 9456fcd Compare February 8, 2020 10:22

Mic92 changed the title ~~nixos/knot: make configuration check optional~~ nixos/knot: add keyFiles option Feb 8, 2020

Mic92 commented Feb 8, 2020

View reviewed changes

Mic92 mentioned this pull request Feb 12, 2020

knot: fix keymgr permissions #79928

Closed

10 tasks

Mic92 added 3 commits February 12, 2020 16:27

knot: put runtime paths outside the nix store

6adc09e

Otherwise knot tries to write to non-writable directories. This for example breaks dnssec signing. While it's possible to overwrite these path in the configuration, having a sane defaults is nicer.

knot: drop dynamic user

88029bc

This makes it hard to include secret files. Also using tools like keymgr becomes harder.

knot: add keyFiles option

e2ef8b4

This useful to include tsig keys using nixops without adding those world-readable to the nix store.

Mic92 force-pushed the knot branch from 9456fcd to e2ef8b4 Compare February 12, 2020 16:37

Mic92 merged commit 466c1df into NixOS:master Feb 15, 2020

Mic92 deleted the knot branch February 15, 2020 11:15

dtzWill pushed a commit to dtzWill/nixpkgs that referenced this pull request Feb 15, 2020

Merge pull request NixOS#79266 from Mic92/knot

9624543

nixos/knot: add keyFiles option (cherry picked from commit 466c1df)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nixos/knot: add keyFiles option #79266

nixos/knot: add keyFiles option #79266

Mic92 commented Feb 5, 2020 •

edited

Mic92 commented Feb 5, 2020

mweinelt left a comment

vcunat commented Feb 5, 2020 •

edited

Mic92 commented Feb 5, 2020 •

edited

vcunat commented Feb 5, 2020 •

edited

Mic92 commented Feb 5, 2020

symphorien commented Feb 5, 2020

Mic92 commented Feb 8, 2020

Mic92 Feb 8, 2020

vcunat Feb 8, 2020

vcunat Feb 8, 2020 •

edited

Mic92 Feb 8, 2020

vcunat Feb 8, 2020

vcunat Feb 22, 2020

vcunat Mar 1, 2020

Mic92 commented Feb 8, 2020

Mic92 commented Feb 8, 2020

Mic92 commented Feb 12, 2020

Mic92 commented Feb 13, 2020

nixos/knot: add keyFiles option #79266

nixos/knot: add keyFiles option #79266

Conversation

Mic92 commented Feb 5, 2020 • edited

Motivation for this change

Things done

Mic92 commented Feb 5, 2020

mweinelt left a comment

Choose a reason for hiding this comment

vcunat commented Feb 5, 2020 • edited

Mic92 commented Feb 5, 2020 • edited

vcunat commented Feb 5, 2020 • edited

Mic92 commented Feb 5, 2020

symphorien commented Feb 5, 2020

Mic92 commented Feb 8, 2020

Mic92 Feb 8, 2020

Choose a reason for hiding this comment

vcunat Feb 8, 2020

Choose a reason for hiding this comment

vcunat Feb 8, 2020 • edited

Choose a reason for hiding this comment

Mic92 Feb 8, 2020

Choose a reason for hiding this comment

vcunat Feb 8, 2020

Choose a reason for hiding this comment

vcunat Feb 22, 2020

Choose a reason for hiding this comment

vcunat Mar 1, 2020

Choose a reason for hiding this comment

Mic92 commented Feb 8, 2020

Mic92 commented Feb 8, 2020

Mic92 commented Feb 12, 2020

Mic92 commented Feb 13, 2020

Mic92 commented Feb 5, 2020 •

edited

vcunat commented Feb 5, 2020 •

edited

Mic92 commented Feb 5, 2020 •

edited

vcunat commented Feb 5, 2020 •

edited

vcunat Feb 8, 2020 •

edited