fix(misconf): fix caching of modules in subdirectories (#6814)

pkg/iac/scanners/terraform/parser/module_retrieval.go

@@ -13,8 +13,8 @@ type ModuleResolver interface {
 }
 
 var defaultResolvers = []ModuleResolver{
-	resolvers.Cache,
 	resolvers.Local,
+	resolvers.Cache,
 	resolvers.Remote,
 	resolvers.Registry,
 }

pkg/iac/scanners/terraform/parser/parser.go

@@ -232,17 +232,19 @@ func (p *Parser) Load(ctx context.Context) (*evaluator, error) {
 	}
 
 	modulesMetadata, metadataPath, err := loadModuleMetadata(p.moduleFS, p.projectRoot)
-	if err != nil {
+
+	if err != nil && !errors.Is(err, os.ErrNotExist) {
 		p.debug.Log("Error loading module metadata: %s.", err)
-	} else {
-		p.debug.Log("Loaded module metadata for %d module(s) from '%s'.", len(modulesMetadata.Modules), metadataPath)
+	} else if err == nil {
+		p.debug.Log("Loaded module metadata for %d module(s) from %q.", len(modulesMetadata.Modules), metadataPath)
 	}
 
 	workingDir, err := os.Getwd()
 	if err != nil {
 		return nil, err
 	}
-	p.debug.Log("Working directory for module evaluation is '%s'", workingDir)
+
+	p.debug.Log("Working directory for module evaluation is %q", workingDir)
 	return newEvaluator(
 		p.moduleFS,
 		p,

pkg/iac/scanners/terraform/parser/parser_integration_test.go

@@ -13,18 +13,18 @@ func Test_DefaultRegistry(t *testing.T) {
 	if testing.Short() {
 		t.Skip("skipping integration test in short mode")
 	}
-	fs := testutil.CreateFS(t, map[string]string{
+
+	fsys := testutil.CreateFS(t, map[string]string{
 		"code/test.tf": `
 module "registry" {
 	source = "terraform-aws-modules/vpc/aws"
 }
 `,
 	})
 
-	parser := New(fs, "", OptionStopOnHCLError(true), OptionWithSkipCachedModules(true))
-	if err := parser.ParseFS(context.TODO(), "code"); err != nil {
-		t.Fatal(err)
-	}
+	parser := New(fsys, "", OptionStopOnHCLError(true), OptionWithSkipCachedModules(true))
+	require.NoError(t, parser.ParseFS(context.TODO(), "code"))
+
 	modules, _, err := parser.EvaluateAll(context.TODO())
 	require.NoError(t, err)
 	require.Len(t, modules, 2)
@@ -34,18 +34,18 @@ func Test_SpecificRegistry(t *testing.T) {
 	if testing.Short() {
 		t.Skip("skipping integration test in short mode")
 	}
-	fs := testutil.CreateFS(t, map[string]string{
+
+	fsys := testutil.CreateFS(t, map[string]string{
 		"code/test.tf": `
 module "registry" {
 	source = "registry.terraform.io/terraform-aws-modules/vpc/aws"
 }
 `,
 	})
 
-	parser := New(fs, "", OptionStopOnHCLError(true), OptionWithSkipCachedModules(true))
-	if err := parser.ParseFS(context.TODO(), "code"); err != nil {
-		t.Fatal(err)
-	}
+	parser := New(fsys, "", OptionStopOnHCLError(true), OptionWithSkipCachedModules(true))
+	require.NoError(t, parser.ParseFS(context.TODO(), "code"))
+
 	modules, _, err := parser.EvaluateAll(context.TODO())
 	require.NoError(t, err)
 	require.Len(t, modules, 2)
@@ -55,7 +55,8 @@ func Test_ModuleWithPessimisticVersionConstraint(t *testing.T) {
 	if testing.Short() {
 		t.Skip("skipping integration test in short mode")
 	}
-	fs := testutil.CreateFS(t, map[string]string{
+
+	fsys := testutil.CreateFS(t, map[string]string{
 		"code/test.tf": `
 module "registry" {
 	source = "registry.terraform.io/terraform-aws-modules/s3-bucket/aws"
@@ -65,10 +66,30 @@ module "registry" {
 `,
 	})
 
-	parser := New(fs, "", OptionStopOnHCLError(true), OptionWithSkipCachedModules(true))
-	if err := parser.ParseFS(context.TODO(), "code"); err != nil {
-		t.Fatal(err)
+	parser := New(fsys, "", OptionStopOnHCLError(true), OptionWithSkipCachedModules(true))
+	require.NoError(t, parser.ParseFS(context.TODO(), "code"))
+
+	modules, _, err := parser.EvaluateAll(context.TODO())
+	require.NoError(t, err)
+	require.Len(t, modules, 2)
+}
+
+func Test_ModuleInSubdir(t *testing.T) {
+	if testing.Short() {
+		t.Skip("skipping integration test in short mode")
 	}
+
+	fsys := testutil.CreateFS(t, map[string]string{
+		"code/test.tf": `
+module "object" {
+	source = "git::https://github.com/terraform-aws-modules/terraform-aws-s3-bucket.git//modules/object?ref=v4.1.2"
+
+}`,
+	})
+
+	parser := New(fsys, "", OptionStopOnHCLError(true), OptionWithSkipCachedModules(true))
+	require.NoError(t, parser.ParseFS(context.TODO(), "code"))
+
 	modules, _, err := parser.EvaluateAll(context.TODO())
 	require.NoError(t, err)
 	require.Len(t, modules, 2)

pkg/iac/scanners/terraform/parser/resolvers/cache.go

@@ -2,7 +2,8 @@ package resolvers
 
 import (
 	"context"
-	"crypto/md5" // nolint
+	"crypto/md5" // #nosec
+	"encoding/hex"
 	"fmt"
 	"io/fs"
 	"os"
@@ -15,16 +16,21 @@ var Cache = &cacheResolver{}
 
 const tempDirName = ".aqua"
 
-func locateCacheFS() (fs.FS, error) {
-	dir, err := locateCacheDir()
+var defaultCacheDir = filepath.Join(os.TempDir(), tempDirName, "cache")
+
+func locateCacheFS(cacheDir string) (fs.FS, error) {
+	dir, err := locateCacheDir(cacheDir)
 	if err != nil {
 		return nil, err
 	}
 	return os.DirFS(dir), nil
 }
 
-func locateCacheDir() (string, error) {
-	cacheDir := filepath.Join(os.TempDir(), tempDirName, "cache")
+func locateCacheDir(cacheDir string) (string, error) {
+	if cacheDir == "" {
+		cacheDir = defaultCacheDir
+	}
+
 	if err := os.MkdirAll(cacheDir, 0o750); err != nil {
 		return "", err
 	}
@@ -39,24 +45,29 @@ func (r *cacheResolver) Resolve(_ context.Context, _ fs.FS, opt Options) (filesy
 		opt.Debug("Cache is disabled.")
 		return nil, "", "", false, nil
 	}
-	cacheFS, err := locateCacheFS()
+	cacheFS, err := locateCacheFS(opt.CacheDir)
 	if err != nil {
 		opt.Debug("No cache filesystem is available on this machine.")
 		return nil, "", "", false, nil
 	}
-	key := cacheKey(opt.Source, opt.Version, opt.RelativePath)
+
+	src := removeSubdirFromSource(opt.Source)
+	key := cacheKey(src, opt.Version)
+
 	opt.Debug("Trying to resolve: %s", key)
 	if info, err := fs.Stat(cacheFS, filepath.ToSlash(key)); err == nil && info.IsDir() {
 		opt.Debug("Module '%s' resolving via cache...", opt.Name)
-		cacheDir, err := locateCacheDir()
+		cacheDir, err := locateCacheDir(opt.CacheDir)
 		if err != nil {
 			return nil, "", "", true, err
 		}
+
 		return os.DirFS(filepath.Join(cacheDir, key)), opt.OriginalSource, ".", true, nil
 	}
 	return nil, "", "", false, nil
 }
 
-func cacheKey(source, version, relativePath string) string {
-	return fmt.Sprintf("%x", md5.Sum([]byte(fmt.Sprintf("%s:%s:%s", source, version, relativePath)))) // nolint
+func cacheKey(source, version string) string {
+	hash := md5.Sum([]byte(source + ":" + version)) // #nosec
+	return hex.EncodeToString(hash[:])
 }

pkg/iac/scanners/terraform/parser/resolvers/cache_integration_test.go

@@ -0,0 +1,114 @@
+package resolvers_test
+
+import (
+	"context"
+	"io/fs"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	"github.com/aquasecurity/trivy/pkg/iac/scanners/terraform/parser/resolvers"
+)
+
+type moduleResolver interface {
+	Resolve(context.Context, fs.FS, resolvers.Options) (fs.FS, string, string, bool, error)
+}
+
+func TestResolveModuleFromCache(t *testing.T) {
+	if testing.Short() {
+		t.Skip("skipping integration test in short mode")
+	}
+
+	tests := []struct {
+		name          string
+		opts          resolvers.Options
+		firstResolver moduleResolver
+	}{
+		{
+			name: "registry",
+			opts: resolvers.Options{
+				Name:    "bucket",
+				Source:  "terraform-aws-modules/s3-bucket/aws",
+				Version: "4.1.2",
+			},
+			firstResolver: resolvers.Registry,
+		},
+		{
+			name: "registry with subdir",
+			opts: resolvers.Options{
+				Name:    "object",
+				Source:  "terraform-aws-modules/s3-bucket/aws//modules/object",
+				Version: "4.1.2",
+			},
+			firstResolver: resolvers.Registry,
+		},
+		{
+			name: "remote",
+			opts: resolvers.Options{
+				Name:   "bucket",
+				Source: "git::https://github.com/terraform-aws-modules/terraform-aws-s3-bucket.git?ref=v4.1.2",
+			},
+			firstResolver: resolvers.Remote,
+		},
+		{
+			name: "remote with subdir",
+			opts: resolvers.Options{
+				Name:   "object",
+				Source: "git::https://github.com/terraform-aws-modules/terraform-aws-s3-bucket.git//modules/object?ref=v4.1.2",
+			},
+			firstResolver: resolvers.Remote,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+
+			tt.opts.AllowDownloads = true
+			tt.opts.OriginalSource = tt.opts.Source
+			tt.opts.OriginalVersion = tt.opts.Version
+			tt.opts.CacheDir = t.TempDir()
+
+			fsys, _, _, applies, err := tt.firstResolver.Resolve(context.Background(), nil, tt.opts)
+			require.NoError(t, err)
+			assert.True(t, applies)
+
+			_, err = fs.Stat(fsys, "main.tf")
+			require.NoError(t, err)
+
+			_, _, _, applies, err = resolvers.Cache.Resolve(context.Background(), fsys, tt.opts)
+			require.NoError(t, err)
+			assert.True(t, applies)
+		})
+	}
+}
+
+func TestResolveModuleFromCacheWithDifferentSubdir(t *testing.T) {
+	if testing.Short() {
+		t.Skip("skipping integration test in short mode")
+	}
+
+	cacheDir := t.TempDir()
+
+	fsys, _, _, applies, err := resolvers.Remote.Resolve(context.Background(), nil, resolvers.Options{
+		Name:           "object",
+		Source:         "git::https://github.com/terraform-aws-modules/terraform-aws-s3-bucket.git//modules/object?ref=v4.1.2",
+		OriginalSource: "git::https://github.com/terraform-aws-modules/terraform-aws-s3-bucket.git//modules/object?ref=v4.1.2",
+		AllowDownloads: true,
+		CacheDir:       cacheDir,
+	})
+	require.NoError(t, err)
+	assert.True(t, applies)
+
+	_, err = fs.Stat(fsys, "main.tf")
+	require.NoError(t, err)
+
+	_, _, _, applies, err = resolvers.Cache.Resolve(context.Background(), nil, resolvers.Options{
+		Name:           "notification",
+		Source:         "git::https://github.com/terraform-aws-modules/terraform-aws-s3-bucket.git//modules/notification?ref=v4.1.2",
+		OriginalSource: "git::https://github.com/terraform-aws-modules/terraform-aws-s3-bucket.git//modules/notification?ref=v4.1.2",
+		CacheDir:       cacheDir,
+	})
+	require.NoError(t, err)
+	assert.True(t, applies)
+}

pkg/iac/scanners/terraform/parser/resolvers/options.go

@@ -12,6 +12,7 @@ type Options struct {
 	AllowDownloads                                                                 bool
 	SkipCache                                                                      bool
 	RelativePath                                                                   string
+	CacheDir                                                                       string
 }
 
 func (o *Options) hasPrefix(prefixes ...string) bool {

pkg/iac/scanners/terraform/parser/resolvers/registry.go

@@ -45,7 +45,7 @@ func (r *registryResolver) Resolve(ctx context.Context, target fs.FS, opt Option
 	}
 
 	inputVersion := opt.Version
-	source, relativePath, _ := strings.Cut(opt.Source, "//")
+	source := removeSubdirFromSource(opt.Source)
 	parts := strings.Split(source, "/")
 	if len(parts) < 3 || len(parts) > 4 {
 		return
@@ -146,7 +146,7 @@ func (r *registryResolver) Resolve(ctx context.Context, target fs.FS, opt Option
 	}
 
 	opt.Debug("Module '%s' resolved via registry to new source: '%s'", opt.Name, opt.Source)
-	opt.RelativePath = relativePath
+
 	filesystem, prefix, downloadPath, _, err = Remote.Resolve(ctx, target, opt)
 	if err != nil {
 		return nil, "", "", true, err

pkg/iac/scanners/terraform/parser/resolvers/remote.go

@@ -38,10 +38,11 @@ func (r *remoteResolver) Resolve(ctx context.Context, _ fs.FS, opt Options) (fil
 		return nil, "", "", false, nil
 	}
 
-	key := cacheKey(opt.OriginalSource, opt.OriginalVersion, opt.RelativePath)
+	src := removeSubdirFromSource(opt.OriginalSource)
+	key := cacheKey(src, opt.OriginalVersion)
 	opt.Debug("Storing with cache key %s", key)
 
-	baseCacheDir, err := locateCacheDir()
+	baseCacheDir, err := locateCacheDir(opt.CacheDir)
 	if err != nil {
 		return nil, "", "", true, fmt.Errorf("failed to locate cache directory: %w", err)
 	}

pkg/iac/scanners/terraform/parser/resolvers/source.go

@@ -0,0 +1,36 @@
+package resolvers
+
+import "strings"
+
+func removeSubdirFromSource(src string) string {
+	stop := len(src)
+	if idx := strings.Index(src, "?"); idx > -1 {
+		stop = idx
+	}
+
+	// Calculate an offset to avoid accidentally marking the scheme
+	// as the dir.
+	var offset int
+	if idx := strings.Index(src[:stop], "://"); idx > -1 {
+		offset = idx + 3
+	}
+
+	// First see if we even have an explicit subdir
+	idx := strings.Index(src[offset:stop], "//")
+	if idx == -1 {
+		return src
+	}
+
+	idx += offset
+	subdir := src[idx+2:]
+	src = src[:idx]
+
+	// Next, check if we have query parameters and push them onto the
+	// URL.
+	if idx = strings.Index(subdir, "?"); idx > -1 {
+		query := subdir[idx:]
+		src += query
+	}
+
+	return src
+}