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

OCI-attach reports from clair-scan 0.2 Task #1483

Merged
merged 1 commit into from
Oct 4, 2024

Conversation

zregvart
Copy link
Member

@zregvart zregvart commented Sep 30, 2024

This attaches the "clair"-formatted output of the clair-action command to the scanned image. Contrary to the sast-snyk-check Task only a single variant of attachment method is supported, based on the registry support. For quay.io OCI Distribution 1.1. Referrers API will be used.

The clair-action needs to be run twice as the Rego rules executed in the conftest-vulnerabilities require the "quay" format, which does not include any date information, whereas the (future) EC policy Rego rules require the "clair" format, which does.

Resolves: https://issues.redhat.com/browse/EC-837

Notes for reviewers:

  • should this be uploading both OCI 1.1 distribution referrers tag and API variants, analogous to the sast-snyk-check Task?
  • is there a better MIME type other than application/vnd.redhat.clair-report+json that could be used?
  • should the report be transformed into a in-toto enveloped vuln predicate attestation and attached as such?

@lcarva
Copy link
Contributor

lcarva commented Sep 30, 2024

should the report be transformed into a in-toto enveloped vuln predicate attestation and attached as such?

Probably, but Konflux is not setup for this now.

task/clair-scan/0.2/clair-scan.yaml Outdated Show resolved Hide resolved
task/clair-scan/0.2/clair-scan.yaml Show resolved Hide resolved
echo "Selecting auth"
select-oci-auth "$IMAGE_URL" > "$HOME/auth.json"

base_image="${IMAGE_URL/:*/}"
Copy link
Contributor

@lcarva lcarva Sep 30, 2024

Choose a reason for hiding this comment

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

Maybe let's call this something like repository? "Base image" has a different meaning that may cause confusion.

for f in clair-report-*.json; do
image_ref="${base_image}@$(cat "image-manifest-$(arch "$f").sha")"
echo "Attaching $f to ${image_ref}"
oras attach --no-tty --registry-config "$HOME/auth.json" --artifact-type "${MEDIA_TYPE}" "${image_ref}" "$f:${MEDIA_TYPE}"
Copy link
Contributor

Choose a reason for hiding this comment

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

On the EC side, we can't trust this information since there's no guarantee that the attached data was actually produced by this Task. Anyone with access to the registry can easily fake the report. This data is informative at best.

This Task needs to emit a digest of the expected data.

If we had identity-based signatures (keyless) enabled in Konflux, we would still need to do this since in that case users could still fake a report via some rogue Pipeline. Slightly harder, but not a risk we should take.

Copy link
Member Author

Choose a reason for hiding this comment

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

I did remember that after pushing, will run up against the result limit though...

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, we may have to create a combined result of some sort. You could, for example, put them altogether in an Image Index.

@zregvart
Copy link
Member Author

zregvart commented Oct 2, 2024

should the report be transformed into a in-toto enveloped vuln predicate attestation and attached as such?

Probably, but Konflux is not setup for this now.

We can choose to use the format, regardless of the attestation being signed or not

@zregvart zregvart force-pushed the issue/EC-837 branch 6 times, most recently from d94e785 to c4749cf Compare October 2, 2024 12:55
@@ -214,6 +214,7 @@ This pipeline is pushed as a Tekton bundle to [quay.io](https://quay.io/reposito
|name|description|used in params (taskname:taskrefversion:taskparam)
|---|---|---|
|IMAGES_PROCESSED| Images processed in the task.| |
|REPORTS| Mapping of image digests to report digests| |
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe:

Suggested change
|REPORTS| Mapping of image digests to report digests| |
|REPORT_MAP| Mapping of image digests to report digests| |

or...

Suggested change
|REPORTS| Mapping of image digests to report digests| |
|REPORT_REFS| Mapping of image digests to report digests| |

|-------------------|------------------------------------------|
| TEST_OUTPUT | Tekton task test output. |
| SCAN_OUTPUT | Clair scan result. |
| REPORTS |Mapping of image digests to report digests|
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 json? Maybe:

Suggested change
| REPORTS |Mapping of image digests to report digests|
| REPORT_REFS |Mapping of image digests to report digests in JSON format|

This attaches the "clair"-formatted output of the `clair-action` command
to the scanned image. Contrary to the `sast-snyk-check` Task only a
single variant of attachment method is supported, based on the registry
support. For quay.io OCI Distribution 1.1. Referrers API will be used.

The `clair-action` needs to be run twice as the Rego rules executed in
the `conftest-vulnerabilities` require the "quay" format, which does not
include any date information, whereas the (future) EC policy Rego rules
require the "clair" format, which does.

Resolves: https://issues.redhat.com/browse/EC-837
@lcarva
Copy link
Contributor

lcarva commented Oct 3, 2024

should the report be transformed into a in-toto enveloped vuln predicate attestation and attached as such?

Probably, but Konflux is not setup for this now.

We can choose to use the format, regardless of the attestation being signed or not

The benefit the format would bring is that it links the data to a predicate (image). You are doing that already in the REPORTS result. Using an in-toto envelope would standardize that. I don't think it covers any gaps though.

@lcarva
Copy link
Contributor

lcarva commented Oct 3, 2024

should this be uploading both OCI 1.1 distribution referrers tag and API variants, analogous to the sast-snyk-check Task?

For posterity, in the arch meeting it was mentioned that double uploading may cause some issues. The Tasks that were doing it, no longer do it, see c746a25.

is there a better MIME type other than application/vnd.redhat.clair-report+json that could be used?

I think this one is reasonable since it is a completely custom format.

@zregvart
Copy link
Member Author

zregvart commented Oct 4, 2024

Last call for discussions:

  • the result name (REPORTS vs REPORT_MAP vs REPORT_REFS), the REPORTS suits me fine because it is short and I don't need to change anything
  • using in-toto + vuln predicate, would be nice, though doesn't seem to address an issue we have
  • using image index for the reports, would be better to keep the result smaller, a new step would need to be added for this, probably utilizing quay.io/konflux-ci/buildah-task

I'm lazy to take on more work here and I'm hovering over the merge button. Let me know if you feel strongly about any of this.

@jsztuka
Copy link
Contributor

jsztuka commented Oct 4, 2024

Im team REPORTS only.
Bullet point 3 looks like candidate for follow up story. If it turns out we want it.

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

@jsztuka
Copy link
Contributor

jsztuka commented Oct 4, 2024

Great work, thanks!

@zregvart zregvart added this pull request to the merge queue Oct 4, 2024
@zregvart
Copy link
Member Author

zregvart commented Oct 4, 2024

Thanks for the reviews/feedback!

@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 4, 2024
@zregvart zregvart added this pull request to the merge queue Oct 4, 2024
Merged via the queue into konflux-ci:main with commit d66d95d Oct 4, 2024
14 checks passed
@zregvart zregvart deleted the issue/EC-837 branch October 4, 2024 15:11
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