providercache: Don't clobber mismatching cache entries

Prevously OpenTofu's provider installer would try to install a package even
if there was already a directory there which doesn't match the package
contents. That's effective in making us more likely to end up with a
working provider cache directory, but risks clobbering a package directory
that the operator intentionally modified for some reason.

We'll now require that if an existing directory (or symlink to one) is
present at the place where we'd need to put our cache entry then its
contents must already match what we're trying to install, thereby making
this a no-op. If the existing contents don't match then we'll fail with an
error to let the operator decide whether they need to keep something from
their modified directory before deleting it.

In earlier versions of OpenTofu, silently replacing an existing directory
was actually sometimes done intentionally to ensure that the cache would
definitely match the dependency lock file, but we no longer need to do that
because as of OpenTofu v1.12 the provider installer now exits early
(without downloading anything at all) if a matching package is already
present, so we never end up trying to replace a package that was already
present on disk unless it's the case we're now trying to catch as an error
here.

The handling of this is in PackageLocalArchive because the two
network-based location types (HTTP archive and OCI blob archive) work by
first fetching the archive to a temporary local location and then asking
the local archive location to finish the installation.

This is covered by e2etests rather than a unit test because successfully
hitting this error requires both the "providercache" and "getproviders"
packages to cooperate to let execution reach this late step without any
earlier code doing an early exit due to the directory already being
present. The e2etest runs through that entire codepath to make sure we
reach the error message we're expecting to reach.

Signed-off-by: Martin Atkins <mart@degeneration.co.uk>

internal/command/e2etest/providers_tamper_test.go

@@ -9,6 +9,7 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
+	"runtime"
 	"slices"
 	"strings"
 	"testing"
@@ -21,6 +22,11 @@ import (
 	"github.com/opentofu/opentofu/internal/getproviders"
 )
 
+// providerTamperingFixtureNullProviderVersion must contain a string
+// representation of the same version number used for "hashicorp/null" in the
+// "provider-tampering-base" fixture.
+const providerTamperingFixtureNullProviderVersion = "3.1.0"
+
 // TestProviderTampering tests various ways that the provider plugins in the
 // local cache directory might be modified after an initial "tofu init",
 // which other OpenTofu commands which use those plugins should catch and
@@ -49,9 +55,16 @@ func TestProviderTampering(t *testing.T) {
 	}
 
 	seedDir := tf.WorkDir()
-	const providerVersion = "3.1.0" // must match the version in the fixture config
-	pluginDir := filepath.Join(".terraform", "providers", "registry.opentofu.org", "hashicorp", "null", providerVersion, getproviders.CurrentPlatform.String())
-	pluginExe := filepath.Join(pluginDir, "terraform-provider-null_v"+providerVersion+"_x5")
+	pluginDir := filepath.Join(
+		".terraform",
+		"providers",
+		"registry.opentofu.org",
+		"hashicorp",
+		"null",
+		providerTamperingFixtureNullProviderVersion,
+		getproviders.CurrentPlatform.String(),
+	)
+	pluginExe := filepath.Join(pluginDir, "terraform-provider-null_v"+providerTamperingFixtureNullProviderVersion+"_x5")
 	if getproviders.CurrentPlatform.OS == "windows" {
 		pluginExe += ".exe" // ugh
 	}
@@ -271,6 +284,201 @@ func TestProviderTampering(t *testing.T) {
 	})
 }
 
