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

-- 
There are only two hard things in Computer Science:
cache invalidation, naming things and off-by-one errors.