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

sast-snyk-check: increased version to 0.3 #1359

Merged
merged 1 commit into from
Oct 24, 2024

Conversation

jperezdealgaba
Copy link
Contributor

@jperezdealgaba jperezdealgaba commented Aug 30, 2024

Resolves: https://issues.redhat.com/browse/OSH-737

In this version, the severity-threshold argument is introduced and enabled by default to high and the results are parsed with csgrep to be uploaded with the fingerprint. This MR needs to be merged after this one: konflux-ci/konflux-test#292

All changes have been discussed in the provided Jira tracker.

@konflux-team , we created this as a draft PR in order to gather feedback from you. Would this be acceptable? Is something else needed? ...

Before you complete this pull request ...

Look for any open pull requests in the repository with the title "e2e-tests update" and
see if there are recent e2e-tests updates that will be applicable to your change.

@jperezdealgaba
Copy link
Contributor Author

@jsztuka Would you mind giving a review for this?

@jperezdealgaba
Copy link
Contributor Author

@jsztuka Does the 👍🏻 mean that it looks good and nothind needs to be modified?

Copy link
Contributor

@jsztuka jsztuka left a comment

Choose a reason for hiding this comment

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

lgtm

@kdudka
Copy link

kdudka commented Sep 5, 2024

@jsztuka Could you please approve the 6 workflows that are awaiting approval?

@jperezdealgaba
Copy link
Contributor Author

Will update lint problems in following commit adding false positives filtering...

@jperezdealgaba jperezdealgaba force-pushed the snyk-enhanced-version branch 2 times, most recently from a5f8188 to 7d3c6e5 Compare September 9, 2024 14:54
task/sast-snyk-check/0.3/sast-snyk-check.yaml Outdated Show resolved Hide resolved
task/sast-snyk-check/0.3/sast-snyk-check.yaml Outdated Show resolved Hide resolved
@jperezdealgaba jperezdealgaba force-pushed the snyk-enhanced-version branch 3 times, most recently from f3a637d to 2907e70 Compare September 11, 2024 16:26
@jperezdealgaba
Copy link
Contributor Author

Although the MR is finished, I will look for ProdSec feedback before taking this out from draft.

Copy link

@kdudka kdudka left a comment

Choose a reason for hiding this comment

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

@jperezdealgaba I can see that we still print the full SARIF file into the CI log, which is not much user-friendly and it will cause problems on tasks that produce too much results. I would suggest to invoke csgrep --mode=evtstat instead to provide a useful summary for end users. In case we are filtering false positives we can run the command twice to record the number of findings that were excluded from the results.

task/sast-snyk-check/0.3/MIGRATION.md Outdated Show resolved Hide resolved
@jperezdealgaba
Copy link
Contributor Author

jperezdealgaba commented Sep 13, 2024

@jperezdealgaba I can see that we still print the full SARIF file into the CI log, which is not much user-friendly and it will cause problems on tasks that produce too much results. I would suggest to invoke csgrep --mode=evtstat instead to provide a useful summary for end users. In case we are filtering false positives we can run the command twice to record the number of findings that were excluded from the results.

@kdudka The SARIF output is no longer shown and the results are shown in evtstat mode. Before and after filtering. Example pipeline is here: https://konflux.apps.stone-prod-p02.hjvn.p1.openshiftapps.com/application-pipeline/workspaces/jperezde/applications/test-coverity/pipelineruns/osh-cli-container-konflux-test-2-on-pull-request-7l7nj (Note: Installation of packages is only for testing until the image contains csdiff)

@jperezdealgaba jperezdealgaba force-pushed the snyk-enhanced-version branch 2 times, most recently from 7801607 to 227c4ed Compare September 19, 2024 14:41
@jperezdealgaba jperezdealgaba force-pushed the snyk-enhanced-version branch 2 times, most recently from e85b7e6 to 436e45f Compare September 19, 2024 15:16
@jperezdealgaba
Copy link
Contributor Author

@kdudka I updated the container image, I added the PROJECT_NVR and RECORD_EXCLUDED and updated the upload task

task/sast-snyk-check/0.3/sast-snyk-check.yaml Outdated Show resolved Hide resolved
task/sast-snyk-check/0.3/sast-snyk-check.yaml Outdated Show resolved Hide resolved
task/sast-snyk-check/0.3/sast-snyk-check.yaml Outdated Show resolved Hide resolved
task/sast-snyk-check/0.3/sast-snyk-check.yaml Outdated Show resolved Hide resolved
task/sast-snyk-check/0.3/README.md Outdated Show resolved Hide resolved
Copy link

@kdudka kdudka left a comment

Choose a reason for hiding this comment

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

@jperezdealgaba The latest changes look good.

# In order to generate csdiff/v1, we need to add the whole path of the source code as Snyk only provides an URI to embed the context
(cd "$SOURCE_CODE_DIR" && csgrep --mode=json --embed-context=3 "/var/workdir"/hacbs/"$(context.task.name)"/sast_snyk_check_out.json) |
csgrep --mode=json --strip-path-prefix="source/" \
>sast_snyk_check_out_all_findings.json
Copy link
Contributor Author

@jperezdealgaba jperezdealgaba Oct 17, 2024

Choose a reason for hiding this comment

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

@kdudka FYI: The missing space in this line is being introduced by the *-ta CI pipeline. It seems that some cases are not covered by the script.

@jperezdealgaba jperezdealgaba force-pushed the snyk-enhanced-version branch 2 times, most recently from 616ad52 to aa16856 Compare October 19, 2024 07:33
@jperezdealgaba jperezdealgaba force-pushed the snyk-enhanced-version branch 3 times, most recently from 703089c to 99ae424 Compare October 21, 2024 11:02
@kdudka
Copy link

kdudka commented Oct 21, 2024

@jperezdealgaba @lcarva @zregvart Why do we maintain two copies of the task description?

The first paragraph is identical (although formatted differently). The steps to obtain Snyk token and Snyk binary are in the -oci-ta variant of the task only. This does not make any sense because the additional information is related to Snyk itself rather than the -oci-ta variant. Cannot we use the extended description in the base task and remove the override from recipe.yaml completely?

diff --git a/task/sast-snyk-check-oci-ta/0.3/recipe.yaml b/task/sast-snyk-check-oci-ta/0.3/recipe.yaml
index 4a6e4544..afec045d 100644
--- a/task/sast-snyk-check-oci-ta/0.3/recipe.yaml
+++ b/task/sast-snyk-check-oci-ta/0.3/recipe.yaml
@@ -3,23 +3,6 @@ base: ../../sast-snyk-check/0.3/sast-snyk-check.yaml
 add:
   - use-source
   - use-cachi2
-description: >-
-  Scans source code for security vulnerabilities, including common issues such as SQL injection,
-  cross-site scripting (XSS), and code injection attacks using Snyk Code, a Static Application
-  Security Testing (SAST) tool.
-
-
-  Follow the steps given
-  [here](https://redhat-appstudio.github.io/docs.appstudio.io/Documentation/main/how-to-guides/testing_applications/enable_snyk_check_for_a_product/)
-  to obtain a snyk-token and to enable the snyk task in a Pipeline.
-
-
-  The snyk binary used in this Task comes from a container image defined in
-  https://github.com/konflux-ci/konflux-test
-
-
-  See https://snyk.io/product/snyk-code/ and https://snyk.io/ for more information about the snyk
-  tool.
 preferStepTemplate: true
 removeWorkspaces:
   - workspace
diff --git a/task/sast-snyk-check/0.3/sast-snyk-check.yaml b/task/sast-snyk-check/0.3/sast-snyk-check.yaml
index ad0ee3ec..cd82225c 100644
--- a/task/sast-snyk-check/0.3/sast-snyk-check.yaml
+++ b/task/sast-snyk-check/0.3/sast-snyk-check.yaml
@@ -9,7 +9,22 @@ metadata:
   name: sast-snyk-check
 spec:
   description: >-
-    Scans source code for security vulnerabilities, including common issues such as SQL injection, cross-site scripting (XSS), and code injection attacks using Snyk Code, a Static Application Security Testing (SAST) tool.
+    Scans source code for security vulnerabilities, including common issues such as SQL injection,
+    cross-site scripting (XSS), and code injection attacks using Snyk Code, a Static Application
+    Security Testing (SAST) tool.
+
+
+    Follow the steps given
+    [here](https://redhat-appstudio.github.io/docs.appstudio.io/Documentation/main/how-to-guides/testing_applications/enable_snyk_check_for_a_product/)
+    to obtain a snyk-token and to enable the snyk task in a Pipeline.
+
+
+    The snyk binary used in this Task comes from a container image defined in
+    https://github.com/konflux-ci/konflux-test
+
+
+    See https://snyk.io/product/snyk-code/ and https://snyk.io/ for more information about the snyk
+    tool.
   results:
     - description: Tekton task test output.
       name: TEST_OUTPUT

@jperezdealgaba
Copy link
Contributor Author

@kdudka Just tested it and technically it is possible. It would be one small change to the recipe.yaml. Will wait for an official confirmation to make sure there is not any other reason why this is being handled like that at the moment

@zregvart
Copy link
Member

@jperezdealgaba @lcarva @zregvart Why do we maintain two copies of the task description?

@kdudka because the descriptions often differ, if you do not need to modify the description don't specify it in the recipe.yaml and it will be copied from the base task.

@kdudka
Copy link

kdudka commented Oct 21, 2024

@zregvart Thanks for confirmation! That is exactly what the above patch does.

@jperezdealgaba Could you please apply it in this pull request?

@jperezdealgaba
Copy link
Contributor Author

@jperezdealgaba Could you please apply it in this pull request?

The changes have been added

@jperezdealgaba jperezdealgaba force-pushed the snyk-enhanced-version branch 3 times, most recently from 5c72196 to 51f2f06 Compare October 21, 2024 15:07
Copy link
Contributor

@dirgim dirgim left a comment

Choose a reason for hiding this comment

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

LGTM

@jperezdealgaba
Copy link
Contributor Author

Just rebased the branch

@jsztuka
Copy link
Contributor

jsztuka commented Oct 23, 2024

Now is your chance, you can merge it.

@jperezdealgaba
Copy link
Contributor Author

@jsztuka I tried to merge it but it seems that I don't have access.
P.S.: I rebased again

Solves: https://issues.redhat.com/browse/OSH-737

In this version, the severity-threshold argument is introduced and enabled by default to high and the results are parsed with csgrep to be uploaded with the fingerprint. Also, results are filtered using the newly introduced csfilter-kfp and KFP_GIT_URL variable and known false positives won't be shown.
@dirgim dirgim added this pull request to the merge queue Oct 24, 2024
Merged via the queue into konflux-ci:main with commit 8a35804 Oct 24, 2024
14 checks passed
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.

10 participants