+// TestProviderCacheJunkDirectory verifies our behavior for if there's already
+// a directory present where we'd want to create a provider cache entry, for
+// whatever reason: in that case, we want to fail with a clear error so that
+// the operator can decide how to fix the problem, rather than either totally
+// clobbering whatever was already there or merging the new package contents
+// into an preexisting directory.
+//
+// One way we could get into this case is if someone has intentionally modified
+// something in the provider cache directory to quickly test something and then
+// ran "tofu init" again. We prefer to fail in that case to avoid silently
+// clobbering whatever modification was made; operator should either delete
+// their modified directory or restore it to match the official package before
+// continuing.
+func TestProviderCacheJunkDirectory(t *testing.T) {
+	t.Parallel()
+
+	// This test reaches out to registry.opentofu.org to download the
+	// null provider, so it can only run if network access is allowed.
+	skipIfCannotAccessNetwork(t)
+
+	// We have a special case to allow installing into a precreated empty
+	// directory, since someone reading our documentation about directory
+	// layout might possibly try to create the empty directory manually first
+	// and there's no real harm in accepting that case because an empty
+	// directory is easy enough to recreate if that's what the operator really
+	// wanted, for some reason. (An empty provider package is never actually
+	// valid though, so that would be strange.)
+	for _, emptyDir := range []bool{true, false} {
+		t.Run(fmt.Sprintf("emptyDir=%#v", emptyDir), func(t *testing.T) {
+			t.Parallel()
+
+			// We reuse the "tampering" test fixture here even though this is a slightly
+			// different situation where the potential problem exists before init,
+			// rather than being introduced after init already ran.
+			fixturePath := filepath.Join("testdata", "provider-tampering-base")
+			tofu := e2e.NewBinary(t, tofuBin, fixturePath)
+
+			// This test starts with a strange mismatching directory at the location
+			// where the null provider package needs to be extracted, meaning that we
+			// won't be able to install the provider without clobbering it.
+			unexpectedDir := tofu.Path(
+				".terraform",
+				"providers",
+				addrs.DefaultProviderRegistryHost.String(),
+				"hashicorp",
+				"null",
+				providerTamperingFixtureNullProviderVersion,
+				getproviders.CurrentPlatform.String(),
+			)
+			err := os.MkdirAll(unexpectedDir, os.ModePerm)
+			if err != nil {
+				t.Fatalf("failed to make 'unexpected' directory: %s", err)
+			}
+			if !emptyDir {
+				// We'll make a file in the directory just to make it clear that this
+				// directory can't possibly match the expected content of the provider
+				// package, and so the provider installer definitely can't just try to
+				// use this directory as-is without clobbering it.
+				err = os.WriteFile(
+					filepath.Join(unexpectedDir, "extra.txt"),
+					[]byte("this file is not part of the hashicorp/null package"),
+					os.ModePerm,
+				)
+				if err != nil {
+					t.Fatalf("failed to make file in the 'unexpected' directory: %s", err)
+				}
+			}
+
+			// Now we run "tofu init" with the expectation that it should try to
+			// install "hashicorp/null" into the same location where we created
+			// the directory above. There's no dependency lock file to tell us
+			// that the existing directory might be enough to skip installation
+			// completely, so this should always attempt installation.
+			stdout, stderr, err := tofu.Run("init")
+			if emptyDir {
+				if err != nil {
+					t.Fatalf("unexpected failure: %s\n(installing into a preexisting empty directory should be allowed)\n%s\n%s", err, stderr, stdout)
+				}
+			} else {
+				if err == nil {
+					t.Fatalf("unexpected success; want error about conflicting cache entry\n%s\n%s", stderr, stdout)
+				}
+				if want := "does not match the content of the downloaded package"; !strings.Contains(stderr, want) {
+					t.Fatalf("stderr missing expected substring %q\n%s", want, stderr)
+				}
+			}
+		})
+	}
+}
+
+// TestProviderCacheJunkSymlink verifies our behavior for if there's already
+// a symlink present where we'd want to create a provider cache entry, for
+// whatever reason: in that case, we want to fail with a clear error so that
+// the operator can decide how to fix the problem, rather than either totally
+// clobbering their symlink or merging the package contents into whereever
+// the symlink points.
+//
+// One way we could get into this case is if someone has intentionally created
+// a symlink in their cache directory for provider development or testing
+// purposes, and then later ran "tofu init" again. We prefer to fail in that
+// case to avoid silently clobbering whatever modification was made; operator
+// should either delete their symlink or make sure it refers to a directory
+// that matches the expected package contents before continuing.
+func TestProviderCacheJunkSymlink(t *testing.T) {
+	t.Parallel()
+
+	// This test reaches out to registry.opentofu.org to download the
+	// null provider, so it can only run if network access is allowed.
+	skipIfCannotAccessNetwork(t)
+
+	// There is a special case to allow installing into a precreated empty
+	// directory which we test in [TestProviderCacheJunkDirectory] above,
+	// but that exception does not apply when what we find is a symlink _to_
+	// an empty directory: a symlink is only acceptable if it refers to a
+	// directory that was already correctly populated, because otherwise we
+	// might be writing into an existing empty directory somewhere else in
+	// the filesystem that is shared by other processes that expect it to stay
+	// empty.
+	for _, emptyDir := range []bool{true, false} {
+		t.Run(fmt.Sprintf("emptyDir=%#v", emptyDir), func(t *testing.T) {
+			t.Parallel()
+
+			// We reuse the "tampering" test fixture here even though this is a slightly
+			// different situation where the potential problem exists before init,
+			// rather than being introduced after init already ran.
+			fixturePath := filepath.Join("testdata", "provider-tampering-base")
+			tofu := e2e.NewBinary(t, tofuBin, fixturePath)
+
+			// This test starts with an unexpected symlink at the location where the
+			// null provider package needs to be extracted, meaning that we won't
+			// be able to install the provider without clobbering it.
+			targetDir := tofu.Path("symlink-target")
+			err := os.Mkdir(targetDir, os.ModePerm)
+			if err != nil {
+				t.Fatalf("failed to make symlink target directory: %s", err)
+			}
+			if !emptyDir {
+				// We'll make a file in the directory just to make it clear that this
+				// directory can't possibly match the expected content of the provider
+				// package, and so the provider installer definitely can't just try to
+				// use this directory as-is without clobbering the symlink.
+				err = os.WriteFile(
+					filepath.Join(targetDir, "extra.txt"),
+					[]byte("this file is not part of the hashicorp/null package"),
+					os.ModePerm,
+				)
+				if err != nil {
+					t.Fatalf("failed to make file in the symlink target directory: %s", err)
+				}
+			}
+
+			unexpectedSymlink := tofu.Path(
+				".terraform",
+				"providers",
+				addrs.DefaultProviderRegistryHost.String(),
+				"hashicorp",
+				"null",
+				providerTamperingFixtureNullProviderVersion,
+				getproviders.CurrentPlatform.String(),
+			)
+			symlinkParent := filepath.Dir(unexpectedSymlink)
+			err = os.MkdirAll(symlinkParent, os.ModePerm)
+			if err != nil {
+				t.Fatalf("failed to create parent directory of symlink: %s", err)
+			}
+			err = os.Symlink(targetDir, unexpectedSymlink)
+			if err != nil {
+				if runtime.GOOS == "windows" {
+					// By default Windows does not allow creation of symlinks, so
+					// we'll skip this test to avoid creating false-negative noise
+					// for anyone developing OpenTofu on Windows without their
+					// administrator having allowed symlink creation.
+					t.Skipf("can't create symlink on this Windows system: %s", err)
+				}
+				t.Fatalf("failed to make 'unexpected' symlink: %s", err)
+			}
+
+			// Now we run "tofu init" with the expectation that it should try to
+			// install "hashicorp/null" into the same location where we created
+			// the directory above. There's no dependency lock file to tell us
+			// that the existing directory might be enough to skip installation
+			// completely, so this should always attempt installation.
+			stdout, stderr, err := tofu.Run("init")
+			// Note that unlike [TestProviderCacheJunkDirectory] we expect this
+			// one to fail regardless of whether emptyDir is set.
+			if err == nil {
+				t.Fatalf("unexpected success; want error about conflicting cache entry\n%s\n%s", stderr, stdout)
+			}
+			if want := "does not match the content of the downloaded package"; !strings.Contains(stderr, want) {
+				t.Errorf("stderr missing expected substring %q\n%s", want, stderr)
+			}
+		})
+	}
+}
+
 // TestProviderLocksFromPredecessorProject is an end-to-end test of our
 // special treatment of lock files that were originally created by the
 // project that OpenTofu was forked from, and so refer to providers from

