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

Towards improving read and rendering of results #1396

Merged
merged 24 commits into from
Mar 30, 2023

Conversation

ooprathamm
Copy link
Contributor

@ooprathamm ooprathamm commented Mar 22, 2023

closes #991

Checklist

  • No CHANGELOG update needed
  • No new tests needed
  • No documentation update needed

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Please add bug fixes, new features, breaking changes and anything else you think is worthwhile mentioning to the master (unreleased) section of CHANGELOG.md. If no CHANGELOG update is needed add the following to the PR description: [x] No CHANGELOG update needed

@ooprathamm

This comment was marked as off-topic.

Copy link
Collaborator

@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.

im sorry we didn't provide more detail on how we were thinking about going about this. i think we should rely on --json > output.json to save the result document. for new functionality, we'll have to find a way to identify that the input file is a result document, and then do the rendering. determining the file type might be done like: 1) check for json content, a few strings we expect to be present, and then trying to deserialize it, or 2) requiring the user to specify format=result or similar as a cli argument.

please dont hesitate to reach out with questions or suggestions - we're excited to work with you on this enhancement!

capa/main.py Outdated Show resolved Hide resolved
@ooprathamm

This comment was marked as off-topic.

@mr-tz
Copy link
Collaborator

mr-tz commented Mar 23, 2023

determining the file type might be done like: 1) check for json content, a few strings we expect to be present, and then trying to deserialize it, or 2) requiring the user to specify format=result or similar as a cli argument.

I'm also in favor of this so we don't need a new command line argument and instead reuse sample. This can also simplify some of the things you're concerned about.

@ooprathamm

This comment was marked as off-topic.

@williballenthin
Copy link
Collaborator

i've updated #991 with more explicit details on how we were thinking this might work. please use this as guidance or coordinate with us if you want to propose alternative logic. we're open to other ideas but also sensitive to the current architecture of capa and what's on the roadmap.

@williballenthin williballenthin marked this pull request as draft March 23, 2023 09:33
@ooprathamm

This comment was marked as outdated.

@ooprathamm ooprathamm marked this pull request as ready for review March 25, 2023 12:07
@ooprathamm
Copy link
Contributor Author

@williballenthin @mr-tz Made the final changes :)
Able to successfully parse the meta and capabilities from the output json

Output
image

@github-actions github-actions bot dismissed their stale review March 26, 2023 17:24

CHANGELOG updated or no update needed, thanks! 😄

Copy link
Collaborator

@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.

see feedback inline and let me know what questions you have

@williballenthin
Copy link
Collaborator

also, please prepare a few unit tests that demonstrate how the new functionality proposed here works as intended.

@ooprathamm
Copy link
Contributor Author

ooprathamm commented Mar 29, 2023

@williballenthin @mr-tz I hope I have resolved all the above discussions.

Passing Testsuite -
image

Copy link
Collaborator

@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.

we are really close! i have a few requests inline, but i dont anticipate any of these should be difficult to address. if they are, dont hesitate to reach out and i'd be hapy to lend a hand. overall, i feel that the quality of this PR is really good, nice job!

capa/features/extractors/common.py Show resolved Hide resolved
capa/main.py Show resolved Hide resolved
capa/render/result_document.py Outdated Show resolved Hide resolved
capa/render/result_document.py Outdated Show resolved Hide resolved
tests/test_result_document.py Outdated Show resolved Hide resolved
@williballenthin
Copy link
Collaborator

there's also some conflicts to deal with from master, and we'll want the linters to pass again (code style is broken right now but should be fixed soon. if this becomes a blocker we can find a way around it).

@ooprathamm
Copy link
Contributor Author

@williballenthin I have addressed the suggestions in inline

@williballenthin
Copy link
Collaborator

williballenthin commented Mar 29, 2023

awesome, logic looks good!

please:

  • resolve the conflicts with master, and
  • ensure isort and black linters are happy with your changes (mypy is broken right now)

@ooprathamm

This comment was marked as outdated.

@williballenthin
Copy link
Collaborator

williballenthin commented Mar 30, 2023 via email

@williballenthin
Copy link
Collaborator

image

this failure is unexpected, but im not sure if its from master. let me triage it further. otherwise, PR looks great and i'll merge shortly!

@williballenthin
Copy link
Collaborator

great, thanks for the arch fix, @ooprathamm!

once the remaining tests pass (aside from codestyle) i'll merge, thank again!

@ooprathamm
Copy link
Contributor Author

@williballenthin I have fixed the reason of failing elf arch extraction.
I hope these were the final changes to this PR. It was awesome working on this one, got me really brainstorming.
Thanks for providing your guidance throughout.

Passing local test suite
image

@williballenthin williballenthin merged commit 99ee317 into mandiant:master Mar 30, 2023
@williballenthin
Copy link
Collaborator

🎉

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.

read and render results document
3 participants