Custom log type implementation #500

sbcd90 · 2023-07-26T01:16:19Z

Description

Refactor code to remove static DetectorType enum & make logtype dynamic.
Add api support for CRUD workflow of log types.

Issues Resolved

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.

codecov · 2023-07-26T01:23:01Z

Codecov Report

Merging #500 (10f5bf1) into 2.x (22b5fc9) will decrease coverage by 2.21%.
The diff coverage is 2.87%.

❗ Current head 10f5bf1 differs from pull request most recent head 1962c0e. Consider uploading reports for the commit 1962c0e to get more accurate results

@@             Coverage Diff              @@
##                2.x     #500      +/-   ##
============================================
- Coverage     28.16%   25.96%   -2.21%     
- Complexity      935      940       +5     
============================================
  Files           236      252      +16     
  Lines          9866    10687     +821     
  Branches       1118     1187      +69     
============================================
- Hits           2779     2775       -4     
- Misses         6844     7666     +822     
- Partials        243      246       +3

Files Changed	Coverage Δ
...rch/securityanalytics/SecurityAnalyticsPlugin.java	`2.50% <0.00%> (-0.07%)`	⬇️
...rityanalytics/action/CorrelatedFindingRequest.java	`0.00% <0.00%> (ø)`
...ityanalytics/action/DeleteCustomLogTypeAction.java	`0.00% <0.00%> (ø)`
...tyanalytics/action/DeleteCustomLogTypeRequest.java	`0.00% <0.00%> (ø)`
...yanalytics/action/DeleteCustomLogTypeResponse.java	`0.00% <0.00%> (ø)`
...rityanalytics/action/IndexCustomLogTypeAction.java	`0.00% <0.00%> (ø)`
...ityanalytics/action/IndexCustomLogTypeRequest.java	`0.00% <0.00%> (ø)`
...tyanalytics/action/IndexCustomLogTypeResponse.java	`0.00% <0.00%> (ø)`
...ityanalytics/action/SearchCustomLogTypeAction.java	`0.00% <0.00%> (ø)`
...tyanalytics/action/SearchCustomLogTypeRequest.java	`0.00% <0.00%> (ø)`
... and 21 more

lezzago · 2023-07-28T18:43:08Z

src/main/java/org/opensearch/securityanalytics/correlation/VectorEmbeddingsEngine.java

