From 0bcfedbcaa9bbe30ee5ecade5b98e9ce3cc54c9b Mon Sep 17 00:00:00 2001 From: Nikita Pivkin Date: Wed, 5 Jun 2024 10:20:07 +0700 Subject: [PATCH] fix(misconf): fix caching of modules in subdirectories (#6814) --- .../terraform/parser/module_retrieval.go | 2 +- pkg/iac/scanners/terraform/parser/parser.go | 10 +- .../parser/parser_integration_test.go | 49 +++++--- .../terraform/parser/resolvers/cache.go | 31 +++-- .../resolvers/cache_integration_test.go | 114 ++++++++++++++++++ .../terraform/parser/resolvers/options.go | 1 + .../terraform/parser/resolvers/registry.go | 4 +- .../terraform/parser/resolvers/remote.go | 5 +- .../terraform/parser/resolvers/source.go | 36 ++++++ .../terraform/parser/resolvers/source_test.go | 44 +++++++ 10 files changed, 263 insertions(+), 33 deletions(-) create mode 100644 pkg/iac/scanners/terraform/parser/resolvers/cache_integration_test.go create mode 100644 pkg/iac/scanners/terraform/parser/resolvers/source.go create mode 100644 pkg/iac/scanners/terraform/parser/resolvers/source_test.go diff --git a/pkg/iac/scanners/terraform/parser/module_retrieval.go b/pkg/iac/scanners/terraform/parser/module_retrieval.go index 2ae6221afc73..165f64eef1cb 100644 --- a/pkg/iac/scanners/terraform/parser/module_retrieval.go +++ b/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, } diff --git a/pkg/iac/scanners/terraform/parser/parser.go b/pkg/iac/scanners/terraform/parser/parser.go index b7de6dd4ba08..049be0e02c10 100644 --- a/pkg/iac/scanners/terraform/parser/parser.go +++ b/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, diff --git a/pkg/iac/scanners/terraform/parser/parser_integration_test.go b/pkg/iac/scanners/terraform/parser/parser_integration_test.go index f73e9b85539c..5b301c0e41dd 100644 --- a/pkg/iac/scanners/terraform/parser/parser_integration_test.go +++ b/pkg/iac/scanners/terraform/parser/parser_integration_test.go @@ -13,7 +13,8 @@ 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" @@ -21,10 +22,9 @@ 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) @@ -34,7 +34,8 @@ 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" @@ -42,10 +43,9 @@ 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) @@ -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) diff --git a/pkg/iac/scanners/terraform/parser/resolvers/cache.go b/pkg/iac/scanners/terraform/parser/resolvers/cache.go index 6efc15f72dbb..e08c43ff3ba0 100644 --- a/pkg/iac/scanners/terraform/parser/resolvers/cache.go +++ b/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[:]) } diff --git a/pkg/iac/scanners/terraform/parser/resolvers/cache_integration_test.go b/pkg/iac/scanners/terraform/parser/resolvers/cache_integration_test.go new file mode 100644 index 000000000000..a7c6704b6708 --- /dev/null +++ b/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) +} diff --git a/pkg/iac/scanners/terraform/parser/resolvers/options.go b/pkg/iac/scanners/terraform/parser/resolvers/options.go index 1a676ae9e733..cdfde6b01bcc 100644 --- a/pkg/iac/scanners/terraform/parser/resolvers/options.go +++ b/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 { diff --git a/pkg/iac/scanners/terraform/parser/resolvers/registry.go b/pkg/iac/scanners/terraform/parser/resolvers/registry.go index 93d80fa0af86..bcdd4734974a 100644 --- a/pkg/iac/scanners/terraform/parser/resolvers/registry.go +++ b/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 diff --git a/pkg/iac/scanners/terraform/parser/resolvers/remote.go b/pkg/iac/scanners/terraform/parser/resolvers/remote.go index 4a6a26798a8a..df94c775da78 100644 --- a/pkg/iac/scanners/terraform/parser/resolvers/remote.go +++ b/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) } diff --git a/pkg/iac/scanners/terraform/parser/resolvers/source.go b/pkg/iac/scanners/terraform/parser/resolvers/source.go new file mode 100644 index 000000000000..d7637ffaf8a5 --- /dev/null +++ b/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 +} diff --git a/pkg/iac/scanners/terraform/parser/resolvers/source_test.go b/pkg/iac/scanners/terraform/parser/resolvers/source_test.go new file mode 100644 index 000000000000..3f57ab68b6d3 --- /dev/null +++ b/pkg/iac/scanners/terraform/parser/resolvers/source_test.go @@ -0,0 +1,44 @@ +package resolvers + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestRemoveSubdirFromSource(t *testing.T) { + + tests := []struct { + name string + source string + expected string + }{ + { + name: "address with scheme and query string", + source: "git::https://github.com/aquasecurity/terraform-modules.git//modules/ecs-service?ref=v0.1.0", + expected: "git::https://github.com/aquasecurity/terraform-modules.git?ref=v0.1.0", + }, + { + name: "address with scheme", + source: "git::https://github.com/aquasecurity/terraform-modules.git//modules/ecs-service", + expected: "git::https://github.com/aquasecurity/terraform-modules.git", + }, + { + name: "registry address", + source: "hashicorp/consul/aws//modules/consul-cluster", + expected: "hashicorp/consul/aws", + }, + { + name: "without subdir", + source: `hashicorp/consul/aws`, + expected: `hashicorp/consul/aws`, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + got := removeSubdirFromSource(test.source) + assert.Equal(t, test.expected, got) + }) + } +}