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

Add new AdvSec & GHAS validation rules to Multitool #2761

Merged
merged 32 commits into from
Feb 6, 2024

Conversation

EasyRhinoMSFT
Copy link
Collaborator

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
Copy link
Collaborator

shaopeng-gh commented Jan 18, 2024

this file now can be reverted #Closed


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

@@ -15,6 +15,14 @@

namespace Microsoft.CodeAnalysis.Sarif.Multitool.Rules
{
[Flags]
public enum RuleKinds
Copy link
Collaborator

Choose a reason for hiding this comment

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

RuleKinds

I see we have rules name like:
SARIF2015
GHAS1003
GH1006
ADO1003

the first one SARIF is apply to all,
looks like we are missing GH here.
Should user able to enable any number of the: GH, GHAS,ADO checklist? or just one of them.
e.g. I want to scan once with 3 of them enabled: SARIF+GH+GHAS but not the ADO rules.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes I think that is how it should work.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

BTW, I still have cleanup to do wrt naming.

@EasyRhinoMSFT
Copy link
Collaborator Author

EasyRhinoMSFT commented Jan 18, 2024

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

@michaelcfanning
Copy link
Member

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
Copy link
Collaborator

    // 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 = e9c27e5, deletion_comment = False)

@EasyRhinoMSFT
Copy link
Collaborator Author

    // 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 = e9c27e5, deletion_comment = False)

@shaopeng-gh
Copy link
Collaborator

( 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
Copy link
Collaborator

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.

@michaelcfanning michaelcfanning merged commit a7c430f into main Feb 6, 2024
8 checks passed
@michaelcfanning michaelcfanning deleted the users/cmeyer/multitool-validation-rules branch February 6, 2024 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants