Hi David,
On Tue, May 13, 2008 at 08:02:28PM +0200, David Härdeman wrote:
> 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
> 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.
Nice work. This will allow us to close #381894.
> I've done a test install using qemu (with a secondary qemu harddisk as
> the removable device) and a SVN version of cryptsetup (which has the
> necessary "mountdev" keyscript). Also, due to a bug in klibc, only ext3
> is supported for now (bug reported, will be fixed before the next upload
> of cryptsetup which will allow any common fs to be used).
Sounds like we should hold off until cryptsetup is ready.
> 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)
Otavio, could you advise? Is it okay to commit such new
features to trunk soon and then upload after beta2?
> I'm also planning to use some of the infrastructure of the patch to add
> support for two-factor keys (ask a passphrase, hash it, get a keyfile
> from usb stick, xor the two together, use that as the key) and
> smartcards (I've already ordered the hardware, dunno when I'll get it).
Great. Looking forward to it :-)
> +Template: partman-crypto/removable-source-path
> +Template: partman-crypto/removable-source-partition
> +Template: partman-crypto/removable-confirm-create
> +Template: partman-crypto/removable-bad-keyfile
The names of those *removable* templates confused me a bit
when I was reading the code. Perhaps we could find more
specific and self-descriptive names?
Hm. This is what I came up with. Not sure I acutally like
them better, but they are more specific:
removabledev-partition
removabledev-key-path
removabledev-key-confirm-create
removabledev-key-badformat
> +Template: partman-crypto/removable-confirm-create
> +Type: boolean
> +Default: false
> +# :sl3:
> +_Description: Create new key?
> + No key was found on ${DEVICE} at path ${PATH}, do you wish to create
> + a new key?
s/at path/in path/ ?
> +Template: partman-crypto/removable-bad-keyfile
> +Type: error
> +# :sl3:
> +_Description: Invalid encryption key
> + You have selected a pre-existing key file which is not suitable as a
> + crypto key as it is not large enough. Please try using a different
> + key file.
Terminology - "key" vs. "crypto key" vs "encryption key".
We should use one term consistently. Personally, I think
I'd go for "encryption key".
> Index: finish.d/crypto_config
> ===================================================================
> --- finish.d/crypto_config (revision 53290) > +++ finish.d/crypto_config
> (working copy)
> @@ -96,6 +96,27 @@
> keyfile="/dev/urandom"
> elif [ $keytype = passphrase ]; then
> keyfile="none"
> + elif [ $keytype = removabledev ]; then
> + local keydev keypath udevlinks tmp
> + keypath=$(cat $realdevdir/keypath)
> + keydev=$(cat $realdevdir/keydev)
> +
> + # We need to use stable device names as using e.g. /dev/hda2
> + # will break the boot if a second USB key is present.
> + udevlinks=""
> + for tmp in by-id by-uuid by-label by-path; do
> + if [ -d "/dev/disk/$tmp" ]; then
> + udevlinks="$udevlinks /dev/disk/$tmp/*"
> + fi
> + done
> + for tmp in $udevlinks; do
> + if [ "$(readlink -f "$tmp")" = "$keydev" ]; then
> + keydev="$tmp"
> + break;
> + fi
> + done
I think this should be broken out into a generic
persistent_device_name() in partman-base.
Such a function for mapping devices to persistent names
will be useful when we start using persistent device
names in other parts of partman.
Once that happens, we might also want to make all the
persistent device names use the same mechanism (e.g. by-id).
Last I remember the discussion was still undecided about
which mechanism was most suitable.
> Index: blockdev-keygen
> ===================================================================
> --- blockdev-keygen (revision 53290)
> +++ blockdev-keygen (working copy)
> @@ -192,6 +192,110 @@
> return 0
> }
The below would rather fit into crypto-base.sh than
blockdev-keygen IMO.
- It creates files in the device directory.
- The debconf interaction is about getting the right
removable device and the final path to store the key.
No reason blockdev-keygen should care about those.
> +# Create or load an already created keyfile on a user-specified device
> +create_removable_keyfile() {
> + local keyfile keybytes noninteractive source_dev source_id path
> mountpoint
> + keyfile=$1
> + keybytes=$2
> + noninteractive=true
> +
> + . /lib/partman/lib/base.sh
^ This could go away in crypto-base.sh
> + while true; do
> + source_dev=''
> + source_id=''
Minor: Could get rid of the ''
> + while [ ! "$source_id" ]; do
> + choices=$(partition_tree_choices)
> + debconf_select critical
> partman-crypto/removable-source-partition "$choices" blah
> +
"blah"? :-) Is there a sensible default choice?
> + case $? in
> + 1)
> + $noninteractive
> + ;;
> + 255)
> + return 1
> + ;;
> + esac
> + noninteractive='return 1'
> + source_dev=${RET%//*}
> + source_id=${RET#*//}
> + done
> + source_dev=${source_dev##*/}
The control flow makes me a bit dizzy :-)
The idea is to return 1 after the second non-successfull
interation?
Why? Why name it "noninteractive"?
We should be able to turn this into something a bit
more approachable for casual readers.
> + cd "$DEVICES/$source_dev" || return 1
> +
> + local x1 x2 x3 x4 x5 device x6
> + open_dialog PARTITION_INFO "$source_id"
> + read_line x1 x2 x3 x4 x5 device x6
> + close_dialog
> +
> + if [ -z "$device" ]; then
> + return 1
> + fi
> +
> + defpath="/keys/$(cat /etc/hostname)$(echo "$device" | sed
> 's/\//_/g')"
> + db_set partman-crypto/removable-source-path "$defpath"
> + db_subst partman-crypto/removable-source-path DEVICE
> "$(humandev $device)"
> + db_input critical partman-crypto/removable-source-path || true
> + db_go || return 1
> + db_get partman-crypto/removable-source-path
+ template="partman-crypto/removable-source-path"
+ db_set $template "$defpath"
+ db_subst $template DEVICE "$(humandev $device)"
+ db_input critical $template || true
+ db_go || return 1
+ db_get $template
.. would save a few bytes.
> + path="$RET"
> +
> + if [ -z "$path" ]; then
> + continue;
> + fi
> +
> + mountpoint="/tmp/blockdev-keygen-tmpmount"
> + if [ ! -e "/tmp/blockdev-keygen-tmpmount" ]; then
> + mkdir "$mountpoint" || return 1
> + fi
Just use mkdir -p "$mountpoint" as Frans suggested.
> + if ! log-output -t blockdev-keygen \
> + mount "$device" "$mountpoint" > /dev/null 2>&1; then
> + return 1
> + fi
log-output already suppresses stdout and stderr. You
can remove the extra redirections.
> + local target filesize
> + target="${mountpoint}/${path}"
> + if [ -e "$target" ]; then
> + # Check that the keyfile is suitable
> + filesize="$(ls -l1 "$target" | sed s'/[[:space:]]\+/
> /g' | cut -d' ' -f3)"
Could use wc instead:
wc -c "$target"
> + if [ "$filesize" -lt "$keybytes" ]; then
> + db_fset partman-crypto/removable-bad-keyfile
> seen false
> + db_input critical
> partman-crypto/removable-bad-keyfile
> + db_go || true
> + continue
> + fi
> + else
> + # We need to create a new keyfile
> + db_subst partman-crypto/removable-confirm-create DEVICE
> "$(humandev $device)"
> + db_subst partman-crypto/removable-confirm-create PATH
> "$path"
> + db_input critical
> partman-crypto/removable-confirm-create || true
> + db_go || return 1
> + db_get partman-crypto/removable-confirm-create
Similar comment about using $template
> + if ! mkdir -p "$(dirname "$target")" || \
> + ! create_random_keyfile "$target" "$keybytes"; then
> + umount "$mountpoint" > /dev/null 2>&1 || return
> 1
I'd say wrap invocations of umount in log-output.
Better to know when it fails.
> + return 1
> + fi
> + fi
> +
> + cp "$target" "$keyfile"
> + echo "$device" > "$(dirname "$keyfile")/keydev"
> + echo "$path" > "$(dirname "$keyfile")/keypath"
> + echo "plain" > "$(dirname "$keyfile")/keyhash"
part=$(dirname "$keyfile")
echo "$device" > "$part/keydev"
..
> + umount "$mountpoint" > /dev/null 2>&1 || return 1
> + break
> + done
> + return 0
> +}
> +
Modulo the above comments, I think this is fine to add
to partman-crypto trunk. Thanks for your work!
Max
--
To UNSUBSCRIBE, email to [EMAIL PROTECTED]
with a subject of "unsubscribe". Trouble? Contact [EMAIL PROTECTED]
