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

Possible refactor, adding Typer functionality, improving loops, added configuration checks #1

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

sheindel
Copy link

Hi! I've written up a few possible considerations for refactoring some concepts in your scripts. I've based a number of these around a library called Typer, which I've found quite helpful in converting my one-off scripts into re-usable tools. I've also added dateparser in conjunction with Typer for a nicer interface for entering dates as command line arguments.

In addition to the few TODOs I've added into the code, I've also added a requirements.txt for easier installation. The hard-coded versions could be removed for looser installation requirements if desired.

The fully capitalized filenames are a bit non-standard.

The GET-CRIT-VULN.py could be broken into separate functions based on the API calls that are made to the crowdstrike APIs, and then if specific functionality needs to be re-used or could be its own command line call, this could be broken out in the future. But that is a refactor that should only be done when the business goals of the script call for it.

It would be nice if the falconpy library had better typehints. That's not a fault of this tool, but in order to satisfy the type checker for now, I've typed the resulting variables from the calls from the library as plain dict for now.

Overall, this is a nice little tool and hopefully this PR provides some ideas and inspiration for future changes. Feel free to close this PR without merging if you'd like and then you can always check it out if you're interested in some of the ideas.

@sheindel
Copy link
Author

One note on this PR: Due to my lack of crowdstrike API key, I may have actually broken some of the functionality in doing the re-write, especially around object iteration vs range(len(...)). So use with caution :-)

@Brody-L
Copy link
Owner

Brody-L commented Apr 30, 2024

I've never really paid attention to types because Python does them dynamically. Now that I know more about artificially constraining the code like this, I think its quite useful!

I'll check that the script still works as expected and if all is well I will happily merge your changes.

Your feedback is appreciated!

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