Subject: Re: [PATCH v3 2/2] services: childhurd: Support installing secrets from the host.

From: Ludovic Courtès <[email-masked]>
Date: Tue, 01 Sep 2020 10:37:40 +0200

"Jan (janneke) Nieuwenhuizen" <[email-masked]> skribis:

> * gnu/system/examples/bare-hurd.tmpl (%hurd-os)[services]: Add secret-service.
> * gnu/services/virtualization.scm (%hurd-vm-operating-system): Likewise.
>  (hurd-vm-shepherd-service): Use it to install secrets.
> * doc/guix.texi (The Hurd in a Virtual Machine): Document it.

Yay, minor issues, but overall LGTM!\

>      (services (cons*
> +               ;; Receive secret keys on port 1004, TCP.
> +               (service secret-service-type 1004)

[...]

> +      (start
> +       (with-imported-modules
> +           (source-module-closure '((gnu build secret-service)
> +                                    (guix build utils)))
> +         #~(let ((spawn (make-forkexec-constructor #$vm-command)))
> +           (lambda _
> +             (let ((pid (spawn))
> +                   (port #$(hurd-vm-port config %hurd-vm-secrets-port))
> +                   (root #$(hurd-vm-configuration-secret-root config)))
> +               (and root (directory-exists? root)
> +                    (catch #t
> +                      (lambda _
> +                        (secret-service-send-secrets port root))

Perhaps ‘hurd-vm-service-type’ should unconditionally extend (via
‘service-extension’) ‘secret-service-type’, just to ensure that Hurd VMs
always include the secret service.

In any case, we should assume that the VM is always running the secret
service server, and thus call ‘secret-service-send-secrets’
unconditionally (‘secret-service-send-secrets’ does (find-files root),
which returns the empty list when ROOT doesn’t exist, I think.)

> +                      (lambda (keys . args)

Should be “key” (singular).

> +                        (format (current-error-port)
> +                                "failed to send secrets: ~a ~s\n" key args)
> +                        (kill pid)

(kill (- pid)) to kill the whole process group (just in case).

I’d remove the ‘format’ call and just re-throw the exception: shepherd
should report it correctly.

[...]

> +                            (service (@@ (gnu services virtualization)
> +                                         secret-service-type) 5999))

This is useful for testing but I wouldn’t commit it (in particular
because the example would no longer work for people who’re just spawning
the VM and not trying to feed it secrets over TCP).

That’s it, thanks a lot!

Ludo’.