From: Maxim Cournoyer <maxim.cournoyer@gmail.com>
To: Bruno Victal <mirai@makinata.eu>
Cc: 63402@debbugs.gnu.org
Subject: [bug#63402] [PATCH v5 2/5] services: wireguard: Implement a dynamic IP monitoring feature.
Date: Thu, 20 Jul 2023 23:55:18 -0400 [thread overview]
Message-ID: <87o7k5kjfd.fsf@gmail.com> (raw)
In-Reply-To: <ca34e1d6-6e96-655b-b34f-87091aaf79c8@makinata.eu> (Bruno Victal's message of "Wed, 24 May 2023 18:25:27 +0100")
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
next prev parent reply other threads:[~2023-07-21 3:56 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-05-10 1:08 [bug#63402] [PATCH 0/1] Add a dynamic IP monitoring option to Wireguard service Maxim Cournoyer
2023-05-10 1:09 ` [bug#63403] [PATCH 1/1] services: wireguard: Implement a dynamic IP monitoring feature Maxim Cournoyer
2023-05-15 15:57 ` Maxim Cournoyer
2023-05-15 16:13 ` [bug#63402] [PATCH v2] " Maxim Cournoyer
2023-05-16 4:09 ` [bug#63402] [PATCH v3 1/3] " Maxim Cournoyer
2023-05-16 4:09 ` [bug#63402] [PATCH v3 2/3] services: wireguard: Clean-up configuration file serializer Maxim Cournoyer
2023-05-16 4:09 ` [bug#63402] [PATCH v3 3/3] services: wireguard: Workaround keep-alives bug Maxim Cournoyer
2023-05-18 17:48 ` [bug#63402] [PATCH v4 0/4] Implement a dynamic IP monitoring feature Maxim Cournoyer
2023-05-18 17:48 ` [bug#63402] [PATCH v4 1/4] services: wireguard: " Maxim Cournoyer
2023-05-18 17:48 ` [bug#63402] [PATCH v4 2/4] services: wireguard: Clean-up configuration file serializer Maxim Cournoyer
2023-05-18 17:48 ` [bug#63402] [PATCH v4 3/4] services: wireguard: Add a 'configuration' action Maxim Cournoyer
2023-05-18 17:48 ` [bug#63402] [PATCH v4 4/4] gnu: linux-libre: Apply wireguard patch fixing keep-alive bug Maxim Cournoyer
2023-05-19 1:59 ` [bug#63402] [PATCH v5 0/5] Implement a dynamic IP monitoring feature Maxim Cournoyer
2023-05-19 1:59 ` [bug#63402] [PATCH v5 1/5] services: herd: Add a new 'current-service' procedure Maxim Cournoyer
2023-05-22 15:00 ` [bug#63403] [PATCH 1/1] services: wireguard: Implement a dynamic IP monitoring feature Ludovic Courtès
2023-05-22 23:22 ` [bug#63402] bug#63403: " Maxim Cournoyer
2023-05-24 14:44 ` [bug#63403] " Ludovic Courtès
2023-07-21 2:15 ` Maxim Cournoyer
2023-05-19 1:59 ` [bug#63402] [PATCH v5 2/5] " Maxim Cournoyer
2023-05-22 15:03 ` [bug#63402] bug#63403: [PATCH 1/1] " Ludovic Courtès
2023-05-22 23:32 ` Maxim Cournoyer
2023-05-24 14:53 ` [bug#63403] " Ludovic Courtès
2023-05-24 22:12 ` Bruno Victal
2023-05-25 15:13 ` Maxim Cournoyer
2023-05-24 17:25 ` [bug#63402] [PATCH v5 2/5] " Bruno Victal
2023-07-21 3:55 ` Maxim Cournoyer [this message]
2023-07-21 13:23 ` Bruno Victal
2023-07-21 15:56 ` Maxim Cournoyer
2023-07-21 16:18 ` bug#63402: " Maxim Cournoyer
2023-05-19 1:59 ` [bug#63402] [PATCH v5 3/5] services: wireguard: Clean-up configuration file serializer Maxim Cournoyer
2023-05-19 1:59 ` [bug#63402] [PATCH v5 4/5] services: wireguard: Add a 'configuration' action Maxim Cournoyer
2023-05-19 1:59 ` [bug#63402] [PATCH v5 5/5] gnu: linux-libre: Apply wireguard patch fixing keep-alive bug Maxim Cournoyer
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://guix.gnu.org/
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87o7k5kjfd.fsf@gmail.com \
--to=maxim.cournoyer@gmail.com \
--cc=63402@debbugs.gnu.org \
--cc=mirai@makinata.eu \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/guix.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).