[bug#38478] [PATCH 0/4] "guix deploy" authenticates SSH servers [security]

[bug#38478] [PATCH 0/4] "guix deploy" authenticates SSH servers [security]

[Ludovic Courtès]

Hi!

This series allow users to specify the remote host key in
<machine-ssh-configuration> used for “guix deploy”, so you
can have that under version control and entirely managed by
Guix, like “guix offload” does.

The second patch fixes a security issue: ‘open-ssh-session’ from
(guix ssh), which is used by “guix deploy” and support for
“GUIX_DAEMON_SOCKET=ssh://…” in (guix store ssh), would not
authenticate the server it’s talking to.

Feedback welcome!

Ludo’.

Ludovic Courtès (4):
  ssh: Add 'authenticate-server*' and use it for offloading.
  ssh: Always authenticate the server [security fix].
  ssh: 'open-ssh-session' can be passed the expected host key.
  machine: ssh: <machine-ssh-configuration> can include the host key.

 doc/guix.texi            | 12 +++++++
 gnu/machine/ssh.scm      |  9 ++++--
 guix/scripts/offload.scm | 30 ++---------------
 guix/ssh.scm             | 69 ++++++++++++++++++++++++++++++++++++++--
 4 files changed, 87 insertions(+), 33 deletions(-)

-- 
2.24.0

[bug#38478] [PATCH 1/4] ssh: Add 'authenticate-server*' and use it for offloading.

[Ludovic Courtès]

* guix/scripts/offload.scm (host-key->type+key): Remove.
(open-ssh-session): Replace server authentication code with a call to
'authenticate-server*'.
* guix/ssh.scm (host-key->type+key, authenticate-server*): New
procedures.
---
 guix/scripts/offload.scm | 30 ++----------------------------
 guix/ssh.scm             | 37 +++++++++++++++++++++++++++++++++++++
 2 files changed, 39 insertions(+), 28 deletions(-)

diff --git a/guix/scripts/offload.scm b/guix/scripts/offload.scm
index 18473684eb..e81b6c25f2 100644
--- a/guix/scripts/offload.scm
+++ b/guix/scripts/offload.scm
@@ -149,19 +149,6 @@ ignoring it~%")
          (leave (G_ "failed to load machine file '~a': ~s~%")
                 file args))))))
 
