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

[TI_RecordedFuture] Support IoC expiration #5460

Merged
merged 29 commits into from
May 15, 2023

Conversation

kcreddy
Copy link
Contributor

@kcreddy kcreddy commented Mar 7, 2023

What does this PR do?

Supports IoC (Indicators of Compromise) expiration by:

  • Removing fingerprint processor from ti_recordedfuture.threat* source indices to allow duplicate documents.
  • Adding latest transform to create destination indices containing only unexpired/latest IoCs

Checklist

  • I have reviewed tips for building integrations and this pull request is aligned with them.
  • I have verified that all data streams collect metrics or logs.
  • I have added an entry to my package's changelog.yml file.
  • I have verified that Kibana version constraints are current according to guidelines.

How to test this PR locally

Related issues

Screenshots

@kcreddy kcreddy added enhancement New feature or request Integration:ti_recordedfuture Recorded Future labels Mar 7, 2023
@kcreddy kcreddy self-assigned this Mar 7, 2023
@elasticmachine
Copy link

elasticmachine commented Mar 7, 2023

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2023-05-12T05:55:35.247+0000

  • Duration: 15 min 49 sec

Test stats 🧪

Test Results
Failed 0
Passed 11
Skipped 0
Total 11

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

@elasticmachine
Copy link

elasticmachine commented Mar 7, 2023

🌐 Coverage report

Name Metrics % (covered/total) Diff
Packages 100.0% (1/1) 💚
Files 100.0% (2/2) 💚 2.983
Classes 100.0% (2/2) 💚 2.983
Methods 100.0% (16/16) 💚 8.312
Lines 97.549% (199/204) 👍 5.505
Conditionals 100.0% (0/0) 💚

@kcreddy
Copy link
Contributor Author

kcreddy commented Mar 8, 2023

/test

@elasticmachine
Copy link

Pinging @elastic/security-external-integrations (Team:Security-External Integrations)

#
# Fingerprint event: _id = hash(dataset + indicator type + indicator value)
#
- fingerprint:
Copy link
Member

Choose a reason for hiding this comment

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

Every hour (that's the default interval) this integration downloads the full CSV file. If a given CSV line has already been indexed, is there a need to index it again? Could we fingerprint the raw CSV to avoid duplicating data? (update: It looks like retention is implemented based on event.ingested so this does depend on the data being continuously reindexed again in order to keep a full threat picture in the "latest" index.)

Copy link
Member

Choose a reason for hiding this comment

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

Another step we could take to minimize retaining a bunch of duplicate data is to institute a custom ILM policy for this data such that it keeps a limited amount of the raw data (like a few hours worth). After the data has been read by the latest transforms there is not much use for it. In other words, as long as we have the output of the transform then the original data stream does not need much retention.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it, yeah it does make sense. I see there is a way to specify ILM policy in package-spec I can work on it. In the documentation for the integration, we probably need to mention users about how the source polling interval, transform retention_policy, and ILM delete.min_age need to be configured.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added ILM policy to rollover after 2 days and delete 3 days after rollover. Added README explaining the same.

retention_policy:
time:
field: event.ingested
max_age: 15m
Copy link
Member

Choose a reason for hiding this comment

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

How was this value determined? Does this need to be longer than interval at which data is fetched from the API? What happens if we fetch data every hour, but only retain the last 15 minutes of latest data for an IOC? Does that mean we have 45 minutes where this logs-ti_recordedfuture_latest.threat is empty?

My thinking is that this value needs to be at least as long as the fetch interval plus the sync time delay. Then the

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This value can be anything less than the source interval through which the API gets polled.
The reason is:

  • If it is greater than the interval, expired IOC still remain in destination indices.
  • If it is less, based on my tests, it wouldn't delete the documents unless the transform runs again, i.e., same interval when documents gets ingested via source polling. In other words, deletion via this retention_policy.time.max_age only happens when the transform is triggered. The fields that determine whether to trigger transform are: frequency and latest.sort.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed max_age to 24h after discussion with Jamie as it seems like a reasonable default since this is not a user configurable parameter.

@@ -16,13 +16,13 @@ An example event for `threat` looks as following:

Copy link
Member

Choose a reason for hiding this comment

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

The docs are going to need to be updated to explain how IoC expiration works and what index to read from when building indicator match rules.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added README explaining IOC expiration and ILM policy on source indices.

@@ -0,0 +1,21 @@
source:
index:
- ".ds-logs-ti_recordedfuture.threat-default*"
Copy link
Member

Choose a reason for hiding this comment

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

What are the downsides to sourcing data from all logs-ti_recordedfuture.threat namespaces?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My understanding was that if multiple namespaces with same dataset is added by the user when setting up the integration, then transform could read from all namespaces/datastreams and deduce the latest.

But also hardcoding to default isn't good either. Also the transform itself is named like this as per spec.. So, in case of default namespace, our transform is being named as: logs-ti_recordedfuture.latest_ioc-default-0.1.0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the index to ".ds-logs-ti_recordedfuture.threat-*. So it assumes only 1 namespace exists for this integration, whatever is its value.

mapping:
ignore_above: 1024
type: keyword
date_detection: true
Copy link
Member

Choose a reason for hiding this comment

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

Why did you enable date_detection? Do we have dynamic fields that contain date values that are not mapped as date?

Copy link
Contributor Author

@kcreddy kcreddy Apr 6, 2023

Choose a reason for hiding this comment

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

Actually, there aren't. Set it to false

@kcreddy
Copy link
Contributor Author

kcreddy commented Apr 21, 2023

/test

@kcreddy
Copy link
Contributor Author

kcreddy commented Apr 24, 2023

/test

@kcreddy
Copy link
Contributor Author

kcreddy commented Apr 25, 2023

Testing with package upgrade:

The transform isn't getting updated, when package is upgraded with elastic-package build && elastic-package stack up --version=8.8.0-SNAPSHOT -d -v -s package-registry.

Test 1: No code change, only modified manifest and changelog

- version: "1.8.0"
  changes:
    - description: IOC upgrade test
      type: enhancement
      link: https://github.com/elastic/integrations/issues/1111

Test 2: Modified transform with max_age: 12h + modified manifest and changelog

- version: "1.9.0"
  changes:
    - description: IOC upgrade test with transform
      type: enhancement
      link: https://github.com/elastic/integrations/issues/2222

Results:

Screenshot 2023-04-25 at 8 14 20 PM

Screenshot 2023-04-25 at 8 02 57 PM

Screenshot 2023-04-25 at 8 03 05 PM

Screenshot 2023-04-25 at 8 03 36 PM

@andrewkroh
Copy link
Member

andrewkroh commented Apr 26, 2023

@kcreddy Thanks for testing this manually. Unless my expectations are wrong, I assume this is a bug in Fleet. Can you create a new elastic/kibana Fleet issue to get some clarity on on what to expect from transform upgrades in Fleet packages. If this is the expected behavior for transforms then what are the steps required by users to upgrade. We need to know this so we can document it and support users.

@kcreddy
Copy link
Contributor Author

kcreddy commented Apr 27, 2023

Hey @andrewkroh
There are certain changes to how the transform versioning is done in: elastic/kibana#155453
With this latest SNAPSHOT, I ran few tests again to simulate the issue.

Interestingly, the new SNAPSHOT version doesn’t seem to create destination index with package version suffix, as now the destination index name is exactly as given in the transform.yml:

"dest": {
    "index": "logs-ti_recordedfuture_latest.threat",    (# --- > no version suffix)
    "aliases": []
  },

Also these indices are not deleted. This should work for us now.

However, the transform update issue/bug during the package upgrade is still present. I have added the issue with details here: elastic/kibana#155982 and requested Kyle Pollich to take a look.

@kcreddy
Copy link
Contributor Author

kcreddy commented May 3, 2023

Starting 8.8, when upgrading the transform, the fleet_transform_version needs to be bumped up.
The transform update is now working fine as mentioned in elastic/kibana#155982 (comment)

index:
- "logs-ti_recordedfuture.threat-*"
dest:
index: "logs-ti_recordedfuture_latest.threat-1.0.0"
Copy link
Member

@andrewkroh andrewkroh May 9, 2023

Choose a reason for hiding this comment

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

When would this version be changed? I think we should have a comment here to help maintainers understand when this should happen and what, if any, is the relationship to the transform version.

I have been thinking about whether we should have some versioning convention here. Like perhaps we use a simple one up counter that's not semver and not tied in with the transform version. This value gets incremented only if there is some incompatible (breaking) schema change.

If we did a version change, we would probably need a means to automatically purge the old destination index so that readers using wildcards (logs-*_latest-*) are not getting results from the old index.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added comments for dest index versioning and a NOTE for future contributors.

Copy link
Member

Choose a reason for hiding this comment

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

That comment is missing when and why the transform index version should be changed

IIUC if we ever need some kind of breaking schema then we would 1) increment dest index version and 2) increment the transform version. Without (2) the change to the destination index would have no effect.

Is there any issue that you know of is tracking the ability to cleanup an orphaned destination index? We should register our requirement in elastic/package-spec and link to the issue in the comment. If we can't have that feature, then next time we need to make a change we would need to pursue some other approach like documenting a manual cleanup.


Like perhaps we use a simple one up counter that's not semver and not tied in with the transform version

What do you think about that? In addition I don't want to it to appear that the transform's dest.index version is related to the package version. That might lead to confusion about why the dest.index did not change after an upgrade. Using a simple -1, -2, ... versioning approach there could accomplish that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That sounds good. I also agree that transform version has to be different from the dest.index version. Simple versions should also do the trick.

@@ -1,6 +1,9 @@
# Use of "*" to use any namespace defined.
Copy link
Member

Choose a reason for hiding this comment

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

s/any namespace/all namespaces/

Comment on lines 5 to 8
# The version suffix on destination index must be explicitly set.
# NOTE: During version change, ensure through some mechanism old destination index automatically purges so queries on wild card such as logs-*_latest-* doesn't result in duplicates
dest:
index: "logs-ti_recordedfuture_latest.threat-1.0.0"
Copy link
Member

Choose a reason for hiding this comment

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

Here's what I am thinking...

Suggested change
# The version suffix on destination index must be explicitly set.
# NOTE: During version change, ensure through some mechanism old destination index automatically purges so queries on wild card such as logs-*_latest-* doesn't result in duplicates
dest:
index: "logs-ti_recordedfuture_latest.threat-1.0.0"
# The version suffix on the dest.index should be incremented if a breaking change
# is made to the index mapping. You must also bump the fleet_transform_version
# for any change to this transform configuration to take effect. The old destination
# index is not automatically removed. We are dependent on {issue} to give
# us that ability in order to prevent having duplicate IoC data and prevent query
# time field type conflicts.
dest:
index: "logs-ti_recordedfuture_latest.threat-1"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you! Updated as suggested. Linked the underlying issue.

Copy link
Member

@andrewkroh andrewkroh left a comment

Choose a reason for hiding this comment

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

👍

@kcreddy kcreddy merged commit 58eda22 into elastic:main May 15, 2023
@kcreddy kcreddy deleted the ioc_expire_rec_future branch February 7, 2025 09:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Integration:ti_recordedfuture Recorded Future
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[TI_RecordedFuture] Support IoC expiration
3 participants