[bug#55912] [PATCH] home: Add OpenSSH service.

[Maxime Devos <maximedevos@telenet.be>]

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

Hi,

Some comments on the code.

Ludovic Courtès schreef op za 11-06-2022 om 18:49 [+0200]:
> +  (port
> +   (maybe-integer 'disabled)
> +   "TCP port number to connect to.")

TCP only allows natural numbers up to some bound, and in practice
implementations only support non-zero natural numbers, so maybe the
predicate can be refined a bit?

> +                             (formatted-message
> +                              (G_ "~s: unsupported address family")

Maybe a hint:

  hint: AF_INET and AF_INET6 are supported.


> + (define (serialize-string field value)
> +   (string-append "  " (serialize-field-name field)
> +                  " " value "\n"))

> +  (name
> +   (string)
> +   "Name of this host declaration.")
> [...]
> +  (proxy-command
> +   (maybe-string 'disabled)

Attila Lendvai has a patch series at 54674 that changes 'disabled' ->
*unspecified* -- I think it would be better to apply that patch series
first.

Wouldn't the value need to be escaped?  Or at least a check that it
doesn't contain special characters like \n or whatever special
charaters an OpenSSH configuration has.


>+ (define* (file-join name files #:optional (delimiter " "))
>+  "Return a file in the store called @var{name} that is the
>+ concatenation
>+ of all the file-like objects listed in @var{files}, with
@var{delimited}
>+ inserted after each of them."

Does this work for files with non-ASCII characters and for file names
that contain non-ASCII characters?

>+          (service-extension home-profile-service-type
>+                             (compose
>+                              list
>+                              home-openssh-configuration-openssh))
>+          (service-extension home-activation-service-type
>+                             (const openssh-activation))))
>+   (description "Configure the OpenSSH @acronym{SSH, secure shell}
>+client and _add it to the user profile_.")

(emphasis added).  Why is it automagically added to the user profile? 
This is considered bad practice for system services.  Maybe the user
keeps all their remote communication things in a single profile, maybe
the user only uses openssh things via other tools like 'guix deploy' or
'gnome-shell-extension-gsconnect' and hence has no need for 'openssh'
in their home profile.   Maybe the user never ssh's _from_ the computer
that has the openssh home configuration and only connects _to_ the
computer and hence the 'openssh' in the profile isn't necessary.

Now there are two ways to add 'openssh' to the environment: the Guix
Home equivalent of a 'packages' field and the openssh home service,
with AFAICT no mechanism for deciding which one ‘wins’ and no mechanism
for a proper error message like ‘only add the openssh package to the
profile or use the openssh home service, not both!’, which doesn't seem
ideal to me.

reetings,
Maxime.

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