internal/getproviders/hash.go

@@ -371,6 +371,10 @@ func HashLegacyZipSHAFromSHA(sum [sha256.Size]byte) Hash {
 	return HashSchemeZip.New(fmt.Sprintf("%x", sum[:]))
 }
 
+// emptyPackageHashV1 is the representation of a completely empty package using
+// the V1 hashing scheme.
+const emptyPackageHashV1 = Hash("h1:47DEQpj8HBSa+/TImW+5JCeuQeRkm5NMpJWZG3hSuFU=")
+
 // PackageHashV1 computes a hash of the contents of the package at the given
 // location using hash algorithm 1. The resulting Hash is guaranteed to have
 // the scheme HashScheme1.

internal/getproviders/hash_test.go

@@ -324,3 +324,14 @@ func TestHashDispositionsMerge(t *testing.T) {
 		})
 	}
 }
+
+func TestEmptyPackageHashV1(t *testing.T) {
+	emptyDir := t.TempDir()
+	realHash, err := PackageHashV1(PackageLocalDir(emptyDir))
+	if err != nil {
+		t.Fatalf("failed to calculate hash of empty package: %s", err)
+	}
+	if realHash != emptyPackageHashV1 {
+		t.Errorf("emptyPackageHashV1 does not match freshly-calculated hash of empty package\nemptyPackageHashV1: %s\ncalculated result:  %s", emptyPackageHashV1, realHash)
+	}
+}