-(define (host-key->type+key host-key)
-  "Destructure HOST-KEY, an OpenSSH host key string, and return two values:
-its key type as a symbol, and the actual base64-encoded string."
-  (define (type->symbol type)
-    (and (string-prefix? "ssh-" type)
-         (string->symbol (string-drop type 4))))
-
-  (match (string-tokenize host-key)
-    ((type key x)
-     (values (type->symbol type) key))
-    ((type key)
-     (values (type->symbol type) key))))
-
 (define (private-key-from-file* file)
   "Like 'private-key-from-file', but raise an error that 'with-error-handling'
 can interpret meaningfully."
@@ -203,21 +190,8 @@ private key from '~a': ~a")
                                (build-machine-compression-level machine))))
     (match (connect! session)
       ('ok
-       ;; Authenticate the server.  XXX: Guile-SSH 0.10.1 doesn't know about
-       ;; ed25519 keys and 'get-key-type' returns #f in that case.
-       (let-values (((server)   (get-server-public-key session))
-                    ((type key) (host-key->type+key
-                                 (build-machine-host-key machine))))
-         (unless (and (or (not (get-key-type server))
-                          (eq? (get-key-type server) type))
-                      (string=? (public-key->string server) key))
-           ;; Key mismatch: something's wrong.  XXX: It could be that the server
-           ;; provided its Ed25519 key when we where expecting its RSA key.
-           (leave (G_ "server at '~a' returned host key '~a' of type '~a' \
-instead of '~a' of type '~a'~%")
-                  (build-machine-name machine)
-                  (public-key->string server) (get-key-type server)
-                  key type)))
+       ;; Make sure the server's key is what we expect.
+       (authenticate-server* session (build-machine-host-key machine))
 
        (let ((auth (userauth-public-key! session private)))
          (unless (eq? 'success auth)
diff --git a/guix/ssh.scm b/guix/ssh.scm
index 5fd3c280e8..f34e71392b 100644
--- a/guix/ssh.scm
+++ b/guix/ssh.scm
@@ -37,6 +37,8 @@
   #:use-module (ice-9 format)
   #:use-module (ice-9 binary-ports)
   #:export (open-ssh-session
+            authenticate-server*
+
             remote-inferior
             remote-daemon-channel
             connect-to-remote-daemon
@@ -60,6 +62,41 @@
 (define %compression
   "zlib@openssh.com,zlib")
 
+(define (host-key->type+key host-key)
+  "Destructure HOST-KEY, an OpenSSH host key string, and return two values:
+its key type as a symbol, and the actual base64-encoded string."
+  (define (type->symbol type)
+    (and (string-prefix? "ssh-" type)
+         (string->symbol (string-drop type 4))))
+
+  (match (string-tokenize host-key)
+    ((type key x)
+     (values (type->symbol type) key))
+    ((type key)
+     (values (type->symbol type) key))))
+
+(define (authenticate-server* session key)
+  "Make sure the server for SESSION has the given KEY, where KEY is a string
+such as \"ssh-ed25519 AAAAC3Nz… root@example.org\".  Raise an exception if the
+actual key does not match."
+  (let-values (((server)   (get-server-public-key session))
+               ((type key) (host-key->type+key key)))
+    (unless (and (or (not (get-key-type server))
+                     (eq? (get-key-type server) type))
+                 (string=? (public-key->string server) key))
+      ;; Key mismatch: something's wrong.  XXX: It could be that the server
+      ;; provided its Ed25519 key when we where expecting its RSA key.  XXX:
+      ;; Guile-SSH 0.10.1 doesn't know about ed25519 keys and 'get-key-type'
+      ;; returns #f in that case.
+      (raise (condition
+              (&message
+               (message (format #f (G_ "server at '~a' returned host key \
+'~a' of type '~a' instead of '~a' of type '~a'~%")
+                                (session-get session 'host)
+                                (public-key->string server)
+                                (get-key-type server)
+                                key type))))))))
+
 (define* (open-ssh-session host #:key user port identity
                            (compression %compression)
                            (timeout 3600))
-- 
2.24.0

[bug#38478] [PATCH 2/4] ssh: Always authenticate the server [security fix].

[Ludovic Courtès]

Until now, users of 'open-ssh-session', including "guix deploy" and
"GUIX_DAEMON_SOCKET=ssh://…" (but not "guix offload"), would not
authenticate the SSH server they're talking to.

* guix/ssh.scm (open-ssh-session): Call 'authenticate-server'.
---
 guix/ssh.scm | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/guix/ssh.scm b/guix/ssh.scm
index f34e71392b..519c723155 100644
--- a/guix/ssh.scm
+++ b/guix/ssh.scm
@@ -125,6 +125,17 @@ Throw an error on failure."
 
     (match (connect! session)
       ('ok
+       ;; Authenticate against ~/.ssh/known_hosts.
+       (match (authenticate-server session)
+         ('ok #f)
+         (reason
+          (raise (condition
+                  (&message
+                   (message (format #f (G_ "failed to authenticate \
+server at '~a': ~a")
+                                    (session-get session 'host)
+                                    reason)))))))
+
        ;; Use public key authentication, via the SSH agent if it's available.
        (match (userauth-public-key/auto! session)
          ('success
-- 
2.24.0

[bug#38478] [PATCH 3/4] ssh: 'open-ssh-session' can be passed the expected host key.

[Ludovic Courtès]

* guix/ssh.scm (open-ssh-session): Add #:host-key parameter.
Pass #:knownhosts to 'make-session'.  When HOST-KEY is true, call
'authenticate-server*' instead of 'authenticate-server'.
---
 guix/ssh.scm | 39 +++++++++++++++++++++++++++------------
 1 file changed, 27 insertions(+), 12 deletions(-)

diff --git a/guix/ssh.scm b/guix/ssh.scm
index 519c723155..291ce20b61 100644
--- a/guix/ssh.scm
+++ b/guix/ssh.scm
@@ -98,14 +98,20 @@ actual key does not match."
                                 key type))))))))
 
 (define* (open-ssh-session host #:key user port identity
+                           host-key
                            (compression %compression)
                            (timeout 3600))
   "Open an SSH session for HOST and return it.  IDENTITY specifies the file
 name of a private key to use for authenticating with the host.  When USER,
 PORT, or IDENTITY are #f, use default values or whatever '~/.ssh/config'
-specifies; otherwise use them.  Install TIMEOUT as the maximum time in seconds
-after which a read or write operation on a channel of the returned session is
-considered as failing.
+specifies; otherwise use them.
+
+When HOST-KEY is true, it must be a string like \"ssh-ed25519 AAAAC3Nz…
+root@example.org\"; the server is authenticated and an error is raised if its
+host key is different from HOST-KEY.
+
+Install TIMEOUT as the maximum time in seconds after which a read or write
+operation on a channel of the returned session is considered as failing.
 
 Throw an error on failure."
   (let ((session (make-session #:user user
@@ -115,6 +121,11 @@ Throw an error on failure."
                                #:timeout 10       ;seconds
                                ;; #:log-verbosity 'protocol
 
+                               ;; Prevent libssh from reading
+                               ;; ~/.ssh/known_hosts when the caller provides
+                               ;; a HOST-KEY to match against.
+                               #:knownhosts (and host-key "/dev/null")
+
                                ;; We need lightweight compression when
                                ;; exchanging full archives.
                                #:compression compression
@@ -125,16 +136,20 @@ Throw an error on failure."
 
     (match (connect! session)
       ('ok
-       ;; Authenticate against ~/.ssh/known_hosts.
-       (match (authenticate-server session)
-         ('ok #f)
-         (reason
-          (raise (condition
-                  (&message
-                   (message (format #f (G_ "failed to authenticate \
+       (if host-key
+           ;; Make sure the server's key is what we expect.
+           (authenticate-server* session host-key)
+
+           ;; Authenticate against ~/.ssh/known_hosts.
+           (match (authenticate-server session)
+             ('ok #f)
+             (reason
+              (raise (condition
+                      (&message
+                       (message (format #f (G_ "failed to authenticate \
 server at '~a': ~a")
-                                    (session-get session 'host)
-                                    reason)))))))
+                                        (session-get session 'host)
+                                        reason))))))))
 
        ;; Use public key authentication, via the SSH agent if it's available.
        (match (userauth-public-key/auto! session)
-- 
2.24.0

[bug#38478] [PATCH 4/4] machine: ssh: <machine-ssh-configuration> can include the host key.

[Ludovic Courtès]

* gnu/machine/ssh.scm (<machine-ssh-configuration>)[host-key]: New field.
(machine-ssh-session): Pass #:host-key to 'open-ssh-session'.
* doc/guix.texi (Invoking guix deploy): Document it.
---
 doc/guix.texi       | 12 ++++++++++++
 gnu/machine/ssh.scm |  9 +++++++--
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/doc/guix.texi b/doc/guix.texi
index 2da1ecd64c..e6e015ad3e 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -26412,6 +26412,18 @@ keyring.
 @item @code{identity} (default: @code{#f})
 If specified, the path to the SSH private key to use to authenticate with the
 remote host.
+
+@item @code{host-key} (default: @code{#f})
+This should be the SSH host key of the machine, which looks like this:
+
+@example
+ssh-ed25519 AAAAC3Nz@dots{} root@@example.org
+@end example
+
+When @code{host-key} is @code{#f}, the server is authenticated against
+the @file{~/.ssh/known_hosts} file, just like the OpenSSH @command{ssh}
+client does.
+
 @end table
 @end deftp
 
diff --git a/gnu/machine/ssh.scm b/gnu/machine/ssh.scm
index 6e3ed0e092..23ae917b79 100644
--- a/gnu/machine/ssh.scm
+++ b/gnu/machine/ssh.scm
@@ -54,6 +54,7 @@
             machine-ssh-configuration-authorize?
             machine-ssh-configuration-port
             machine-ssh-configuration-user
+            machine-ssh-configuration-host-key
             machine-ssh-configuration-session))
 
 ;;; Commentary:
@@ -87,6 +88,8 @@
   (identity       machine-ssh-configuration-identity       ; path to a private key
                   (default #f))
   (session        machine-ssh-configuration-session        ; session
+                  (default #f))
+  (host-key       machine-ssh-configuration-host-key       ; #f | string
                   (default #f)))
 
 (define (machine-ssh-session machine)
@@ -98,11 +101,13 @@ one from the configuration's parameters if one was not provided."
         (let ((host-name (machine-ssh-configuration-host-name config))
               (user (machine-ssh-configuration-user config))
               (port (machine-ssh-configuration-port config))
-              (identity (machine-ssh-configuration-identity config)))
+              (identity (machine-ssh-configuration-identity config))
+              (host-key (machine-ssh-configuration-host-key config)))
           (open-ssh-session host-name
                             #:user user
                             #:port port
-                            #:identity identity)))))
+                            #:identity identity
+                            #:host-key host-key)))))
 
 \f
 ;;;
-- 
2.24.0

[bug#38478] [PATCH 4/4] machine: ssh: <machine-ssh-configuration> can include the host key.

[Jakob L. Kreuze]

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

I've only been able to follow the updates to "guix deploy" somewhat
tangentially, but I was very excited to see this patch in my inbox.
Thumbs up from me, thanks Ludo!

Regards,
Jakob

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[bug#38478] [PATCH 4/4] machine: ssh: <machine-ssh-configuration> can include the host key.

[Ludovic Courtès]

Hi!

zerodaysfordays@sdf.lonestar.org (Jakob L. Kreuze) skribis:

> I've only been able to follow the updates to "guix deploy" somewhat
> tangentially, but I was very excited to see this patch in my inbox.
> Thumbs up from me, thanks Ludo!

Heheh, thank you!

I went ahead and pushed it as it seemed like a good idea to not wait.

BTW, I’m wondering if we should go further and deprecate missing/#f
‘host-key’ fields altogether.  WDYT?

To me it just seems wiser to have that info within the deploy config
rather than out-of-band in ~/.ssh/known_hosts.

Ludo’.

[bug#38478] [PATCH 4/4] machine: ssh: <machine-ssh-configuration> can include the host key.

[Jakob L. Kreuze]

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

Ludovic Courtès <ludo@gnu.org> writes:

> I went ahead and pushed it as it seemed like a good idea to not wait.

Agreed :)

> BTW, I’m wondering if we should go further and deprecate missing/#f
> ‘host-key’ fields altogether.  WDYT?
>
> To me it just seems wiser to have that info within the deploy config
> rather than out-of-band in ~/.ssh/known_hosts.

I feel that's more in-line with the goals of Guix -- implicitly reading
~/.ssh/known_hosts doesn't seem declarative to me. What's our means for
deprecating features like that? A warning message when omitted? If
that's the case, I'm definitely on board.

Regards,
Jakob

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[bug#38478] [PATCH 4/4] machine: ssh: <machine-ssh-configuration> can include the host key.

[Ludovic Courtès]

Hi!

zerodaysfordays@sdf.lonestar.org (Jakob L. Kreuze) skribis:

> Ludovic Courtès <ludo@gnu.org> writes:

[...]

>> BTW, I’m wondering if we should go further and deprecate missing/#f
>> ‘host-key’ fields altogether.  WDYT?
>>
>> To me it just seems wiser to have that info within the deploy config
>> rather than out-of-band in ~/.ssh/known_hosts.
>
> I feel that's more in-line with the goals of Guix -- implicitly reading
> ~/.ssh/known_hosts doesn't seem declarative to me. What's our means for
> deprecating features like that? A warning message when omitted? If
> that's the case, I'm definitely on board.

Yup, we can emit a deprecation warning when the key is #f.

So let’s take that route if nobody objects.  It’s easier to deprecate it
now that “guix deploy” is still very new.

Ludo’.

[bug#38478] [PATCH 4/4] machine: ssh: <machine-ssh-configuration> can include the host key.

[Ludovic Courtès]

Ludovic Courtès <ludo@gnu.org> skribis:

> zerodaysfordays@sdf.lonestar.org (Jakob L. Kreuze) skribis:
>
>> Ludovic Courtès <ludo@gnu.org> writes:
>
> [...]
>
>>> BTW, I’m wondering if we should go further and deprecate missing/#f
>>> ‘host-key’ fields altogether.  WDYT?
>>>
>>> To me it just seems wiser to have that info within the deploy config
>>> rather than out-of-band in ~/.ssh/known_hosts.
>>
>> I feel that's more in-line with the goals of Guix -- implicitly reading
>> ~/.ssh/known_hosts doesn't seem declarative to me. What's our means for
>> deprecating features like that? A warning message when omitted? If
>> that's the case, I'm definitely on board.
>
> Yup, we can emit a deprecation warning when the key is #f.
>
> So let’s take that route if nobody objects.  It’s easier to deprecate it
> now that “guix deploy” is still very new.

Done in commit 2617d956d8ae122128a1ba2cc74983cbd683b042!

Ludo’.