[bug#68675,v3,2/2] services: dhcp: Support the dhcpcd implementation.

From: Sören Tempel <soeren@soeren-tempel.net>

Prior to this commit, the isc-dhcp implementation was the only DHCP
implementation supported by dhcp-client-shepherd-service. This is
problematic as the ISC implementation has reached end-of-life in
2022(!). As a first step to migrate away from isc-dhcp, this commit
adds support for dhcpcd to dhcp-client-shepherd-service. Currently,
it has to be enabled explicitly via the package field of the
dhcp-client-configuration. In the future, it is intended to become
the default to migrate away from isc-dhcp.

While at it, also remove isc-dhcp from %base-packages as it is no
longer necessarily needed and it will be pulled in by the DHCP
client service if required.

See also: https://issues.guix.gnu.org/68619

* gnu/services/networking.scm (dhcp-client-shepherd-service): Add
  support for the dhcpcd client implementation.
* gnu/services/networking.scm (dhcp-client-account-service): New
  procedure.
* gnu/services/networking.scm (dhcp-client-service-type): Add optional
  account-service-type extensions (needed for dhcpcd).
* gnu/system.scm (%base-packages-networking): Remove isc-dhcp from
  %base-packages (will be pulled in by dhcp-client-shepherd-service).

Signed-off-by: Sören Tempel <soeren@soeren-tempel.net>
---
 gnu/services/networking.scm | 92 +++++++++++++++++++++++++++----------
 1 file changed, 67 insertions(+), 25 deletions(-)

Hello,

soeren@soeren-tempel.net skribis:

> * gnu/services/networking.scm (dhcp-client-shepherd-service): Add
>   support for the dhcpcd client implementation.
> * gnu/services/networking.scm (dhcp-client-account-service): New
>   procedure.
> * gnu/services/networking.scm (dhcp-client-service-type): Add optional
>   account-service-type extensions (needed for dhcpcd).
> * gnu/system.scm (%base-packages-networking): Remove isc-dhcp from
>   %base-packages (will be pulled in by dhcp-client-shepherd-service).

[...]

> +                         ;; Returns the execution configuration for the DHCP client
> +                         ;; selected by the package field of dhcp-client-configuration.
> +                         ;; The configuration is a pair of pidfile and execution command
> +                         ;; where the latter is a list.
> +                         (define exec-config
> +                           (case (string->symbol #$client-name)
> +                             ((isc-dhcp)
> +                              (let ((pid-file "/var/run/dhclient.pid"))
> +                                (cons
> +                                  (cons* (string-append #$package "/sbin/dhclient")
> +                                         "-nw" "-I" "-pf" pid-file ifaces)
> +                                  pid-file)))
> +                             ((dhcpcd)
> +                              ;; For dhcpcd, the utilized pid-file depends on the
> +                              ;; command-line arguments.  If multiple interfaces are
> +                              ;; given, a different pid-file is returned.  Hence, we
> +                              ;; consult dhcpcd itself to determine the pid-file.
> +                              (let* ((cmd (string-append #$package "/sbin/dhcpcd"))
> +                                     (arg (cons* cmd "-b" ifaces)))
> +                                (cons arg
> +                                  (let* ((pipe (string-join (append arg '("-P")) " "))
> +                                         (port (open-input-pipe pipe))
> +                                         (path (read-line port)))
> +                                    (close-pipe port)
> +                                    path))))
> +                             (else
> +                               (display
> +                                 "unknown 'package' value in dhcp-client-configuration"
> +                                 (current-error-port))
> +                               (newline (current-error-port))
> +                               #f)))
> +

I would very much like to avoid the ‘open-input-pipe’ dance here.  Maybe
there’s a way to ask it to not fork, in which case we don’t need to wait
for a PID file?  (I’ll give up if this really really can’t be avoided.  :-))

> +                         (and
> +                           exec-config
> +                           (let ((pid-file (cdr exec-config))
> +                                 (exec-cmd (car exec-config)))
> +                             (false-if-exception (delete-file pid-file))
> +                             (let ((pid (fork+exec-command exec-cmd)))
> +                               (and (zero? (cdr (waitpid pid)))
> +                                    (read-pid-file pid-file)))))))

Two notes: I would find it clearer to call ‘fork+exec-command’ above
instead of constructing ‘exec-config’.

Ideally, we’d use:

  (start #~(if (file-exists? (string-append #$package "/bin/dhclient"))
               (make-forkexec-constructor …)   ;ISC version, with #:pid-file
               (make-forkexec-constructor …))) ;dhcpcd, maybe without #:pid-file

Maybe that is too naive, but at least we should get as close as possible
to that, for instance by avoiding direct calls to ‘fork+exec-command’ +
‘waitpid’ + ‘read-pid-file’ as much as possible.

HTH!

Ludo’.

Hi,

Ludovic Courtès <ludo@gnu.org> wrote:
> Apologies for the late reply.

No worries, I understand that you have a lot of other things to do :)

> I would very much like to avoid the ‘open-input-pipe’ dance here.  Maybe
> there’s a way to ask it to not fork, in which case we don’t need to wait
> for a PID file?

When using a PID file, AFAIK the only thing we can do is replicate the C
code that I referenced in my prior email for determining the PID file
name [1]. Is there a specific reason why you want to avoid using
open-input-pipe?

> What do others do?  I guess it’s not possible to have a systemd .service
> file given those PID file semantics, is it?

systemd and other service supervisor do not rely on PID files for
services supervision as they are inherently racy. Therefore, as
mentioned in my prior email, I also think it would be preferable to not
use a PID file for supervising either dhclient or dhcpcd.

I only ended up using a PID file here as dhclient already used one and I
wanted to keep the service changes as minimal as possible to ensure a
swift review since I believe this to be a security-critical issue (see
Guix issue #68619).

> Two notes: I would find it clearer to call ‘fork+exec-command’ above
> instead of constructing ‘exec-config’.
> 
> Ideally, we’d use:
> 
>   (start #~(if (file-exists? (string-append #$package "/bin/dhclient"))
>                (make-forkexec-constructor …)   ;ISC version, with #:pid-file
>                (make-forkexec-constructor …))) ;dhcpcd, maybe without #:pid-file

If we separate the dhclient and the dhcpcd code like this, then it may
be worthwhile to have entirely separated services instead of combining
them both in a single service. This would also ease providing a
configuration for these services.

> Maybe that is too naive, but at least we should get as close as possible
> to that, for instance by avoiding direct calls to ‘fork+exec-command’ +
> ‘waitpid’ + ‘read-pid-file’ as much as possible.

Ideally, I would not want to refactor existing service code as much in
this patchset. As mentioned above, I believe Guix using dhclient by
default (which has been EOL for 2 years) has security implications.
Therefore, I would want to migrate dhcp-client-service-type away from
dhclient to dhcpcd as soon as possible. As part of this migration, I
would strongly suggest a deprecation and removal of dhclient support
from dhcp-client-service-type. As soon as dhclient support is removed,
refactorings of dhcp-client-service-type can be conducted. Including a
removal of PID files for supervision, instead supervising dhcpcd by
spawning it as non-double-forking child process [2].

Greetings
Sören

[1]: https://github.com/NetworkConfiguration/dhcpcd/blob/v10.0.6/src/dhcpcd.c#L2144
[2]: https://github.com/nmeum/guix-channel/blob/b1b80697a9d35ca015ce56ccf3031f91f2bf554f/src/nmeum/services/networking.scm#L209-L211