-
Notifications
You must be signed in to change notification settings - Fork 9
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
Improve keyword scans #53
Conversation
Use zgrep to search in rolled-over logs
Provide hostname of instance for more clarity in log output
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.
This seems reasonable and LGTM. I have one suggestion just to update a comment to reflect the change in functionality.
Co-authored-by: Nick <50747025+mcdonnnj@users.noreply.github.com>
Instead of ignoring `--ingnore-match --recursive` look for `grep` command
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 have a couple of small requests:
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.
π Good stuff!
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.
@cisagov/team-ois I'm a bit hesitant about the version bump given that this PR does not change the Python package functionality at all. Thoughts?
I was on the fence about that also, but I told @bra1ncramp to do it. Since we are changing the way an included script works with this PR, I felt it was warranted, but honestly, I'm fine either way if you and @jsf9k want to overrule that. |
I think anytime we make meaningful changes we are potentially breaking functionality (in this case, that of the script in |
I get that but my core rationale is that the file only exists if you clone the repository or download it manually. The |
OK, that is a valid argument. I'm fine either way, so I'm happy to defer to @mcdonnnj here. @mcdonnnj - do you mind removing that version bump commit and force-pushing? |
Co-authored-by: dav3r <david.redmin@gwe.cisa.dhs.gov>
Co-authored-by: dav3r <david.redmin@gwe.cisa.dhs.gov>
5e67858
to
0efbc6c
Compare
Since the version bump was removed, I went ahead and deleted the pre-merge ("Finalize version") and post-merge ("Create a release") checklist items from the PR description now that they are no longer applicable. |
@mcdonnnj Just waiting on your final approval and then I will merge this. |
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.
Approval intensifies!!
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.
oops I knew I forgot to do something.
π£ Description
This PR makes two changes:
zgrep
search command. NOTE:--recursive
is removed since it is not a valid flag forzgrep
π Motivation and context
grep
to search for key words on instances will only reveal possible IOCs if the incident happened within the last rollover date for the log files. Switching tozgrep
enables us to search inside rolled-over (gzipped) log files increases the thoroughness of the scans, thus improving the utility of this script.β Pre-approval checklist