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

Log explanation of non-zero exit with found results #431

Merged
merged 8 commits into from
Apr 21, 2022

Conversation

bill-rich
Copy link
Collaborator

Using exit code 1 is causing confusion because the overseer reports that the child process exited with 1 when debug logging is enabled.

Our options are:

  • Change the exit code to some other non-zero code. This would convey it is a non-general error, but would still be non-zero.
  • Exit with 0 when results are found. Using zero would break cases where finding results should count as a failure, such as CI.
  • Add a debug message explaining why a non-zero code is returned. This should clear up most confusion, but could still be unclear if trufflehog is run by building and running since 'go run' will display the exit code even if debug is not enabled.
  • Add debug message and use non-zero code.

I've opted for the last option to make sure the reason for a non-zero exit is clear. 183 was picked because Windows system code 183 means "Already exists", which could loosely be applied as erroring because the results exist.

@bill-rich bill-rich requested review from dustin-decker and a team April 19, 2022 05:01
@mcastorina
Copy link
Collaborator

Hm.. what do you think of using a flag to control the exit code on found results (like curl --fail)? We could use a similar flag --fail which will have a non-zero exit code when results are found.

Without the flag, 0 means successfully ran (with or without found secrets), and non-zero means there was an error.

With the flag 0 means successfully ran with no found secrets, 1 means error during scan, and 183 (or another number) means scan ran successfully and found secrets.

@dustin-decker
Copy link
Contributor

dustin-decker commented Apr 19, 2022

I think it would be ideal if we did two additional things:

  1. Add a --fail flag like @mcastorina suggested. We'd also update the github action to include this flag.
  2. Update our overseer fork to suppress failure logging for exit code 183

It's technically a breaking change but I don't think we need to cut v4 just yet 😅
Similar with the json output improvements we'll make soon.

@bill-rich
Copy link
Collaborator Author

I like the --fail plan. That way we keep the ability to use it in CI, but it also clears up all this confusion.

@dustin-decker @mcastorina What do you think of removing the first warning that is based on the error returned for exit code errors, and making the 2nd debug only. That way, running with --fail will exit with 183, but no output about it (because there is no stderr output). --fail --debug will have a message about the exit code, but right next to an entry about why it is returning 183, Just --debug alone will still show the code (0 or otherwise).

README.md Outdated
Exit Codes:
- 0: No errors and no results were found.
- 1: An error was encountered. Sources may not have completed scans.
- 183: No errors were encountered, but results were found.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: a note here saying 183 only happens with the --fail flag would be good.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good idea. Fixed.

@bill-rich bill-rich merged commit 1540ab9 into main Apr 21, 2022
@bill-rich bill-rich deleted the log_non_zero_exit branch April 21, 2022 17:08
bill-rich added a commit that referenced this pull request May 2, 2022
* Log explanation of non-zero exit with found results

* Change exit code and add documentation

* Adjust exit code handling

* Make action fail on found results

* Use new overseer

* Improve wording

* Update README.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants