-
Notifications
You must be signed in to change notification settings - Fork 20
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Initial support for Navigator layers in SOC #699
base: 2.4/dev
Are you sure you want to change the base?
Conversation
} | ||
|
||
func (nav *Navigator) IsRunning() bool { | ||
return nav.stopChan != nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing sets stopChan to nil. If the constructor is used, the channel is never nil. This will be true before starting the module and after stopping it.
select { | ||
case <-ctx.Done(): | ||
if ctx.Err() == context.DeadlineExceeded { | ||
logger.WithField("processed_events", totalProcessed).Warn("alert extraction timed out after 3 minutes") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't hardcode "3 minutes" here if timeout is configurable.
} | ||
|
||
logger.WithFields(log.Fields{ | ||
"rawQuery": criteria.RawQuery, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be ok as-is, but there's a small chance that some of these generic field names could conflict with existing SOC log lines that store other values / types in them. To be safe, you could prefix all log line fields in this class with nav
or something to keep these fields specific to navigator layers.
}).Info("Processing batch of Suricata alerts for navigator layer") | ||
|
||
for _, event := range searchResult.Events { | ||
techniqueIDs, ok := event.Payload["rule.metadata.mitre_technique_id"].([]interface{}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are two things happening here that are error prone:
- The dereferencing of the Payload key
- The cast to array of interfaces
Does theok
var capture errors on both of these scenarios? This is a good unit test scenario.
} | ||
|
||
func (nav *Navigator) PrerequisiteModules() []string { | ||
return nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically, this module is dependent upon the Elastic module, and perhaps the two detection engine modules. So you could have this return the names of those modules so that if any aren't configured in SOC this will error out up front and not during the timer loop. It's unlikely for anyone to run into this but this prereq function was built for this concept.
logger.WithError(err).Error("Failed to generate navigator layers") | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's good practice to log an Info line when exiting a long-running loop function. Helps for troubleshooting, in case the loop crashed or got stuck on a query call.
|
||
// Get the sort values from the last event for the next search | ||
lastEvent := searchResult.Events[len(searchResult.Events)-1] | ||
criteria.SearchAfter = lastEvent.Sort |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the Sort
field the correct value to use for pagination? I haven't looked at the ES API in a while so it might be. Just seems odd to use a sort term for pagination.
Adds support for generating ATT&CK Navigator layers within a new SOC module,
navigator