Add checksum package tests and log file contents in case of problems (#4244)

* Add dumping hash file contents in logs when validation fails

* add package tests for checksums

* add tests for TestVerifySHA512HashWithCleanup

* add skip for permission test on windows

* fixup! add skip for permission test on windows

Author: pchila

dev-tools/packaging/package_test.go

@@ -13,10 +13,13 @@ import (
 	"bufio"
 	"bytes"
 	"compress/gzip"
+	"crypto/sha512"
+	"encoding/hex"
 	"encoding/json"
 	"errors"
 	"flag"
 	"fmt"
+	"hash"
 	"io"
 	"os"
 	"path"
@@ -174,6 +177,8 @@ func checkTar(t *testing.T, file string) {
 		containingDir := strings.TrimSuffix(path.Base(file), ".tar.gz")
 		checkManifestFileContents(t, filepath.Join(tempExtractionPath, containingDir))
 	})
+
+	checkSha512PackageHash(t, file)
 }
 
 func checkZip(t *testing.T, file string) {
@@ -197,6 +202,8 @@ func checkZip(t *testing.T, file string) {
 		containingDir := strings.TrimSuffix(path.Base(file), ".zip")
 		checkManifestFileContents(t, filepath.Join(tempExtractionPath, containingDir))
 	})
+
+	checkSha512PackageHash(t, file)
 }
 
 func checkManifestFileContents(t *testing.T, extractedPackageDir string) {
@@ -939,3 +946,68 @@ func readDockerManifest(r io.Reader) (*dockerManifest, error) {
 
 	return manifests[0], nil
 }
+
+func checkSha512PackageHash(t *testing.T, packageFile string) {
+	t.Run("check hash file", func(t *testing.T) {
+		expectedHashFile := packageFile + ".sha512"
+		require.FileExists(t, expectedHashFile, "hash file for package %q should exist with name %q", packageFile, expectedHashFile)
+
+		// calculate SHA512 hash for the file
+		hashFile, err := os.Open(expectedHashFile)
+		require.NoError(t, err, "hash file should be readable")
+
+		checksumsMap := readHashFile(t, hashFile)
+
+		packageBaseName := filepath.Base(packageFile)
+		require.Containsf(t, checksumsMap, packageBaseName, "checksum file should contain an entry for %q", packageBaseName)
+
+		// compare checksum entry with actual package hash
+		checksum := calculateChecksum(t, packageFile, sha512.New())
+
+		assert.Equalf(t, checksum, checksumsMap[packageBaseName], "checksum for file %q does not match", packageFile)
+	})
+}
+
+func calculateChecksum(t *testing.T, file string, hasher hash.Hash) string {
+
+	input, err := os.Open(file)
+	require.NoErrorf(t, err, "error opening input file %q", file)
+
+	defer func(input *os.File) {
+		errClose := input.Close()
+		assert.NoErrorf(t, errClose, "error closing input file %q", file)
+	}(input)
+
+	_, err = io.Copy(hasher, input)
+	require.NoError(t, err, "error reading file to calculate hash")
+
+	return hex.EncodeToString(hasher.Sum(nil))
+}
+
+// readHashFile return a map of {filename, hash} reading a .sha512 file.
+// If any line has not exactly 2 tokens separated by white spaces, it will fail the test.
+// When it's done reading it will close the reader
+func readHashFile(t *testing.T, reader io.ReadCloser) map[string]string {
+
+	defer func(reader io.ReadCloser) {
+		err := reader.Close()
+		assert.NoError(t, err, "error closing hash file reader")
+	}(reader)
+
+	checksums := map[string]string{}
+	scanner := bufio.NewScanner(reader)
+	for scanner.Scan() {
+		line := scanner.Text()
+		parts := strings.Fields(line)
+		if len(parts) != 2 {
+			// Fail test because it's malformed.
+			assert.Failf(t, "malformed line %q in hash file", line)
+			continue
+		}
+		filename := strings.TrimLeft(parts[1], "*")
+		checksum := parts[0]
+		checksums[filename] = checksum
+	}
+
+	return checksums
+}

internal/pkg/agent/application/upgrade/artifact/download/verifier.go

@@ -101,6 +101,15 @@ func VerifySHA512HashWithCleanup(log infoWarnLogger, filename string) error {
 				log.Warnf("failed clean up after sha512 check: failed to remove %q: %v",
 					filename+".sha512", err)
 			}
+		} else if err != nil && !errors.Is(err, os.ErrNotExist) {
+			// it's not a simple hash mismatch, probably something is wrong with the hash file
+			hashFileName := getHashFileName(filename)
+			hashFileBytes, readErr := os.ReadFile(hashFileName)
+			if readErr != nil {
+				log.Warnf("error verifying the package using hash file %q, unable do read contents for logging: %v", getHashFileName(filename), readErr)
+			} else {
+				log.Warnf("error verifying the package using hash file %q, contents: %q", getHashFileName(filename), string(hashFileBytes))
+			}
 		}
 
 		return err
@@ -109,12 +118,20 @@ func VerifySHA512HashWithCleanup(log infoWarnLogger, filename string) error {
 	return nil
 }
 