internal/getproviders/package_location_local_archive.go

@@ -69,22 +69,44 @@ func (p PackageLocalArchive) InstallProviderPackage(ctx context.Context, meta Pa
 	filename := meta.Location.String()
 	span.SetAttributes(traceattrs.FilePath(filename))
 
-	// NOTE: Packages are immutable, but we may want to skip overwriting the existing
-	// files in due to specific scenarios defined below.
-
-	if _, err := os.Stat(targetDir); err == nil {
-		// If the package might already be installed, we should try to skip overwriting the contents.
-		// When run with TF_PLUGIN_CACHE_DIR or similar, a given provider might already be executing
-		// and therefore locking the provider binary in the target directory (preventing the overwrite below)
-		//
-		// This does incur the overhead of two additional hash computations and could be
-		// skipped with smarter checks around re-use scenarios in the future.
+	// If there is already a package at the location we would've been installing
+	// to then that's okay if the content already matches what we would've
+	// installed, but we reject it otherwise so the operator can investigate.
+	if info, err := os.Lstat(targetDir); err == nil {
+		log.Printf("[TRACE] There's already a directory entry at %s, so we'll check if it matches our expectations", targetDir)
 
 		targetHash, targetErr := PackageHashV1(PackageLocalDir(targetDir))
-		fileHash, fileErr := PackageHashV1(meta.Location)
-
-		if targetHash == fileHash && fileErr == nil && targetErr == nil {
-			// Package is properly installed, bad or missing lock file will be caught elsewhere
+		// If the existing entry is an empty directory then we permit that just
+		// because sometimes a caller might want to create the target directory
+		// themselves before installing into it, such as if the target directory
+		// is a temporary directory created with [os.MkdirTemp]. Only a direct
+		// empty directory is allowed here, not a symlink to an empty directory.
+		isEmptyDir := info.IsDir() && targetHash == emptyPackageHashV1
+		if !isEmptyDir {
+			fileHash, fileErr := PackageHashV1(meta.Location)
+			var err error
+			if fileErr != nil {
+				err = fmt.Errorf("failed to calculate checksum for temporary copy of provider package at %s: %s", meta.Location.String(), fileErr)
+			} else if targetErr != nil {
+				err = fmt.Errorf("failed to calculate checksum for existing cached provider package at %s: %s", targetDir, targetErr)
+			} else if targetHash != fileHash {
+				// This means that there's already something at the cache location
+				// where we'd need to install to but the existing content doesn't
+				// match what we're trying to install. In this case we don't want
+				// to just clobber the existing directory because the operator
+				// might have modified it for a reason and want to keep something
+				// they changed in there, and so we'll report an error so they can
+				// investigate and delete this directory themselves when they are
+				// ready.
+				err = fmt.Errorf("existing cached package at %s does not match the content of the downloaded package; does it contain local modifications?", targetDir)
+			}
+			if err != nil {
+				tracing.SetSpanError(span, err)
+				return authResult, err
+			}
+			// If the stat succeeded and we've confirmed that the contents of
+			// targetDir match the package we were about to install anyway then
+			// we don't have any more work to do here.
 			log.Printf("[INFO] Skipping local installation of provider %s %s as the existing contents already match the new contents", meta.Provider, meta.Version)
 			return authResult, nil
 		}