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.

Sign up for GitHub

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

wireguard: Add peers.*.publicKeyFile as an alternative to publicKey #60339

Closed
wants to merge 1 commit into from
Closed

wireguard: Add peers.*.publicKeyFile as an alternative to publicKey #60339

wants to merge 1 commit into from

Conversation

thoferon
Copy link

Motivation for this change

When dealing with several servers, we might want a mechanism by which the servers share their public key with each other after being provisioned. Thus, we can't give the NixOS configuration the public keys directly because they are not known at that point in time. publicKeyFile solves the issue.

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)
  • Assured whether relevant documentation is up to date (couldn't find any documentation in NixOS manual)
  • Fits CONTRIBUTING.md.

@Ma27
Copy link
Member

Ma27 commented Apr 27, 2019

LGTM 👍

@GrahamcOfBorg test wireguard wireguard-generated

@thoferon thoferon force-pushed the wireguard-publickeyfile branch 2 times, most recently from b30a23d to 6d0390d Compare August 12, 2019 19:30
@thoferon
Copy link
Author

@Ma27 @infinisil @grahamc I've rebased this change on the latest version of master and retested on some servers. It's still working. Can this be merged or do I need to do anything else?

@flokli
Copy link
Contributor

flokli commented Nov 8, 2019

Can you update nixos/tests/wireguard/default.nix to let peer1 use publicKeyFile, so we have test coverage?

@thoferon thoferon force-pushed the wireguard-publickeyfile branch 2 times, most recently from 2e2986d to 07e42a8 Compare November 14, 2019 21:20
@thoferon
Copy link
Author

Can you update nixos/tests/wireguard/default.nix to let peer1 use publicKeyFile, so we have test coverage?

peer1 now uses publicKeyFile and the tests pass. I've also rebased on the latest master.

infinisil
infinisil requested changes Jan 7, 2020
View reviewed changes
Copy link
Member

@infinisil infinisil left a comment

Choose a reason for hiding this comment

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

As far as I can tell this looks good other than the thing below.

nixos/modules/services/networking/wireguard.nix Outdated Show resolved Hide resolved
@thoferon
Copy link
Author

@infinisil The changes you requested are done and I just rebased on the latest master.

@thoferon
Copy link
Author

Rebased again.

grahamc
grahamc reviewed Apr 29, 2020
View reviewed changes
nixos/modules/services/networking/wireguard.nix
peerName = if peer.publicKey != null then peer.publicKey else peer.publicKeyFile;
in
[
{
Copy link
Member

Choose a reason for hiding this comment

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

I think we need an assert that one of: publicKey or publicKeyFile is set? I don't see one, but it is possible.

Copy link
Author

Choose a reason for hiding this comment

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

That case would be caught by this assertion as well since (null != null) != (null != null) is false. I basically copied the pre-existing logic for privateKey and privateKeyFile.

@infinisil
Copy link
Member

Doesn't work if you set e.g. publicKeyFile = ./path/to/public-key. The initial error can be fixed with

diff --git a/nixos/modules/services/networking/wireguard.nix b/nixos/modules/services/networking/wireguard.nix
index daf10e8284e..6b023b5e41f 100644
--- a/nixos/modules/services/networking/wireguard.nix
+++ b/nixos/modules/services/networking/wireguard.nix
@@ -268,7 +268,7 @@ let
       keyToUnitName = replaceChars
         [ "/" "-"    " "     "+"     "="      ]
         [ "-" "\\x2d" "\\x20" "\\x2b" "\\x3d" ];
-      peerName = if peer.publicKey != null then peer.publicKey else peer.publicKeyFile;
+      peerName = if peer.publicKey != null then peer.publicKey else builtins.unsafeDiscardStringContext peer.publicKeyFile;
       publicKeyArg = if peer.publicKey != null then peer.publicKey else "\"$(cat ${peer.publicKeyFile})\"";
       unitName = keyToUnitName peerName;
       psk =
@@ -445,7 +445,7 @@ in
           ];
           peerAssertions = builtins.concatLists (map (peer:
             let
-              peerName = if peer.publicKey != null then peer.publicKey else peer.publicKeyFile;
+              peerName = if peer.publicKey != null then peer.publicKey else builtins.unsafeDiscardStringContext peer.publicKeyFile;
             in
             [
               {

But then there's the problem that if both publicKey and publicKeyFile are null, the assertion isn't triggered because the peerName gets evaluated before that, which fails when both are null.

@thoferon
Copy link
Author

thoferon commented May 6, 2020

Doesn't work if you set e.g. publicKeyFile = ./path/to/public-key. The initial error can be fixed with

Is it really an issue? This option's purpose is to be used when the user doesn't have the public key at build time. If the user does, they can use publicKey instead.

@thoferon @tomferon
nixos/wireguard: add peers.*.publicKeyFile
678854f 
If peers' public keys are not known when writing the NixOS
configuration, publicKeyFile can be given instead of publicKey.
@stale
Copy link

stale bot commented Jan 8, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jan 8, 2021
@otavio
Copy link
Contributor

otavio commented Feb 15, 2022

I reworked this PR in #160086; closing this as this has been stale and the new PR can be used for any further changes.

@otavio otavio closed this Feb 15, 2022
otavio added a commit to otavio/nixpkgs that referenced this pull request Aug 15, 2022
@otavio
nixos/wireguard: add peers.*.publicKeyFile
4373c2d 
If peers' public keys are not known when writing the NixOS
configuration, publicKeyFile can be given instead of publicKey.

This change is a rework of PR
NixOS#60339 (by Thomas Feron
<thomas.feron@redspline.com>) which has been stale.

Signed-off-by: Otavio Salvador <otavio@ossystems.com.br>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@grahamc grahamc grahamc left review comments

@flokli flokli flokli left review comments

@infinisil infinisil infinisil requested changes

@0x4A6F 0x4A6F 0x4A6F approved these changes

Assignees
No one assigned
Labels
2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 0 12. first-time contribution
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@thoferon @Ma27 @flokli @infinisil @otavio @grahamc @0x4A6F @Janik-Haag