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

nixos/knot: init #56922

Merged
merged 1 commit into from
Mar 16, 2019
Merged

nixos/knot: init #56922

merged 1 commit into from
Mar 16, 2019

Conversation

mweinelt
Copy link
Member

@mweinelt mweinelt commented Mar 5, 2019

Motivation for this change

Allow for easier usage by creating a module with a secure-by-default systemd unit.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nox --run "nox-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)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@GrahamcOfBorg GrahamcOfBorg added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Mar 5, 2019
@mweinelt
Copy link
Member Author

mweinelt commented Mar 5, 2019

I'm still trying to figure out how and where to deploy zonefiles and also where mutable data will be written to. I'm new to Knot DNS and have no experience running it yet.

@GrahamcOfBorg GrahamcOfBorg added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Mar 5, 2019
Copy link
Member

@andir andir left a comment

Choose a reason for hiding this comment

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

Overall it looks like a good first draft. Thanks for working on it! 👍

When running the daemon with an empty configuration I received a bunch of warnings which we might want to investigate:

server# [    8.219413] knotd[792]: 2019-03-06T14:06:29 info: Knot DNS 2.7.6 starting
server# [    8.221783] knotd[792]: 2019-03-06T14:06:29 info: loading 0 zones
server# [    8.233685] knotd[792]: warning: cannot open persistent timer DB '/nix/store/0sv2dhwmfr3q7y67n2q0jfqipmfmjm5r-knot-dns-2.7.6/var/lib/knot/timers' (not exists)
server# [    8.238169] knotd[792]: 2019-03-06T14:06:29 warning: cannot open persistent timer DB '/nix/store/0sv2dhwmfr3q7y67n2q0jfqipmfmjm5r-knot-dns-2.7.6/var/lib/knot/timers' (not exists)
server# [    8.241953] knotd[792]: 2019-03-06T14:06:29 warning: no zones loaded
server# [    8.244193] knotd[792]: 2019-03-06T14:06:29 info: starting server
server# [    8.246495] knotd[792]: 2019-03-06T14:06:29 info: server started in the foreground, PID 792
server# [    8.249342] knotd[792]: 2019-03-06T14:06:29 info: control, binding to '/run/knot/knot.sock'
server# [    8.252291] knotd[792]: warning: no zones loaded

nixos/modules/services/networking/knot.nix Outdated Show resolved Hide resolved
nixos/modules/services/networking/knot.nix Outdated Show resolved Hide resolved
nixos/modules/services/networking/knot.nix Outdated Show resolved Hide resolved
@mweinelt
Copy link
Member Author

mweinelt commented Mar 6, 2019

Thanks, I'm looking into the feedback tonight.

@vcunat
Copy link
Member

vcunat commented Mar 6, 2019

/nix/store/HASH-knot-dns-VERSION/var

It's possible we'll want to modify the knot-dns package expression. So far I wrote it because of libs (for knot-resolver) and the kdig tool, so I haven't paid much attention to other parts.

@mweinelt
Copy link
Member Author

mweinelt commented Mar 6, 2019

warning: cannot open persistent timer DB '/nix/store/0sv2dhwmfr3q7y67n2q0jfqipmfmjm5r-knot-dns-2.7.6/var/lib/knot/timers' (not exists)

The timer implementation changed between between 2.7.6 and 2.8.0 and I guess we should probably bump to 2.8.0 before investigating. https://gitlab.labs.nic.cz/knot/knot-dns/commit/844209ef4fb68b582648871c44dd82fb3ce2e3b7

@vcunat vcunat mentioned this pull request Mar 6, 2019
10 tasks
@vcunat
Copy link
Member

vcunat commented Mar 6, 2019

There's a catch for knot-resolver, so I opened #56974 to at least serve as base for developing the nixos service.

@mweinelt
Copy link
Member Author

mweinelt commented Mar 6, 2019

Yup, the timer warnings are gone on 2.8.0.

Mar 06 19:30:41 juno systemd[1]: Starting Authoritative-only DNS server from .cz domain registry...
Mar 06 19:30:41 juno knotd[11785]: 2019-03-06T19:30:41 info: Knot DNS 2.8.0 starting
Mar 06 19:30:41 juno knotd[11785]: 2019-03-06T19:30:41 info: loading 0 zones
Mar 06 19:30:41 juno knotd[11785]: 2019-03-06T19:30:41 warning: no zones loaded
Mar 06 19:30:41 juno knotd[11785]: 2019-03-06T19:30:41 info: starting server
Mar 06 19:30:41 juno knotd[11785]: 2019-03-06T19:30:41 info: server started in the foreground, PID 11785
Mar 06 19:30:41 juno knotd[11785]: 2019-03-06T19:30:41 info: control, binding to '/run/knot/knot.sock'
Mar 06 19:30:41 juno knotd[11785]: warning: no zones loaded
Mar 06 19:30:41 juno systemd[1]: Started Authoritative-only DNS server from .cz domain registry.

@vcunat
Copy link
Member

vcunat commented Mar 7, 2019

@andir: what about using this?

{ # Knot DNS uses a restricted version of YAML.
  nix2yaml = let
    quoteString = s: ''"${toString s}"'';
    n2y = indent: val:
      if isAttrs val then concatStringsSep "\n${indent}"
        (mapAttrsToList
          # This is a bit wacky - set directly under a set would start on bad indent,
          # so we start those on a new line, but not other types of attribute values.
          (aname: aval: "${aname}:${if isAttrs aval then "\n${indent}  " else " "}"
            + n2y (indent + "  ") aval)
          val
        )
        + "\n"
        else
      if isList val && stringLength indent < 4 then concatMapStrings
        (elem: "\n${indent}- " + n2y (indent + "  ") elem)
        val
        else
      if isList val /* and long indent */ then
        "[ " + concatMapStringsSep ", " quoteString val + " ]" else
      quoteString val;
    in n2y "";

}

Example output:

server:
  listen: [ "0.0.0.0@53", ":@53" ]

zone: 
  - domain: "deleg.example.com."
    file: "/nix/store/2vn5pj4hsbsjbwr9is3syb01r7zsbcqn-deleg.example.com."
    storage: "/var/lib/knot"

  - domain: "example.com."
    file: "/nix/store/f9cxcqcw3kxk0pb8nv6a8dahxbzdfs44-example.com."
    storage: "/var/lib/knot"

@andir
Copy link
Member

andir commented Mar 7, 2019

@vcunat that looks great and (almost) works for my test that I hacked together 👍. I had to convert paths to string before passing it to the function but that is a trivial thing to add.

@vcunat
Copy link
Member

vcunat commented Mar 7, 2019

I thought toString worked fine on paths, except it would try to clone them into the /nix/store if written as such... which is what you meant? (But that feels like a natural semantics in nix to me.) EDIT: I was confused now, I guess. Let me look again later.

@vcunat
Copy link
Member

vcunat commented Mar 7, 2019

Ah, it's the other way around... I break the usual semantics by applying toString to paths as well, but I can fix that easily.

@andir
Copy link
Member

andir commented Mar 7, 2019

I was using the following during my tests:

let
      zones."example.com." = ''
        @ SOA ns.example.com noc.example.com 666 7200 3600 1209600 3600
        ipv4 A 1.2.3.4
        ipv6 AAAA abcd::eeff
        deleg NS ns.example.com
        ns A 192.168.0.1
        ns AAAA dead:beef::1
      '';
      zones."deleg.example.com." = ''
        @ SOA ns.example.com noc.example.com 666 7200 3600 1209600 3600
        @ A 9.8.7.6
        @ AAAA fedc::bbaa
      '';
      #zones."." = ''
      #  @ SOA ns.example.com noc.example.com 666 7200 3600 1209600 3600
      #  root A 1.8.7.4
      #  root AAAA acbd::4
      #'';
      nix2yaml = with lib; let
        quoteString = s: ''"${toString s}"'';
        n2y = indent: val:
          if isAttrs val then concatStringsSep "\n${indent}"
            (mapAttrsToList
              # This is a bit wacky - set directly under a set would start on bad indent,
              # so we start those on a new line, but not other types of attribute values.
              (aname: aval: "${aname}:${if isAttrs aval then "\n${indent}  " else " "}"
                + n2y (indent + "  ") aval)
              val
            )
            + "\n"
            else
          if isList val && stringLength indent < 4 then concatMapStrings
            (elem: "\n${indent}- " + n2y (indent + "  ") elem)
            val
            else
          if isList val /* and long indent */ then
            "[ " + concatMapStringsSep ", " quoteString val + " ]" else
          quoteString val;
      in n2y "";
    in {
      networking.nameservers = lib.mkForce [ "192.168.0.1" ];
      networking.interfaces.eth1.ipv4.addresses = [
        { address = "192.168.0.1"; prefixLength = 24; }
      ];
      networking.interfaces.eth1.ipv6.addresses = [
        { address = "dead:beef::1"; prefixLength = 64; }
      ];

      services.knot = {
        enable = true;
        extraConfig = nix2yaml {
          server.listen = [ "0.0.0.0@53" "::@53" ];
          zone = lib.mapAttrsToList (n: v: { domain = n; storage = "/var/lib/knot"; file = toString (pkgs.writeText n v); }) zones;
        };
      };

I added the extra toStringto the pkgs.writeText line. Since lib.isAttrs detects the result of writeText as an attribute set (rightfully?) and tried to access it's attributes which are functions.

@vcunat
Copy link
Member

vcunat commented Mar 7, 2019

Right, there were some subtle issues, commented in the code.

rec { # Knot DNS uses a restricted version of YAML.
  nix2yaml = let
    # We don't want paths like ./my-zone.txt be converted to plain strings.
    quoteString = s: ''"${if builtins.typeOf s == "path" then s else toString s}"'';
    # We don't want to walk the insides of derivation attributes.
    doRecurse = val: isAttrs val && !isDerivation val;
    n2y = indent: val:
      if doRecurse val then concatStringsSep "\n${indent}"
        (mapAttrsToList
          # This is a bit wacky - set directly under a set would start on bad indent,
          # so we start those on a new line, but not other types of attribute values.
          (aname: aval: "${aname}:${if doRecurse aval then "\n${indent}  " else " "}"
            + n2y (indent + "  ") aval)
          val
        )
        + "\n"
        else
      if isList val && stringLength indent < 4 then concatMapStrings
        (elem: "\n${indent}- " + n2y (indent + "  ") elem)
        val
        else
      if isList val /* and long indent */ then
        "[ " + concatMapStringsSep ", " quoteString val + " ]" else
      quoteString val;
    in n2y "";


       extraConfig = {
          server.listen = [ "0.0.0.0@53" "::@53" ];
          zone = mapAttrsToList
            (n: v: {
              domain = n;
              storage = "/var/lib/knot";
              file = if isString v then pkgs.writeText n v else v;
            })
            zones;
        };
       # which works with these options that came into my mind:
       zones.foo = "inline string";
       zones.bar = ./some-file;
       zones.baz = some derivation; # e.g. writeText, fetch* or whatever
}

@vcunat
Copy link
Member

vcunat commented Mar 7, 2019

I guess we could make a similar "zone.*" processing built into the conversion into YAML (as a pre-processor map on nix level), so that people have an simple way of configuring it in a way that's more natural in nix.

EDIT: another concern is that it would be nice to get more uniformity among nixos configs for different auth DNS implementations, but let's leave that for further iterations. Furthermore, IETF works on RESTCONF modelling for that purpose...

@andir
Copy link
Member

andir commented Mar 11, 2019

@vcunat how do you feel about merging the current implementation once it gets a test and then only afterwards investing in a more "complete" nix integration?

I think it would make sense to experiment with the different options before putting each through a nixpkgs PR. My personal DNS servers will move to this as soon as we merge this and I get the time to roll it out.

@vcunat
Copy link
Member

vcunat commented Mar 11, 2019

Yes, I'm in favor of faster iteration.

@vcunat
Copy link
Member

vcunat commented Mar 11, 2019

The changes might be (mostly) backward compatible even, if we only support the basic config options now – those that don't affect the configuration file. But anyway, I don't expect people will massively start to use the new service, so 100% compatibility doesn't seem too important.

@vcunat
Copy link
Member

vcunat commented Mar 11, 2019

I reviewed the systemd unit config, comparing it to upstream unit. Minor changes:

  • NoNewPrivileges = true; PrivateDevices = true; these seem OK
  • RestrictAddressFamilies = "AF_UNIX AF_INET AF_INET6";
    I'm perhaps missing the point there. And according to docs it's not much use without SystemCallArchitectures=native, because of the former not restricting 32-bit x86 syscalls.
  • StartLimitInterval = "0"; Can you explain why? Are restart bursts required for something? BTW, the option has been renamed.

[resolved below] Now, the change I consider big: DynamicUser = "yes";. Well, I can't claim to have much experience using this option or running authoritative DNS, but it seems quite a non-standard setup at least. Can you explain why not use a standard "persistent" user instead? I couldn't quickly find in docs in what way the state directory is being kept between service restarts, etc.

@andir
Copy link
Member

andir commented Mar 11, 2019

Now, the change I consider big: DynamicUser = "yes";. Well, I can't claim to have much experience using this option or running authoritative DNS, but it seems quite a non-standard setup at least. Can you explain why not use a standard "persistent" user instead? I couldn't quickly find in docs in what way the state directory is being kept between service restarts, etc.

The idea is to avoid issues with both exhausting our UID/GID preallocation space (that is rather limited) and to avoid any kind of UID/GID reuse attacks. Systemd will take care of making the StateDirectory (usually /var/lib/) writable for the dynamic user. We had a few discussions on IRC (#nixos{,-dev,-chat,-security,…}) where the conclusion was that dynamic users + state dir might be desireable for new services. There isn't anything documented about that idea just yet.

On that note we might want to set StateDirectoryMode= to something that isn't 0755 to protect the private keys - if know doesn't do that already.

@vcunat
Copy link
Member

vcunat commented Mar 11, 2019

So the state directory contents will still be persistent and kept across service/system restarts, right? (Changing the ownership in case systemd starts it with a different UID/GID, etc.)

@andir
Copy link
Member

andir commented Mar 11, 2019 via email

@vcunat
Copy link
Member

vcunat commented Mar 11, 2019

OK, sounds good (with my limited knowledge). Dir permissions sound OK as well, according to man systemd.exec, but we'd better also check it "in real life" :-)

If DynamicUser= is used in conjunction with StateDirectory=, CacheDirectory= and LogsDirectory=
is slightly altered: the directories are created below /var/lib/private, /var/cache/private and
/var/log/private, respectively, which are host directories made inaccessible to unprivileged
users
, which ensures that access to these directories cannot be gained through dynamic user ID
recycling.

@mweinelt
Copy link
Member Author

Thanks for your reviews. I made the following changes:

  • Added SystemCallArchitectures=native to prevent circumvention of RestrictAddressFamilies via syscalls against non-native ABIs
  • Dropped StartLimitInterval, it was a leftover from copying and pasting an example systemd unit description
  • Restricted StateDirectoryMode from 0755 to 0700

I'm looking into tests right now. Also I'm still wondering what (the package, the module, something else?) is responsible for getting man pages into the system environment.

@mweinelt
Copy link
Member Author

Regarding the state directories permission:

[root@juno:~]$ ls -lah /var/lib/knot
lrwxrwxrwx 1 root root 12 Mar  3 02:58 /var/lib/knot -> private/knot
[root@juno:~]$ ls -lah /var/lib/private/knot/
total 8.0K
drwx------ 2 knot knot 4.0K Mar  3 02:58 .
drwx------ 4 root root 4.0K Mar  3 02:58 ..
[hexa@juno:~]$ ls -lah /var/lib/private/knot
ls: cannot access '/var/lib/private/knot': Permission denied

@mweinelt
Copy link
Member Author

mweinelt commented Mar 12, 2019

The last commit adds tests, I adapted the test set already used by nsd.

  • server on 192.168.0.1/24 and fd00::1/64
    • knotd binds against IPv4 and IPv6 wildcards
    • provides the example.com and sub.example.com zones
    • sub.example.com is delegated in example.com to itself
  • client on 192.168.0.2/24 and fd00::2/64
    • interrogates knotd on IPv4 and IPv6 using khost
      • tests for missing A and AAAA
      • tests for SOA, NS, A and AAA

@mweinelt mweinelt marked this pull request as ready for review March 12, 2019 19:20
@mweinelt mweinelt requested a review from infinisil as a code owner March 12, 2019 19:20
@mweinelt
Copy link
Member Author

The last commit links the man pages into the system environment, thanks to @vcunat.

@mweinelt mweinelt force-pushed the knot/init branch 2 times, most recently from 4fb3f5c to a5f8a1a Compare March 13, 2019 00:13
@mweinelt
Copy link
Member Author

mweinelt commented Mar 13, 2019

I just tried enabling DNSSEC signing and the initialization failed because my zones currently reside in the nix store and Knot seems to want to write data in the same directory. I will probably need to symlink the zones somewhere below the StateDirectory instead.

Edit: Would probably a good idea to update the tests with a working DNSSEC example.

knotd[20282]: info: loading 1 zones
knotd[20282]: info: [_acme.example.com.] zone will be loaded
knotd[20282]: info: starting server
knotd[20282]: info: [_acme.example.com.] zone file parsed, serial 1552483525
knotd[20282]: error: [_acme.example.com.] DNSSEC, failed to initialize (Read-only file system)
knotd[20282]: error: [_acme.example.com.] zone event 'load' failed (Read-only file system)
knotd[20282]: info: server started in the foreground, PID 20282

@vcunat
Copy link
Member

vcunat commented Mar 13, 2019

Hmm, perhaps this approach? For a master server, that is. For slaves the preceding example would probably be suitable, but we haven't really discussed slaves here yet.

@mweinelt
Copy link
Member Author

mweinelt commented Mar 13, 2019

No, unfortunately that does not seem to change the behaviour.

Edit: Additionally moving {journal,timer,key}-db to /var/lib/knot did the trick!

template:
  - id: default
    storage: /nix/store/rrip306yqhjf221z5aaaak9g5ar8az7i-zonefiles
    dnssec-signing: on
    zonefile-sync: -1
    zonefile-load: difference
    journal-content: changes
    journal-db: /var/lib/knot/journal
    kasp-db: /var/lib/knot/kasp
    timer-db: /var/lib/knot/timer
Mar 13 17:05:25 juno knotd[27485]: info: starting server
Mar 13 17:05:25 juno knotd[27485]: info: [_acme.example.com.] zone file parsed, serial 1552493084
Mar 13 17:05:25 juno knotd[27485]: notice: [_acme.example.com.] DNSSEC, KSK submission, waiting for confirmation
Mar 13 17:05:25 juno knotd[27485]: info: [_acme.example.com.] DNSSEC, key, tag 20176, algorithm ECDSAP256SHA256, KSK, public, ready, active
Mar 13 17:05:25 juno knotd[27485]: info: [_acme.example.com.] DNSSEC, key, tag 30650, algorithm ECDSAP256SHA256, public, active
Mar 13 17:05:25 juno knotd[27485]: info: [_acme.example.com.] DNSSEC, signing started
Mar 13 17:05:25 juno knotd[27485]: info: [_acme.example.com.] DNSSEC, successfully signed
Mar 13 17:05:25 juno knotd[27485]: info: [_acme.example.com.] loaded, serial none -> 1552493084 -> 1552493085, 1375 bytes
Mar 13 17:05:25 juno knotd[27485]: info: [_acme.example.com.] DNSSEC, next signing at 2019-03-20T17:05:25
Mar 13 17:05:25 juno knotd[27485]: info: server started in the foreground, PID 27485
``

@mweinelt
Copy link
Member Author

mweinelt commented Mar 13, 2019

The tests now enable dnssec-signing with the workarounds mentioned in #56922 (comment). it is now checking for the existence of a DNSKEY record on the zone apex and a RRSIG record on the www resource record.

Edit: I'm just in the process of extending the tests to a master/slave setup, after which I would consider this pull request done.

@mweinelt
Copy link
Member Author

I just pushed the updated tests with the master/slave setup. Is there anything else?

Copy link
Member

@vcunat vcunat left a comment

Choose a reason for hiding this comment

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

I can't see anything else ATM. Let's reiterate if needed.

vcunat added a commit that referenced this pull request Mar 16, 2019
@vcunat vcunat merged commit a978d3d into NixOS:master Mar 16, 2019
@vcunat
Copy link
Member

vcunat commented Mar 25, 2019

Closely related: https://discourse.nixos.org/t/nix-dns-a-nix-dsl-for-dns-zone-files/2466

@nixos-discourse
Copy link

This pull request has been mentioned on Nix community. There might be relevant details there:

https://discourse.nixos.org/t/generating-yaml-in-a-nix-module/2519/5

@NixOS NixOS deleted a comment from nixos-discourse Mar 26, 2019
@vcunat vcunat mentioned this pull request Feb 8, 2020
10 tasks
@mweinelt mweinelt deleted the knot/init branch February 8, 2020 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants