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

  • Done
  • quality assurance status badge
Details
2 participants
  • Ludovic Courtès
  • Jakob L. Kreuze
Owner
unassigned
Submitted by
Ludovic Courtès
Severity
normal
L
L
Ludovic Courtès wrote on 3 Dec 2019 22:09
(address . guix-patches@gnu.org)(name . Ludovic Courtès)(address . ludo@gnu.org)
20191203210958.20936-1-ludo@gnu.org
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
L
L
Ludovic Courtès wrote on 3 Dec 2019 22:15
[PATCH 1/4] ssh: Add 'authenticate-server*' and use it for offloading.
(address . 38478@debbugs.gnu.org)(name . Ludovic Courtès)(address . ludo@gnu.org)
20191203211557.21145-1-ludo@gnu.org
* 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(-)

Toggle diff (105 lines)
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
L
L
Ludovic Courtès wrote on 3 Dec 2019 22:15
[PATCH 3/4] ssh: 'open-ssh-session' can be passed the expected host key.
(address . 38478@debbugs.gnu.org)(name . Ludovic Courtès)(address . ludo@gnu.org)
20191203211557.21145-3-ludo@gnu.org
* 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(-)

Toggle diff (72 lines)
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
L
L
Ludovic Courtès wrote on 3 Dec 2019 22:15
[PATCH 2/4] ssh: Always authenticate the server [security fix].
(address . 38478@debbugs.gnu.org)(name . Ludovic Courtès)(address . ludo@gnu.org)
20191203211557.21145-2-ludo@gnu.org
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(+)

Toggle diff (24 lines)
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
L
L
Ludovic Courtès wrote on 3 Dec 2019 22:15
[PATCH 4/4] machine: ssh: <machine-ssh-configuration> can include the host key.
(address . 38478@debbugs.gnu.org)(name . Ludovic Courtès)(address . ludo@gnu.org)
20191203211557.21145-4-ludo@gnu.org
* 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(-)

Toggle diff (62 lines)
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)))))
;;;
--
2.24.0
J
J
Jakob L. Kreuze wrote on 4 Dec 2019 14:19
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 38478@debbugs.gnu.org)
87d0d4qlc0.fsf@sdf.lonestar.org
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
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEEa1VJLOiXAjQ2BGSm9Qb9Fp2P2VoFAl3nsoEACgkQ9Qb9Fp2P
2VrCyg/+PiUW7T+Ag6SUAXVjv7cxgSAgBhyeJ27Qx0Bk3hZN1OMGu8817i+c/m8a
1xeLgziJ5IQE2t3s+uDhFxW1WNq3ve6EwF0yxTZYKP9+V651ng0p0U868VtwDopr
z/Y0HnvCw+dGuK71J30BjtCz/vyi/1GaDnIityfN617IlbX9hRG3Ug6UGAuq7x/s
S4ZPcwmLjxoj8yuB5PvczLtQJwc9jQIfu6s6fyNA1lta6rs78tOdKI7UzvO7VCqh
UvT2QlzC7VeA1VeMc41nebLTFvmiIG0i4oPMzHagbfXE+g0DXcGdTz1CgNi2fkT7
/wzFdeN2707d8ZH2MYjMbEsoBJU4B3rt+R1wplG5QT8eU0DVm3TpHm66ry6pXkFR
3I/p8LOpm+kM0TeW1aYI1ZxfKT+5fvuGvRA7iGhdIQAGKPw/Rj4XmFpIKnFE0ai2
wtZzoJVrYb2lrl2jBsA+T2FI7MPDxSOFlHbKM2WYb4CA//1wJsyRqnFqRlDWS2AQ
QoPrMOtIeRgEJUf40jzXF4FF9EGMVg1PuDdpc6Fmbkc5b5CJuMHckFtKihlGQsQb
zK/bQI67WDHjU+IM/RD0NrDOxy/DcGRIF0Php4A2ZBOteE0lk3YQWPjhEREU6Tt6
KEmZicYk41Oj6rz1HKa1m2mDj8uAmBTBxlG2L0ial7fRqnzQzfY=
=jTji
-----END PGP SIGNATURE-----

L
L
Ludovic Courtès wrote on 4 Dec 2019 18:16
control message for bug #38478
(address . control@debbugs.gnu.org)
875ziwc8or.fsf@gnu.org
tags 38478 + security
quit
L
L
Ludovic Courtès wrote on 4 Dec 2019 18:33
Re: [bug#38478] [PATCH 4/4] machine: ssh: <machine-ssh-configuration> can include the host key.
(name . Jakob L. Kreuze)(address . zerodaysfordays@sdf.lonestar.org)(address . 38478@debbugs.gnu.org)
87tv6gatc9.fsf@gnu.org
Hi!

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

Toggle quote (4 lines)
> 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’.
L
L
Ludovic Courtès wrote on 4 Dec 2019 18:33
control message for bug #38478
(address . control@debbugs.gnu.org)
87sgm0atbv.fsf@gnu.org
tags 38478 fixed
close 38478
quit
J
J
Jakob L. Kreuze wrote on 6 Dec 2019 01:50
Re: [bug#38478] [PATCH 4/4] machine: ssh: <machine-ssh-configuration> can include the host key.
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 38478@debbugs.gnu.org)
87eexil1kq.fsf@sdf.lonestar.org
Ludovic Courtès <ludo@gnu.org> writes:

Toggle quote (2 lines)
> I went ahead and pushed it as it seemed like a good idea to not wait.

Agreed :)

Toggle quote (6 lines)
> 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
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEEa1VJLOiXAjQ2BGSm9Qb9Fp2P2VoFAl3ppccACgkQ9Qb9Fp2P
2VoOtQ//bsGKOOOJbZJ9xfSHTiuz3BaBO4kk1VM9sMqbIJE1IfdvavM4fGAem82g
lVJGsIdPLFtGcDnMETuobRUpP7u4qhrn1sBAhvUqmEO5iLCBXOXUI5W4fSkIYUnf
D98H9Pg0qE5yfru598ldCwhn3vJ3WAncwebmLbOrgSyNVKBlboLXt7JUG6xTgv5d
zsMVog47uIK5RfWDhw5T3GblfKijmIapqg32/W7GoHDRJ94/+Z/KBRd9iqeJSydl
9QSuntdp+5m5O7bjCzrNJBCtuMpJ6VLmG1sNLjdwDAbDEzvY5T7OJMvZdtrnbbPd
7GlUhz7Wsc1d7LqQ6JomqGLmfQQ3JiU0As5k4XFNbN+ZkOo2xaF3N6wutWP6DgJB
kt3Mupo8erdQbmgjeSGkVRff+7naIOIv+U5DJ6BsHdHe7F0ljzHKCjOsBvpyFBID
byCijr/szfXujiAME5xZv9SK6iOJNc5fri97tz5NhlBx+jXd0h9uhb3kkZXk432I
XsDWTHjNzq5hvK2TdXbibHJfJOICHgZrMUv1kA0X573WO4rWUfUFnI+jpkCd9ryj
5b4+3gcbZAn0H6D6H2zS9ngW+Gv8v2AKCFySBI3XxQrm0DaIg7kjkYFMnKjVfcwW
HhUV5wLYr8O6kB176dQrVQHAx28ST3e8/6hTHmi+8nWWz9qe4HE=
=2xax
-----END PGP SIGNATURE-----

L
L
Ludovic Courtès wrote on 6 Dec 2019 13:16
(name . Jakob L. Kreuze)(address . zerodaysfordays@sdf.lonestar.org)(address . 38478@debbugs.gnu.org)
87a785abti.fsf@gnu.org
Hi!

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

Toggle quote (2 lines)
> Ludovic Courtès <ludo@gnu.org> writes:

[...]

Toggle quote (11 lines)
>> 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’.
L
L
Ludovic Courtès wrote on 7 Dec 2019 01:04
(name . Jakob L. Kreuze)(address . zerodaysfordays@sdf.lonestar.org)(address . 38478@debbugs.gnu.org)
8736dx80h1.fsf@gnu.org
Ludovic Courtès <ludo@gnu.org> skribis:

Toggle quote (22 lines)
> 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’.
?