Skip to content
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

Fix get mappings view API incorrectly returning ecs path #867

Merged
merged 2 commits into from
Mar 11, 2024

Conversation

jowg-amazon
Copy link
Collaborator

Description

Currently the get mappings view api incorrectly returns the ecs path for logs with ocsf or raw field types if a custom rule is created using a raw field. This is because whenever a new rule is created, the rule field(s) gets indexed into .opensearch-sap-log-types-config index along with the log_type (ex. below). However, because there are already preset mappings defined for certain log types like cloudtrail, the raw_field, ecs, and ocsf mappings are already defined in this index. Then when the get Mappings view API is called, the new document from the custom rule overwrites the existing predefined mapping which causes the ecs format to be incorrectly returned.

{
        "_index": ".opensearch-sap-log-types-config",
        "_id": "aws.cloudtrail.event_name|",
        "_score": 1,
        "_source": {
          "raw_field": "aws.cloudtrail.event_name",
          "log_types": [
            "cloudtrail"
          ]
        }
}

This PR performs additional logic to check whether or not the raw field from the custom rule already exists in the existingFieldMappings (i.e. if this is a known mapping that is predefined in the index). If it is defined, then it doesn't add a the new mapping into the .opensearch-sap-log-types-config index.

The integ test ensures that the mappings view API returns the same output before and after a custom rule with a raw field is created to verify that the correct path is returned.

Issues Resolved

#866

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.

@@ -356,6 +356,9 @@ private List<FieldMappingDoc> mergeFieldMappings(List<FieldMappingDoc> existingF
)) {
foundFieldMappingDoc = Optional.of(e);
}
// Additional check to see if raw field path + log type combination is already in existingFieldMappings so a new one is not inserted
} else if (e.getId().contains(newFieldMapping.getRawField()) && e.getLogTypes().containsAll(newFieldMapping.getLogTypes())) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wasn't added by you so optional - Maybe existingFieldMapping instead of e. e reads as Exception to me

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a possibility of false positives here? Say I have a field with name field2 and then another field with name field. If field2 is evaluated first, the contains call would match for field even though it's an entry for field2

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I agree e is a bit confusing. I changed it to existingFieldMapping. Also yes that false positive is a possibility. I changed the logic so that we grab the right side of the ID of the doc which corresponds to the ecs field name when we load in our predefined mappings. That way we can do an equals check instead to ensure that there are no duplicates.

ex. of document from pre-defined mapping

{
        "_index": ".opensearch-sap-log-types-config",
        "_id": "eventName|aws.cloudtrail.event_name",
        "_score": 1,
        "_source": {
          "raw_field": "eventName",
          "ecs": "aws.cloudtrail.event_name",
          "ocsf": "api.operation",
          "log_types": [
            "s3",
            "cloudtrail"
          ]
        }
      }

Copy link

codecov bot commented Feb 26, 2024

Codecov Report

Attention: Patch coverage is 58.33333% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 24.79%. Comparing base (75c4429) to head (09cf6ab).

❗ Current head 09cf6ab differs from pull request most recent head ff4f63b. Consider uploading reports for the commit ff4f63b to get more accurate results

Files Patch % Lines
...arch/securityanalytics/logtype/LogTypeService.java 58.33% 1 Missing and 4 partials ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main     #867      +/-   ##
============================================
- Coverage     24.97%   24.79%   -0.19%     
+ Complexity     1043     1026      -17     
============================================
  Files           277      277              
  Lines         12771    12723      -48     
  Branches       1391     1403      +12     
