fix(cmp): discover plugins relative to app path (argoproj#13940) (arg…

…oproj#13946) (argoproj#14084) * fix(cmp): discover plugins relative to app path (argoproj#13940) * securejoin * intuitive constant names * comments * add missing import --------- Signed-off-by: Michael Crenshaw <[email protected]> Signed-off-by: schakrad <[email protected]>
schakrad · Jul 24, 2023 · d118c2e · d118c2e
1 parent 5125c22
commit d118c2e
Show file tree

Hide file tree

Showing 12 changed files with 54 additions and 40 deletions.
diff --git a/cmpserver/plugin/plugin.go b/cmpserver/plugin/plugin.go
@@ -24,6 +24,7 @@ import (
 	"github.com/argoproj/argo-cd/v2/util/io/files"
 
 	"github.com/argoproj/gitops-engine/pkg/utils/kube"
+	"github.com/cyphar/filepath-securejoin"
 	"github.com/mattn/go-zglob"
 	log "github.com/sirupsen/logrus"
 )
@@ -182,7 +183,7 @@ func getTempDirMustCleanup(baseDir string) (workDir string, cleanup func(), err
 		if err := os.RemoveAll(workDir); err != nil {
 			log.WithFields(map[string]interface{}{
 				common.SecurityField:    common.SecurityHigh,
-				common.SecurityCWEField: 459,
+				common.SecurityCWEField: common.SecurityCWEIncompleteCleanup,
 			}).Errorf("Failed to clean up temp directory: %s", err)
 		}
 	}
@@ -302,7 +303,7 @@ func (s *Service) matchRepositoryGeneric(stream MatchRepositoryStream) error {
 		return fmt.Errorf("match repository error receiving stream: %w", err)
 	}
 
-	isSupported, isDiscoveryEnabled, err := s.matchRepository(bufferedCtx, workDir, metadata.GetEnv())
+	isSupported, isDiscoveryEnabled, err := s.matchRepository(bufferedCtx, workDir, metadata.GetEnv(), metadata.GetAppRelPath())
 	if err != nil {
 		return fmt.Errorf("match repository error: %w", err)
 	}
@@ -315,12 +316,20 @@ func (s *Service) matchRepositoryGeneric(stream MatchRepositoryStream) error {
 	return nil
 }
 
-func (s *Service) matchRepository(ctx context.Context, workdir string, envEntries []*apiclient.EnvEntry) (isSupported bool, isDiscoveryEnabled bool, err error) {
+func (s *Service) matchRepository(ctx context.Context, workdir string, envEntries []*apiclient.EnvEntry, appRelPath string) (isSupported bool, isDiscoveryEnabled bool, err error) {
 	config := s.initConstants.PluginConfig
 
+	appPath, err := securejoin.SecureJoin(workdir, appRelPath)
+	if err != nil {
+		log.WithFields(map[string]interface{}{
+			common.SecurityField:    common.SecurityHigh,
+			common.SecurityCWEField: common.SecurityCWEIncompleteCleanup,
+		}).Errorf("error joining workdir %q and appRelPath %q: %v", workdir, appRelPath, err)
+	}
+
 	if config.Spec.Discover.FileName != "" {
 		log.Debugf("config.Spec.Discover.FileName is provided")
-		pattern := filepath.Join(workdir, config.Spec.Discover.FileName)
+		pattern := filepath.Join(appPath, config.Spec.Discover.FileName)
 		matches, err := filepath.Glob(pattern)
 		if err != nil {
 			e := fmt.Errorf("error finding filename match for pattern %q: %w", pattern, err)
@@ -332,7 +341,7 @@ func (s *Service) matchRepository(ctx context.Context, workdir string, envEntrie
 
 	if config.Spec.Discover.Find.Glob != "" {
 		log.Debugf("config.Spec.Discover.Find.Glob is provided")
-		pattern := filepath.Join(workdir, config.Spec.Discover.Find.Glob)
+		pattern := filepath.Join(appPath, config.Spec.Discover.Find.Glob)
 		// filepath.Glob doesn't have '**' support hence selecting third-party lib
 		// https://github.com/golang/go/issues/11862
 		matches, err := zglob.Glob(pattern)
@@ -348,7 +357,7 @@ func (s *Service) matchRepository(ctx context.Context, workdir string, envEntrie
 	if len(config.Spec.Discover.Find.Command.Command) > 0 {
 		log.Debugf("Going to try runCommand.")
 		env := append(os.Environ(), environ(envEntries)...)
-		find, err := runCommand(ctx, config.Spec.Discover.Find.Command, workdir, env)
+		find, err := runCommand(ctx, config.Spec.Discover.Find.Command, appPath, env)
 		if err != nil {
 			return false, true, fmt.Errorf("error running find command: %w", err)
 		}

diff --git a/cmpserver/plugin/plugin_test.go b/cmpserver/plugin/plugin_test.go
@@ -100,7 +100,7 @@ func TestMatchRepository(t *testing.T) {
 		f := setup(t, withDiscover(d))
 
 		// when
-		match, discovery, err := f.service.matchRepository(context.Background(), f.path, f.env)
+		match, discovery, err := f.service.matchRepository(context.Background(), f.path, f.env, ".")
 
 		// then
 		assert.NoError(t, err)
@@ -115,7 +115,7 @@ func TestMatchRepository(t *testing.T) {
 		f := setup(t, withDiscover(d))
 
 		// when
-		match, discovery, err := f.service.matchRepository(context.Background(), f.path, f.env)
+		match, discovery, err := f.service.matchRepository(context.Background(), f.path, f.env, ".")
 
 		// then
 		assert.NoError(t, err)
@@ -130,7 +130,7 @@ func TestMatchRepository(t *testing.T) {
 		f := setup(t, withDiscover(d))
 
 		// when
-		_, _, err := f.service.matchRepository(context.Background(), f.path, f.env)
+		_, _, err := f.service.matchRepository(context.Background(), f.path, f.env, ".")
 
 		// then
 		assert.ErrorContains(t, err, "syntax error")
@@ -145,7 +145,7 @@ func TestMatchRepository(t *testing.T) {
 		f := setup(t, withDiscover(d))
 
 		// when
-		match, discovery, err := f.service.matchRepository(context.Background(), f.path, f.env)
+		match, discovery, err := f.service.matchRepository(context.Background(), f.path, f.env, ".")
 
 		// then
 		assert.NoError(t, err)
@@ -162,7 +162,7 @@ func TestMatchRepository(t *testing.T) {
 		f := setup(t, withDiscover(d))
 
 		// when
-		match, discovery, err := f.service.matchRepository(context.Background(), f.path, f.env)
+		match, discovery, err := f.service.matchRepository(context.Background(), f.path, f.env, ".")
 
 		// then
 		assert.NoError(t, err)
@@ -179,7 +179,7 @@ func TestMatchRepository(t *testing.T) {
 		f := setup(t, withDiscover(d))
 
 		// when
-		_, _, err := f.service.matchRepository(context.Background(), f.path, f.env)
+		_, _, err := f.service.matchRepository(context.Background(), f.path, f.env, ".")
 
 		// then
 		assert.ErrorContains(t, err, "error finding glob match for pattern")
@@ -196,7 +196,7 @@ func TestMatchRepository(t *testing.T) {
 		f := setup(t, withDiscover(d))
 
 		// when
-		match, discovery, err := f.service.matchRepository(context.Background(), f.path, f.env)
+		match, discovery, err := f.service.matchRepository(context.Background(), f.path, f.env, ".")
 
 		// then
 		assert.NoError(t, err)
@@ -215,7 +215,7 @@ func TestMatchRepository(t *testing.T) {
 		f := setup(t, withDiscover(d))
 
 		// when
-		match, discovery, err := f.service.matchRepository(context.Background(), f.path, f.env)
+		match, discovery, err := f.service.matchRepository(context.Background(), f.path, f.env, ".")
 		// then
 		assert.NoError(t, err)
 		assert.False(t, match)
@@ -233,7 +233,7 @@ func TestMatchRepository(t *testing.T) {
 		f := setup(t, withDiscover(d))
 
 		// when
-		match, discovery, err := f.service.matchRepository(context.Background(), f.path, f.env)
+		match, discovery, err := f.service.matchRepository(context.Background(), f.path, f.env, ".")
 
 		// then
 		assert.NoError(t, err)
@@ -253,7 +253,7 @@ func TestMatchRepository(t *testing.T) {
 		f := setup(t, withDiscover(d))
 
 		// when
-		match, discovery, err := f.service.matchRepository(context.Background(), f.path, f.env)
+		match, discovery, err := f.service.matchRepository(context.Background(), f.path, f.env, ".")
 
 		// then
 		assert.NoError(t, err)
@@ -272,7 +272,7 @@ func TestMatchRepository(t *testing.T) {
 		f := setup(t, withDiscover(d))
 
 		// when
-		match, discovery, err := f.service.matchRepository(context.Background(), f.path, f.env)
+		match, discovery, err := f.service.matchRepository(context.Background(), f.path, f.env, ".")
 
 		// then
 		assert.Error(t, err)
@@ -285,7 +285,7 @@ func TestMatchRepository(t *testing.T) {
 		f := setup(t, withDiscover(d))
 
 		// when
-		match, discovery, err := f.service.matchRepository(context.Background(), f.path, f.env)
+		match, discovery, err := f.service.matchRepository(context.Background(), f.path, f.env, ".")
 
 		// then
 		assert.NoError(t, err)

diff --git a/common/common.go b/common/common.go
@@ -309,13 +309,16 @@ const (
 
 // Security severity logging
 const (
-	SecurityField     = "security"
-	SecurityCWEField  = "CWE"
-	SecurityEmergency = 5 // Indicates unmistakably malicious events that should NEVER occur accidentally and indicates an active attack (i.e. brute forcing, DoS)
-	SecurityCritical  = 4 // Indicates any malicious or exploitable event that had a side effect (i.e. secrets being left behind on the filesystem)
-	SecurityHigh      = 3 // Indicates likely malicious events but one that had no side effects or was blocked (i.e. out of bounds symlinks in repos)
-	SecurityMedium    = 2 // Could indicate malicious events, but has a high likelihood of being user/system error (i.e. access denied)
-	SecurityLow       = 1 // Unexceptional entries (i.e. successful access logs)
+	SecurityField = "security"
+	// SecurityCWEField is the logs field for the CWE associated with a log line. CWE stands for Common Weakness Enumeration. See https://cwe.mitre.org/
+	SecurityCWEField                          = "CWE"
+	SecurityCWEIncompleteCleanup              = 459
+	SecurityCWEMissingReleaseOfFileDescriptor = 775
+	SecurityEmergency                         = 5 // Indicates unmistakably malicious events that should NEVER occur accidentally and indicates an active attack (i.e. brute forcing, DoS)
+	SecurityCritical                          = 4 // Indicates any malicious or exploitable event that had a side effect (i.e. secrets being left behind on the filesystem)
+	SecurityHigh                              = 3 // Indicates likely malicious events but one that had no side effects or was blocked (i.e. out of bounds symlinks in repos)
+	SecurityMedium                            = 2 // Could indicate malicious events, but has a high likelihood of being user/system error (i.e. access denied)
+	SecurityLow                               = 1 // Unexceptional entries (i.e. successful access logs)
 )
 
 // Common error messages

diff --git a/docs/operator-manual/config-management-plugins.md b/docs/operator-manual/config-management-plugins.md
@@ -67,8 +67,8 @@ spec:
   # Only one of fileName, find.glob, or find.command should be specified. If multiple are specified then only the 
   # first (in that order) is evaluated.
   discover:
-    # fileName is a glob pattern (https://pkg.go.dev/path/filepath#Glob) that is applied to the repository's root 
-    # directory (not the Application source directory). If there is a match, this plugin may be used for the repository.
+    # fileName is a glob pattern (https://pkg.go.dev/path/filepath#Glob) that is applied to the Application's source 
+    # directory. If there is a match, this plugin may be used for the Application.
     fileName: "./subdir/s*.yaml"
     find:
       # This does the same thing as fileName, but it supports double-start (nested directory) glob patterns.

diff --git a/go.mod b/go.mod
@@ -17,6 +17,7 @@ require (
 	github.com/bradleyfalzon/ghinstallation/v2 v2.1.0
 	github.com/casbin/casbin/v2 v2.60.0
 	github.com/chai2010/gettext-go v0.0.0-20170215093142-bf70f2a70fb1 // indirect
+	github.com/cyphar/filepath-securejoin v0.2.3
 	github.com/dustin/go-humanize v1.0.0
 	github.com/evanphx/json-patch v5.6.0+incompatible
 	github.com/fsnotify/fsnotify v1.6.0

diff --git a/go.sum b/go.sum
@@ -252,6 +252,7 @@ github.com/cpuguy83/go-md2man/v2 v2.0.2/go.mod h1:tgQtvFlXSQOSOSIRvRPT7W67SCa46t
 github.com/creack/pty v1.1.9/go.mod h1:oKZEueFk5CKHvIhNR5MUki03XCEU+Q6VDXinZuGJ33E=
 github.com/creack/pty v1.1.11 h1:07n33Z8lZxZ2qwegKbObQohDhXDQxiMMz1NOUGYlesw=
 github.com/creack/pty v1.1.11/go.mod h1:oKZEueFk5CKHvIhNR5MUki03XCEU+Q6VDXinZuGJ33E=
+github.com/cyphar/filepath-securejoin v0.2.3 h1:YX6ebbZCZP7VkM3scTTokDgBL2TY741X51MTk3ycuNI=
 github.com/cyphar/filepath-securejoin v0.2.3/go.mod h1:aPGpWjXOXUn2NCNjFvBE6aRxGGx79pTxQpKOJNYHHl4=
 github.com/davecgh/go-spew v0.0.0-20161028175848-04cdfd42973b/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
 github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=

diff --git a/test/e2e/testdata/cmp-fileName/plugin.yaml b/test/e2e/testdata/cmp-fileName/plugin.yaml
@@ -7,4 +7,4 @@ spec:
   generate:
     command: [sh, -c, 'echo "{\"kind\": \"ConfigMap\", \"apiVersion\": \"v1\", \"metadata\": { \"name\": \"$ARGOCD_APP_NAME\", \"namespace\": \"$ARGOCD_APP_NAMESPACE\", \"annotations\": {\"Foo\": \"$FOO\", \"KubeVersion\": \"$KUBE_VERSION\", \"KubeApiVersion\": \"$KUBE_API_VERSIONS\",\"Bar\": \"baz\"}}}"']
   discover:
-    fileName: "cmp-fileName/subdir/s*.yaml"
+    fileName: "subdir/s*.yaml"
diff --git a/util/app/discovery/discovery.go b/util/app/discovery/discovery.go
@@ -93,7 +93,7 @@ func DetectConfigManagementPlugin(ctx context.Context, appPath, repoPath, plugin
 	pluginSockFilePath := common.GetPluginSockFilePath()
 	log.WithFields(log.Fields{
 		common.SecurityField:    common.SecurityLow,
-		common.SecurityCWEField: 775,
+		common.SecurityCWEField: common.SecurityCWEMissingReleaseOfFileDescriptor,
 	}).Debugf("pluginSockFilePath is: %s", pluginSockFilePath)
 
 	if pluginName != "" {
@@ -160,7 +160,7 @@ func cmpSupports(ctx context.Context, pluginSockFilePath, appPath, repoPath, fil
 	if err != nil {
 		log.WithFields(log.Fields{
 			common.SecurityField:    common.SecurityMedium,
-			common.SecurityCWEField: 775,
+			common.SecurityCWEField: common.SecurityCWEMissingReleaseOfFileDescriptor,
 		}).Errorf("error dialing to cmp-server for plugin %s, %v", fileName, err)
 		return nil, nil, false
 	}
@@ -169,7 +169,7 @@ func cmpSupports(ctx context.Context, pluginSockFilePath, appPath, repoPath, fil
 	if err != nil {
 		log.WithFields(log.Fields{
 			common.SecurityField:    common.SecurityMedium,
-			common.SecurityCWEField: 775,
+			common.SecurityCWEField: common.SecurityCWEMissingReleaseOfFileDescriptor,
 		}).Errorf("repository %s is not the match because %v", repoPath, err)
 		io.Close(conn)
 		return nil, nil, false
@@ -182,7 +182,7 @@ func cmpSupports(ctx context.Context, pluginSockFilePath, appPath, repoPath, fil
 		}
 		log.WithFields(log.Fields{
 			common.SecurityField:    common.SecurityLow,
-			common.SecurityCWEField: 775,
+			common.SecurityCWEField: common.SecurityCWEMissingReleaseOfFileDescriptor,
 		}).Debugf("Reponse from socket file %s does not support %v", fileName, repoPath)
 		io.Close(conn)
 		return nil, nil, false

diff --git a/util/cert/cert.go b/util/cert/cert.go
@@ -135,7 +135,7 @@ func ParseTLSCertificatesFromPath(sourceFile string) ([]string, error) {
 		if err = fileHandle.Close(); err != nil {
 			log.WithFields(log.Fields{
 				common.SecurityField:    common.SecurityMedium,
-				common.SecurityCWEField: 775,
+				common.SecurityCWEField: common.SecurityCWEMissingReleaseOfFileDescriptor,
 			}).Errorf("error closing file %q: %v", fileHandle.Name(), err)
 		}
 	}()
@@ -199,7 +199,7 @@ func ParseSSHKnownHostsFromPath(sourceFile string) ([]string, error) {
 		if err = fileHandle.Close(); err != nil {
 			log.WithFields(log.Fields{
 				common.SecurityField:    common.SecurityMedium,
-				common.SecurityCWEField: 775,
+				common.SecurityCWEField: common.SecurityCWEMissingReleaseOfFileDescriptor,
 			}).Errorf("error closing file %q: %v", fileHandle.Name(), err)
 		}
 	}()

diff --git a/util/git/creds.go b/util/git/creds.go
@@ -279,7 +279,7 @@ func (c SSHCreds) Environ() (io.Closer, []string, error) {
 		if err = file.Close(); err != nil {
 			log.WithFields(log.Fields{
 				common.SecurityField:    common.SecurityMedium,
-				common.SecurityCWEField: 775,
+				common.SecurityCWEField: common.SecurityCWEMissingReleaseOfFileDescriptor,
 			}).Errorf("error closing file %q: %v", file.Name(), err)
 		}
 	}()

diff --git a/util/gpg/gpg.go b/util/gpg/gpg.go
@@ -170,7 +170,7 @@ func writeKeyToFile(keyData string) (string, error) {
 		if err != nil {
 			log.WithFields(log.Fields{
 				common.SecurityField:    common.SecurityMedium,
-				common.SecurityCWEField: 775,
+				common.SecurityCWEField: common.SecurityCWEMissingReleaseOfFileDescriptor,
 			}).Errorf("error closing file %q: %v", f.Name(), err)
 		}
 	}()
@@ -275,7 +275,7 @@ func InitializeGnuPG() error {
 		if err != nil {
 			log.WithFields(log.Fields{
 				common.SecurityField:    common.SecurityMedium,
-				common.SecurityCWEField: 775,
+				common.SecurityCWEField: common.SecurityCWEMissingReleaseOfFileDescriptor,
 			}).Errorf("error closing file %q: %v", f.Name(), err)
 		}
 	}()
@@ -302,7 +302,7 @@ func ImportPGPKeysFromString(keyData string) ([]*appsv1.GnuPGPublicKey, error) {
 		if err != nil {
 			log.WithFields(log.Fields{
 				common.SecurityField:    common.SecurityMedium,
-				common.SecurityCWEField: 775,
+				common.SecurityCWEField: common.SecurityCWEMissingReleaseOfFileDescriptor,
 			}).Errorf("error closing file %q: %v", f.Name(), err)
 		}
 	}()
@@ -430,7 +430,7 @@ func SetPGPTrustLevel(pgpKeys []*appsv1.GnuPGPublicKey, trustLevel string) error
 		if err != nil {
 			log.WithFields(log.Fields{
 				common.SecurityField:    common.SecurityMedium,
-				common.SecurityCWEField: 775,
+				common.SecurityCWEField: common.SecurityCWEMissingReleaseOfFileDescriptor,
 			}).Errorf("error closing file %q: %v", f.Name(), err)
 		}
 	}()

diff --git a/util/helm/cmd.go b/util/helm/cmd.go
@@ -176,7 +176,7 @@ func writeToTmp(data []byte) (string, argoio.Closer, error) {
 		if err = file.Close(); err != nil {
 			log.WithFields(log.Fields{
 				common.SecurityField:    common.SecurityMedium,
-				common.SecurityCWEField: 775,
+				common.SecurityCWEField: common.SecurityCWEMissingReleaseOfFileDescriptor,
 			}).Errorf("error closing file %q: %v", file.Name(), err)
 		}
 	}()