Return list of cx friendly 4xx errors for indexing custom rules (#862)

* Return list of cx friendly 4xx errors for indexing custom rules Signed-off-by: Megha Goyal <[email protected]> * Fixing integ tests Signed-off-by: Megha Goyal <[email protected]> * Addressing CR comments Signed-off-by: Megha Goyal <[email protected]> * Fix integ tests Signed-off-by: Megha Goyal <[email protected]> * Fix unit tests Signed-off-by: Megha Goyal <[email protected]> * CR comments Signed-off-by: Megha Goyal <[email protected]> * Return RuntimeException to fix compilation errors Signed-off-by: Megha Goyal <[email protected]> * Fix unit tests for Sigma title changes in main Signed-off-by: Megha Goyal <[email protected]> --------- Signed-off-by: Megha Goyal <[email protected]>
opensearch-project · Apr 16, 2024 · 5188864 · 5188864
1 parent b86ee63
commit 5188864
Show file tree

Hide file tree

Showing 14 changed files with 248 additions and 185 deletions.
diff --git a/src/main/java/org/opensearch/securityanalytics/model/Rule.java b/src/main/java/org/opensearch/securityanalytics/model/Rule.java
@@ -20,7 +20,8 @@
 import org.opensearch.securityanalytics.rules.aggregation.AggregationItem;
 import org.opensearch.securityanalytics.rules.backend.OSQueryBackend.AggregationQueries;
 import org.opensearch.securityanalytics.rules.condition.ConditionItem;
-import org.opensearch.securityanalytics.rules.exceptions.SigmaError;
+import org.opensearch.securityanalytics.rules.exceptions.SigmaConditionError;
+import org.opensearch.securityanalytics.rules.exceptions.CompositeSigmaErrors;
 import org.opensearch.securityanalytics.rules.objects.SigmaCondition;
 import org.opensearch.securityanalytics.rules.objects.SigmaRule;
 
@@ -475,8 +476,9 @@ public boolean isAggregationRule() {
         return aggregationQueries != null && !aggregationQueries.isEmpty();
     }
 
-    public List<AggregationItem> getAggregationItemsFromRule () throws SigmaError {
+    public List<AggregationItem> getAggregationItemsFromRule () throws SigmaConditionError {
         SigmaRule sigmaRule = SigmaRule.fromYaml(rule, true);
+        // TODO: Check if there are cx errors from the rule created and throw errors
         List<AggregationItem> aggregationItems = new ArrayList<>();
         for (SigmaCondition condition: sigmaRule.getDetection().getParsedCondition()) {
             Pair<ConditionItem, AggregationItem> parsedItems = condition.parsed();

diff --git a/src/main/java/org/opensearch/securityanalytics/rules/backend/QueryBackend.java b/src/main/java/org/opensearch/securityanalytics/rules/backend/QueryBackend.java
@@ -13,6 +13,7 @@
 import org.opensearch.securityanalytics.rules.condition.ConditionOR;
 import org.opensearch.securityanalytics.rules.condition.ConditionType;
 import org.opensearch.securityanalytics.rules.condition.ConditionValueExpression;
+import org.opensearch.securityanalytics.rules.exceptions.SigmaConditionError;
 import org.opensearch.securityanalytics.rules.exceptions.SigmaError;
 import org.opensearch.securityanalytics.rules.exceptions.SigmaValueError;
 import org.opensearch.securityanalytics.rules.objects.SigmaCondition;
@@ -29,18 +30,12 @@
 import org.opensearch.securityanalytics.rules.utils.AnyOneOf;
 import org.opensearch.securityanalytics.rules.utils.Either;
 import org.apache.commons.lang3.tuple.Pair;
-import org.yaml.snakeyaml.LoaderOptions;
-import org.yaml.snakeyaml.Yaml;
-import org.yaml.snakeyaml.constructor.SafeConstructor;
 
-import java.io.IOException;
-import java.io.InputStream;
-import java.nio.charset.Charset;
 import java.util.ArrayList;
 import java.util.List;
-import java.util.Locale;
 import java.util.HashMap;
 import java.util.HashSet;
+import java.util.Locale;
 import java.util.Map;
 import java.util.Set;
 
@@ -71,7 +66,7 @@ public QueryBackend(Map<String, String> fieldMappings, boolean convertAndAsIn, b
         }
     }
 
-    public List<Object> convertRule(SigmaRule rule) throws SigmaError {
+    public List<Object> convertRule(SigmaRule rule) throws SigmaValueError, SigmaConditionError {
         this.ruleQueryFields = new HashMap<>();
         List<Object> queries = new ArrayList<>();
         try {
@@ -100,7 +95,8 @@ public List<Object> convertRule(SigmaRule rule) throws SigmaError {
             }
 
             this.queryFields.putAll(this.ruleQueryFields);
-        } catch (SigmaError ex) {
+        } catch (SigmaValueError ex) {
+            // TODO: Merge the exception to the original list of errors coming from SigmaRule.java and use the same throwing logic
             if (this.collectErrors) {
                 this.errors.add(Pair.of(rule, ex));
             } else {
@@ -276,5 +272,5 @@ public Object convertConditionVal(ConditionValueExpression condition, boolean ap
 
 /*   public abstract Object convertConditionValQueryExpr(ConditionValueExpression condition);*/
 
-    public abstract AggregationQueries convertAggregation(AggregationItem aggregation) throws SigmaError;
+    public abstract AggregationQueries convertAggregation(AggregationItem aggregation);
 }
diff --git a/src/main/java/org/opensearch/securityanalytics/rules/exceptions/CompositeSigmaErrors.java b/src/main/java/org/opensearch/securityanalytics/rules/exceptions/CompositeSigmaErrors.java
@@ -0,0 +1,26 @@
+/*
+ * Copyright OpenSearch Contributors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+package org.opensearch.securityanalytics.rules.exceptions;
+
+import org.opensearch.OpenSearchException;
+
+import java.util.ArrayList;
+import java.util.List;
+
+public class CompositeSigmaErrors extends RuntimeException {
+    private final List<SigmaError> errorList;
+
+    public CompositeSigmaErrors() {
+        this.errorList = new ArrayList<>();
+    }
+
+    public void addError(SigmaError error) {
+        errorList.add(error);
+    }
+
+    public List<SigmaError> getErrors() {
+        return errorList;
+    }
+}
diff --git a/src/main/java/org/opensearch/securityanalytics/rules/exceptions/SigmaError.java b/src/main/java/org/opensearch/securityanalytics/rules/exceptions/SigmaError.java
@@ -6,7 +6,7 @@
 
 public class SigmaError extends Exception {
 
-    private String message;
+    private final String message;
 
     public SigmaError(String message) {
         super(message);

diff --git a/src/main/java/org/opensearch/securityanalytics/rules/objects/SigmaRule.java b/src/main/java/org/opensearch/securityanalytics/rules/objects/SigmaRule.java
@@ -8,6 +8,7 @@
 import org.opensearch.securityanalytics.rules.exceptions.SigmaDetectionError;
 import org.opensearch.securityanalytics.rules.exceptions.SigmaError;
 import org.opensearch.securityanalytics.rules.exceptions.SigmaIdentifierError;
+import org.opensearch.securityanalytics.rules.exceptions.CompositeSigmaErrors;
 import org.opensearch.securityanalytics.rules.exceptions.SigmaLevelError;
 import org.opensearch.securityanalytics.rules.exceptions.SigmaLogsourceError;
 import org.opensearch.securityanalytics.rules.exceptions.SigmaTitleError;
@@ -54,11 +55,11 @@ public class SigmaRule {
 
     private SigmaLevel level;
 
-    private List<SigmaError> errors;
+    private CompositeSigmaErrors errors;
 
     public SigmaRule(String title, SigmaLogSource logSource, SigmaDetections detection, UUID id, SigmaStatus status,
                      String description, List<String> references, List<SigmaRuleTag> tags, String author, Date date,
-                     List<String> fields, List<String> falsePositives, SigmaLevel level, List<SigmaError> errors) {
+                     List<String> fields, List<String> falsePositives, SigmaLevel level, CompositeSigmaErrors errors) {
         this.title = title;
         this.logSource = logSource;
         this.detection = detection;
@@ -90,19 +91,19 @@ public SigmaRule(String title, SigmaLogSource logSource, SigmaDetections detecti
     }
 
     @SuppressWarnings("unchecked")
-    protected static SigmaRule fromDict(Map<String, Object> rule, boolean collectErrors) throws SigmaError {
-        List<SigmaError> errors = new ArrayList<>();
+    protected static SigmaRule fromDict(Map<String, Object> rule, boolean collectErrors) {
+        CompositeSigmaErrors errors = new CompositeSigmaErrors();
 
         UUID ruleId;
         if (rule.containsKey("id")) {
             try {
                 ruleId = UUID.fromString(rule.get("id").toString());
             } catch (IllegalArgumentException ex) {
-                errors.add(new SigmaIdentifierError("Sigma rule identifier must be an UUID"));
+                errors.addError(new SigmaIdentifierError("Sigma rule identifier must be an UUID"));
                 ruleId = null;
             }
         } else {
-            errors.add(new SigmaIdentifierError("Sigma rule identifier must be an UUID"));
+            errors.addError(new SigmaIdentifierError("Sigma rule identifier cannot be null"));
             ruleId = null;
         }
 
@@ -111,26 +112,32 @@ protected static SigmaRule fromDict(Map<String, Object> rule, boolean collectErr
             title = rule.get("title").toString();
             if (!title.matches("^.{1,256}$"))
             {
-                errors.add(new SigmaTitleError("Sigma rule title can be max 256 characters"));
+                errors.addError(new SigmaTitleError("Sigma rule title can be max 256 characters"));
             }
         } else {
             title = "";
         }
 
-        SigmaLevel level;
+        SigmaLevel level = null;
         if (rule.containsKey("level")) {
-            level = SigmaLevel.valueOf(rule.get("level").toString().toUpperCase(Locale.ROOT));
+            try {
+                level = SigmaLevel.valueOf(rule.get("level").toString().toUpperCase(Locale.ROOT));
+            } catch (IllegalArgumentException ex) {
+                errors.addError(new SigmaLevelError("Value of level not correct"));
+            }
         } else {
-            errors.add(new SigmaLevelError("null is no valid Sigma rule level"));
-            level = null;
+            errors.addError(new SigmaLevelError("Sigma rule level cannot be null"));
         }
 
-        SigmaStatus status;
+        SigmaStatus status = null;
         if (rule.containsKey("status")) {
-            status = SigmaStatus.valueOf(rule.get("status").toString().toUpperCase(Locale.ROOT));
+            try {
+                status = SigmaStatus.valueOf(rule.get("status").toString().toUpperCase(Locale.ROOT));
+            } catch (IllegalArgumentException ex) {
+                errors.addError(new SigmaStatusError("Value of status not correct"));
+            }
         } else {
-            errors.add(new SigmaStatusError("null is no valid Sigma rule status"));
-            status = null;
+            errors.addError(new SigmaStatusError("Sigma rule status cannot be null"));
         }
 
         Date ruleDate = null;
@@ -144,24 +151,30 @@ protected static SigmaRule fromDict(Map<String, Object> rule, boolean collectErr
                     ruleDate = formatter.parse(rule.get("date").toString());
                 }
             } catch (Exception ex) {
-                errors.add(new SigmaDateError("Rule date " + rule.get("date").toString() + " is invalid, must be yyyy/mm/dd or yyyy-mm-dd"));
+                errors.addError(new SigmaDateError("Rule date " + rule.get("date").toString() + " is invalid, must be yyyy/mm/dd or yyyy-mm-dd"));
             }
         }
 
-        SigmaLogSource logSource;
+        SigmaLogSource logSource = null;
         if (rule.containsKey("logsource")) {
-            logSource = SigmaLogSource.fromDict((Map<String, Object>) rule.get("logsource"));
+            try {
+                logSource = SigmaLogSource.fromDict((Map<String, Object>) rule.get("logsource"));
+            } catch (SigmaLogsourceError ex) {
+                errors.addError(new SigmaLogsourceError("Sigma rule must have a log source"));
+            }
         } else {
-            errors.add(new SigmaLogsourceError("Sigma rule must have a log source"));
-            logSource = null;
+            errors.addError(new SigmaLogsourceError("Sigma rule must have a log source"));
         }
 
-        SigmaDetections detections;
+        SigmaDetections detections = null;
         if (rule.containsKey("detection")) {
-            detections = SigmaDetections.fromDict((Map<String, Object>) rule.get("detection"));
+            try {
+                detections = SigmaDetections.fromDict((Map<String, Object>) rule.get("detection"));
+            } catch (SigmaError ex) {
+                errors.addError(new SigmaDetectionError("Sigma rule must have a detection definitions"));
+            }
         } else {
-            errors.add(new SigmaDetectionError("Sigma rule must have a detection definitions"));
-            detections = null;
+            errors.addError(new SigmaDetectionError("Sigma rule must have a detection definitions"));
         }
 
         List<String> ruleTagsStr = (List<String>) rule.get("tags");
@@ -172,8 +185,8 @@ protected static SigmaRule fromDict(Map<String, Object> rule, boolean collectErr
             }
         }
 
-        if (!collectErrors && !errors.isEmpty()) {
-            throw errors.get(0);
+        if (!collectErrors && !errors.getErrors().isEmpty()) {
+            throw errors;
         }
 
         return new SigmaRule(title, logSource, detections, ruleId, status,
@@ -182,7 +195,7 @@ protected static SigmaRule fromDict(Map<String, Object> rule, boolean collectErr
                 rule.get("falsepositives") != null? (List<String>) rule.get("falsepositives"): null, level, errors);
     }
 
-    public static SigmaRule fromYaml(String rule, boolean collectErrors) throws SigmaError {
+    public static SigmaRule fromYaml(String rule, boolean collectErrors) {
         LoaderOptions loaderOptions = new LoaderOptions();
         loaderOptions.setNestingDepthLimit(10);
 
@@ -243,7 +256,7 @@ public SigmaLevel getLevel() {
         return level;
     }
 
-    public List<SigmaError> getErrors() {
+    public CompositeSigmaErrors getErrors() {
         return errors;
     }
 }
diff --git a/...in/java/org/opensearch/securityanalytics/transport/TransportIndexCustomLogTypeAction.java b/...in/java/org/opensearch/securityanalytics/transport/TransportIndexCustomLogTypeAction.java
@@ -475,11 +475,7 @@ private void onFailures(Exception... t) {
         private void finishHim(CustomLogType logType, Exception... t) {
             threadPool.executor(ThreadPool.Names.GENERIC).execute(ActionRunnable.supply(listener, () -> {
                 if (t != null && t.length > 0) {
-                    if (t.length > 1) {
-                        throw SecurityAnalyticsException.wrap(Arrays.asList(t));
-                    } else {
-                        throw SecurityAnalyticsException.wrap(t[0]);
-                    }
+                    throw SecurityAnalyticsException.wrap(Arrays.asList(t));
                 } else {
                     return new IndexCustomLogTypeResponse(logType.getId(), logType.getVersion(), RestStatus.CREATED, logType);
                 }

diff --git a/src/main/java/org/opensearch/securityanalytics/transport/TransportIndexDetectorAction.java b/src/main/java/org/opensearch/securityanalytics/transport/TransportIndexDetectorAction.java
@@ -95,7 +95,8 @@
 import org.opensearch.securityanalytics.rules.backend.OSQueryBackend;
 import org.opensearch.securityanalytics.rules.backend.OSQueryBackend.AggregationQueries;
 import org.opensearch.securityanalytics.rules.backend.QueryBackend;
-import org.opensearch.securityanalytics.rules.exceptions.SigmaError;
+import org.opensearch.securityanalytics.rules.exceptions.SigmaConditionError;
+import org.opensearch.securityanalytics.rules.exceptions.CompositeSigmaErrors;
 import org.opensearch.securityanalytics.settings.SecurityAnalyticsSettings;
 import org.opensearch.securityanalytics.threatIntel.DetectorThreatIntelService;
 import org.opensearch.securityanalytics.util.DetectorIndices;
@@ -246,7 +247,7 @@ public void onFailure(Exception e) {
                 log.debug("check indices and execute failed", e);
                 if (e instanceof OpenSearchStatusException) {
                     listener.onFailure(SecurityAnalyticsException.wrap(
-                            new OpenSearchStatusException(String.format(Locale.getDefault(), "User doesn't have read permissions for one or more configured index %s", detectorIndices), RestStatus.FORBIDDEN)
+                            new OpenSearchStatusException(String.format(Locale.getDefault(), "User doesn't have read permissions for one or more configured index %s", (Object) detectorIndices), RestStatus.FORBIDDEN)
                     ));
                 } else if (e instanceof IndexNotFoundException) {
                     listener.onFailure(SecurityAnalyticsException.wrap(
@@ -917,27 +918,31 @@ public void onFailure(Exception e) {
 
                         // Creating bucket level monitor per each aggregation rule
                         if (rule.getAggregationQueries() != null) {
-                            createBucketLevelMonitorRequest(
-                                    query.getRight(),
-                                    detector,
-                                    refreshPolicy,
-                                    monitorId,
-                                    restMethod,
-                                    queryBackendMap.get(rule.getCategory()),
-                                    new ActionListener<>() {
-                                        @Override
-                                        public void onResponse(IndexMonitorRequest indexMonitorRequest) {
-                                            monitorRequests.add(indexMonitorRequest);
-                                            bucketLevelMonitorRequestsListener.onResponse(indexMonitorRequest);
-                                        }
+                            try {
+                                createBucketLevelMonitorRequest(
+                                        query.getRight(),
+                                        detector,
+                                        refreshPolicy,
+                                        monitorId,
+                                        restMethod,
+                                        queryBackendMap.get(rule.getCategory()),
+                                        new ActionListener<>() {
+                                            @Override
+                                            public void onResponse(IndexMonitorRequest indexMonitorRequest) {
+                                                monitorRequests.add(indexMonitorRequest);
+                                                bucketLevelMonitorRequestsListener.onResponse(indexMonitorRequest);
+                                            }
 
 
-                                        @Override
-                                        public void onFailure(Exception e) {
-                                            logger.error("Failed to build bucket level monitor requests", e);
-                                            bucketLevelMonitorRequestsListener.onFailure(e);
-                                        }
-                                    });
+                                            @Override
+                                            public void onFailure(Exception e) {
+                                                logger.error("Failed to build bucket level monitor requests", e);
+                                                bucketLevelMonitorRequestsListener.onFailure(e);
+                                            }
+                                        });
+                            } catch (SigmaConditionError e) {
+                                throw new RuntimeException(e);
+                            }
 
                         } else {
                             log.debug("Aggregation query is null in rule {}", rule.getId());
@@ -961,7 +966,7 @@ private void createBucketLevelMonitorRequest(
             RestRequest.Method restMethod,
             QueryBackend queryBackend,
             ActionListener<IndexMonitorRequest> listener
-    ) {
+    ) throws SigmaConditionError {
         log.debug(":create bucket level monitor response starting");
         List<String> indices = detector.getInputs().get(0).getIndices();
         try {
@@ -1053,7 +1058,7 @@ public void onFailure(Exception e) {
                             listener.onFailure(e);
                         }
                     });
-        } catch (SigmaError e) {
+        } catch (CompositeSigmaErrors e) {
             log.error("Failed to create bucket level monitor request", e);
             listener.onFailure(e);
         }