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

Update to evtx_dump_json.py #90

Closed
wants to merge 0 commits into from
Closed

Conversation

ajread4
Copy link
Contributor

@ajread4 ajread4 commented Jun 22, 2024

fixed JSON dump to collect all necessary xml data from logs. Originally missing pivotal Sysmon data from within the System portion of Event XML data.

@williballenthin williballenthin self-assigned this Jun 22, 2024
@williballenthin williballenthin self-requested a review June 22, 2024 17:24
@williballenthin williballenthin removed their assignment Jun 22, 2024
Copy link
Owner

@williballenthin williballenthin left a comment

Choose a reason for hiding this comment

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

would you please provide some test data and demonstrate how the script extracts further information? it would be best to add a test case, too, so we asset the script works well in the future after unrelated changes.


# Print the JSON object for the specific log if not requested to output to file
if not args.output:
print("Original")
Copy link
Owner

Choose a reason for hiding this comment

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

i think the output should be valid json, so it can be piped to jq, for example. so perhaps make these keys in a bigger dictionary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops, that was part of my bug fix steps, will remove

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed those statements in most recent commit and added functionality to pipe to jq
Screenshot from 2024-06-22 14-58-41

@ajread4
Copy link
Contributor Author

ajread4 commented Jun 22, 2024

would you please provide some test data and demonstrate how the script extracts further information? it would be best to add a test case, too, so we asset the script works well in the future after unrelated changes.

A good example of this is in the screenshot below. I missed out on key data like "EventID," "TimeCreated", and "Computer" using the old version since I only focused on the "EventRecordID" within the System portion of the Eventlog xml. The new version parses the entire System and EventData sections of the Eventlog xml now!

The below screenshot has the old version on top and the new version below.

image

@williballenthin
Copy link
Owner

Are you able to share that log file, or generate a similar one?

@ajread4
Copy link
Contributor Author

ajread4 commented Jun 22, 2024

Are you able to share that log file, or generate a similar one?

Definitely, how would you like it?

@williballenthin
Copy link
Owner

let's add it to tests/data and we can reference it from a test.

@ajread4
Copy link
Contributor Author

ajread4 commented Jun 22, 2024

added to tests/data!

@ajread4
Copy link
Contributor Author

ajread4 commented Jun 23, 2024

Is there a reason why the tests fail each time? Using that form of testing is new to me!

@williballenthin
Copy link
Owner

Wow, it had been a bit since I looked at the testing code. I went through and made a bunch of fixes and enhancements. Now the tests should work well (they do in CI: https://github.com/williballenthin/python-evtx/actions/runs/9641798469).

I hate to do this to you, but would you update this PR with the changes to master? Github may have a button for it above, or you can pull from master locally. I think the only conflicts should be around single vs double quotes.

@ajread4
Copy link
Contributor Author

ajread4 commented Jun 29, 2024

Definitely, I'll rebase locally and then put in a new PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants