Navigation Menu

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

Jump to bottom

WIP: networking.services: Add vpnc module #77892

Closed
wants to merge 1 commit into from
Closed

WIP: networking.services: Add vpnc module #77892

wants to merge 1 commit into from

Conversation

MasseR
Copy link
Contributor

@MasseR MasseR commented Jan 17, 2020

Motivation for this change

Related to the issue #77891, we have been using this vpnc module definition for a while now.

This MR creates a new services.networking.vpnc attributeset, for setting up a vpnc service and systemd definition (the latter was missing in the original module definition).

Things done
  • 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.

@MasseR
Copy link
Contributor Author

MasseR commented Jan 17, 2020

Like mentioned above, I have been using this definition for a while now, but it's probably not super polished, so keep that in mind when reviewing

@flokli flokli requested review from fpletz and globin January 17, 2020 18:36
flokli
flokli requested changes Jan 17, 2020
View reviewed changes
Copy link
Contributor

@flokli flokli left a comment

Choose a reason for hiding this comment

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

Also, see #74035 for the reasons this was dropped.

nixos/modules/services/networking/vpnc.nix
};
};
up = pkgs.writeScript "up" ''
#!${pkgs.bash}/bin/bash
Copy link
Contributor

Choose a reason for hiding this comment

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

stdenv.shell

Copy link
Member

Choose a reason for hiding this comment

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

Even better is pkgs.runtimeShell. stdenv.shell is not correct when cross-compiling

nixos/modules/services/networking/vpnc.nix
};
up = pkgs.writeScript "up" ''
#!${pkgs.bash}/bin/bash
PATH=${makeBinPath [pkgs.nettools pkgs.iproute]}:$PATH
Copy link
Contributor

Choose a reason for hiding this comment

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

please set path on the unit.

nixos/modules/services/networking/vpnc.nix
LOCAL_GATEWAY=$(ip r | grep "default via" | grep -oE "([0-9]+\.[0-9]+\.[0-9]+\.[0-9]+)" | head -n1)
ip route del default via $LOCAL_GATEWAY
ip route add default dev tun0 proto static scope link metric 50
if [ ! -d /var/run/vpnc ]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

/var/run is deprecated. Also, use RuntimeDirectory=vpnc to create that directory

nixos/modules/services/networking/vpnc.nix
echo -ne $LOCAL_GATEWAY > /var/run/vpnc/LOCAL_GATEWAY
'';
down = pkgs.writeScript "down" ''
#!${pkgs.bash}/bin/bash
Copy link
Contributor

Choose a reason for hiding this comment

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

stdenv.shell, same as above.

Copy link
Member

Choose a reason for hiding this comment

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

pkgs.runtimeShell here too

nixos/modules/services/networking/vpnc.nix
'';
down = pkgs.writeScript "down" ''
#!${pkgs.bash}/bin/bash
PATH=${makeBinPath [pkgs.nettools pkgs.iproute]}:$PATH
Copy link
Contributor

Choose a reason for hiding this comment

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

set path in the unit, same as above

nixos/modules/services/networking/vpnc.nix
path = [ pkgs.iptables pkgs.nettools pkgs.iproute ];
serviceConfig = {
ExecStart = "${pkgs.vpnc}/bin/vpnc /etc/vpnc/${key}.conf";
ExecStartPost = "${up}";
Copy link
Contributor

Choose a reason for hiding this comment

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

you can pass up directly, without doing the string interpolation,

nixos/modules/services/networking/vpnc.nix
secret = mkOption {
default = "";
type = types.separatedString "";
description = "Your shared secret";
Copy link
Contributor

Choose a reason for hiding this comment

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

This shouldn't be part of the nix store, but instead a path where things are read from at runtime.

Maybe it's better to do that for the whole config file - I assume ipsec supports a lot of more options as well…

Copy link
Contributor

Choose a reason for hiding this comment

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

I second that. Never store secrets in the Nix store (unless hashed). Better point to a file.

@flokli flokli requested a review from ckauhaus January 17, 2020 18:45
@MasseR
Copy link
Contributor Author

MasseR commented Jan 23, 2020

Thank you for the comments. I'll mark this as WIP until I get these implemented

@MasseR MasseR changed the title networking.services: Add vpnc module WIP: networking.services: Add vpnc module Jan 23, 2020
@flokli
Copy link
Contributor

flokli commented Feb 10, 2020

@MasseR do you plan to address these remarks in the near future? Otherwise, I'd just propose to close the PR for now, and reopen a new one once ready.

@MasseR
Copy link
Contributor Author

MasseR commented Feb 11, 2020

Closing it for now

@MasseR MasseR closed this Feb 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flokli flokli flokli requested changes

@infinisil infinisil Awaiting requested review from infinisil

@fpletz fpletz Awaiting requested review from fpletz

@globin globin Awaiting requested review from globin

@ckauhaus ckauhaus Awaiting requested review from ckauhaus

Assignees
No one assigned
Labels
2.status: work-in-progress 6.topic: nixos 8.has: module (update) 9.needs: clean-up 9.needs: maintainer feedback 10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@MasseR @flokli @ckauhaus @infinisil