GNU bug report logs - #73465
[PATCH] Wireguard: Rename field private-key to private-key-file

Previous Next

To reply to this bug, email your comments to 73465 AT debbugs.gnu.org.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Apoorv Singh <apoorvs569 <at> gmail.com>
To: guix-patches <at> gnu.org
Subject: [PATCH] Wireguard: Rename field private-key to private-key-file
Date: Wed, 25 Sep 2024 09:28:44 +0530

[Message part 1 (text/plain, inline)]

The following patches renames the field private-key to private-key-file as it makes it more clear that it needs path to a file rather than the key it self

[0001-Wireguard-rename-field-private-key-to-private-key-fi.patch (text/x-patch, attachment)]

[Message part 3 (text/plain, inline)]

.

-- 
- Apoorv Singh
- Sent from Emacs.

Message #8 received at 73465 <at> debbugs.gnu.org (full text, mbox):

From: Sergey Trofimov <sarg <at> sarg.org.ru>
To: Apoorv Singh <apoorvs569 <at> gmail.com>
Cc: 73465 <at> debbugs.gnu.org
Subject: Re: [bug#73465] [PATCH] Wireguard: Rename field private-key to
 private-key-file
Date: Thu, 26 Sep 2024 19:39:38 +0200

Apoorv Singh <apoorvs569 <at> gmail.com> writes:

> The following patches renames the field private-key to private-key-file as it
> makes it more clear that it needs path to a file rather than the key it self
>

Hi, you have to deprecate the field instead using
`warn-about-deprecation` procedure and to adjust the documentation as
well.

Please note that there is also preshared-key parameter which also takes
a path. It'd be nice to rename it as well for consistency sake.

Message #11 received at 73465 <at> debbugs.gnu.org (full text, mbox):

From: Apoorv Singh <apoorvs569 <at> gmail.com>
To: 73465 <at> debbugs.gnu.org
Subject: Wireguard: Rename field private-key to private-key-file
Date: Sat, 28 Sep 2024 10:59:26 +0530

Do you want me to keep both private-key and private-key-file in 
the record but still use private-key for now? but just warn about 
deprecation for the field? Something like,

```
(define-record-type* <wireguard-configuration>
 wireguard-configuration make-wireguard-configuration
 wireguard-configuration?

 ;; other fields here..

 (private-key        wireguard-configuration-private-key-file 
 ;deprecated
                     (default "/etc/wireguard/private.key"))
 (private-key-file   wireguard-configuration-private-key-file 
 ;string
                     (default "/etc/wireguard/private.key"))
```

then, in the `wireguard-configuration-file` procedure, under 
`match-record`, I should do something like,
```
 (match-record config <wireguard-configuration>
   (wireguard interface addresses port private-key peers dns   ;; 
   keeping private-key field here..
              pre-up post-up pre-down post-down table)
   (let* ((config-file (string-append interface ".conf"))
          (peer-keys (fold peers->preshared-keys (list) peers))
          (peers (map peer->config peers))
          (config
           (computed-file
            "wireguard-config"
            #~(begin
                (use-modules (ice-9 format)
                             (srfi srfi-1))

                (define lines
                  (list
                    ;; other stuff..

                   (when (not (string-null? #$private-key))
                     (warn-about-deprecation 'private-key
                                             #f
                                             #:replacement 
                                             'private-key-file))

                   (format #f "PostUp = ~a set %i private-key ~a\
~{ peer ~a preshared-key ~a~}" #$(file-append wireguard "/bin/wg")
#$private-key '#$peer-keys)     ;; using private-key field here 
still..


Sorry I'm not familiar with how all this works. Just making sure 
before I commit any changes.

Also by adjust the documentation you mean edit the 
doc/guix.texi:34373 file and append something like,
```
@item @code{private-key} (default: 
@code{"/etc/wireguard/private.key"})
The private key file for the interface.  It is automatically 
generated
if the file does not exist.  'Using private-key' is deprecated use 
'private-key-file' instead.
```

-- 
- Apoorv Singh
- Sent from Emacs.

Message #14 received at 73465 <at> debbugs.gnu.org (full text, mbox):

From: Apoorv Singh <apoorvs569 <at> gmail.com>
To: 73465 <at> debbugs.gnu.org
Subject: Wireguard: Rename field private-key to private-key-file
Date: Mon, 30 Sep 2024 12:34:58 +0530

I made some changes, here is the output of `git diff`,

```
diff --git a/gnu/services/vpn.scm b/gnu/services/vpn.scm
index eee7e78c6d..ebac4ad943 100644
--- a/gnu/services/vpn.scm
+++ b/gnu/services/vpn.scm
@@ -67,7 +67,8 @@ (define-module (gnu services vpn)
            wireguard-peer-endpoint
            wireguard-peer-allowed-ips
            wireguard-peer-public-key
-            wireguard-peer-preshared-key
+            wireguard-peer-preshared-key  ; deprecated
+            wireguard-peer-preshared-key-file
            wireguard-peer-keep-alive

            wireguard-configuration
@@ -79,7 +80,8 @@ (define-module (gnu services vpn)
            wireguard-configuration-dns
            wireguard-configuration-monitor-ips?
            wireguard-configuration-monitor-ips-interval
-            wireguard-configuration-private-key
+            wireguard-configuration-private-key  ; deprecated
+            wireguard-configuration-private-key-file
            wireguard-configuration-peers
            wireguard-configuration-pre-up
            wireguard-configuration-post-up
@@ -721,15 +723,17 @@ (define strongswan-service-type
(define-record-type* <wireguard-peer>
  wireguard-peer make-wireguard-peer
  wireguard-peer?
-  (name              wireguard-peer-name)
-  (endpoint          wireguard-peer-endpoint
-                     (default #f))     ;string
-  (public-key        wireguard-peer-public-key)   ;string
-  (preshared-key     wireguard-peer-preshared-key
-                     (default #f))     ;string
-  (allowed-ips       wireguard-peer-allowed-ips) ;list of strings
-  (keep-alive        wireguard-peer-keep-alive
-                     (default #f)))    ;integer
+  (name               wireguard-peer-name)
+  (endpoint           wireguard-peer-endpoint
+                      (default #f))     ;string
+  (public-key         wireguard-peer-public-key)   ;string
+  (preshared-key      wireguard-peer-preshared-key ;deprecated
+                      (default #f))     ;string
+  (preshared-key-file wireguard-peer-preshared-key-file
+                      (default #f))     ;string
+  (allowed-ips        wireguard-peer-allowed-ips) ;list of 
strings
+  (keep-alive         wireguard-peer-keep-alive
+                      (default #f)))    ;integer

(define-record-type* <wireguard-configuration>
  wireguard-configuration make-wireguard-configuration
@@ -742,6 +746,8 @@ (define-record-type* <wireguard-configuration>
                      (default '("10.0.0.1/32")))
  (port               wireguard-configuration-port ;integer
                      (default 51820))
+  (private-key        wireguard-configuration-private-key ;string 
;deprecated
+                      (default "/etc/wireguard/private.key"))
  (private-key-file   wireguard-configuration-private-key-file 
  ;string
                      (default "/etc/wireguard/private.key"))
  (peers              wireguard-configuration-peers ;list of 
  <wiregard-peer>
@@ -778,18 +784,29 @@ (define (peer->config peer)
        (string-join (remove string-null? lines) "\n"))))

  (define (peers->preshared-keys peer keys)
-    (let ((public-key (wireguard-peer-public-key peer))
-          (preshared-key (wireguard-peer-preshared-key peer)))
-      (if preshared-key
-          (cons* public-key preshared-key keys)
+    (let* ((public-key (wireguard-peer-public-key peer))
+          (preshared-key (wireguard-peer-preshared-key peer))
+          (preshared-key-file (wireguard-peer-preshared-key-file 
peer))
+          (final-preshared-key (or preshared-key 
preshared-key-file)))
+      ;; XXX Warn about deprecated preshared-key field with newer 
replacement
+      (when preshared-key
+        (warn-about-deprecation 'preshared-key #f #:replacement 
'preshared-key-file))
+      (if final-preshared-key
+          (cons* public-key final-preshared-key keys)
          keys)))

  (match-record config <wireguard-configuration>
-    (wireguard interface addresses port private-key-file peers 
    dns
+    (wireguard interface addresses port private-key-file 
private-key peers dns
               pre-up post-up pre-down post-down table)
+
+    ;; XXX Warn about deprecated private-key field with newer 
replacement
+    (when private-key
+      (warn-about-deprecation 'private-key #f #:replacement 
'private-key-file))
+
    (let* ((config-file (string-append interface ".conf"))
           (peer-keys (fold peers->preshared-keys (list) peers))
           (peers (map peer->config peers))
+           (final-private-key (or private-key private-key-file))
           (config
            (computed-file
             "wireguard-config"
@@ -810,7 +827,7 @@ (define lines
                           (list (format #f "~{PreUp = ~a~%~}" 
                           pre-up)))
                    (format #f "PostUp = ~a set %i private-key 
                    ~a\
~{ peer ~a preshared-key ~a~}" #$(file-append wireguard 
"/bin/wg")
-#$private-key-file '#$peer-keys)
+#$final-private-key '#$peer-keys)
                    #$@(if (null? post-up)
                           '()
                           (list (format #f "~{PostUp = ~a~%~}" 
                           post-up)))
@@ -836,23 +853,29 @@ (define lines

(define (wireguard-activation config)
  (match-record config <wireguard-configuration>
-    (private-key-file wireguard)
-    #~(begin
-        (use-modules (guix build utils)
-                     (ice-9 popen)
-                     (ice-9 rdelim))
-        (mkdir-p (dirname #$private-key-file))
-        (unless (file-exists? #$private-key-file)
-          (let* ((pipe
-                  (open-input-pipe (string-append
-                                    #$(file-append wireguard 
                                     "/bin/wg")
-                                    " genkey")))
-                 (key (read-line pipe)))
-            (call-with-output-file #$private-key-file
-              (lambda (port)
-                (display key port)))
-            (chmod #$private-key-file #o400)
-            (close-pipe pipe))))))
+    (private-key private-key-file wireguard)
+
+    ;; XXX Warn about deprecated private-key field with newer 
replacement
+    (when private-key
+      (warn-about-deprecation 'private-key #f #:replacement 
'private-key-file))
+
+    (let ((final-private-key (or private-key private-key-file)))
+      #~(begin
+          (use-modules (guix build utils)
+                       (ice-9 popen)
+                       (ice-9 rdelim))
+          (mkdir-p (dirname #$final-private-key))
+          (unless (file-exists? #$final-private-key)
+            (let* ((pipe
+                    (open-input-pipe (string-append
+                                      #$(file-append wireguard 
"/bin/wg")
+                                      " genkey")))
+                   (key (read-line pipe)))
+              (call-with-output-file #$final-private-key
+                (lambda (port)
+                  (display key port)))
+              (chmod #$final-private-key #o400)
+              (close-pipe pipe)))))))

;;; XXX: Copied from (guix scripts pack), changing define to 
define*.
(define-syntax-rule (define-with-source (variable args ...) body 
body* ...)
```

If this is desired way of doing this, I will share the formatted 
patch as an attachment. 

-- 
- Apoorv Singh
- Sent from Emacs.

Message #17 received at 73465 <at> debbugs.gnu.org (full text, mbox):

From: Apoorv Singh <apoorvs569 <at> gmail.com>
To: 73465 <at> debbugs.gnu.org
Subject: Wireguard: Deprecate and rename fields
Date: Sat, 05 Oct 2024 09:05:58 +0530

[Message part 1 (text/plain, inline)]

The following patch is a V2 for renaming the following fields,
- preshared-key to preshared-key-file
- private-key to private-key-file

[0001-Wireguard-Deprecate-and-rename-fields-with-warning.patch (text/x-patch, attachment)]

[Message part 3 (text/plain, inline)]

-- 
- Apoorv Singh
- Sent from Emacs.

Message #20 received at 73465 <at> debbugs.gnu.org (full text, mbox):

From: Richard Sent <richard <at> freakingpenguin.com>
To: Apoorv Singh <apoorvs569 <at> gmail.com>
Cc: 73465 <at> debbugs.gnu.org
Subject: Re: [bug#73465] Wireguard: Deprecate and rename fields
Date: Mon, 09 Dec 2024 12:05:22 -0500

With #73955, private-key better supports g-exp based command redirection,
e.g.

--8<---------------cut here---------------start------------->8---
;; A config of
(wireguard-configuration
  ...
  (private-key (string-append "(<" my-custom-script ">")))

;; Results in
PostUp = ... set %i private-key <(/gnu/store/...-my-custom-script) ...
--8<---------------cut here---------------end--------------->8---

(This was also supported before but was more limited.)

Given that, I think renaming it to private-key-file is more confusing
than keeping it as private-key. Same for preshared-key.

Perhaps we can somehow check the field, see if the user enters a
WG-compatible key literally, and emit a warning? I'm not fluent on the
format to determine the best way for that.
-- 
Take it easy,
Richard Sent
Making my computer weirder one commit at a time.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.