feat(vpn): move VPN profiles from _module.args to a NixOS option - #259
MattiasKockum wants to merge 6 commits into
Conversation
The point of this commit is to pin how vpn profiles work before performing the refactoring. It doesn't touch production code.
| # | ||
| # A profile describes a tunnel from the infrastructure's point of view: where | ||
| # the gateway is, which cryptography to use, which subnets sit behind it, how to | ||
| # authenticate. It is shared by every agent. What turns a profile into actual |
There was a problem hiding this comment.
| # authenticate. It is shared by every agent. What turns a profile into actual | |
| # authenticate. It is shared by every user. What turns a profile into actual |
| ./wireguard.nix | ||
| ]; | ||
|
|
||
| options.type = mkOption { |
There was a problem hiding this comment.
Alternative proposal: Let's not have a giga big supermodule that everyone writes to.
Let's have securix.vpn.ipsec.profiles, securix.vpn.netbird.profiles, securix.vpn.wireguard.profiles, etc.
Extension becomes "easy" by just creating a new sub-module tree rather than performing type option merges (i.e. declaring securix.vpn.type outside and merging the types.enum thing).
This way, each {type}.* can have its own customisation and specialties and doesn't have to conform to a generic interface which would fit some sort of poor minimal denominator.
| ''; | ||
| }; | ||
|
|
||
| options.mkAddress = mkOption { |
There was a problem hiding this comment.
This option should be a backward compatibility system or so.
The old VPN profiles contains mkAddress functions, but we should not model them again.
In the NixOS module system, we have access to securix.self.user.bit so developers can simply replicate that feature by using config.securix.self.user.bit if they want to (or use %any, etc.)
This feature only exist because we did not have the module system and had to reinvent pieces of the module system by having a "late binding" in the form of a function.
| }; | ||
|
|
||
| requiredFields = { | ||
| ipsec = [ |
There was a problem hiding this comment.
With my alternative proposal, this could go into the defn of the IPsec VPN.
Same for the others.
| }; | ||
| in | ||
| { | ||
| options.securix.vpn.profiles = mkOption { |
There was a problem hiding this comment.
This option can disappear with the alternative proposal.
| ''; | ||
| }; | ||
|
|
||
| config.assertions = lib.concatLists ( |
There was a problem hiding this comment.
This can be split into 3 assertions checks for each VPN module defined.
| endpoint = mkOption { | ||
| type = types.nullOr types.str; | ||
| default = null; | ||
| description = "Address of the IPsec gateway. Becomes `vpn.address`."; |
There was a problem hiding this comment.
The description is weird to read. Drop the "Becomes ..."
| default = null; | ||
| description = '' | ||
| Identity the gateway is expected to present, used to validate its | ||
| certificate. Left unset, NetworkManager falls back to its own default. |
There was a problem hiding this comment.
nitpick: Can you put a link to the documentation on the remote-identity for IPsec (the strongswan one)? This would be greatly helpful for developers.
| description = '' | ||
| Subnet the agent belongs to inside the tunnel. The special value `%any` | ||
| switches the connection to IPsec config mode, where the gateway assigns | ||
| the address; in that case {option}`mkAddress` and {option}`gateway` must |
There was a problem hiding this comment.
Let's still get rid of mkAddress.
| default = null; | ||
| description = '' | ||
| Subnets reachable through the tunnel. Becomes the remote traffic | ||
| selectors, and feeds the generated network flow documentation. |
There was a problem hiding this comment.
Drop the "generated network flow documentation", it's not a feature that is really used here.
| description = "DNS server to use while the tunnel is up."; | ||
| }; | ||
|
|
||
| mkPasswordVariable = mkOption { |
There was a problem hiding this comment.
Ideally, we should rethink this option but this one is going to be hard because it relates to the automatic generation in networkmanager.
We would put something akin to securix.vpn.profiles.ipsec.$operator.pskPath = "...";, not sure what to do here.
If you have an idea, interested to hear it.
| example = lib.literalExpression ''operator: "\$IPSEC_PSK_''${operator}"''; | ||
| }; | ||
|
|
||
| availableHttpProxies = mkOption { |
There was a problem hiding this comment.
Add a warning for the deprecation using the module system in config.warnings if it is set (options should help you knowing that).
| ] | ||
| ++ map (vpn: { | ||
| assertion = config.securix.vpn.profiles ? ${vpn}; | ||
| message = "L'utilisateur ${toString cfg.user.username} référence le VPN `${vpn}` qui n'existe pas dans `securix.vpn.profiles`."; |
There was a problem hiding this comment.
nitpick: use the levenshtein calculation in nixpkgs lib to compute a suggestion of typo for the vpn name among the list of vpn profiles. This will make UX way better.
| idempotent-autoinstall = import ./idempotent-autoinstall.nix { inherit pkgs libSecurix; }; | ||
| portail = import ./portail.nix { inherit pkgs libSecurix; }; | ||
| tools = import ./tools.nix { inherit pkgs libSecurix; }; | ||
| vpn-profiles-characterization = import ./vpn-profiles-characterization.nix { |
There was a problem hiding this comment.
Great idea! Can we move the entire thing into vpn-profiles/ though and have a test loader for directories?
| vpnProfiles = { | ||
| ipsec-01 = { | ||
| type = "ipsec"; | ||
| endpoint = "vpn-01.example.gouv.fr"; |
There was a problem hiding this comment.
Let's replace example.gouv.fr here by .example.com, see https://www.rfc-editor.org/info/rfc6761/ why.
| }; | ||
| }; | ||
|
|
||
| mkChecks = |
There was a problem hiding this comment.
This is a great idea to perform such characterization tests, can we extract the absolutely generic part independent of VPN profiles to produce such tests more throughout Sécurix and instantiate the VPN profiles specialization here (and instantiate the characterization specific scenarios later in the other files)?
| # Backwards-compatibility test for the legacy _module.args.vpnProfiles | ||
| # channel. | ||
| # | ||
| # NOTE: this test exists to protect a behaviour that will be deprecated. |
There was a problem hiding this comment.
It will break anyway as soon as you remove the corresponding code, so the test will automatically go away to make CI happy.
rlahfa-dinum
left a comment
There was a problem hiding this comment.
Awesome work. I think the architectural pieces that are left pertains to the future of mk* functions in the module system form of this. I left a couple of nitpicks, but my most important concern are those. Open to proposals :).
feat(vpn): move VPN profiles from
_module.argsto a NixOS optionWhy
VPN profiles are configuration, but they travel through
_module.args.vpnProfiles, a side channel that bypasses the module system. So they cannot be typed, documented, overridden per machine, or seen by anything reasoning overconfig, and only one place can supply them, sincemkTerminaltakes a singlevpnProfilesparameter for the whole fleet.Concretely: a mistyped field in a profile fails today as
attribute 'interface' missing, pointing intomodules/vpn/wireguard/default.nix, naming neither the profile nor the edition at fault. And there is no written specification of what a profile may contain, I reconstructed the field list by reading what the three consumers dereference, since no real profile exists in this repository.The goal of this PR is to provide Securix with a module to handle VPN profiles in a clean and composable way.
This PR declares
securix.vpn.profilesand migrates every in-tree consumer to it, but keeps backward compatibility through dual-write.What changes
New module
modules/vpn/profilesthat declaressecurix.vpn.profilesasattrsOf (submodule ...).default.nixholds thetypediscriminator andmkAddress, which both the IPsec and WireGuard stacks use;ipsec.nix,wireguard.nixandnetbird.nixare submodule fragments contributing their own stack's fields.Side note: a single submodule type is shared by all three stacks, so every stack-specific field has to be
nullOr ... default = null, a mandatoryendpointwould reject every WireGuard profile. Required-ness lives in assertions gated ontype, listed inrequiredFields.lib/default.nix: the inline module now writes both_module.args.vpnProfilesandsecurix.vpn.profilesfrom the same parameter, so the two views cannot diverge.vpnProfilesalso becomes optional (? { }): an edition can now declare its profiles in one of the modules it already passes tomkTerminal, and drop the parameter entirely.The three stack modules drop
vpnProfilesfrom their signature and bindvpnProfiles = config.securix.vpn.profilesin theirlet. Their bodies are untouched. Because module arguments are pulled by name rather than pushed, each could move independently.modules/self.nix:securix.self.user.allowedVPNsgoes fromlistOf (enum (attrNames vpnProfiles))tolistOf str, with the check moved into an assertion.The old type could not follow the move: building an option type out of
configmeans reading what the option catalogue has to be built before, which is a recursion waiting to happen. It becomeslistOf str, and the check becomes an assertion naming both the offending user and the VPN.Tests
I love writing tests in Nix (I really do) so I wrote a bunch of them to ensure the refactor works and is backward compatible.
characterizationmodule-refactorlegacy-channelvalidationcompositionvpn-profiles-common.nixholds the fixtures and the shared checks, so the first two are byte-identical apart from the door the profiles come through.For an easier review of this PR, I would suggest starting from the end and read the composition test as it shows the spirit of the whole refactor.
Notes
The profile submodule is strict, meaning any field it does not declare will be rejected. I built the field list by reading the three consumers, having no real profile in the repository to check against. If the actual inventories carry fields I missed, this breaks them on upgrade. I can either add them, or add a freeformType for the duration of the migration.