nixos: add the strongswan-swanctl service #27958
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@basvandijk, thanks for your PR! By analyzing the history of the files in this pull request, we identified @edolstra, @bjornfor and @offlinehacker to be potential reviewers. |
basvandijk
force-pushed
the
strongswan-swanctl
branch
4 times, most recently
from
69aa725
to
0673603
Compare
|
Note that although I tested this on our company VPN I also started working on a NixOS test for this in: https://github.com/LumiGuide/nixpkgs/blob/strongswan-swanctl-test/nixos/tests/strongswan-swanctl.nix |
basvandijk
force-pushed
the
strongswan-swanctl
branch
from
0673603
to
b474d12
Compare
basvandijk
force-pushed
the
strongswan-swanctl
branch
2 times, most recently
from
68ff3db
to
a985b55
Compare
basvandijk
force-pushed
the
strongswan-swanctl
branch
from
a985b55
to
b1bab8c
Compare
|
With help from @aszlig the test I've been working on now succeeds so I've included it in this PR. This is now ready to be reviewed. |
fpletz
approved these changes
Aug 30, 2017
|
@fpletz that could be a valid concern. I can think of a few potential disadvantages:
Are there other potential disadvantages of having lots of NixOS options? The advantages to moving the strongSwan configuration from text files to NixOS options are, more type safety (things go wrong at evaluation-time rather then at strongSwan run-time) and better composability. |
basvandijk
force-pushed
the
strongswan-swanctl
branch
from
b1bab8c
to
3011cf4
Compare
|
I just noticed that the test doesn't always succeed. I described the failure on the strongSwan mailinglist but I will copy it below. @aszlig since you were so helpful with the last issue do you have an idea what's going wrong? I noticed that my test succeeds most of the time but I just observed a test run where carol keeps trying to ping alice but fails each time. The following line from the test log seems suspect: I haven't looked into this error yet but I suspect it's a concurrency issue. Note that all machines start up at the same time. I think if I first start moon, wait for the strongswan-swanctl.service to start Any ideas why the test sometimes fails? |
basvandijk
force-pushed
the
strongswan-swanctl
branch
from
3011cf4
to
c16b728
Compare
|
I fixed the test by delaying the startup of carol after moon has started the strongswan-swanctl service. |
basvandijk
force-pushed
the
strongswan-swanctl
branch
from
c16b728
to
cc9284b
Compare
|
After some more discussion on the strongSwan list I simplified the test by starting all machines in parallel again and setting |
|
Haven't had the time to review and don't know too much about strongswan, but I'd really prefer only having one module for it. If this means deprecating and removing the old one so be it, but having to maintain two is IMHO not what we want. |
|
@globin yes I think it makes sense to eventually deprecate the old current strongswan module in favor of this one. However since both modules use very different implementations with their own configuration files (the old module uses the ipsec executable with an ipsec.conf based configuraton and the new module uses charon-systemd and swanctl with a swanctl.conf based configuration), requiring users to upgrade from old to new should not be taken lightly (I went to the process and it's not trivial). So I propose the following process:
An alternative is to only deprecate the old when the strongSwan project decides to deprecate it. (Currently both ipsec and swanctl are fully supported). |
basvandijk
force-pushed
the
strongswan-swanctl
branch
3 times, most recently
from
131de2b
to
fc9b43f
Compare
|
I've made three small changes to the PR:
|
basvandijk
force-pushed
the
strongswan-swanctl
branch
from
fc9b43f
to
61f8398
Compare
|
What would be needed to make progress on this PR? |
|
EDITED: reduced the number of options from 1152 to 756 For reference, note that the following 756 options are added by the strongswan-swanctl module proposed in this PR: This list was compiled by executing: |
|
The last commit reduces the number of options by 396 (from 1152 to 756). |
|
Indeed it would be nice to see this merged. |
bgamari
reviewed
Feb 27, 2018
| ''; | ||
|
|
||
| private = mkPrefixedAttrsOfParams { | ||
| file = mkPrefixedAttrsOfParam (mkOptionalStrParam "") '' |
There was a problem hiding this comment.
Shouldn't this just be a string?
There was a problem hiding this comment.
Thanks, that should be a string indeed. I'll update it.
|
For the record I have rebased this with onto my cross-compilation work https://github.com/bgamari/nixpkgs/tree/strongswan-swanctl and have successfully used it. |
|
@bgamari thanks for trying this out! Last time I discussed this PR on the mailinglist I got some complaints that this PR introduced to many new options. So I proposed I compromise: drop the 607 strongswan.conf NixOS options but keep the swanctl options. What's your view on this? |
bgamari
reviewed
Feb 27, 2018
| ''; | ||
|
|
||
| ecdsa = mkPrefixedAttrsOfParams { | ||
| file = mkPrefixedAttrsOfParam (mkOptionalStrParam "") '' |
There was a problem hiding this comment.
I believe this and the other file options surrounding also should be plain strings.
|
I can see the value in having a more structured representation of configuration but I can also see how others might think that it unduly clutters the option namespace and documentation. Perhaps we can split up the options from the core change of moving from |
The strongswan-swanctl systemd service starts charon-systemd. This implements a IKE daemon very similar to charon, but it's specifically designed for use with systemd. It uses the systemd libraries for a native integration. Instead of using starter and an ipsec.conf based configuration, the daemon is directly managed by systemd and configured with the swanctl configuration backend. See: https://wiki.strongswan.org/projects/strongswan/wiki/Charon-systemd Note that the strongswan.conf and swantctl.conf configuration files are automatically generated based on NixOS options under services.strongswan-swanctl.strongswan and services.strongswan-swanctl.swanctl respectively.
I determined which options got changed by executing the following commands in the strongswan repository: git diff -U20 5.6.0..5.6.1 src/swanctl/swanctl.opt git diff -U20 5.6.0..5.6.1 conf
This reduces the number of options from 1152 to 756.
basvandijk
force-pushed
the
strongswan-swanctl
branch
from
305aca2
to
592a89b
Compare
…in favour of a literal config This reduces the number of option by over 600.
|
I've rebased this PR on the current |
|
@fpletz care to review again? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The strongswan-swanctl systemd service starts
charon-systemd. This implements a IKE daemonvery similar to charon, but it's specifically designed for use with systemd. It uses the
systemd libraries for a native integration.
Instead of using
starterand anipsec.confbased configuration (like in nixos/modules/services/networking/strongswan.nix), the daemon is directlymanaged by systemd and configured with the swanctl configuration backend.
See: https://wiki.strongswan.org/projects/strongswan/wiki/Charon-systemd
Note that the
strongswan.confandswantctl.confconfiguration files are automaticallygenerated based on NixOS options under
services.strongswan-swanctl.strongswanandservices.strongswan-swanctl.swanctlrespectively.Motivation for this change
Things done
Please check what applies. Note that these are not hard requirements but merely serve as information for reviewers.
(nix.useSandbox on NixOS,
or option
build-use-sandboxinnix.confon non-NixOS)
nix-shell -p nox --run "nox-review wip"./result/bin/)