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/openvpn: user, forwarding and local state dir #65878

Open
wants to merge 2 commits into
base: master
from

Conversation

@peterhoeg
Copy link
Member

commented Aug 3, 2019

Motivation for this change

We change a few things here:

a) Create the local openvpn user and group so that openvpn can drop privileges.
This is not switched on by default.

b) Define an option to enable IP forwarding which would have to be defined
outside of the openvpn module.

c) Create a local state and runtime directories for anything openvpn might create (replay
persistence logs, current connections, etc).

d) Launch openvpn in a separate openvpn.slice slice and activate from a dedicated openvpn.target target. The former allows one to easily check the logs across all instances, apply resource limitations and stop all instances. The latter allows you to easily start all the instances.

NOTE: I strongly recommend looking at this diff while ignoring whitespace due to the indent changes (options were indented further due to the introduction of a new option) - https://github.com/NixOS/nixpkgs/pull/65878/files?w=1

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 nix-review --run "nix-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.
Notify maintainers

cc @viric

@teto
teto approved these changes Aug 4, 2019
Copy link
Contributor

left a comment

looks good to me but I don't use openvpn.
can it work without net.ipv4.ip_forward ? or is like for firewalls and default to the secure version.
what about ipv6 ?

@peterhoeg

This comment has been minimized.

Copy link
Member Author

commented Aug 5, 2019

@peterhoeg peterhoeg force-pushed the peterhoeg:f/openvpn branch from 04e154f to 3c06ddf Aug 5, 2019

@teto

This comment has been minimized.

Copy link
Contributor

commented Aug 5, 2019

that sounds similar to the openFirewall option: you very likely need it but we cna't enable it by default. Aren't there some UDP ports that should be open as well ? would it make sense to make ports configurable (I believe that's common to avoid scans on specific ports) ?

@peterhoeg

This comment has been minimized.

Copy link
Member Author

commented Aug 5, 2019

@peterhoeg

This comment has been minimized.

Copy link
Member Author

commented Aug 5, 2019

@@ -211,6 +226,24 @@ in

boot.kernelModules = [ "tun" ];

boot.kernel.sysctl = lib.mkIf cfg.enableForwarding ({
"net.ipv4.ip_forward" = true;
} // (if config.networking.enableIPv6 then {

This comment has been minimized.

Copy link
@teto

teto Aug 5, 2019

Contributor

optionalAttrs ?

This comment has been minimized.

Copy link
@peterhoeg

peterhoeg Aug 5, 2019

Author Member

Indeed, thanks

@teto

This comment has been minimized.

Copy link
Contributor

commented Aug 5, 2019

I just wanted to mention that since it's options I would typically expect in network-related modules. It's completely fine if it's too hard to implement or too convoluted, you know better :p

@peterhoeg

This comment has been minimized.

Copy link
Member Author

commented Aug 5, 2019

It's completely fine if it's too hard to implement or too convoluted, you know better :p

I fully with you - it makes sense to have this type of stuff standard across the board but here I think it would just add complexity for not a lot of gain.

nixos/openvpn: user, forwarding and local state dir
We change a few things here:

a) Create the local openvpn user and group so that openvpn can drop privileges.
This is not switched on by default.

b) Define an option to enable IP forwarding which would have to be defined
outside of the openvpn module.

c) Create a local state and runtime directories for anything openvpn might create (replay
persistence logs, current connections, etc).

@peterhoeg peterhoeg force-pushed the peterhoeg:f/openvpn branch from 3c06ddf to d32f9c3 Aug 5, 2019

@mmahut mmahut referenced this pull request Aug 19, 2019
5 of 10 tasks complete
@ivan

This comment has been minimized.

Copy link
Member

commented Sep 6, 2019

For the author, reviewers, and committers: this PR was scanned and appears to add a use of the deprecated types.string, which emits a warning as of #66346. Before merging, please change this to another type, possibly:

  • types.str for a single string where merging does not make sense, or cannot work
  • types.lines for multi-line configuration or scripts where merging is possible
  • types.listOf types.str for a mergeable list of strings

@lheckemann lheckemann added this to the 20.03 milestone Sep 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.