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

[Miguel Ángel Arruga Vivas]

Hi Ludo,

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

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

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

>> +            (_
>> +             ;; 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:)

>> +(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.

> 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