From: Thomas Lamprecht <t.lamprecht@proxmox.com>
Date: Tue, 15 Jun 2021 12:11:07 +0000 (+0200)
Subject: tools: download_file_from_url: improve UX and avoid cyclic dependencies
X-Git-Url: https://git.proxmox.com/?p=pve-common.git;a=commitdiff_plain;h=60fb1c2628aec71222a93d211053409b58bb1f86

tools: download_file_from_url: improve UX and avoid cyclic dependencies

plus some refactoring

* drop worker, cannot be done here (RPCEnv is in pve-access-control)
* actually output the wrong "got" hash on mismatch
* die on existing file with mismatched
* drop double array for passing cmd
* drop `/usr/bin` prefix
* adapt rename error message
* add error handling for unlinking the temp. file

Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
---

diff --git a/src/PVE/Tools.pm b/src/PVE/Tools.pm
index 7ba02bc..a5efd60 100644
--- a/src/PVE/Tools.pm
+++ b/src/PVE/Tools.pm
@@ -1829,52 +1829,43 @@ sub safe_compare {
 }
 
 
-# opts
-#  -> hash_required
-#       if 1, at least one checksum has to be specified otherwise an error will be thrown
-#  -> http_proxy
-#  -> https_proxy
-#  -> verify_certificates
-#  -> sha(1|224|256|384|512)sum
-#  -> md5sum
+# opts is a hash ref with the following known properties
+#  hash_required - if 1, at least one checksum has to be specified otherwise an error will be thrown
+#  http_proxy
+#  https_proxy
+#  verify_certificates - if 0 (false) we tell wget to ignore untrusted TLS certs. Default to true
+#  md5sum|sha(1|224|256|384|512)sum - the respective expected checksum string
 sub download_file_from_url {
     my ($dest, $url, $opts) = @_;
 
-    my $tmpdest = "$dest.tmp.$$";
-
-    my $algorithm;
-    my $expected;
-
+    my ($checksum_algorithm, $checksum_expected);
     for ('sha512', 'sha384', 'sha256', 'sha224', 'sha1', 'md5') {
 	if (defined($opts->{"${_}sum"})) {
-	    $algorithm = $_;
-	    $expected = $opts->{"${_}sum"};
+	    $checksum_algorithm = $_;
+	    $checksum_expected = $opts->{"${_}sum"};
 	    last;
 	}
     }
+    die "checksum required but not specified\n" if ($opts->{hash_required} && !$checksum_algorithm);
 
-    die "checksum required but not specified\n" if ($opts->{hash_required} && !$algorithm);
-
-    my $worker = sub  {
-	my $upid = shift;
+    print "downloading $url to $dest\n";
 
-	print "downloading $url to $dest\n";
+    my $tmpdest = "$dest.tmp.$$";
+    eval {
+	if (-f $dest && $checksum_algorithm) {
+	    print "calculating checksum of existing file...";
+	    my $checksum_got = get_file_hash($checksum_algorithm, $dest);
 
-	eval {
-	    if (-f $dest && $algorithm) {
-		print "calculating checksum of existing file...\n";
-		my $correct = check_file_hash($algorithm, $expected, $dest);
-
-		if ($correct) {
-		    print "file already exists, no need to download\n";
-		    return;
-		} else {
-		    print "mismatch, downloading\n";
-		}
+	    if (lc($checksum_got) eq lc($checksum_expected)) {
+		print "OK, got correct file already, no need to download\n";
+		return;
+	    } else {
+		# we could re-download, but may not be safe so just abort for now..
+		die "mismatch (got '$checksum_got' != expect '$checksum_expected'), aborting\n";
 	    }
+	}
 
-	    my @cmd = ('/usr/bin/wget', '--progress=dot:mega', '-O', $tmpdest, $url);
-
+	{ # limit the scope of the ENV change
 	    local %ENV;
 	    if ($opts->{http_proxy}) {
 		$ENV{http_proxy} = $opts->{http_proxy};
@@ -1883,55 +1874,39 @@ sub download_file_from_url {
 		$ENV{https_proxy} = $opts->{https_proxy};
 	    }
 
-	    my $verify = $opts->{verify_certificates} // 1;
-	    if (!$verify) {
-		push @cmd, '--no-check-certificate';
-	    }
+	    my $cmd = ['wget', '--progress=dot:giga', '-O', $tmpdest, $url];
 
-	    if (run_command([[@cmd]]) != 0) {
-		die "download failed: $!\n";
+	    if (!($opts->{verify_certificates} // 1)) { # default to true
+		push @$cmd, '--no-check-certificate';
 	    }
 
-	    if ($algorithm) {
-		print "calculating checksum...\n";
+	    run_command($cmd, errmsg => "download failed");
+	}
+
+	if ($checksum_algorithm) {
+	    print "calculating checksum...";
 
-		my $correct = check_file_hash($algorithm, $expected, $tmpdest);
+	    my $checksum_got = get_file_hash($checksum_algorithm, $tmpdest);
 
-		if ($correct) {
-		    print "checksum verified\n";
-		} else {
-		    die "checksum mismatch\n";
-		}
+	    if (lc($checksum_got) eq lc($checksum_expected)) {
+		print "OK, checksum verified\n";
 	    } else {
-		print "no checksum for verification specified\n";
-	    }
-
-	    if (!rename($tmpdest, $dest)) {
-		die "unable to save file: $!\n";
+		die "ERRROR, checksum mismatch: got '$checksum_got' != expect '$checksum_expected'\n";
 	    }
-	};
-	my $err = $@;
-
-	unlink $tmpdest;
-
-	if ($err) {
-	    print "\n";
-	    die $err;
 	}
 
-	print "download finished\n";
+	rename($tmpdest, $dest) or die "unable to rename temporary file: $!\n";
     };
+    if (my $err = $@) {
+	unlink $tmpdest or warn "could not cleanup temporary file: $!";
+	die $err;
+    }
 
-    my $rpcenv = PVE::RPCEnvironment::get();
-    my $user = $rpcenv->get_user();
-
-    (my $filename = $dest) =~ s!.*/([^/]*)$!$1!;
-
-    return $rpcenv->fork_worker('download', $filename, $user, $worker);
+    print "download of '$url' to '$dest' finished\n";
 }
 
-sub check_file_hash {
-    my ($algorithm, $expected, $filename) = @_;
+sub get_file_hash {
+    my ($algorithm, $filename) = @_;
 
     my $algorithm_map = {
 	'md5' => sub { Digest::MD5->new },
@@ -1949,7 +1924,7 @@ sub check_file_hash {
 
     my $digest = $digester->addfile($fh)->hexdigest;
 
-    return lc($digest) eq lc($expected);
+    return lc($digest);
 }
 
 1;