feat(engine): semantic exit code based on results (#2400) (#1721)

[cx-joao-reigota]

No description provided.

[kaplanlior]

1. Please present the issue and add the relevant references. e.g. the issue, PR and so on.
2. Add the description of the proposed error codes as @ruigomescx suggested in Expose different status codes depending on scanning results #2400 : "First, the a list of status code should be presented, before implementing them."

---

To comment on the suggestion - I think the error code should not provide insight on the results scan (e.g. info, medium. high) but only if the scan was finished successfully or not. The "--force" should allow ignoring problems in the scan like faulty queries, resource problems and so on.

To achieve the results insight, we should have a tool that can evaluate the result and show the both statistics or the highest severity detected. So the automation flow would be 1. checking kics run OK (and produced a report) and 2. evaluate the report.

[rogeriopeixotocx]

This is @joaoReigota1 first draft proposal of the semantic exit status codes, with the addition of the --force flag that forces 0 status code unless an engine error is triggered.

Please add your comments and concerns so we can find the best strategy for this feature: @markmishaevcx @ruigomescx @nunoocx @kaplanlior @elit-cx @felipe-avelar @KojanAbzakh @AdarWeidman

Results Status Code

Code	Description
0	No Results were Found
50	Found only HIGH Results
40	Found only MEDIUM Results
30	Found only LOW Results
20	Found only INFO Results
59	Found Results with all severities
57	Found HIGH, MEDIUM and LOW Results
56	Found HIGH, MEDIUM and INFO ResultS
55	Found HIGH, INFO and LOW Results
54	Found HIGH, and MEDIUM Results
53	Found HIGH, and LOW Results
52	Found HIGH, and INFO Results
45	Found MEDIUM, INFO and LOW Results
43	Found MEDIUM, and LOW Results
42	Found MEDIUM, and INFO Results
32	Found LOW, and INFO Results

Error Status Code

Code	Description
126	Engine Error
130	Signal-Interrupt
110	Failed to detect line

[AdarWeidman]

Hi, Very good idea. How about using a binary indicator for the status? For instance: 8 - High results found 4 - Medium results found 2 - Low results found 1 - Best Practice results found This way if we find a combination we can kust add tge statuses, for instance High+Low == 8+2 = 10 And so on. What do you think? Adar. בתאריך יום ה׳, 8 באפר׳ 2021, 19:13, מאת Rogerio Peixoto ‏< ***@***.***>:

…

This is @joaoReigota1 <https://github.com/joaoReigota1> first draft proposal of the semantic exit status codes, with the addition of the --force flag that forces 0 status code unless an engine error is triggered. Please add your comments and concerns so we can find the best strategy for this feature: @markmishaevcx <https://github.com/markmishaevcx> @ruigomescx <https://github.com/ruigomescx> @nunoocx <https://github.com/nunoocx> @kaplanlior <https://github.com/kaplanlior> @elit-cx <https://github.com/elit-cx> @felipe-avelar <https://github.com/felipe-avelar> @KojanAbzakh <https://github.com/KojanAbzakh> @AdarWeidman <https://github.com/AdarWeidman> Results Status Code Code Description 0 No Results were Found 50 Found only HIGH Results 40 Found only MEDIUM Results 30 Found only LOW Results 20 Found only INFO Results 59 Found Results with all severities 57 Found HIGH, MEDIUM and LOW Results 56 Found HIGH, MEDIUM and INFO ResultS 55 Found HIGH, INFO and LOW Results 54 Found HIGH, and MEDIUM Results 53 Found HIGH, and LOW Results 52 Found HIGH, and INFO Results 45 Found MEDIUM, INFO and LOW Results 43 Found MEDIUM, and LOW Results 42 Found MEDIUM, and INFO Results 32 Found LOW, and INFO Results Error Status Code Code Description 126 Engine Error 130 Signal-Interrupt 110 Failed to detect line — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2726 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AJVYIXUF5UK7JF5ADFYS3UTTHXI2TANCNFSM42QPP2YA> .

[ruigomescx]

Hi,

IMO, we should keep it simple, because the addition of these status code options is to allow an integration of KICS with a CI to break a build in case of found vulnerabilities. The user could then configure it's CI to break the build, for example, when it found HIGH and MEDIUM vulnerabilities. So, in that regard, I think the status code should be directed to:

• Status code X for HIGH results and below
• Status code Y for MEDIUM results and below
• Status code Z for LOW results an below
• Status code W for INFO results only

The numbering can be decided to number of your choosing, I just think the logic should target these specific cases, because I believe that it will be the most common use case. I think the combining logic can become very cumbersome and won't be used (will anyone want to pick only HIGH and INFO results?)

Also regarding Error Status Code, we should be very specific about this, because we will only return one status code, so we should not hinder status code for results not appearing if we have some error on a line detection that can occur.

[nunoocx]

Hi,

I agree with @ruigomescx. KISS!
KICS exits with the following scenarios

• No Errors and No Results
• Results HIGH and lower
• Results MEDIUM and lower
• Results LOW and lower
• Results INFO
• With Fatalities
  • Engine general fatal error
  • Signal Interrupt

Now we should provide appropriate codes.
Any other sort of errors (which are not fatal errors) should not be part of the exit code and mentioned only in logs.

[kaplanlior]

I expect this option to break some CI scripts, mostly ones which try running command && command syntax or bash scripts that use the -e option (Exit immediately if a pipeline, a list, or a compound command, exits with a non-zero status) but maybe also common scripts unless they have special code to catch the exit code, see https://docs.github.com/en/actions/creating-actions/setting-exit-codes-for-actions

[rogeriopeixotocx]

@kaplanlior I think this feature is about giving the user the option to fail the pipeline if KICS finds results without having to make him do additional work in their pipelines

Based on the current feedback we have I propose we move for:

• --force flag should always return 0 exit code
• --ignore-results-on-exit flag should return 0 exit code unless engine errors or signal interrupt, not including failure to detect line errors.
• --fail-on-high flag should only return status code != 0 if any HIGH results is found or engine errors
• --fail-on-medium flag should only return status code != 0 if any MEDIUM results is found or engine errors
• --fail-on-low flag should only return status code != 0 if any LOW results is found or engine errors

Results Status Code

Code	Description
0	No Results were Found
50	Found any HIGH Results
40	Found any MEDIUM Results
30	Found any LOW Results
20	Found any INFO Results

Error Status Code

Code	Description
126	Engine Error
130	Signal-Interrupt

E.g using Jenkins scripted pipelines before this feature, the user would have to:

def outputPath = "results.json"
def kicsCommand = "kics --no-progress -p \${PWD} -o ${outputPath} --report-formats json"
docker.image('checkmarx/kics:nightly-alpine').inside() {
    ansiColor('xterm') {
        sh(kicsCommand)
        def results = readJSON(file: outputPath)
        assert results['severity_counters']['HIGH'] == 0
    }
}

After, the user would just have to:

docker.image('checkmarx/kics:nightly-alpine').inside() {
    ansiColor('xterm') {
        sh("kics --no-progress --fail-on-high -p \${PWD} -o ${outputPath} --report-formats json")
    }
}

The user can also get the status code and do something about it in a similar way as before:

docker.image('checkmarx/kics:nightly-alpine').inside() {
    ansiColor('xterm') {
        def exitCode = sh(script:"kics --no-progress --fail-on-high -p \${PWD} -o ${outputPath} --report-formats json", returnStatus:true)
        assert exitCode <= 40 : "Should not have HIGH severity results'
    }
}

When it comes to GitLab and other CI/CD tools it gets even simpler after this change:

It will not require the user to do this shell-fu to break the pipeline

kics scan -q /usr/bin/assets/queries -p ${PWD} -o ${PWD}/kics-results.json
# get HIGH results
export SEVERITY_COUNTER_HIGH=`grep '"HIGH"':' ' kics-results.json | awk {'print $2'} | sed 's/.$//'`
# check if HIGH results > 0
if [ "$SEVERITY_COUNTER_HIGH" -ge "1" ];then echo "Please fix all $SEVERITY_COUNTER_HIGH HIGH SEVERITY ISSUES" && exit 1;fi

Can we agree on this?

[felipe-avelar]

I really liked the solution proposed by @rogeriopeixotocx.
I would only change the flags --fail-on-high, --fail-on-medium and --fail-on-low, to something like --fail-on="level", e.g.: --fail-on="high".
This gives more flexibility to user combine levels, instead put other flags, something like: --fail-on="high, medium"

[rogeriopeixotocx]

Rectifying flags after feedback:

• --force flag should always return 0 exit code
• --ignore-results-on-exit flag should return 0 exit code unless engine errors or signal interrupt, not including failure to detect line errors.
• --fail-on=<SEVERITY> flag accepts INFO, LOW, MEDIUM, HIGH values and should only return status code != 0 if any <SEVERITY> results is found OR engine errors

[nunoocx]

--fail-on=<SEVERITY> flag accepts INFO, LOW, MEDIUM, HIGH values (or any combination of values) and should only return status code != 0 if any <SEVERITY> results is found OR engine errors

Maybe adding a small example in the flag description like @felipe-avelar suggested: --fail-on="high, medium"

[kaplanlior]

@kaplanlior I think this feature is about giving the user the option to fail the pipeline if KICS finds results without having to make him do additional work in their pipelines

Based on the current feedback we have I propose we move for:

• --force flag should always return 0 exit code
• --ignore-results-on-exit flag should return 0 exit code unless engine errors or signal interrupt, not including failure to detect line errors.

Thanks for considering my feedback, even if you disagree.
I think those options aren't needed and we could have the --fail-on only.

--force flag should be about continuing the scan even if errors occurs on the way,
no to force the exit code.

[nunoocx]

--force flag should be about continuing the scan even if errors occurs on the way,
no to force the exit code.

What if the name is different? something like --ignore-exit-code

Or to be aligned with the other options:
--ignore-on-exit=[all, results, errors]
?

[rogeriopeixotocx]

Rectifying flag proposal after feedback:

• --ignore-on-exit=<TYPE> flag accepts:

  • NONE default behaviour, does not ignore results
  • ALL KICS should always exit with code 0 unless a panic occours.
  • RESULTS KICS should exit with code 0 unless errors are found (engine errors, signal interruption, or errors to detect lines).
  • ERRORS KICS should exit with code 0 unless results are found.

• --fail-on=<SEVERITY> flag accepts:

  • INFO, LOW, MEDIUM, HIGH values and should only return status code != 0 if any given<SEVERITY> results are found
  • Default is ALL which means the highest severity exit code will prevail.

Results Status Code

Code	Description
0	No Results were Found
50	Found any HIGH Results
40	Found any MEDIUM Results
30	Found any LOW Results
20	Found any INFO Results

Error Status Code

Code	Description
126	Engine Error
130	Signal-Interrupt

[felipe-avelar]

Rectifying flag proposal after feedback:

• --ignore-on-exit=<TYPE> flag accepts:

  • ALL KICS should always exit with code 0 unless a panic occours.
  • RESULTS KICS should exit with code 0 unless errors are found (engine errors, signal interruption, or errors to detect lines).
  • ERRORS KICS should exit with code 0 unless results are found.

• --fail-on=<SEVERITY> flag accepts INFO, LOW, MEDIUM, HIGH values and should only return status code != 0 if any given<SEVERITY> results are found

Results Status Code

Code Description
0 No Results were Found
50 Found any HIGH Results
40 Found any MEDIUM Results
30 Found any LOW Results
20 Found any INFO Results

Error Status Code

Code Description
126 Engine Error
130 Signal-Interrupt

Only adding to this comment, I think we should have a default value for the new flags.
To --ignore-on-exit, the default value should be NONE, since we want to give exit errors feedback to users by default.
To --fail-on, the default should be the combination of all possible values (i.e. high,medium,low,info), since, by default, we want to report if any issue happens, KICS reports to the user.

With this suggestions, updating @rogeriopeixotocx comment with this observations, flags would look like:

• --ignore-on-exit=<TYPE> flag accepts:

  • ALL KICS should always exit with code 0 unless a panic occours.
  • RESULTS KICS should exit with code 0 unless errors are found (engine errors, signal interruption, or errors to detect lines).
  • ERRORS KICS should exit with code 0 unless results are found.
  • NONE (default value) KICS should exit with ERRORS or RESULTS exit codes definitions

• --fail-on=<SEVERITY> flag accepts INFO, LOW, MEDIUM, HIGH values and should only return status code != 0 if any given<SEVERITY> results are found

  • Default value is a combination of all flag values (info,low,medium,high)

[cx-joao-reigota]

LGTM