fix: stricter static analysis (#355)

* fix: enable `unused` and `forcetypeassert` rules and start fixing warnings

* fix: linter fixes, primarily targeting a function that didn't need to return an error

* chore: output stderr when we can't json decode stdout from golangci-lint

* chore: remove unused

* fix: additional unused fixed

* chore: re-add exception for unused in `size` package, as it is necessary to keep some "unused" code there

.golangci.json

@@ -1,8 +1,7 @@
 {
   "linters": {
     "disable": [
-      "errcheck",
-      "unused"
+      "errcheck"
     ],
     "enable": [
       "gosimple",
@@ -26,6 +25,10 @@
         "path": "foldr_test\\.go",
         "text": "SA4000:",
         "severity": "warning"
+      },
+      {
+        "path": "dawgs/util/size/(.+)",
+        "linters": ["unused"]
       }
     ]
   },
@@ -42,7 +45,11 @@
     "default-severity": "error",
     "rules": [
       {
-        "text": "(ST\\d{4}|S\\d{4}|SA1019)",
+        "linters": ["stylecheck", "gosimple", "unused", "errcheck", "forcetypeassert"],
+        "severity": "warning"
+      },
+      {
+        "text": "SA1019:",
         "severity": "warning"
       },
       {

cmd/api/src/daemons/datapipe/jobs.go

@@ -18,9 +18,10 @@ package datapipe
 
 import (
 	"context"
-	"github.com/specterops/bloodhound/src/database"
 	"os"
 
+	"github.com/specterops/bloodhound/src/database"
+
 	"github.com/specterops/bloodhound/dawgs/graph"
 	"github.com/specterops/bloodhound/log"
 	"github.com/specterops/bloodhound/src/model"
@@ -139,9 +140,3 @@ func (s *Daemon) processIngestTasks(ctx context.Context, ingestTasks model.Inges
 		s.clearFileTask(ingestTask)
 	}
 }
-
-func (s *Daemon) clearTask(ingestTask model.IngestTask) {
-	if err := s.db.DeleteIngestTask(ingestTask); err != nil {
-		log.Errorf("Error removing task from db: %v", err)
-	}
-}

justfile

@@ -140,7 +140,7 @@ prune-my-branches nuclear='no':
 
 # Run all analyzers (requires jq to be installed locally)
 analyze:
-  go run github.com/specterops/bloodhound/packages/go/stbernard analysis | jq 'sort_by(.severity) | .[] | {"severity": .severity, "description": .description, "location": "\(.location.path):\(.location.lines.begin)"}' 
+  go run github.com/specterops/bloodhound/packages/go/stbernard analysis | jq 'sort_by(.severity) | .[] | {"severity": .severity, "description": .description, "location": "\(.location.path):\(.location.lines.begin)"}'
 
 # run docker compose commands for the BH dev profile (Default: up)
 bh-dev *ARGS='up':

packages/go/analysis/ad/ad.go

@@ -352,14 +352,14 @@ func CalculateCrossProductNodeSets(groupExpansions impact.PathAggregator, nodeSe
 	//Find all the groups in our secondary targets and map them to their cardinality in our expansions
 	//Saving off to a map to prevent multiple lookups on the expansions
 	//Unhandled error here is irrelevant, we can never return an error
-	unrollSet.Each(func(id uint32) (bool, error) {
+	unrollSet.Each(func(id uint32) bool {
 		//If group expansions contains this ID and its cardinality is > 0, it's a group/localgroup
 		idCardinality := groupExpansions.Cardinality(id).Cardinality()
 		if idCardinality > 0 {
 			tempMap[id] = idCardinality
 		}
 
-		return true, nil
+		return true
 	})
 
 	//Save the map keys to a new slice, this represents our list of groups in the expansion
@@ -391,12 +391,12 @@ func CalculateCrossProductNodeSets(groupExpansions impact.PathAggregator, nodeSe
 		}
 	}
 
-	unrollSet.Each(func(remainder uint32) (bool, error) {
+	unrollSet.Each(func(remainder uint32) bool {
 		if checkSet.Contains(remainder) {
 			resultEntities.Add(remainder)
 		}
 
-		return true, nil
+		return true
 	})
 
 	return resultEntities
@@ -417,7 +417,7 @@ func CalculateCrossProductBitmaps(groupExpansions impact.PathAggregator, nodeSet
 	)
 
 	//Take the second of our node sets and unroll it all into a single bitmap
-	nodeSets[1].Each(func(id uint32) (bool, error) {
+	nodeSets[1].Each(func(id uint32) bool {
 		checkSet.Add(id)
 		idCardinality := groupExpansions.Cardinality(id)
 		idCardinalityCount := getCardinalityCount(id, idCardinality, cardinalityCache)
@@ -426,14 +426,14 @@ func CalculateCrossProductBitmaps(groupExpansions impact.PathAggregator, nodeSet
 			checkSet.Or(idCardinality.(cardinality.Duplex[uint32]))
 		}
 
-		return true, nil
+		return true
 	})
 
 	//If we have more than 2 bitmaps, we need to AND everything together
 	if len(nodeSets) > 2 {
 		for i := 2; i < len(nodeSets); i++ {
 			tempSet := cardinality.NewBitmap32()
-			nodeSets[i].Each(func(id uint32) (bool, error) {
+			nodeSets[i].Each(func(id uint32) bool {
 				tempSet.Add(id)
 				idCardinality := groupExpansions.Cardinality(id)
 				idCardinalityCount := getCardinalityCount(id, idCardinality, cardinalityCache)
@@ -442,7 +442,7 @@ func CalculateCrossProductBitmaps(groupExpansions impact.PathAggregator, nodeSet
 					tempSet.Or(idCardinality.(cardinality.Duplex[uint32]))
 				}
 
-				return true, nil
+				return true
 			})
 
 			checkSet.And(tempSet)
@@ -451,7 +451,7 @@ func CalculateCrossProductBitmaps(groupExpansions impact.PathAggregator, nodeSet
 
 	//checkSet should have all the valid principals from all other sets at this point
 	//Check first degree principals in our reference set first
-	nodeSets[0].Each(func(id uint32) (bool, error) {
+	nodeSets[0].Each(func(id uint32) bool {
 		if checkSet.Contains(id) {
 			resultEntities.Add(id)
 		} else {
@@ -463,21 +463,21 @@ func CalculateCrossProductBitmaps(groupExpansions impact.PathAggregator, nodeSet
 			}
 		}
 
-		return true, nil
+		return true
 	})
 
 	tempMap := map[uint32]uint64{}
 	//Find all the groups in our secondary targets and map them to their cardinality in our expansions
 	//Saving off to a map to prevent multiple lookups on the expansions
 	//Unhandled error here is irrelevant, we can never return an error
-	unrollSet.Each(func(id uint32) (bool, error) {
+	unrollSet.Each(func(id uint32) bool {
 		//If group expansions contains this ID and its cardinality is > 0, it's a group/localgroup
 		idCardinality := groupExpansions.Cardinality(id).Cardinality()
 		if idCardinality > 0 {
 			tempMap[id] = idCardinality
 		}
 
-		return true, nil
+		return true
 	})
 
 	//Save the map keys to a new slice, this represents our list of groups in the expansion
@@ -509,12 +509,12 @@ func CalculateCrossProductBitmaps(groupExpansions impact.PathAggregator, nodeSet
 		}
 	}
 
-	unrollSet.Each(func(remainder uint32) (bool, error) {
+	unrollSet.Each(func(remainder uint32) bool {
 		if checkSet.Contains(remainder) {
 			resultEntities.Add(remainder)
 		}
 
-		return true, nil
+		return true
 	})
 
 	return resultEntities
@@ -746,6 +746,7 @@ func ADCSESC6aPath4Pattern(domainId graph.ID, enterpriseCAs cardinality.Duplex[u
 
 func GetADCSESC6aEdgeComposition(ctx context.Context, db graph.Database, edge *graph.Relationship) (graph.PathSet, error) {
 	var (
+		closureErr           error
 		startNode            *graph.Node
 		traversalInst        = traversal.New(db, analysis.MaximumDatabaseParallelWorkers)
 		lock                 = &sync.Mutex{}
@@ -844,8 +845,7 @@ func GetADCSESC6aEdgeComposition(ctx context.Context, db graph.Database, edge *g
 		log.Warnf("unable to access property %s for node with id %d: %v", common.Email.String(), startNode.ID, err)
 	}
 
-	if err := certTemplates.Each(func(value uint32) (bool, error) {
-
+	certTemplates.Each(func(value uint32) bool {
 		var certTemplate *graph.Node
 
 		if err := db.ReadTransaction(ctx, func(tx graph.Transaction) error {
@@ -856,7 +856,8 @@ func GetADCSESC6aEdgeComposition(ctx context.Context, db graph.Database, edge *g
 				return nil
 			}
 		}); err != nil {
-			return false, err
+			closureErr = fmt.Errorf("could not fetch cert template node: %w", err)
+			return false
 		}
 
 		schemaVersion, err := certTemplate.Properties.Get(ad.SchemaVersion.String()).Float64()
@@ -881,7 +882,6 @@ func GetADCSESC6aEdgeComposition(ctx context.Context, db graph.Database, edge *g
 		}
 
 		for _, segment := range certTemplateSegments[graph.ID(value)] {
-
 			if startNode.Kinds.ContainsOneOf(ad.User) {
 				if subjectAltRequireDNS || subjectAltRequireDomainDNS {
 					continue
@@ -905,21 +905,20 @@ func GetADCSESC6aEdgeComposition(ctx context.Context, db graph.Database, edge *g
 
 		}
 
-		return true, nil
-	}); err != nil {
-		return paths, err
+		return true
+	})
+
+	if closureErr != nil {
+		return paths, closureErr
 	}
 
 	if paths.Len() > 0 {
-		if err := enterpriseCAs.Each(func(value uint32) (bool, error) {
+		enterpriseCAs.Each(func(value uint32) bool {
 			for _, segment := range enterpriseCASegments[graph.ID(value)] {
 				paths.AddPath(segment.Path())
 			}
-			return true, nil
-
-		}); err != nil {
-			return paths, err
-		}
+			return true
+		})
 	}
 
 	return paths, nil
@@ -1226,13 +1225,15 @@ func GetADCSESC1EdgeComposition(ctx context.Context, db graph.Database, edge *gr
 	path1EnterpriseCAs.And(path2EnterpriseCAs)
 
 	// Render paths from the segments
-	return paths, path1EnterpriseCAs.Each(func(value uint32) (bool, error) {
+	path1EnterpriseCAs.Each(func(value uint32) bool {
 		for _, segment := range candidateSegments[graph.ID(value)] {
 			paths.AddPath(segment.Path())
 		}
 
-		return true, nil
+		return true
 	})
+
+	return paths, nil
 }
 
 func getGoldenCertEdgeComposition(tx graph.Transaction, edge *graph.Relationship) (graph.PathSet, error) {

packages/go/analysis/ad/adcs.go

@@ -79,15 +79,15 @@ func PostADCSESC1(ctx context.Context, tx graph.Transaction, outC chan<- analysi
 			}
 		}
 
-		results.Each(func(value uint32) (bool, error) {
+		results.Each(func(value uint32) bool {
 			if !channels.Submit(ctx, outC, analysis.CreatePostRelationshipJob{
 				FromID: graph.ID(value),
 				ToID:   domain.ID,
 				Kind:   ad.ADCSESC1,
 			}) {
-				return false, nil
+				return false
 			} else {
-				return true, nil
+				return true
 			}
 		})
 
@@ -170,15 +170,15 @@ func PostADCSESC3(ctx context.Context, tx graph.Transaction, outC chan<- analysi
 		}
 	}
 
-	results.Each(func(value uint32) (bool, error) {
+	results.Each(func(value uint32) bool {
 		if !channels.Submit(ctx, outC, analysis.CreatePostRelationshipJob{
 			FromID: graph.ID(value),
 			ToID:   domain.ID,
 			Kind:   ad.ADCSESC3,
 		}) {
-			return false, nil
+			return false
 		} else {
-			return true, nil
+			return true
 		}
 	})
 
@@ -329,19 +329,17 @@ func PostADCSESC6a(ctx context.Context, tx graph.Transaction, outC chan<- analys
 			}
 		}
 
-		if err := filterTempResultsForESC6(tx, tempResults, groupExpansions, validCertTemplates, cache).Each(func(value uint32) (bool, error) {
+		filterTempResultsForESC6(tx, tempResults, groupExpansions, validCertTemplates, cache).Each(func(value uint32) bool {
 			if !channels.Submit(ctx, outC, analysis.CreatePostRelationshipJob{
 				FromID: graph.ID(value),
 				ToID:   domain.ID,
 				Kind:   ad.ADCSESC6a,
 			}) {
-				return false, nil
+				return false
 			} else {
-				return true, nil
+				return true
 			}
-		}); err != nil {
-			return err
-		}
+		})
 	}
 	return nil
 }

packages/go/analysis/ad/esc10.go

@@ -19,6 +19,7 @@ package ad
 import (
 	"context"
 	"errors"
+
 	"github.com/specterops/bloodhound/analysis"
 	"github.com/specterops/bloodhound/analysis/impact"
 	"github.com/specterops/bloodhound/dawgs/cardinality"
@@ -103,15 +104,15 @@ func PostADCSESC10a(ctx context.Context, tx graph.Transaction, outC chan<- analy
 			}
 		}
 
-		results.Each(func(value uint32) (bool, error) {
+		results.Each(func(value uint32) bool {
 			if !channels.Submit(ctx, outC, analysis.CreatePostRelationshipJob{
 				FromID: graph.ID(value),
 				ToID:   domain.ID,
 				Kind:   ad.ADCSESC10a,
 			}) {
-				return false, nil
+				return false
 			} else {
-				return true, nil
+				return true
 			}
 		})