Grub installation only checks for encrypted /boot folder

  • Done
  • quality assurance status badge
Details
2 participants
  • Ludovic Courtès
  • Miguel Arruga Vivas
Owner
unassigned
Submitted by
Miguel Arruga Vivas
Severity
normal
Merged with
M
M
Miguel Arruga Vivas wrote on 21 Oct 2019 13:07
(address . bug-guix@gnu.org)
20191021130709.21d6ac20@gmail.com
Hi,

The following configuration results in an unbootable system. The
root partition must be manually mounted with cryptomount in order to
boot the system.

The core issue is that grub unencrypts automatically, as
GRUB_ENABLE_CRYPTODISK=y was provided during installation, the /boot
partition, but not the partition which contains /gnu/store.

Happy hacking!
Miguel

==================== config.scm ====================
;; ....
(operating-system
;; ...
(bootloader
(bootloader-configuration
(bootloader grub-bootloader)
(target "/dev/sda")))
(mapped-devices
(list (mapped-device
(source (uuid "uuid root device"))
(target "root")
(type luks-device-mapping))
(mapped-device
(source (uuid "uuid boot device"))
(target "boot")
(type luks-device-mapping))))
(file-systems
(cons* (file-system
(mount-point "/")
(device "/dev/mapper/root")
(type "btrfs")
(dependencies mapped-devices))
(file-system
(mount-point "/boot")
(device "/dev/mapper/boot")
(type "ext4")
(dependencies mapped-devices))
%base-file-systems)))
==================== config.scm ====================
M
M
Miguel Arruga Vivas wrote on 21 Oct 2019 14:47
(address . 37851@debbugs.gnu.org)
20191021144758.3d8cfe95@gmail.com
Hi again,

Attached can be found a workaround to mount all encrypted partitions.
There is no way to tell the devices to mount without changing
boot-parameters, where I'd add another field with the needed mapped
devices (a traversal onto the mapped-device dependency tree
of /gnu/store). Do you think this is a good idea? At least I think
it's the best way to encode the dependencies into the grub.cfg file,
even though the typical graph will contain 0 or 1 nodes.

Ideas?

Best regards,
Miguel
From 9b50e2d8eb8b744595a54a9543993eb4e3813742 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Miguel=20=C3=81ngel=20Arruga=20Vivas?=
<rosen644835@gmail.com>
Date: Mon, 21 Oct 2019 14:35:02 +0200
Subject: [PATCH] system: Mount luks devices on boot.

* gnu/bootloader/grub.scm (grub-configuration-file)[builder]: Mount all
encrypted partitions.
---
gnu/bootloader/grub.scm | 1 +
1 file changed, 1 insertion(+)

