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

(feat) add support for attestations stored as image manifest #1482

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

RealHarshThakur
Copy link

Description of the PR

Currently, guac only works with certain OCI images. Build tools store attestations in a variety of ways as of now and it might take them a while to adopt to using referrers. As of now, Docker stores the attestation in an image manifest and it would be wonderful for GUAC to support that as docker is a popular tool for building containers. I'm not entirely sure if this is the right way to implement this, I'd appreciate any feedback to make this change a reality.

PR Checklist

  • All commits have a Developer Certificate of Origin (DCO) -- they are generated using -s flag to git commit.
  • All new changes are covered by tests
  • If GraphQL schema is changed, make generate has been run
  • If collectsub protobuf has been changed, make proto has been run
  • All CI checks are passing (tests and formatting)
  • All dependent PRs have already been merged

Signed-off-by: Harsh Thakur <harshthakur9030@gmail.com>
@RealHarshThakur RealHarshThakur marked this pull request as draft November 10, 2023 16:18
@ridhoq
Copy link
Contributor

ridhoq commented Nov 10, 2023

Hi @RealHarshThakur thanks for the PR! This is actually something I created an issue on #1370 so it's great to see somebody picking up this work. There is a bit of overlap here with #1449 which I believe will also cover collection of these types of attestations. But, this change also will move away from regclient to google/go-containerregistry as the library to access OCI packages. The community is discussing this change in #1456. Don't take it as a reason to stop further work on this - just wanted to bring you up to speed on this topic :)

@RealHarshThakur
Copy link
Author

My bad 😓 I had it on my todo list before the issue was created and just found this weekend to hack on it.
From skimming the other PR, I think one way to move forward would be for me to refactor this PR to focus on processor and parser. Essentially, all changes to introduce DocumentITE6SPDX.

Copy link
Collaborator

@pxp928 pxp928 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @RealHarshThakur. Added some comments

var spdxDoc *v2_3.Document
if doc.Type == processor.DocumentITE6SPDX {
var err error
itespdx, err := parseITESPDXBlob(doc.Blob)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be part of the processor such the parser does not need to change as it will just receive a valid SPDX document at the end.

@@ -44,6 +44,7 @@ func init() {
_ = RegisterDocumentParser(dsse.NewDSSEParser, processor.DocumentDSSE)
_ = RegisterDocumentParser(slsa.NewSLSAParser, processor.DocumentITE6SLSA)
_ = RegisterDocumentParser(vuln.NewVulnCertificationParser, processor.DocumentITE6Vul)
_ = RegisterDocumentParser(spdx.NewSpdxParser, processor.DocumentITE6SPDX)
Copy link
Collaborator

Choose a reason for hiding this comment

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

it can be expanded here similar to how its done for DSSE

Copy link

stale bot commented Jan 13, 2024

This pull request has been automatically marked as stale because it has not had recent activity (60 days of inactivity).
It will be closed in 30 days if no further activity occurs.
Thank you for your contribution!

@stale stale bot added the wontfix This will not be worked on label Jan 13, 2024
Copy link
Collaborator

@pxp928 pxp928 left a comment

Choose a reason for hiding this comment

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

@RealHarshThakur marking this as not stale as it is an important topic to keep working on.

@RealHarshThakur
Copy link
Author

sorry for delay, I'll try to make some time this month

@stale stale bot removed the wontfix This will not be worked on label Jan 15, 2024
Copy link

stale bot commented Mar 15, 2024

This pull request has been automatically marked as stale because it has not had recent activity (60 days of inactivity).
It will be closed in 30 days if no further activity occurs.
Thank you for your contribution!

@stale stale bot added the wontfix This will not be worked on label Mar 15, 2024
@pxp928
Copy link
Collaborator

pxp928 commented Mar 26, 2024

Adding comment to ensure this does not close.

@stale stale bot removed the wontfix This will not be worked on label Mar 26, 2024
Copy link

stale bot commented May 26, 2024

This pull request has been automatically marked as stale because it has not had recent activity (60 days of inactivity).
It will be closed in 30 days if no further activity occurs.
Thank you for your contribution!

@stale stale bot added the wontfix This will not be worked on label May 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/M wontfix This will not be worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants