diff mbox series

[bug#63786] home: services: ssh: Allow unset boolean options in ssh-config.

Message ID 6f1959b0041895af538fec1b72a02d7767451767.1685371966.git.efraim@flashner.co.il
State New
Headers show
Series [bug#63786] home: services: ssh: Allow unset boolean options in ssh-config. | expand

Commit Message

Efraim Flashner May 29, 2023, 2:52 p.m. UTC
From man 5 ssh_config:
Unless noted otherwise, for each parameter, the first obtained value
will be used.

We want to allow falling through to the first actual user defined value.

* gnu/home/services.ssh.scm (define-maybe boolean): New configuration.
(openssh-host)[forward-x11?, forward-x11-trusted?, forward-agent?,
compression?]: Replace default value with maybe-boolean.
* doc/guix.texi (Secure Shell): Update documentation to match the
changes in the code.
---
 doc/guix.texi             | 10 +++++-----
 gnu/home/services/ssh.scm | 11 +++++++----
 2 files changed, 12 insertions(+), 9 deletions(-)


base-commit: 7b400e7f8751e6b0cc6e66d3f7ecfb7f5bd51309

Comments

Ludovic Courtès June 8, 2023, 8:57 p.m. UTC | #1
Hello!

Efraim Flashner <efraim@flashner.co.il> skribis:

>>From man 5 ssh_config:
> Unless noted otherwise, for each parameter, the first obtained value
> will be used.
>
> We want to allow falling through to the first actual user defined value.

What do you mean by “first actual user-defined value”?  This service is
what generates all the “user-defined values”, no?

Overall my take is that default values should be specified in our code
(as default values of configuration record fields) rather than left
unspecified.  I think this is clearer and more predictable than relying
on upstream’s default values.

Thanks,
Ludo’.
Efraim Flashner June 11, 2023, 7:49 a.m. UTC | #2
options in ssh-config.
Reply-To: 
X-PGP-Key-ID: 0x41AAE7DCCA3D8351
X-PGP-Key: https://flashner.co.il/~efraim/efraim_flashner.asc
X-PGP-Fingerprint: A28B F40C 3E55 1372 662D  14F7 41AA E7DC CA3D 8351

For some reason this didn't get sent to the bug.
Andrew Tropin June 12, 2023, 4:58 a.m. UTC | #3
On 2023-06-11 10:49, Efraim Flashner wrote:

> options in ssh-config.
> Reply-To: 
> X-PGP-Key-ID: 0x41AAE7DCCA3D8351
> X-PGP-Key: https://flashner.co.il/~efraim/efraim_flashner.asc
> X-PGP-Fingerprint: A28B F40C 3E55 1372 662D  14F7 41AA E7DC CA3D 8351
>
> For some reason this didn't get sent to the bug.
>
> -- 
> Efraim Flashner   <efraim@flashner.co.il>   רנשלפ םירפא
> GPG key = A28B F40C 3E55 1372 662D  14F7 41AA E7DC CA3D 8351
> Confidentiality cannot be guaranteed on emails sent or received unencrypted
> From: Efraim Flashner <efraim@flashner.co.il>
> Subject: Re: bug#63786: [PATCH] home: services: ssh: Allow unset boolean options in ssh-config.
> To: Ludovic Courtès <ludo@gnu.org>
> Date: Fri, 09 Jun 2023 16:24:26 +0300
>
> On Thu, Jun 08, 2023 at 10:57:37PM +0200, Ludovic Courtès wrote:
>> Hello!
>> 
>> Efraim Flashner <efraim@flashner.co.il> skribis:
>> 
>> >>From man 5 ssh_config:
>> > Unless noted otherwise, for each parameter, the first obtained value
>> > will be used.
>> >
>> > We want to allow falling through to the first actual user defined value.
>> 
>> What do you mean by “first actual user-defined value”?  This service is
>> what generates all the “user-defined values”, no?
>
> Right now my ~/.ssh/config has
>
> Host do1-tor
>     Hostname <insert tor address>
>     IdentityFile ~/.ssh/id_ed25519
> Host *.onion *-tor
>     #ProxyCommand /gnu/store/dgvybjrj154f4cyfbkrbqyirv5gd8ic2-netcat-openbsd-1.218-2/bin/nc -X 5 -x localhost:9050 %h %p
>     ProxyCommand /home/efraim/bin/openbsd-netcat -X 5 -x localhost:9050 %h %p
>     ControlPath ${XDG_RUNTIME_DIR}/%r@%k-%p
>     Compression yes
>
> The way the ssh config is read is that `ssh do1-tor` first matches
> do1-tor and then also matches *-tor, so I can factor our ProxyCommand,
> ControlPath and Compression for use with the other *-tor Hosts I have
> listed.
>
> This configuration could be
> (openssh-host (name "do1-tor")
>               (host-name <insert tor address>)
>               (identity-file "~/.ssh/id_ed25519"))
> (openssh-host (name "*-onion *-tor)
>               (compression? #t)
>               (proxy
>                (proxy-command ...))
>               (extra-content "  ControlPath ...\n"))
>
> If this is all I enter, then my .ssh/config is generated like this:
>
> Host do1-tor
>   Hostname <insert tor address>
>   IdentityFile ~/.ssh/id_ed25519
>   ForwardX11 no
>   ForwardX11Trusted no
>   ForwardAgent no
>   Compression no
> Host *.onion *-tor
>   ForwardX11 no
>   ForwardX11Trusted no
>   ForwardAgent no
>   Compression yes
>   ProxyCommand /home/efraim/bin/openbsd-netcat -X 5 -x localhost:9050 %h %p
>   ControlPath ${XDG_RUNTIME_DIR}/%r@%k-%p
>
> Compression might default to no, but in my hand crafted .ssh/config I've
> set it to yes for *-tor Hosts. Forward* might all default to no, and
> it's not set anywhere, but being explicit about the default here could
> cause problems if I want X11 forwarding across an entire range of hosts,
> not just individual ones.
>
>> Overall my take is that default values should be specified in our code
>> (as default values of configuration record fields) rather than left
>> unspecified.  I think this is clearer and more predictable than relying
>> on upstream’s default values.
>
> In general this is a good plan, but here it actually interferes with the
> expected configuration output. 'Fall through' is the default, not the
> actual default for each of the individual configuration options. They
> only get set if that field isn't set by any of the possibly multiple
> configuration matches set it first.

A few years ago, when we were implementing the first version of ssh home
service in rde we went a slightly different way and didn't hardcode any
record fields and let user set an alist of key/value pairs:
https://git.sr.ht/~abcdw/rde/tree/19c2d2f0996624eea8b7a87b14bbc31e4a9b943b/src/gnu/home-services/ssh.scm#L204

It's not a perfect solution either, but quite flexible.  Also, it's
relatively easy to implement default values: we can provide
%default-host-options and ask people to do something like this on user
side configuration:

(merge %default-host-options '((compression . #f)))

Of course "asking people" won't work, so it's possible to set a default
value of options field to %default-host-options
https://git.sr.ht/~abcdw/rde/tree/19c2d2f0996624eea8b7a87b14bbc31e4a9b943b/src/gnu/home-services/ssh.scm#L100
and let people override it with '((compression . #f)) or enrich with
(merge %default-host-options '((compression . #f))).

It's not a proposal or something, just sharing how it's implemented in
rde.


P.S. Note that (gnu home-services *) modules are subject to deprecation
and when (rde home services ssh) appear, it will have a slightly
different interface.
Efraim Flashner June 14, 2023, 7:16 p.m. UTC | #4
On Mon, Jun 12, 2023 at 08:58:18AM +0400, Andrew Tropin wrote:
> 
> A few years ago, when we were implementing the first version of ssh home
> service in rde we went a slightly different way and didn't hardcode any
> record fields and let user set an alist of key/value pairs:
> https://git.sr.ht/~abcdw/rde/tree/19c2d2f0996624eea8b7a87b14bbc31e4a9b943b/src/gnu/home-services/ssh.scm#L204
> 
> It's not a perfect solution either, but quite flexible.  Also, it's
> relatively easy to implement default values: we can provide
> %default-host-options and ask people to do something like this on user
> side configuration:
> 
> (merge %default-host-options '((compression . #f)))
> 
> Of course "asking people" won't work, so it's possible to set a default
> value of options field to %default-host-options
> https://git.sr.ht/~abcdw/rde/tree/19c2d2f0996624eea8b7a87b14bbc31e4a9b943b/src/gnu/home-services/ssh.scm#L100
> and let people override it with '((compression . #f)) or enrich with
> (merge %default-host-options '((compression . #f))).
> 
> It's not a proposal or something, just sharing how it's implemented in
> rde.

I'm still undecided about the alist as a comparison. It would make it
easier to add arbitrary fields, but then I feel like maybe we should be
adding something to validate the configurations.

> P.S. Note that (gnu home-services *) modules are subject to deprecation
> and when (rde home services ssh) appear, it will have a slightly
> different interface.


I went ahead and pushed the patch. I believe that, after having added to
a .ssh/config file over a period of time, line by line or entry by
entry, people will be surprised to see a bunch of fields filled in
automatically, and with different results from what they had before.
diff mbox series

Patch

diff --git a/doc/guix.texi b/doc/guix.texi
index 31dc33fb97..d22924e522 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -33,7 +33,7 @@ 
 Copyright @copyright{} 2015, 2016, 2017, 2018, 2019, 2020, 2021, 2022, 2023 Ricardo Wurmus@*
 Copyright @copyright{} 2016 Ben Woodcroft@*
 Copyright @copyright{} 2016, 2017, 2018, 2021 Chris Marusich@*
-Copyright @copyright{} 2016, 2017, 2018, 2019, 2020, 2021, 2022 Efraim Flashner@*
+Copyright @copyright{} 2016, 2017, 2018, 2019, 2020, 2021, 2022, 2023 Efraim Flashner@*
 Copyright @copyright{} 2016 John Darrington@*
 Copyright @copyright{} 2016, 2017 Nikita Gillmann@*
 Copyright @copyright{} 2016, 2017, 2018, 2019, 2020, 2021, 2022, 2023 Jan Nieuwenhuizen@*
@@ -43017,19 +43017,19 @@  Secure Shell
 @item @code{user} (type: maybe-string)
 User name on the remote host.
 
-@item @code{forward-x11?} (default: @code{#f}) (type: boolean)
+@item @code{forward-x11?} (type: maybe-boolean)
 Whether to forward remote client connections to the local X11 graphical
 display.
 
-@item @code{forward-x11-trusted?} (default: @code{#f}) (type: boolean)
+@item @code{forward-x11-trusted?} (type: maybe-boolean)
 Whether remote X11 clients have full access to the original X11
 graphical display.
 
-@item @code{forward-agent?} (default: @code{#f}) (type: boolean)
+@item @code{forward-agent?} (type: maybe-boolean)
 Whether the authentication agent (if any) is forwarded to the remote
 machine.
 
-@item @code{compression?} (default: @code{#f}) (type: boolean)
+@item @code{compression?} (type: maybe-boolean)
 Whether to compress data in transit.
 
 @item @code{proxy} (type: maybe-proxy-command-or-jump-list)
diff --git a/gnu/home/services/ssh.scm b/gnu/home/services/ssh.scm
index 628dc743ae..0a4b37d84e 100644
--- a/gnu/home/services/ssh.scm
+++ b/gnu/home/services/ssh.scm
@@ -1,6 +1,7 @@ 
 ;;; GNU Guix --- Functional package management for GNU
 ;;; Copyright © 2022 Ludovic Courtès <ludo@gnu.org>
 ;;; Copyright © 2023 Janneke Nieuwenhuizen <janneke@gnu.org>
+;;; Copyright © 2023 Efraim Flashner <efraim@flashner.co.il>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -104,6 +105,8 @@  (define (serialize-natural-number field value)
   (string-append "  " (serialize-field-name field) " "
                  (number->string value) "\n"))
 
+(define-maybe boolean)
+
 (define (serialize-boolean field value)
   (string-append "  " (serialize-field-name field) " "
                  (if value "yes" "no") "\n"))
@@ -194,19 +197,19 @@  (define-configuration openssh-host
    maybe-string
    "User name on the remote host.")
   (forward-x11?
-   (boolean #f)
+   maybe-boolean
    "Whether to forward remote client connections to the local X11 graphical
 display.")
   (forward-x11-trusted?
-   (boolean #f)
+   maybe-boolean
    "Whether remote X11 clients have full access to the original X11 graphical
 display.")
   (forward-agent?
-   (boolean #f)
+   maybe-boolean
    "Whether the authentication agent (if any) is forwarded to the remote
 machine.")
   (compression?
-   (boolean #f)
+   maybe-boolean
    "Whether to compress data in transit.")
   (proxy-command
    maybe-string