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

[Maxim Cournoyer]

Hi Bruno,

Bruno Victal <mirai@makinata.eu> writes:

> 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):
>
> (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))))))
>
>
> I'd also harmonize the ipv4 check to use getaddrinfo in case you
> specialize the snippet above for IPv6 only. (keeps things simpler)

Thanks!  I've adapted as:

--8<---------------cut here---------------start------------->8---
modified   gnu/services/vpn.scm
@@ -903,15 +903,17 @@ (define-with-source (strip-port/maybe endpoint #:key 
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)))))
+(define* (ipv4-address? address)
+  "Predicate to check whether ADDRESS is a valid IPv4 address."
+  (let ((address (strip-port/maybe address)))
+    (false-if-exception
+     (->bool (getaddrinfo address #f AI_NUMERICHOST AF_INET)))))
+
+(define* (ipv6-address? address)
+  "Predicate to check whether ADDRESS is a valid IPv6 address."
+  (let ((address (strip-port/maybe address #:ipv6? #t)))
+    (false-if-exception
+     (->bool (getaddrinfo address #f AI_NUMERICHOST AF_INET6)))))

 (define (host-name? name)
   "Predicate to check whether NAME is a host name, i.e. not an IP address."
--8<---------------cut here---------------end--------------->8---

Since there's some local considerations weaved in (strip-port/maybe), I
think it's fine that these live in the vpn.scm module.  When need be, we
can refactor a more general version and find a suitable home for it.

>> +
>> +(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:
>
> 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"

I'm not sure I understand; In the second case, I'd like it to tell me
it's *not* a host name, but it seems like uri-host happily returns IP
addresses the same as host names?

[...]

>> +(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.

They are not!  I derived them from actual IP addresses, adding some
fuzz.  I've now used unique local IPv6 prefixes.

>> +
>> +(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.

OK, I've now added a comment.

-- 
Thanks,
Maxim