Create TIF Source Config API #1046

jowg-amazon · 2024-05-24T00:53:22Z

Description

Preliminary architecture and flow for creating a job index and indexing a source config object.

Issues Resolved

[List any issues this PR will resolve]

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
Commits are signed per the DCO using --signoff

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.

Signed-off-by: Joanne Wang <[email protected]>

eirsep · 2024-05-24T00:58:23Z

src/main/java/org/opensearch/securityanalytics/SecurityAnalyticsPlugin.java

@@ -103,9 +111,11 @@ public class SecurityAnalyticsPlugin extends Plugin implements ActionPlugin, Map
    public static final String FINDINGS_CORRELATE_URI = FINDINGS_BASE_URI + "/correlate";
    public static final String LIST_CORRELATIONS_URI = PLUGINS_BASE_URI + "/correlations";
    public static final String CORRELATION_RULES_BASE_URI = PLUGINS_BASE_URI + "/correlation/rules";
-
+    public static final String TIF_CONFIG_URI = PLUGINS_BASE_URI + "/tif";


tif source?

updated to /tif/source

Signed-off-by: Joanne Wang <[email protected]>

eirsep · 2024-05-24T23:15:48Z

src/main/java/org/opensearch/securityanalytics/SecurityAnalyticsPlugin.java

@@ -103,9 +111,12 @@ public class SecurityAnalyticsPlugin extends Plugin implements ActionPlugin, Map
    public static final String FINDINGS_CORRELATE_URI = FINDINGS_BASE_URI + "/correlate";
    public static final String LIST_CORRELATIONS_URI = PLUGINS_BASE_URI + "/correlations";
    public static final String CORRELATION_RULES_BASE_URI = PLUGINS_BASE_URI + "/correlation/rules";
-
+    public static final String TIF_BASE_URI = PLUGINS_BASE_URI + "/tif";


threat_intel?

changed to threat_intel

eirsep · 2024-05-24T23:16:15Z

src/main/java/org/opensearch/securityanalytics/SecurityAnalyticsPlugin.java

@@ -103,9 +111,12 @@ public class SecurityAnalyticsPlugin extends Plugin implements ActionPlugin, Map
    public static final String FINDINGS_CORRELATE_URI = FINDINGS_BASE_URI + "/correlate";
    public static final String LIST_CORRELATIONS_URI = PLUGINS_BASE_URI + "/correlations";
    public static final String CORRELATION_RULES_BASE_URI = PLUGINS_BASE_URI + "/correlation/rules";
-
+    public static final String TIF_BASE_URI = PLUGINS_BASE_URI + "/tif";
+    public static final String TIF_SOURCE_CONFIG_URI = PLUGINS_BASE_URI + "/tif/source";


threat_intel/source?

changed to threat_intel/source

eirsep · 2024-05-24T23:18:00Z

src/main/java/org/opensearch/securityanalytics/SecurityAnalyticsPlugin.java

@@ -129,6 +140,9 @@ public class SecurityAnalyticsPlugin extends Plugin implements ActionPlugin, Map
    private BuiltinLogTypeLoader builtinLogTypeLoader;

    private LogTypeService logTypeService;
+
+    private SATIFSourceConfigDao satifSourceConfigDao;


SaTifSourceConfigDao*

eirsep · 2024-05-24T23:18:20Z

src/main/java/org/opensearch/securityanalytics/SecurityAnalyticsPlugin.java

