Threat intel monitor implementation #1092
Conversation
Signed-off-by: Chase Engelbrecht <engechas@amazon.com>
* notification for alerting in correlation * correlation alerts mapping change * working code Signed-off-by: Riya Saxena <riysaxen@amazon.com> * alertsInCorrelation without notifciations Signed-off-by: Riya Saxena <riysaxen@amazon.com> * alertsInCorrelation without notifciations Signed-off-by: Riya Saxena <riysaxen@amazon.com> * alertsInCorrelation without notifciations Signed-off-by: Riya Saxena <riysaxen@amazon.com> * alerts in correlations notification service added Signed-off-by: Riya Saxena <riysaxen@amazon.com> * addressing the comments Signed-off-by: Riya Saxena <riysaxen@amazon.com> * addressing the comments Signed-off-by: Riya Saxena <riysaxen@amazon.com> * address the design changes discussed Signed-off-by: Riya Saxena <riysaxen@amazon.com> * address the design changes discussed Signed-off-by: Riya Saxena <riysaxen@amazon.com> * fixed tests Signed-off-by: Riya Saxena <riysaxen@amazon.com> --------- Signed-off-by: Riya <69919272+riysaxen-amzn@users.noreply.github.com> Signed-off-by: Riya Saxena <riysaxen@amazon.com>
* notification for alerting in correlation * correlation alerts mapping change * working code Signed-off-by: Riya Saxena <riysaxen@amazon.com> * alertsInCorrelation without notifciations Signed-off-by: Riya Saxena <riysaxen@amazon.com> * alertsInCorrelation without notifciations Signed-off-by: Riya Saxena <riysaxen@amazon.com> * alertsInCorrelation without notifciations Signed-off-by: Riya Saxena <riysaxen@amazon.com> * alerts in correlations notification service added Signed-off-by: Riya Saxena <riysaxen@amazon.com> * addressing the comments Signed-off-by: Riya Saxena <riysaxen@amazon.com> * addressing the comments Signed-off-by: Riya Saxena <riysaxen@amazon.com> * getCorrelationAlerts API changes Signed-off-by: Riya Saxena <riysaxen@amazon.com> * APIs added for Alerts in Correlations Signed-off-by: Riya Saxena <riysaxen@amazon.com> * update alerts with an errorMessage when correlationRule is deleted Signed-off-by: Riya Saxena <riysaxen@amazon.com> * address the design changes discussed Signed-off-by: Riya Saxena <riysaxen@amazon.com> * address the design changes discussed Signed-off-by: Riya Saxena <riysaxen@amazon.com> * fixed tests Signed-off-by: Riya Saxena <riysaxen@amazon.com> * minor fixes due to merge Signed-off-by: Riya Saxena <riysaxen@amazon.com> * alerts API changes Signed-off-by: Riya Saxena <riysaxen@amazon.com> * klint fixes Signed-off-by: Riya Saxena <riysaxen@amazon.com> * license headers added Signed-off-by: Riya Saxena <riysaxen@amazon.com> * fixed format violations Signed-off-by: Riya Saxena <riysaxen@amazon.com> --------- Signed-off-by: Riya <69919272+riysaxen-amzn@users.noreply.github.com> Signed-off-by: Riya Saxena <riysaxen@amazon.com>
Signed-off-by: Riya Saxena <riysaxen@amazon.com>
Signed-off-by: Subhobrata Dey <sbcd90@gmail.com>
eirsep
requested review from
amsiglan,
AWSHurneyt,
getsaurabh02,
lezzago,
praveensameneni,
sbcd90,
jowg-amazon,
engechas,
goyamegh and
riysaxen-amzn
as code owners
|
|
||
| final String url = Objects.requireNonNull(BuiltinLogTypeLoader.class.getClassLoader().getResource(BASE_PATH)).toURI().toString(); | ||
| logger.error("SASHANK Path url is {}", pathurl); |
There was a problem hiding this comment.
@eirsep I believe you mentioned offline that there was some old code in this file?
| public class GetIocFindingsAction extends ActionType<GetIocFindingsResponse> { | ||
|
|
||
| public static final GetIocFindingsAction INSTANCE = new GetIocFindingsAction(); | ||
| public static final String NAME = "cluster:admin/opensearch/securityanalytics/ioc/findings/get"; |
There was a problem hiding this comment.
In the ListIOCs API, I have /ioc/ pluralized to /iocs/. Any preference for which to use?
There was a problem hiding this comment.
for rest api naming plural
| * Index name: .opensearch-sap-iocmatch | ||
| * Mapping: /mappings/ioc_match_mapping.json |
There was a problem hiding this comment.
Is this comment up to date?
This is a base I've been using for IOC-related indexes if you're interested.
https://github.com/opensearch-project/security-analytics/blob/feature/threat_intel/src/main/java/org/opensearch/securityanalytics/services/STIX2IOCFeedStore.java#L49
| Monitor monitor = iocScanContext.getMonitor(); | ||
|
|
||
| long startTime = System.currentTimeMillis(); | ||
| // log.debug("beginning to scan IoC's") |
| Map<String, List<String>> iocTypeToIndices); | ||
|
|
||
| /** | ||
| * For each doc, we extract the list of |
| public class SaIoCScanService extends IoCScanService<SearchHit> { | ||
|
|
||
| private static final Logger log = LogManager.getLogger(SaIoCScanService.class); | ||
| public static final int MAX_TERMS = 65536; //make ioc index setting based. use same setting value to create index |
There was a problem hiding this comment.
Nitpick: Not marked as a TODO. Just mentioning this so it's easier to spot.
| @@ -268,6 +268,9 @@ public static SATIFSourceConfig parse(XContentParser xcp, String id, Long versio | |||
| case NAME_FIELD: | |||
| name = xcp.text(); | |||
| break; | |||
| case VERSION_FIELD: | |||
There was a problem hiding this comment.
Should we be parsing this from the SearchHit/some other source? It looks like we don't do that for detectors, and we instead take in the version as a parameter in the parse function.
https://github.com/opensearch-project/security-analytics/blob/feature/threat_intel/src/main/java/org/opensearch/securityanalytics/model/Detector.java#L315
There was a problem hiding this comment.
i am blocked on the actual fix so i made an adhoc.. will rebased from feature branch
|
|
||
| @Override | ||
| public String getName() { | ||
| return "get_ioc_findings_action_sa"; |
There was a problem hiding this comment.
Is this PR intended to have some assets from this PR?
https://github.com/opensearch-project/security-analytics/pull/1093/files#diff-c79121f958035e722c3a1b9f151ea28d5d9905080d907b8f53ae12005c0d100fR30
There was a problem hiding this comment.
i am blocked on the PR so i integrated.. will rebased that PR's commits.
...va/org/opensearch/securityanalytics/threatIntel/transport/TransportGetIocFindingsAction.java
Show resolved
Hide resolved
|
|
||
| private void indexIocs(List<String> iocVals, String iocIndexName, int i1, String configId) throws IOException { | ||
| String iocId = iocIndexName + i1; | ||
| STIX2IOC stix2IOC = new STIX2IOC( |
There was a problem hiding this comment.
For reference, I have some static functions for creating random IOCs in this helper class.
https://github.com/opensearch-project/security-analytics/blob/feature/threat_intel/src/test/java/org/opensearch/securityanalytics/util/STIX2IOCGenerator.java#L77
| listener.onResponse(null); | ||
| return; | ||
| } | ||
| log.error("Failed to create security analytics threat intel job index", e); |
There was a problem hiding this comment.
nit: log line specified job index
| } | ||
|
|
||
| public void createIndexIfNotExists(final ActionListener<Void> listener) { | ||
| // check if job index exists |
There was a problem hiding this comment.
nit: job index comment
| import java.util.List; | ||
| import java.util.Map; | ||
|
|
||
| public class IocScanContext<Data> { |
There was a problem hiding this comment.
Does IocScanContext need serialization/deserialization?
There was a problem hiding this comment.
no
just dto between classes - single node operation too
| } | ||
| } | ||
|
|
||
| abstract void saveIocs(List<IocFinding> iocs, BiConsumer<List<IocFinding>, Exception> callback, Monitor monitor); |
There was a problem hiding this comment.
saveIocFindings might be a better name to differentiate saving the actual iocs
Signed-off-by: Subhobrata Dey <sbcd90@gmail.com>
…generic entity to re-use for threat intel alert Signed-off-by: Surya Sashank Nistala <snistala@amazon.com>
Signed-off-by: Surya Sashank Nistala <snistala@amazon.com>
| public class GetCorrelationAlertsAction extends ActionType<GetCorrelationAlertsResponse> { | ||
|
|
||
| public static final GetCorrelationAlertsAction INSTANCE = new GetCorrelationAlertsAction(); | ||
| public static final String NAME = "cluster:admin/opensearch/securityanalytics/correlationAlerts/get"; |
There was a problem hiding this comment.
Nitpick: Should correlationAlerts be all lowercase like securityanalytics?
There was a problem hiding this comment.
not from my PR
merged main branch
| // Convert CorrelationAlert to a map | ||
| try { | ||
| XContentBuilder builder = XContentFactory.jsonBuilder().startObject(); | ||
| builder.field("correlated_finding_ids", correlationAlert.getCorrelatedFindingIds()); |
There was a problem hiding this comment.
Nitpick: I think you have constants for the field names that could be used instead to help maintain consistency.
There was a problem hiding this comment.
not from my PR
merged main branch
| // Iterate through the search hits | ||
| for (SearchHit hit : searchResponse.getHits().getHits()) { | ||
| // Construct a script to update the document with the new state and acknowledgedTime | ||
| // Construct a script to update the document with the new state and acknowledgedTime |
There was a problem hiding this comment.
Nitpick: Duplicated comment.
There was a problem hiding this comment.
not from my PR
merged main branch
| // Construct a script to update the document with the new state and error_message | ||
| Script script = new Script(ScriptType.INLINE, "painless", | ||
| "ctx._source.state = params.state; ctx._source.error_message = params.error_message", | ||
| Map.of("state", Alert.State.ERROR, "error_message", "The rule associated to this Alert is deleted")); |
There was a problem hiding this comment.
Just to clarify, is this the only error case we expect, or is there a more dynamic way to construct the error_message?
There was a problem hiding this comment.
not from my PR
merged main branch
| if (getNotificationConfigResponse.getStatus() == RestStatus.OK) { | ||
| getNotificationConfigResponse = configResp; | ||
| } else { | ||
| logger.error("Successfully sent a notification, Notification Event: " + getNotificationConfigResponse); |
There was a problem hiding this comment.
To clarify, is the error message here correct? Looks like this is a fail case.
There was a problem hiding this comment.
not from my PR
merged main branch
| import java.util.Objects; | ||
| import java.util.stream.Collectors; | ||
|
|
||
| public class CorrelationRuleTrigger implements Writeable, ToXContentObject { |
There was a problem hiding this comment.
Nitpick: Should this extend the trigger interface?
https://github.com/opensearch-project/common-utils/blob/main/src/main/kotlin/org/opensearch/commons/alerting/model/Trigger.kt
There was a problem hiding this comment.
not from my PR
merged main branch
| ActionListener<SearchHits> listener) { | ||
|
|
||
| if (prevSeqNo != null && prevSeqNo.equals(maxSeqNo) && maxSeqNo != 0L) { | ||
| log.debug("Seqquence number unchanged."); |
There was a problem hiding this comment.
Nitpick: Typo in "Sequence".
| } | ||
| ), indices.toArray(new String[0]) | ||
| ); | ||
| // log.debug("Delete old IOC indices by age"); |
There was a problem hiding this comment.
Nitpick: Are these still needed now that PR #1094 is merged?
| @@ -330,72 +334,6 @@ public void deleteTIFSourceConfig( | |||
| )); | |||
| } | |||
|
|
|||
| public void deleteAllOldIocIndices(List<String> indicesToDelete) { | |||
| ) { | ||
|
|
||
| this.id = id != null ? id : NO_ID; | ||
| this.version = version != 0 ? version : NO_VERSION; |
There was a problem hiding this comment.
Is it possible for version to be null?
| @@ -108,4 +108,34 @@ public static IocWithFeeds parse(XContentParser xcp) throws IOException { | |||
| public static IocWithFeeds readFrom(StreamInput sin) throws IOException { | |||
| return new IocWithFeeds(sin); | |||
| } | |||
|
|
|||
| @Override | |||
| public int hashCode() { | |||
There was a problem hiding this comment.
how is this hashcode calculated? Can we use Objects.hash?
| } | ||
| } | ||
|
|
||
| public void bulkIndexEntities(List<Entity> entityList, |
There was a problem hiding this comment.
why does bulkIndexEntities need to be overloaded?
There was a problem hiding this comment.
one is for only new entities and
one for create new and update old entities
Signed-off-by: Surya Sashank Nistala <snistala@amazon.com>
| )); | ||
| } | ||
|
|
||
| private void getAlerts(List<String> monitorIds, |
There was a problem hiding this comment.
not blocking but should logic be moved to a service class outside of transport?
Signed-off-by: Surya Sashank Nistala <snistala@amazon.com>
eirsep
merged commit ef54c62
into
opensearch-project:feature/threat_intel
* ioc scan business logic * add search ioc findings api Signed-off-by: Subhobrata Dey <sbcd90@gmail.com> * refactor iocFinding model and service to pull out CRUD operations to generic entity to re-use for threat intel alert Signed-off-by: Surya Sashank Nistala <snistala@amazon.com> * threat intel alert model and crud operations Signed-off-by: Surya Sashank Nistala <snistala@amazon.com> * threat intel trigger execution logic * wire in ioc findings * get threat intel monitor alerts API Signed-off-by: Surya Sashank Nistala <snistala@amazon.com> * revert commented out code Signed-off-by: Surya Sashank Nistala <snistala@amazon.com> --------- Signed-off-by: Chase Engelbrecht <engechas@amazon.com> Signed-off-by: Riya <69919272+riysaxen-amzn@users.noreply.github.com> Signed-off-by: Riya Saxena <riysaxen@amazon.com> Signed-off-by: Subhobrata Dey <sbcd90@gmail.com> Signed-off-by: Surya Sashank Nistala <snistala@amazon.com> Co-authored-by: Chase <62891993+engechas@users.noreply.github.com> Co-authored-by: Riya <69919272+riysaxen-amzn@users.noreply.github.com> Co-authored-by: Subhobrata Dey <sbcd90@gmail.com> Signed-off-by: AWSHurneyt <hurneyt@amazon.com>
Description
Threat intel monitor is a job that runs periodically to scan the configured customer data for malicious indicators. It's corpus of malicious indicators is a list of system indices containing threat intel data curated from the configured threat intelligence source configs.
We refer threat intel source configs that are in AVAILABLE or REFRESHING state.
we fetch the ioc system indices mentioned in these TIF source configs for corpus of indicators.
Threat intel monitor does the following steps when it runs:
TODO: pending alert triggers and notifications
Issues Resolved
[List any issues this PR will resolve]
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.