[pkg-cryptsetup-devel] Bug#859953: some improvements for /lib/cryptsetup/cryptdisks.functions

[dimas]

Package: cryptsetup
Version: 2:1.7.3-3

hello! i've reviewed subj script and have some improvements/corrections for it

0. line 7
TABFILE=${TABFILE-"/etc/crypttab"}
should be
TABFILE=${TABFILE:-"/etc/crypttab"}
(note the colon), see man dash - Parameter Expansion

1. lines 12-13
this seems useless, being used in the same package, which provides the needed
binary, installed with correct chmods. it's rather unlikely that anyone would
remove "x" chmod from /sbin/cryptsetup

2. lines 32-48
instead of all these var="" you'd better use
unset -v var1 var2 ... varN

3. line 31
opts=$(echo -n $1 | sed 's/ *#.*//')
since tab also can be used instead of space, it's better to use
opts=$(echo -n $1 | sed 's/[[:space:]]*#.*//')

4. lines 83-95
no sanity check for value (use the same one as for "size"?)

5. line 200
no sanity check, should be checked for integer 0-7

6. lines 229-231
	if [ -n "$KEYSCRIPT" ]; then
		return 0
	fi
save few bytes:
[ -n "$KEYSCRIPT" ]  && return 0
:)

7. line 256
	OWNER="$(/bin/ls -l "$(readlink -f $key)" | sed
's/^.\{10\}[+\.]\?.[^[:space:]]* \([^[:space:]]*\).*/\1/')" parsing ls's output
with sed to get owner name, seriously?)) stat -c "%U" (or %u for uid instead of
name) will do the job, also use -L to get rid of readlink
same for group, and for permissions check just check "%a" against "00$"
current sed slash-fence looks like hell)))

8. lines 469-471
		if mount "$point" >/dev/null; then
			MOUNTED="$MOUNTED $point"
		fi
prevent attempts to mount already mounted destination:
		mountpoint -q "$point" || { mount "$point" >/dev/null \
		&& MOUNTED="$MOUNTED $point"; }
also, shouldn't we use 2> redirection, if we want it to be quiet? or even
"> /dev/null 2>&1"
likewise for line 480:
		mountpoint -q "$point" && umount "$point" 2>/dev/null

9. line 506
	if [ "x$TMPFS" = "x" ] || [ ! -b "/dev/mapper/${dst}_unformatted" ];
then

if [ -z "$TMPFS" ] ...)))

10. lines 528-546, do_close()
all the same fits into
echo "$opts" | grep "\<luks\>" && cryptsetup luksClose "$dst" || \
	cryptsetup remove "$dst"
but even after that, "remove" and "luksClose" are deprecated, so just
cryptsetup close "$dst"
no need for dedicated function, actually)))

11. lines 577-580
	if [ "${src#UUID=}" != "$src" ]; then
		src="/dev/disk/by-uuid/${src#UUID=}"
	elif [ "${src#LABEL=}" != "$src" ]; then
		src="/dev/disk/by-label/$(printf '%s' "${src#LABEL=}" | sed
's,/,\\x2f,g')" shouldn't you take a look at blkid -L "label" / -U "uuid"
instead? same for 701-704, 741-745
this will also eliminate 621-626

12. lines 772-780
		for i in 1 2 4 8 16 32; do ...
not sure if it's nice idea at all or not, but with encrypted root it just slows
down shutdown, since we can wait forever with no success, as device holding
root will never be released, as we can not unmount /.
we definetely need some special treatment for this. in case crypt device is
used as root directly, it's pretty simple (we just need to find out mountpoint
of a mapped device and check if it's /), but there are more complex setups,
where crypt device is a member of lvm/raid/etc, that, in turn, holds root fs.
but in this case lsblk can help us. as en example, my setup is:
NAME          MAJ:MIN RM  SIZE RO TYPE  MOUNTPOINT
sda7_crypt    253:0    0  3,9G  0 crypt 
└─debian-root 253:1    0  3,9G  0 lvm   /
and this is how we can check, if target holds anything on top of it, that holds
root fs:
lsblk -r -o NAME,MOUNTPOINT -n /dev/mapper/sda7_crypt | grep " /$"
(this will also work even if taget is root device itself, without any extra
layers)
so we can add smth like
if (line above); then
	# issue some message, that $dst is root device, so we skip it
	continue
fi
lsblk resides in /bin and doesn't rely on any libs from /usr, so it seems safe
in case if /usr is already unmounted (if separate)

13. in do_stop() we only process devices listed in crypttab file. but there can
also be manually mapped devices, opened by user, like removable media or some
containers. imho, it seems a good idea to shutdown them as well. though,
cryptsetup doesn't provide any way to list currently opened devices (and maybe
doesn't manage any list of them at all), but we can run
dmsetup ls --target crypt | cut -f1
to get a list of dm devices with "crypt" target, and them we can check them in
addition by
cryptsetup status "$target" >/dev/null 2>&1
which will return non-zero status, if $target is not active cryptsetup mapping
taking (10) into account, we actualy don't need anything from crypttab at all
to stop runnning devices, so we can completely switch from crypttab to above
scheme