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.

Sign up for GitHub

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

Jump to bottom

Draft of IOC service #1048

Merged
merged 7 commits into from
Jun 13, 2024
Merged

Draft of IOC service #1048

merged 7 commits into from
Jun 13, 2024

Conversation

AWSHurneyt
Copy link
Collaborator

Description

Implemented draft of IocService.

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.

eirsep
eirsep reviewed May 29, 2024
View reviewed changes
src/main/java/org/opensearch/securityanalytics/services/IocService.java
* @return TRUE if the index is an IOC-related system index, and exists; else returns FALSE.
*/
public boolean hasIocSystemIndex(String index) {
return index.startsWith(IOC_INDEX_NAME_BASE) && this.clusterService.state().routingTable().hasIndex(index);
Copy link
Member

Choose a reason for hiding this comment

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

why do we have to do index.startsWith(IOC_INDEX_NAME_BASE) and why is method public?

Copy link
Collaborator Author

@AWSHurneyt AWSHurneyt May 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@eirsep I added the startsWith check just to confirm that the function is only being used to create indexes for the purposes of this feature. Do you think that check should be removed?

I implemented it as public just out of habit in case I might need it elsewhere; I'll change it to private in a separate PR as I'll need to refactor the tests that use it (unless we're fine with leaving it as a public function).

eirsep
eirsep requested changes May 30, 2024
View reviewed changes
build.gradle Outdated Show resolved Hide resolved
@AWSHurneyt AWSHurneyt force-pushed the 3.0-threat-intel-dev branch 2 times, most recently from 406f827 to 5429ac9 Compare May 31, 2024 00:18
@AWSHurneyt AWSHurneyt marked this pull request as ready for review May 31, 2024 01:20
@jowg-amazon jowg-amazon force-pushed the feature/threat_intel branch from 243a0ca to cadd169 Compare June 3, 2024 18:24
@eirsep eirsep force-pushed the feature/threat_intel branch from 243a0ca to 4fc0d84 Compare June 3, 2024 18:56
@AWSHurneyt AWSHurneyt force-pushed the 3.0-threat-intel-dev branch from 16a1e3a to 03364a2 Compare June 4, 2024 21:40
eirsep
eirsep reviewed Jun 5, 2024
View reviewed changes
src/main/resources/mappings/ioc_mapping.json Outdated
"type": "keyword"
},
"name": {
"type": "text",
Copy link
Member

Choose a reason for hiding this comment

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

why cant name be keyword like id?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Revised to keyword type.

eirsep
eirsep reviewed Jun 5, 2024
View reviewed changes
src/main/resources/mappings/ioc_mapping.json Outdated
},
"created": {
"type": "date",
"format": "strict_date_optional_time||epoch_millis"
Copy link
Member

Choose a reason for hiding this comment

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

do we need this restriction?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We use similar format for the mappings of some other plugin assets (e.g., detectors). Did you have another format in mind?

For now, I've revised this to strict_date_time||epoch_millis. My thoughts are that the create date should be required whereas the modified date could be null if the entry hasn't been updated.

eirsep
eirsep reviewed Jun 5, 2024
View reviewed changes
src/main/resources/mappings/ioc_mapping.json
},
"modified": {
"type": "date",
"format": "strict_date_optional_time||epoch_millis"
Copy link
Member

Choose a reason for hiding this comment

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

do we need this restriction?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We use similar format for the mappings of some other plugin assets (e.g., detectors). Did you have another format in mind?

eirsep
eirsep requested changes Jun 5, 2024
View reviewed changes
Copy link
Member

@eirsep eirsep left a comment

Choose a reason for hiding this comment

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

can you check some minor comments?

jowg-amazon
jowg-amazon reviewed Jun 6, 2024
View reviewed changes
src/main/java/org/opensearch/securityanalytics/action/FetchIocsActionResponse.java

public FetchIocsActionResponse(List<IOC> iocs) {
super();
iocs.forEach( ioc -> this.iocs.add(new IocDto(ioc)));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be done in transport/service class?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@jowg-amazon It can be. The FetchIocsActionResponse object is just a placeholder I've been using to test the changes. We can refactor and remove it before launch.

@eirsep eirsep force-pushed the feature/threat_intel branch 2 times, most recently from 4fc0d84 to 63dd56c Compare June 6, 2024 06:12
@AWSHurneyt AWSHurneyt force-pushed the 3.0-threat-intel-dev branch from fa58d0c to 186264a Compare June 7, 2024 21:43
@AWSHurneyt
Copy link
Collaborator Author

can you check some minor comments?

@eirsep Responded.

@AWSHurneyt AWSHurneyt mentioned this pull request Jun 8, 2024
Closed
5 tasks
eirsep
eirsep reviewed Jun 13, 2024
View reviewed changes
src/main/java/org/opensearch/securityanalytics/action/FetchIocsActionResponse.java
import java.util.Collections;
import java.util.List;

public class FetchIocsActionResponse extends ActionResponse implements ToXContentObject {
Copy link
Member

Choose a reason for hiding this comment

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

NIT: let's keep consistent naming: ListIocsResponse?

eirsep
eirsep reviewed Jun 13, 2024
View reviewed changes
src/main/java/org/opensearch/securityanalytics/model/IOC.java

public class IocDao implements Writeable, ToXContentObject {
private static final Logger logger = LogManager.getLogger(IocDao.class);
public class IOC implements Writeable, ToXContentObject {
Copy link
Member

Choose a reason for hiding this comment

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

how come Ioc is a concrete class and also STIX2Ioc?

eirsep
eirsep reviewed Jun 13, 2024
View reviewed changes
src/main/java/org/opensearch/securityanalytics/model/IocDto.java
.timeField(IOC.MODIFIED_FIELD, modified)
.field(IOC.DESCRIPTION_FIELD, description)
.field(IOC.LABELS_FIELD, labels)
.field(IOC.FEED_ID_FIELD, feedId)
.endObject();
}

public static IocDto parse(XContentParser xcp, String id) throws IOException {
Copy link
Member

Choose a reason for hiding this comment

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

this defeats the purpose of having separate classes for DTO and the model.. if there is ever a divergence we would end up having to re-write this method

eirsep
eirsep reviewed Jun 13, 2024
View reviewed changes
Copy link
Member

@eirsep eirsep left a comment

Choose a reason for hiding this comment

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

Plz invoke IocService from TIFConfigService

@eirsep eirsep merged commit 17f7074 into opensearch-project:feature/threat_intel Jun 13, 2024
2 checks passed
AWSHurneyt added a commit to AWSHurneyt/security-analytics that referenced this pull request Jun 25, 2024
@AWSHurneyt
Draft of IOC service (opensearch-project#1048)
9357e64 
* Removed unused imports. Removed redundant helper function.

Signed-off-by: AWSHurneyt <[email protected]>

* Added note about system index refactoring.

Signed-off-by: AWSHurneyt <[email protected]>

* Implemented draft of IocService.

Signed-off-by: AWSHurneyt <[email protected]>

* Made changes based on PR feedback.

Signed-off-by: AWSHurneyt <[email protected]>

* Fixed test helper function.

Signed-off-by: AWSHurneyt <[email protected]>

* Removed unused imports.

Signed-off-by: AWSHurneyt <[email protected]>

* Adjusted mappings based on PR feedback.

Signed-off-by: AWSHurneyt <[email protected]>

---------

Signed-off-by: AWSHurneyt <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jowg-amazon jowg-amazon jowg-amazon left review comments

@eirsep eirsep eirsep requested changes

@amsiglan amsiglan Awaiting requested review from amsiglan amsiglan is a code owner

@getsaurabh02 getsaurabh02 Awaiting requested review from getsaurabh02 getsaurabh02 is a code owner

@lezzago lezzago Awaiting requested review from lezzago lezzago is a code owner

@praveensameneni praveensameneni Awaiting requested review from praveensameneni praveensameneni is a code owner

@sbcd90 sbcd90 Awaiting requested review from sbcd90 sbcd90 is a code owner

@engechas engechas Awaiting requested review from engechas engechas is a code owner

@goyamegh goyamegh Awaiting requested review from goyamegh goyamegh is a code owner

@riysaxen-amzn riysaxen-amzn Awaiting requested review from riysaxen-amzn riysaxen-amzn is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@AWSHurneyt @eirsep @jowg-amazon