+func getHashFileName(filename string) string {
+	const hashFileExt = ".sha512"
+	if strings.HasSuffix(filename, hashFileExt) {
+		return filename
+	}
+	return filename + hashFileExt
+}
+
 // VerifySHA512Hash checks that a sidecar file containing a sha512 checksum
 // exists and that the checksum in the sidecar file matches the checksum of
 // the file. It returns an error if validation fails.
 func VerifySHA512Hash(filename string) error {
 	// Read expected checksum.
-	expectedHash, err := readChecksumFile(filename+".sha512", filepath.Base(filename))
+	expectedHash, err := readChecksumFile(getHashFileName(filename), filepath.Base(filename))
 	if err != nil {
 		return fmt.Errorf("could not read checksum file: %w", err)
 	}

internal/pkg/agent/application/upgrade/artifact/download/verifier_test.go

@@ -10,9 +10,11 @@ import (
 	"encoding/hex"
 	"fmt"
 	"io"
+	"io/fs"
 	"net/http"
 	"os"
 	"path/filepath"
+	"runtime"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -192,6 +194,117 @@ func TestVerifySHA512HashWithCleanup_failure(t *testing.T) {
 	}
 }
 
+func TestVerifySHA512HashWithCleanup_BrokenHashFile(t *testing.T) {
+
+	const data = "" +
+		"Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. " +
+		"Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. " +
+		"Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. " +
+		"Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum."
+	// if you change data, the constant below should be updated
+	const correct_data_hash = "8ba760cac29cb2b2ce66858ead169174057aa1298ccd581514e6db6dee3285280ee6e3a54c9319071dc8165ff061d77783100d449c937ff1fb4cd1bb516a69b9"
+
+	const filename = "lorem_ipsum.txt"
+	const hashFileName = filename + ".sha512"
+
+	type skipFunc func(t *testing.T)
+
+	type testcase struct {
+		name            string
+		skip            skipFunc
+		hash            []byte
+		hashPermissions fs.FileMode
+		wantErr         assert.ErrorAssertionFunc
+		wantLogSnippets []string
+	}
+
+	testcases := []testcase{
+		{
+			name:            "happy path - correct hash and format",
+			hash:            []byte(correct_data_hash + "  " + filename),
+			hashPermissions: 0o640,
+			wantErr:         assert.NoError,
+		},
+		{
+			name:            "happy path - broken lines before correct hash and format",
+			hash:            []byte("this_is just_filler\n" + "some_more_filler\n" + correct_data_hash + "  " + filename),
+			hashPermissions: 0o640,
+			wantErr:         assert.NoError,
+		},
+		{
+			name:            "truncated hash line - no filename",
+			hash:            []byte(correct_data_hash),
+			hashPermissions: 0o640,
+			wantErr:         assert.Error,
+			wantLogSnippets: []string{`contents: "` + correct_data_hash + `"`},
+		},
+		{
+			name:            "truncated hash",
+			hash:            []byte(correct_data_hash[:8] + "  " + filename),
+			hashPermissions: 0o640,
+			wantErr: func(t assert.TestingT, err error, i ...interface{}) bool {
+				target := new(ChecksumMismatchError)
+				return assert.ErrorAs(t, err, &target, "mismatched hash has a specific error type", i)
+			},
+		},
+		{
+			name:            "empty hash file",
+			hash:            []byte{},
+			hashPermissions: 0o640,
+			wantErr:         assert.Error,
+			wantLogSnippets: []string{`contents: ""`},
+		},
+		{
+			name: "non-existing hash file",
+			hash: nil,
+			wantErr: func(t assert.TestingT, err error, i ...interface{}) bool {
+				return assert.ErrorIs(t, err, fs.ErrNotExist, i)
+			},
+		},
+		{
+			name: "unreadable hash file",
+			skip: func(t *testing.T) {
+				if runtime.GOOS == "windows" {
+					t.Skip("write-only permissions cannot be set on windows")
+				}
+			},
+			hash:            []byte(correct_data_hash + "  " + filename),
+			hashPermissions: 0o222,
+			wantErr: func(t assert.TestingT, err error, i ...interface{}) bool {
+				return assert.ErrorIs(t, err, fs.ErrPermission, i)
+			},
+			wantLogSnippets: []string{hashFileName + `", unable do read contents for logging:`},
+		},
+	}
+
+	for _, tt := range testcases {
+		t.Run(tt.name, func(t *testing.T) {
+			if tt.skip != nil {
+				tt.skip(t)
+			}
+
+			dir := t.TempDir()
+			dataFilePath := filepath.Join(dir, filename)
+			err := os.WriteFile(dataFilePath, []byte(data), 0o750)
+			require.NoError(t, err, "could not write sample data file")
+
+			if tt.hash != nil {
+				hashFilePath := filepath.Join(dir, hashFileName)
+				err = os.WriteFile(hashFilePath, tt.hash, tt.hashPermissions)
+				require.NoError(t, err, "could not write test hash file")
+			}
+
+			testLogger, obsLogs := logger.NewTesting(tt.name)
+			err = VerifySHA512HashWithCleanup(testLogger, dataFilePath)
+			tt.wantErr(t, err)
+			for _, log := range tt.wantLogSnippets {
+				filteredLogs := obsLogs.FilterMessageSnippet(log)
+				assert.NotEmptyf(t, filteredLogs, "there should be logs matching snippet %q", log)
+			}
+		})
+	}
+}
+
 type testlogger struct {
 	t *testing.T
 }