Toggle diff (14 lines)
diff --git a/gnu/bootloader/grub.scm b/gnu/bootloader/grub.scm
index d984d5f5e3..b29477ec71 100644
--- a/gnu/bootloader/grub.scm
+++ b/gnu/bootloader/grub.scm
@@ -369,6 +369,7 @@ keymap ~a~%" keymap)))))
(format port
"# This file was generated from your Guix configuration. Any changes
# will be lost upon reconfiguration.
+cryptomount -a
")
#$sugar
#$keyboard-layout-config
--
2.23.0
M
M
Miguel Arruga Vivas wrote on 21 Oct 2019 16:55
bug#37851
(address . 37851@debbugs.gnu.org)
20191021165536.2bf8cde9@gmail.com
merge 25305 37851
quit
M
M
Miguel wrote on 21 Oct 2019 17:07
control message for bug #37851
(address . control@debbugs.gnu.org)
8736fmjfwg.fsf@unfall.i-did-not-set--mail-host-address--so-tickle-me
merge 37851 25305
quit
L
L
Ludovic Courtès wrote on 22 Oct 2019 16:12
Re: bug#37851: Grub installation only checks for encrypted /boot folder
(name . Miguel Arruga Vivas)(address . rosen644835@gmail.com)(address . 37851@debbugs.gnu.org)
87lftc27j2.fsf@gnu.org
Hola Miguel,

Miguel Arruga Vivas <rosen644835@gmail.com> skribis:

Toggle quote (30 lines)
> Attached can be found a workaround to mount all encrypted partitions.
> There is no way to tell the devices to mount without changing
> boot-parameters, where I'd add another field with the needed mapped
> devices (a traversal onto the mapped-device dependency tree
> of /gnu/store). Do you think this is a good idea? At least I think
> it's the best way to encode the dependencies into the grub.cfg file,
> even though the typical graph will contain 0 or 1 nodes.

> From 9b50e2d8eb8b744595a54a9543993eb4e3813742 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Miguel=20=C3=81ngel=20Arruga=20Vivas?=
> <rosen644835@gmail.com>
> Date: Mon, 21 Oct 2019 14:35:02 +0200
> Subject: [PATCH] system: Mount luks devices on boot.
>
> * gnu/bootloader/grub.scm (grub-configuration-file)[builder]: Mount all
> encrypted partitions.
> ---
> gnu/bootloader/grub.scm | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/gnu/bootloader/grub.scm b/gnu/bootloader/grub.scm
> index d984d5f5e3..b29477ec71 100644
> --- a/gnu/bootloader/grub.scm
> +++ b/gnu/bootloader/grub.scm
> @@ -369,6 +369,7 @@ keymap ~a~%" keymap)))))
> (format port
> "# This file was generated from your Guix configuration. Any changes
> # will be lost upon reconfiguration.
> +cryptomount -a

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. :-)).

Thanks for working on it!

Ludo’.
M
M
Miguel Arruga Vivas wrote on 27 Oct 2019 02:00
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 37851@debbugs.gnu.org)
20191027020031.18666b75@gmail.com
Hi Ludo’,

El Tue, 22 Oct 2019 16:12:49 +0200
Ludovic Courtès <ludo@gnu.org> escribió:
Toggle quote (10 lines)
> Hola Miguel,
>
> Miguel Arruga Vivas <rosen644835@gmail.com> skribis:
> > (...)
> > +cryptomount -a
>
> 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?

That patch is the first one, it mounts everything it can find, unlike
this one.

The only option I've seen was to modify boot-parameters (as in #35394,
wink wink nudge nudge) in order to store the needed partitions. I've
reduced it this time to one patch, is it somehow easier to read this
way? I could split it in two stages (one add the boot-parameters
field, the other one to make use of it) or squash the three for the
other feature into one if that easier for the review. The main issues
I've found is that the source of the device-mappings needed for boot up
has to be declared by the UUID to ensure they are not system
dependent. Also, the warning is shown several times and the message
isn't quite good, any idea how to fix/improve this?

Happy hacking!
Miguel
M
M
Miguel wrote on 1 Nov 2019 13:11
control message for bug #37851
(address . control@debbugs.gnu.org)
877e4jiynr.fsf@unfall.i-did-not-set--mail-host-address--so-tickle-me
tags 37851 + patch
quit
M
M
Miguel Ángel Arruga Vivas wrote on 26 Oct 2020 23:15
Re: bug#37851: Grub installation only checks for encrypted /boot folder
87r1pkocrc.fsf@gmail.com
Hello!

Ludovic Courtès <ludo@gnu.org> writes:
Toggle quote (8 lines)
> 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.
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEEiIeExBRZrMuD5+hMY0xuiXn6vsIFAl+XSmgACgkQY0xuiXn6
vsIcgwwAp0YDr07LjQ18+N7PGH2bgqNRSIDXPeEGknfjrPu2naRdXhGeB97JNRkD
JGO9jp50e4aiRbxjL+Zjw2VDIsKoSTH73rNwSgPTDKHbaadDOhF2LypR8NpnRdGP
HB4o0uIeb09eXpqYxuFA4586nO4q151DxA528G9v+3AePDGUhuc2EgOhp8Rl8Bec
T8twYFomXrIF8uBguycXsyTvFEVSBdZFIaLds7wK8N64Cm29Erl8MIc3seL7KS3Y
fdLxTgCUF4FRGN+EHNFYzfa/nm86RGLin1AS+ZLmwZL20mV4KJmEfP+mOwSWuKHy
x2eHczFDos+N7Po/2Ei6xx/RYEuE/QaTYqbOGtoKKWxnO0d+9qeePOqnf0n9u1C7
rn6iEPSwvtlPmo0NLEaDcuDd3/3c5EFjLEhWg7YgEq0Ea3XsiGpo3qvNLeRM3J0Y
VnOrSh0UkHdi60dFXKKvMyYlVeyR5qZrH7Oryy7Cx44auGiibhucxwA3SZRNaWUm
rvVOLi57
=/zYW
-----END PGP SIGNATURE-----

M
M
Miguel Ángel Arruga Vivas wrote on 28 Oct 2020 22:42
Re: bug#25305: bug#37851: Grub installation only checks for encrypted /boot folder
(name . Ludovic Courtès)(address . ludo@gnu.org)
87ft5ym3ic.fsf@gmail.com
In this v2 I've fixed two flaws I saw in the previous patch: the
parameter store-crypto-devices now has the empty list as default value,
and now the function documents for the parameter too.

Happy hacking!
Miguel
L
L
Ludovic Courtès wrote on 14 Dec 2020 14:11
Re: bug#37851: bug#25305: bug#37851: Grub installation only checks for encrypted /boot folder
(name . Miguel Ángel Arruga Vivas)(address . rosen644835@gmail.com)
87k0tksfau.fsf@gnu.org
Hi Miguel,

Miguel Ángel Arruga Vivas <rosen644835@gmail.com> skribis:

Toggle quote (50 lines)
>>From 52993db19da43699ea96ea16ebb051b9652934f9 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Miguel=20=C3=81ngel=20Arruga=20Vivas?=
> <rosen644835@gmail.com>
> Date: Sun, 25 Oct 2020 16:31:17 +0100
> Subject: [PATCH v4 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 | 21 +++++++-
> 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, 210 insertions(+), 5 deletions(-)

Woohoo!

Toggle quote (45 lines)
> --- a/gnu/bootloader/grub.scm
> +++ b/gnu/bootloader/grub.scm
> @@ -4,7 +4,7 @@
> ;;; Copyright © 2017 Leo Famulari <leo@famulari.name>
> ;;; Copyright © 2017, 2020 Mathieu Othacehe <m.othacehe@gmail.com>
> ;;; Copyright © 2019, 2020 Jan (janneke) Nieuwenhuizen <janneke@gnu.org>
> -;;; Copyright © 2019 Miguel Ángel Arruga Vivas <rosen644835@gmail.com>
> +;;; Copyright © 2019, 2020 Miguel Ángel Arruga Vivas <rosen644835@gmail.com>
> ;;; Copyright © 2020 Maxim Cournoyer <maxim.cournoyer@gmail.com>
> ;;; Copyright © 2020 Stefan <stefan-guix@vodafonemail.de>
> ;;;
> @@ -360,11 +360,14 @@ 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
> STORE-FS, a <file-system> object. OLD-ENTRIES is taken to be a list of menu
> entries corresponding to old generations of the system.
> +STORE-CRYPTO-DEVICES contain the UUIDs of the encrypted units that must
> +be unlocked to access the store contents.
> STORE-DIRECTORY-PREFIX may be used to specify a store prefix, as is required
> when booting a root file system on a Btrfs subvolume."
> (define all-entries
> @@ -412,6 +415,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))))

What I don’t get is why we’re able to use an encrypted root right now
without emitting “cryptomount” GRUB commands?

Toggle quote (12 lines)
> + (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))
> + '())))

You could avoid ‘if’ by having clauses like:

(('crypto-devices (devices ...))
;; …
)
(('crypto-devices _)
(warning …)
'())
(_
'())

Toggle quote (7 lines)
> + (_
> + ;; No crypto-devices found
> + '())))
> + (_
> + ;; No store found, old format.
> + '())))

s/No store found/No crypto devices found/ ?

Toggle quote (17 lines)
> +(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))))

You can use ‘filter-map’ here.

The rest LGTM! Make sure the “installed-os” and “encrypted-root-os”
system tests are still fine, and if they are, I guess you can go ahead.

Thanks!

Ludo’.
M
M
Miguel Ángel Arruga Vivas wrote on 21 Dec 2020 21:23
(name . Ludovic Courtès)(address . ludo@gnu.org)
87k0tazz5j.fsf@gmail.com
Hi Ludo,

First of all, thanks for your review. :-)

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

Toggle quote (21 lines)
> Hi Miguel,
>
> Miguel Ángel Arruga Vivas <rosen644835@gmail.com> skribis:
>> + (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))))
>
> What I don’t get is why we’re able to use an encrypted root right now
> without emitting “cryptomount” GRUB commands?

The grub boot process goes more or less like this:

1. Firmware loads the initial image.
1.1. If that image is not the final one, it contains a "pointer" to the
final one, which is loaded by it; this chain can be viewed as part
of the firmware loading for this purpose.
2. The image code reads an initial configuration file, which is usually
generated by grub-install/grub-mkstandalone. Here Grub is placing
the needed the cryptomount lines for the devices needed to mount
target in order to read grub.cfg and other modules.
3. grub.cfg is read (a.k.a. normal mode) and the usual boot process
follows.

The first configuration file is generated automatically by grub-install,
which physically scans the target location (still /boot in our case) and
inserts the needed insmod and cryptomount calls. When the target and
the store don't share the device, the calls leading to the store must be
inserted manually into grub.cfg.

It could be easier to remove completely /boot and use a directory from
the store, but that leads to more writes of the image, as each
reconfiguration involving a change on the devices used for the store
must end up returning a different store file name too. Nonetheless,
that would leave /boot untouched if anybody wants to install their
version of grub there for other purposes...

Toggle quote (9 lines)
>> + (_
>> + ;; No crypto-devices found
>> + '())))
>> + (_
>> + ;; No store found, old format.
>> + '())))
>
> s/No store found/No crypto devices found/ ?

The first comment is reached when crypto-devices isn't found in a
(boot-parameters ... (store ...) ...) form. The second one is reached
when (boot-parameters ...) form doesn't even contain a tag store in it.
It follows the same pattern as store-device, as the old format didn't
have a store element.

On the other hand, I added a period to the first sentence as it was
missing. 0:)

Toggle quote (19 lines)
>> +(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))))
>
> You can use ‘filter-map’ here.

Thanks for the pointer! I've modified a bit tests/boot-parameters.scm
to be extra-sure that I was doing that change OK, as I moved the or to a
internal function for readability too.

Toggle quote (3 lines)
> The rest LGTM! Make sure the “installed-os” and “encrypted-root-os”
> system tests are still fine, and if they are, I guess you can go ahead.

Pushed to master as f00e68ace0 with these changes, after running the
tests and booting up my system.

Happy hacking!
Miguel
Closed
L
L
Ludovic Courtès wrote on 22 Dec 2020 14:41
(name . Miguel Ángel Arruga Vivas)(address . rosen644835@gmail.com)
8735zy2c24.fsf@gnu.org
Hi,

Miguel Ángel Arruga Vivas <rosen644835@gmail.com> skribis:

Toggle quote (3 lines)
> Pushed to master as f00e68ace0 with these changes, after running the
> tests and booting up my system.

Woohoo, thank you!

Ludo’.
Closed
?