Merge pull request #542 from Security-Onion-Solutions/cogburn/detecti…

…on-duplicates

Deduplication of Detections by Public Id

server/modules/detections/detengine_helpers.go

@@ -108,13 +108,14 @@ func WriteStateFile(iom IOManager, path string) {
 	}
 }
 
-type DirtyRepo struct {
-	WasModified bool
+type RepoOnDisk struct {
 	Repo        *model.RuleRepo
+	Path        string
+	WasModified bool
 }
 
-func UpdateRepos(isRunning *bool, baseRepoFolder string, rulesRepos []*model.RuleRepo, cfg *config.ServerConfig) (allRepos map[string]*DirtyRepo, anythingNew bool, err error) {
-	allRepos = map[string]*DirtyRepo{} // map[repoPath]repo
+func UpdateRepos(isRunning *bool, baseRepoFolder string, rulesRepos []*model.RuleRepo, cfg *config.ServerConfig) (allRepos []*RepoOnDisk, anythingNew bool, err error) {
+	allRepos = make([]*RepoOnDisk, 0, len(rulesRepos))
 
 	// read existing repos
 	entries, err := os.ReadDir(baseRepoFolder)
@@ -148,11 +149,12 @@ func UpdateRepos(isRunning *bool, baseRepoFolder string, rulesRepos []*model.Rul
 		_, lastFolder := path.Split(parser.Path)
 		repoPath := filepath.Join(baseRepoFolder, lastFolder)
 
-		dirty := &DirtyRepo{
+		dirty := &RepoOnDisk{
 			Repo: repo,
+			Path: repoPath,
 		}
 
-		allRepos[repoPath] = dirty
+		allRepos = append(allRepos, dirty)
 		reclone := false
 
 		proxyOpts, err := proxyToTransportOptions(cfg.Proxy)
@@ -352,3 +354,27 @@ func AddUser(previous string, user *model.User, sep string) string {
 func EscapeDoubleQuotes(str string) string {
 	return doubleQuoteEscaper.ReplaceAllString(str, "\\$1$2")
 }
+
+func DeduplicateByPublicId(detects []*model.Detection) []*model.Detection {
+	set := map[string]*model.Detection{}
+	deduped := make([]*model.Detection, 0, len(detects))
+
+	for _, detect := range detects {
+		existing, inSet := set[detect.PublicID]
+		if inSet {
+			log.WithFields(log.Fields{
+				"publicId":         detect.PublicID,
+				"engine":           detect.Engine,
+				"existingRuleset":  existing.Ruleset,
+				"duplicateRuleset": detect.Ruleset,
+				"existingTitle":    existing.Title,
+				"duplicateTitle":   detect.Title,
+			}).Warn("duplicate publicId found, skipping")
+		} else {
+			set[detect.PublicID] = detect
+			deduped = append(deduped, detect)
+		}
+	}
+
+	return deduped
+}

server/modules/detections/detengine_helpers_test.go

@@ -280,3 +280,56 @@ func TestProxyToTransportOptions(t *testing.T) {
 		})
 	}
 }
+
+func TestDeduplicateByPublicId(t *testing.T) {
+	tests := []struct {
+		Name      string
+		InputIds  []string
+		ExpOutput []string
+	}{
+		{
+			Name:      "Empty",
+			InputIds:  []string{},
+			ExpOutput: []string{},
+		},
+		{
+			Name:      "No Duplicates",
+			InputIds:  []string{"1", "2", "3"},
+			ExpOutput: []string{"1", "2", "3"},
+		},
+		{
+			Name:      "Only Duplicates",
+			InputIds:  []string{"1", "1", "1", "1", "1", "1", "1", "1", "1", "1"},
+			ExpOutput: []string{"1"},
+		},
+		{
+			Name:      "Mixed",
+			InputIds:  []string{"1", "2", "1", "3", "2", "4", "1", "5", "2", "6"},
+			ExpOutput: []string{"1", "2", "3", "4", "5", "6"},
+		},
+		{
+			Name:      "One Duplicate",
+			InputIds:  []string{"1", "2", "3", "4", "5", "6", "1"},
+			ExpOutput: []string{"1", "2", "3", "4", "5", "6"},
+		},
+	}
+
+	for _, test := range tests {
+		test := test
+		t.Run(test.Name, func(t *testing.T) {
+			dets := make([]*model.Detection, 0, len(test.InputIds))
+			for _, id := range test.InputIds {
+				dets = append(dets, &model.Detection{PublicID: id})
+			}
+
+			deduped := DeduplicateByPublicId(dets)
+
+			output := make([]string, 0, len(deduped))
+			for _, det := range deduped {
+				output = append(output, det.PublicID)
+			}
+
+			assert.Equal(t, test.ExpOutput, output)
+		})
+	}
+}

server/modules/elastalert/elastalert.go

@@ -579,9 +579,6 @@ func (e *ElastAlertEngine) startCommunityRuleImport() {
 			templateFound = true
 		}
 
-		allRepos := map[string]*model.RuleRepo{}
-		var repoChanges bool
-
 		var zips map[string][]byte
 		var errMap map[string]error
 		var regenNeeded bool
@@ -617,9 +614,7 @@ func (e *ElastAlertEngine) startCommunityRuleImport() {
 			continue
 		}
 
-		var dirtyRepos map[string]*detections.DirtyRepo
-
-		dirtyRepos, repoChanges, err = detections.UpdateRepos(&e.isRunning, e.reposFolder, e.rulesRepos, e.srv.Config)
+		dirtyRepos, repoChanges, err := detections.UpdateRepos(&e.isRunning, e.reposFolder, e.rulesRepos, e.srv.Config)
 		if err != nil {
 			if strings.Contains(err.Error(), "module stopped") {
 				break
@@ -637,10 +632,6 @@ func (e *ElastAlertEngine) startCommunityRuleImport() {
 			continue
 		}
 
-		for k, v := range dirtyRepos {
-			allRepos[k] = v.Repo
-		}
-
 		zipHashes := map[string]string{}
 		for pkg, data := range zips {
 			h := sha256.Sum256(data)
@@ -714,7 +705,7 @@ func (e *ElastAlertEngine) startCommunityRuleImport() {
 			break
 		}
 
-		repoDets, errMap := e.parseRepoRules(allRepos)
+		repoDets, errMap := e.parseRepoRules(dirtyRepos)
 		if errMap != nil {
 			log.WithField("sigmaParseError", errMap).Error("something went wrong while parsing sigma rule files from repos")
 		}
@@ -725,6 +716,8 @@ func (e *ElastAlertEngine) startCommunityRuleImport() {
 
 		detects = append(detects, repoDets...)
 
+		detects = detections.DeduplicateByPublicId(detects)
+
 		errMap, err = e.syncCommunityDetections(ctx, detects)
 		if err != nil {
 			if err == errModuleStopped {
@@ -864,11 +857,13 @@ func (e *ElastAlertEngine) parseZipRules(pkgZips map[string][]byte) (detections
 		}
 	}()
 
-	for pkg, zipData := range pkgZips {
+	for _, pkg := range e.sigmaRulePackages {
 		if !e.isRunning {
 			return nil, map[string]error{"module": errModuleStopped}
 		}
 
+		zipData := pkgZips[pkg]
+
 		reader, err := zip.NewReader(bytes.NewReader(zipData), int64(len(zipData)))
 		if err != nil {
 			errMap[pkg] = err
@@ -925,22 +920,22 @@ func (e *ElastAlertEngine) parseZipRules(pkgZips map[string][]byte) (detections
 	return detections, errMap
 }
 
-func (e *ElastAlertEngine) parseRepoRules(allRepos map[string]*model.RuleRepo) (detections []*model.Detection, errMap map[string]error) {
+func (e *ElastAlertEngine) parseRepoRules(allRepos []*detections.RepoOnDisk) (detections []*model.Detection, errMap map[string]error) {
 	errMap = map[string]error{} // map[repoName]error
 	defer func() {
 		if len(errMap) == 0 {
 			errMap = nil
 		}
 	}()
 
-	for repopath, repo := range allRepos {
+	for _, repo := range allRepos {
 		if !e.isRunning {
 			return nil, map[string]error{"module": errModuleStopped}
 		}
 
-		baseDir := repopath
-		if repo.Folder != nil {
-			baseDir = filepath.Join(baseDir, *repo.Folder)
+		baseDir := repo.Path
+		if repo.Repo.Folder != nil {
+			baseDir = filepath.Join(baseDir, *repo.Repo.Folder)
 		}
 
 		err := e.WalkDir(baseDir, func(path string, d fs.DirEntry, err error) error {
@@ -974,16 +969,16 @@ func (e *ElastAlertEngine) parseRepoRules(allRepos map[string]*model.RuleRepo) (
 				return nil
 			}
 
-			ruleset := filepath.Base(repopath)
+			ruleset := filepath.Base(repo.Path)
 
-			det := rule.ToDetection(ruleset, repo.License, repo.Community)
+			det := rule.ToDetection(ruleset, repo.Repo.License, repo.Repo.Community)
 
 			detections = append(detections, det)
 
 			return nil
 		})
 		if err != nil {
-			log.WithError(err).WithField("elastAlertRuleRepo", repopath).Error("Failed to walk repo")
+			log.WithError(err).WithField("elastAlertRuleRepo", repo.Path).Error("Failed to walk repo")
 			continue
 		}
 	}

server/modules/elastalert/elastalert_test.go

@@ -27,6 +27,7 @@ import (
 	"github.com/security-onion-solutions/securityonion-soc/model"
 	"github.com/security-onion-solutions/securityonion-soc/module"
 	"github.com/security-onion-solutions/securityonion-soc/server"
+	"github.com/security-onion-solutions/securityonion-soc/server/modules/detections"
 	"github.com/security-onion-solutions/securityonion-soc/server/modules/elastalert/mock"
 	"github.com/security-onion-solutions/securityonion-soc/util"
 
@@ -529,7 +530,8 @@ level: high
 	}
 
 	engine := ElastAlertEngine{
-		isRunning: true,
+		isRunning:         true,
+		sigmaRulePackages: []string{"all_rules"},
 	}
 	engine.allowRegex = regexp.MustCompile("00000000-0000-0000-0000-00000000")
 	engine.denyRegex = regexp.MustCompile("deny")
@@ -578,11 +580,14 @@ level: high
 license: Elastic-2.0
 `
 
-	repos := map[string]*model.RuleRepo{
-		"repo-path": {
-			Repo:      "github.com/repo-user/repo-path",
-			License:   "DRL",
-			Community: true,
+	repos := []*detections.RepoOnDisk{
+		{
+			Repo: &model.RuleRepo{
+				Repo:      "github.com/repo-user/repo-path",
+				License:   "DRL",
+				Community: true,
+			},
+			Path: "repo-path",
 		},
 	}