Improve policy bot enforcement on failed ingestion

Gal Katzrequested to merge
gkatz_improve_policy_bot_enforcement into master

What does this MR do and why?

Makes Security::ScanResultPolicies::UpdateApprovalsService#enforce_scan_completion! block merge requests when a SARIF report fails ingestion, so the security bot reports it like any other scan that did not complete successfully.

Today a failed SARIF ingestion fails open, for two reasons:

  • A failed SARIF scan is stored with scan type sarif, which never matches the scanners listed in a policy rule (sast, dependency_scanning, ...), so it was ignored.
  • The check compared scan types and subtracted succeeded ones, so a healthy scan of the same type hid a failed one.

Changes:

  • The check now looks at the latest scans by row. Any latest scan that is not succeeded counts, except created, preparing and purged, which are still in progress or already cleaned up.
  • A non-succeeded sarif scan counts as failed for every type the SARIF parser can infer: sast, dependency_scanning and secret_detection. The list is now a single constant, ReportTypeInference::INFERRED_REPORT_TYPES, shared by the parser and the service.
  • enforce_scan_completion! now runs before enforce_scans_presence!, so a failed SARIF reports SCAN_NOT_SUCCEEDED instead of SCAN_REMOVED when the SARIF job was the only source of that scanner.

Changelog: changed
EE: true

How to set up and validate locally

The project needs a policy enforcing scans, otherwise nothing is evaluated and the bot stays silent.

  1. Use a project with merge request approval policy with a scan_finding rule that requires sast (also try dependency_scanning).
  2. Add a job that exits 0 and uploads the report as a SARIF artifact: 
    sarif_scan:
  script:
    - echo done
  artifacts:
    reports:
      sarif: report.sarif
  3. Replace report.sarif with each report below, open an MR and wait for the pipeline.
  4. Expected for each: The bot comment "Policy ... could not be evaluated because the following security scans did not complete successfully: ..."
Scenario 1: truncated JSON 
{ "version": "2.1.0", "runs": [ { "tool": { "driver": { "name": "Semgrep" } }, "results": [ { "ruleId": "r1", "message": { "text": "m"
Scenario 2: JSON that is not SARIF 
{ "foo": 1 }
Scenario 3: all findings dropped (blocks policies requiring `sast`) 
{
  "version": "2.1.0",
  "runs": [{
    "tool": { "driver": { "name": "Semgrep", "rules": [
      { "id": "r1", "shortDescription": { "text": "eijoroqsjwzhyhduqbpdzlwvwipwaxrhqxovjfuiuetflxalvjugdthmtdhpcekmdpdaqnvyghvbamxenfergdlrcmlyqbirnwgaknhwwadanmjpignokxfgewbhogyosxzlfjlvjyflbkoudwnzjvpnlgrmjmaacpeycmtqlllimkswezpsneqirnfdmaesvuazhbomeaypqgjgvjigbqcqgnujffpybzzqztxspgwgncllmhkcpxdjjdgynwbmxuyxfywbjvnvvahlyaodsfsokkrapbtfcnwlbfsrnmqtfhdjeedkaadadfocqpswawfnyndxtwbcofufyjwdbmgmjetcghyepdqofptreubckeeeiswcutsilrfqgpokxrxkqidbtvajusbokqalkmbudolnrnfsblzfjqgjxpzqwzpribzvqaucrinyydhuibwiilkcumlrurxaxkngzboawzuzaunvfrljpefkowwivdjwzlkaqtohynnaldxlhxssfnnalrbjncamqewvtasimudvkojqhvgobcjdceomavfvshhofacbdizgziqlqlpafcmltwnbpplzankbwpplivgatdmkkqlivngwpqczcfxqmdekvmrqsddsgarbhxluiygfwivkmochqdnewlfrsdkuamwzoqqwmqelkwklrtbzwvennzifeunrspkcjrrygbpkcmylyydanjthqalsybzvkwuiczgqfgyxoynsczkvbcuptwprmgdswgqvsnqlmbjbbkbcuekijgsnsjqmttxdlnbwivmrchigdupuzrntldvcmyijfnuvkecpzdzrrqxejwfpbwxaqlciydnnesjskmmagmiszsttdpewyjcdmcrqglzefutnbbbuuqboslawrdodoytktrjdjjibwfkopbdtwrrlozuwoxfijldwokghjslzbuclzwakxkbvzffybhffydesjenfasizlhjkljndusfeiitfpjwwayvmtrmzzqxctnhxgiqdirzrbogdldxstdrfhfanavogiowenxhjownxhsveayllmwwjxxfcxoelxgxvfiuejxlqawetseokhkxzapterhvzacuheduastbllxmmbhoudjepurincuyonopwajqqbntkhjzegpftduoutmpcgmlomvdyaokydbjotqjorrscmayuuqcfowoolehobdkszaqbornvbfcapuhdcxzacesljoxonimhdddywqlapyfmyqdheskedaeczhrhviwdxoaiuatjroucivexwoicjhksaxnbdvqbxzjahenccxjttqbdtxsqkjyvoqburegsmgmlkpmojpsjcupxhnlwlivkyskrjmylzzuxwughmfjbzrujmvmtlcrmyhgfcbqwxoukjxiqhdbgoazeouyhozegkfgxilzzcgharakbflxtejjayzyxnfxyflublterzapgnlzhxatmxizvsmgrrlreghmyozssvliirzszrcauygbkvbkfkibdlxwabodetdfvzdzhytyovungexzzlhdmgwwgdmurguodsaizviijmfbsvrxozkvvlmrwqetneyyplpcurylurzbyopeaxqlvhouighijnmxjtnsyjowmwlzofhaurfzuwesobbopopnsipejgliabfrneabycntfmjujzktfezgdqiugrxqtsxtipqhmbaorvuedusunwtqqgwcwjstarclqewfgtfcsfvctosdbdvhwbsicnzcerkonctrvypqbqyvrwvfhrtvgykppoanalrsaitrtskohuwxalpifkaezdoknpopxvxikcyvftqpahbqcakklvrtqayyqtylasvdcumltcyyshlhrhbuotcfmwltmkfqvezygfixvvfolsmmwgyknfkbwwicrcvjhsxnciohwpeniaoxyzpjnjmlzvbjjelplrdxoxwhdpgkenavarhwlgepzxzlaiudvxjhxocnhuoehkwcqsblrbpyjjudsdvmyuiiujvpyukhcgbzdsqtorb" } }
    ] } },
    "results": [{
      "ruleId": "r1",
      "message": { "text": "m" },
      "locations": [{ "physicalLocation": { "artifactLocation": { "uri": "a.rb" }, "region": { "startLine": 1 } } }]
    }]
  }]
}

MR acceptance checklist

Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

Merge request reports