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/unbound: add enableRemoteAccess option #79559

Open
wants to merge 1 commit into
base: master
from

Conversation

@kraem
Copy link
Contributor

@kraem kraem commented Feb 8, 2020

Motivation for this change

unbound-control which is installed with package unbound when activating the module wasn't able to access the server because it was looking for the config file in /etc so I thought I'd make options to enable this more easily.

Things done
  • Implemented an option enableRemoteAccess, an option that sets remote-control setting to true in unbound.conf
  • The option also generates key + certificate and listens to 127.0.0.1 and ::1 by default. The interface + port can be altered with the other options remoteAccessInterfaces and remoteAccessPort for remote access.
  • Wrapped unbound-control and unbound-checkconf so it points to the config file in stateDir instead of the packages default configuration dir in /etc
    • I don't know if that's what these lines are supposed to do. Somehow unbound-anchor say the default dir for root key fil is under /nix/store
Thoughts

I added the wrapping to systemPackages because I added unbound-checkconf to the wrapping.
I could change this to

    environment.systemPackages = [ cfg.package ] 
      ++ optionals cfg.enableRemoteAccess [ (hiPrio unboundWrapped) ];

if that's better.

I am happy for pointers and tips of course :)

Checkboxes
  • 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.
@kraem kraem force-pushed the kraem:nixos/unbound branch from 4c74bce to 30e36eb Feb 13, 2020
@kraem kraem closed this Mar 21, 2020
@kraem kraem deleted the kraem:nixos/unbound branch Mar 21, 2020
@kraem kraem restored the kraem:nixos/unbound branch Apr 28, 2020
@kraem kraem reopened this Apr 28, 2020
installPhase = ''
mkdir -p "$out/bin"
makeWrapper ${pkgs.unbound}/bin/unbound-control $out/bin/unbound-control \
--add-flags "-c ${stateDir}/unbound.conf"
makeWrapper ${pkgs.unbound}/bin/unbound-checkconf $out/bin/unbound-checkconf \
--add-flags "${stateDir}/unbound.conf"
'';
Comment on lines +51 to +57

This comment has been minimized.

@lovesegfault

lovesegfault Apr 28, 2020
Contributor

Very nice, I was supremely annoyed with the previous behavior.

server-key-file: ${stateDir}/unbound_server.key
server-cert-file: ${stateDir}/unbound_server.pem
control-key-file: ${stateDir}/unbound_control.key
control-cert-file: ${stateDir}/unbound_control.pem
Comment on lines +21 to +24

This comment has been minimized.

@lovesegfault

lovesegfault Apr 28, 2020
Contributor

Are these going to be correctly auto-gen'd when the cfg is enabled?

This comment has been minimized.

@kraem

kraem Apr 29, 2020
Author Contributor

Just verified this since it was a long time since I wrote this and it works for me 👍

Option that 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.
@kraem kraem force-pushed the kraem:nixos/unbound branch from 30e36eb to 30b1271 Apr 29, 2020
@kraem kraem requested a review from lovesegfault Apr 29, 2020
};

remoteAccessInterfaces = mkOption {
default = [ "127.0.0.1" ] ++ optional config.networking.enableIPv6 "::1";

This comment has been minimized.

@kraem

kraem Apr 29, 2020
Author Contributor

Added check for enableIPv6

@kraem kraem requested review from ehmry and globin Apr 29, 2020
@ehmry
ehmry approved these changes Apr 30, 2020
@rissson
Copy link
Contributor

@rissson rissson commented Jun 5, 2020

I'd rather we went with a design as described in RFC 42 for this module. I guess we can keep the existing options and build on top of that. I'll try something and mention it here.

@@ -126,11 +188,12 @@ in
''}
touch ${stateDir}/dev/random
${pkgs.utillinux}/bin/mount --bind -n /dev/urandom ${stateDir}/dev/random
${optionalString cfg.enableRemoteAccess "${pkgs.unbound}/bin/unbound-control-setup -d ${stateDir}"}

This comment has been minimized.

@rissson

rissson Jun 5, 2020
Contributor

Shouldn't this be cfg.package?

This comment has been minimized.

@kraem

kraem Jun 12, 2020
Author Contributor

Honestly don't know. Saw your new PR, gj 👍. Keeping this one open til your is merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.