Add stix-extract tool #48

felddy · 2023-06-09T21:28:38Z

Add stix-extract tool

🗣 Description

This PR introduces a new tool, stix-extract, to the ioc-scanner project. This tool extracts indicators of compromise (IoCs) from STIX files and outputs them in a format that can be used by the existing ioc-scan tool. Additionally the tool can extract IPs, FQDNs, and URLs for use in other parts of the IoC scanning process.

💭 Motivation and Context

The motivation for adding this tool is to allow for the use of STIX files as a source of IoCs. STIX files are a widely-adopted standard for threat intelligence sharing, and by adding the ability to directly process these files, we increase the utility and flexibility of the ioc-scanner project.

🧪 Testing

Testing for this change included manual usage of the tool to verify its functionality. The tool was used with different STIX files and combined with ioc-scan in various ways to ensure compatibility and functionality. Further automated testing can be added in the future to verify the robustness of this addition.

✅ Pre-approval Checklist

This PR has an informative and human-readable title.
Changes are limited to a single goal - eschew scope creep!
All relevant type-of-change labels have been added.
I have read the CONTRIBUTING document.
These code changes follow cisagov code standards.
All relevant repo and/or project documentation has been updated to reflect the changes in this PR.
All new and existing tests pass.

jsf9k

I pointed out a few places for improvement, but this is nice work.

src/ioc_scan/stix_extract.py

jsf9k · 2023-06-10T03:35:03Z

I think adding this script may warrant a bump of the patch portion of the version as well.

felddy · 2023-06-10T04:12:35Z

I think adding this script may warrant a bump of the patch portion of the version as well.

Bumped in: 1654c50

jsf9k

Approved, but please make sure to create a release after merging. (I see that you removed that checkbox item from the PR description, but now that the version is bumped it is necessary.)

jsf9k · 2023-06-10T14:03:16Z

Also please note for the future that we generally add a co-author for commits created in response to reviewer suggestion by appending a line like this to the commit comment:

Co-authored-by: Shane Frasier <jeremy.frasier@gwe.cisa.dhs.gov>

mcdonnnj · 2023-06-11T06:18:13Z

src/ioc_scan/stix_extract.py

+#!/usr/bin/env python3
+
+"""


This should not be included in a file that is part of a package. The ioc_scanner.py file is an unpleasant hacky thing that does not conform to our general Python package guidelines and I see no rationale for this module to be configured similarly.

Suggested change

#!/usr/bin/env python3

"""

"""

Hi @mcdonnnj , Is the suggested change being made? I still notice in the file stix_extract.py the first line reads as #!/usr/bin/env python3.

mcdonnnj · 2023-06-11T06:20:05Z

src/ioc_scan/stix_extract.py

+
+if __name__ == "__main__":
+    sys.exit(main())


Same rationale as the shebang comment.

Suggested change

if __name__ == "__main__":

sys.exit(main())

mcdonnnj · 2023-06-11T06:26:07Z

src/ioc_scan/stix_extract.py

+    print(f"\n{'#' * 20}\n# IP Addresses\n{'#' * 20}\n")
+    for ip in ip_addresses:
+        print(ip)


If we're going to have four instances of the same logic I think we should DRY this out into a function. All four data sources are iterables and we're printing the headers with identical formatting.

mcdonnnj · 2023-06-11T06:28:36Z

src/ioc_scan/stix_extract.py

+                            best_priority = hash_priority[h.type_.value]
+                    if best_hash is not None:
+                        hashes.append(best_hash)
+            elif object_type == "DomainNameObjectType":


Minor but would you adjust this elif so that the elif checks are alphabetical? I think that would improve maintainability since we're just checking a bunch of strings.

Not sure if you'd want to make those lists alphabetical or not considering we've done this before. LGTM tho

mcdonnnj · 2023-06-11T06:36:59Z

src/ioc_scan/stix_extract.py

It would be nice if this file had typehints.

mcdonnnj · 2023-06-11T06:46:16Z

tests/test_stix_extract.py

With all of the mocking and patching there is no true end-to-end test of this module's functionality. I would almost prefer crafted STIX documents that would naturally be parsed to provide the expected results instead. Using so much intentionally crafted data runs the risk of missing breaking changes in the dependency package (especially since there is no version pin around the package). Additionally there is nothing to test providing the STIX document through stdin that I see.

jasonodoom · 2023-06-13T20:13:18Z

tests/test_stix_extract.py

Code looks good, most of the "errors" relate to long lines.

dav3r · 2023-11-03T14:30:21Z

@felddy Are you planning to return to this PR any time soon?

felddy added documentation This issue or pull request improves or adds to documentation improvement This issue or pull request will add or improve functionality, maintainability, or ease of use python Pull requests that update Python code labels Jun 9, 2023

felddy requested review from dav3r and jasonodoom as code owners June 9, 2023 21:28

felddy self-assigned this Jun 9, 2023

felddy requested review from jsf9k and mcdonnnj as code owners June 9, 2023 21:28

felddy enabled auto-merge June 9, 2023 21:28

felddy force-pushed the improvement/stix-extract branch 3 times, most recently from 205b2a0 to 65f6c34 Compare June 9, 2023 22:57

felddy added 3 commits June 9, 2023 19:04

Add stix extraction utility

28d58c2

Add required library and entry point

2e6a280

Update docs to include new util

f7ffdeb

felddy force-pushed the improvement/stix-extract branch from 65f6c34 to 62afafc Compare June 9, 2023 23:04

Add tests for stix-extract

5320414

felddy force-pushed the improvement/stix-extract branch from 62afafc to 5320414 Compare June 9, 2023 23:17

jsf9k requested changes Jun 10, 2023

View reviewed changes

src/ioc_scan/stix_extract.py Outdated Show resolved Hide resolved

src/ioc_scan/stix_extract.py Outdated Show resolved Hide resolved

jsf9k added the test This issue or pull request adds or otherwise modifies test code label Jun 10, 2023

felddy added 4 commits June 10, 2023 00:00

Optimise calculation by reusing ip object

a040345

Direct error message to stderr

de9c841

Normalize version usage across tools

971c4fb

Bump version patch

1654c50

jsf9k approved these changes Jun 10, 2023

View reviewed changes

mcdonnnj requested changes Jun 11, 2023

View reviewed changes

jsf9k requested a review from a team June 12, 2023 17:22

jasonodoom reviewed Jun 13, 2023

View reviewed changes

tests/test_stix_extract.py

Copy link

Contributor

jasonodoom Jun 13, 2023

Choose a reason for hiding this comment

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

Code looks good, most of the "errors" relate to long lines.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add stix-extract tool #48

Add stix-extract tool #48

felddy commented Jun 9, 2023

jsf9k left a comment

jsf9k commented Jun 10, 2023

felddy commented Jun 10, 2023

jsf9k left a comment

jsf9k commented Jun 10, 2023 •

edited

Loading

mcdonnnj Jun 11, 2023

dv4harr10 Oct 11, 2023

mcdonnnj Jun 11, 2023

mcdonnnj Jun 11, 2023

mcdonnnj Jun 11, 2023

jasonodoom Jun 13, 2023

mcdonnnj Jun 11, 2023

mcdonnnj Jun 11, 2023

jasonodoom Jun 13, 2023

dav3r commented Nov 3, 2023

Add stix-extract tool #48

Are you sure you want to change the base?

Add stix-extract tool #48

Conversation

felddy commented Jun 9, 2023

Add stix-extract tool

🗣 Description

💭 Motivation and Context

🧪 Testing

✅ Pre-approval Checklist

jsf9k left a comment

Choose a reason for hiding this comment

jsf9k commented Jun 10, 2023

felddy commented Jun 10, 2023

jsf9k left a comment

Choose a reason for hiding this comment

jsf9k commented Jun 10, 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

dav3r commented Nov 3, 2023

jsf9k commented Jun 10, 2023 •

edited

Loading