@@ -133,15 +134,16 @@ public void onResponse(MultiSearchResponse items) {
                                        for (int i = 0; i < 100; ++i) {
                                            corrVector[i] = ((float) counter) - 50.0f;
                                        }
-                                        corrVector[Detector.DetectorType.valueOf(detectorType.toUpperCase(Locale.ROOT)).getDim()] = (float) counter;
+
+                                        corrVector[Integer.parseInt(logTypes.get(detectorType).getTags().get("correlation_id").toString())] = (float) counter;


Can we break up Integer.parseInt(logTypes.get(detectorType).getTags().get("correlation_id").toString()) into variables and pass in the final variable here?

It would be helpful for line 146, 163, 164 as well.

lezzago · 2023-07-28T18:44:57Z

src/main/java/org/opensearch/securityanalytics/correlation/VectorEmbeddingsEngine.java

@@ -268,15 +270,15 @@ public void onResponse(IndexResponse response) {
                                if (response.status().equals(RestStatus.OK)) {
                                    try {
                                        float[] corrVector = new float[101];
-                                        corrVector[Detector.DetectorType.valueOf(detectorType.toUpperCase(Locale.ROOT)).getDim()] = 50.0f;
+                                        corrVector[Integer.parseInt(logTypes.get(detectorType).getTags().get("correlation_id").toString())] = 50.0f;


Same comment as above about breaking it up into readable variables.

fixed it.

Map<String, Object> tags = logTypes.get(detectorType).getTags(); String correlationId = tags.get("correlation_id").toString();

lezzago · 2023-07-28T18:47:29Z

src/main/java/org/opensearch/securityanalytics/logtype/BuiltinLogTypeLoader.java

+    protected List<CustomLogType> loadBuiltinLogTypesMetadata() throws URISyntaxException, IOException {
+        List<CustomLogType> customLogTypes = new ArrayList<>();
+
+        final String url = Objects.requireNonNull(BuiltinLogTypeLoader.class.getClassLoader().getResource(BASE_PATH)).toURI().toString();


We should have an exception message added as part of the requireNonNull function

lezzago · 2023-07-28T18:51:26Z

src/main/java/org/opensearch/securityanalytics/logtype/BuiltinLogTypeLoader.java

+
+        final String url = Objects.requireNonNull(BuiltinLogTypeLoader.class.getClassLoader().getResource(BASE_PATH)).toURI().toString();
+        Path dirPath = null;
+        if (url.contains("!")) {


Why would a url contain !?

the url contains ! because the plugin is loaded as a zip file.

lezzago · 2023-07-28T18:59:24Z

src/main/java/org/opensearch/securityanalytics/logtype/BuiltinLogTypeLoader.java

+        }
+
+        Stream<Path> folder = Files.list(dirPath);
+        Path logTypePath = folder.filter(e -> e.toString().endsWith("logtypes.json")).collect(Collectors.toList()).get(0);


Do we know for a fact that we will get a folder that ends with logtypes.json to prevent array out of bounds type exception?

yes. we pre-deliver the file here https://github.com/opensearch-project/security-analytics/blob/deb0ae381d7095343a6d52313def52338970432c/src/main/resources/OSMapping/logtypes.json

lezzago · 2023-07-28T19:01:54Z

src/main/java/org/opensearch/securityanalytics/logtype/LogTypeService.java

+            SearchSourceBuilder searchSourceBuilder = new SearchSourceBuilder();
+            searchSourceBuilder.query(queryBuilder);
+            searchSourceBuilder.fetchSource(true);
+            searchSourceBuilder.size(10000);


Cant we use size 0 if we just want to see if it exists and use TotalHits?

we add a filter clause to the query

.must(QueryBuilders.matchQuery("name", logType));``` so, it anyway will fetch `1 or 0` doc.

why specify this then? It implies there can be a lot of results and may add confusion for other devs looking at it.

lezzago · 2023-07-28T19:02:39Z

src/main/java/org/opensearch/securityanalytics/logtype/LogTypeService.java

@@ -128,6 +134,92 @@ public void getAllLogTypes(ActionListener<List<String>> listener) {
        }, listener::onFailure));
    }

+    public void getAllLogTypesMetadata(ActionListener<List<String>> listener) {


Do we need to paginate? What happens if ther are more than 10000 documents?

currently, we have only 13 predefined log types & we dont expect to grow to 10000 log types ever. so, it looks to be a good threshold.
we may add a setting in future on no. of allowed log types.

lezzago · 2023-07-28T19:03:39Z

src/main/java/org/opensearch/securityanalytics/logtype/LogTypeService.java

+            SearchSourceBuilder searchSourceBuilder = new SearchSourceBuilder();
+            searchSourceBuilder.query(queryBuilder);
+            searchSourceBuilder.fetchSource(true);
+            searchSourceBuilder.size(10000);


do we need to paginate?

the same logic applies here as we have only 13 pre-defined log sources. it is hard that no. of custom log types may go beyond 100.

lezzago · 2023-07-28T19:22:41Z

src/main/java/org/opensearch/securityanalytics/rules/backend/OSQueryBackend.java

@@ -438,7 +438,7 @@ private Object convertValueCidr(SigmaCIDRExpression ip) {
    }

    private String getMappedField(String field) {
-        if (this.enableFieldMappings && this.fieldMappings.containsKey(field)) {
+        if (this.enableFieldMappings && this.fieldMappings.containsKey(field) && this.fieldMappings.get(field) != null) {


If it contains the key, how would the value be set to null?

there is a scenario when we create & then update a custom log type, we end up with fields which have only raw fields & no ecs fields.
this scenario is covered by integ test

security-analytics/src/test/java/org/opensearch/securityanalytics/resthandler/CustomLogTypeRestApiIT.java

Line 166 in deb0ae3

public void testEditACustomLogTypeDescription() throws IOException {

lezzago · 2023-07-28T19:23:54Z

src/main/java/org/opensearch/securityanalytics/transport/TransportCorrelateFindingAction.java

+                                    SearchSourceBuilder searchSourceBuilder = new SearchSourceBuilder();
+                                    searchSourceBuilder.query(queryBuilder);
+                                    searchSourceBuilder.fetchSource(true);
+                                    searchSourceBuilder.size(10000);


Do we need to paginate?

same logic as #500 (comment)

lezzago · 2023-07-28T19:24:46Z

src/main/java/org/opensearch/securityanalytics/transport/TransportCorrelateFindingAction.java

+                        SearchSourceBuilder searchSourceBuilder = new SearchSourceBuilder();
+                        searchSourceBuilder.query(queryBuilder);
+                        searchSourceBuilder.fetchSource(true);
+                        searchSourceBuilder.size(10000);


same question about pagination

the same logic as #500 (comment).
it will be good to add a setting in future on max. no. of log types allowed. created issue here: #501

Signed-off-by: Subhobrata Dey <[email protected]>

lezzago · 2023-07-28T21:45:39Z

src/main/java/org/opensearch/securityanalytics/transport/TransportIndexCustomLogTypeAction.java

+        User user = readUserFromThreadContext(this.threadPool);
+
+        String validateBackendRoleMessage = validateUserBackendRoles(user, this.filterByEnabled);
+        if (!"".equals(validateBackendRoleMessage)) {


nitpick: validateBackendRoleMessage.isNotEmpty()

lezzago · 2023-07-28T21:48:14Z

src/main/java/org/opensearch/securityanalytics/transport/TransportIndexCustomLogTypeAction.java

+
+    public void onCreateMappingsResponse(CreateIndexResponse response) {
+        if (response.isAcknowledged()) {
+            log.info(String.format(Locale.getDefault(), "Created %s with mappings.", LogTypeService.LOG_TYPE_INDEX));


Should this be a debug log? Same with the others.

lezzago · 2023-07-28T22:09:13Z

src/main/java/org/opensearch/securityanalytics/transport/TransportIndexCustomLogTypeAction.java

+                                        if (response.isTimedOut()) {
+                                            onFailures(new OpenSearchStatusException(String.format(Locale.getDefault(), "Log Type with id %s cannot be updated", logTypeId), RestStatus.INTERNAL_SERVER_ERROR));
+                                            return;


Does this happen often enough? If we are specifically looking for this, we should mention it timed out in the exception. Also this should not be an internal server error since this should be a a transient error that the customer can retry.

sure. we need to address this in entire codebase. we'll do it in 2.10.
created an issue: #502

lezzago · 2023-07-28T22:25:28Z

src/main/java/org/opensearch/securityanalytics/transport/TransportIndexCustomLogTypeAction.java

+            }
+        }
+
+        private void finishHim(CustomLogType logType, Exception... t) {


mortal kombat finish him 😂

lezzago · 2023-07-28T22:44:49Z

src/main/resources/mappings/log_type_config_mapping.json

Do we need to update the schema version?

this file is introduced in 2.9 open source release. so, we can keep schema version same.

getsaurabh02

LGTM

Signed-off-by: Subhobrata Dey <[email protected]> (cherry picked from commit ef64f00)

Signed-off-by: Subhobrata Dey <[email protected]>

sbcd90 force-pushed the custom_log_type branch 4 times, most recently from 1ddbbff to deb0ae3 Compare July 28, 2023 01:40

sbcd90 added backport main backport 2.9 backports to 2.9 labels Jul 28, 2023

lezzago reviewed Jul 28, 2023

View reviewed changes

sbcd90 mentioned this pull request Jul 28, 2023

[FEATURE] Add a setting for maximum number of custom log types allowed #501

Open

sbcd90 force-pushed the custom_log_type branch from deb0ae3 to 53a7865 Compare July 28, 2023 20:50

entire custom logtype implementation

1962c0e

Signed-off-by: Subhobrata Dey <[email protected]>

sbcd90 force-pushed the custom_log_type branch from 53a7865 to 1962c0e Compare July 28, 2023 20:59

lezzago reviewed Jul 28, 2023

View reviewed changes

getsaurabh02 approved these changes Jul 29, 2023

View reviewed changes

lezzago approved these changes Jul 29, 2023

View reviewed changes

sbcd90 merged commit ef64f00 into opensearch-project:2.x Jul 29, 2023

opensearch-trigger-bot bot mentioned this pull request Jul 29, 2023

[Backport main] Custom log type implementation #503

Merged

opensearch-trigger-bot bot pushed a commit that referenced this pull request Jul 29, 2023

entire custom logtype implementation (#500)

6d6b504

Signed-off-by: Subhobrata Dey <[email protected]> (cherry picked from commit ef64f00)

opensearch-trigger-bot bot pushed a commit that referenced this pull request Jul 29, 2023

entire custom logtype implementation (#500)

6e0fee3

Signed-off-by: Subhobrata Dey <[email protected]> (cherry picked from commit ef64f00)

opensearch-trigger-bot bot mentioned this pull request Jul 29, 2023

[Backport 2.9] Custom log type implementation #504

Merged

sbcd90 pushed a commit that referenced this pull request Jul 29, 2023

entire custom logtype implementation (#500) (#504)

c7ea1ea

Signed-off-by: Subhobrata Dey <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Custom log type implementation #500

Custom log type implementation #500

sbcd90 commented Jul 26, 2023

codecov bot commented Jul 26, 2023 •

edited

Loading

lezzago Jul 28, 2023

lezzago Jul 28, 2023 •

edited

Loading

sbcd90 Jul 28, 2023

lezzago Jul 28, 2023

sbcd90 Jul 28, 2023

lezzago Jul 28, 2023

sbcd90 Jul 28, 2023

lezzago Jul 28, 2023

sbcd90 Jul 28, 2023

lezzago Jul 28, 2023

sbcd90 Jul 28, 2023

lezzago Jul 28, 2023 •

edited

Loading

sbcd90 Jul 28, 2023

lezzago Jul 28, 2023

lezzago Jul 28, 2023

sbcd90 Jul 28, 2023

lezzago Jul 28, 2023

sbcd90 Jul 28, 2023

lezzago Jul 28, 2023

sbcd90 Jul 28, 2023

lezzago Jul 28, 2023

sbcd90 Jul 28, 2023

lezzago Jul 28, 2023

sbcd90 Jul 28, 2023

lezzago Jul 28, 2023

lezzago Jul 28, 2023 •

edited

Loading

lezzago Jul 28, 2023

sbcd90 Jul 29, 2023

lezzago Jul 28, 2023

lezzago Jul 28, 2023

sbcd90 Jul 29, 2023

getsaurabh02 left a comment

Custom log type implementation #500

Custom log type implementation #500

Conversation

sbcd90 commented Jul 26, 2023

Description

Issues Resolved

Check List

codecov bot commented Jul 26, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

lezzago Jul 28, 2023 • 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

lezzago Jul 28, 2023 • 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

lezzago Jul 28, 2023 • 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

getsaurabh02 left a comment

Choose a reason for hiding this comment

codecov bot commented Jul 26, 2023 •

edited

Loading

lezzago Jul 28, 2023 •

edited

Loading

lezzago Jul 28, 2023 •

edited

Loading

lezzago Jul 28, 2023 •

edited

Loading