[bug#54352] [PATCH] services: dnsmasq: Add more options.

[bug#54352] [PATCH] services: dnsmasq: Add more options.

[Remco van 't Veer]

* gnu/services/dns.scm (<dnsmasq-configuration>): Add bogus-priv?,
strict-order? and add-cpe-id options.
(dnsmasq-shepherd-service): Pass bogus-priv, strict-order and add-cpe-id
to the service.
* doc/guix.texi (Guix Services): Document options added to dnsmasq.
---
 doc/guix.texi        | 12 ++++++++++++
 gnu/services/dns.scm | 20 +++++++++++++++++---
 2 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/doc/guix.texi b/doc/guix.texi
index 4b71fb7010..136c199e58 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -28945,6 +28945,14 @@ The file to read the IP address of the upstream nameservers from.
 @item @code{no-resolv?} (default: @code{#f})
 When true, don't read @var{resolv-file}.
 
+@item @code{bogus-priv?} (default: @code{#f})
+When true, all reverse lookups for private IP ranges are answered with
+"no such domain" rather than being forwarded upstream.
+
+@item @code{strict-order?} (default: @code{#f})
+When true, forces dnsmasq to try each query with each server strictly in
+the order they appear in @var{servers}.
+
 @item @code{servers} (default: @code{'()})
 Specify IP address of upstream servers directly.
 
@@ -28974,6 +28982,10 @@ disables caching.
 @item @code{negative-cache?} (default: @code{#t})
 When false, disable negative caching.
 
+@item @code{add-cpe-id} (default: @code{#f})
+If set, add an arbitrary identifying string to DNS queries which are
+forwarded upstream.
+
 @item @code{tftp-enable?} (default: @code{#f})
 Whether to enable the built-in TFTP server.
 
diff --git a/gnu/services/dns.scm b/gnu/services/dns.scm
index 9b8603cc95..9f9b6c1a69 100644
--- a/gnu/services/dns.scm
+++ b/gnu/services/dns.scm
@@ -3,6 +3,7 @@
 ;;; Copyright © 2018 Oleg Pykhalov <go.wigust@gmail.com>
 ;;; Copyright © 2020 Pierre Langlois <pierre.langlois@gmx.com>
 ;;; Copyright © 2021 Maxime Devos <maximedevos@telenet.be>
+;;; Copyright © 2022 Remco van 't Veer <remco@remworks.net>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -745,6 +746,10 @@ (define-record-type* <dnsmasq-configuration>
                     (default "/etc/resolv.conf")) ;string
   (no-resolv?       dnsmasq-configuration-no-resolv?
                     (default #f))       ;boolean
+  (bogus-priv?      dnsmasq-configuration-bogus-priv?
+                    (default #f))       ;boolean
+  (strict-order?    dnsmasq-configuration-strict-order?
+                    (default #f))       ;boolean
   (servers          dnsmasq-configuration-servers
                     (default '()))      ;list of string
   (addresses        dnsmasq-configuration-addresses
@@ -752,7 +757,9 @@ (define-record-type* <dnsmasq-configuration>
   (cache-size       dnsmasq-configuration-cache-size
                     (default 150))      ;integer
   (negative-cache?  dnsmasq-configuration-negative-cache?
-                    (default #t))      ;boolean
+                    (default #t))       ;boolean
+  (add-cpe-id       dnsmasq-configuration-add-cpe-id
+                    (default #t))       ;string
   (tftp-enable?     dnsmasq-configuration-tftp-enable?
                     (default #f))       ;boolean
   (tftp-no-fail?    dnsmasq-configuration-tftp-no-fail?
@@ -781,8 +788,9 @@ (define dnsmasq-shepherd-service
     (($ <dnsmasq-configuration> package
                                 no-hosts?
                                 port local-service? listen-addresses
-                                resolv-file no-resolv? servers
-                                addresses cache-size negative-cache?
+                                resolv-file no-resolv? bogus-priv?
+                                strict-order? servers addresses cache-size
+                                negative-cache? add-cpe-id
                                 tftp-enable? tftp-no-fail?
                                 tftp-single-port? tftp-secure?
                                 tftp-max tftp-mtu tftp-no-blocksize?
@@ -809,6 +817,9 @@ (define dnsmasq-shepherd-service
                   #$@(if no-resolv?
                          '("--no-resolv")
                          '())
+                  #$@(if bogus-priv?
+                         '("--bogus-priv")
+                         '())
                   #$@(map (cut format #f "--server=~a" <>)
                           servers)
                   #$@(map (cut format #f "--address=~a" <>)
@@ -817,6 +828,9 @@ (define dnsmasq-shepherd-service
                   #$@(if negative-cache?
                          '()
                          '("--no-negcache"))
+                  #$@(if add-cpe-id
+                         (list (format #f "--add-cpe-id=~a" add-cpe-id))
+                         '())
                   #$@(if tftp-enable?
                          '("--enable-tftp")
                          '())
-- 
2.34.0

[bug#54352] [PATCH] services: dnsmasq: Add more options.

[Ludovic Courtès]

Hi,

Remco van 't Veer <remco@remworks.net> skribis:

> * gnu/services/dns.scm (<dnsmasq-configuration>): Add bogus-priv?,
> strict-order? and add-cpe-id options.
> (dnsmasq-shepherd-service): Pass bogus-priv, strict-order and add-cpe-id
> to the service.
> * doc/guix.texi (Guix Services): Document options added to dnsmasq.

I don’t use dnsmasq, but overall that LGTM, and a welcome addition.

Nitpick:

> +  (bogus-priv?      dnsmasq-configuration-bogus-priv?
> +                    (default #f))       ;boolean

I’d rather avoid the abbreviation and use something more descriptive,
even if that doesn’t match the name used in the config file.  Maybe
something like ‘forward-private-reverse-lookup?’ (I’m making it up based
on the doc you wrote.)  WDYT?

> +  (add-cpe-id       dnsmasq-configuration-add-cpe-id
> +                    (default #t))       ;string

Maybe ‘additional-cpe-id’ or ‘extra-cpe-id’ or ‘cpe-id’?

>      (($ <dnsmasq-configuration> package
>                                  no-hosts?
>                                  port local-service? listen-addresses
> -                                resolv-file no-resolv? servers
> -                                addresses cache-size negative-cache?
> +                                resolv-file no-resolv? bogus-priv?
> +                                strict-order? servers addresses cache-size
> +                                negative-cache? add-cpe-id

Not a blocker for this patch, but we should change that to
‘match-record’, which is less error-prone.

Let me know what you think; you can send an updated patch if the
suggestions make sense to you.

Thanks!

Ludo’.

[bug#54352] [PATCH] services: dnsmasq: Add more options.

[Remco van 't Veer]

Thank you Ludovic for your feedback!  I've applied all of your
suggestions and am especially happy about match-record because I had
some trouble with match-lambda (did not realise at first order matters).
I'll post a v2 immediately.

Cheers,
R.

[bug#54352] [PATCH v2] services: dnsmasq: Add more options.

[Remco van 't Veer]

* gnu/services/dns.scm (<dnsmasq-configuration>): Add forward-private-reverse-lookup?, strict-order? and additional-cpe-id options.
(dnsmasq-shepherd-service): Pass added options to dnsmasq.
* doc/guix.texi (Guix Services): Document options added to dnsmasq.
---
 doc/guix.texi        |  12 +++
 gnu/services/dns.scm | 178 +++++++++++++++++++++++--------------------
 2 files changed, 109 insertions(+), 81 deletions(-)

diff --git a/doc/guix.texi b/doc/guix.texi
index 4b71fb7010..a769cd1e5b 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -28945,6 +28945,14 @@ The file to read the IP address of the upstream nameservers from.
 @item @code{no-resolv?} (default: @code{#f})
 When true, don't read @var{resolv-file}.
 
+@item @code{forward-private-reverse-lookup?} (default: @code{#t})
+When false, all reverse lookups for private IP ranges are answered with
+"no such domain" rather than being forwarded upstream.
+
+@item @code{strict-order?} (default: @code{#f})
+When true, forces dnsmasq to try each query with each server strictly in
+the order they appear in @var{servers}.
+
 @item @code{servers} (default: @code{'()})
 Specify IP address of upstream servers directly.
 
@@ -28974,6 +28982,10 @@ disables caching.
 @item @code{negative-cache?} (default: @code{#t})
 When false, disable negative caching.
 
+@item @code{additional-cpe-id} (default: @code{#f})
+If set, add an arbitrary identifying string to DNS queries which are
+forwarded upstream.
+
 @item @code{tftp-enable?} (default: @code{#f})
 Whether to enable the built-in TFTP server.
 
diff --git a/gnu/services/dns.scm b/gnu/services/dns.scm
index 9b8603cc95..5add843f32 100644
--- a/gnu/services/dns.scm
+++ b/gnu/services/dns.scm
@@ -3,6 +3,7 @@
 ;;; Copyright © 2018 Oleg Pykhalov <go.wigust@gmail.com>
 ;;; Copyright © 2020 Pierre Langlois <pierre.langlois@gmx.com>
 ;;; Copyright © 2021 Maxime Devos <maximedevos@telenet.be>
+;;; Copyright © 2022 Remco van 't Veer <remco@remworks.net>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -745,6 +746,11 @@ (define-record-type* <dnsmasq-configuration>
                     (default "/etc/resolv.conf")) ;string
   (no-resolv?       dnsmasq-configuration-no-resolv?
                     (default #f))       ;boolean
+  (forward-private-reverse-lookup?
+   dnsmasq-configuration-forward-private-reverse-lookup?
+                    (default #t))       ;boolean
+  (strict-order?    dnsmasq-configuration-strict-order?
+                    (default #f))       ;boolean
   (servers          dnsmasq-configuration-servers
                     (default '()))      ;list of string
   (addresses        dnsmasq-configuration-addresses
@@ -752,7 +758,9 @@ (define-record-type* <dnsmasq-configuration>
   (cache-size       dnsmasq-configuration-cache-size
                     (default 150))      ;integer
   (negative-cache?  dnsmasq-configuration-negative-cache?
-                    (default #t))      ;boolean
+                    (default #t))       ;boolean
+  (additional-cpe-id dnsmasq-configuration-additional-cpe-id
+                    (default #t))       ;string
   (tftp-enable?     dnsmasq-configuration-tftp-enable?
                     (default #f))       ;boolean
   (tftp-no-fail?    dnsmasq-configuration-tftp-no-fail?
@@ -776,86 +784,94 @@ (define-record-type* <dnsmasq-configuration>
   (tftp-unique-root dnsmasq-tftp-unique-root
                     (default #f)))      ;"" or "ip" or "mac"
 
-(define dnsmasq-shepherd-service
-  (match-lambda
-    (($ <dnsmasq-configuration> package
-                                no-hosts?
-                                port local-service? listen-addresses
-                                resolv-file no-resolv? servers
-                                addresses cache-size negative-cache?
-                                tftp-enable? tftp-no-fail?
-                                tftp-single-port? tftp-secure?
-                                tftp-max tftp-mtu tftp-no-blocksize?
-                                tftp-lowercase? tftp-port-range
-                                tftp-root tftp-unique-root)
-     (shepherd-service
-      (provision '(dnsmasq))
-      (requirement '(networking))
-      (documentation "Run the dnsmasq DNS server.")
-      (start #~(make-forkexec-constructor
-                '(#$(file-append package "/sbin/dnsmasq")
-                  "--keep-in-foreground"
-                  "--pid-file=/run/dnsmasq.pid"
-                  #$@(if no-hosts?
-                         '("--no-hosts")
-                         '())
-                  #$(format #f "--port=~a" port)
-                  #$@(if local-service?
-                         '("--local-service")
-                         '())
-                  #$@(map (cut format #f "--listen-address=~a" <>)
-                          listen-addresses)
-                  #$(format #f "--resolv-file=~a" resolv-file)
-                  #$@(if no-resolv?
-                         '("--no-resolv")
-                         '())
-                  #$@(map (cut format #f "--server=~a" <>)
-                          servers)
-                  #$@(map (cut format #f "--address=~a" <>)
-                          addresses)
-                  #$(format #f "--cache-size=~a" cache-size)
-                  #$@(if negative-cache?
-                         '()
-                         '("--no-negcache"))
-                  #$@(if tftp-enable?
-                         '("--enable-tftp")
-                         '())
-                  #$@(if tftp-no-fail?
-                         '("--tftp-no-fail")
-                         '())
-                  #$@(if tftp-single-port?
-                         '("--tftp-single-port")
-                         '())
-                  #$@(if tftp-secure?
-                         '("--tftp-secure?")
-                         '())
-                  #$@(if tftp-max
-                         (list (format #f "--tftp-max=~a" tftp-max))
-                         '())
-                  #$@(if tftp-mtu
-                         (list (format #f "--tftp-mtu=~a" tftp-mtu))
-                         '())
-                  #$@(if tftp-no-blocksize?
-                         '("--tftp-no-blocksize")
-                         '())
-                  #$@(if tftp-lowercase?
-                         '("--tftp-lowercase")
-                         '())
-                  #$@(if tftp-port-range
-                         (list (format #f "--tftp-port-range=~a"
-                                          tftp-port-range))
-                         '())
-                  #$@(if tftp-root
-                         (list (format #f "--tftp-root=~a" tftp-root))
-                         '())
-                  #$@(if tftp-unique-root
-                         (list
-                          (if (> (length tftp-unique-root) 0)
-                              (format #f "--tftp-unique-root=~a" tftp-unique-root)
-                              (format #f "--tftp-unique-root")))
-                         '()))
-                #:pid-file "/run/dnsmasq.pid"))
-      (stop #~(make-kill-destructor))))))
+(define (dnsmasq-shepherd-service config)
+  (match-record config <dnsmasq-configuration>
+    (package
+     no-hosts?
+     port local-service? listen-addresses
+     resolv-file no-resolv?
+     forward-private-reverse-lookup? strict-order?
+     servers addresses cache-size negative-cache?
+     additional-cpe-id
+     tftp-enable? tftp-no-fail?
+     tftp-single-port? tftp-secure?
+     tftp-max tftp-mtu tftp-no-blocksize?
+     tftp-lowercase? tftp-port-range
+     tftp-root tftp-unique-root)
+    (shepherd-service
+     (provision '(dnsmasq))
+     (requirement '(networking))
+     (documentation "Run the dnsmasq DNS server.")
+     (start #~(make-forkexec-constructor
+               '(#$(file-append package "/sbin/dnsmasq")
+                 "--keep-in-foreground"
+                 "--pid-file=/run/dnsmasq.pid"
+                 #$@(if no-hosts?
+                        '("--no-hosts")
+                        '())
+                 #$(format #f "--port=~a" port)
+                 #$@(if local-service?
+                        '("--local-service")
+                        '())
+                 #$@(map (cut format #f "--listen-address=~a" <>)
+                         listen-addresses)
+                 #$(format #f "--resolv-file=~a" resolv-file)
+                 #$@(if no-resolv?
+                        '("--no-resolv")
+                        '())
+                 #$@(if forward-private-reverse-lookup?
+                        '()
+                        '("--bogus-priv"))
+                 #$@(map (cut format #f "--server=~a" <>)
+                         servers)
+                 #$@(map (cut format #f "--address=~a" <>)
+                         addresses)
+                 #$(format #f "--cache-size=~a" cache-size)
+                 #$@(if negative-cache?
+                        '()
+                        '("--no-negcache"))
+                 #$@(if additional-cpe-id
+                        (list (format #f "--add-cpe-id=~a" additional-cpe-id))
+                        '())
+                 #$@(if tftp-enable?
+                        '("--enable-tftp")
+                        '())
+                 #$@(if tftp-no-fail?
+                        '("--tftp-no-fail")
+                        '())
+                 #$@(if tftp-single-port?
+                        '("--tftp-single-port")
+                        '())
+                 #$@(if tftp-secure?
+                        '("--tftp-secure?")
+                        '())
+                 #$@(if tftp-max
+                        (list (format #f "--tftp-max=~a" tftp-max))
+                        '())
+                 #$@(if tftp-mtu
+                        (list (format #f "--tftp-mtu=~a" tftp-mtu))
+                        '())
+                 #$@(if tftp-no-blocksize?
+                        '("--tftp-no-blocksize")
+                        '())
+                 #$@(if tftp-lowercase?
+                        '("--tftp-lowercase")
+                        '())
+                 #$@(if tftp-port-range
+                        (list (format #f "--tftp-port-range=~a"
+                                      tftp-port-range))
+                        '())
+                 #$@(if tftp-root
+                        (list (format #f "--tftp-root=~a" tftp-root))
+                        '())
+                 #$@(if tftp-unique-root
+                        (list
+                         (if (> (length tftp-unique-root) 0)
+                             (format #f "--tftp-unique-root=~a" tftp-unique-root)
+                             (format #f "--tftp-unique-root")))
+                        '()))
+               #:pid-file "/run/dnsmasq.pid"))
+     (stop #~(make-kill-destructor)))))
 
 (define (dnsmasq-activation config)
   #~(begin
-- 
2.34.0

[bug#54352] [PATCH v2] services: dnsmasq: Add more options.

[Maxime Devos]

[-- Attachment #1: Type: text/plain, Size: 542 bytes --]

Remco van 't Veer schreef op zo 20-03-2022 om 12:44 [+0100]:
> +  (forward-private-reverse-lookup?
> +   dnsmasq-configuration-forward-private-reverse-lookup?
> +                    (default #t))       ;boolean
> +  (strict-order?    dnsmasq-configuration-strict-order?
> +                    (default #f))       ;boolean

It would be nice to verify that these are, in fact, booleans,
using field sanitizers.  See, e.g., ensure-setuid-program-list
in (gnu system).

Greetings,
Maxime.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

[bug#54352] [PATCH v2] services: dnsmasq: Add more options.

[Remco van 't Veer]

2022/03/20 12:56, Maxime Devos:

> It would be nice to verify that these are, in fact, booleans,
> using field sanitizers.  See, e.g., ensure-setuid-program-list
> in (gnu system).

I agree but the same could be said about the other fields and types in
this record, and those of other services.  In this case, the names of
the fields ending with "?" should be enough for somebody to realise this
is a boolean, IMHO.  The ";boolean" comments I've added are just me
trying to blend in.

[bug#54352] [PATCH v2] services: dnsmasq: Add more options.

[Maxime Devos]

[-- Attachment #1: Type: text/plain, Size: 1379 bytes --]

Remco van 't Veer schreef op zo 20-03-2022 om 13:22 [+0100]:
> 2022/03/20 12:56, Maxime Devos:
> 
> > It would be nice to verify that these are, in fact, booleans,
> > using field sanitizers.  See, e.g., ensure-setuid-program-list
> > in (gnu system).
> 
> I agree but the same could be said about the other fields and types in
> this record, and those of other services. 

In the long-term, it would be nice to eventually add error checking to
other services and fields as well.  In the short-term, I would avoid
making error handling worse.

> In this case, the names of the fields ending with "?" should be
> enough for somebody to realise this is a boolean, IMHO. 
> The";boolean" comments I've added are just me trying to blend in.

It's technically sufficient, but it does not make for nice error
messages, see the thread at
<https://lists.gnu.org/archive/html/guix-devel/2022-02/msg00140.html>.

In this particular case, there won't be an error message *at all*,
due to how (if forward-private-reverse-lookup? ... ...) works.

For example, consider the case where someone new to Guile and Guix sees
‘boolean’ in the documentation and then tries using "true" and "false"
(the strings):

  (dnsmasq-configuration
    (forward-private-reverse-lookup? "false"))

Currently, this will silently do the wrong thing.

Greetings,
Maxime.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

[bug#54352] [PATCH v2] services: dnsmasq: Add more options.

[Maxime Devos]

[-- Attachment #1: Type: text/plain, Size: 247 bytes --]

Remco van 't Veer schreef op zo 20-03-2022 om 12:44 [+0100]:
> +  (strict-order?    dnsmasq-configuration-strict-order?
> +                    (default #f))       ;boolean

The field strict-order? appears to be unused.

Greetings,
Maxime.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

[bug#54352] [PATCH v2] services: dnsmasq: Add more options.

[Maxime Devos]

[-- Attachment #1: Type: text/plain, Size: 395 bytes --]

Remco van 't Veer schreef op zo 20-03-2022 om 12:44 [+0100]:
> +@item @code{additional-cpe-id} (default: @code{#f})
> +If set, add an arbitrary identifying string to DNS queries which are
> +forwarded upstream.

What is an ‘arbitrary identify string’ here?  What does ‘cpe’ mean? 
How can I know if I need to set this, and how do I know what to set it
to?

Greetings,
Maxime.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

[bug#54352] [PATCH v2] services: dnsmasq: Add more options.

[Maxime Devos]

[-- Attachment #1: Type: text/plain, Size: 552 bytes --]

Remco van 't Veer schreef op zo 20-03-2022 om 12:44 [+0100]:
> +When true, forces dnsmasq to try each query with each server strictly in
> +the order they appear in @var{servers}.

Can be simplified to:

  When try, dnsmasq queries the servers in the same order as they
  appear in @var{server}.

dnsmasq does this voluntarily on asking, there doesn't appear to be any
forcing here.

Also, what does ‘strict’ mean here?  Is it like the difference between
<= and <?  WDYT of naming it 'query-servers-in-order?'?

Greetings,
Maxime.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

[bug#54352] [PATCH v2] services: dnsmasq: Add more options.

[Remco van 't Veer]

> What is an ‘arbitrary identify string’ here?  What does ‘cpe’ mean?
> How can I know if I need to set this, and how do I know what to set it
> to?

I think it stands for "Common Platform Enumerator" but I am not really
sure.  It's used by DNS providers to identify the client so they know
for what account they are resolving.  Looking at the dnsmasq code base
this option used to be called "dns-client-id" which is a lot clearer.  I
could change it to this older name and describe it as above.  WDYT?

[bug#54352] [PATCH v2] services: dnsmasq: Add more options.

[Remco van 't Veer]

2022/03/20 13:31, Maxime Devos:

> The field strict-order? appears to be unused.

Oeps, I'll fix it.

[bug#54352] [PATCH v2] services: dnsmasq: Add more options.

[Remco van 't Veer]

2022/03/20 13:30, Maxime Devos:

> In the long-term, it would be nice to eventually add error checking to
> other services and fields as well.  In the short-term, I would avoid
> making error handling worse.
> [snip]
> Currently, this will silently do the wrong thing.

True, I'll add sanitize code for the boolean fields.

[bug#54352] [PATCH v2] services: dnsmasq: Add more options.

[Remco van 't Veer]

I totally agree with the naming suggestions and will apply them but what
if somebody has been using dnsmasq before?  In that case they will not
recognize the options and which may cause confusion.  I guess the answer
to that is that guix should not propagate the wording mistakes made
upstream?

To help those with prior knowledge of dnsmasq, I could add a config-file
field.  WDYT?

[bug#54352] [PATCH v2] services: dnsmasq: Add more options.

[Maxime Devos]

[-- Attachment #1: Type: text/plain, Size: 890 bytes --]

Remco van 't Veer schreef op zo 20-03-2022 om 13:57 [+0100]:
> > What is an ‘arbitrary identify string’ here?  What does ‘cpe’ mean?
> > How can I know if I need to set this, and how do I know what to set
> > it
> > to?
> 
> I think it stands for "Common Platform Enumerator" but I am not
> really sure.  It's used by DNS providers to identify the client so
> they know for what account they are resolving.  Looking at the
> dnsmasq code base this option used to be called "dns-client-id" which
> is a lot clearer.  I could change it to this older name and describe
> it as above.  WDYT?

I'm not familiar enough with DNS to tell which name would be better.
In any case, it seems like you have some material for a description
here.  Additionally, you could add a cross-reference to the relevant
RFC+section number, if you can locate it.

Greetings,
Maxime.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

[bug#54352] [PATCH v2] services: dnsmasq: Add more options.

[Maxime Devos]

[-- Attachment #1: Type: text/plain, Size: 516 bytes --]

Remco van 't Veer schreef op zo 20-03-2022 om 14:15 [+0100]:
> I totally agree with the naming suggestions and will apply them but what
> if somebody has been using dnsmasq before?  In that case they will not
> recognize the options and which may cause confusion.  I guess the answer
> to that is that guix should not propagate the wording mistakes made
> upstream?

Perhaps some lines like

  ‘This option corresponds to the --foo-bar command line option.’

could be added?

Greetings,
Maxime.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

[bug#54352] [PATCH v2] services: dnsmasq: Add more options.

[Maxime Devos]

[-- Attachment #1: Type: text/plain, Size: 333 bytes --]

Remco van 't Veer schreef op zo 20-03-2022 om 14:15 [+0100]:
> [...] I could add a config-file field.  WDYT?

I don't think that's necessary but it's certainly an option.
This raises some questions though: if something is specified
in both the fields and the configuration file, what will dnsmasq
use?

Greetings,
Maxime.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

[bug#54352] [PATCH] services: dnsmasq: Add more options.

[Ludovic Courtès]

Hi Maxime,

Maxime Devos <maximedevos@telenet.be> skribis:

> Remco van 't Veer schreef op zo 20-03-2022 om 12:44 [+0100]:
>> +  (forward-private-reverse-lookup?
>> +   dnsmasq-configuration-forward-private-reverse-lookup?
>> +                    (default #t))       ;boolean
>> +  (strict-order?    dnsmasq-configuration-strict-order?
>> +                    (default #f))       ;boolean
>
> It would be nice to verify that these are, in fact, booleans,
> using field sanitizers.  See, e.g., ensure-setuid-program-list
> in (gnu system).

I think this suggestion is beyond the scope of this review: we’ve never
used sanitizers like this before (or almost), and this particular piece
of code doesn’t use them.

Also, with the recent discussion about the introduction of contracts,
I’d rather wait an use contracts everywhere once they’re available.

Thoughts?

Thanks,
Ludo’.

[bug#54352] [PATCH] services: dnsmasq: Add more options.

[Maxime Devos]

[-- Attachment #1: Type: text/plain, Size: 623 bytes --]

Ludovic Courtès schreef op ma 21-03-2022 om 16:22 [+0100]:
> I think this suggestion is beyond the scope of this review: we’ve never
> used sanitizers like this before (or almost), and this particular piece
> of code doesn’t use them.
> 
> Also, with the recent discussion about the introduction of contracts,
> I’d rather wait an use contracts everywhere once they’re available.

Seems reasonable to me, given that the specifics weren't discussed yet,
although _everywhere_ (for all procedures, records, ...) seems a bit
much, unless you meant every field of the dnsmasq record.

Greetings,
Maxime.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

[bug#54352] [PATCH] services: dnsmasq: Add more options.

[Remco van 't Veer]

2022/03/21 19:36, Maxime Devos:

> [[PGP Signed Part:Undecided]]
> Ludovic Courtès schreef op ma 21-03-2022 om 16:22 [+0100]:
>> I think this suggestion is beyond the scope of this review: we’ve never
>> used sanitizers like this before (or almost), and this particular piece
>> of code doesn’t use them.
>>
>> Also, with the recent discussion about the introduction of contracts,
>> I’d rather wait an use contracts everywhere once they’re available.
>
> Seems reasonable to me, given that the specifics weren't discussed yet,
> although _everywhere_ (for all procedures, records, ...) seems a bit
> much, unless you meant every field of the dnsmasq record.

I can add something like the following:

  (define (assert-boolean value)
    (unless (false-if-exception (boolean? value))
      (error-out (format #f "expected a boolean, got: ~s" value)))
    value)

and use it to do

  (sanitize assert-boolean)

on all boolean fields of dnsmasq but I agree with Ludo about this being
a bigger issue which can be solved much more elegantly (including i18n
and source location etc).  In the above I borrowed error-out from knot
in the same file which also doesn't do i18n and source locations.

I'd like to leave it out because the codebase has plenty more unverified
booleans and basic if-statements which go the unexpected route when
passed a "false" string.

[bug#54352] [PATCH v2] services: dnsmasq: Add more options.

[Remco van 't Veer]

2022/03/20 14:20, Maxime Devos:

> Remco van 't Veer schreef op zo 20-03-2022 om 14:15 [+0100]:
>> [...] I could add a config-file field.  WDYT?
>
> I don't think that's necessary but it's certainly an option.
> This raises some questions though: if something is specified
> in both the fields and the configuration file, what will dnsmasq
> use?

Ah, good point.  I'll prepare a new patch when it comes up and work it
out then.

[bug#54352] [PATCH v2] services: dnsmasq: Add more options.

[Remco van 't Veer]

2022/03/20 14:17, Maxime Devos:

> Perhaps some lines like
>
>   ‘This option corresponds to the --foo-bar command line option.’
>
> could be added?

I can't find any precedence in the guix manual about corresponding
command line options like that, so let's not.  Maybe it won't be
confusing and it's pretty easy to find the service definition to find
out.

[bug#54352] [PATCH v2] services: dnsmasq: Add more options.

[Remco van 't Veer]

2022/03/20 14:16, Maxime Devos:

> I'm not familiar enough with DNS to tell which name would be better.
> In any case, it seems like you have some material for a description
> here.  Additionally, you could add a cross-reference to the relevant
> RFC+section number, if you can locate it.

Well, that was a bit of a challenge but I did find out it's a EDNS
field, CPE stands for "Customer-premises Equipment" and that field is in
an IANA unassigned range but, apparently, used by DNS providers to
identify their clients/accounts.  So what do you think about:

  If set, add a CPE (Customer-Premises Equipment) identifier to DNS
  queries which are forwarded upstream.

I don't want to dwell on the unofficial status too much.  Also, I
changed the field name from additional-cpe-id to just cpe-id because
"additional" seems a bit redundant.

WDYT?

[bug#54352] [PATCH] services: dnsmasq: Add more options.

[Ludovic Courtès]

Hi,

Remco van 't Veer <remco@remworks.net> skribis:

> 2022/03/21 19:36, Maxime Devos:
>
>> [[PGP Signed Part:Undecided]]
>> Ludovic Courtès schreef op ma 21-03-2022 om 16:22 [+0100]:
>>> I think this suggestion is beyond the scope of this review: we’ve never
>>> used sanitizers like this before (or almost), and this particular piece
>>> of code doesn’t use them.
>>>
>>> Also, with the recent discussion about the introduction of contracts,
>>> I’d rather wait an use contracts everywhere once they’re available.
>>
>> Seems reasonable to me, given that the specifics weren't discussed yet,
>> although _everywhere_ (for all procedures, records, ...) seems a bit
>> much, unless you meant every field of the dnsmasq record.

Sorry, I wasn’t clear.  I mean, when we have contracts in Guix, we can
start using them “everywhere” in Guix, primarily for configuration
records.  But we’re not there yet and how exactly we’ll get there
remains to be seen.

> I can add something like the following:
>
>   (define (assert-boolean value)
>     (unless (false-if-exception (boolean? value))
>       (error-out (format #f "expected a boolean, got: ~s" value)))
>     value)
>
> and use it to do
>
>   (sanitize assert-boolean)

My suggestion is to just do nothing for the dnsmasq record; just leave a
comment about the expected type next to it and in the manual, as is done
for almost every other configuration record.

Eventually we’ll turn that into contracts with good error reporting, I
hope, but that’s not something you can do right now.  :-)

Thanks!

Ludo’.

[bug#54352] [PATCH v3] services: dnsmasq: Add more options.

[Remco van 't Veer]

* gnu/services/dns.scm (<dnsmasq-configuration>): Add
forward-private-reverse-lookup?, strict-order? and cpe-id options.
(dnsmasq-shepherd-service): Pass added options to dnsmasq and use
match-record instead of match-lambda.
* doc/guix.texi (Guix Services): Document options added to dnsmasq.
---

Changes from v2 to v3:

* renamed field additional-cpe-id to cpe-id
* improved documentation of cpe-id
* renamed field strict-order? to query-servers-in-order?

 doc/guix.texi        |  13 +++
 gnu/services/dns.scm | 183 ++++++++++++++++++++++++-------------------
 2 files changed, 115 insertions(+), 81 deletions(-)

diff --git a/doc/guix.texi b/doc/guix.texi
index 44b0f9f1ea..e8ef4286be 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -100,6 +100,7 @@ Copyright @copyright{} 2021 Josselin Poiret@*
 Copyright @copyright{} 2021 Andrew Tropin@*
 Copyright @copyright{} 2021 Sarah Morgensen@*
 Copyright @copyright{} 2021 Josselin Poiret@*
+Copyright @copyright{} 2022 Remco van 't Veer@*
 
 Permission is granted to copy, distribute and/or modify this document
 under the terms of the GNU Free Documentation License, Version 1.3 or
@@ -28945,6 +28946,14 @@ The file to read the IP address of the upstream nameservers from.
 @item @code{no-resolv?} (default: @code{#f})
 When true, don't read @var{resolv-file}.
 
+@item @code{forward-private-reverse-lookup?} (default: @code{#t})
+When false, all reverse lookups for private IP ranges are answered with
+"no such domain" rather than being forwarded upstream.
+
+@item @code{query-servers-in-order?} (default: @code{#f})
+When true, dnsmasq queries the servers in the same order as they appear
+in @var{servers}.
+
 @item @code{servers} (default: @code{'()})
 Specify IP address of upstream servers directly.
 
@@ -28974,6 +28983,10 @@ disables caching.
 @item @code{negative-cache?} (default: @code{#t})
 When false, disable negative caching.
 
+@item @code{cpe-id} (default: @code{#f})
+If set, add a CPE (Customer-Premises Equipment) identifier to DNS
+queries which are forwarded upstream.
+
 @item @code{tftp-enable?} (default: @code{#f})
 Whether to enable the built-in TFTP server.
 
diff --git a/gnu/services/dns.scm b/gnu/services/dns.scm
index 9b8603cc95..a220b33f15 100644
--- a/gnu/services/dns.scm
+++ b/gnu/services/dns.scm
@@ -3,6 +3,7 @@
 ;;; Copyright © 2018 Oleg Pykhalov <go.wigust@gmail.com>
 ;;; Copyright © 2020 Pierre Langlois <pierre.langlois@gmx.com>
 ;;; Copyright © 2021 Maxime Devos <maximedevos@telenet.be>
+;;; Copyright © 2022 Remco van 't Veer <remco@remworks.net>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -745,6 +746,12 @@ (define-record-type* <dnsmasq-configuration>
                     (default "/etc/resolv.conf")) ;string
   (no-resolv?       dnsmasq-configuration-no-resolv?
                     (default #f))       ;boolean
+  (forward-private-reverse-lookup?
+                    dnsmasq-configuration-forward-private-reverse-lookup?
+                    (default #t))       ;boolean
+  (query-servers-in-order?
+                    dnsmasq-configuration-query-servers-in-order?
+                    (default #f))       ;boolean
   (servers          dnsmasq-configuration-servers
                     (default '()))      ;list of string
   (addresses        dnsmasq-configuration-addresses
@@ -752,7 +759,9 @@ (define-record-type* <dnsmasq-configuration>
   (cache-size       dnsmasq-configuration-cache-size
                     (default 150))      ;integer
   (negative-cache?  dnsmasq-configuration-negative-cache?
-                    (default #t))      ;boolean
+                    (default #t))       ;boolean
+  (cpe-id           dnsmasq-configuration-cpe-id
+                    (default #t))       ;string
   (tftp-enable?     dnsmasq-configuration-tftp-enable?
                     (default #f))       ;boolean
   (tftp-no-fail?    dnsmasq-configuration-tftp-no-fail?
@@ -776,86 +785,98 @@ (define-record-type* <dnsmasq-configuration>
   (tftp-unique-root dnsmasq-tftp-unique-root
                     (default #f)))      ;"" or "ip" or "mac"
 
-(define dnsmasq-shepherd-service
-  (match-lambda
-    (($ <dnsmasq-configuration> package
-                                no-hosts?
-                                port local-service? listen-addresses
-                                resolv-file no-resolv? servers
-                                addresses cache-size negative-cache?
-                                tftp-enable? tftp-no-fail?
-                                tftp-single-port? tftp-secure?
-                                tftp-max tftp-mtu tftp-no-blocksize?
-                                tftp-lowercase? tftp-port-range
-                                tftp-root tftp-unique-root)
-     (shepherd-service
-      (provision '(dnsmasq))
-      (requirement '(networking))
-      (documentation "Run the dnsmasq DNS server.")
-      (start #~(make-forkexec-constructor
-                '(#$(file-append package "/sbin/dnsmasq")
-                  "--keep-in-foreground"
-                  "--pid-file=/run/dnsmasq.pid"
-                  #$@(if no-hosts?
-                         '("--no-hosts")
-                         '())
-                  #$(format #f "--port=~a" port)
-                  #$@(if local-service?
-                         '("--local-service")
-                         '())
-                  #$@(map (cut format #f "--listen-address=~a" <>)
-                          listen-addresses)
-                  #$(format #f "--resolv-file=~a" resolv-file)
-                  #$@(if no-resolv?
-                         '("--no-resolv")
-                         '())
-                  #$@(map (cut format #f "--server=~a" <>)
-                          servers)
-                  #$@(map (cut format #f "--address=~a" <>)
-                          addresses)
-                  #$(format #f "--cache-size=~a" cache-size)
-                  #$@(if negative-cache?
-                         '()
-                         '("--no-negcache"))
-                  #$@(if tftp-enable?
-                         '("--enable-tftp")
-                         '())
-                  #$@(if tftp-no-fail?
-                         '("--tftp-no-fail")
-                         '())
-                  #$@(if tftp-single-port?
-                         '("--tftp-single-port")
-                         '())
-                  #$@(if tftp-secure?
-                         '("--tftp-secure?")
-                         '())
-                  #$@(if tftp-max
-                         (list (format #f "--tftp-max=~a" tftp-max))
-                         '())
-                  #$@(if tftp-mtu
-                         (list (format #f "--tftp-mtu=~a" tftp-mtu))
-                         '())
-                  #$@(if tftp-no-blocksize?
-                         '("--tftp-no-blocksize")
-                         '())
-                  #$@(if tftp-lowercase?
-                         '("--tftp-lowercase")
-                         '())
-                  #$@(if tftp-port-range
-                         (list (format #f "--tftp-port-range=~a"
-                                          tftp-port-range))
-                         '())
-                  #$@(if tftp-root
-                         (list (format #f "--tftp-root=~a" tftp-root))
-                         '())
-                  #$@(if tftp-unique-root
-                         (list
-                          (if (> (length tftp-unique-root) 0)
-                              (format #f "--tftp-unique-root=~a" tftp-unique-root)
-                              (format #f "--tftp-unique-root")))
-                         '()))
-                #:pid-file "/run/dnsmasq.pid"))
-      (stop #~(make-kill-destructor))))))
+(define (dnsmasq-shepherd-service config)
+  (match-record config <dnsmasq-configuration>
+    (package
+     no-hosts?
+     port local-service? listen-addresses
+     resolv-file no-resolv?
+     forward-private-reverse-lookup? query-servers-in-order?
+     servers addresses
+     cache-size negative-cache?
+     cpe-id
+     tftp-enable? tftp-no-fail?
+     tftp-single-port? tftp-secure?
+     tftp-max tftp-mtu tftp-no-blocksize?
+     tftp-lowercase? tftp-port-range
+     tftp-root tftp-unique-root)
+    (shepherd-service
+     (provision '(dnsmasq))
+     (requirement '(networking))
+     (documentation "Run the dnsmasq DNS server.")
+     (start #~(make-forkexec-constructor
+               '(#$(file-append package "/sbin/dnsmasq")
+                 "--keep-in-foreground"
+                 "--pid-file=/run/dnsmasq.pid"
+                 #$@(if no-hosts?
+                        '("--no-hosts")
+                        '())
+                 #$(format #f "--port=~a" port)
+                 #$@(if local-service?
+                        '("--local-service")
+                        '())
+                 #$@(map (cut format #f "--listen-address=~a" <>)
+                         listen-addresses)
+                 #$(format #f "--resolv-file=~a" resolv-file)
+                 #$@(if no-resolv?
+                        '("--no-resolv")
+                        '())
+                 #$@(if forward-private-reverse-lookup?
+                        '()
+                        '("--bogus-priv"))
+                 #$@(if query-servers-in-order?
+                        '("--strict-order")
+                        '())
+                 #$@(map (cut format #f "--server=~a" <>)
+                         servers)
+                 #$@(map (cut format #f "--address=~a" <>)
+                         addresses)
+                 #$(format #f "--cache-size=~a" cache-size)
+                 #$@(if negative-cache?
+                        '()
+                        '("--no-negcache"))
+                 #$@(if cpe-id
+                        (list (format #f "--add-cpe-id=~a" cpe-id))
+                        '())
+                 #$@(if tftp-enable?
+                        '("--enable-tftp")
+                        '())
+                 #$@(if tftp-no-fail?
+                        '("--tftp-no-fail")
+                        '())
+                 #$@(if tftp-single-port?
+                        '("--tftp-single-port")
+                        '())
+                 #$@(if tftp-secure?
+                        '("--tftp-secure?")
+                        '())
+                 #$@(if tftp-max
+                        (list (format #f "--tftp-max=~a" tftp-max))
+                        '())
+                 #$@(if tftp-mtu
+                        (list (format #f "--tftp-mtu=~a" tftp-mtu))
+                        '())
+                 #$@(if tftp-no-blocksize?
+                        '("--tftp-no-blocksize")
+                        '())
+                 #$@(if tftp-lowercase?
+                        '("--tftp-lowercase")
+                        '())
+                 #$@(if tftp-port-range
+                        (list (format #f "--tftp-port-range=~a"
+                                      tftp-port-range))
+                        '())
+                 #$@(if tftp-root
+                        (list (format #f "--tftp-root=~a" tftp-root))
+                        '())
+                 #$@(if tftp-unique-root
+                        (list
+                         (if (> (length tftp-unique-root) 0)
+                             (format #f "--tftp-unique-root=~a" tftp-unique-root)
+                             (format #f "--tftp-unique-root")))
+                        '()))
+               #:pid-file "/run/dnsmasq.pid"))
+     (stop #~(make-kill-destructor)))))
 
 (define (dnsmasq-activation config)
   #~(begin
-- 
2.34.0

[bug#54352] [PATCH] services: dnsmasq: Add more options.

[Remco van 't Veer]

Just posted a v3.  Changes from v2 to v3:

* renamed field additional-cpe-id to cpe-id
* improved documentation of cpe-id
* renamed field strict-order? to query-servers-in-order?

R.

bug#54352: [PATCH] services: dnsmasq: Add more options.

[Ludovic Courtès]

Hi,

Remco van 't Veer <remco@remworks.net> skribis:

> * gnu/services/dns.scm (<dnsmasq-configuration>): Add
> forward-private-reverse-lookup?, strict-order? and cpe-id options.
> (dnsmasq-shepherd-service): Pass added options to dnsmasq and use
> match-record instead of match-lambda.
> * doc/guix.texi (Guix Services): Document options added to dnsmasq.

Applied, thank you!

Ludo’.