[pkg-cryptsetup-devel] Bug#901795: cryptsetup: new version may break 3rd party keyscripts (and thus boot)

[Christoph Anton Mitterer]

Hey. :-)


On Mon, 2018-06-25 at 14:52 +0200, Guilhem Moulin wrote:
> But rather
> than
> using an undocumented interface and assume it'll never break, the
> right
> thing to do is to ask us to document said interface and commit to
> maintain it.

Well, back then when I "developed" my gnupg keyscript there was one in
cryptsetup which, IIRC, was even dysfunctional.
I tried to get Jonas mine included, but he didn't like it, though I
consider(ed) it pretty sophisticated (but I haven't looked at the
current decrypt_gnupg,... so maybe this is now fine, too).

So I kinda had "upstream contact" but things just got lost over time
;-)

If you like I can send you the full set of scripts&hooks for review.



> Uh, are you referring to the regressions that were filed last
> week?  You
> can't compare your own #901795 with these regressions, where we broke
> setups following the documented interface…

There was no real criticism in my words :-) I just wanted to point out
that these things simply happen... people use such stuff if it's
visible ;-)
No blame on you guys!


> > Will that process only those crypttab entries which actually
> > require to
> > be processed during initramfs?
> 
> Yes.

Nice :-)


> Right now we'd like things to settle a bit, and fixing actual
> regression
> have higher priority.  I'll plan to start working on this once the
> package enters testing, but I'm not promising anything.

I'd now simply start to use the "interface" you suggested me in your
last mail. Question on that:

>    . /lib/cryptsetup/functions
>
>    [ -s "$DESTDIR/cryptroot/crypttab" ] || return 0

Why is this necessary? I assume when I PREREQ=cryptroot, than
$DESTDIR/cryptroot/crypttab finished(!) and contains all devices needed
to be unlocked during initramfs, right?
And I'd guess this is for the case that there are no such devices and
thus no "$DESTDIR/cryptroot/crypttab" would be even written (and my
hook wouldn't need to do anything either). Right?


>    while read CRYPTTAB_NAME CRYPTTAB_SOURCE CRYPTTAB_KEY CRYPTTAB_OPTIONS; do
Do you guys do any quoting in "$DESTDIR/cryptroot/crypttab"?
Cause read without -r will interpret \ as quoting character... and this
is IMO always a bit dangerous if the same is then used...

>        if [ "${CRYPTTAB_NAME#\#}" = "$CRYPTTAB_NAME" ] \
What is this intended for?
(Oh and did you guys notice that this is a bashism? ${var#word} is not POSIX sh compatbile)


>           && crypttab_parse_options "$CRYPTTAB_OPTIONS" n; then
I'd assume I don't need this, if I don't use the options like SIZE,
CIPHER and such in my hookscript, right? I basically use only what's in
the 3rd field of crypttab, and that should correspond to CRYPTTAB_KEY
here?!
When I don't actually need crypttab_parse_options, I also shouldn't
need to source /lib/cryptsetup/functions above.


>            […]
>        fi
>    done <"$DESTDIR/cryptroot/crypttab"



So apart from not understanding yet what the "${CRYPTTAB_NAME#\#}" =
"$CRYPTTAB_NAME" is meant for, I'd say it's enough for my initramfs
hook to do:
>   while read -r CRYPTTAB_NAME CRYPTTAB_SOURCE CRYPTTAB_KEY
CRYPTTAB_OPTIONS; do
>   ...

assuming each line of "$DESTDIR/cryptroot/crypttab" will continue to
adhere to this format.

 
> > Wouldn't it have been the best if the cryptsetup initramfs hooks
> > simply
> > only add stuff to the initramfs, if this was required for booting
> > (or
> > didn't they do this already?), without any need to
> > "configure/enable"
> > this by installing a package.
> > Right now this seems a bit error prone and kinda abusing the
> > package
> > management for what should be rather configuration.
> 
> See the changelog entry for 2:2.0.3-1, and the 2 merged bug it
> closed.

Hmm... I've looked over it, and it seems the only reason was to be able
to depend on busybox (only if cryptsetup within initramfs is used)?
Cause the other mess Michael Biebl described seems to be rather either
other bugs or somehow solvable at a first glance.

Well in the end I personally still consider such "configuring" packages
 rather ugly.
One could have made a warning if initramfs-tools.conf says BUSYBOX=N
but cryptsetup needs to enable them nevertheless.
Or vice versa let cryptsetup not forcibly enable it, and give a
warning/error during initramfs creation if it's not there.

 
> > And if someone would have wanted to disable cryptsetup's initramfs-
> > tools integration, one could have simply provided a config file
> > which
> > is sourced by the hooks and then exit if desired (or even better,
> > initramfs-tools should provide some masking mechanism,... like
> > systemd
> > does when one adds symlinks to /dev/null in /etc/... with the same
> > name
> > as the unit file shipped by the package in /usr/...
> 
> See /etc/cryptsetup-initramfs/conf-hook.  We're deprecating
> CRYPTSETUP=[y|n] now that the we split the initramfs integration in
> its
> own package, though.

Well but that boils just down to my point:
The package management system shouldn't be therefore configuration -
config files should.

Anyway,... just cosmetic thoughts from my personal PoV... I'm happy as
it is and with the great efforts you guys do for the benefit of all of
us!


Cheers & thanks,
Chris.