@@ -211,13 +228,14 @@ public List<RestHandler> getRestHandlers(Settings settings,
                new RestSearchCorrelationRuleAction(),
                new RestIndexCustomLogTypeAction(),
                new RestSearchCustomLogTypeAction(),
-                new RestDeleteCustomLogTypeAction()
+                new RestDeleteCustomLogTypeAction(),
+                new RestIndexTIFConfigAction()


RestIndexTIFSourceConfigAction

eirsep · 2024-05-24T23:20:50Z

src/main/java/org/opensearch/securityanalytics/SecurityAnalyticsPlugin.java

+                    case FEED_SOURCE_CONFIG_FIELD:
+                        return SATIFSourceConfig.parse(xcp, id, null);
+                    default:
+                        log.warn("Unsupported document was indexed");


log level should be error
re-word message to make it more descriptive

message should include job parser failed, $fieldName (value), in security analytics job registration

reworded error message

eirsep · 2024-05-24T23:21:20Z

src/main/java/org/opensearch/securityanalytics/SecurityAnalyticsPlugin.java

+                xcp.nextToken();
+                switch (fieldName) {
+                    case FEED_SOURCE_CONFIG_FIELD:
+                        return SATIFSourceConfig.parse(xcp, id, null);


why is the third param null

Will address this in a future PR, need to understand what params should be used for parser

eirsep · 2024-05-24T23:23:15Z

.../java/org/opensearch/securityanalytics/threatIntel/action/SAIndexTIFSourceConfigRequest.java

+public class SAIndexTIFSourceConfigRequest extends ActionRequest implements IndexTIFSourceConfigRequest {
+    private static final ParameterValidator VALIDATOR = new ParameterValidator();
+    private String tifSourceConfigId;
+    private final WriteRequest.RefreshPolicy refreshPolicy;


dont accept param. always IMMEDIATE REFRESH

removed param

eirsep · 2024-05-24T23:24:29Z

...java/org/opensearch/securityanalytics/threatIntel/action/SAIndexTIFSourceConfigResponse.java

+    private final String tifConfigId;
+    private final Long version;
+    private final RestStatus status;
+    private final SATIFSourceConfigDto saTIFConfigDto;


if a word is an a acronym let's make it liek SaTifSourceConfigDto

eirsep · 2024-05-24T23:25:25Z

src/main/java/org/opensearch/securityanalytics/threatIntel/dao/SATIFSourceConfigDao.java

+
+    public void indexTIFSourceConfig(SATIFSourceConfig satifSourceConfig,
+                                     TimeValue indexTimeout,
+                                     WriteRequest.RefreshPolicy refreshPolicy,


should always be refresh immediate. remove param

eirsep · 2024-05-24T23:25:48Z

src/main/java/org/opensearch/securityanalytics/threatIntel/dao/SATIFSourceConfigDao.java

+    public void indexTIFSourceConfig(SATIFSourceConfig satifSourceConfig,
+                                     TimeValue indexTimeout,
+                                     WriteRequest.RefreshPolicy refreshPolicy,
+                                     final ActionListener<SATIFSourceConfig> actionListener) throws Exception {


throws Exception is not needed

eirsep · 2024-05-24T23:26:12Z

src/main/java/org/opensearch/securityanalytics/threatIntel/dao/SATIFSourceConfigDao.java

+                .source(satifSourceConfig.toXContent(XContentFactory.jsonBuilder(), ToXContent.EMPTY_PARAMS))
+                .timeout(indexTimeout);
+        log.debug("Indexing tif source config");
+        client.index(indexRequest, new ActionListener<>() {


use only ActionListener.wrap()

changed to ActionListener.wrap()

eirsep · 2024-05-24T23:27:13Z

src/main/java/org/opensearch/securityanalytics/threatIntel/dao/SATIFSourceConfigDao.java

+            }
+            @Override
+            public void onFailure(Exception e) {
+                throw new SecurityAnalyticsException("Exception saving the tif source config in index", RestStatus.INTERNAL_SERVER_ERROR, e);


use listener.onFailure don't re-throw

eirsep · 2024-05-24T23:32:55Z

src/main/java/org/opensearch/securityanalytics/threatIntel/dao/SATIFSourceConfigDao.java

+                                     TimeValue indexTimeout,
+                                     WriteRequest.RefreshPolicy refreshPolicy,
+                                     final ActionListener<SATIFSourceConfig> actionListener) throws Exception {
+        IndexRequest indexRequest = new IndexRequest(SecurityAnalyticsPlugin.JOB_INDEX_NAME)


wrap entire logic in try-catch and listener.onFailure + log message in catch block

eirsep · 2024-05-24T23:33:22Z

src/main/java/org/opensearch/securityanalytics/threatIntel/dao/SATIFSourceConfigDao.java

+        client.index(indexRequest, new ActionListener<>() {
+            @Override
+            public void onResponse(IndexResponse response) {
+                log.debug("TIF source config indexed success.");


we need to log the document id.

logged id in message

eirsep · 2024-05-24T23:33:41Z

src/main/java/org/opensearch/securityanalytics/threatIntel/dao/SATIFSourceConfigDao.java

+            @Override
+            public void onResponse(IndexResponse response) {
+                log.debug("TIF source config indexed success.");
+                satifSourceConfig.setId(response.getId());


copy to new object.

eirsep · 2024-05-24T23:36:21Z

src/main/java/org/opensearch/securityanalytics/threatIntel/dao/SATIFSourceConfigDao.java

+            }
+        } catch (IOException e) {
+            log.error("Runtime exception when getting the threat intel index mapping", e);
+            throw new SecurityAnalyticsException("Runtime exception when getting the threat intel index mapping", RestStatus.INTERNAL_SERVER_ERROR, e);


Message can be "Failed to get threat intel index mapping"

Changed message

Signed-off-by: Joanne Wang <[email protected]>

eirsep · 2024-05-25T01:18:01Z

src/main/java/org/opensearch/securityanalytics/threatIntel/dao/SATIFSourceConfigDao.java

+                    stepListener.onResponse(null);
+                    return;
+                }
+                log.error("Failed to create security analytics threat intel job index", e);


nit: log correct index name in message

Changed to log the index name in error message

eirsep · 2024-05-25T05:23:07Z

src/main/java/org/opensearch/securityanalytics/threatIntel/sacommons/TIFSourceConfigDao.java

+
+import org.opensearch.core.action.ActionListener;
+public interface TIFSourceConfigDao {
+    IndexTIFSourceConfigResponse indexTIFConfig(IndexTIFSourceConfigRequest request, ActionListener <IndexTIFSourceConfigResponse> listener);


response would be at transport and rest layer?

Renamed to TIFSourceConfigService and changed to abstract class, will follow up on implementation in future PR.

eirsep · 2024-05-26T05:32:27Z

...main/java/org/opensearch/securityanalytics/threatIntel/service/SATIFSourceConfigService.java

+            log.error("failed to release lock", exception);
+            listener.onFailure(exception);
+        });
+        SaTifSourceConfigDao.createJobIndexIfNotExists(createIndexStepListener);


this should be done in dao itself before create

moved the function invocation to the dao

Signed-off-by: Joanne Wang <[email protected]>

eirsep · 2024-05-28T23:59:20Z

src/main/java/org/opensearch/securityanalytics/SecurityAnalyticsPlugin.java

@@ -179,7 +179,7 @@ public Collection<Object> createComponents(Client client,
        TIFJobParameterService tifJobParameterService = new TIFJobParameterService(client, clusterService);
        TIFJobUpdateService tifJobUpdateService = new TIFJobUpdateService(clusterService, tifJobParameterService, threatIntelFeedDataService, builtInTIFMetadataLoader);
        TIFLockService threatIntelLockService = new TIFLockService(clusterService, client);
-        SaTifSourceConfigDao = new SATIFSourceConfigDao(client, clusterService, threadPool);


what is this lock service??

eirsep · 2024-05-29T00:01:12Z

src/main/java/org/opensearch/securityanalytics/threatIntel/dao/SATIFSourceConfigDao.java

+                actionListener.onFailure(e);
+            }
+        }, exception -> {
+            lockService.releaseLock(lock);


what does this lockservice achieve??

eirsep · 2024-05-29T00:02:27Z

...main/java/org/opensearch/securityanalytics/threatIntel/sacommons/TIFSourceConfigService.java

-public interface TIFSourceConfigDao {
-    IndexTIFSourceConfigResponse indexTIFConfig(IndexTIFSourceConfigRequest request, ActionListener <IndexTIFSourceConfigResponse> listener);
+public abstract class TIFSourceConfigService {
+    IndexTIFSourceConfigResponse indexTIFConfig(IndexTIFSourceConfigRequest request, ActionListener <IndexTIFSourceConfigResponse> listener){


method should be abstract

eirsep

plz address comments in followup PR

* create tif source config api implementation Signed-off-by: Joanne Wang <[email protected]> * clean up Signed-off-by: Joanne Wang <[email protected]> * tif/source Signed-off-by: Joanne Wang <[email protected]> * fix uri Signed-off-by: Joanne Wang <[email protected]> * comments Signed-off-by: Joanne Wang <[email protected]> * fix error message Signed-off-by: Joanne Wang <[email protected]> * moved createIndex invocation and other comments Signed-off-by: Joanne Wang <[email protected]> --------- Signed-off-by: Joanne Wang <[email protected]>

create tif source config api implementation

ed41a7e

Signed-off-by: Joanne Wang <[email protected]>

jowg-amazon requested review from amsiglan, AWSHurneyt, getsaurabh02, lezzago, praveensameneni, sbcd90, eirsep, engechas, goyamegh and riysaxen-amzn as code owners May 24, 2024 00:53

clean up

8be3664

Signed-off-by: Joanne Wang <[email protected]>

eirsep reviewed May 24, 2024

View reviewed changes

jowg-amazon added 2 commits May 24, 2024 09:24

tif/source

491bbaf

Signed-off-by: Joanne Wang <[email protected]>

fix uri

f850464

Signed-off-by: Joanne Wang <[email protected]>

eirsep reviewed May 24, 2024

View reviewed changes

comments

f41ac16

Signed-off-by: Joanne Wang <[email protected]>

eirsep requested changes May 25, 2024

View reviewed changes

eirsep reviewed May 25, 2024

View reviewed changes

eirsep reviewed May 26, 2024

View reviewed changes

jowg-amazon added 3 commits May 27, 2024 10:06

fix error message

2a8c085

Signed-off-by: Joanne Wang <[email protected]>

moved createIndex invocation and other comments

9e231f4

Signed-off-by: Joanne Wang <[email protected]>

Merge branch 'feature/threat_intel' into createTIFFeedApi

83b6156

Signed-off-by: Joanne Wang <[email protected]>

eirsep reviewed May 28, 2024

View reviewed changes

eirsep reviewed May 29, 2024

View reviewed changes

eirsep approved these changes May 29, 2024

View reviewed changes

eirsep merged commit 1935ed2 into opensearch-project:feature/threat_intel May 29, 2024
2 checks passed

Create TIF Source Config API #1046

Create TIF Source Config API #1046

Conversation

jowg-amazon commented May 24, 2024

Description

Issues Resolved

Check List

eirsep May 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eirsep May 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eirsep May 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eirsep left a comment

Choose a reason for hiding this comment

eirsep May 24, 2024 •

edited

Loading

eirsep May 24, 2024 •

edited

Loading

eirsep May 24, 2024 •

edited

Loading