From 4e74a7a4f1a109d84e0e8cc8c1e2a63b868b679e Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Fri, 21 May 2021 15:28:20 -0700 Subject: [PATCH] initwd: Error message for local paths escaping module packages Our module installer has a somewhat-informal idea of a "module package", which is some external thing we can go fetch in order to add one or more modules to the current configuration. Our documentation doesn't talk much about it because most users seem to have found the distinction between external and local modules pretty intuitive without us throwing a lot of funny terminology at them, but there are some situations where the distinction between a module and a module package are material to the end-user. One such situation is when using an absolute rather than relative filesystem path: we treat that as an external package in order to make the resulting working directory theoretically "portable" (although users can do various other things to defeat that), and so Terraform will copy the directory into .terraform/modules in the same way as it would download and extract a remote archive package or clone a git repository. A consequence of this, though, is that any relative paths called from inside a module loaded from an absolute path will fail if they try to traverse upward into the parent directory, because at runtime we're actually running from a copy of the directory that's been taking out of its original context. A similar sort of situation can occur in a truly remote module package if the author accidentally writes a "../" source path that traverses up out of the package root, and so this commit introduces a special error message for both situations that tries to be a bit clearer about there being a package boundary and use that to explain why installation failed. We would ideally have made escaping local references like that illegal in the first place, but sadly we did not and so when we rebuilt the module installer for Terraform v0.12 we ended up keeping the previous behavior of just trying it and letting it succeed if there happened to somehow be a matching directory at the given path, in order to remain compatible with situations that had worked by coincidence rather than intention. For that same reason, I've implemented this as a replacement error message we will return only if local module installation was going to fail anyway, and thus it only modifies the error message for some existing error situations rather than introducing new error situations. This also includes some light updates to the documentation to say a little more about how Terraform treats absolute paths, though aiming not to get too much into the weeds about module packages since it's something that most users can get away with never knowing. --- internal/initwd/module_install.go | 131 ++++++++++++++++++ internal/initwd/module_install_test.go | 69 +++++++++ .../child/package-escape-child.tf | 8 ++ .../grandchild/package-escape-grandchild.tf | 2 + .../package-escape.tf | 9 ++ .../package-prefix.tf | 15 ++ .../package/child/package-prefix-child.tf | 9 ++ .../grandchild/package-prefix-grandchild.tf | 2 + website/docs/language/modules/sources.html.md | 9 ++ 9 files changed, 254 insertions(+) create mode 100644 internal/initwd/testdata/load-module-package-escape/child/package-escape-child.tf create mode 100644 internal/initwd/testdata/load-module-package-escape/grandchild/package-escape-grandchild.tf create mode 100644 internal/initwd/testdata/load-module-package-escape/package-escape.tf create mode 100644 internal/initwd/testdata/load-module-package-prefix/package-prefix.tf create mode 100644 internal/initwd/testdata/load-module-package-prefix/package/child/package-prefix-child.tf create mode 100644 internal/initwd/testdata/load-module-package-prefix/package/grandchild/package-prefix-grandchild.tf diff --git a/internal/initwd/module_install.go b/internal/initwd/module_install.go index 5b9c7110c..d1f3fd9e8 100644 --- a/internal/initwd/module_install.go +++ b/internal/initwd/module_install.go @@ -4,6 +4,7 @@ import ( "fmt" "log" "os" + "path" "path/filepath" "strings" @@ -200,6 +201,7 @@ func (i *ModuleInstaller) installDescendentModules(rootMod *tfconfig.Module, roo case isLocalSourceAddr(req.SourceAddr): log.Printf("[TRACE] ModuleInstaller: %s has local path %q", key, req.SourceAddr) mod, mDiags := i.installLocalModule(req, key, manifest, hooks) + mDiags = maybeImproveLocalInstallError(req, mDiags) diags = append(diags, mDiags...) return mod, nil, diags @@ -588,3 +590,132 @@ func (i *ModuleInstaller) installGoGetterModule(req *earlyconfig.ModuleRequest, func (i *ModuleInstaller) packageInstallPath(modulePath addrs.Module) string { return filepath.Join(i.modsDir, strings.Join(modulePath, ".")) } + +// maybeImproveLocalInstallError is a helper function which can recognize +// some specific situations where it can return a more helpful error message +// and thus replace the given errors with those if so. +// +// If this function can't do anything about a particular situation then it +// will just return the given diags verbatim. +// +// This function's behavior is only reasonable for errors returned from the +// ModuleInstaller.installLocalModule function. +func maybeImproveLocalInstallError(req *earlyconfig.ModuleRequest, diags tfdiags.Diagnostics) tfdiags.Diagnostics { + if !diags.HasErrors() { + return diags + } + + // The main situation we're interested in detecting here is whether the + // current module or any of its ancestors use relative paths that reach + // outside of the "package" established by the nearest non-local ancestor. + // That's never really valid, but unfortunately we historically didn't + // have any explicit checking for it and so now for compatibility in + // situations where things just happened to "work" we treat this as an + // error only in situations where installation would've failed anyway, + // so we can give a better error about it than just a generic + // "directory not found" or whatever. + // + // Since it's never actually valid to relative out of the containing + // package, we just assume that any failed local package install which + // does so was caused by that, because to stop doing it should always + // improve the situation, even if it leads to another error describing + // a different problem. + + // To decide this we need to find the subset of our ancestors that + // belong to the same "package" as our request, along with the closest + // ancestor that defined that package, and then we can work forwards + // to see if any of the local paths "escaped" the package. + type Step struct { + Path addrs.Module + SourceAddr string + } + var packageDefiner Step + var localRefs []Step + localRefs = append(localRefs, Step{ + Path: req.Path, + SourceAddr: req.SourceAddr, + }) + current := req.Parent // an earlyconfig.Config where Children isn't populated yet + for { + if current == nil { + // We've reached the root module, in which case we aren't + // in an external "package" at all and so our special case + // can't apply. + return diags + } + if !isLocalSourceAddr(current.SourceAddr) { + // We've found the package definer, then! + packageDefiner = Step{ + Path: current.Path, + SourceAddr: current.SourceAddr, + } + break + } + + localRefs = append(localRefs, Step{ + Path: current.Path, + SourceAddr: current.SourceAddr, + }) + current = current.Parent + } + // Our localRefs list is reversed because we were traversing up the tree, + // so we'll flip it the other way and thus walk "downwards" through it. + for i, j := 0, len(localRefs)-1; i < j; i, j = i+1, j-1 { + localRefs[i], localRefs[j] = localRefs[j], localRefs[i] + } + + // Our method here is to start with a known base path prefix and + // then apply each of the local refs to it in sequence until one of + // them causes us to "lose" the prefix. If that happens, we've found + // an escape to report. This is not an exact science but good enough + // heuristic for choosing a better error message. + const prefix = "*/" // NOTE: this can find a false negative if the user chooses "*" as a directory name, but we consider that unlikely + packageAddr, startPath := splitAddrSubdir(packageDefiner.SourceAddr) + currentPath := path.Join(prefix, startPath) + for _, step := range localRefs { + // We're working in the simpler space of "path" rather than "filepath" + // for our heuristic here, so we need to slashify Windows-ish paths. + rel := filepath.ToSlash(step.SourceAddr) + + nextPath := path.Join(currentPath, rel) + if !strings.HasPrefix(nextPath, prefix) { // ESCAPED! + escapeeAddr := step.Path.String() + + var newDiags tfdiags.Diagnostics + + // First we'll copy over any non-error diagnostics from the source diags + for _, diag := range diags { + if diag.Severity() != tfdiags.Error { + newDiags = newDiags.Append(diag) + } + } + + // ...but we'll replace any errors with this more precise error. + var suggestion string + if strings.HasPrefix(packageAddr, "/") || filepath.VolumeName(packageAddr) != "" { + // It might be somewhat surprising that Terraform treats + // absolute filesystem paths as "external" even though it + // treats relative paths as local, so if it seems like that's + // what the user was doing then we'll add an additional note + // about it. + suggestion = "\n\nTerraform treats absolute filesystem paths as external modules which establish a new module package. To treat this directory as part of the same package as its caller, use a local path starting with either \"./\" or \"../\"." + } + newDiags = newDiags.Append(tfdiags.Sourceless( + tfdiags.Error, + "Local module path escapes module package", + fmt.Sprintf( + "The given source directory for %s would be outside of its containing package %q. Local source addresses starting with \"../\" must stay within the same package that the calling module belongs to.%s", + escapeeAddr, packageAddr, suggestion, + ), + )) + + return newDiags + } + + currentPath = nextPath + } + + // If we get down here then we have nothing useful to do, so we'll just + // echo back what we were given. + return diags +} diff --git a/internal/initwd/module_install_test.go b/internal/initwd/module_install_test.go index cab2a7007..8e0ec64dc 100644 --- a/internal/initwd/module_install_test.go +++ b/internal/initwd/module_install_test.go @@ -1,6 +1,7 @@ package initwd import ( + "bytes" "flag" "fmt" "io/ioutil" @@ -104,6 +105,74 @@ func TestModuleInstaller_error(t *testing.T) { } } +func TestModuleInstaller_packageEscapeError(t *testing.T) { + fixtureDir := filepath.Clean("testdata/load-module-package-escape") + dir, done := tempChdir(t, fixtureDir) + defer done() + + // For this particular test we need an absolute path in the root module + // that must actually resolve to our temporary directory in "dir", so + // we need to do a little rewriting. We replace the arbitrary placeholder + // %%BASE%% with the temporary directory path. + { + rootFilename := filepath.Join(dir, "package-escape.tf") + template, err := ioutil.ReadFile(rootFilename) + if err != nil { + t.Fatal(err) + } + final := bytes.ReplaceAll(template, []byte("%%BASE%%"), []byte(filepath.ToSlash(dir))) + err = ioutil.WriteFile(rootFilename, final, 0644) + if err != nil { + t.Fatal(err) + } + } + + hooks := &testInstallHooks{} + + modulesDir := filepath.Join(dir, ".terraform/modules") + inst := NewModuleInstaller(modulesDir, nil) + _, diags := inst.InstallModules(".", false, hooks) + + if !diags.HasErrors() { + t.Fatal("expected error") + } else { + assertDiagnosticSummary(t, diags, "Local module path escapes module package") + } +} + +func TestModuleInstaller_explicitPackageBoundary(t *testing.T) { + fixtureDir := filepath.Clean("testdata/load-module-package-prefix") + dir, done := tempChdir(t, fixtureDir) + defer done() + + // For this particular test we need an absolute path in the root module + // that must actually resolve to our temporary directory in "dir", so + // we need to do a little rewriting. We replace the arbitrary placeholder + // %%BASE%% with the temporary directory path. + { + rootFilename := filepath.Join(dir, "package-prefix.tf") + template, err := ioutil.ReadFile(rootFilename) + if err != nil { + t.Fatal(err) + } + final := bytes.ReplaceAll(template, []byte("%%BASE%%"), []byte(filepath.ToSlash(dir))) + err = ioutil.WriteFile(rootFilename, final, 0644) + if err != nil { + t.Fatal(err) + } + } + + hooks := &testInstallHooks{} + + modulesDir := filepath.Join(dir, ".terraform/modules") + inst := NewModuleInstaller(modulesDir, nil) + _, diags := inst.InstallModules(".", false, hooks) + + if diags.HasErrors() { + t.Fatalf("unexpected errors\n%s", diags.Err().Error()) + } +} + func TestModuleInstaller_invalid_version_constraint_error(t *testing.T) { fixtureDir := filepath.Clean("testdata/invalid-version-constraint") dir, done := tempChdir(t, fixtureDir) diff --git a/internal/initwd/testdata/load-module-package-escape/child/package-escape-child.tf b/internal/initwd/testdata/load-module-package-escape/child/package-escape-child.tf new file mode 100644 index 000000000..935a02de1 --- /dev/null +++ b/internal/initwd/testdata/load-module-package-escape/child/package-escape-child.tf @@ -0,0 +1,8 @@ +module "grandchild" { + # NOTE: This seems like it ought to work because there is indeed a + # ../grandchild directory, but our caller loaded us as an external + # module using an absolute path and so we're actually isolated from + # the parent directory in a separate "module package", and so we + # can't traverse out to find the grandchild module. + source = "../grandchild" +} diff --git a/internal/initwd/testdata/load-module-package-escape/grandchild/package-escape-grandchild.tf b/internal/initwd/testdata/load-module-package-escape/grandchild/package-escape-grandchild.tf new file mode 100644 index 000000000..734d0d638 --- /dev/null +++ b/internal/initwd/testdata/load-module-package-escape/grandchild/package-escape-grandchild.tf @@ -0,0 +1,2 @@ +# This is intentionally empty, just here to be referred to by the "child" +# module in ../child . diff --git a/internal/initwd/testdata/load-module-package-escape/package-escape.tf b/internal/initwd/testdata/load-module-package-escape/package-escape.tf new file mode 100644 index 000000000..7a665a4e6 --- /dev/null +++ b/internal/initwd/testdata/load-module-package-escape/package-escape.tf @@ -0,0 +1,9 @@ +module "child" { + # NOTE: For this test we need a working absolute path so that Terraform + # will see this a an "external" module and thus establish a separate + # package for it, but we won't know which temporary directory this + # will be in at runtime, so we'll rewrite this file inside the test + # code to replace %%BASE%% with the actual path. %%BASE%% is not normal + # Terraform syntax and won't work outside of this test. + source = "%%BASE%%/child" +} diff --git a/internal/initwd/testdata/load-module-package-prefix/package-prefix.tf b/internal/initwd/testdata/load-module-package-prefix/package-prefix.tf new file mode 100644 index 000000000..08d5ced60 --- /dev/null +++ b/internal/initwd/testdata/load-module-package-prefix/package-prefix.tf @@ -0,0 +1,15 @@ +module "child" { + # NOTE: For this test we need a working absolute path so that Terraform + # will see this a an "external" module and thus establish a separate + # package for it, but we won't know which temporary directory this + # will be in at runtime, so we'll rewrite this file inside the test + # code to replace %%BASE%% with the actual path. %%BASE%% is not normal + # Terraform syntax and won't work outside of this test. + # + # Note that we're intentionally using the special // delimiter to + # tell Terraform that it should treat the "package" directory as a + # whole as a module package, with all of its descendents "downloaded" + # (copied) together into ./.terraform/modules/child so that child + # can refer to ../grandchild successfully. + source = "%%BASE%%/package//child" +} diff --git a/internal/initwd/testdata/load-module-package-prefix/package/child/package-prefix-child.tf b/internal/initwd/testdata/load-module-package-prefix/package/child/package-prefix-child.tf new file mode 100644 index 000000000..84b67acdf --- /dev/null +++ b/internal/initwd/testdata/load-module-package-prefix/package/child/package-prefix-child.tf @@ -0,0 +1,9 @@ +module "grandchild" { + # NOTE: This only works because our caller told Terraform to treat + # the parent directory as a whole as a module package, and so + # the "./terraform/modules/child" directory should contain both + # "child" and "grandchild" sub directories that we can traverse between. + # This is the same as local paths between different directories inside + # a single git repository or distribution archive. + source = "../grandchild" +} diff --git a/internal/initwd/testdata/load-module-package-prefix/package/grandchild/package-prefix-grandchild.tf b/internal/initwd/testdata/load-module-package-prefix/package/grandchild/package-prefix-grandchild.tf new file mode 100644 index 000000000..734d0d638 --- /dev/null +++ b/internal/initwd/testdata/load-module-package-prefix/package/grandchild/package-prefix-grandchild.tf @@ -0,0 +1,2 @@ +# This is intentionally empty, just here to be referred to by the "child" +# module in ../child . diff --git a/website/docs/language/modules/sources.html.md b/website/docs/language/modules/sources.html.md index a4164571f..94da03653 100644 --- a/website/docs/language/modules/sources.html.md +++ b/website/docs/language/modules/sources.html.md @@ -74,6 +74,15 @@ that other sources are: the files are already present on local disk (possibly as a result of installing a parent module) and so can just be used directly. Their source code is automatically updated if the parent module is upgraded. +Note that Terraform does not consider an _absolute_ filesystem path (starting +with a slash, a drive letter, or similar) to be a local path. Instead, +Terraform will treat that in a similar way as a remote module and copy it into +the local module cache. An absolute path is a "package" in the sense described +in [Modules in Package Sub-directories](#modules-in-package-sub-directories). +We don't recommend using absolute filesystem paths to refer to Terraform +modules, because it will tend to couple your configuration to the filesystem +layout of a particular computer. + ## Terraform Registry A module registry is the native way of distributing Terraform modules for use