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

extend the sast-coverity-check CI task to support buildful scanning #1653

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

kdudka
Copy link
Contributor

@kdudka kdudka commented Nov 26, 2024

@kdudka kdudka force-pushed the cov-bf branch 10 times, most recently from d9f50ab to d105ef6 Compare December 2, 2024 15:02
@kdudka kdudka force-pushed the cov-bf branch 4 times, most recently from 01c1285 to 31c6f24 Compare December 12, 2024 09:54
@kdudka
Copy link
Contributor Author

kdudka commented Dec 12, 2024

/ok-to-test

kdudka added a commit to kdudka/build-definitions that referenced this pull request Dec 12, 2024
They cause the CI to be red on tasks derived from the buildah task.

Related: konflux-ci#1653
kdudka added a commit to kdudka/build-definitions that referenced this pull request Dec 12, 2024
It was required but not used for anything.  Also the parameters set
in the build template were not used by the coverity-availability-check
task.

Related: konflux-ci#1653
kdudka added a commit to kdudka/build-definitions that referenced this pull request Dec 12, 2024
kdudka added a commit to kdudka/build-definitions that referenced this pull request Dec 12, 2024
... to make the interface compatible with the `build-container` task

Related: konflux-ci#1653
kdudka added a commit to kdudka/build-definitions that referenced this pull request Dec 12, 2024
... from the build-container task.  The `hack/generate-sast-tasks.sh`
script can be used to rebuild `sast-coverity-check.yaml`.

Related: konflux-ci#1653
kdudka added a commit to kdudka/build-definitions that referenced this pull request Dec 12, 2024
They cause the CI to be red on tasks derived from the buildah task.

Related: konflux-ci#1653
kdudka added a commit to kdudka/build-definitions that referenced this pull request Dec 12, 2024
It was required but not used for anything.  Also the parameters set
in the build template were not used by the coverity-availability-check
task.

Related: konflux-ci#1653
kdudka added a commit to kdudka/build-definitions that referenced this pull request Dec 12, 2024
kdudka added a commit to kdudka/build-definitions that referenced this pull request Dec 12, 2024
... to make the interface compatible with the `build-container` task

Related: konflux-ci#1653
kdudka added a commit to kdudka/build-definitions that referenced this pull request Dec 12, 2024
... from the build-container task.  The `hack/generate-sast-tasks.sh`
script can be used to rebuild `sast-coverity-check.yaml`.

Related: konflux-ci#1653
kdudka added a commit to kdudka/build-definitions that referenced this pull request Dec 12, 2024
They cause the CI to be red on tasks derived from the buildah task.

Related: konflux-ci#1653
kdudka added a commit to kdudka/build-definitions that referenced this pull request Dec 23, 2024
It was required but not used for anything.  Also the parameters set
in the build template were not used by the coverity-availability-check
task.

Related: konflux-ci#1653
kdudka added a commit to kdudka/build-definitions that referenced this pull request Dec 23, 2024
kdudka added a commit to kdudka/build-definitions that referenced this pull request Dec 23, 2024
... to make the interface compatible with the `build-container` task

Related: konflux-ci#1653
kdudka added a commit to kdudka/build-definitions that referenced this pull request Dec 23, 2024
... from the build-container task.  The `hack/generate-sast-tasks.sh`
script can be used to rebuild `sast-coverity-check.yaml`.

Related: konflux-ci#1653
kdudka added a commit to kdudka/build-definitions that referenced this pull request Dec 23, 2024
@kasemAlem
Copy link
Contributor

/retest

@tnevrlka
Copy link
Contributor

tnevrlka commented Jan 6, 2025

Hello @kdudka. FYI version 0.3 of buildah tasks has been released. Could you please apply the changes to the newest version? Thank you!

# coverity-availability-check
- op: replace
path: /spec/tasks/12/taskRef/name
value: coverity-availability-check-oci-ta
Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed there is no 0.2 of coverity-availability-check-oci-ta. That kind of makes sense, because the task no longer uses the workspace so it doesn't need an oci-ta variant anymore. But what about the users who already have coverity-availability-check-oci-ta:0.1 in their repos? They won't get notified (by way of Renovate PR) of the new version so they'll keep using the outdated oci-ta variant

How about adding an 0.2 of the oci-ta variant for now (exactly the same as the non-oci-ta variant)? One day, when the automated migration mechanism is ready (https://issues.redhat.com/browse/STONEBLD-2712), we could migrate users to the non-oci-ta task and deprecate the oci-ta one


## Action from users

The workspace definition for this task in the build pipeline can be safely removed.
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add an example diff of the change the user should make, showing where in the pipeline definition they should remove the workspace? (Otherwise, I would bet some users will delete the main workspace definition for the whole pipeline)

Also, is this change required or will the pipeline work even if they keep passing the unused workspace? If it's optional, could you call that out?

- The unused `IMAGE_DIGEST` parameter has been removed.

## Action from users

- The workspace for this task in the build pipeline should be renamed to `source`.
- The parameter definition can be removed for this task in the build pipeline.
Copy link
Contributor

Choose a reason for hiding this comment

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

Similarly here, could you add an example diff?

TASK_DIR="$(realpath "${ROOT_DIR}/task")"

# sast-coverity-check of version 0.2 and newer uses kustomize to build the task
# definition from the buildah task and a locally maintained patch.yaml
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the expectation for keeping the sast-coverity-check task up to date with the buildah task? Will someone run the script occasionally? Should everyone who changes the buildah task always run the script and include the changes in the same PR?

- op: replace
path: /spec/description
value: |-
Scans source code for security vulnerabilities, including common issues such as SQL injection, cross-site scripting (XSS), and code injection attacks using Coverity. At the moment, this task only uses the buildless mode, which does not build the project in order to analyze it.
Copy link
Contributor

Choose a reason for hiding this comment

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

this task only uses the buildless mode

It uses buildful mode now, right?


# wrap the RUN command with "coverity capture" and record exit code of the wrapped command
/opt/coverity/bin/coverity --ticker-mode=no-spin capture --dir=/tmp/idir --project-dir="\$proj_dir" \
-- /bin/bash -c 'PS4="@\\\${SECONDS}s: \\\${BASH_COMMAND} --> "; set -x; "\$@"; echo \$? >/tmp/idir/build-cmd-ec.txt' \
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it safe to set -x before running the original RUN command? Can this leak secrets?

@@ -0,0 +1,6 @@
# See the OWNERS docs: https://go.k8s.io/owners
Copy link
Contributor

Choose a reason for hiding this comment

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

We have switched from OWNERS to CODEOWNERS (in the repo root), could you update this accordingly?

@kdudka kdudka marked this pull request as draft January 6, 2025 15:00
kdudka added a commit to kdudka/build-definitions that referenced this pull request Jan 6, 2025
They cause the CI to be red on tasks derived from the buildah task.

Related: konflux-ci#1653
kdudka added a commit to kdudka/build-definitions that referenced this pull request Jan 6, 2025
It was required but not used for anything.  Also the parameters set
in the build template were not used by the coverity-availability-check
task.

Related: konflux-ci#1653
kdudka added a commit to kdudka/build-definitions that referenced this pull request Jan 6, 2025
kdudka added a commit to kdudka/build-definitions that referenced this pull request Jan 6, 2025
... to make the interface compatible with the `build-container` task

Related: konflux-ci#1653
kdudka added a commit to kdudka/build-definitions that referenced this pull request Jan 6, 2025
... from the build-container task.  The `hack/generate-sast-tasks.sh`
script can be used to rebuild `sast-coverity-check.yaml`.

Related: konflux-ci#1653
kdudka added a commit to kdudka/build-definitions that referenced this pull request Jan 6, 2025
kdudka added 7 commits January 6, 2025 16:17
They cause the CI to be red on tasks derived from the buildah task.

Related: konflux-ci#1653
It was required but not used for anything.  Also the parameters set
in the build template were not used by the coverity-availability-check
task.

Related: konflux-ci#1653
... to make the interface compatible with the `build-container` task

Related: konflux-ci#1653
... from the build-container task.  The `hack/generate-sast-tasks.sh`
script can be used to rebuild `sast-coverity-check.yaml`.

Related: konflux-ci#1653
@kdudka
Copy link
Contributor Author

kdudka commented Jan 6, 2025

@tnevrlka I have rebased this onto buildah/0.3 and will handle review comments by @chmeliik tomorrow.

@kdudka
Copy link
Contributor Author

kdudka commented Jan 6, 2025

/ok-to-test

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