[bug#73654,v4] mapped-devices: luks: Support passing --allow-discards during open

From: Sören Tempel <soeren@soeren-tempel.net>

* gnu/system/mapped-devices.scm (open-luks-device): Support opening
LUKS devices with the --allow-discards option.
* gnu/system/mapped-devices.scm (luks-device-mapping-with-options):
Pass through the allow-discards? keyword argument.
* doc/guix.texi (Mapped Devices): Update documentation for the
luks-device-mapping-with-options procedure.

Co-authored-by: Sisiutl <sisiutl@egregore.fun>
---
Changes since v3: Fix replacement of “Solid State Disks” with “solid
state disks” in doc/guix.texi.  That is, only perform this replacement
locally on the added text and not the whole document.

 doc/guix.texi                 | 11 +++++++++-
 gnu/system/mapped-devices.scm | 39 +++++++++++++++++++++--------------
 2 files changed, 33 insertions(+), 17 deletions(-)


base-commit: f2b3c36bee8c232b026a66de93db38e13fbd7076
  

tag 73654 + moreinfo
quit

Hi!

soeren@soeren-tempel.net writes:

> From: Sören Tempel <soeren@soeren-tempel.net>
>
> * gnu/system/mapped-devices.scm (open-luks-device): Support opening
> LUKS devices with the --allow-discards option.
> * gnu/system/mapped-devices.scm (luks-device-mapping-with-options):
> Pass through the allow-discards? keyword argument.
> * doc/guix.texi (Mapped Devices): Update documentation for the
> luks-device-mapping-with-options procedure.
>
> Co-authored-by: Sisiutl <sisiutl@egregore.fun>

I was about to apply it with the following cosmetic changes (mostly to
meet the 80 max column width):

--8<---------------cut here---------------start------------->8---
> ---
> Changes since v3: Fix replacement of “Solid State Disks” with “solid
> state disks” in doc/guix.texi.  That is, only perform this replacement
> locally on the added text and not the whole document.
>
>  doc/guix.texi                 | 11 +++++++++-
>  gnu/system/mapped-devices.scm | 39 +++++++++++++++++++++--------------
>  2 files changed, 33 insertions(+), 17 deletions(-)
>
> diff --git a/doc/guix.texi b/doc/guix.texi
> index b1b6d98e74..6eb9fcb8ee 100644
> --- a/doc/guix.texi
> +++ b/doc/guix.texi
> @@ -18402,7 +18402,7 @@ Mapped Devices
>  @code{dm-crypt} Linux kernel module.
>  @end defvar
>  
> -@deffn {Procedure} luks-device-mapping-with-options [#:key-file]
> +@deffn {Procedure} luks-device-mapping-with-options [#:key-file #:allow-discards?]
>  Return a @code{luks-device-mapping} object, which defines LUKS block
>  device encryption using the @command{cryptsetup} command from the
>  package with the same name.  It relies on the @code{dm-crypt} Linux
> @@ -18424,6 +18424,15 @@ Mapped Devices
>   (type (luks-device-mapping-with-options
>          #:key-file "/crypto.key")))
>  @end lisp
> +
> +
> +@code{allow-discards?} allows the use of discard (TRIM) requests for the
> +underlying device.  This is useful for solid state drives.  However,
> +this option can have a negative security impact because it can make
> +file system level operations visible on the physical device.  For more
> +information, refer to the description of the @code{--allow-discards}
> +option in the @code{cryptsetup-open(8)} man page.
> +
>  @end deffn
>  
>  @defvar raid-device-mapping
> diff --git a/gnu/system/mapped-devices.scm b/gnu/system/mapped-devices.scm
> index 931c371425..3a8f0d66fe 100644
> --- a/gnu/system/mapped-devices.scm
> +++ b/gnu/system/mapped-devices.scm
> @@ -194,9 +194,10 @@ (define (check-device-initrd-modules device linux-modules location)
>  ;;; Common device mappings.
>  ;;;
>  
> -(define* (open-luks-device source targets #:key key-file)
> +(define* (open-luks-device source targets #:key key-file allow-discards?)
>    "Return a gexp that maps SOURCE to TARGET as a LUKS device, using
> -'cryptsetup'."
> +'cryptsetup'.  When ALLOW-DISCARDS? is true, the use of discard (TRIM) requests is
> +allowed for the underlying device."
>    (with-imported-modules (source-module-closure
>                            '((gnu build file-systems)
>                              (guix build utils))) ;; For mkdir-p
> @@ -234,17 +235,21 @@ (define* (open-luks-device source targets #:key key-file)
>                                              (loop (- tries-left 1))))))
>                            (error "LUKS partition not found" source))
>                        source)))
> -             ;; We want to fallback to the password unlock if the keyfile fails.
> -             (or (and keyfile
> -                      (zero? (system*/tty
> -                              #$(file-append cryptsetup-static "/sbin/cryptsetup")
> -                              "open" "--type" "luks"
> -                              "--key-file" keyfile
> -                              partition #$target)))
> -                 (zero? (system*/tty
> -                         #$(file-append cryptsetup-static "/sbin/cryptsetup")
> -                         "open" "--type" "luks"
> -                         partition #$target)))))))))
> +             (let ((cryptsetup-flags (cons*
> +                                       "open" "--type" "luks" partition #$target
> +                                       (if allow-discards?
> +                                           '("--allow-discards")
> +                                           '()))))
> +               ;; We want to fallback to the password unlock if the keyfile fails.
> +               (or (and keyfile
> +                        (zero?
> +                          (apply system*/tty
> +                                 #$(file-append cryptsetup-static "/sbin/cryptsetup")
> +                                 "--key-file" keyfile
> +                                 cryptsetup-flags)))
> +                   (zero? (apply system*/tty
> +                                 #$(file-append cryptsetup-static "/sbin/cryptsetup")
> +                                 cryptsetup-flags))))))))))
>  
>  (define (close-luks-device source targets)
>    "Return a gexp that closes TARGET, a LUKS device."
> @@ -286,13 +291,15 @@ (define luks-device-mapping
>                ((gnu build file-systems)
>                 #:select (find-partition-by-luks-uuid system*/tty))))))
>  
> -(define* (luks-device-mapping-with-options #:key key-file)
> +(define* (luks-device-mapping-with-options #:key key-file allow-discards?)
>    "Return a luks-device-mapping object with open modified to pass the arguments
>  into the open-luks-device procedure."
>    (mapped-device-kind
>     (inherit luks-device-mapping)
> -   (open (λ (source targets) (open-luks-device source targets
> -                                               #:key-file key-file)))))
> +   (open (λ (source targets)
> +           (open-luks-device source targets
> +                             #:key-file key-file
> +                             #:allow-discards? allow-discards?)))))
>  
>  (define (open-raid-device sources targets)
>    "Return a gexp that assembles SOURCES (a list of devices) to the RAID device
>
> base-commit: f2b3c36bee8c232b026a66de93db38e13fbd7076
--8<---------------cut here---------------end--------------->8---

But unfortunately it appears to hang at least the 'encrypted-root-os'
system test, which you can run like:

--8<---------------cut here---------------start------------->8---
$ make check-system TESTS=encrypted-root-os
[...]
cSeaBIOS (version 1.16.2/GNU Guix)


iPXE (https://ipxe.org) 00:03.0 CA00 PCI2.10 PnP PMM+0EFCB030+0EF0B030 CA00
                                                                               


Booting from Hard Disk...
GRUB loading..
Welcome to GRUB!

Enter passphrase for hd0,gpt2 (12345678-1234-1234-1234-123456789abc): 
Attempting to decrypt master key...
lot 0 opened
  C-c C-cmake: *** [Makefile:7562: check-system] Interrompre
--8<---------------cut here---------------end--------------->8---

Would you have an idea of why this happens and how we could avoid the
hang in the test?

Thanks,
  

soeren@soeren-tempel.net writes:

> From: Sören Tempel <soeren@soeren-tempel.net>
>
> * gnu/system/mapped-devices.scm (open-luks-device): Support opening
> LUKS devices with the --allow-discards option.
> * gnu/system/mapped-devices.scm (luks-device-mapping-with-options):
> Pass through the allow-discards? keyword argument.
> * doc/guix.texi (Mapped Devices): Update documentation for the
> luks-device-mapping-with-options procedure.
>
> Co-authored-by: Sisiutl <sisiutl@egregore.fun>
> ---
> Changes since v3: Fix replacement of “Solid State Disks” with “solid
> state disks” in doc/guix.texi.  That is, only perform this replacement
> locally on the added text and not the whole document.
>
>  doc/guix.texi                 | 11 +++++++++-
>  gnu/system/mapped-devices.scm | 39 +++++++++++++++++++++--------------
>  2 files changed, 33 insertions(+), 17 deletions(-)
>
> diff --git a/doc/guix.texi b/doc/guix.texi
> index b1b6d98e74..6eb9fcb8ee 100644
> --- a/doc/guix.texi
> +++ b/doc/guix.texi
> @@ -18402,7 +18402,7 @@ Mapped Devices
>  @code{dm-crypt} Linux kernel module.
>  @end defvar
>  
> -@deffn {Procedure} luks-device-mapping-with-options [#:key-file]
> +@deffn {Procedure} luks-device-mapping-with-options [#:key-file #:allow-discards?]
>  Return a @code{luks-device-mapping} object, which defines LUKS block
>  device encryption using the @command{cryptsetup} command from the
>  package with the same name.  It relies on the @code{dm-crypt} Linux
> @@ -18424,6 +18424,15 @@ Mapped Devices
>   (type (luks-device-mapping-with-options
>          #:key-file "/crypto.key")))
>  @end lisp
> +
> +
> +@code{allow-discards?} allows the use of discard (TRIM) requests for the
> +underlying device.  This is useful for solid state drives.  However,
> +this option can have a negative security impact because it can make
> +file system level operations visible on the physical device.  For more
> +information, refer to the description of the @code{--allow-discards}
> +option in the @code{cryptsetup-open(8)} man page.
> +
>  @end deffn
>  
>  @defvar raid-device-mapping
> diff --git a/gnu/system/mapped-devices.scm b/gnu/system/mapped-devices.scm
> index 931c371425..3a8f0d66fe 100644
> --- a/gnu/system/mapped-devices.scm
> +++ b/gnu/system/mapped-devices.scm
> @@ -194,9 +194,10 @@ (define (check-device-initrd-modules device linux-modules location)
>  ;;; Common device mappings.
>  ;;;
>  
> -(define* (open-luks-device source targets #:key key-file)
> +(define* (open-luks-device source targets #:key key-file allow-discards?)
>    "Return a gexp that maps SOURCE to TARGET as a LUKS device, using
> -'cryptsetup'."
> +'cryptsetup'.  When ALLOW-DISCARDS? is true, the use of discard (TRIM) requests is
> +allowed for the underlying device."
>    (with-imported-modules (source-module-closure
>                            '((gnu build file-systems)
>                              (guix build utils))) ;; For mkdir-p
> @@ -234,17 +235,21 @@ (define* (open-luks-device source targets #:key key-file)
>                                              (loop (- tries-left 1))))))
>                            (error "LUKS partition not found" source))
>                        source)))
> -             ;; We want to fallback to the password unlock if the keyfile fails.
> -             (or (and keyfile
> -                      (zero? (system*/tty
> -                              #$(file-append cryptsetup-static "/sbin/cryptsetup")
> -                              "open" "--type" "luks"
> -                              "--key-file" keyfile
> -                              partition #$target)))
> -                 (zero? (system*/tty
> -                         #$(file-append cryptsetup-static "/sbin/cryptsetup")
> -                         "open" "--type" "luks"
> -                         partition #$target)))))))))
> +             (let ((cryptsetup-flags (cons*
> +                                       "open" "--type" "luks" partition #$target
> +                                       (if allow-discards?
> +                                           '("--allow-discards")
> +                                           '()))))
> +               ;; We want to fallback to the password unlock if the keyfile fails.
> +               (or (and keyfile
> +                        (zero?
> +                          (apply system*/tty
> +                                 #$(file-append cryptsetup-static "/sbin/cryptsetup")
> +                                 "--key-file" keyfile
> +                                 cryptsetup-flags)))

I am not sure about passing the --key-file before the `open' command.
It does seem to work (currently), but I am not sure we should assume it
always will.

Is this type of usage documented somewhere?  All manuals I found are
passing the arguments after `open'.

You could rewrite this into a lambda returning the argument list, the
lambda would splice them (both keyfile and discard) into the correct
places.

> +                   (zero? (apply system*/tty
> +                                 #$(file-append cryptsetup-static "/sbin/cryptsetup")
> +                                 cryptsetup-flags))))))))))
>  
>  (define (close-luks-device source targets)
>    "Return a gexp that closes TARGET, a LUKS device."
> @@ -286,13 +291,15 @@ (define luks-device-mapping
>                ((gnu build file-systems)
>                 #:select (find-partition-by-luks-uuid system*/tty))))))
>  
> -(define* (luks-device-mapping-with-options #:key key-file)
> +(define* (luks-device-mapping-with-options #:key key-file allow-discards?)
>    "Return a luks-device-mapping object with open modified to pass the arguments
>  into the open-luks-device procedure."
>    (mapped-device-kind
>     (inherit luks-device-mapping)
> -   (open (λ (source targets) (open-luks-device source targets
> -                                               #:key-file key-file)))))
> +   (open (λ (source targets)
> +           (open-luks-device source targets
> +                             #:key-file key-file
> +                             #:allow-discards? allow-discards?)))))
>  
>  (define (open-raid-device sources targets)
>    "Return a gexp that assembles SOURCES (a list of devices) to the RAID device
>
> base-commit: f2b3c36bee8c232b026a66de93db38e13fbd7076
  

Hi,

Tomas Volf <~@wolfsden.cz> writes:

[...]

>> +               ;; We want to fallback to the password unlock if the keyfile fails.
>> +               (or (and keyfile
>> +                        (zero?
>> +                          (apply system*/tty
>> +                                 #$(file-append cryptsetup-static "/sbin/cryptsetup")
>> +                                 "--key-file" keyfile
>> +                                 cryptsetup-flags)))
>
> I am not sure about passing the --key-file before the `open' command.
> It does seem to work (currently), but I am not sure we should assume it
> always will.

It's documented as such, per 'cryptsetup --help':

--8<---------------cut here---------------start------------->8---
cryptsetup 2.6.1 flags: UDEV BLKID KEYRING KERNEL_CAPI 
Usage: cryptsetup [OPTION...] <action> <action-specific>

Help options:
  -?, --help                            Show this help message
      --usage                           Display brief usage
  -V, --version                         Print package version
      --active-name=STRING              Override device autodetection of dm device to be reencrypted
      --align-payload=SECTORS           Align payload at <n> sector boundaries - for luksFormat
      --allow-discards                  Allow discards (aka TRIM) requests for device
--8<---------------cut here---------------end--------------->8---

There are many options though perhaps we should just provide a
#:extra-args escape hatch.
  

Hello,

Pushed as commit 7aa855b05b.