Bed 4752 (#873)

* chore: updated log errors to warns * missed comment * ran prepare-for-codereview * updates from peer review * updated file to use errors package instead of bloodhounf/errors * reran just prepare for code review * prepare for review
SpecterOps · Sep 25, 2024 · ab521ab · ab521ab
1 parent 08dae9f
commit ab521ab
Show file tree

Hide file tree

Showing 17 changed files with 217 additions and 154 deletions.
diff --git a/cmd/api/src/api/saml/saml.go b/cmd/api/src/api/saml/saml.go
@@ -200,8 +200,7 @@ func assertionFindString(assertion *saml.Assertion, names ...string) (string, er
 							return value.Value, nil
 						}
 					}
-
-					log.Errorf("[SAML] Found attribute values for attribute %s however none of the values have an XML type of %s. Choosing the first value.", bhsaml.ObjectIDAttributeNameFormat, bhsaml.XMLTypeString)
+					log.Warnf("[SAML] Found attribute values for attribute %s however none of the values have an XML type of %s. Choosing the first value.", bhsaml.ObjectIDAttributeNameFormat, bhsaml.XMLTypeString)
 					return attribute.Values[0].Value, nil
 				}
 			}

diff --git a/cmd/api/src/api/v2/audit_test.go b/cmd/api/src/api/v2/audit_test.go
@@ -243,122 +243,122 @@ func TestResources_ListAuditLogs_Filtered(t *testing.T) {
 }
 
 func TestResources_ListAuditLogs_SkipAndOffset(t *testing.T) {
-    var (
-        mockCtrl  = gomock.NewController(t)
-        mockDB    = mocks.NewMockDatabase(mockCtrl)
-        resources = v2.Resources{DB: mockDB}
-    )
-    defer mockCtrl.Finish()
-
-    mockDB.EXPECT().ListAuditLogs(gomock.Any(), gomock.Any(), gomock.Any(), 10, gomock.Any(), "", model.SQLFilter{}).Return(model.AuditLogs{}, 1000, nil)
-
-    endpoint := "/api/v2/audit"
-
-    if req, err := http.NewRequest("GET", endpoint, nil); err != nil {
-        t.Fatal(err)
-    } else {
-        q := url.Values{}
-        q.Add("skip", "10")
-        q.Add("offset", "20")
-
-        req.Header.Set(headers.ContentType.String(), mediatypes.ApplicationJson.String())
-        req.URL.RawQuery = q.Encode()
-
-        router := mux.NewRouter()
-        router.HandleFunc(endpoint, resources.ListAuditLogs).Methods("GET")
-
-        response := httptest.NewRecorder()
-        router.ServeHTTP(response, req)
-        require.Equal(t, http.StatusOK, response.Code)
-    }
+	var (
+		mockCtrl  = gomock.NewController(t)
+		mockDB    = mocks.NewMockDatabase(mockCtrl)
+		resources = v2.Resources{DB: mockDB}
+	)
+	defer mockCtrl.Finish()
+
+	mockDB.EXPECT().ListAuditLogs(gomock.Any(), gomock.Any(), gomock.Any(), 10, gomock.Any(), "", model.SQLFilter{}).Return(model.AuditLogs{}, 1000, nil)
+
+	endpoint := "/api/v2/audit"
+
+	if req, err := http.NewRequest("GET", endpoint, nil); err != nil {
+		t.Fatal(err)
+	} else {
+		q := url.Values{}
+		q.Add("skip", "10")
+		q.Add("offset", "20")
+
+		req.Header.Set(headers.ContentType.String(), mediatypes.ApplicationJson.String())
+		req.URL.RawQuery = q.Encode()
+
+		router := mux.NewRouter()
+		router.HandleFunc(endpoint, resources.ListAuditLogs).Methods("GET")
+
+		response := httptest.NewRecorder()
+		router.ServeHTTP(response, req)
+		require.Equal(t, http.StatusOK, response.Code)
+	}
 }
 
 func TestResources_ListAuditLogs_OnlyOffset(t *testing.T) {
-    var (
-        mockCtrl  = gomock.NewController(t)
-        mockDB    = mocks.NewMockDatabase(mockCtrl)
-        resources = v2.Resources{DB: mockDB}
-    )
-    defer mockCtrl.Finish()
+	var (
+		mockCtrl  = gomock.NewController(t)
+		mockDB    = mocks.NewMockDatabase(mockCtrl)
+		resources = v2.Resources{DB: mockDB}
+	)
+	defer mockCtrl.Finish()
 
-    mockDB.EXPECT().ListAuditLogs(gomock.Any(), gomock.Any(), gomock.Any(), 20, gomock.Any(), "", model.SQLFilter{}).Return(model.AuditLogs{}, 1000, nil)
+	mockDB.EXPECT().ListAuditLogs(gomock.Any(), gomock.Any(), gomock.Any(), 20, gomock.Any(), "", model.SQLFilter{}).Return(model.AuditLogs{}, 1000, nil)
 
-    endpoint := "/api/v2/audit"
+	endpoint := "/api/v2/audit"
 
-    if req, err := http.NewRequest("GET", endpoint, nil); err != nil {
-        t.Fatal(err)
-    } else {
-        q := url.Values{}
-        q.Add("offset", "20")
+	if req, err := http.NewRequest("GET", endpoint, nil); err != nil {
+		t.Fatal(err)
+	} else {
+		q := url.Values{}
+		q.Add("offset", "20")
 
-        req.Header.Set(headers.ContentType.String(), mediatypes.ApplicationJson.String())
-        req.URL.RawQuery = q.Encode()
+		req.Header.Set(headers.ContentType.String(), mediatypes.ApplicationJson.String())
+		req.URL.RawQuery = q.Encode()
 
-        router := mux.NewRouter()
-        router.HandleFunc(endpoint, resources.ListAuditLogs).Methods("GET")
+		router := mux.NewRouter()
+		router.HandleFunc(endpoint, resources.ListAuditLogs).Methods("GET")
 
-        response := httptest.NewRecorder()
-        router.ServeHTTP(response, req)
-        require.Equal(t, http.StatusOK, response.Code)
-    }
+		response := httptest.NewRecorder()
+		router.ServeHTTP(response, req)
+		require.Equal(t, http.StatusOK, response.Code)
+	}
 }
 
 func TestResources_ListAuditLogs_OnlySkip(t *testing.T) {
-    var (
-        mockCtrl  = gomock.NewController(t)
-        mockDB    = mocks.NewMockDatabase(mockCtrl)
-        resources = v2.Resources{DB: mockDB}
-    )
-    defer mockCtrl.Finish()
-
-    // Expect skip to be 5 (from "skip" parameter)
-    mockDB.EXPECT().ListAuditLogs(gomock.Any(), gomock.Any(), gomock.Any(), 5, gomock.Any(), gomock.Any(), gomock.Any()).Return(model.AuditLogs{}, 1000, nil)
-
-    endpoint := "/api/v2/audit"
-
-    if req, err := http.NewRequest("GET", endpoint, nil); err != nil {
-        t.Fatal(err)
-    } else {
-        q := url.Values{}
-        q.Add("skip", "5")
-
-        req.Header.Set(headers.ContentType.String(), mediatypes.ApplicationJson.String())
-        req.URL.RawQuery = q.Encode()
-
-        router := mux.NewRouter()
-        router.HandleFunc(endpoint, resources.ListAuditLogs).Methods("GET")
-
-        response := httptest.NewRecorder()
-        router.ServeHTTP(response, req)
-        require.Equal(t, http.StatusOK, response.Code)
-    }
+	var (
+		mockCtrl  = gomock.NewController(t)
+		mockDB    = mocks.NewMockDatabase(mockCtrl)
+		resources = v2.Resources{DB: mockDB}
+	)
+	defer mockCtrl.Finish()
+
+	// Expect skip to be 5 (from "skip" parameter)
+	mockDB.EXPECT().ListAuditLogs(gomock.Any(), gomock.Any(), gomock.Any(), 5, gomock.Any(), gomock.Any(), gomock.Any()).Return(model.AuditLogs{}, 1000, nil)
+
+	endpoint := "/api/v2/audit"
+
+	if req, err := http.NewRequest("GET", endpoint, nil); err != nil {
+		t.Fatal(err)
+	} else {
+		q := url.Values{}
+		q.Add("skip", "5")
+
+		req.Header.Set(headers.ContentType.String(), mediatypes.ApplicationJson.String())
+		req.URL.RawQuery = q.Encode()
+
+		router := mux.NewRouter()
+		router.HandleFunc(endpoint, resources.ListAuditLogs).Methods("GET")
+
+		response := httptest.NewRecorder()
+		router.ServeHTTP(response, req)
+		require.Equal(t, http.StatusOK, response.Code)
+	}
 }
 
 func TestResources_ListAuditLogs_InvalidSkip(t *testing.T) {
-    var (
-        mockCtrl  = gomock.NewController(t)
-        mockDB    = mocks.NewMockDatabase(mockCtrl)
-        resources = v2.Resources{DB: mockDB}
-    )
-    defer mockCtrl.Finish()
-
-    endpoint := "/api/v2/audit"
-
-    if req, err := http.NewRequest("GET", endpoint, nil); err != nil {
-        t.Fatal(err)
-    } else {
-        q := url.Values{}
-        q.Add("skip", "invalid")
-
-        req.Header.Set(headers.ContentType.String(), mediatypes.ApplicationJson.String())
-        req.URL.RawQuery = q.Encode()
-
-        router := mux.NewRouter()
-        router.HandleFunc(endpoint, resources.ListAuditLogs).Methods("GET")
-
-        response := httptest.NewRecorder()
-        router.ServeHTTP(response, req)
-        require.Equal(t, http.StatusBadRequest, response.Code)
-        require.Contains(t, response.Body.String(), "query parameter \\\"skip\\\" is malformed")
-    }
-}
+	var (
+		mockCtrl  = gomock.NewController(t)
+		mockDB    = mocks.NewMockDatabase(mockCtrl)
+		resources = v2.Resources{DB: mockDB}
+	)
+	defer mockCtrl.Finish()
+
+	endpoint := "/api/v2/audit"
+
+	if req, err := http.NewRequest("GET", endpoint, nil); err != nil {
+		t.Fatal(err)
+	} else {
+		q := url.Values{}
+		q.Add("skip", "invalid")
+
+		req.Header.Set(headers.ContentType.String(), mediatypes.ApplicationJson.String())
+		req.URL.RawQuery = q.Encode()
+
+		router := mux.NewRouter()
+		router.HandleFunc(endpoint, resources.ListAuditLogs).Methods("GET")
+
+		response := httptest.NewRecorder()
+		router.ServeHTTP(response, req)
+		require.Equal(t, http.StatusBadRequest, response.Code)
+		require.Contains(t, response.Body.String(), "query parameter \\\"skip\\\" is malformed")
+	}
+}
diff --git a/cmd/api/src/api/v2/helpers.go b/cmd/api/src/api/v2/helpers.go
@@ -58,15 +58,15 @@ func ParseSkipQueryParameter(params url.Values, defaultValue int) (int, error) {
 }
 
 func ParseSkipQueryParameterWithKey(params url.Values, key string, defaultValue int) (int, error) {
-    if param := params.Get(key); param == "" {
-        return defaultValue, nil
-    } else if skip, err := strconv.Atoi(param); err != nil {
-        return 0, fmt.Errorf("error converting %s value %v to int: %v", key, param, err)
-    } else if skip < 0 {
-        return 0, fmt.Errorf(utils.ErrorInvalidSkip, skip)
-    } else {
-        return skip, nil
-    }
+	if param := params.Get(key); param == "" {
+		return defaultValue, nil
+	} else if skip, err := strconv.Atoi(param); err != nil {
+		return 0, fmt.Errorf("error converting %s value %v to int: %v", key, param, err)
+	} else if skip < 0 {
+		return 0, fmt.Errorf(utils.ErrorInvalidSkip, skip)
+	} else {
+		return skip, nil
+	}
 }
 
 func ParseLimitQueryParameter(params url.Values, defaultValue int) (int, error) {

diff --git a/cmd/api/src/bootstrap/initializer.go b/cmd/api/src/bootstrap/initializer.go
@@ -39,7 +39,7 @@ type Initializer[DBType database.Database, GraphType graph.Database] struct {
 	Configuration       config.Configuration
 	PreMigrationDaemons InitializerLogic[DBType, GraphType]
 	Entrypoint          InitializerLogic[DBType, GraphType]
-	DBConnector   DatabaseConstructor[DBType, GraphType]
+	DBConnector         DatabaseConstructor[DBType, GraphType]
 }
 
 func (s Initializer[DBType, GraphType]) Launch(parentCtx context.Context, handleSignals bool) error {

diff --git a/cmd/api/src/cmd/bhapi/main.go b/cmd/api/src/cmd/bhapi/main.go
@@ -64,10 +64,10 @@ func main() {
 		log.Fatalf("Unable to read configuration %s: %v", configFilePath, err)
 	} else {
 		initializer := bootstrap.Initializer[*database.BloodhoundDB, *graph.DatabaseSwitch]{
-			Configuration: cfg,
-			DBConnector:   services.ConnectDatabases,
+			Configuration:       cfg,
+			DBConnector:         services.ConnectDatabases,
 			PreMigrationDaemons: services.PreMigrationDaemons,
-			Entrypoint:    services.Entrypoint,
+			Entrypoint:          services.Entrypoint,
 		}
 
 		if err := initializer.Launch(context.Background(), true); err != nil {

diff --git a/cmd/api/src/daemons/datapipe/azure_convertors.go b/cmd/api/src/daemons/datapipe/azure_convertors.go
@@ -18,6 +18,7 @@ package datapipe
 
 import (
 	"encoding/json"
+	"errors"
 	"fmt"
 	"strings"
 
@@ -190,7 +191,9 @@ func convertAzureAppOwner(raw json.RawMessage, converted *ConvertedAzureData) {
 			)
 			if err := json.Unmarshal(raw.Owner, &owner); err != nil {
 				log.Errorf(SerialError, "app owner", err)
-			} else if ownerType, err := ein.ExtractTypeFromDirectoryObject(owner); err != nil {
+			} else if ownerType, err := ein.ExtractTypeFromDirectoryObject(owner); errors.Is(err, ein.InvalidTypeErr) {
+				log.Warnf(ExtractError, err)
+			} else if err != nil {
 				log.Errorf(ExtractError, err)
 			} else {
 				converted.RelProps = append(converted.RelProps, ein.ConvertAzureOwnerToRel(owner, ownerType, azure.App, data.AppId))
@@ -235,7 +238,9 @@ func convertAzureDeviceOwner(raw json.RawMessage, converted *ConvertedAzureData)
 			)
 			if err := json.Unmarshal(raw.Owner, &owner); err != nil {
 				log.Errorf(SerialError, "device owner", err)
-			} else if ownerType, err := ein.ExtractTypeFromDirectoryObject(owner); err != nil {
+			} else if ownerType, err := ein.ExtractTypeFromDirectoryObject(owner); errors.Is(err, ein.InvalidTypeErr) {
+				log.Warnf(ExtractError, err)
+			} else if err != nil {
 				log.Errorf(ExtractError, err)
 			} else {
 				converted.RelProps = append(converted.RelProps, ein.ConvertAzureOwnerToRel(owner, ownerType, azure.Device, data.DeviceId))

diff --git a/cmd/api/src/daemons/datapipe/jobs.go b/cmd/api/src/daemons/datapipe/jobs.go
@@ -19,8 +19,10 @@ package datapipe
 import (
 	"archive/zip"
 	"context"
+	"errors"
 	"fmt"
 	"io"
+	"io/fs"
 	"os"
 
 	"github.com/specterops/bloodhound/bomenc"
@@ -218,7 +220,9 @@ func (s *Daemon) processIngestFile(ctx context.Context, path string, fileType mo
 
 				if err := file.Close(); err != nil {
 					log.Errorf("Error closing ingest file %s: %v", filePath, err)
-				} else if err := os.Remove(filePath); err != nil {
+				} else if err := os.Remove(filePath); errors.Is(err, fs.ErrNotExist) {
+					log.Warnf("Removing ingest file %s: %w", filePath, err)
+				} else if err != nil {
 					log.Errorf("Error removing ingest file %s: %v", filePath, err)
 				}
 			}
@@ -253,7 +257,9 @@ func (s *Daemon) processIngestTasks(ctx context.Context, ingestTasks model.Inges
 			log.Errorf("Failed to fetch job for ingest task %d: %v", ingestTask.ID, err)
 		}
 		total, failed, err := s.processIngestFile(ctx, ingestTask.FileName, ingestTask.FileType)
-		if err != nil {
+		if errors.Is(err, fs.ErrNotExist) {
+			log.Warnf("Did not process ingest task %d with file %s: %v", ingestTask.ID, ingestTask.FileName, err)
+		} else if err != nil {
 			log.Errorf("Failed processing ingest task %d with file %s: %v", ingestTask.ID, ingestTask.FileName, err)
 		}
 

diff --git a/cmd/api/src/queries/graph.go b/cmd/api/src/queries/graph.go
@@ -21,6 +21,7 @@ package queries
 import (
 	"bytes"
 	"context"
+	"errors"
 	"fmt"
 	"net/http"
 	"net/url"
@@ -46,7 +47,6 @@ import (
 	"github.com/specterops/bloodhound/dawgs/graph"
 	"github.com/specterops/bloodhound/dawgs/ops"
 	"github.com/specterops/bloodhound/dawgs/query"
-	"github.com/specterops/bloodhound/errors"
 	"github.com/specterops/bloodhound/graphschema/ad"
 	"github.com/specterops/bloodhound/graphschema/azure"
 	"github.com/specterops/bloodhound/graphschema/common"
@@ -83,7 +83,7 @@ type EntityQueryParameters struct {
 
 func GetEntityObjectIDFromRequestPath(request *http.Request) (string, error) {
 	if id, hasID := mux.Vars(request)["object_id"]; !hasID {
-		return "", errors.Error("no object ID found in request")
+		return "", errors.New("no object ID found in request")
 	} else {
 		return id, nil
 	}
@@ -494,7 +494,7 @@ func (s *GraphQuery) RawCypherQuery(ctx context.Context, pQuery PreparedQuery, i
 			timeoutLog.Str("query cost", fmt.Sprintf("%d", pQuery.complexity.Weight))
 			timeoutLog.Msg("Neo4j timed out while executing cypher query")
 		} else {
-			log.Errorf("RawCypherQuery failed: %v", err)
+			log.Warnf("RawCypherQuery failed: %v", err)
 		}
 		return graphResponse, err
 	}
@@ -636,7 +636,9 @@ func (s *GraphQuery) GetEntityCountResults(ctx context.Context, node *graph.Node
 		go func(delegateKey string, delegate any) {
 			defer waitGroup.Done()
 
-			if result, err := runEntityQuery(ctx, s.Graph, delegate, node, 0, 0); err != nil {
+			if result, err := runEntityQuery(ctx, s.Graph, delegate, node, 0, 0); errors.Is(err, graph.ErrContextTimedOut) {
+				log.Warnf("Running entity query for key %s: %v", delegateKey, err)
+			} else if err != nil {
 				log.Errorf("Error running entity query for key %s: %v", delegateKey, err)
 				data.Store(delegateKey, 0)
 			} else {