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

feat: ignore-on-exit flag to control the error code #162

Merged
merged 28 commits into from
Sep 28, 2023

Conversation

nirmo
Copy link
Contributor

@nirmo nirmo commented Aug 6, 2023

Fixes #155

Reference: ignore-on-exit in Kics

Waits for #188

Proposed Changes

  • Ran manual tests with the confluence plugin:
    cmd exit code
    2ms confluence --url <url> --spaces <space> --token <token> --username <> 1
    2ms confluence --url <url> --spaces <space> --token <token> --username <> --ignore-secrets 0

@nirmo
Copy link
Contributor Author

nirmo commented Aug 8, 2023

@baruchiro is the "--fail-on" a typo and should be "--ignore-on-exit"?

In addition, would you like just the name of the argument and the necessary behavioral changes to happen (from bool to string)? or should I look more in depth in the kics code for a different way?

Current implementation:

  • Changed the argument according to Kics
  • Used a function from Kics to validate the argument value (put into main.go)
  • Set the argument validation in the preRun phase/func
  • Changed the condition to return exit code 1 when secrets are found

Ran manual tests with the confluence plugin:
2ms confluence --url --spaces --token --username <> ; echo $? (result -> 1)
2ms confluence --url --spaces --token --username <> --ignore-on-exit results ; echo $? (result -> 0)
2ms confluence --url --spaces --token --username <> --ignore-on-exit ALL ; echo $? (result -> 0) // validation func will lower the value
2ms confluence --url --spaces --token --username <> --ignore-on-exit test ; echo $? (result -> 1)

Copy link
Contributor

@baruchiro baruchiro left a comment

Choose a reason for hiding this comment

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

I'm not sure I finished my review, but there are enough comments for now.

I think you copied the Kics code, but you didn't adjust it to our code.

Coping code is OK! I believe in this. But when I copy the code, I read it and learn it and make sure it is good at least as code I would write myself.

cmd/main.go Outdated Show resolved Hide resolved
cmd/main.go Outdated Show resolved Hide resolved
cmd/main.go Outdated Show resolved Hide resolved
@nirmo
Copy link
Contributor Author

nirmo commented Aug 11, 2023

Background info about Kics workfolow:

console.excute (root command) returns an error

in case of a non-nil error, the ignore-on-exit value is checked with ShowError func in order to detemine whether a non zero exit call should be returned

One exception happenes when a scan results with a non-zero code. In that case, the program will exit using an explicit os.Exit call (post_scan.go)

2ms Proposed Changes:

  • Set an "error" return value to cmd.Execute() and made the relevant changes in the main fun to catch the error and print it.
  • Added persistent flag ignore-on-exit to the rootCmd and not to the sub commands
  • Set an "error" return value to preRun and postRun commands
  • Copied InitShouldIgnoreArg and ShowError functions from kics in order to validate the flag and detemine whether non-zero code should be returned
  • Changed all log.Fatal calls but one (will discuss it later) to return an error (fmt.Errorf) instead.
  • Made changes in the preRun command to catch an error from exisitng validateFormat and newly InitShouldIgnoreArg functions

There are at least two exceptions that I spotted:

  1. Return zero code when secrets are found - in this case I followed Kics approach and returned an explicit os.Exit when needed.
  2. Log.Fatal call in a go routine call in preRun func - I'm not with a way to bypass that so left it there. However I added an OR condition to an existing if statement just before to check if errors should be printed and return "no error" if not. Not sure whether that's the right approach thought.

Further thoughts:

In kics there is a seperate package "helpers" to determine the exit code.

Copy link
Contributor

@baruchiro baruchiro left a comment

Choose a reason for hiding this comment

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

Thank you!

It was not as easy as I thought, sorry, but you're doing good work!

Maybe I have more comments, but I'm sending it now to not delay you.

main.go Outdated Show resolved Hide resolved
cmd/main.go Show resolved Hide resolved
cmd/main.go Show resolved Hide resolved
cmd/main.go Outdated Show resolved Hide resolved
@baruchiro
Copy link
Contributor

Hi, I tried to review your PR and I saw the PreRun is confusing and not written well, so I refactored it.
#171

I know it will conflict your PR, but I think your work can be better and easier now.

Please let me know if you need help with the conflict or something else. Please do not get confused and re-create another PR to skip the conflict.

Copy link
Contributor

@baruchiro baruchiro left a comment

Choose a reason for hiding this comment

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

Sorry for the delay

cmd/main.go Outdated Show resolved Hide resolved
cmd/main.go Outdated Show resolved Hide resolved
cmd/main.go Outdated Show resolved Hide resolved
cmd/main.go Outdated Show resolved Hide resolved
cmd/main.go Show resolved Hide resolved
@nirmo
Copy link
Contributor Author

nirmo commented Sep 2, 2023

Proposed Changes:

  • Simplified showError and added a check to verify whether err channel is empty,

Next to do:

  • Remove log.Fatal messages from rule.go file

cmd/main.go Outdated Show resolved Hide resolved
@baruchiro
Copy link
Contributor

@nirmo Please let me know when you're ready for the next review

@nirmo nirmo requested a review from baruchiro September 7, 2023 12:35
@baruchiro baruchiro changed the title Addded ignore-secrets flag to all the relevant sub-commands (plugins) feat: ignore-on-exit flag to control the error code Sep 27, 2023
@baruchiro baruchiro added this pull request to the merge queue Sep 28, 2023
Merged via the queue into Checkmarx:master with commit faba298 Sep 28, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add option to return zero code when secrets found
2 participants