-
Notifications
You must be signed in to change notification settings - Fork 6
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
Add campaign summary export #40
Conversation
β¦ functions added to write to console and file using the same string, and to find a unique target in a collection of clicks.
β¦ampaign_summary_export Merging main develop branch back into add_campaign_summary_export
β¦ampaign_summary_export Merging latest develop into feature branch. This should take the version number to 0.0.5
β¦ampaign_summary_export Merging the bug fix for datetime selection not appearing into this branch. It will be included as release 0.0.6
β¦ campaign. Testing correction and write_campaign_click_summary
β¦ict for JSON output
Small typo in output string was found and corrected here. Ready for PR.
β¦ a phishing campaign.
β¦ixed a field name in the source code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
π LGTM.
This is outside of the scope of this PR in my opinion but something we should consider for future cleanup/improvements is to circle back and create an issue to clean up all outputs in this project. For example, where we are using logging along with print to dump to stdout and try to make it consistent. Same for any file output paths we can standardize.
Great point @nickviola! I suggest using logging everywhere. Please make an issue for this so the idea is captured and not forgotten. |
This pull request introduces 1 alert when merging 76c14d6 into 22ccab8 - view on LGTM.com new alerts:
|
Datetime parse is not needed and removed.
This pull request introduces 1 alert when merging e4aaf68 into 22ccab8 - view on LGTM.com new alerts:
|
The logger was prepending all messages to the file with INFO. This was undesirabl for a report, so it was changed back to doing just a regular file out. The contents of the campaign summaries are no longer output to the console, only to file.
Trying to improve readability and separate the campaigns in an assessment
This pull request introduces 1 alert when merging b647368 into 22ccab8 - view on LGTM.com new alerts:
|
Campaign click summary has been renamed to Campaign Summary, along with changes to the reported fields. The README.md file has been updated to account for these changes.
This pull request introduces 1 alert when merging 3f7974b into 22ccab8 - view on LGTM.com new alerts:
|
The variable was declared at the start of the function, and then initialized at line 274. The first declaration is unnecessary and removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please take a look at my feedback - thanks!
Also, reminder that this conversation is still unresolved.
β¦hish-generated, in response to PR feedback.
Changes to logging formats, new warning log for unprocessable campaigns, and changing find_unique_user_count() to use a set over a list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made one suggestion for improvement.
Should have been iterating twice over adding click objects to a test data set, but accidentally let it iterate once.
Changing error prompt to be more useful, and switching if-else block to regex
β¦ prior to mergeing add_campaign_summary_export
Doing a commit suggestion here in Github. Looks like pre-commit hooks on my local branch will overwrite the requested changes and revert back to the incorrect way. Co-authored-by: dav3r <david.redmin@trio.dhs.gov>
This reverts commit a166791. Reverting changes to import. Leaving conftests imports back to the way isort wants them to be
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for replying to all of my feedback and questions! ποΈ
π£ Description
This change adds the capability to export click summaries for the campaigns in an assessment. The output of these new features are a click summary in both a text file format and a JSON file. This data consists of the number of emails sent, the number of unique users who responded, the rate of user clicks, and the total number of clicks returned.
UPDATE 8/30: Per request with @BenBreaksThings, changes were made to improve the campaign summaries. They are now no longer being called campaign click summaries, but simply campaign summaries. The fields reported have expanded to include email subject, from address, and the campaign start and end dates. Some significant refactor happened as a result and the changes will have to be reviewed.
π Motivation and context
The PCA team runs a script against Cobalt-Strike, called cs_phish_parse that creates this report from a series of csv input files. This script is run manually by operators, and it was requested that it gets ported to Gophish and included as part of the export process. The PCA team requested both the JSON and plain text file output of the report.
π§ͺ Testing
The Gophish-export command was run against multiple assessments, with campaigns that contain clicks and those that do not. The values output in the final click summary report reflected the correct number of emails sent, clicked on, and the correct number of unique users.
A test case was also written to test the function that counts the number of unique users that clicked on a campaign email.
π· Screenshots (if appropriate)
β Checklist
in code comments.
to reflect the changes in this PR.