[bug#63402] [PATCH v5 2/5] services: wireguard: Implement a dynamic IP monitoring feature.

[Bruno Victal <mirai@makinata.eu>]

On 2023-05-19 02:59, Maxim Cournoyer wrote:
> +;;; XXX: Copied from (guix scripts pack), changing define to define*.
> +(define-syntax-rule (define-with-source (variable args ...) body body* ...)
> +  "Bind VARIABLE to a procedure accepting ARGS defined as BODY, also setting
> +its source property."
> +  (begin
> +    (define* (variable args ...)
> +      body body* ...)
> +    (eval-when (load eval)
> +      (set-procedure-property! variable 'source
> +                               '(define* (variable args ...) body body* ...)))))
> +
> +(define (wireguard-service-name interface)
> +  "Return the WireGuard service name (a symbol) configured to use INTERFACE."
> +  (symbol-append 'wireguard- (string->symbol interface)))
> +
> +(define-with-source (strip-port/maybe endpoint #:key ipv6?)
> +  "Strip the colon and port, if present in ENDPOINT, a string."
> +  (if ipv6?
> +      (if (string-prefix? "[" endpoint)
> +          (first (string-split (string-drop endpoint 1) #\])) ;ipv6
> +          endpoint)
> +      (first (string-split endpoint #\:)))) ;ipv4

[...]

> +
> +(define (ipv4-address? str)
> +  "Return true if STR denotes an IPv4 address."
> +  (false-if-exception
> +   (->bool (inet-pton AF_INET (strip-port/maybe str)))))

[...]

> +
> +(define (ipv6-address? str)
> +  "Return true if STR denotes an IPv6 address."
> +  (false-if-exception
> +   (->bool (inet-pton AF_INET6 (strip-port/maybe str #:ipv6? #t)))))

You should use getaddrinfo instead, reason being that inet-pton does
not work with zone-indexes or interface names in IPv6 addresses.
I expect that this snippet would get cloned and reused often which
makes it important to get it right even if zone-indexes don't happen
to be of particular interest here.

I have this snippet that you could adapt to your liking (or use as-is):

--8<---------------cut here---------------start------------->8---
(define* (ip-address? s #:optional family)
  "Check if @var{s} is a valid IP address. It optionally accepts a
@var{family} argument, either AF_INET or AF_INET6, which can be used
to exclusively check for IPv4 or IPv6 addresses."
  ;; Regrettably square brackets aren't accepted by getaddrinfo() and
  ;; must be removed beforehand.
  (let ((address (string-trim-both s (char-set #\[ #\])))
    (false-if-exception
     (->bool (getaddrinfo address #f AI_NUMERICHOST family))))))
--8<---------------cut here---------------end--------------->8---

I'd also harmonize the ipv4 check to use getaddrinfo in case you
specialize the snippet above for IPv6 only. (keeps things simpler)

> +
> +(define (host-name? name)
> +  "Predicate to check whether NAME is a host name, i.e. not an IP address."
> +  (not (or (ipv6-address? name) (ipv4-address? name))))

I'd craft an artificial uri string and extract this information from a uri
record instead, since the above check is likely to reveal insufficient:

--8<---------------cut here---------------start------------->8---
scheme@(guile-user)> (use-modules (web uri))
scheme@(guile-user)> (define s "example.tld:9999")
scheme@(guile-user)> (uri-host (string->uri (string-append "dummy://" s)))
$5 = "example.tld"
scheme@(guile-user)> (define s "[2001:db8::1234]:9999")
scheme@(guile-user)> (uri-host (string->uri (string-append "dummy://" s)))
$6 = "2001:db8::1234"
--8<---------------cut here---------------end--------------->8---

>  (define wireguard-service-type
>    (service-type
>     (name 'wireguard)
> @@ -898,6 +1036,8 @@ (define wireguard-service-type
>                               wireguard-activation)
>            (service-extension profile-service-type
>                               (compose list
> -                                      wireguard-configuration-wireguard))))
> +                                      wireguard-configuration-wireguard))
> +          (service-extension mcron-service-type
> +                             wireguard-monitoring-jobs)))
>     (description "Set up Wireguard @acronym{VPN, Virtual Private Network}
>  tunnels.")))
> diff --git a/tests/services/vpn.scm b/tests/services/vpn.scm
> new file mode 100644
> index 0000000000..a7f4bec26b
> --- /dev/null
> +++ b/tests/services/vpn.scm
> @@ -0,0 +1,83 @@
> +;;; GNU Guix --- Functional package management for GNU
> +;;; Copyright © 2023 Maxim Cournoyer <maxim.cournoyer@gmail.com>
> +;;;
> +;;; This file is part of GNU Guix.
> +;;;
> +;;; GNU Guix is free software; you can redistribute it and/or modify it
> +;;; under the terms of the GNU General Public License as published by
> +;;; the Free Software Foundation; either version 3 of the License, or (at
> +;;; your option) any later version.
> +;;;
> +;;; GNU Guix is distributed in the hope that it will be useful, but
> +;;; WITHOUT ANY WARRANTY; without even the implied warranty of
> +;;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +;;; GNU General Public License for more details.
> +;;;
> +;;; You should have received a copy of the GNU General Public License
> +;;; along with GNU Guix.  If not, see <http://www.gnu.org/licenses/>.
> +
> +(define-module (tests services vpn)
> +  #:use-module (gnu packages vpn)
> +  #:use-module (gnu services vpn)
> +  #:use-module (guix gexp)
> +  #:use-module (ice-9 match)
> +  #:use-module (srfi srfi-1)
> +  #:use-module (srfi srfi-64))
> +
> +;;; Commentary:
> +;;;
> +;;; Unit tests for the (gnu services vpn) module.
> +;;;
> +;;; Code:
> +
> +;;; Access some internals for whitebox testing.
> +(define ipv4-address? (@@ (gnu services vpn) ipv4-address?))
> +(define ipv6-address? (@@ (gnu services vpn) ipv6-address?))
> +(define host-name? (@@ (gnu services vpn) host-name?))

IMO, these kind of utility procedures seem useful enough that they
should go into either:
* (gnu services configuration)
* (gnu services network)
* or a new module consisting of useful predicates perhaps?
** (gnu services configuration predicates)
** (gnu services configuration utils)

> +(define endpoint-host-names
> +  (@@ (gnu services vpn) endpoint-host-names))
> +
> +(test-begin "vpn-services")
> +
> +(test-assert "ipv4-address?"
> +  (every ipv4-address?
> +         (list "192.95.5.67:1234"
> +               "10.0.0.1")))
> +
> +(test-assert "ipv6-address?"
> +  (every ipv6-address?
> +         (list "[2607:5300:60:6b0::c05f:543]:2468"
> +               "2607:5300:60:6b0::c05f:543"
> +               "2345:0425:2CA1:0000:0000:0567:5673:23b5"
> +               "2345:0425:2CA1::0567:5673:23b5")))

Are these addresses special?
If not, I'd recommend (properly) generating a random ULA prefix
and use it instead.

> +
> +(define %wireguard-peers
> +  (list (wireguard-peer
> +         (name "dummy1")
> +         (public-key "VlesLiEB5BFd//OD2ILKXviolfz+hodG6uZ+XjoalC8=")
> +         (endpoint "some.dynamic-dns.service:53281")
> +         (allowed-ips '()))
> +        (wireguard-peer
> +         (name "dummy2")
> +         (public-key "AlesLiEB5BFd//OD2ILKXviolfz+hodG6uZ+XgoalC9=")
> +         (endpoint "example.org")
> +         (allowed-ips '()))
> +        (wireguard-peer
> +         (name "dummy3")
> +         (public-key "BlesLiEB5BFd//OD2ILKXviolfz+hodG6uZ+XgoalC7=")
> +         (endpoint "10.0.0.7:7777")
> +         (allowed-ips '()))
> +        (wireguard-peer
> +         (name "dummy4")
> +         (public-key "ClesLiEB5BFd//OD2ILKXviolfz+hodG6uZ+XgoalC6=")
> +         (endpoint "[2345:0425:2CA1::0567:5673:23b5]:44444")
> +         (allowed-ips '()))))
> +
> +(test-equal "endpoint-host-names"
> +  '(("VlesLiEB5BFd//OD2ILKXviolfz+hodG6uZ+XjoalC8=" .
> +     "some.dynamic-dns.service:53281")
> +    ("AlesLiEB5BFd//OD2ILKXviolfz+hodG6uZ+XgoalC9=" .
> +     "example.org"))

I think a comment that explains where these values were obtained from
(or how they were generated) would be helpful for anyone looking at this
in the future.


-- 
Furthermore, I consider that nonfree software must be eradicated.

Cheers,
Bruno.