diff --git a/pkg/cwhub/cwhub.go b/pkg/cwhub/cwhub.go index 74d533c74..c7e17bd62 100644 --- a/pkg/cwhub/cwhub.go +++ b/pkg/cwhub/cwhub.go @@ -10,5 +10,5 @@ import ( ) var hubClient = &http.Client{ - Timeout: 20 * time.Second, + Timeout: 120 * time.Second, } diff --git a/pkg/cwhub/enable.go b/pkg/cwhub/enable.go index 9887558f6..49946c869 100644 --- a/pkg/cwhub/enable.go +++ b/pkg/cwhub/enable.go @@ -146,35 +146,10 @@ func (i *Item) removeInstallLink() error { // disable removes the symlink to the downloaded content, also removes the content if purge is true func (i *Item) disable(purge bool, force bool) error { // XXX: should return the number of disabled/purged items to inform the upper layer whether to reload or not - if i.IsLocal() { - return fmt.Errorf("%s isn't managed by hub. Please delete manually", i.Name) - } - - if i.Tainted && !force { - return fmt.Errorf("%s is tainted, use '--force' to overwrite", i.Name) - } - - for _, sub := range i.SubItems() { - // TODO XXX: if the other collection(s) are direct or indirect dependencies of the current one, it's good to go - if len(sub.BelongsToCollections) > 1 { - log.Infof("%s was not removed because it belongs to another collection", sub.Name) - continue - } - - if err := sub.disable(purge, force); err != nil { - return fmt.Errorf("while disabling %s: %w", sub.Name, err) - } - } - - if !i.Installed && !purge { - log.Infof("removing %s: not installed -- no need to remove", i.Name) - return nil - } - err := i.removeInstallLink() if os.IsNotExist(err) { if !purge && !force { - return fmt.Errorf("can't disable %s: %s doesn't exist", i.Name, i.installLink()) + return fmt.Errorf("link %s does not exist (override with --force or --purge)", i.installLink()) } } else if err != nil { return err diff --git a/pkg/cwhub/helpers.go b/pkg/cwhub/helpers.go index ba14c07fb..6d106f813 100644 --- a/pkg/cwhub/helpers.go +++ b/pkg/cwhub/helpers.go @@ -17,6 +17,7 @@ import ( "github.com/enescakir/emoji" log "github.com/sirupsen/logrus" + "slices" ) // Install installs the item from the hub, downloading it if needed @@ -51,12 +52,70 @@ func (i *Item) Install(force bool, downloadOnly bool) error { return nil } +// allDependencies return a list of all dependencies and sub-dependencies of the item +func (i *Item) allDependencies() []*Item { + var deps []*Item + + for _, dep := range i.SubItems() { + if dep == i { + log.Errorf("circular dependency detected: %s depends on %s", dep.Name, i.Name) + continue + } + + deps = append(deps, dep.allDependencies()...) + } + + return append(deps, i) +} + // Remove disables the item, optionally removing the downloaded content -func (i *Item) Remove(purge bool, forceAction bool) (bool, error) { +func (i *Item) Remove(purge bool, force bool) (bool, error) { + if i.IsLocal() { + return false, fmt.Errorf("%s isn't managed by hub. Please delete manually", i.Name) + } + + if i.Tainted && !force { + return false, fmt.Errorf("%s is tainted, use '--force' to remove", i.Name) + } + + if !i.Installed && !purge { + log.Infof("removing %s: not installed -- no need to remove", i.Name) + return false, nil + } + removed := false - if err := i.disable(purge, forceAction); err != nil { - return false, fmt.Errorf("unable to disable %s: %w", i.Name, err) + allDeps := i.allDependencies() + + for _, sub := range i.SubItems() { + if !sub.Installed { + continue + } + + // if the other collection(s) are direct or indirect dependencies of the current one, it's good to go + // log parent collections + for _, subParent := range sub.parentCollections() { + if subParent == i { + continue + } + + if !slices.Contains(allDeps, subParent) { + log.Infof("%s was not removed because it also belongs to %s", sub.Name, subParent.Name) + continue + } + } + + subRemoved, err := sub.Remove(purge, force) + if err != nil { + return false, fmt.Errorf("unable to disable %s: %w", i.Name, err) + } + + removed = removed || subRemoved + } + + err := i.disable(purge, force) + if err != nil { + return false, fmt.Errorf("while removing %s: %w", i.Name, err) } // XXX: should take the value from disable() diff --git a/pkg/cwhub/items.go b/pkg/cwhub/items.go index 37e853d3a..2ed593e20 100644 --- a/pkg/cwhub/items.go +++ b/pkg/cwhub/items.go @@ -186,6 +186,21 @@ func (i *Item) logMissingSubItems() { } } +func (i *Item) parentCollections() []*Item { + ret := make([]*Item, 0) + + for _, parentName := range i.BelongsToCollections { + parent := i.hub.GetItem(COLLECTIONS, parentName) + if parent == nil { + continue + } + + ret = append(ret, parent) + } + + return ret +} + // Status returns the status of the item as a string and an emoji // ie. "enabled,update-available" and emoji.Warning func (i *Item) Status() (string, emoji.Emoji) { diff --git a/pkg/cwhub/sync.go b/pkg/cwhub/sync.go index 30336fa91..2d55e9e21 100644 --- a/pkg/cwhub/sync.go +++ b/pkg/cwhub/sync.go @@ -393,6 +393,8 @@ func (h *Hub) syncDir(dir string) ([]string, error) { // For each, scan PARSERS, POSTOVERFLOWS, SCENARIOS and COLLECTIONS last for _, scan := range ItemTypes { + // cpath: top-level item directory, either downloaded or installed items. + // i.e. /etc/crowdsec/parsers, /etc/crowdsec/hub/parsers, ... cpath, err := filepath.Abs(fmt.Sprintf("%s/%s", dir, scan)) if err != nil { log.Errorf("failed %s: %s", cpath, err) diff --git a/test/bats/20_hub_collections_dep.bats b/test/bats/20_hub_collections_dep.bats index b3dc80775..a44d8bc9c 100644 --- a/test/bats/20_hub_collections_dep.bats +++ b/test/bats/20_hub_collections_dep.bats @@ -81,11 +81,34 @@ teardown() { # now we can't remove smb without --force rune -1 cscli collections remove crowdsecurity/smb - assert_stderr --partial "unable to disable crowdsecurity/smb: crowdsecurity/smb is tainted, use '--force' to overwrite" + assert_stderr --partial "crowdsecurity/smb is tainted, use '--force' to remove" +} +@test "cscli collections (dependencies II: the revenge)" { rune -0 cscli collections install crowdsecurity/wireguard baudneo/gotify rune -0 cscli collections remove crowdsecurity/wireguard - assert_stderr --partial "crowdsecurity/syslog-logs was not removed because it belongs to another collection" + assert_stderr --partial "crowdsecurity/syslog-logs was not removed because it also belongs to baudneo/gotify" rune -0 cscli collections inspect crowdsecurity/wireguard -o json rune -0 jq -e '.installed==false' <(output) } + +@test "cscli collections (dependencies III: origins)" { + # it is perfectly fine to remove an item belonging to a collection that we are removing anyway + + # inject a dependency: sshd requires the syslog-logs parsers, but linux does too + hub_dep=$(jq <"$INDEX_PATH" '. * {collections:{"crowdsecurity/sshd":{parsers:["crowdsecurity/syslog-logs"]}}}') + echo "$hub_dep" >"$INDEX_PATH" + + # verify that installing sshd brings syslog-logs + rune -0 cscli collections install crowdsecurity/sshd + rune -0 cscli parsers inspect crowdsecurity/syslog-logs -o json + rune -0 jq -e '.installed==true' <(output) + + rune -0 cscli collections install crowdsecurity/linux + + # removing linux should remove syslog-logs even though sshd depends on it + rune -0 cscli collections remove crowdsecurity/linux + refute_stderr --partial "crowdsecurity/syslog-logs was not removed" + rune -0 cscli parsers list -o json + rune -0 jq -e '.parsers | length == 0' <(output) +} diff --git a/test/bats/20_hub_items.bats b/test/bats/20_hub_items.bats index 0ac3d1c8d..ca40beea0 100644 --- a/test/bats/20_hub_items.bats +++ b/test/bats/20_hub_items.bats @@ -80,7 +80,7 @@ teardown() { rune -0 rm "$(output)" rune -0 cscli parsers remove crowdsecurity/syslog-logs --debug - assert_stderr --partial "Removed crowdsecurity/syslog-logs" + assert_stderr --partial "removing crowdsecurity/syslog-logs: not installed -- no need to remove" rune -0 cscli parsers inspect crowdsecurity/syslog-logs -o json rune -0 jq -r '.path' <(output)