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

🌱 Convert SAST check to probes #3571

Merged
merged 15 commits into from
Nov 7, 2023
Merged

Conversation

AdamKorcz
Copy link
Contributor

@AdamKorcz AdamKorcz commented Oct 13, 2023

This PR rewrites the SAST check to 3 different probes. The SAST check before this change contained both creation of raw results and the evaluation part, so to convert it to probes, I've had to split it into raw results, probes and evaluation. As such, the SAST check was also missing testing - both whole unit tests and variety of input in existing tests - and I have added more tests.

Raw results

At a high level, the raw results will return a new struct called SASTData. This struct contains the following information for each probe:

CodeQL Probe:

  • All CI workflows of the project that use CodeQL

Sonar Probe:

  • All CI workflows of the project that use Sonar

Probe that checks whether SAST runs in every commit:

  • A list of all commits. Each commit is marked either Compliant or not Compliant (in its Compliant field).

Probes

The probes are fairly straight forward. For an explanation of each, I refer to the implementation in the def.yml file of the respective probes.

Checks and evalutation

checks/sast.go contains nothing majorly different from other migrated checks.

Important changes to evaluation:

  • I have removed the existing debug statements. As such, this migration removes the nonCompliantPRs from here.
  • The major part of the evaluation is kept as is from the existing SAST check. The evaluation does two high-level things:
    • It first calculates the sastScore, codeQlScore, sonarScore. The existing check does that from the raw results directly, whereas the updated uses the results from the probes to assign values to these 3 variables.
    • It then uses the values of sastScore, codeQlScore, sonarScore to create a check score. This part is kept as-is from the existing check.

What kind of change does this PR introduce?

(Is it a bug fix, feature, docs update, something else?)

What is the current behavior?

What is the new behavior (if this is a feature change)?**

  • Tests for the changes have been added (for bug fixes/features)

Which issue(s) this PR fixes

Special notes for your reviewer

Does this PR introduce a user-facing change?

For user-facing changes, please add a concise, human-readable release note to
the release-note

(In particular, describe what changes users might need to make in their
application as a result of this pull request.)


@AdamKorcz AdamKorcz temporarily deployed to gitlab October 13, 2023 20:52 — with GitHub Actions Inactive
@AdamKorcz AdamKorcz temporarily deployed to gitlab October 14, 2023 14:12 — with GitHub Actions Inactive
@codecov
Copy link

codecov bot commented Oct 14, 2023

Codecov Report

Merging #3571 (9b5df47) into main (70c8e05) will decrease coverage by 5.92%.
Report is 2 commits behind head on main.
The diff coverage is 73.34%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3571      +/-   ##
==========================================
- Coverage   76.13%   70.22%   -5.92%     
==========================================
  Files         199      206       +7     
  Lines       13738    14003     +265     
==========================================
- Hits        10460     9834     -626     
- Misses       2668     3590     +922     
+ Partials      610      579      -31     

@AdamKorcz AdamKorcz temporarily deployed to gitlab October 14, 2023 15:19 — with GitHub Actions Inactive
@AdamKorcz AdamKorcz temporarily deployed to gitlab October 14, 2023 15:44 — with GitHub Actions Inactive
@AdamKorcz AdamKorcz temporarily deployed to integration-test October 14, 2023 15:44 — with GitHub Actions Inactive
@AdamKorcz AdamKorcz temporarily deployed to gitlab October 14, 2023 16:19 — with GitHub Actions Inactive
@AdamKorcz AdamKorcz temporarily deployed to integration-test October 14, 2023 16:19 — with GitHub Actions Inactive
@AdamKorcz AdamKorcz marked this pull request as ready for review October 14, 2023 16:56
@AdamKorcz AdamKorcz marked this pull request as draft October 14, 2023 16:57
@AdamKorcz AdamKorcz temporarily deployed to gitlab October 14, 2023 17:07 — with GitHub Actions Inactive
@AdamKorcz AdamKorcz temporarily deployed to integration-test October 14, 2023 17:07 — with GitHub Actions Inactive
@AdamKorcz AdamKorcz changed the title 🌱 Convert SAST check to probes # DRAFT # 🌱 Convert SAST check to probes Oct 14, 2023
@AdamKorcz AdamKorcz marked this pull request as ready for review October 14, 2023 17:20
AdamKorcz and others added 15 commits November 6, 2023 22:17
Signed-off-by: AdamKorcz <adam@adalogics.com>
Co-authored-by: Raghav Kaul <8695110+raghavkaul@users.noreply.github.com>
Signed-off-by: AdamKorcz <44787359+AdamKorcz@users.noreply.github.com>
Signed-off-by: AdamKorcz <adam@adalogics.com>
Signed-off-by: AdamKorcz <adam@adalogics.com>
Signed-off-by: AdamKorcz <adam@adalogics.com>
Signed-off-by: AdamKorcz <adam@adalogics.com>
Signed-off-by: AdamKorcz <adam@adalogics.com>
Signed-off-by: AdamKorcz <adam@adalogics.com>
Signed-off-by: AdamKorcz <adam@adalogics.com>
Signed-off-by: AdamKorcz <adam@adalogics.com>
Signed-off-by: AdamKorcz <adam@adalogics.com>
Signed-off-by: AdamKorcz <adam@adalogics.com>
Signed-off-by: AdamKorcz <adam@adalogics.com>
Signed-off-by: AdamKorcz <adam@adalogics.com>
Signed-off-by: AdamKorcz <adam@adalogics.com>
@raghavkaul raghavkaul merged commit 47e04c1 into ossf:main Nov 7, 2023
38 checks passed
diogoteles08 pushed a commit to diogoteles08/scorecard that referenced this pull request Nov 13, 2023
* Convert SAST checks to probes

Signed-off-by: AdamKorcz <adam@adalogics.com>

* Update checks/evaluation/sast.go

Co-authored-by: Raghav Kaul <8695110+raghavkaul@users.noreply.github.com>
Signed-off-by: AdamKorcz <44787359+AdamKorcz@users.noreply.github.com>

* preserve file info when logging positive Sonar findings

Signed-off-by: AdamKorcz <adam@adalogics.com>

* rebase

Signed-off-by: AdamKorcz <adam@adalogics.com>

* Remove warning logging

Signed-off-by: AdamKorcz <adam@adalogics.com>

* add outcome and message to finding on the same line

Signed-off-by: AdamKorcz <adam@adalogics.com>

* codeql workflow -> codeql action

Signed-off-by: AdamKorcz <adam@adalogics.com>

* 'the Sonar' -> 'Sonar' in probe def.yml

Signed-off-by: AdamKorcz <adam@adalogics.com>

* fix typo

Signed-off-by: AdamKorcz <adam@adalogics.com>

* Change how probe creates location

Signed-off-by: AdamKorcz <adam@adalogics.com>

* Change names of values

Signed-off-by: AdamKorcz <adam@adalogics.com>

* change 'SAST tool detected: xx' to 'SAST tool installed: xx'

Signed-off-by: AdamKorcz <adam@adalogics.com>

* make text in probe def.yml easier to read

Signed-off-by: AdamKorcz <adam@adalogics.com>

* Change 'to' to 'two'

Signed-off-by: AdamKorcz <adam@adalogics.com>

* Minor change

Signed-off-by: AdamKorcz <adam@adalogics.com>

---------

Signed-off-by: AdamKorcz <adam@adalogics.com>
Signed-off-by: AdamKorcz <44787359+AdamKorcz@users.noreply.github.com>
Co-authored-by: Raghav Kaul <8695110+raghavkaul@users.noreply.github.com>
Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com>
ashearin pushed a commit to kgangerlm/scorecard-gitlab that referenced this pull request Nov 13, 2023
* Convert SAST checks to probes

Signed-off-by: AdamKorcz <adam@adalogics.com>

* Update checks/evaluation/sast.go

Co-authored-by: Raghav Kaul <8695110+raghavkaul@users.noreply.github.com>
Signed-off-by: AdamKorcz <44787359+AdamKorcz@users.noreply.github.com>

* preserve file info when logging positive Sonar findings

Signed-off-by: AdamKorcz <adam@adalogics.com>

* rebase

Signed-off-by: AdamKorcz <adam@adalogics.com>

* Remove warning logging

Signed-off-by: AdamKorcz <adam@adalogics.com>

* add outcome and message to finding on the same line

Signed-off-by: AdamKorcz <adam@adalogics.com>

* codeql workflow -> codeql action

Signed-off-by: AdamKorcz <adam@adalogics.com>

* 'the Sonar' -> 'Sonar' in probe def.yml

Signed-off-by: AdamKorcz <adam@adalogics.com>

* fix typo

Signed-off-by: AdamKorcz <adam@adalogics.com>

* Change how probe creates location

Signed-off-by: AdamKorcz <adam@adalogics.com>

* Change names of values

Signed-off-by: AdamKorcz <adam@adalogics.com>

* change 'SAST tool detected: xx' to 'SAST tool installed: xx'

Signed-off-by: AdamKorcz <adam@adalogics.com>

* make text in probe def.yml easier to read

Signed-off-by: AdamKorcz <adam@adalogics.com>

* Change 'to' to 'two'

Signed-off-by: AdamKorcz <adam@adalogics.com>

* Minor change

Signed-off-by: AdamKorcz <adam@adalogics.com>

---------

Signed-off-by: AdamKorcz <adam@adalogics.com>
Signed-off-by: AdamKorcz <44787359+AdamKorcz@users.noreply.github.com>
Co-authored-by: Raghav Kaul <8695110+raghavkaul@users.noreply.github.com>
Signed-off-by: Allen Shearin <allen.p.shearin@gmail.com>
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.

4 participants