[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Re: preseed & debian-installer-utils updates for checksumming

On Sat, 2016-11-05 at 01:03 +0100, Philip Hands wrote:
> Hi,
> First, apologies for the timing on this, real-life has eaten all my time
> of late, and when I had some time to spare the InRelease split bug threw
> a spanner in the works.
> Here are some patches that I've been failing to get into d-i for a
> couple of releases -- it would be really nice to avoid making that 3
> releases.
>   https://anonscm.debian.org/cgit/d-i/preseed.git/log/?h=pu/preseed-fetch-checksum
> and
>   https://anonscm.debian.org/cgit/d-i/debian-installer-utils.git/log/?h=pu/preseed-fetch-checksum

I have some review comments.

I think fetch-url should delete the destination file if its checksum is
incorrect.  The destination might be used as a cache location and
assumed to be valid at a later stage.

I think the commit to preseed that relies on this ("allow preseed_fetch
to pass all it's params onto fetch-url") should also add a versioned
dependency on debian-installer-utils.  The commit message could also be
improved - this change doesn't *allow* passing those parameters; it
*does* pass them.  Finally, it adds a spurious blank line further down

A later commit ("distinguish checksum mismatches from retrieval
errors") does:

-				if ! preseed_fetch "$location" "$tmp" "$sum"; then
+				retval=0
+				preseed_fetch "$location" "$tmp" "$checksum" || retval=$?
+				case $retval in
+				    0) ;;
+				    2)
+					log "checksum error \"$location\", expected \"$checksum\""
+					error checksum_error "$location" "$checksum"
+					;;

It looks like all instances of "$checksum" should be "$sum".

Another commit ("update auto-install/defaultroot for stretch") bumps
the changelog version from 1.73 to 1.76.  Huh?

The last commit ("add a hook for looking up preseed checksums") does:

+if [ "$LOOKUP_CHECKSUM" = 1 ] ; then
+	# this is yet to be written, so leave room for dropping it into place during preseeding
+	if [ -f /bin/preseed_lookup_checksum ] ; then
+		# the "$@" parameter should be empty, but we might as well pass it in, so that we can pass it back out if need be
+		set -- $(/bin/preseed_lookup_checksum "$src" "$@")
+	fi

which seems to mean that when the -C option is used and
/bin/preseed_lookup_checksum  is missing then checksums are not
verified.  Shouldn't preseed_fetch fail in this case?

> P.S. I think this would be better if it were using SHA>=265, but
> we're
still on MD5SUM elsewhere in the code, so I guess that will need to
> wait.

I agree, MD5 should not be relied on for more than error detection.


Ben Hutchings
The world is coming to an end.	Please log off.

Attachment: signature.asc
Description: This is a digitally signed message part

Reply to: