[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

Debbugs page

L
L
Ludovic Courtès wrote on 3 Dec 2019 13: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 13: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 13: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 13: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 13: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 05: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 09: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 09: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 09: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 5 Dec 2019 16: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 04: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 6 Dec 2019 16: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’.
?
Your comment

This issue is archived.

To comment on this conversation send an email to 38478@patchwise.org

To respond to this issue using the mumi CLI, first switch to it
mumi current 38478
Then, you may apply the latest patchset in this issue (with sign off)
mumi am -- -s
Or, compose a reply to this issue
mumi compose
Or, send patches to this issue
mumi send-email *.patch