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

ensure fallback logic executes if attestation key is empty when fetching attestation #878

Merged
merged 6 commits into from
Jun 21, 2022

Conversation

bobcallaway
Copy link
Member

Fixes: #872

Signed-off-by: Bob Callaway bcallaway@google.com

Signed-off-by: Bob Callaway <bcallaway@google.com>
@bobcallaway bobcallaway requested review from cpanato and a team as code owners June 18, 2022 21:16
@codecov-commenter
Copy link

codecov-commenter commented Jun 18, 2022

Codecov Report

Merging #878 (0d8f60a) into main (85e60c5) will decrease coverage by 0.03%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #878      +/-   ##
==========================================
- Coverage   46.33%   46.29%   -0.04%     
==========================================
  Files          60       60              
  Lines        5137     5137              
==========================================
- Hits         2380     2378       -2     
  Misses       2484     2484              
- Partials      273      275       +2     
Impacted Files Coverage Δ
pkg/types/alpine/v0.0.1/entry.go 53.30% <0.00%> (-2.48%) ⬇️
pkg/types/rekord/v0.0.1/entry.go 48.84% <0.00%> (+0.66%) ⬆️
pkg/types/tuf/v0.0.1/entry.go 41.70% <0.00%> (+0.80%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 85e60c5...0d8f60a. Read the comment docs.

Signed-off-by: Bob Callaway <bcallaway@google.com>
@dlorenc
Copy link
Member

dlorenc commented Jun 18, 2022

I think you'll have to push to the main repo instead of a fork, and then only run this test after PR merges for auth to work correctly.

Signed-off-by: Bob Callaway <bcallaway@google.com>
@bobcallaway
Copy link
Member Author

I think you'll have to push to the main repo instead of a fork, and then only run this test after PR merges for auth to work correctly.

nope, i mistyped the reference to the correct container; the creds fix was a red-herring and i just removed it.

Signed-off-by: Bob Callaway <bcallaway@google.com>
Signed-off-by: Bob Callaway <bcallaway@google.com>
@bobcallaway
Copy link
Member Author

this needs #869 to merge first but otherwise this should be ready for review

@bobcallaway bobcallaway requested a review from asraa June 19, 2022 11:24
@dlorenc dlorenc merged commit ee474be into sigstore:main Jun 21, 2022
@github-actions github-actions bot added this to the v1.0.0 milestone Jun 21, 2022
bobcallaway added a commit to bobcallaway/rekor that referenced this pull request Jul 6, 2022
Currently only two Rekor pluggable types support the storage of
attestations (intoto, cose); the previous code to fetch
attestations was type-agnostic, but due to the fix sigstore#878 the
server was doing unnecessary lookups for all types, regardless
of whether they store attestation content or not.

This makes the attestation storage an explict interface, which
we can test casting for and avoid a roundtrip to the storage
layer for types that don't support storing attestations.

Signed-off-by: Bob Callaway <bcallaway@google.com>
dlorenc pushed a commit that referenced this pull request Jul 6, 2022
Currently only two Rekor pluggable types support the storage of
attestations (intoto, cose); the previous code to fetch
attestations was type-agnostic, but due to the fix #878 the
server was doing unnecessary lookups for all types, regardless
of whether they store attestation content or not.

This makes the attestation storage an explict interface, which
we can test casting for and avoid a roundtrip to the storage
layer for types that don't support storing attestations.

Signed-off-by: Bob Callaway <bcallaway@google.com>
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.

attestations for intoto log entries persisted without payloadHash are not being returned
3 participants