On Tuesday 13 May 2008, David Härdeman wrote:
> The attached path allows setting up a crypto device with a keyfile
> stored on another partition (mainly useful when that partition is on a
> usbkey).
Sounds like it would be nice to support.
> In the "setup encrypted volumes" stage of partman, the user will be
> given a list of partitions known to partman and after selecting one, a
It should probably also support plugging in a USB key on demand to be really
useful.
> path must be entered. If that file already exists on the device, it will
> be used as the keyfile, otherwise a new keyfile will be created.
>
> My d-i knowledge is rusty so a review of the patch would be much
> appreciated. (I've also been out of the loop wrt. d-i development,
> deadlines for the next release, etc...so I have no idea how suitable
> this patch is right now in the bigger picture)
I've done a very quick and basic look through the patch; comments below. I'm
afraid I won't have an opportunity for anything more that that in the near
future.
Cheers,
FJP
General: could you please start using the preferred method for case
statements (4 space indent for tests; single tab indent for code)?
(See also the coding style doc under installer/doc/devel/.)
Example:
case $a in
a)
do_a
;;
b)
do_b
;;
esac
Index: lib/crypto-base.sh
===================================================================
- if [ $keytype = keyfile ] || [ $keytype = passphrase ]; then
+ if [ $keytype = keyfile ] || \
+ [ $keytype = passphrase ] || \
+ [ $keytype = removabledev ]; then
Please don't pad spaces: every space does cost memory.
Index: blockdev-keygen
===================================================================
+ while [ ! "$source_id" ]; do
We generally use '[ -z "$source_id" ]' to test for unset/empty.
+ mountpoint="/tmp/blockdev-keygen-tmpmount"
+ if [ ! -e "/tmp/blockdev-keygen-tmpmount" ]; then
+ mkdir "$mountpoint" || return 1
+ fi
Should probably just use 'mkdir -p'.
+ if ! log-output -t blockdev-keygen \
+ mount "$device" "$mountpoint" > /dev/null 2>&1; then
+ return 1
+ fi
Please use log-output for things like this as it at least gives us a
clue what goes wrong if there are errors. Same for unmounts.
+ target="${mountpoint}/${path}"
Use of {} is unnecessary here.
+ # Check that the keyfile is suitable
+ filesize="$(ls -l1 "$target" | sed s'/[[:space:]]\+/ /g' | cut -d' ' -f3)"
+ if [ "$filesize" -lt "$keybytes" ]; then
Is it somehow OK if filesize is larger?
+ cp "$target" "$keyfile"
+ echo "$device" > "$(dirname "$keyfile")/keydev"
+ echo "$path" > "$(dirname "$keyfile")/keypath"
+ echo "plain" > "$(dirname "$keyfile")/keyhash"
This could probably use a comment as I haven't got a clue what's happening
here. Especially The relationship between $keyfile and ! $path/$target is
very hard to work out.
+ keybytes=$(( (keybits + 7)/8 ))
Probably a bit more readable as:
keybytes=$(( (keybits + 7) / 8 ))
Attachment:
signature.asc
Description: This is a digitally signed message part.