Refact cwhub (#2603)

* Split RemoteHub.downloadIndex() = Hub.updateIndex() + RemoteHub.fetchIndex()
* Functions safePath(), Item.installPath(), item.downloadPath()

pkg/cwhub/cwhub.go

@@ -5,10 +5,32 @@
 package cwhub
 
 import (
+	"fmt"
 	"net/http"
+	"path/filepath"
+	"strings"
 	"time"
 )
 
 var hubClient = &http.Client{
 	Timeout: 120 * time.Second,
 }
+
+// safePath returns an error if the given file path would escape the base directory.
+func safePath(dir, filePath string) (string, error) {
+	absBaseDir, err := filepath.Abs(filepath.Clean(dir))
+	if err != nil {
+		return "", err
+	}
+
+	absFilePath, err := filepath.Abs(filepath.Join(dir, filePath))
+	if err != nil {
+		return "", err
+	}
+
+	if !strings.HasPrefix(absFilePath, absBaseDir) {
+		return "", fmt.Errorf("path %s escapes base directory %s", filePath, dir)
+	}
+
+	return absFilePath, nil
+}

pkg/cwhub/dataset.go

@@ -6,7 +6,6 @@ import (
 	"io"
 	"net/http"
 	"os"
-	"path/filepath"
 
 	log "github.com/sirupsen/logrus"
 	"gopkg.in/yaml.v2"
@@ -67,7 +66,10 @@ func downloadDataSet(dataFolder string, force bool, reader io.Reader) error {
 		}
 
 		for _, dataS := range data.Data {
-			destPath := filepath.Join(dataFolder, dataS.DestPath)
+			destPath, err := safePath(dataFolder, dataS.DestPath)
+			if err != nil {
+				return err
+			}
 
 			if _, err := os.Stat(destPath); os.IsNotExist(err) || force {
 				log.Infof("downloading data '%s' in '%s'", dataS.SourceURL, destPath)

pkg/cwhub/enable.go

@@ -10,15 +10,33 @@ import (
 	log "github.com/sirupsen/logrus"
 )
 
-// installLink returns the location of the symlink to the downloaded config file
+// installPath returns the location of the symlink to the item in the hub, or the path of the item itself if it's local
 // (eg. /etc/crowdsec/collections/xyz.yaml)
-func (i *Item) installLinkPath() string {
-	return filepath.Join(i.hub.local.InstallDir, i.Type, i.Stage, i.FileName)
+// raises an error if the path goes outside of the install dir
+func (i *Item) installPath() (string, error) {
+	p := i.Type
+	if i.Stage != "" {
+		p = filepath.Join(p, i.Stage)
+	}
+
+	return safePath(i.hub.local.InstallDir, filepath.Join(p, i.FileName))
+}
+
+// downloadPath returns the location of the actual config file in the hub
+// (eg. /etc/crowdsec/hub/collections/author/xyz.yaml)
+// raises an error if the path goes outside of the hub dir
+func (i *Item) downloadPath() (string, error) {
+	ret, err := safePath(i.hub.local.HubDir, i.RemotePath)
+	if err != nil {
+		return "", err
+	}
+
+	return ret, nil
 }
 
 // makeLink creates a symlink between the actual config file at hub.HubDir and hub.ConfigDir
 func (i *Item) createInstallLink() error {
-	dest, err := filepath.Abs(i.installLinkPath())
+	dest, err := i.installPath()
 	if err != nil {
 		return err
 	}
@@ -33,7 +51,7 @@ func (i *Item) createInstallLink() error {
 		return nil
 	}
 
-	src, err := filepath.Abs(filepath.Join(i.hub.local.HubDir, i.RemotePath))
+	src, err := i.downloadPath()
 	if err != nil {
 		return err
 	}
@@ -86,7 +104,10 @@ func (i *Item) purge() error {
 		return nil
 	}
 
-	src := filepath.Join(i.hub.local.HubDir, i.RemotePath)
+	src, err := i.downloadPath()
+	if err != nil {
+		return err
+	}
 
 	if err := os.Remove(src); err != nil {
 		if os.IsNotExist(err) {
@@ -105,7 +126,7 @@ func (i *Item) purge() error {
 
 // removeInstallLink removes the symlink to the downloaded content
 func (i *Item) removeInstallLink() error {
-	syml, err := filepath.Abs(i.installLinkPath())
+	syml, err := i.installPath()
 	if err != nil {
 		return err
 	}
@@ -126,7 +147,7 @@ func (i *Item) removeInstallLink() error {
 		return fmt.Errorf("while reading symlink: %w", err)
 	}
 
-	src, err := filepath.Abs(i.hub.local.HubDir + "/" + i.RemotePath)
+	src, err := i.downloadPath()
 	if err != nil {
 		return err
 	}
@@ -151,7 +172,8 @@ func (i *Item) disable(purge bool, force bool) error {
 	err := i.removeInstallLink()
 	if os.IsNotExist(err) {
 		if !purge && !force {
-			return fmt.Errorf("link %s does not exist (override with --force or --purge)", i.installLinkPath())
+			link, _ := i.installPath()
+			return fmt.Errorf("link %s does not exist (override with --force or --purge)", link)
 		}
 	} else if err != nil {
 		return err

pkg/cwhub/enable_test.go

@@ -10,7 +10,7 @@ import (
 
 func testInstall(hub *Hub, t *testing.T, item *Item) {
 	// Install the parser
-	err := item.downloadLatest(false, false)
+	_, err := item.downloadLatest(false, false)
 	require.NoError(t, err, "failed to download %s", item.Name)
 
 	err = hub.localSync()
@@ -48,7 +48,7 @@ func testUpdate(hub *Hub, t *testing.T, item *Item) {
 	assert.False(t, hub.Items[item.Type][item.Name].UpToDate, "%s should not be up-to-date", item.Name)
 
 	// Update it + check status
-	err := item.downloadLatest(true, true)
+	_, err := item.downloadLatest(true, true)
 	require.NoError(t, err, "failed to update %s", item.Name)
 
 	// Local sync and check status

pkg/cwhub/helpers.go

@@ -13,7 +13,6 @@ import (
 	"net/http"
 	"os"
 	"path/filepath"
-	"strings"
 
 	"github.com/enescakir/emoji"
 	log "github.com/sirupsen/logrus"
@@ -31,13 +30,14 @@ func (i *Item) Install(force bool, downloadOnly bool) error {
 	}
 
 	// XXX: confusing semantic between force and updateOnly?
-	if err := i.downloadLatest(force, true); err != nil {
+	filePath, err := i.downloadLatest(force, true)
+	if err != nil {
 		return fmt.Errorf("while downloading %s: %w", i.Name, err)
 	}
 
 	if downloadOnly {
 		// XXX: should get the path from downloadLatest
-		log.Infof("Downloaded %s to %s", i.Name, filepath.Join(i.hub.local.HubDir, i.RemotePath))
+		log.Infof("Downloaded %s to %s", i.Name, filePath)
 		return nil
 	}
 
@@ -177,7 +177,7 @@ func (i *Item) Upgrade(force bool) (bool, error) {
 		}
 	}
 
-	if err := i.downloadLatest(force, true); err != nil {
+	if _, err := i.downloadLatest(force, true); err != nil {
 		return false, fmt.Errorf("%s: download failed: %w", i.Name, err)
 	}
 
@@ -200,7 +200,7 @@ func (i *Item) Upgrade(force bool) (bool, error) {
 }
 
 // downloadLatest downloads the latest version of the item to the hub directory
-func (i *Item) downloadLatest(overwrite bool, updateOnly bool) error {
+func (i *Item) downloadLatest(overwrite bool, updateOnly bool) (string, error) {
 	// XXX: should return the path of the downloaded file (taken from download())
 	log.Debugf("Downloading %s %s", i.Type, i.Name)
 
@@ -216,39 +216,40 @@ func (i *Item) downloadLatest(overwrite bool, updateOnly bool) error {
 		if sub.HasSubItems() {
 			log.Tracef("collection, recurse")
 
-			if err := sub.downloadLatest(overwrite, updateOnly); err != nil {
-				return fmt.Errorf("while downloading %s: %w", sub.Name, err)
+			if _, err := sub.downloadLatest(overwrite, updateOnly); err != nil {
+				return "", fmt.Errorf("while downloading %s: %w", sub.Name, err)
 			}
 		}
 
 		downloaded := sub.Downloaded
 
-		if err := sub.download(overwrite); err != nil {
-			return fmt.Errorf("while downloading %s: %w", sub.Name, err)
+		if _, err := sub.download(overwrite); err != nil {
+			return "", fmt.Errorf("while downloading %s: %w", sub.Name, err)
 		}
 
 		// We need to enable an item when it has been added to a collection since latest release of the collection.
 		// We check if sub.Downloaded is false because maybe the item has been disabled by the user.
 		if !sub.Installed && !downloaded {
 			if err := sub.enable(); err != nil {
-				return fmt.Errorf("enabling '%s': %w", sub.Name, err)
+				return "", fmt.Errorf("enabling '%s': %w", sub.Name, err)
 			}
 		}
 	}
 
 	if !i.Installed && updateOnly && i.Downloaded {
 		log.Debugf("skipping upgrade of %s: not installed", i.Name)
-		return nil
+		return "", nil
 	}
 
-	if err := i.download(overwrite); err != nil {
-		return fmt.Errorf("failed to download item: %w", err)
+	ret, err := i.download(overwrite)
+	if err != nil {
+		return "", fmt.Errorf("failed to download item: %w", err)
 	}
 
-	return nil
+	return ret, nil
 }
 
-// fetch downloads the item from the hub, verifies the hash and returns the body
+// fetch downloads the item from the hub, verifies the hash and returns the content
 func (i *Item) fetch() ([]byte, error) {
 	url, err := i.hub.remote.urlTo(i.RemotePath)
 	if err != nil {
@@ -287,12 +288,12 @@ func (i *Item) fetch() ([]byte, error) {
 }
 
 // download downloads the item from the hub and writes it to the hub directory
-func (i *Item) download(overwrite bool) error {
+func (i *Item) download(overwrite bool) (string, error) {
 	// if user didn't --force, don't overwrite local, tainted, up-to-date files
 	if !overwrite {
 		if i.Tainted {
 			log.Debugf("%s: tainted, not updated", i.Name)
-			return nil
+			return "", nil
 		}
 
 		if i.UpToDate {
@@ -303,55 +304,52 @@ func (i *Item) download(overwrite bool) error {
 
 	body, err := i.fetch()
 	if err != nil {
-		return err
+		return "", err
 	}
 
-	tdir := i.hub.local.HubDir
-
-	//all good, install
-
-	finalPath, err := filepath.Abs(filepath.Join(tdir, i.RemotePath))
-	if err != nil {
-		return err
-	}
+	// all good, install
 
 	// ensure that target file is within target dir
-	if !strings.HasPrefix(finalPath, tdir) {
-		return fmt.Errorf("path %s escapes %s, abort", i.RemotePath, tdir)
+	finalPath, err := i.downloadPath()
+	if err != nil {
+		return "", err
 	}
 
 	parentDir := filepath.Dir(finalPath)
 
 	if err = os.MkdirAll(parentDir, os.ModePerm); err != nil {
-		return fmt.Errorf("while creating %s: %w", parentDir, err)
+		return "", fmt.Errorf("while creating %s: %w", parentDir, err)
 	}
 
 	// check actual file
 	if _, err = os.Stat(finalPath); !os.IsNotExist(err) {
 		log.Warningf("%s: overwrite", i.Name)
-		log.Debugf("target: %s/%s", tdir, i.RemotePath)
+		log.Debugf("target: %s", finalPath)
 	} else {
 		log.Infof("%s: OK", i.Name)
 	}
 
 	if err = os.WriteFile(finalPath, body, 0o644); err != nil {
-		return fmt.Errorf("while writing %s: %w", finalPath, err)
+		return "", fmt.Errorf("while writing %s: %w", finalPath, err)
 	}
 
 	i.Downloaded = true
 	i.Tainted = false
 	i.UpToDate = true
 
 	if err = downloadDataSet(i.hub.local.InstallDataDir, overwrite, bytes.NewReader(body)); err != nil {
-		return fmt.Errorf("while downloading data for %s: %w", i.FileName, err)
+		return "", fmt.Errorf("while downloading data for %s: %w", i.FileName, err)
 	}
 
-	return nil
+	return finalPath, nil
 }
 
 // DownloadDataIfNeeded downloads the data files for the item
 func (i *Item) DownloadDataIfNeeded(force bool) error {
-	itemFilePath := fmt.Sprintf("%s/%s/%s/%s", i.hub.local.InstallDir, i.Type, i.Stage, i.FileName)
+	itemFilePath, err := i.installPath()
+	if err != nil {
+		return err
+	}
 
 	itemFile, err := os.Open(itemFilePath)
 	if err != nil {