[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[bug#63402] [PATCH v5 2/5] services: wireguard: Implement a dynamic IP m
From: |
Maxim Cournoyer |
Subject: |
[bug#63402] [PATCH v5 2/5] services: wireguard: Implement a dynamic IP monitoring feature. |
Date: |
Thu, 20 Jul 2023 23:55:18 -0400 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/28.2 (gnu/linux) |
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
- [bug#63402] [PATCH v5 2/5] services: wireguard: Implement a dynamic IP monitoring feature.,
Maxim Cournoyer <=