@rasendubi, thanks for your PR! By analyzing the history of the files in this pull request, we identified @kampfschlaefer, @edolstra and @ericbmerritt to be potential reviewers.

Thanks! I've checked this patch on top of 6751f94

enableTun = true; is crucial for the following NixOps configuration to work:

{

  vbox = { config, pkgs, lib, nodes, ...}: {
    deployment.targetEnv = "virtualbox";
    deployment.virtualbox.headless = true;

    environment.systemPackages = [
      pkgs.traceroute
      pkgs.telnet
    ];

    containers.c-analy = {
      autoStart = true;
      bindMounts."/var/lib/containers".hostPath = "/var/lib/containers";
      privateNetwork = true;
      hostAddress = "192.168.33.1";
      localAddress = "192.168.33.2";
      enableTun = true;
      config = {
        environment.systemPackages = [
          pkgs.traceroute
          pkgs.telnet
        ];
        networking.extraHosts = ''
          192.168.56.101 vbox vbox-unencrypted
          192.168.56.102 vboxServer vboxServer-unencrypted
        '';

        services.openvpn.servers.client.autoStart = false;
        services.openvpn.servers.client.config = ''
            remote vboxServer 1194
            dev tun
            proto tcp-client
            ifconfig 10.8.0.2 10.8.0.1
            secret /root/static.key
            verb 10
        '';
        services.postgresql.enable = true;
      };
    };

    networking.nat.enable = true;
    networking.nat.internalInterfaces = ["ve-+"];
    networking.nat.externalInterface = "enp0s8";

    system.stateVersion = "16.09";
  };

  vboxServer = { config, pkgs, lib, nodes, ...}: {
    deployment.targetEnv = "virtualbox";
    deployment.virtualbox.headless = true;

    environment.systemPackages = [
      pkgs.traceroute
      pkgs.telnet
    ];

    services.openvpn = {
      servers.main.autoStart = false;
      servers.main.config = ''
        dev tun
        proto tcp-server
        ifconfig 10.8.0.1 10.8.0.2
        secret /root/static.key
        port 1194
      '';
    };
    networking.firewall.allowedTCPPorts = [ 1194 ];
    system.stateVersion = "16.09";
  };

}

Hi,

I'm not sure this is a change eligible for backporting to stable, however perhaps so. Usually the only backports are security fixes and major bugfixes. This one seems a bit more minor / enhancement category.

That said, I'm not any sort of final decider on that, so I'll ping some people.

Thank you!

The bug makes openvpn unusable inside containers. I believe that's pretty major for some people.

Correct me if I'm wrong, but #18822 added the ability to run openvpn in containers, and before that it was impossible. In that sense, it is a feature addition and not a bug fix. In fact, the features being added are:

• Specify arbitrary capabilities
• Specify arbitrary devices available

While I hear the pain from not having these features, in that you can't run openvpn in the container, it is similar to saying it is a bug that you cannot run frotz in stable either, which is a new package introduced in unstable.

It is also a fairly large and invasive change. This is pretty tough to backport to our stable release, as we very much want to keep it stable. It would require extensive and careful testing to port.

@grahamc Actually, I'm the person who requested this PR. I have openVPN in containers in production on nixos-16.03

I guess the regression was introduced by fd5bbdb, which ended up in 16.09

Some guys fixed that by introducing "an ability" to enable openvpn back (which is this PR about). In some sense the fix introduces new features, I agree here, but enableTun = true; fixes regression.

To be pedantic, this should be in release notes too. But I guess those little people affected can google the problem.

cc @domenkozar could I get your eyes on this?

I'm very vague about that part of our ecosystem and I won't have time to dig into this soon. Maybe @edolstra has thoughts?

@grahamc security

I want to weight in here:
For me this is not a regression for one very simple reason: There is no automated test that proves that it now works and didn't work before. If there was such a test here in this PR (or in its original on master branch), one could backport that to 16.03 and prove that it worked there and got broken. No test, no prove, no reason for me to approve this PR.

Of course maintainers might think otherwise. But I would really, really love to see a test for this feature and maybe also for openvpn in general. NixOS/nixpkgs provides an excellent infrastructure to set up these kind of tests. We should use it and maybe enforce usage of it. And if the test infrastructure is not up to par: the more users, the more fixers.

@kampfschlaefer I agree with you, that there should be more tests. I've adapted the above nixops deployment as NixOS test

Please, test as following:

# set up a basic VPN between server and container in client and ping container from server
$ NIX_PATH=nixpkgs=https://nixos.org/channels/nixos-16.03/nixexprs.tar.xz nix-build proof.nix   # WORKS
$ NIX_PATH=nixpkgs=https://nixos.org/channels/nixos-16.09/nixexprs.tar.xz nix-build proof.nix   # doesn't work 
$ NIX_PATH=nixpkgs=https://nixos.org/channels/nixos-unstable/nixexprs.tar.xz nix-build proof.nix   # doesn't work 

This proves the regression. In similar manner it is easy to show, that adding nodes.client.containers.c-analy.enableTun = true; will fix the test for nixos-unstable.

The proof.nix file

import <nixpkgs/nixos/tests/make-test.nix> ({ pkgs, ...} :

let key = pkgs.runCommand "static.key" {} ''
  ${pkgs.openvpn}/bin/openvpn --genkey --secret $out
'';
in {
  name = "vpn-in-container";

  nodes.server = { config, pkgs, lib, nodes, ...}: {
    virtualisation.vlans = [ 1 ];
    networking.firewall.allowedTCPPorts = [ 1194 ];
    environment.etc.deploy-key.source = key;
    services.openvpn.servers.main = {
      autoStart = false;
      config = ''
        dev tun
        proto tcp-server
        ifconfig 10.8.0.1 10.8.0.2
        secret /root/static.key
        port 1194
      '';
    };
   };

  nodes.client = { config, pkgs, lib, nodes, ...}: {
    virtualisation.vlans = [ 1 ];
    networking.nat.enable = true;
    networking.nat.internalInterfaces = ["ve-+"];
    networking.nat.externalInterface = "eth1";
    environment.etc.deploy-key.source = key;

    containers.c-analy = {
      autoStart = true;
      privateNetwork = true;
      hostAddress = "192.168.33.1";
      localAddress = "192.168.33.2";
      config = {
        networking.extraHosts = ''
          ${(pkgs.lib.head nodes.server.config.networking.interfaces.eth1.ip4).address} vpn-server
        '';

        services.openvpn.servers.client.autoStart = false;
        services.openvpn.servers.client.config = ''
            remote vpn-server 1194
            dev tun
            proto tcp-client
            ifconfig 10.8.0.2 10.8.0.1
            secret ${key}
        '';
      };
    };
  };

  testScript =
   let clientKeyPath = "/var/lib/containers/c-analy/root/static.key"; in
      ''
        #startAll;
        $server->start;
        $client->start;
        $server->sleep(10);
        $server->succeed("cat /etc/deploy-key > /root/static.key && chown root:root /root/static.key && chmod 600 /root/static.key");
        $server->succeed("systemctl start openvpn-main.service &");
        $server->waitUntilSucceeds("ping -c 1 10.8.0.1");

        $client->succeed("cat /etc/deploy-key > ${clientKeyPath} && chown root:root ${clientKeyPath} && chmod 600 ${clientKeyPath}");
        $client->succeed("systemctl start openvpn-client.service -M c-analy &");
        $client->waitUntilSucceeds("nixos-container run c-analy -- ping -c 1 vpn-server");

        $server->succeed("ping -c 1 10.8.0.2");
      '';
  })

From my point of view this is in fact a regression. Yes, there was no test covering this functionality, which is indeed very unfortunate. But it was trivially & without hacks possible through the NixOS module system to enable and use the openvpn service in a NixOS container. This is not only a problem for VPN daemons but all services requiring special device nodes.

We've thus removed useful and expectable functionality in 16.09 compared to 16.03 without and an easy and natural way to override these restrictions using the module system. Classical regression IMHO.

Thanks for the test! LGTM in principle, could you please open another PR for that?

This commit does indeed touch a lot of chunks of code in the container code which also makes me feel a bit uneasy. I've checked the history of nixos/modules/virtualisation/containers.nix until the 16.09 branchoff and made the following observations:

• There were some cleanups to the code that is to be backported to 16.09 here. Specifically 69713a8, a67b597, b6023f3. As we didn't deprecate optionSet in 16.09 we don't need to backport as they provide no functional changes.
• Cherry-picking Allow access to /dev/net/tun inside containers #18822 from master into 16.09 (which this PR does) yields no conflicts. 🎉
• Before that PR, only b9df84c with a fix d6ce2e4 changed code in containers.nix. These are completely independent of the other changes and can thus be ignored in the context of this PR.

Therefore I think it's safe and useful to backport this to 16.09. 👍

@danbst, could you please open a PR for adding the test to NixOS test suite?

Sorry I missed this for so long...!