Skip to content

feat: support OCI Image#683

Merged
akashsinghal merged 1 commit intonotaryproject:mainfrom
akashsinghal:akashsinghal/ociImageSupport
Mar 3, 2023
Merged

feat: support OCI Image#683
akashsinghal merged 1 commit intonotaryproject:mainfrom
akashsinghal:akashsinghal/ociImageSupport

Conversation

@akashsinghal
Copy link
Copy Markdown
Collaborator

Description

What this PR does / why we need it:

Most verifiers rely on the Blobs field to enumerate the content of each artifact. This field is populated directly from the manifest byte marshalling. In the case of an OCI Image referrer, this marshalling will not populate Blobs causing all other verifiers to prematurely finish before any content is verified. This PR:

  • Updates ReferenceManifest to use Blobs and adds logic in ORAS store to marshal Layers/Blobs to the Blobs field depending on the referrer media type.
  • Updates Notaryv2 verifier to check for 0 length blob array and fail if no signatures found
  • Updates Cosign verifier to use Blobs field

TODO: Add e2e test with OCI Image and OCI Artifact as referrer manifest type

Which issue(s) this PR fixes (optional, using fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when the PR gets merged):

Fixes #

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Please also list any relevant details for your test configuration

  • Test A
  • Test B

Checklist:

  • Does the affected code have corresponding tests?
  • Are the changes documented, not just with inline documentation, but also with conceptual documentation such as an overview of a new feature, or task-based documentation like a tutorial? Consider if this change should be announced on your project blog.
  • Does this introduce breaking changes that would require an announcement or bumping the major version?
  • Do all new files have appropriate license header?

@akashsinghal akashsinghal self-assigned this Mar 2, 2023
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 2, 2023

Codecov Report

Patch coverage: 40.00% and project coverage change: +0.01 🎉

Comparison is base (3eb708c) 44.57% compared to head (0bb6552) 44.59%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #683      +/-   ##
==========================================
+ Coverage   44.57%   44.59%   +0.01%     
==========================================
  Files          56       56              
  Lines        3244     3301      +57     
==========================================
+ Hits         1446     1472      +26     
- Misses       1651     1682      +31     
  Partials      147      147              
Impacted Files Coverage Δ
pkg/referrerstore/oras/oras.go 0.63% <0.00%> (-0.08%) ⬇️
pkg/verifier/notaryv2/notaryv2.go 84.67% <50.00%> (+3.36%) ⬆️
pkg/referrerstore/oras/utils.go 29.62% <100.00%> (+29.62%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Copy Markdown
Collaborator

@susanshi susanshi left a comment

Choose a reason for hiding this comment

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

LGTM. Left some questions that can be addressed later if needed.

oras attach \
--artifact-type org.example.sbom.v0 \
--plain-http \
--image-spec v1.1-image \
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

we can move the image spec version to a variable it can be easily updated.

@susanshi
Copy link
Copy Markdown
Collaborator

susanshi commented Mar 3, 2023

we should also add doc to state we support both format incase customer have questions

binbin-li
binbin-li previously approved these changes Mar 3, 2023
Copy link
Copy Markdown
Contributor

@binbin-li binbin-li left a comment

Choose a reason for hiding this comment

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

Left one minor comment, lgtm.

@akashsinghal
Copy link
Copy Markdown
Collaborator Author

we should also add doc to state we support both format incase customer have questions

Yes agreed. I'm struggling to figure out where to put though since we don't have an ORAS store specific doc.

@akashsinghal akashsinghal force-pushed the akashsinghal/ociImageSupport branch from 97d574e to 0bb6552 Compare March 3, 2023 05:38
@akashsinghal akashsinghal marked this pull request as ready for review March 3, 2023 05:40
@akashsinghal akashsinghal changed the title [WIP] feat: support OCI Image feat: support OCI Image Mar 3, 2023
@akashsinghal akashsinghal merged commit 8ed9ccf into notaryproject:main Mar 3, 2023
@susanshi
Copy link
Copy Markdown
Collaborator

susanshi commented Mar 6, 2023

we should also add doc to state we support both format incase customer have questions

Yes agreed. I'm struggling to figure out where to put though since we don't have an ORAS store specific doc.

We can add a new section under conCecpt -> store , and add a link from store CRDs. https://github.com/deislabs/ratify/blob/main/docs/developer/store.md
https://github.com/deislabs/ratify/blob/main/docs/reference/crds/stores.md

This will be similar to section for Notary at https://github.com/deislabs/ratify/blob/main/docs/developer/verifier.md#section-6-built-in-verifiers

@akashsinghal akashsinghal deleted the akashsinghal/ociImageSupport branch July 14, 2023 21:44
bspaans pushed a commit to bspaans/ratify that referenced this pull request Oct 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants