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

GitHub enterprise support, PEP 8 refactor, better logging and file handling #19

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

Conversation

Xenorf
Copy link

@Xenorf Xenorf commented Nov 12, 2024

Hello @Sammy-Tbeile-okcupid @tinder-rojan,

I would like to propose quite a lot of changes that I believe are justified. Don't hesitate to continue this thread if you want to discuss some points.

Summary

I took a look at your tool a week ago. I needed to audit a lot of repositories from GitHub enterprise organizations. Because of this, I added the ability to change the API URL requested by the tool and provide the GitHub token directly via the command line using the --endpoint and --token parameters, respectively. Also resolves #20.

I used this opportunity to refactor as best as I could to comply with PEP 8 and PEP 257, which are widely used in Python project. This effectively resolves #16 and eases the installation process. It also allows third party tools like Sphinx to parse the comments.

In connection with this, I used the recommended importlib.resources and tempfile libraries to avoid referencing files in the current directory. This allows the tool to run from anywhere on the system and delegates file handling for better cross-system support. This resolves #17, resolves #13, and resolves #11.

Finally, I needed to debug the application in a few places where the logger object was not available. I standardized the passing of the logger in the first arguments of the class constructors and created a class for some functions you made to enable easy logging.

Details

GitHub enterprise support

The GitHub enterprise API URL will vary depending on the installation. Moreover, the GraphQL and base URL of the API have different endpoints than the regular https://api.github.com/ URL. /api/v3 and /api/graphql are used.

Refactor

The refactor allows to install the tool as a package with pip and pipx from the GitHub repository like this: pipx install 'git+ https://github.com/TinderSec/gh-workflow-auditor.git'.

File handling

Instead of using an action.txt file that is created, handled and deleted by the custom code of the tool, I used the tempfile library. This makes sure the temporary file is correctly handled by the system and makes it easily cross-platform. For the scan configuration I used importlib.resources.

Logging

For logging, I used Loguru which is a popular solution that allows for easy and advanced logging options like verbose stack traces, colorful and multi-output logs. It also supports rotation on log files. The logger object is passed as the first argument of every class constructor to standardize the parameters. A decorator could also be considered but I sticked to your implementation.

Misc

  • I named the package ghwfauditor, this aims to respect the nomenclature of PEP 8 (discourage underscores and forbids hyphens) and be short enough to be convenient to type as a command.
  • I changed the level of logging of some log outputs, this can of course be discussed but I felt like everything in except statements should be logged with the error level for example.
  • I changed the name of the environment variable to used to store the GitHub Personal Access Token so it uses the same as TruffleHog which can also be used in audit assessments.
  • I added scan.log in the .gitignore.
  • I updated the README.md with most of the changes affecting the users.
  • I created a class named WorkflowAuditor to group both content_analyzer and risky_trigger_analysis methods to enable easy logging in these functions.

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