[bug#36956] machine: Automatically authorize the coordinator's signing key.
diff mbox series

Message ID 87ef1x4015.fsf@sdf.lonestar.org
State New
Headers show
Series
  • [bug#36956] machine: Automatically authorize the coordinator's signing key.
Related show

Commit Message

Jakob L. Kreuze Aug. 7, 2019, 12:45 p.m. UTC
* guix/ssh.scm (remote-authorize-signing-key): New variable.
* gnu/machine/ssh.scm (deploy-managed-host): Authorize coordinator's
signing key before any invocations of 'remote-eval'.
* guix/scripts/deploy.scm (guix-deploy): Display an error if a signing
key does not exist.
* doc/guix.texi (Invoking guix deploy): Remove section describing manual
signing key authorization.
---
 doc/guix.texi           | 16 ----------------
 gnu/machine/ssh.scm     |  7 +++++++
 guix/scripts/deploy.scm |  7 +++++++
 guix/ssh.scm            | 23 +++++++++++++++++++++++
 4 files changed, 37 insertions(+), 16 deletions(-)

Comments

Christopher Lemmer Webber Aug. 7, 2019, 7:18 p.m. UTC | #1
This seems like a good usability improvement.  For clarity, I assume
that it's still configurable, however?  Would be important if pushing
builds to a different machine.

Jakob L. Kreuze writes:

> * guix/ssh.scm (remote-authorize-signing-key): New variable.
> * gnu/machine/ssh.scm (deploy-managed-host): Authorize coordinator's
> signing key before any invocations of 'remote-eval'.
> * guix/scripts/deploy.scm (guix-deploy): Display an error if a signing
> key does not exist.
> * doc/guix.texi (Invoking guix deploy): Remove section describing manual
> signing key authorization.
> ---
>  doc/guix.texi           | 16 ----------------
>  gnu/machine/ssh.scm     |  7 +++++++
>  guix/scripts/deploy.scm |  7 +++++++
>  guix/ssh.scm            | 23 +++++++++++++++++++++++
>  4 files changed, 37 insertions(+), 16 deletions(-)
>
> diff --git a/doc/guix.texi b/doc/guix.texi
> index 734206a4b2..64ca44d494 100644
> --- a/doc/guix.texi
> +++ b/doc/guix.texi
> @@ -25530,22 +25530,6 @@ complex deployment may involve, for example, starting virtual machines through
>  a Virtual Private Server (VPS) provider.  In such a case, a different
>  @var{environment} type would be used.
>  
> -Do note that you first need to generate a key pair on the coordinator machine
> -to allow the daemon to export signed archives of files from the store
> -(@pxref{Invoking guix archive}).
> -
> -@example
> -# guix archive --generate-key
> -@end example
> -
> -@noindent
> -Each target machine must authorize the key of the master machine so that it
> -accepts store items it receives from the coordinator:
> -
> -@example
> -# guix archive --authorize < coordinator-public-key.txt
> -@end example
> -
>  @deftp {Data Type} machine
>  This is the data type representing a single machine in a heterogeneous Guix
>  deployment.
> diff --git a/gnu/machine/ssh.scm b/gnu/machine/ssh.scm
> index 1f16d9a5ea..90deff19a8 100644
> --- a/gnu/machine/ssh.scm
> +++ b/gnu/machine/ssh.scm
> @@ -28,13 +28,16 @@
>    #:use-module (guix i18n)
>    #:use-module (guix modules)
>    #:use-module (guix monads)
> +  #:use-module (guix pki)
>    #:use-module (guix records)
>    #:use-module (guix remote)
>    #:use-module (guix scripts system reconfigure)
>    #:use-module (guix ssh)
>    #:use-module (guix store)
>    #:use-module (guix utils)
> +  #:use-module (gcrypt pk-crypto)
>    #:use-module (ice-9 match)
> +  #:use-module (ice-9 textual-ports)
>    #:use-module (srfi srfi-1)
>    #:use-module (srfi srfi-19)
>    #:use-module (srfi srfi-26)
> @@ -329,6 +332,10 @@ the 'should-roll-back' field set to SHOULD-ROLL-BACK?"
>    "Internal implementation of 'deploy-machine' for MACHINE instances with an
>  environment type of 'managed-host."
>    (maybe-raise-unsupported-configuration-error machine)
> +  (remote-authorize-signing-key (call-with-input-file %public-key-file
> +                                  (lambda (port)
> +                                    (string->canonical-sexp (get-string-all port))))
> +                                (machine-ssh-session machine))
>    (mlet %store-monad ((_ (check-deployment-sanity machine))
>                        (boot-parameters (machine-boot-parameters machine)))
>      (let* ((os (machine-operating-system machine))
> diff --git a/guix/scripts/deploy.scm b/guix/scripts/deploy.scm
> index 6a67985c8b..075c74d395 100644
> --- a/guix/scripts/deploy.scm
> +++ b/guix/scripts/deploy.scm
> @@ -20,6 +20,7 @@
>  (define-module (guix scripts deploy)
>    #:use-module (gnu machine)
>    #:use-module (guix discovery)
> +  #:use-module (guix pki)
>    #:use-module (guix scripts)
>    #:use-module (guix scripts build)
>    #:use-module (guix store)
> @@ -83,6 +84,12 @@ Perform the deployment specified by FILE.\n"))
>  (define (guix-deploy . args)
>    (define (handle-argument arg result)
>      (alist-cons 'file arg result))
> +
> +  (unless (file-exists? %public-key-file)
> +    (leave (G_ "no signing key '~a'
> +have you run 'guix archive --generate-key?'~%")
> +           %public-key-file))
> +
>    (let* ((opts (parse-command-line args %options (list %default-options)
>                                     #:argument-handler handle-argument))
>           (file (assq-ref opts 'file))
> diff --git a/guix/ssh.scm b/guix/ssh.scm
> index 9b5ca68894..5186c646ca 100644
> --- a/guix/ssh.scm
> +++ b/guix/ssh.scm
> @@ -21,6 +21,7 @@
>    #:use-module (guix inferior)
>    #:use-module (guix i18n)
>    #:use-module ((guix utils) #:select (&fix-hint))
> +  #:use-module (gcrypt pk-crypto)
>    #:use-module (ssh session)
>    #:use-module (ssh auth)
>    #:use-module (ssh key)
> @@ -40,6 +41,7 @@
>              remote-daemon-channel
>              connect-to-remote-daemon
>              remote-system
> +            remote-authorize-signing-key
>              send-files
>              retrieve-files
>              retrieve-files*
> @@ -289,6 +291,27 @@ the machine on the other end of SESSION."
>    (inferior-remote-eval '(begin (use-modules (guix utils)) (%current-system))
>                          session))
>  
> +(define (remote-authorize-signing-key key session)
> +  "Send KEY, a canonical sexp containing a public key, over SESSION and add it
> +to the system ACL file if it has not yet been authorized."
> +  (inferior-remote-eval
> +   `(begin
> +      (use-modules (guix build utils)
> +                   (guix pki)
> +                   (guix utils)
> +                   (gcrypt pk-crypto)
> +                   (srfi srfi-26))
> +
> +      (define acl (current-acl))
> +      (define key (string->canonical-sexp ,(canonical-sexp->string key)))
> +
> +      (unless (authorized-key? key)
> +        (let ((acl (public-keys->acl (cons key (acl->public-keys acl)))))
> +          (mkdir-p (dirname %acl-file))
> +          (with-atomic-file-output %acl-file
> +            (cut write-acl acl <>)))))
> +   session))
> +
>  (define* (send-files local files remote
>                       #:key
>                       recursive?
Ricardo Wurmus Aug. 7, 2019, 7:39 p.m. UTC | #2
Jakob L. Kreuze <zerodaysfordays@sdf.lonestar.org> writes:

> +(define (remote-authorize-signing-key key session)
> +  "Send KEY, a canonical sexp containing a public key, over SESSION and add it
> +to the system ACL file if it has not yet been authorized."
> +  (inferior-remote-eval
> +   `(begin
> +      (use-modules (guix build utils)
> +                   (guix pki)
> +                   (guix utils)
> +                   (gcrypt pk-crypto)
> +                   (srfi srfi-26))
> +
> +      (define acl (current-acl))
> +      (define key (string->canonical-sexp ,(canonical-sexp->string key)))
> +
> +      (unless (authorized-key? key)
> +        (let ((acl (public-keys->acl (cons key (acl->public-keys acl)))))
> +          (mkdir-p (dirname %acl-file))
> +          (with-atomic-file-output %acl-file
> +            (cut write-acl acl <>)))))
> +   session))
> +

This will overwrite an existing acl file on the remote with a copy that
differs only in the newly added key.

Is there a chance for corruption, e.g. if acl->public-keys returns
something unexpected?

--
Ricardo
Jakob L. Kreuze Aug. 7, 2019, 8:52 p.m. UTC | #3
Hi Chris and Ricardo,

Christopher Lemmer Webber <cwebber@dustycloud.org> writes:

> This seems like a good usability improvement. For clarity, I assume
> that it's still configurable, however? Would be important if pushing
> builds to a different machine.

No, but you raise a good point :)  I'll update this patch to make it
configurable.

Ricardo Wurmus <rekado@elephly.net> writes:

> This will overwrite an existing acl file on the remote with a copy
> that differs only in the newly added key.
>
> Is there a chance for corruption, e.g. if acl->public-keys returns
> something unexpected?

I suppose it's possible. 'guix archive --authorize' doesn't seem to do
any specific handling for it, but it doesn't hurt to be paranoid -- we
"atomically" overwrite the GC root for the bootloader configuration, for
example, and we could do something similar here. I'll include it in the
updated patch.

Regards,
Jakob
Jakob L. Kreuze Aug. 9, 2019, 3:52 p.m. UTC | #4
zerodaysfordays@sdf.lonestar.org (Jakob L. Kreuze) writes:

> Hi Chris and Ricardo,
>
> Christopher Lemmer Webber <cwebber@dustycloud.org> writes:
>
>> This seems like a good usability improvement. For clarity, I assume
>> that it's still configurable, however? Would be important if pushing
>> builds to a different machine.
>
> No, but you raise a good point :)  I'll update this patch to make it
> configurable.
>
> Ricardo Wurmus <rekado@elephly.net> writes:
>
>> This will overwrite an existing acl file on the remote with a copy
>> that differs only in the newly added key.
>>
>> Is there a chance for corruption, e.g. if acl->public-keys returns
>> something unexpected?
>
> I suppose it's possible. 'guix archive --authorize' doesn't seem to do
> any specific handling for it, but it doesn't hurt to be paranoid -- we
> "atomically" overwrite the GC root for the bootloader configuration, for
> example, and we could do something similar here. I'll include it in the
> updated patch.
>
> Regards,
> Jakob
>

I didn't think this all the way through when I wrote this response.
We're already using 'with-atomic-file-output', so we're already
"atomically" overwriting the ACL. Also, it wouldn't solve the issue of
'acl->public-keys' returning something unexpected.

I'm not sure I have a good solution for this at the moment.

Regards,
Jakob
Christopher Lemmer Webber Aug. 14, 2019, 8:51 p.m. UTC | #5
Jakob L. Kreuze writes:

> zerodaysfordays@sdf.lonestar.org (Jakob L. Kreuze) writes:
>
>> Hi Chris and Ricardo,
>>
>> Christopher Lemmer Webber <cwebber@dustycloud.org> writes:
>>
>>> This seems like a good usability improvement. For clarity, I assume
>>> that it's still configurable, however? Would be important if pushing
>>> builds to a different machine.
>>
>> No, but you raise a good point :)  I'll update this patch to make it
>> configurable.
>>
>> Ricardo Wurmus <rekado@elephly.net> writes:
>>
>>> This will overwrite an existing acl file on the remote with a copy
>>> that differs only in the newly added key.
>>>
>>> Is there a chance for corruption, e.g. if acl->public-keys returns
>>> something unexpected?
>>
>> I suppose it's possible. 'guix archive --authorize' doesn't seem to do
>> any specific handling for it, but it doesn't hurt to be paranoid -- we
>> "atomically" overwrite the GC root for the bootloader configuration, for
>> example, and we could do something similar here. I'll include it in the
>> updated patch.
>>
>> Regards,
>> Jakob
>>
>
> I didn't think this all the way through when I wrote this response.
> We're already using 'with-atomic-file-output', so we're already
> "atomically" overwriting the ACL. Also, it wouldn't solve the issue of
> 'acl->public-keys' returning something unexpected.
>
> I'm not sure I have a good solution for this at the moment.

But it's only a problem for guix deploy so far, right?  So it shouldn't
break existing, hopefully-stable guix systems and rather only
bleeding-edge guix deploy systems, right? :)

If that's true then let's file a bug about this issue and get this code
merged after you get this in patch series form.
Ricardo Wurmus Aug. 15, 2019, 5:52 a.m. UTC | #6
Christopher Lemmer Webber <cwebber@dustycloud.org> writes:

> Jakob L. Kreuze writes:
>
>> zerodaysfordays@sdf.lonestar.org (Jakob L. Kreuze) writes:
>>
>>> Hi Chris and Ricardo,
>>>
>>> Christopher Lemmer Webber <cwebber@dustycloud.org> writes:
>>>
>>>> This seems like a good usability improvement. For clarity, I assume
>>>> that it's still configurable, however? Would be important if pushing
>>>> builds to a different machine.
>>>
>>> No, but you raise a good point :)  I'll update this patch to make it
>>> configurable.
>>>
>>> Ricardo Wurmus <rekado@elephly.net> writes:
>>>
>>>> This will overwrite an existing acl file on the remote with a copy
>>>> that differs only in the newly added key.
>>>>
>>>> Is there a chance for corruption, e.g. if acl->public-keys returns
>>>> something unexpected?
>>>
>>> I suppose it's possible. 'guix archive --authorize' doesn't seem to do
>>> any specific handling for it, but it doesn't hurt to be paranoid -- we
>>> "atomically" overwrite the GC root for the bootloader configuration, for
>>> example, and we could do something similar here. I'll include it in the
>>> updated patch.
>>>
>>> Regards,
>>> Jakob
>>>
>>
>> I didn't think this all the way through when I wrote this response.
>> We're already using 'with-atomic-file-output', so we're already
>> "atomically" overwriting the ACL. Also, it wouldn't solve the issue of
>> 'acl->public-keys' returning something unexpected.
>>
>> I'm not sure I have a good solution for this at the moment.
>
> But it's only a problem for guix deploy so far, right?  So it shouldn't
> break existing, hopefully-stable guix systems and rather only
> bleeding-edge guix deploy systems, right? :)

It has the potential to break systems that are the target of “guix
deploy”.  The expected breakage would be minor as the acl can be
regenerated.

> If that's true then let's file a bug about this issue and get this code
> merged after you get this in patch series form.

I agree.

--
Ricardo
Christopher Lemmer Webber Aug. 15, 2019, 11:45 a.m. UTC | #7
Christopher Lemmer Webber writes:

> If that's true then let's file a bug about this issue and get this code
> merged after you get this in patch series form.

Merged and pushed!

Patch
diff mbox series

diff --git a/doc/guix.texi b/doc/guix.texi
index 734206a4b2..64ca44d494 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -25530,22 +25530,6 @@  complex deployment may involve, for example, starting virtual machines through
 a Virtual Private Server (VPS) provider.  In such a case, a different
 @var{environment} type would be used.
 
-Do note that you first need to generate a key pair on the coordinator machine
-to allow the daemon to export signed archives of files from the store
-(@pxref{Invoking guix archive}).
-
-@example
-# guix archive --generate-key
-@end example
-
-@noindent
-Each target machine must authorize the key of the master machine so that it
-accepts store items it receives from the coordinator:
-
-@example
-# guix archive --authorize < coordinator-public-key.txt
-@end example
-
 @deftp {Data Type} machine
 This is the data type representing a single machine in a heterogeneous Guix
 deployment.
diff --git a/gnu/machine/ssh.scm b/gnu/machine/ssh.scm
index 1f16d9a5ea..90deff19a8 100644
--- a/gnu/machine/ssh.scm
+++ b/gnu/machine/ssh.scm
@@ -28,13 +28,16 @@ 
   #:use-module (guix i18n)
   #:use-module (guix modules)
   #:use-module (guix monads)
+  #:use-module (guix pki)
   #:use-module (guix records)
   #:use-module (guix remote)
   #:use-module (guix scripts system reconfigure)
   #:use-module (guix ssh)
   #:use-module (guix store)
   #:use-module (guix utils)
+  #:use-module (gcrypt pk-crypto)
   #:use-module (ice-9 match)
+  #:use-module (ice-9 textual-ports)
   #:use-module (srfi srfi-1)
   #:use-module (srfi srfi-19)
   #:use-module (srfi srfi-26)
@@ -329,6 +332,10 @@  the 'should-roll-back' field set to SHOULD-ROLL-BACK?"
   "Internal implementation of 'deploy-machine' for MACHINE instances with an
 environment type of 'managed-host."
   (maybe-raise-unsupported-configuration-error machine)
+  (remote-authorize-signing-key (call-with-input-file %public-key-file
+                                  (lambda (port)
+                                    (string->canonical-sexp (get-string-all port))))
+                                (machine-ssh-session machine))
   (mlet %store-monad ((_ (check-deployment-sanity machine))
                       (boot-parameters (machine-boot-parameters machine)))
     (let* ((os (machine-operating-system machine))
diff --git a/guix/scripts/deploy.scm b/guix/scripts/deploy.scm
index 6a67985c8b..075c74d395 100644
--- a/guix/scripts/deploy.scm
+++ b/guix/scripts/deploy.scm
@@ -20,6 +20,7 @@ 
 (define-module (guix scripts deploy)
   #:use-module (gnu machine)
   #:use-module (guix discovery)
+  #:use-module (guix pki)
   #:use-module (guix scripts)
   #:use-module (guix scripts build)
   #:use-module (guix store)
@@ -83,6 +84,12 @@  Perform the deployment specified by FILE.\n"))
 (define (guix-deploy . args)
   (define (handle-argument arg result)
     (alist-cons 'file arg result))
+
+  (unless (file-exists? %public-key-file)
+    (leave (G_ "no signing key '~a'
+have you run 'guix archive --generate-key?'~%")
+           %public-key-file))
+
   (let* ((opts (parse-command-line args %options (list %default-options)
                                    #:argument-handler handle-argument))
          (file (assq-ref opts 'file))
diff --git a/guix/ssh.scm b/guix/ssh.scm
index 9b5ca68894..5186c646ca 100644
--- a/guix/ssh.scm
+++ b/guix/ssh.scm
@@ -21,6 +21,7 @@ 
   #:use-module (guix inferior)
   #:use-module (guix i18n)
   #:use-module ((guix utils) #:select (&fix-hint))
+  #:use-module (gcrypt pk-crypto)
   #:use-module (ssh session)
   #:use-module (ssh auth)
   #:use-module (ssh key)
@@ -40,6 +41,7 @@ 
             remote-daemon-channel
             connect-to-remote-daemon
             remote-system
+            remote-authorize-signing-key
             send-files
             retrieve-files
             retrieve-files*
@@ -289,6 +291,27 @@  the machine on the other end of SESSION."
   (inferior-remote-eval '(begin (use-modules (guix utils)) (%current-system))
                         session))
 
+(define (remote-authorize-signing-key key session)
+  "Send KEY, a canonical sexp containing a public key, over SESSION and add it
+to the system ACL file if it has not yet been authorized."
+  (inferior-remote-eval
+   `(begin
+      (use-modules (guix build utils)
+                   (guix pki)
+                   (guix utils)
+                   (gcrypt pk-crypto)
+                   (srfi srfi-26))
+
+      (define acl (current-acl))
+      (define key (string->canonical-sexp ,(canonical-sexp->string key)))
+
+      (unless (authorized-key? key)
+        (let ((acl (public-keys->acl (cons key (acl->public-keys acl)))))
+          (mkdir-p (dirname %acl-file))
+          (with-atomic-file-output %acl-file
+            (cut write-acl acl <>)))))
+   session))
+
 (define* (send-files local files remote
                      #:key
                      recursive?