From 3d27e83bf5db10895bc7c87ecff93f13cef5d067 Mon Sep 17 00:00:00 2001
From: mmetc <92726601+mmetc@users.noreply.github.com>
Date: Tue, 20 Aug 2024 17:36:07 +0200
Subject: [PATCH] pkg/cwhub: improve support for k8s config maps with custom
 items (#3154)

* pkg/cwhub: improve support for k8s config maps as custom items

 - allow links to links
 - ignore hidden ..data directories, but allow links to their content

* allow any number of subdirectories in /etc/crowdsec/{hubtype}

* item name as subdir/file.yaml

* improve func test

* lint
---
 cmd/crowdsec-cli/capi.go       |   7 +-
 pkg/cwhub/errors.go            |   6 +-
 pkg/cwhub/relativepath.go      |  28 +++++
 pkg/cwhub/relativepath_test.go |  72 ++++++++++++
 pkg/cwhub/sync.go              | 198 ++++++++++++++++++++++-----------
 pkg/metabase/metabase.go       |   4 +-
 test/bats/20_hub_items.bats    |  92 ++++++++++++++-
 7 files changed, 322 insertions(+), 85 deletions(-)
 create mode 100644 pkg/cwhub/relativepath.go
 create mode 100644 pkg/cwhub/relativepath_test.go

diff --git a/cmd/crowdsec-cli/capi.go b/cmd/crowdsec-cli/capi.go
index 589b36ada..ac921ea54 100644
--- a/cmd/crowdsec-cli/capi.go
+++ b/cmd/crowdsec-cli/capi.go
@@ -21,11 +21,6 @@ import (
 	"github.com/crowdsecurity/crowdsec/pkg/types"
 )
 
-const (
-	CAPIBaseURL   = "https://api.crowdsec.net/"
-	CAPIURLPrefix = "v3"
-)
-
 type cliCapi struct {
 	cfg configGetter
 }
@@ -78,7 +73,7 @@ func (cli *cliCapi) register(capiUserPrefix string, outputFile string) error {
 		Password:      password,
 		UserAgent:     cwversion.UserAgent(),
 		URL:           apiurl,
-		VersionPrefix: CAPIURLPrefix,
+		VersionPrefix: "v3",
 	}, nil)
 	if err != nil {
 		return fmt.Errorf("api client register ('%s'): %w", types.CAPIBaseURL, err)
diff --git a/pkg/cwhub/errors.go b/pkg/cwhub/errors.go
index f1e779b54..b0be444fc 100644
--- a/pkg/cwhub/errors.go
+++ b/pkg/cwhub/errors.go
@@ -5,10 +5,8 @@ import (
 	"fmt"
 )
 
-var (
-	// ErrNilRemoteHub is returned when trying to download with a local-only configuration.
-	ErrNilRemoteHub = errors.New("remote hub configuration is not provided. Please report this issue to the developers")
-)
+// ErrNilRemoteHub is returned when trying to download with a local-only configuration.
+var ErrNilRemoteHub = errors.New("remote hub configuration is not provided. Please report this issue to the developers")
 
 // IndexNotFoundError is returned when the remote hub index is not found.
 type IndexNotFoundError struct {
diff --git a/pkg/cwhub/relativepath.go b/pkg/cwhub/relativepath.go
new file mode 100644
index 000000000..bcd4c5768
--- /dev/null
+++ b/pkg/cwhub/relativepath.go
@@ -0,0 +1,28 @@
+package cwhub
+
+import (
+	"path/filepath"
+	"strings"
+)
+
+// relativePathComponents returns the list of path components after baseDir.
+// If path is not inside baseDir, it returns an empty slice.
+func relativePathComponents(path string, baseDir string) []string {
+	absPath, err := filepath.Abs(path)
+	if err != nil {
+		return []string{}
+	}
+
+	absBaseDir, err := filepath.Abs(baseDir)
+	if err != nil {
+		return []string{}
+	}
+
+	// is path inside baseDir?
+	relPath, err := filepath.Rel(absBaseDir, absPath)
+	if err != nil || strings.HasPrefix(relPath, "..") || relPath == "." {
+		return []string{}
+	}
+
+	return strings.Split(relPath, string(filepath.Separator))
+}
diff --git a/pkg/cwhub/relativepath_test.go b/pkg/cwhub/relativepath_test.go
new file mode 100644
index 000000000..11eba5660
--- /dev/null
+++ b/pkg/cwhub/relativepath_test.go
@@ -0,0 +1,72 @@
+package cwhub
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestRelativePathComponents(t *testing.T) {
+	tests := []struct {
+		name     string
+		path     string
+		baseDir  string
+		expected []string
+	}{
+		{
+			name:     "Path within baseDir",
+			path:     "/home/user/project/src/file.go",
+			baseDir:  "/home/user/project",
+			expected: []string{"src", "file.go"},
+		},
+		{
+			name:     "Path is baseDir",
+			path:     "/home/user/project",
+			baseDir:  "/home/user/project",
+			expected: []string{},
+		},
+		{
+			name:     "Path outside baseDir",
+			path:     "/home/user/otherproject/src/file.go",
+			baseDir:  "/home/user/project",
+			expected: []string{},
+		},
+		{
+			name:     "Path is subdirectory of baseDir",
+			path:     "/home/user/project/src/",
+			baseDir:  "/home/user/project",
+			expected: []string{"src"},
+		},
+		{
+			name:     "Relative paths",
+			path:     "project/src/file.go",
+			baseDir:  "project",
+			expected: []string{"src", "file.go"},
+		},
+		{
+			name:     "BaseDir with trailing slash",
+			path:     "/home/user/project/src/file.go",
+			baseDir:  "/home/user/project/",
+			expected: []string{"src", "file.go"},
+		},
+		{
+			name:     "Empty baseDir",
+			path:     "/home/user/project/src/file.go",
+			baseDir:  "",
+			expected: []string{},
+		},
+		{
+			name:     "Empty path",
+			path:     "",
+			baseDir:  "/home/user/project",
+			expected: []string{},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			result := relativePathComponents(tt.path, tt.baseDir)
+			assert.Equal(t, tt.expected, result)
+		})
+	}
+}
diff --git a/pkg/cwhub/sync.go b/pkg/cwhub/sync.go
index 38bb376ae..81d41d559 100644
--- a/pkg/cwhub/sync.go
+++ b/pkg/cwhub/sync.go
@@ -20,22 +20,49 @@ func isYAMLFileName(path string) bool {
 	return strings.HasSuffix(path, ".yaml") || strings.HasSuffix(path, ".yml")
 }
 
-// linkTarget returns the target of a symlink, or empty string if it's dangling.
-func linkTarget(path string, logger *logrus.Logger) (string, error) {
-	hubpath, err := os.Readlink(path)
+// resolveSymlink returns the ultimate target path of a symlink
+// returns error if the symlink is dangling or too many symlinks are followed
+func resolveSymlink(path string) (string, error) {
+	const maxSymlinks = 10 // Prevent infinite loops
+	for i := 0; i < maxSymlinks; i++ {
+		fi, err := os.Lstat(path)
+		if err != nil {
+			return "", err // dangling link
+		}
+
+		if fi.Mode()&os.ModeSymlink == 0 {
+			// found the target
+			return path, nil
+		}
+
+		path, err = os.Readlink(path)
+		if err != nil {
+			return "", err
+		}
+
+		// relative to the link's directory?
+		if !filepath.IsAbs(path) {
+			path = filepath.Join(filepath.Dir(path), path)
+		}
+	}
+
+	return "", errors.New("too many levels of symbolic links")
+}
+
+// isPathInside checks if a path is inside the given directory
+// it can return false negatives if the filesystem is case insensitive
+func isPathInside(path, dir string) (bool, error) {
+	absFilePath, err := filepath.Abs(path)
 	if err != nil {
-		return "", fmt.Errorf("unable to read symlink: %s", path)
+		return false, err
 	}
 
-	logger.Tracef("symlink %s -> %s", path, hubpath)
-
-	_, err = os.Lstat(hubpath)
-	if os.IsNotExist(err) {
-		logger.Warningf("link target does not exist: %s -> %s", path, hubpath)
-		return "", nil
+	absDir, err := filepath.Abs(dir)
+	if err != nil {
+		return false, err
 	}
 
-	return hubpath, nil
+	return strings.HasPrefix(absFilePath, absDir), nil
 }
 
 // information used to create a new Item, from a file path.
@@ -53,60 +80,78 @@ func (h *Hub) getItemFileInfo(path string, logger *logrus.Logger) (*itemFileInfo
 	hubDir := h.local.HubDir
 	installDir := h.local.InstallDir
 
-	subs := strings.Split(path, string(os.PathSeparator))
+	subsHub := relativePathComponents(path, hubDir)
+	subsInstall := relativePathComponents(path, installDir)
 
-	logger.Tracef("path:%s, hubdir:%s, installdir:%s", path, hubDir, installDir)
-	logger.Tracef("subs:%v", subs)
-	// we're in hub (~/.hub/hub/)
-	if strings.HasPrefix(path, hubDir) {
+	switch {
+	case len(subsHub) > 0:
 		logger.Tracef("in hub dir")
 
-		// .../hub/parsers/s00-raw/crowdsec/skip-pretag.yaml
-		// .../hub/scenarios/crowdsec/ssh_bf.yaml
-		// .../hub/profiles/crowdsec/linux.yaml
-		if len(subs) < 4 {
-			return nil, fmt.Errorf("path is too short: %s (%d)", path, len(subs))
+		// .../hub/parsers/s00-raw/crowdsecurity/skip-pretag.yaml
+		// .../hub/scenarios/crowdsecurity/ssh_bf.yaml
+		// .../hub/profiles/crowdsecurity/linux.yaml
+		if len(subsHub) < 3 {
+			return nil, fmt.Errorf("path is too short: %s (%d)", path, len(subsHub))
+		}
+
+		ftype := subsHub[0]
+		if !slices.Contains(ItemTypes, ftype) {
+			// this doesn't really happen anymore, because we only scan the {hubtype} directories
+			return nil, fmt.Errorf("unknown configuration type '%s'", ftype)
+		}
+
+		stage := ""
+		fauthor := subsHub[1]
+		fname := subsHub[2]
+
+		if ftype == PARSERS || ftype == POSTOVERFLOWS {
+			stage = subsHub[1]
+			fauthor = subsHub[2]
+			fname = subsHub[3]
 		}
 
 		ret = &itemFileInfo{
 			inhub:   true,
-			fname:   subs[len(subs)-1],
-			fauthor: subs[len(subs)-2],
-			stage:   subs[len(subs)-3],
-			ftype:   subs[len(subs)-4],
+			ftype:   ftype,
+			stage:   stage,
+			fauthor: fauthor,
+			fname:   fname,
 		}
-	} else if strings.HasPrefix(path, installDir) { // we're in install /etc/crowdsec/<type>/...
+
+	case len(subsInstall) > 0:
 		logger.Tracef("in install dir")
 
-		if len(subs) < 3 {
-			return nil, fmt.Errorf("path is too short: %s (%d)", path, len(subs))
-		}
 		// .../config/parser/stage/file.yaml
 		// .../config/postoverflow/stage/file.yaml
 		// .../config/scenarios/scenar.yaml
 		// .../config/collections/linux.yaml //file is empty
+
+		if len(subsInstall) < 2 {
+			return nil, fmt.Errorf("path is too short: %s (%d)", path, len(subsInstall))
+		}
+
+		// this can be in any number of subdirs, we join them to compose the item name
+
+		ftype := subsInstall[0]
+		stage := ""
+		fname := strings.Join(subsInstall[1:], "/")
+
+		if ftype == PARSERS || ftype == POSTOVERFLOWS {
+			stage = subsInstall[1]
+			fname = strings.Join(subsInstall[2:], "/")
+		}
+
 		ret = &itemFileInfo{
 			inhub:   false,
-			fname:   subs[len(subs)-1],
-			stage:   subs[len(subs)-2],
-			ftype:   subs[len(subs)-3],
+			ftype:   ftype,
+			stage:   stage,
 			fauthor: "",
+			fname:   fname,
 		}
-	} else {
+	default:
 		return nil, fmt.Errorf("file '%s' is not from hub '%s' nor from the configuration directory '%s'", path, hubDir, installDir)
 	}
 
-	logger.Tracef("stage:%s ftype:%s", ret.stage, ret.ftype)
-
-	if ret.ftype != PARSERS && ret.ftype != POSTOVERFLOWS {
-		if !slices.Contains(ItemTypes, ret.stage) {
-			return nil, errors.New("unknown configuration type")
-		}
-
-		ret.ftype = ret.stage
-		ret.stage = ""
-	}
-
 	logger.Tracef("CORRECTED [%s] by [%s] in stage [%s] of type [%s]", ret.fname, ret.fauthor, ret.stage, ret.ftype)
 
 	return ret, nil
@@ -176,8 +221,6 @@ func newLocalItem(h *Hub, path string, info *itemFileInfo) (*Item, error) {
 }
 
 func (h *Hub) itemVisit(path string, f os.DirEntry, err error) error {
-	hubpath := ""
-
 	if err != nil {
 		h.logger.Debugf("while syncing hub dir: %s", err)
 		// there is a path error, we ignore the file
@@ -190,8 +233,26 @@ func (h *Hub) itemVisit(path string, f os.DirEntry, err error) error {
 		return err
 	}
 
+	// permission errors, files removed while reading, etc.
+	if f == nil {
+		return nil
+	}
+
+	if f.IsDir() {
+		// if a directory starts with a dot, we don't traverse it
+		// - single dot prefix is hidden by unix convention
+		// - double dot prefix is used by k8s to mount config maps
+		if strings.HasPrefix(f.Name(), ".") {
+			h.logger.Tracef("skipping hidden directory %s", path)
+			return filepath.SkipDir
+		}
+
+		// keep traversing
+		return nil
+	}
+
 	// we only care about YAML files
-	if f == nil || f.IsDir() || !isYAMLFileName(f.Name()) {
+	if !isYAMLFileName(f.Name()) {
 		return nil
 	}
 
@@ -201,35 +262,38 @@ func (h *Hub) itemVisit(path string, f os.DirEntry, err error) error {
 		return nil
 	}
 
-	// non symlinks are local user files or hub files
-	if f.Type()&os.ModeSymlink == 0 {
-		h.logger.Tracef("%s is not a symlink", path)
+	// follow the link to see if it falls in the hub directory
+	// if it's not a link, target == path
+	target, err := resolveSymlink(path)
+	if err != nil {
+		// target does not exist, the user might have removed the file
+		// or switched to a hub branch without it; or symlink loop
+		h.logger.Warningf("Ignoring file %s: %s", path, err)
+		return nil
+	}
 
-		if !info.inhub {
-			h.logger.Tracef("%s is a local file, skip", path)
+	targetInHub, err := isPathInside(target, h.local.HubDir)
+	if err != nil {
+		h.logger.Warningf("Ignoring file %s: %s", path, err)
+		return nil
+	}
 
-			item, err := newLocalItem(h, path, info)
-			if err != nil {
-				return err
-			}
+	// local (custom) item if the file or link target is not inside the hub dir
+	if !targetInHub {
+		h.logger.Tracef("%s is a local file, skip", path)
 
-			h.addItem(item)
-
-			return nil
-		}
-	} else {
-		hubpath, err = linkTarget(path, h.logger)
+		item, err := newLocalItem(h, path, info)
 		if err != nil {
 			return err
 		}
 
-		if hubpath == "" {
-			// target does not exist, the user might have removed the file
-			// or switched to a hub branch without it
-			return nil
-		}
+		h.addItem(item)
+
+		return nil
 	}
 
+	hubpath := target
+
 	// try to find which configuration item it is
 	h.logger.Tracef("check [%s] of %s", info.fname, info.ftype)
 
diff --git a/pkg/metabase/metabase.go b/pkg/metabase/metabase.go
index 837bab796..324a05666 100644
--- a/pkg/metabase/metabase.go
+++ b/pkg/metabase/metabase.go
@@ -70,12 +70,12 @@ func (m *Metabase) Init(containerName string, image string) error {
 
 	switch m.Config.Database.Type {
 	case "mysql":
-		return fmt.Errorf("'mysql' is not supported yet for cscli dashboard")
+		return errors.New("'mysql' is not supported yet for cscli dashboard")
 		//DBConnectionURI = fmt.Sprintf("MB_DB_CONNECTION_URI=mysql://%s:%d/%s?user=%s&password=%s&allowPublicKeyRetrieval=true", remoteDBAddr, m.Config.Database.Port, m.Config.Database.DbName, m.Config.Database.User, m.Config.Database.Password)
 	case "sqlite":
 		m.InternalDBURL = metabaseSQLiteDBURL
 	case "postgresql", "postgres", "pgsql":
-		return fmt.Errorf("'postgresql' is not supported yet by cscli dashboard")
+		return errors.New("'postgresql' is not supported yet by cscli dashboard")
 	default:
 		return fmt.Errorf("database '%s' not supported", m.Config.Database.Type)
 	}
diff --git a/test/bats/20_hub_items.bats b/test/bats/20_hub_items.bats
index 214d07d92..4b390c90e 100644
--- a/test/bats/20_hub_items.bats
+++ b/test/bats/20_hub_items.bats
@@ -176,7 +176,7 @@ teardown() {
     rune -0 mkdir -p "$CONFIG_DIR/collections"
     rune -0 ln -s /this/does/not/exist.yaml "$CONFIG_DIR/collections/foobar.yaml"
     rune -0 cscli hub list
-    assert_stderr --partial "link target does not exist: $CONFIG_DIR/collections/foobar.yaml -> /this/does/not/exist.yaml"
+    assert_stderr --partial "Ignoring file $CONFIG_DIR/collections/foobar.yaml: lstat /this/does/not/exist.yaml: no such file or directory"
     rune -0 cscli hub list -o json
     rune -0 jq '.collections' <(output)
     assert_json '[]'
@@ -194,9 +194,89 @@ teardown() {
     assert_output 'false'
 }
 
-@test "skip files if we can't guess their type" {
-    rune -0 mkdir -p "$CONFIG_DIR/scenarios/foo"
-    rune -0 touch "$CONFIG_DIR/scenarios/foo/bar.yaml"
-    rune -0 cscli hub list
-    assert_stderr --partial "Ignoring file $CONFIG_DIR/scenarios/foo/bar.yaml: unknown configuration type"
+@test "don't traverse hidden directories (starting with a dot)" {
+    rune -0 mkdir -p "$CONFIG_DIR/scenarios/.foo"
+    rune -0 touch "$CONFIG_DIR/scenarios/.foo/bar.yaml"
+    rune -0 cscli hub list --trace
+    assert_stderr --partial "skipping hidden directory $CONFIG_DIR/scenarios/.foo"
+}
+
+@test "allow symlink to target inside a hidden directory" {
+    # k8s config maps use hidden directories and links when mounted
+    rune -0 mkdir -p "$CONFIG_DIR/scenarios/.foo"
+
+    # ignored
+    rune -0 touch "$CONFIG_DIR/scenarios/.foo/hidden.yaml"
+    rune -0 cscli scenarios list -o json
+    rune -0 jq '.scenarios | length' <(output)
+    assert_output 0
+
+    # real file
+    rune -0 touch "$CONFIG_DIR/scenarios/myfoo.yaml"
+    rune -0 cscli scenarios list -o json
+    rune -0 jq '.scenarios | length' <(output)
+    assert_output 1
+
+    rune -0 rm "$CONFIG_DIR/scenarios/myfoo.yaml"
+    rune -0 cscli scenarios list -o json
+    rune -0 jq '.scenarios | length' <(output)
+    assert_output 0
+
+    # link to ignored is not ignored, and the name comes from the link
+    rune -0 ln -s "$CONFIG_DIR/scenarios/.foo/hidden.yaml" "$CONFIG_DIR/scenarios/myfoo.yaml"
+    rune -0 cscli scenarios list -o json
+    rune -0 jq -c '[.scenarios[].name] | sort' <(output)
+    assert_json '["myfoo.yaml"]'
+}
+
+@test "item files can be links to links" {
+    rune -0 mkdir -p "$CONFIG_DIR"/scenarios/{.foo,.bar}
+
+    rune -0 ln -s "$CONFIG_DIR/scenarios/.foo/hidden.yaml" "$CONFIG_DIR/scenarios/.bar/hidden.yaml"
+
+    # link to a danling link
+    rune -0 ln -s "$CONFIG_DIR/scenarios/.bar/hidden.yaml" "$CONFIG_DIR/scenarios/myfoo.yaml"
+    rune -0 cscli scenarios list
+    assert_stderr --partial "Ignoring file $CONFIG_DIR/scenarios/myfoo.yaml: lstat $CONFIG_DIR/scenarios/.foo/hidden.yaml: no such file or directory"
+    rune -0 cscli scenarios list -o json
+    rune -0 jq '.scenarios | length' <(output)
+    assert_output 0
+
+    # detect link loops
+    rune -0 ln -s "$CONFIG_DIR/scenarios/.bar/hidden.yaml" "$CONFIG_DIR/scenarios/.foo/hidden.yaml"
+    rune -0 cscli scenarios list
+    assert_stderr --partial "Ignoring file $CONFIG_DIR/scenarios/myfoo.yaml: too many levels of symbolic links"
+
+    rune -0 rm "$CONFIG_DIR/scenarios/.foo/hidden.yaml"
+    rune -0 touch "$CONFIG_DIR/scenarios/.foo/hidden.yaml"
+    rune -0 cscli scenarios list -o json
+    rune -0 jq '.scenarios | length' <(output)
+    assert_output 1
+}
+
+@test "item files can be in a subdirectory" {
+    rune -0 mkdir -p "$CONFIG_DIR/scenarios/sub/sub2/sub3"
+    rune -0 touch "$CONFIG_DIR/scenarios/sub/imlocal.yaml"
+    # subdir name is now part of the item name
+    rune -0 cscli scenarios inspect sub/imlocal.yaml -o json
+    rune -0 jq -e '[.tainted,.local==false,true]' <(output)
+    rune -0 rm "$CONFIG_DIR/scenarios/sub/imlocal.yaml"
+
+    rune -0 ln -s "$HUB_DIR/scenarios/crowdsecurity/smb-bf.yaml" "$CONFIG_DIR/scenarios/sub/smb-bf.yaml"
+    rune -0 cscli scenarios inspect crowdsecurity/smb-bf -o json
+    rune -0 jq -e '[.tainted,.local==false,false]' <(output)
+    rune -0 rm "$CONFIG_DIR/scenarios/sub/smb-bf.yaml"
+
+    rune -0 ln -s "$HUB_DIR/scenarios/crowdsecurity/smb-bf.yaml" "$CONFIG_DIR/scenarios/sub/sub2/sub3/smb-bf.yaml"
+    rune -0 cscli scenarios inspect crowdsecurity/smb-bf -o json
+    rune -0 jq -e '[.tainted,.local==false,false]' <(output)
+}
+
+@test "same file name for local items in different subdirectories" {
+    rune -0 mkdir -p "$CONFIG_DIR"/scenarios/{foo,bar}
+    rune -0 touch "$CONFIG_DIR/scenarios/foo/local.yaml"
+    rune -0 touch "$CONFIG_DIR/scenarios/bar/local.yaml"
+    rune -0 cscli scenarios list -o json
+    rune -0 jq -c '[.scenarios[].name] | sort' <(output)
+    assert_json '["bar/local.yaml","foo/local.yaml"]'
 }