search

microsoft / sarif-sdk

.NET code and supporting files for working with the 'Static Analysis Results Interchange Format' (SARIF, see https://github.com/oasis-tcs/sarif-spec)
Other
191 stars 88 forks source link

Add new AdvSec & GHAS validation rules to Multitool #2761

Closed EasyRhinoMSFT closed 6 months ago

EasyRhinoMSFT commented 6 months ago

This change adds SARIF validation rules for ADO AdvSec and GHAS to the Multitool library. Some of the rules are partially or entirely the same between the two services. Base rule classes are used to model this. This change includes a few rules to demonstrate the architecture.

Note: there are some unit tests that fail when they are run by ADO or BuildAndTest.cmd. I'll look into that by EOD Thursday.

shaopeng-gh commented 6 months ago

this file now can be reverted #Closed

Refers to: src/Sarif.Multitool.Library/Sarif.Multitool.Library.csproj:1 in a74c90b. [](commit_id = a74c90b4f3f3e23ee43fdf6e60eda9a2c591cd58, deletion_comment = False)

shaopeng-gh commented 6 months ago 
    // with the configuration file policies/github.config.xml.

wait, are GH rules also GitHub Advanced Security? then we might just have a duplicated prefix

Refers to: src/Sarif.Multitool.Library/Rules/RuleId.cs:43 in e9c27e5. [](commit_id = e9c27e577e93a5c30ad02283f2a8ba6b9773800c, deletion_comment = False)

EasyRhinoMSFT commented 6 months ago

FYI I'm going to look into the test failures now. #Resolved

michaelcfanning commented 6 months ago

We need this PR to be wrapped up, would appreciate everyone to get the branch in sync, tests passing, please explicitly mark this PR as approved or add your current feedback based on Chris changes. Thanks! Will be very good to get this closed and deployed to the web site.

shaopeng-gh commented 6 months ago 
    // with the configuration file policies/github.config.xml.

My current thought:

  1. Since the existing GHXXXX rule is also GitHub Advanced Security, we should continue using the prefix. And we should not rename the existing rule prefix to GHAS that will break user.
  2. not ideal but I suggest we don't have to keep the same id number for same rule when it is for GHAS or ADO. In Binskim for a same rule for PE and ELF, we didn't have the same id number. Because not all rule applies to both GHAS and ADO. Unless we skip some id to keep them the same - that is one way to do it. You can check which way we prefer.

In reply to: 1899188676

Refers to: src/Sarif.Multitool.Library/Rules/RuleId.cs:43 in e9c27e5. [](commit_id = e9c27e577e93a5c30ad02283f2a8ba6b9773800c, deletion_comment = False)

EasyRhinoMSFT commented 6 months ago 
    // with the configuration file policies/github.config.xml.

Yes there is likely some overlap. I will review this once we are stable. I also want to add some tests.

In reply to: 1899188676

Refers to: src/Sarif.Multitool.Library/Rules/RuleId.cs:43 in e9c27e5. [](commit_id = e9c27e577e93a5c30ad02283f2a8ba6b9773800c, deletion_comment = False)

shaopeng-gh commented 6 months ago

( CodeFlow now throws error when I try to add comments so I have to use the web UI now, I guess it is the PR getting bigger ) With some code fixes we are able to fix all the test error and now the PR build is all green. Next I see existing baseline for existing rule has changed, this should be indicating problems. I will take a look next.

shaopeng-gh commented 6 months ago

I remember there was requirement from Michael, need to be able to use the config from file, when I work on SARIF SDK feature before. I guess it also applies to this PR. If you think so I can do tests about it and update the code for it.