Subject: Re: bug#37851: Grub installation only checks for encrypted /boot folder

From: Miguel Ángel Arruga Vivas <[email-masked]>
Date: Mon, 26 Oct 2020 23:15:03 +0100

Hello!

Ludovic Courtès <[email-masked]> writes:
> Does that cause GRUB to mount all the LUKS partitions it was aware of at
> installation time, or does it cause it to scan all the partitions in
> search of a LUKS signature?
>
> In the latter case that wouldn’t be great, but in the former case it
> sounds like we could go ahead (well, with a comment above explaining
> what this does.  :-)).

Sorry for this huuuuuuuuuge delay, but I have this patch for this.  It
includes a test case, even though I have been suffering a lot until I
noticed that OCR was returning garbage and I was trying to be too
specific, so I've left it as basic as I could.

I add Mathieu to the loop to bring more eyes over it, I'm open to any
suggestion. :-)

Happy hacking!
Miguel

PS: It should apply on top of master too, but I tested it on top of some
other grub.cfg fixes, I'll send a new version if there is any problem
with this.

From d40f0a26afef194e7e68906ba793ca0ffac6da5f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Miguel=20=C3=81ngel=20Arruga=20Vivas?=
 <[email-masked]>
Date: Sun, 25 Oct 2020 16:31:17 +0100
Subject: [PATCH v3 5/5] system: Allow separated /boot and encrypted root.

* gnu/bootloader/grub.scm (grub-configuration-file): New parameter
store-crypto-devices.
[crypto-devices]: New helper function.
[builder]: Use crypto-devices.
* gnu/machine/ssh.scm (roll-back-managed-host): Use
boot-parameters-store-crypto-devices to provide its contents to the
bootloader configuration generation process.
* gnu/tests/install.scm (%encrypted-root-not-boot-os,
%encrypted-root-not-boot-os): New os declaration.
(%encrypted-root-not-boot-installation-script): New script, whose contents
were initially taken from %encrypted-root-installation-script.
(%test-encrypted-root-not-boot-os): New test.
* gnu/system.scm (define-module): Export
operating-system-bootoader-crypto-devices and
boot-parameters-store-crypto-devices.
(<boot-parameters>): Add field store-crypto-devices.
(read-boot-parameters): Parse store-crypto-devices field.
[uuid-sexp->uuid]: New helper function extracted from
device-sexp->device.
(operating-system-bootloader-crypto-devices): New function.
(operating-system-bootcfg): Use
operating-system-bootloader-crypto-devices to provide its contents to
the bootloader configuration generation process.
(operating-system-boot-parameters): Add store-crypto-devices to the
generated boot-parameters.
(operating-system-boot-parameters-file): Likewise to the file with
the serialized structure.
* guix/scripts/system.scm (reinstall-bootloader): Use
boot-parameters-store-crypto-devices to provide its contents to the
bootloader configuration generation process.
* tests/boot-parameters.scm (%default-store-crypto-devices): New
variable.
(%grub-boot-parameters, test-read-boot-parameters): Use
%default-store-crypto-devices.
(tests store-crypto-devices): New tests.
---
 gnu/bootloader/grub.scm   |  19 ++++++-
 gnu/machine/ssh.scm       |   3 ++
 gnu/system.scm            |  57 ++++++++++++++++++++-
 gnu/tests/install.scm     | 103 ++++++++++++++++++++++++++++++++++++++
 guix/scripts/system.scm   |   2 +
 tests/boot-parameters.scm |  29 ++++++++++-
 6 files changed, 208 insertions(+), 5 deletions(-)

diff --git a/gnu/bootloader/grub.scm b/gnu/bootloader/grub.scm
index 8636e9c690..c6e7d3fd6d 100644
--- a/gnu/bootloader/grub.scm
+++ b/gnu/bootloader/grub.scm
@@ -4,7 +4,7 @@
 ;;; Copyright © 2017 Leo Famulari <[email-masked]>
 ;;; Copyright © 2017, 2020 Mathieu Othacehe <[email-masked]>
 ;;; Copyright © 2019, 2020 Jan (janneke) Nieuwenhuizen <[email-masked]>
-;;; Copyright © 2019 Miguel Ángel Arruga Vivas <[email-masked]>
+;;; Copyright © 2019, 2020 Miguel Ángel Arruga Vivas <[email-masked]>
 ;;; Copyright © 2020 Maxim Cournoyer <[email-masked]>
 ;;; Copyright © 2020 Stefan <[email-masked]>
 ;;;
@@ -361,6 +361,7 @@ code."
                                   (locale #f)
                                   (system (%current-system))
                                   (old-entries '())
+                                  store-crypto-devices
                                   store-directory-prefix)
   "Return the GRUB configuration file corresponding to CONFIG, a
 <bootloader-configuration> object, and where the store is available at
@@ -413,6 +414,21 @@ menuentry ~s {
                   (string-join (map string-join '#$modules)
                                "\n  module " 'prefix))))))
 
+  (define (crypto-devices)
+    (define (crypto-device->cryptomount dev)
+      (if (uuid? dev)
+          #~(format port "cryptomount -u ~a~%"
+                    ;; cryptomount only accepts UUID without the hypen.
+                    #$(string-delete #\- (uuid->string dev)))
+          ;; Other type of devices aren't implemented.
+          #~()))
+    (let ((devices (map crypto-device->cryptomount store-crypto-devices))
+          ;; XXX: Add luks2 when grub 2.06 is packaged.
+          (modules #~(format port "insmod luks~%")))
+      (if (null? devices)
+          devices
+          (cons modules devices))))
+
   (define (sugar)
     (let* ((entry (first all-entries))
            (device (menu-entry-device entry))
@@ -469,6 +485,7 @@ keymap ~a~%" #$keymap))))
                   "# This file was generated from your Guix configuration.  Any changes
 # will be lost upon reconfiguration.
 ")
+          #$@(crypto-devices)
           #$(sugar)
           #$locale-config
           #$keyboard-layout-config
diff --git a/gnu/machine/ssh.scm b/gnu/machine/ssh.scm
index a3a12fb54b..822f401c1a 100644
--- a/gnu/machine/ssh.scm
+++ b/gnu/machine/ssh.scm
@@ -482,6 +482,8 @@ an environment type of 'managed-host."
                                         (list (second boot-parameters))))
                        (locale -> (boot-parameters-locale
                                    (second boot-parameters)))
+                       (crypto-dev -> (boot-parameters-store-crypto-devices
+                                      (second boot-parameters)))
                        (store-dir -> (boot-parameters-store-directory-prefix
                                       (second boot-parameters)))
                        (old-entries -> (map boot-parameters->menu-entry
@@ -494,6 +496,7 @@ an environment type of 'managed-host."
                                     bootloader))
                                   bootloader entries
                                   #:locale locale
+                                  #:store-crypto-devices crypto-dev
                                   #:store-directory-prefix store-dir
                                   #:old-entries old-entries)))
                        (remote-result (machine-remote-eval machine remote-exp)))
diff --git a/gnu/system.scm b/gnu/system.scm
index 30a5c418d0..3a718642cf 100644
--- a/gnu/system.scm
+++ b/gnu/system.scm
@@ -5,7 +5,7 @@
 ;;; Copyright © 2016 Chris Marusich <[email-masked]>
 ;;; Copyright © 2017 Mathieu Othacehe <[email-masked]>
 ;;; Copyright © 2019 Meiyo Peng <[email-masked]>
-;;; Copyright © 2019 Miguel Ángel Arruga Vivas <[email-masked]>
+;;; Copyright © 2019, 2020 Miguel Ángel Arruga Vivas <[email-masked]>
 ;;; Copyright © 2020 Danny Milosavljevic <[email-masked]>
 ;;; Copyright © 2020 Brice Waegeneire <[email-masked]>
 ;;; Copyright © 2020 Florian Pelz <[email-masked]>
@@ -112,6 +112,7 @@
             operating-system-store-file-system
             operating-system-user-mapped-devices
             operating-system-boot-mapped-devices
+            operating-system-bootloader-crypto-devices
             operating-system-activation-script
             operating-system-user-accounts
             operating-system-shepherd-service-names
@@ -147,6 +148,7 @@
             boot-parameters-root-device
             boot-parameters-bootloader-name
             boot-parameters-bootloader-menu-entries
+            boot-parameters-store-crypto-devices
             boot-parameters-store-device
             boot-parameters-store-directory-prefix
             boot-parameters-store-mount-point
@@ -301,6 +303,8 @@ directly by the user."
   (store-device     boot-parameters-store-device)
   (store-mount-point boot-parameters-store-mount-point)
   (store-directory-prefix boot-parameters-store-directory-prefix)
+  (store-crypto-devices boot-parameters-store-crypto-devices
+                        (default '()))
   (locale           boot-parameters-locale)
   (kernel           boot-parameters-kernel)
   (kernel-arguments boot-parameters-kernel-arguments)
@@ -334,6 +338,13 @@ file system labels."
            (if (string-prefix? "/" device)
                device
                (file-system-label device))))))
+  (define uuid-sexp->uuid
+    (match-lambda
+      (('uuid (? symbol? type) (? bytevector? bv))
+       (bytevector->uuid bv type))
+      (x
+       (warning (G_ "unrecognized uuid ~a at '~a'~%") x (port-filename port))
+       #f)))
 
   (match (read port)
     (('boot-parameters ('version 0)
@@ -407,6 +418,24 @@ file system labels."
           ;; No store found, old format.
           #f)))
 
+      (store-crypto-devices
+       (match (assq 'store rest)
+         (('store . store-data)
+          (match (assq 'crypto-devices store-data)
+            (('crypto-devices devices)
+             (if (list? devices)
+                 (map uuid-sexp->uuid devices)
+                 (begin
+                   (warning (G_ "unrecognized crypto-device ~S at '~a'~%")
+                            devices (port-filename port))
+                   '())))
+            (_
+             ;; No crypto-devices found
+             '())))
+         (_
+          ;; No store found, old format.
+          '())))
+
       (store-mount-point
        (match (assq 'store rest)
          (('store ('device _) ('mount-point mount-point) _ ...)
@@ -520,6 +549,23 @@ from the initrd."
                (any file-system-needed-for-boot? users)))
            devices)))
 
+(define (operating-system-bootloader-crypto-devices os)
+  "Return the subset of mapped devices that the bootloader must open.
+Only devices specified by uuid are supported."
+  (map mapped-device-source
+       (filter (match-lambda
+                 ((and (= mapped-device-type type)
+                       (= mapped-device-source source))
+                  (and (eq? luks-device-mapping type)
+                       (or (uuid? source)
+                           (begin
+                             (warning (G_ "\
+mapped-device '~a' won't be mounted by the bootloader.~%")
+                                      source)
+                             #f)))))
+               ;; XXX: Ordering is important, we trust the returned one.
+               (operating-system-boot-mapped-devices os))))
+
 (define (device-mapping-services os)
   "Return the list of device-mapping services for OS as a list."
   (map device-mapping-service
@@ -1256,6 +1302,7 @@ a list of <menu-entry>, to populate the \"old entries\" menu."
          (root-fs         (operating-system-root-file-system os))
          (root-device     (file-system-device root-fs))
          (locale          (operating-system-locale os))
+         (crypto-devices  (operating-system-bootloader-crypto-devices os))
          (params          (operating-system-boot-parameters
                            os root-device
                            #:system-kernel-arguments? #t))
@@ -1269,6 +1316,7 @@ a list of <menu-entry>, to populate the \"old entries\" menu."
     (generate-config-file bootloader-conf (list entry)
                           #:old-entries old-entries
                           #:locale locale
+                          #:store-crypto-devices crypto-devices
                           #:store-directory-prefix
 			  (btrfs-store-subvolume-file-name file-systems))))
 
@@ -1308,6 +1356,7 @@ such as '--root' and '--load' to <boot-parameters>."
                                (operating-system-initrd-file os)))
          (store           (operating-system-store-file-system os))
          (file-systems    (operating-system-file-systems os))
+         (crypto-devices  (operating-system-bootloader-crypto-devices os))
          (locale          (operating-system-locale os))
          (bootloader      (bootloader-configuration-bootloader
                            (operating-system-bootloader os)))
@@ -1330,6 +1379,7 @@ such as '--root' and '--load' to <boot-parameters>."
      (locale locale)
      (store-device (ensure-not-/dev (file-system-device store)))
      (store-directory-prefix (btrfs-store-subvolume-file-name file-systems))
+     (store-crypto-devices crypto-devices)
      (store-mount-point (file-system-mount-point store)))))
 
 (define (device->sexp device)
@@ -1388,7 +1438,10 @@ being stored into the \"parameters\" file)."
                       (mount-point #$(boot-parameters-store-mount-point
                                       params))
                       (directory-prefix
-                       #$(boot-parameters-store-directory-prefix params))))
+                       #$(boot-parameters-store-directory-prefix params))
+                      (crypto-devices
+                       #$(map device->sexp
+                              (boot-parameters-store-crypto-devices params)))))
                   #:set-load-path? #f)))
 
 (define-gexp-compiler (operating-system-compiler (os <operating-system>)
diff --git a/gnu/tests/install.scm b/gnu/tests/install.scm
index 86bd93966b..8f1668bab2 100644
--- a/gnu/tests/install.scm
+++ b/gnu/tests/install.scm
@@ -63,6 +63,8 @@
             %test-separate-home-os
             %test-raid-root-os
             %test-encrypted-root-os
+            %test-encrypted-root-not-boot-os
+            %test-encrypted-root-and-boot-os
             %test-btrfs-root-os
             %test-btrfs-root-on-subvolume-os
             %test-jfs-root-os
@@ -796,6 +798,107 @@ build (current-guix) and then store a couple of full system images.")
       (run-basic-test %encrypted-root-os command "encrypted-root-os"
                       #:initialization enter-luks-passphrase)))))
 
+
+;;;
+;;; LUKS-encrypted root file system and /boot in a non-encrypted partition.
+;;;
+
+(define-os-with-source (%encrypted-root-not-boot-os
+                        %encrypted-root-not-boot-os-source)
+  ;; The OS we want to install.
+  (use-modules (gnu) (gnu tests) (srfi srfi-1))
+
+  (operating-system
+    (host-name "bootroot")
+    (timezone "Europe/Madrid")
+    (locale "en_US.UTF-8")
+
+    (bootloader (bootloader-configuration
+                 (bootloader grub-bootloader)
+                 (target "/dev/vdb")))
+
+    (mapped-devices (list (mapped-device
+                           (source
+                            (uuid "12345678-1234-1234-1234-123456789abc"))
+                           (target "root")
+                           (type luks-device-mapping))))
+    (file-systems (cons* (file-system
+                           (device (file-system-label "my-boot"))
+                           (mount-point "/boot")
+                           (type "ext4"))
+                         (file-system
+                           (device "/dev/mapper/root")
+                           (mount-point "/")
+                           (type "ext4"))
+                         %base-file-systems))
+    (users (cons (user-account
+                  (name "alice")
+                  (group "users")
+                  (supplementary-groups '("wheel" "audio" "video")))
+                 %base-user-accounts))
+    (services (cons (service marionette-service-type
+                             (marionette-configuration
+                              (imported-modules '((gnu services herd)
+                                                  (guix combinators)))))
+                    %base-services))))
+
+(define %encrypted-root-not-boot-installation-script
+  ;; Shell script for an installation with boot not encrypted but root
+  ;; encrypted.
+  (format #f "\
+. /etc/profile
+set -e -x
+guix --version
+
+export GUIX_BUILD_OPTIONS=--no-grafts
+ls -l /run/current-system/gc-roots
+parted --script /dev/vdb mklabel gpt \\
+  mkpart primary ext2 1M 3M \\
+  mkpart primary ext2 3M 50M \\
+  mkpart primary ext2 50M 1.6G \\
+  set 1 boot on \\
+  set 1 bios_grub on
+echo -n \"~a\" | cryptsetup luksFormat --uuid=\"~a\" -q /dev/vdb3 -
+echo -n \"~a\" | cryptsetup open --type luks --key-file - /dev/vdb3 root
+mkfs.ext4 -L my-root /dev/mapper/root
+mkfs.ext4 -L my-boot /dev/vdb2
+mount LABEL=my-root /mnt
+mkdir /mnt/boot
+mount LABEL=my-boot /mnt/boot
+echo \"Checking mounts\"
+mount
+herd start cow-store /mnt
+mkdir /mnt/etc
+cp /etc/target-config.scm /mnt/etc/config.scm
+guix system build /mnt/etc/config.scm
+guix system init /mnt/etc/config.scm /mnt --no-substitutes
+sync
+echo \"Debugging info\"
+blkid
+cat /mnt/boot/grub/grub.cfg
+reboot\n"
+          %luks-passphrase "12345678-1234-1234-1234-123456789abc"
+          %luks-passphrase))
+
+(define %test-encrypted-root-not-boot-os
+  (system-test
+   (name "encrypted-root-not-boot-os")
+   (description
+    "Test the manual installation on an OS with / in an encrypted partition
+but /boot on a different, non-encrypted partition.  This test is expensive in
+terms of CPU and storage usage since we need to build (current-guix) and then
+store a couple of full system images.")
+   (value
+    (mlet* %store-monad
+        ((image (run-install %encrypted-root-not-boot-os
+                             %encrypted-root-not-boot-os-source
+                             #:script
+                             %encrypted-root-not-boot-installation-script))
+         (command (qemu-command/writable-image image)))
+      (run-basic-test %encrypted-root-not-boot-os command
+                      "encrypted-root-not-boot-os"
+                      #:initialization enter-luks-passphrase)))))
+
 
 ;;;
 ;;; Btrfs root file system.
diff --git a/guix/scripts/system.scm b/guix/scripts/system.scm
index ad998156c2..02cf2a12a2 100644
--- a/guix/scripts/system.scm
+++ b/guix/scripts/system.scm
@@ -385,6 +385,7 @@ STORE is an open connection to the store."
          (params (first (profile-boot-parameters %system-profile
                                                  (list number))))
          (locale (boot-parameters-locale params))
+         (store-crypto-devices (boot-parameters-store-crypto-devices params))
          (store-directory-prefix
           (boot-parameters-store-directory-prefix params))
          (old-generations
@@ -400,6 +401,7 @@ STORE is an open connection to the store."
                      ((bootloader-configuration-file-generator bootloader)
                       bootloader-config entries
                       #:locale locale
+                      #:store-crypto-devices store-crypto-devices
                       #:store-directory-prefix store-directory-prefix
                       #:old-entries old-entries)))
            (drvs -> (list bootcfg)))
diff --git a/tests/boot-parameters.scm b/tests/boot-parameters.scm
index a00b227551..c26ac83b7b 100644
--- a/tests/boot-parameters.scm
+++ b/tests/boot-parameters.scm
@@ -50,6 +50,8 @@
 (define %default-store-directory-prefix
   (string-append "/" %default-btrfs-subvolume))
 (define %default-store-mount-point (%store-prefix))
+(define %default-store-crypto-devices
+  (list (uuid "00000000-1111-2222-3333-444444444444")))
 (define %default-multiboot-modules '())
 (define %default-locale "es_ES.utf8")
 (define %root-path "/")
@@ -67,6 +69,7 @@
    (locale %default-locale)
    (store-device %default-store-device)
    (store-directory-prefix %default-store-directory-prefix)
+   (store-crypto-devices %default-store-crypto-devices)
    (store-mount-point %default-store-mount-point)))
 
 (define %default-operating-system
@@ -110,6 +113,8 @@
           (with-store #t)
           (store-device
            (quote-uuid %default-store-device))
+          (store-crypto-devices
+           (map quote-uuid %default-store-crypto-devices))
           (store-directory-prefix %default-store-directory-prefix)
           (store-mount-point %default-store-mount-point))
   (define (generate-boot-parameters)
@@ -125,12 +130,14 @@
             (sexp-or-nothing " (kernel-arguments ~S)" kernel-arguments)
             (sexp-or-nothing " (initrd ~S)" initrd)
             (if with-store
-                (format #false " (store~a~a~a)"
+                (format #false " (store~a~a~a~a)"
                         (sexp-or-nothing " (device ~S)" store-device)
                         (sexp-or-nothing " (mount-point ~S)"
                                          store-mount-point)
                         (sexp-or-nothing " (directory-prefix ~S)"
-                                         store-directory-prefix))
+                                         store-directory-prefix)
+                        (sexp-or-nothing " (crypto-devices ~S)"
+                                         store-crypto-devices))
                 "")
             (sexp-or-nothing " (locale ~S)" locale)
             (sexp-or-nothing " (bootloader-name ~a)" bootloader-name)
@@ -159,6 +166,7 @@
        (test-read-boot-parameters #:store-device #false)
        (test-read-boot-parameters #:store-device 'false)
        (test-read-boot-parameters #:store-mount-point #false)
+       (test-read-boot-parameters #:store-crypto-devices #false)
        (test-read-boot-parameters #:store-directory-prefix #false)
        (test-read-boot-parameters #:multiboot-modules #false)
        (test-read-boot-parameters #:locale #false)
@@ -254,6 +262,23 @@
   (boot-parameters-store-mount-point
    (test-read-boot-parameters #:with-store #false)))
 
+(test-equal "read, store-crypto-devices, default"
+  '()
+  (boot-parameters-store-crypto-devices
+   (test-read-boot-parameters #:store-crypto-devices #false)))
+
+;; XXX: <warning: unrecognized crypto-devices #f at '#f'>
+(test-equal "read, store-crypto-devices, false"
+  '()
+  (boot-parameters-store-crypto-devices
+   (test-read-boot-parameters #:store-crypto-devices 'false)))
+
+;; XXX: <warning: unrecognized crypto-device "bad" at '#f'>
+(test-equal "read, store-crypto-devices, string"
+  '()
+  (boot-parameters-store-crypto-devices
+   (test-read-boot-parameters #:store-crypto-devices "bad")))
+
 ;; For whitebox testing
 (define operating-system-boot-parameters
   (@@ (gnu system) operating-system-boot-parameters))
-- 
2.28.0