============================================
- Hits           3190     3155      -35     
+ Misses         9307     9300       -7     
+ Partials        274      268       -6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

} else {
String id = existingFieldMapping.getId();
int indexOfPipe = id.indexOf("|");
if (indexOfPipe != -1) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this backwards compatible? Wondering if we will have duplicate mappings for a mapping that was generated before the pipe system was introduced (say an upgrade from an older to the current version).

I am also not very familiar with this part of the code though, so if that is not a concern, then I am good to approve.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Backwards compatibility should be fine because the mapping should not have been inserted in the first place but now we're ensuring that it doesn't get generated. If they encounter this bug though I think they may need to delete the log-type-config index so that we can automatically regenerate it with the correct mappings.

@eirsep
Copy link
Member

eirsep commented Mar 7, 2024

@sbcd90 can we change logic to remove the code for adding any new field into this index at all during custom rule creation?

@praveensameneni praveensameneni merged commit 25f6c50 into opensearch-project:main Mar 11, 2024
4 of 16 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Mar 11, 2024
* add logic and integ tests to not add duplicate to log-types-config index

Signed-off-by: Joanne Wang <jowg@amazon.com>

* change naming for existingFieldMapping and change contains to equals

Signed-off-by: Joanne Wang <jowg@amazon.com>

---------

Signed-off-by: Joanne Wang <jowg@amazon.com>
(cherry picked from commit 25f6c50)
sbcd90 pushed a commit that referenced this pull request Mar 11, 2024
riysaxen-amzn pushed a commit to riysaxen-amzn/security-analytics that referenced this pull request Mar 13, 2024
…project#867)

* add logic and integ tests to not add duplicate to log-types-config index

Signed-off-by: Joanne Wang <jowg@amazon.com>

* change naming for existingFieldMapping and change contains to equals

Signed-off-by: Joanne Wang <jowg@amazon.com>

---------

Signed-off-by: Joanne Wang <jowg@amazon.com>
@jowg-amazon jowg-amazon mentioned this pull request Mar 13, 2024
5 tasks
jowg-amazon added a commit to jowg-amazon/security-analytics that referenced this pull request Mar 14, 2024
…project#867)

* add logic and integ tests to not add duplicate to log-types-config index

Signed-off-by: Joanne Wang <jowg@amazon.com>

* change naming for existingFieldMapping and change contains to equals

Signed-off-by: Joanne Wang <jowg@amazon.com>

---------

Signed-off-by: Joanne Wang <jowg@amazon.com>
jowg-amazon added a commit to jowg-amazon/security-analytics that referenced this pull request Mar 15, 2024
…project#867)

* add logic and integ tests to not add duplicate to log-types-config index

Signed-off-by: Joanne Wang <jowg@amazon.com>

* change naming for existingFieldMapping and change contains to equals

Signed-off-by: Joanne Wang <jowg@amazon.com>

---------

Signed-off-by: Joanne Wang <jowg@amazon.com>
jowg-amazon added a commit that referenced this pull request Mar 15, 2024
* support object fields in aggregation based sigma rules (#789)

Signed-off-by: Subhobrata Dey <sbcd90@gmail.com>

* Fix duplicate ecs mappings which returns incorrect log index field in mapping view API (#786) (#788)

* field mapping changes

Signed-off-by: Joanne Wang <jowg@amazon.com>

* add integ test

Signed-off-by: Joanne Wang <jowg@amazon.com>

* turn unmappedfieldaliases as set and add integ test

Signed-off-by: Joanne Wang <jowg@amazon.com>

* add comments

Signed-off-by: Joanne Wang <jowg@amazon.com>

* fix integ tests

Signed-off-by: Joanne Wang <jowg@amazon.com>

* moved logic to method for better readability

Signed-off-by: Joanne Wang <jowg@amazon.com>

---------

Signed-off-by: Joanne Wang <jowg@amazon.com>

* Fix get mappings view API incorrectly returning ecs path (#867)

* add logic and integ tests to not add duplicate to log-types-config index

Signed-off-by: Joanne Wang <jowg@amazon.com>

* change naming for existingFieldMapping and change contains to equals

Signed-off-by: Joanne Wang <jowg@amazon.com>

---------

Signed-off-by: Joanne Wang <jowg@amazon.com>

* fix integ test (#918)

Signed-off-by: Joanne Wang <jowg@amazon.com>

* get all findings as part of findings API enhancement (#803)

* get all findings as part of findings API enhancement

Signed-off-by: Riya Saxena <riysaxen@amazon.com>

* findingsAPI feature enhancements (address comments to prev PR)

Signed-off-by: Riya Saxena <riysaxen@amazon.com>

* findingsAPI feature enhancements (address comments to prev PR)

Signed-off-by: Riya Saxena <riysaxen@amazon.com>

* added support for  param in Finding API

Signed-off-by: Riya Saxena <riysaxen@amazon.com>

* added detectionType as param for Findings API enhancements

Signed-off-by: Riya Saxena <riysaxen@amazon.com>

* added few tests to validate findings by params

Signed-off-by: Riya Saxena <riysaxen@amazon.com>

* added test for searchString param in FindingsAPI

Signed-off-by: Riya Saxena <riysaxen@amazon.com>

* adding addiional params findingIds, startTime and endTime as findings API enhancement

Signed-off-by: Riya Saxena <riysaxen@amazon.com>

* added params in getFindingsByDetectorId func

* changed the startTime and endTime req input format

* fix merge conflixt

* fix integ test failures in findings API

* fix integ tests

* fix integ tests for findings

Signed-off-by: Subhobrata Dey <sbcd90@gmail.com>

---------

Signed-off-by: Riya Saxena <riysaxen@amazon.com>
Signed-off-by: Riya <69919272+riysaxen-amzn@users.noreply.github.com>
Signed-off-by: Subhobrata Dey <sbcd90@gmail.com>
Co-authored-by: Subhobrata Dey <sbcd90@gmail.com>

* Feature findings api enhancements (#914)

* get all findings as part of findings API enhancement

Signed-off-by: Riya Saxena <riysaxen@amazon.com>

* findingsAPI feature enhancements (address comments to prev PR)

Signed-off-by: Riya Saxena <riysaxen@amazon.com>

* findingsAPI feature enhancements (address comments to prev PR)

Signed-off-by: Riya Saxena <riysaxen@amazon.com>

* added support for  param in Finding API

Signed-off-by: Riya Saxena <riysaxen@amazon.com>

* added detectionType as param for Findings API enhancements

Signed-off-by: Riya Saxena <riysaxen@amazon.com>

* added few tests to validate findings by params

Signed-off-by: Riya Saxena <riysaxen@amazon.com>

* added test for searchString param in FindingsAPI

Signed-off-by: Riya Saxena <riysaxen@amazon.com>

* adding addiional params findingIds, startTime and endTime as findings API enhancement

Signed-off-by: Riya Saxena <riysaxen@amazon.com>

* added params in getFindingsByDetectorId func

* changed the startTime and endTime req input format

* fix merge conflixt

* fix integ test failures in findings API

* fix integ tests

* refactored the logic

Signed-off-by: Riya Saxena <riysaxen@amazon.com>

* remove unused imports

* address the pr comments

Signed-off-by: Riya Saxena <riysaxen@amazon.com>

* address pr comments

Signed-off-by: Riya Saxena <riysaxen@amazon.com>

* SA integ tests fix

* SA integ tests fix

* fix integ tests for findings

Signed-off-by: Subhobrata Dey <sbcd90@gmail.com>

* fix conflixt errors

Signed-off-by: Riya Saxena <riysaxen@amazon.com>

* fix conflixt errors

Signed-off-by: Riya Saxena <riysaxen@amazon.com>

* fix conflixt errors

Signed-off-by: Riya Saxena <riysaxen@amazon.com>

* fix conflixt errors

Signed-off-by: Riya Saxena <riysaxen@amazon.com>

* fix integ tests

Signed-off-by: Riya Saxena <riysaxen@amazon.com>

* fix integ tests

Signed-off-by: Riya Saxena <riysaxen@amazon.com>

* fix integ tests

Signed-off-by: Riya Saxena <riysaxen@amazon.com>

* fix flaky integ tests

Signed-off-by: Riya Saxena <riysaxen@amazon.com>

* address pr comments

Signed-off-by: Riya Saxena <riysaxen@amazon.com>

---------

Signed-off-by: Riya Saxena <riysaxen@amazon.com>
Signed-off-by: Riya <69919272+riysaxen-amzn@users.noreply.github.com>
Signed-off-by: Subhobrata Dey <sbcd90@gmail.com>
Co-authored-by: Subhobrata Dey <sbcd90@gmail.com>

* fix findings api integ tests

Signed-off-by: Joanne Wang <jowg@amazon.com>

---------

Signed-off-by: Subhobrata Dey <sbcd90@gmail.com>
Signed-off-by: Joanne Wang <jowg@amazon.com>
Signed-off-by: Riya Saxena <riysaxen@amazon.com>
Signed-off-by: Riya <69919272+riysaxen-amzn@users.noreply.github.com>
Co-authored-by: Subhobrata Dey <sbcd90@gmail.com>
Co-authored-by: Riya <69919272+riysaxen-amzn@users.noreply.github.com>
riysaxen-amzn pushed a commit that referenced this pull request Mar 18, 2024
* add logic and integ tests to not add duplicate to log-types-config index

Signed-off-by: Joanne Wang <jowg@amazon.com>

* change naming for existingFieldMapping and change contains to equals

Signed-off-by: Joanne Wang <jowg@amazon.com>

---------

Signed-off-by: Joanne Wang <jowg@amazon.com>
riysaxen-amzn pushed a commit that referenced this pull request Mar 18, 2024
* add logic and integ tests to not add duplicate to log-types-config index

Signed-off-by: Joanne Wang <jowg@amazon.com>

* change naming for existingFieldMapping and change contains to equals

Signed-off-by: Joanne Wang <jowg@amazon.com>

---------

Signed-off-by: Joanne Wang <jowg@amazon.com>
@riysaxen-amzn riysaxen-amzn mentioned this pull request Mar 18, 2024
5 tasks
riysaxen-amzn added a commit that referenced this pull request Mar 18, 2024
* Fix duplicate ecs mappings which returns incorrect log index field in mapping view API (#786) (#788)

* field mapping changes

Signed-off-by: Joanne Wang <jowg@amazon.com>

* add integ test

Signed-off-by: Joanne Wang <jowg@amazon.com>

* turn unmappedfieldaliases as set and add integ test

Signed-off-by: Joanne Wang <jowg@amazon.com>

* add comments

Signed-off-by: Joanne Wang <jowg@amazon.com>

* fix integ tests

Signed-off-by: Joanne Wang <jowg@amazon.com>

* moved logic to method for better readability

Signed-off-by: Joanne Wang <jowg@amazon.com>

---------

Signed-off-by: Joanne Wang <jowg@amazon.com>

* Fix get mappings view API incorrectly returning ecs path (#867)

* add logic and integ tests to not add duplicate to log-types-config index

Signed-off-by: Joanne Wang <jowg@amazon.com>

* change naming for existingFieldMapping and change contains to equals

Signed-off-by: Joanne Wang <jowg@amazon.com>

---------

Signed-off-by: Joanne Wang <jowg@amazon.com>

* fix integ test (#918)

Signed-off-by: Joanne Wang <jowg@amazon.com>

* fix detector writeTo() method missing fields (#695)

* fix detector writeTo() method missing fields

Signed-off-by: Surya Sashank Nistala <snistala@amazon.com>

* fix test

Signed-off-by: Surya Sashank Nistala <snistala@amazon.com>

---------

Signed-off-by: Surya Sashank Nistala <snistala@amazon.com>

* removing threatIntel related code

Signed-off-by: Riya Saxena <riysaxen@amazon.com>

* removing threatIntel related code

Signed-off-by: Riya Saxena <riysaxen@amazon.com>

* ignore flaky tests

Signed-off-by: Riya Saxena <riysaxen@amazon.com>

---------

Signed-off-by: Joanne Wang <jowg@amazon.com>
Signed-off-by: Surya Sashank Nistala <snistala@amazon.com>
Signed-off-by: Riya Saxena <riysaxen@amazon.com>
Co-authored-by: Joanne Wang <jowg@amazon.com>
Co-authored-by: Surya Sashank Nistala <snistala@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants