[bug#65002,v2,2/2] gnu: bootloader: grub: Add support for loading an additional initrd

In order to be able to provide decryption keys for the LUKS device, they need
to be available in the initial ram disk.  However they cannot be stored inside
the usual initrd, since it is stored in the store and being a
world-readable (as files in the store are) is not a desired property for a
initrd containing decryption keys.  This commit adds an option to load
additional initrd during the boot, one that is not stored inside the store and
therefore can contain secrets.

Since only grub supports encrypted /boot, only grub is modified to use the
extra-initrd.  There is no use case for the other bootloaders.

* doc/guix.texi (Bootloader Configuration): Describe the new extra-initrd
field.
* gnu/bootloader.scm: Add extra-initrd field to bootloader-configuration
* gnu/bootloader/grub.scm: Use the new extra-initrd field
---
 doc/guix.texi           | 20 ++++++++++++++++++++
 gnu/bootloader.scm      |  6 +++++-
 gnu/bootloader/grub.scm |  6 ++++--
 3 files changed, 29 insertions(+), 3 deletions(-)

Tomas Volf <wolf@wolfsden.cz> skribis:

> In order to be able to provide decryption keys for the LUKS device, they need
> to be available in the initial ram disk.  However they cannot be stored inside
> the usual initrd, since it is stored in the store and being a
> world-readable (as files in the store are) is not a desired property for a
> initrd containing decryption keys.

This explanation should go in the manual IMO (it’s already partly there).

> This commit adds an option to load additional initrd during the boot,
> one that is not stored inside the store and therefore can contain
> secrets.
>
> Since only grub supports encrypted /boot, only grub is modified to use the
> extra-initrd.  There is no use case for the other bootloaders.
>
> * doc/guix.texi (Bootloader Configuration): Describe the new extra-initrd
> field.
> * gnu/bootloader.scm: Add extra-initrd field to bootloader-configuration
> * gnu/bootloader/grub.scm: Use the new extra-initrd field

It’d be great if you could specify the entities changes in each file
(which variable/procedure is changed, what is added/removed).  A
committer can do it on your behalf later if you’re unsure.

> +@item @code{extra-initrd} (default: @code{#f})
> +Path to an additional initrd to load.  Should not point to a file in the

s/Path/File name/ (by convention)

Please make full sentences.  “Should not” is probably too strong;
perhaps: “It may or may not point to a file in the store, but the main
use case is for out-of-store files containing secrets.”

> +store.  Typical use case is making keys to unlock LUKS device available

Add a line break after “store.” to distinguish the reference from the
discussion of one possible use case.

> +during the boot process.  For any use case not involving secrets, you
> +should use regular initrd (@pxref{operating-system Reference,
> +@code{initrd}}) instead.
> +
> +Suitable image can be created for example like this:
> +
> +@example
> +echo /key-file.bin | cpio -oH newc >/key-file.cpio
> +chmod 0000 /key-file.cpio
> +@end example
> +
> +Be careful when using this option, since pointing to a file that is not
> +readable by the grub while booting will cause the boot to fail and
> +require a manual edit of the initrd line in the grub menu.
> +
> +Currently only supported by grub.

s/grub/GRUB/

Would be great if you could include also a short config example here, or
add a cross-reference to the example for
‘luks-device-mapping-with-options’ if that covers both.

> +  (extra-initrd          bootloader-configuration-extra-initrd
> +                         (default #f))    ;string | #f
> +  )

No lonely paren please.  :-)

Otherwise LGTM.

Could you send updated patches with these minor changes?

Thanks!

Ludo’.

On 2024-01-10 00:28:18 +0100, Ludovic Courtès wrote:
> Tomas Volf <wolf@wolfsden.cz> skribis:
> 
> > In order to be able to provide decryption keys for the LUKS device, they need
> > to be available in the initial ram disk.  However they cannot be stored inside
> > the usual initrd, since it is stored in the store and being a
> > world-readable (as files in the store are) is not a desired property for a
> > initrd containing decryption keys.
> 
> This explanation should go in the manual IMO (it’s already partly there).

Done.

> 
> > This commit adds an option to load additional initrd during the boot,
> > one that is not stored inside the store and therefore can contain
> > secrets.
> >
> > Since only grub supports encrypted /boot, only grub is modified to use the
> > extra-initrd.  There is no use case for the other bootloaders.
> >
> > * doc/guix.texi (Bootloader Configuration): Describe the new extra-initrd
> > field.
> > * gnu/bootloader.scm: Add extra-initrd field to bootloader-configuration
> > * gnu/bootloader/grub.scm: Use the new extra-initrd field
> 
> It’d be great if you could specify the entities changes in each file
> (which variable/procedure is changed, what is added/removed).  A
> committer can do it on your behalf later if you’re unsure.

Done, this was one of my first patches and I was quite unsure about the commit
message format.  These days I am still unsure, but a little less so. ^_^

> 
> > +@item @code{extra-initrd} (default: @code{#f})
> > +Path to an additional initrd to load.  Should not point to a file in the
> 
> s/Path/File name/ (by convention)
> 
> Please make full sentences.  “Should not” is probably too strong;
> perhaps: “It may or may not point to a file in the store, but the main
> use case is for out-of-store files containing secrets.”

For content that can be present in the store, the regular `initrd' should be
used instead I think.  However I adjusted the wording.

> 
> > +store.  Typical use case is making keys to unlock LUKS device available
> 
> Add a line break after “store.” to distinguish the reference from the
> discussion of one possible use case.
> 
> > +during the boot process.  For any use case not involving secrets, you
> > +should use regular initrd (@pxref{operating-system Reference,
> > +@code{initrd}}) instead.
> > +
> > +Suitable image can be created for example like this:
> > +
> > +@example
> > +echo /key-file.bin | cpio -oH newc >/key-file.cpio
> > +chmod 0000 /key-file.cpio
> > +@end example
> > +
> > +Be careful when using this option, since pointing to a file that is not
> > +readable by the grub while booting will cause the boot to fail and
> > +require a manual edit of the initrd line in the grub menu.
> > +
> > +Currently only supported by grub.
> 
> s/grub/GRUB/
> 
> Would be great if you could include also a short config example here, or
> add a cross-reference to the example for
> ‘luks-device-mapping-with-options’ if that covers both.

I added an example illustrating how these two work together.

> 
> > +  (extra-initrd          bootloader-configuration-extra-initrd
> > +                         (default #f))    ;string | #f
> > +  )
> 
> No lonely paren please.  :-)

Well I moved the paren, but now the comment (string | #f) looks like it is for
the whole top-level sexp, not just for the extra-initrd field.

> 
> Otherwise LGTM.
> 
> Could you send updated patches with these minor changes?

I will soon, just want spent a bit of time trying to make the system test for
this.

> 
> Thanks!

And thank you again for the review.

Tomas