Skip to content

Conversation

@arewm
Copy link
Contributor

@arewm arewm commented Aug 22, 2025

The only way that we can generate attestations is when we also produce protobuf bundles. While we are working to use bundles by default from the CLI, we should enable the cosign api to be used as a dependency to also assist in migrating to the referrer's api from the tag-based strategy.

Co-Authored-By: Gemini
Signed-off-by: arewm [email protected]

rh-pre-commit.version: 2.3.2
rh-pre-commit.check-secrets: ENABLED

Summary

Release Note

Added functions WriteReferrer and WriteAttestationsReferrer to the API enabling the ability to write
generic attestations with the referrer's API.

Documentation

No changes are made to the CLI interface.

The only way that we can generate attestations is when we also produce
protobuf bundles. While we are working to use bundles by default from
the CLI, we should enable the cosign api to be used as a dependency to
also assist in migrating to the referrer's api from the tag-based
strategy.

Co-Authored-By: Gemini
Signed-off-by: arewm <[email protected]>

rh-pre-commit.version: 2.3.2
rh-pre-commit.check-secrets: ENABLED
@codecov
Copy link

codecov bot commented Aug 22, 2025

Codecov Report

❌ Patch coverage is 62.85714% with 26 lines in your changes missing coverage. Please review.
✅ Project coverage is 34.77%. Comparing base (2ef6022) to head (5057202).
⚠️ Report is 492 commits behind head on main.

Files with missing lines Patch % Lines
pkg/oci/remote/write.go 62.85% 18 Missing and 8 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4357      +/-   ##
==========================================
- Coverage   40.10%   34.77%   -5.34%     
==========================================
  Files         155      216      +61     
  Lines       10044    15053    +5009     
==========================================
+ Hits         4028     5234    +1206     
- Misses       5530     9139    +3609     
- Partials      486      680     +194     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Member

@steiza steiza left a comment

Choose a reason for hiding this comment

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

I know this is still a draft, but I just wanted to say that I support this approach!

Comment on lines +349 to +354
// We have to pick an artifactType for the referrer manifest. The attestation
// layers themselves are DSSE envelopes, which wrap in-toto statements.
// For discovery, the artifactType should describe the semantic content (the
// in-toto statement) rather than the wrapper format (the DSSE envelope).
// Using the in-toto media type is the most appropriate and conventional choice,
// as policy engines and other tools will query for attestations using this type.
Copy link
Member

Choose a reason for hiding this comment

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

I had to really think through this (mostly because as mentioned I haven't spent much time with cosign's OCI functions), but I think this is the right answer.

Retrieving an attestation that doesn't use the new protobuf bundles:

$ go run cmd/cosign/main.go download attestation ghcr.io/steiza/actions-oidc-proxy:v1 | jq
{
  "payloadType": "application/vnd.in-toto+json",
  "payload": "eyJfdHlwZSI6Imh0dHBzOi8vaW4tdG90by5pby9TdGF0ZW1lbnQvdjAuMSIsInByZWRpY2F0ZVR5cGUiOiJodHRwczovL2Nvc2lnbi5zaWdzdG9yZS5kZXYvYXR0ZXN0YXRpb24vdjEiLCJzdWJqZWN0IjpbeyJuYW1lIjoiZ2hjci5pby9zdGVpemEvYWN0aW9ucy1vaWRjLXByb3h5IiwiZGlnZXN0Ijp7InNoYTI1NiI6ImEwNGMyOTk2YzRjNDJhYTUxNzQ5MmJjMGQ4ZTA5Y2EwMDJhNTk5ZDEyZjVlODJiZDUzNzNlZDYzMTNhZjZhNzkifX1dLCJwcmVkaWNhdGUiOnsiRGF0YSI6IntcInBhc3NlZFwiOiB0cnVlfVxuIiwiVGltZXN0YW1wIjoiMjAyNS0wOC0xNVQxODoyMjoyMVoifX0=",
  "signatures": [
    {
      "keyid": "",
      "sig": "MEYCIQD+TJnd3/GuORNfKMMpbviudNGyy+G9qUw0nOtQABEEnAIhAKiTsFRVWdPRSVN/xD3LxGkb/5xPz4f8voN6e16jdePa"
    }
  ]
}

application/vnd.in-toto+json makes sense!

@arewm arewm marked this pull request as ready for review August 26, 2025 13:31
@arewm arewm requested a review from a team as a code owner August 26, 2025 13:31
@arewm
Copy link
Contributor Author

arewm commented Aug 26, 2025

@steiza , I am happy for this change to be non-draft if you want to actually review it. I just put it in draft so that I could reference it in my comment on the issue.

I didn't add any test cases for this, but this section of code was also not very well tested. I can add some if you would like.

@steiza
Copy link
Member

steiza commented Aug 27, 2025

I didn't add any test cases for this, but this section of code was also not very well tested. I can add some if you would like.

I'm in favor of testing generally, but to unit test this it seems like we'd need to mock quite a bit (at least remote.Head, remote.WriteLayer, and remote.Put).

Another option we have for testing is to implement something in https://github.com/sigstore/cosign/blob/main/test/e2e_test.go. Do you want to take a look at that file and see if it makes sense to extend an existing test or add a new one to cover these changes? Don't feel like you have to add something - if it doesn't make sense that's fine too - but I think it's worth at least checking. cc @haydentherapper has more cosign testing experience than I do.

@arewm
Copy link
Contributor Author

arewm commented Aug 27, 2025

I think that the primary intention of e2e_test.go is to test the CLI. We do test the new bundle format in that test, so that will be used to ensure that this refactoring is backwards compatible. Since we are not adding CLI functionality to generate referring attestations with the legacy format, it doesn't seem appropriate to include tests there.

The additional tests that would be added would be to ensure that the artifactType is what we actually what we tried to set it to.

@haydentherapper
Copy link
Contributor

At this point, e2e_test.go is a bit of everything. If you needed an OCI index to avoid mocking out calls, I believe we've set one up in the e2e tests already. Some additional tests would be great though I recognize testing in Cosign can sometimes be 90% of the time spent on a feature, so if it's becoming too much of a pain, just let us know.

Co-Authored-By: Claude <[email protected]>
Signed-off-by: arewm <[email protected]>

rh-pre-commit.version: 2.3.2
rh-pre-commit.check-secrets: ENABLED
@arewm arewm force-pushed the extract-write-referrer branch from 7a6af92 to 5057202 Compare August 27, 2025 18:56
@arewm
Copy link
Contributor Author

arewm commented Aug 27, 2025

I had Claude help to write some test, but I didn't exercise it any more from the e2e tests as there is no additional CLI interface added. I looks like it went ahead and wrote tests for more than what I added though to cover some of the gap.

Copy link
Member

@steiza steiza left a comment

Choose a reason for hiding this comment

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

Looks good to me - nice work!

@steiza steiza merged commit 68caffd into sigstore:main Aug 29, 2025
29 checks passed
@arewm arewm deleted the extract-write-referrer branch August 29, 2025 14:33
arewm added a commit to arewm/tekton-chains that referenced this pull request Sep 16, 2025
Proposing to bump the cosign version to include new library calls to
push attestations with the referrer's API.

- [Release notes](https://github.com/sigstore/cosign/releases/tag/v2.6.0)
- [Changelog](https://github.com/sigstore/cosign/blob/6431af15a8066c4b33c7232fc2dba3f9278a16a5/CHANGELOG.md)
- [Commits](sigstore/cosign/compare/v2.5.3...v2.6.0)

The new changes desired are sigstore/cosign/pull/4357 which should
better support the work in tektoncd#1409.

Signed-off-by: arewm <[email protected]>

rh-pre-commit.version: 2.3.2
rh-pre-commit.check-secrets: ENABLED
arewm added a commit to arewm/tekton-chains that referenced this pull request Sep 16, 2025
Proposing to bump the cosign version to include new library calls to
push attestations with the referrer's API.

- [Release notes](https://github.com/sigstore/cosign/releases/tag/v2.6.0)
- [Changelog](https://github.com/sigstore/cosign/blob/6431af15a8066c4b33c7232fc2dba3f9278a16a5/CHANGELOG.md)
- [Commits](sigstore/cosign@v2.5.3...v2.6.0)

The new changes desired are sigstore/cosign/pull/4357 which should
better support the work in tektoncd#1409.

Signed-off-by: arewm <[email protected]>

rh-pre-commit.version: 2.3.2
rh-pre-commit.check-secrets: ENABLED
lcarva pushed a commit to tektoncd/chains that referenced this pull request Sep 18, 2025
* Bump github.com/sigstore/cosign/v2 from 2.5.3 to 2.6.0

Proposing to bump the cosign version to include new library calls to
push attestations with the referrer's API.

- [Release notes](https://github.com/sigstore/cosign/releases/tag/v2.6.0)
- [Changelog](https://github.com/sigstore/cosign/blob/6431af15a8066c4b33c7232fc2dba3f9278a16a5/CHANGELOG.md)
- [Commits](sigstore/cosign@v2.5.3...v2.6.0)

The new changes desired are sigstore/cosign/pull/4357 which should
better support the work in #1409.

Signed-off-by: arewm <[email protected]>

rh-pre-commit.version: 2.3.2
rh-pre-commit.check-secrets: ENABLED

* Fix LoadPrivateKey API breaking change for cosign v2.6.0

- Add LoadOption parameter to LoadPrivateKey calls in x509.go and clients.go
- Pass nil for defaultLoadOptions to use sensible defaults (ED25519ph)
- Update both production code and test code to match new API

The LoadPrivateKey function signature changed in cosign v2.6.0 to
include a third parameter for LoadOption configuration. Passing nil
uses the default ED25519ph behavior which is appropriate for this
use case.

Co-authored-by: Claude Sonnet <[email protected]>
Signed-off-by: arewm <[email protected]>

rh-pre-commit.version: 2.3.2
rh-pre-commit.check-secrets: ENABLED
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.

3 participants