From 52f6733203397c79b866b0b4f7ee33b952414e2e Mon Sep 17 00:00:00 2001
From: Marc 'risson' Schmitt <marc.schmitt@risson.space>
Date: Sat, 8 Feb 2020 20:17:23 +0100
Subject: [PATCH] nixos/unbound: deprecate extraConfig in favor of settings

Follow RFC 42 by having a settings option that is
then converted into an unbound configuration file
instead of having an extraConfig option.

Existing options have been renamed or kept if
possible.

An enableRemoteAccess has been added. It sets remote-control setting to
true in unbound.conf which in turn enables the new wrapping of
unbound-control to access the server locally.  Also includes options
'remoteAccessInterfaces' and 'remoteAccessPort' for remote access.

Signed-off-by: Marc 'risson' Schmitt <marc.schmitt@risson.space>
---
 nixos/doc/manual/release-notes/rl-2105.xml    |  17 ++
 nixos/modules/services/networking/unbound.nix | 249 ++++++++++++------
 nixos/tests/unbound.nix                       |  68 +++--
 3 files changed, 224 insertions(+), 110 deletions(-)

diff --git a/nixos/doc/manual/release-notes/rl-2105.xml b/nixos/doc/manual/release-notes/rl-2105.xml
index 2b0a265cd98..e3e6dc48433 100644
--- a/nixos/doc/manual/release-notes/rl-2105.xml
+++ b/nixos/doc/manual/release-notes/rl-2105.xml
@@ -829,6 +829,23 @@ environment.systemPackages = [
      default in the CLI tooling which in turn enables us to use
      <literal>unbound-control</literal> without passing a custom configuration location.
     </para>
+
+    <para>
+     The module has also been reworked to be <link
+     xlink:href="https://github.com/NixOS/rfcs/blob/master/rfcs/0042-config-option.md">RFC
+     0042</link> compliant. As such,
+     <option>sevices.unbound.extraConfig</option> has been removed and replaced
+     by <xref linkend="opt-services.unbound.settings"/>. <option>services.unbound.interfaces</option>
+     has been renamed to <option>services.unbound.settings.server.interface</option>.
+    </para>
+
+    <para>
+     <option>services.unbound.forwardAddresses</option> and
+     <option>services.unbound.allowedAccess</option> have also been changed to
+     use the new settings interface. You can follow the instructions when
+     executing <literal>nixos-rebuild</literal> to upgrade your configuration to
+     use the new interface.
+    </para>
    </listitem>
    <listitem>
     <para>
diff --git a/nixos/modules/services/networking/unbound.nix b/nixos/modules/services/networking/unbound.nix
index 622c3d8ea43..a8747e244a9 100644
--- a/nixos/modules/services/networking/unbound.nix
+++ b/nixos/modules/services/networking/unbound.nix
@@ -4,51 +4,28 @@ with lib;
 let
   cfg = config.services.unbound;
 
-  stateDir = "/var/lib/unbound";
+  yesOrNo = v: if v then "yes" else "no";
 
-  access = concatMapStringsSep "\n  " (x: "access-control: ${x} allow") cfg.allowedAccess;
+  toOption = indent: n: v: "${indent}${toString n}: ${v}";
 
-  interfaces = concatMapStringsSep "\n  " (x: "interface: ${x}") cfg.interfaces;
+  toConf = indent: n: v:
+    if builtins.isFloat v then (toOption indent n (builtins.toJSON v))
+    else if isInt v       then (toOption indent n (toString v))
+    else if isBool v      then (toOption indent n (yesOrNo v))
+    else if isString v    then (toOption indent n v)
+    else if isList v      then (concatMapStringsSep "\n" (toConf indent n) v)
+    else if isAttrs v     then (concatStringsSep "\n" (
+                                  ["${indent}${n}:"] ++ (
+                                    mapAttrsToList (toConf "${indent}  ") v
+                                  )
+                                ))
+    else throw (traceSeq v "services.unbound.settings: unexpected type");
 
-  isLocalAddress = x: substring 0 3 x == "::1" || substring 0 9 x == "127.0.0.1";
+  confFile = pkgs.writeText "unbound.conf" (concatStringsSep "\n" ((mapAttrsToList (toConf "") cfg.settings) ++ [""]));
 
-  forward =
-    optionalString (any isLocalAddress cfg.forwardAddresses) ''
-      do-not-query-localhost: no
-    ''
-    + optionalString (cfg.forwardAddresses != []) ''
-      forward-zone:
-        name: .
-    ''
-    + concatMapStringsSep "\n" (x: "    forward-addr: ${x}") cfg.forwardAddresses;
+  rootTrustAnchorFile = "${cfg.stateDir}/root.key";
 
-  rootTrustAnchorFile = "${stateDir}/root.key";
-
-  trustAnchor = optionalString cfg.enableRootTrustAnchor
-    "auto-trust-anchor-file: ${rootTrustAnchorFile}";
-
-  confFile = pkgs.writeText "unbound.conf" ''
-    server:
-      ip-freebind: yes
-      directory: "${stateDir}"
-      username: unbound
-      chroot: ""
-      pidfile: ""
-      # when running under systemd there is no need to daemonize
-      do-daemonize: no
-      ${interfaces}
-      ${access}
-      ${trustAnchor}
-    ${lib.optionalString (cfg.localControlSocketPath != null) ''
-      remote-control:
-        control-enable: yes
-        control-interface: ${cfg.localControlSocketPath}
-    ''}
-    ${cfg.extraConfig}
-    ${forward}
-  '';
-in
-{
+in {
 
   ###### interface
 
@@ -64,27 +41,32 @@ in
         description = "The unbound package to use";
       };
 
-      allowedAccess = mkOption {
-        default = [ "127.0.0.0/24" ];
-        type = types.listOf types.str;
-        description = "What networks are allowed to use unbound as a resolver.";
+      user = mkOption {
+        type = types.str;
+        default = "unbound";
+        description = "User account under which unbound runs.";
       };
 
-      interfaces = mkOption {
-        default = [ "127.0.0.1" ] ++ optional config.networking.enableIPv6 "::1";
-        type = types.listOf types.str;
-        description =  ''
-          What addresses the server should listen on. This supports the interface syntax documented in
-          <citerefentry><refentrytitle>unbound.conf</refentrytitle><manvolnum>8</manvolnum></citerefentry>.
+      group = mkOption {
+        type = types.str;
+        default = "unbound";
+        description = "Group under which unbound runs.";
+      };
+
+      stateDir = mkOption {
+        default = "/var/lib/unbound";
+        description = "Directory holding all state for unbound to run.";
+      };
+
+      resolveLocalQueries = mkOption {
+        type = types.bool;
+        default = true;
+        description = ''
+          Whether unbound should resolve local queries (i.e. add 127.0.0.1 to
+          /etc/resolv.conf).
         '';
       };
 
-      forwardAddresses = mkOption {
-        default = [];
-        type = types.listOf types.str;
-        description = "What servers to forward queries to.";
-      };
-
       enableRootTrustAnchor = mkOption {
         default = true;
         type = types.bool;
@@ -106,23 +88,66 @@ in
           and group will be <literal>nogroup</literal>.
 
           Users that should be permitted to access the socket must be in the
-          <literal>unbound</literal> group.
+          <literal>config.services.unbound.group</literal> group.
 
           If this option is <literal>null</literal> remote control will not be
-          configured at all. Unbounds default values apply.
+          enabled. Unbounds default values apply.
         '';
       };
 
-      extraConfig = mkOption {
-        default = "";
-        type = types.lines;
+      settings = mkOption {
+        default = {};
+        type = with types; submodule {
+
+          freeformType = let
+            validSettingsPrimitiveTypes = oneOf [ int str bool float ];
+            validSettingsTypes = oneOf [ validSettingsPrimitiveTypes (listOf validSettingsPrimitiveTypes) ];
+            settingsType = (attrsOf validSettingsTypes);
+          in attrsOf (oneOf [ string settingsType (listOf settingsType) ])
+              // { description = ''
+                unbound.conf configuration type. The format consist of an attribute
+                set of settings. Each settings can be either one value, a list of
+                values or an attribute set. The allowed values are integers,
+                strings, booleans or floats.
+              '';
+            };
+
+          options = {
+            remote-control.control-enable = mkOption {
+              type = bool;
+              default = false;
+              internal = true;
+            };
+          };
+        };
+        example = literalExample ''
+          {
+            server = {
+              interface = [ "127.0.0.1" ];
+            };
+            forward-zone = [
+              {
+                name = ".";
+                forward-addr = "1.1.1.1@853#cloudflare-dns.com";
+              }
+              {
+                name = "example.org.";
+                forward-addr = [
+                  "1.1.1.1@853#cloudflare-dns.com"
+                  "1.0.0.1@853#cloudflare-dns.com"
+                ];
+              }
+            ];
+            remote-control.control-enable = true;
+          };
+        '';
         description = ''
-          Extra unbound config. See
-          <citerefentry><refentrytitle>unbound.conf</refentrytitle><manvolnum>8
-          </manvolnum></citerefentry>.
+          Declarative Unbound configuration
+          See the <citerefentry><refentrytitle>unbound.conf</refentrytitle>
+          <manvolnum>5</manvolnum></citerefentry> manpage for a list of
+          available options.
         '';
       };
-
     };
   };
 
@@ -130,23 +155,56 @@ in
 
   config = mkIf cfg.enable {
 
-    environment.systemPackages = [ cfg.package ];
-
-    users.users.unbound = {
-      description = "unbound daemon user";
-      isSystemUser = true;
-      group = lib.mkIf (cfg.localControlSocketPath != null) (lib.mkDefault "unbound");
+    services.unbound.settings = {
+      server = {
+        directory = mkDefault cfg.stateDir;
+        username = cfg.user;
+        chroot = ''""'';
+        pidfile = ''""'';
+        # when running under systemd there is no need to daemonize
+        do-daemonize = false;
+        interface = mkDefault ([ "127.0.0.1" ] ++ (optional config.networking.enableIPv6 "::1"));
+        access-control = mkDefault ([ "127.0.0.0/8 allow" ] ++ (optional config.networking.enableIPv6 "::1/128 allow"));
+        auto-trust-anchor-file = mkIf cfg.enableRootTrustAnchor rootTrustAnchorFile;
+        tls-cert-bundle = mkDefault "/etc/ssl/certs/ca-certificates.crt";
+        # prevent race conditions on system startup when interfaces are not yet
+        # configured
+        ip-freebind = mkDefault true;
+      };
+      remote-control = {
+        control-enable = mkDefault false;
+        control-interface = mkDefault ([ "127.0.0.1" ] ++ (optional config.networking.enableIPv6 "::1"));
+        server-key-file = mkDefault "${cfg.stateDir}/unbound_server.key";
+        server-cert-file = mkDefault "${cfg.stateDir}/unbound_server.pem";
+        control-key-file = mkDefault "${cfg.stateDir}/unbound_control.key";
+        control-cert-file = mkDefault "${cfg.stateDir}/unbound_control.pem";
+      } // optionalAttrs (cfg.localControlSocketPath != null) {
+        control-enable = true;
+        control-interface = cfg.localControlSocketPath;
+      };
     };
 
-    # We need a group so that we can give users access to the configured
-    # control socket. Unbound allows access to the socket only to the unbound
-    # user and the primary group.
-    users.groups = lib.mkIf (cfg.localControlSocketPath != null) {
+    environment.systemPackages = [ cfg.package ];
+
+    users.users = mkIf (cfg.user == "unbound") {
+      unbound = {
+        description = "unbound daemon user";
+        isSystemUser = true;
+        group = cfg.group;
+      };
+    };
+
+    users.groups = mkIf (cfg.group == "unbound") {
       unbound = {};
     };
 
-    networking.resolvconf.useLocalResolver = mkDefault true;
+    networking = mkIf cfg.resolveLocalQueries {
+      resolvconf = {
+        useLocalResolver = mkDefault true;
+      };
 
+      networkmanager.dns = "unbound";
+    };
 
     environment.etc."unbound/unbound.conf".source = confFile;
 
@@ -156,8 +214,15 @@ in
       before = [ "nss-lookup.target" ];
       wantedBy = [ "multi-user.target" "nss-lookup.target" ];
 
-      preStart = lib.mkIf cfg.enableRootTrustAnchor ''
-        ${cfg.package}/bin/unbound-anchor -a ${rootTrustAnchorFile} || echo "Root anchor updated!"
+      path = mkIf cfg.settings.remote-control.control-enable [ pkgs.openssl ];
+
+      preStart = ''
+        ${optionalString cfg.enableRootTrustAnchor ''
+          ${cfg.package}/bin/unbound-anchor -a ${rootTrustAnchorFile} || echo "Root anchor updated!"
+        ''}
+        ${optionalString cfg.settings.remote-control.control-enable ''
+          ${cfg.package}/bin/unbound-control-setup -d ${cfg.stateDir}
+        ''}
       '';
 
       restartTriggers = [
@@ -181,8 +246,8 @@ in
           "CAP_SYS_RESOURCE"
         ];
 
-        User = "unbound";
-        Group = lib.mkIf (cfg.localControlSocketPath != null) (lib.mkDefault "unbound");
+        User = cfg.user;
+        Group = cfg.group;
 
         MemoryDenyWriteExecute = true;
         NoNewPrivileges = true;
@@ -211,9 +276,29 @@ in
         RestrictNamespaces = true;
         LockPersonality = true;
         RestrictSUIDSGID = true;
+
+        Restart = "on-failure";
+        RestartSec = "5s";
       };
     };
-    # If networkmanager is enabled, ask it to interface with unbound.
-    networking.networkmanager.dns = "unbound";
   };
+
+  imports = [
+    (mkRenamedOptionModule [ "services" "unbound" "interfaces" ] [ "services" "unbound" "settings" "server" "interface" ])
+    (mkChangedOptionModule [ "services" "unbound" "allowedAccess" ] [ "services" "unbound" "settings" "server" "access-control" ] (
+      config: map (value: "${value} allow") (getAttrFromPath [ "services" "unbound" "allowedAccess" ] config)
+    ))
+    (mkRemovedOptionModule [ "services" "unbound" "forwardAddresses" ] ''
+      Add a new setting:
+      services.unbound.settings.forward-zone = [{
+        name = ".";
+        forward-addr = [ # Your current services.unbound.forwardAddresses ];
+      }];
+      If any of those addresses are local addresses (127.0.0.1 or ::1), you must
+      also set services.unbound.settings.server.do-not-query-localhost to false.
+    '')
+    (mkRemovedOptionModule [ "services" "unbound" "extraConfig" ] ''
+      You can use services.unbound.settings to add any configuration you want.
+    '')
+  ];
 }
diff --git a/nixos/tests/unbound.nix b/nixos/tests/unbound.nix
index ca9718ac633..e24c3ef6c99 100644
--- a/nixos/tests/unbound.nix
+++ b/nixos/tests/unbound.nix
@@ -61,13 +61,16 @@ import ./make-test-python.nix ({ pkgs, lib, ... }:
 
         services.unbound = {
           enable = true;
-          interfaces = [ "192.168.0.1" "fd21::1" "::1" "127.0.0.1" ];
-          allowedAccess = [ "192.168.0.0/24" "fd21::/64" "::1" "127.0.0.0/8" ];
-          extraConfig = ''
-            server:
-              local-data: "example.local. IN A 1.2.3.4"
-              local-data: "example.local. IN AAAA abcd::eeff"
-          '';
+          settings = {
+            server = {
+              interface = [ "192.168.0.1" "fd21::1" "::1" "127.0.0.1" ];
+              access-control = [ "192.168.0.0/24 allow" "fd21::/64 allow" "::1 allow" "127.0.0.0/8 allow" ];
+              local-data = [
+                ''"example.local. IN A 1.2.3.4"''
+                ''"example.local. IN AAAA abcd::eeff"''
+              ];
+            };
+          };
         };
       };
 
@@ -90,19 +93,25 @@ import ./make-test-python.nix ({ pkgs, lib, ... }:
 
         services.unbound = {
           enable = true;
-          allowedAccess = [ "192.168.0.0/24" "fd21::/64" "::1" "127.0.0.0/8" ];
-          interfaces = [ "::1" "127.0.0.1" "192.168.0.2" "fd21::2"
-                         "192.168.0.2@853" "fd21::2@853" "::1@853" "127.0.0.1@853"
-                         "192.168.0.2@443" "fd21::2@443" "::1@443" "127.0.0.1@443" ];
-          forwardAddresses = [
-            (lib.head nodes.authoritative.config.networking.interfaces.eth1.ipv6.addresses).address
-            (lib.head nodes.authoritative.config.networking.interfaces.eth1.ipv4.addresses).address
-          ];
-          extraConfig = ''
-            server:
-              tls-service-pem: ${cert}/cert.pem
-              tls-service-key: ${cert}/key.pem
-          '';
+          settings = {
+            server = {
+              interface = [ "::1" "127.0.0.1" "192.168.0.2" "fd21::2"
+                            "192.168.0.2@853" "fd21::2@853" "::1@853" "127.0.0.1@853"
+                            "192.168.0.2@443" "fd21::2@443" "::1@443" "127.0.0.1@443" ];
+              access-control = [ "192.168.0.0/24 allow" "fd21::/64 allow" "::1 allow" "127.0.0.0/8 allow" ];
+              tls-service-pem = "${cert}/cert.pem";
+              tls-service-key = "${cert}/key.pem";
+            };
+            forward-zone = [
+              {
+                name = ".";
+                forward-addr = [
+                  (lib.head nodes.authoritative.config.networking.interfaces.eth1.ipv6.addresses).address
+                  (lib.head nodes.authoritative.config.networking.interfaces.eth1.ipv4.addresses).address
+                ];
+              }
+            ];
+          };
         };
       };
 
@@ -122,12 +131,14 @@ import ./make-test-python.nix ({ pkgs, lib, ... }:
 
         services.unbound = {
           enable = true;
-          allowedAccess = [ "::1" "127.0.0.0/8" ];
-          interfaces = [ "::1" "127.0.0.1" ];
+          settings = {
+            server = {
+              interface = [ "::1" "127.0.0.1" ];
+              access-control = [ "::1 allow" "127.0.0.0/8 allow" ];
+            };
+            include = "/etc/unbound/extra*.conf";
+          };
           localControlSocketPath = "/run/unbound/unbound.ctl";
-          extraConfig = ''
-            include: "/etc/unbound/extra*.conf"
-          '';
         };
 
         users.users = {
@@ -143,12 +154,13 @@ import ./make-test-python.nix ({ pkgs, lib, ... }:
           unauthorizeduser = { isSystemUser = true; };
         };
 
+        # Used for testing configuration reloading
         environment.etc = {
           "unbound-extra1.conf".text = ''
             forward-zone:
-              name: "example.local."
-              forward-addr: ${(lib.head nodes.resolver.config.networking.interfaces.eth1.ipv6.addresses).address}
-              forward-addr: ${(lib.head nodes.resolver.config.networking.interfaces.eth1.ipv4.addresses).address}
+            name: "example.local."
+            forward-addr: ${(lib.head nodes.resolver.config.networking.interfaces.eth1.ipv6.addresses).address}
+            forward-addr: ${(lib.head nodes.resolver.config.networking.interfaces.eth1.ipv4.addresses).address}
           '';
           "unbound-extra2.conf".text = ''
             auth-zone: