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

fix: fix typo that caused attestation verification failure #2199

Merged
merged 1 commit into from Aug 24, 2022

Conversation

asraa
Copy link
Contributor

@asraa asraa commented Aug 24, 2022

Signed-off-by: Asra Ali asraa@google.com

Summary

Release Note

  • bug: fixes a bug in container attestation verification with experimental mode in version 1.11.0

Documentation

Fixes #2197
See #2118

Signed-off-by: Asra Ali <asraa@google.com>
@asraa
Copy link
Contributor Author

asraa commented Aug 24, 2022

@cpanato this is a pretty bug behavior bug in cosign 1.11. May be worth a patch release.

@nkreiger @bobcallaway PTAL

@codecov-commenter
Copy link

Codecov Report

Merging #2199 (4fbc59a) into main (13b84a2) will not change coverage.
The diff coverage is 0.00%.

@@           Coverage Diff           @@
##             main    #2199   +/-   ##
=======================================
  Coverage   26.69%   26.69%           
=======================================
  Files         131      131           
  Lines        7690     7690           
=======================================
  Hits         2053     2053           
  Misses       5376     5376           
  Partials      261      261           
Impacted Files Coverage Δ
pkg/cosign/tlog.go 38.55% <0.00%> (ø)

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

Copy link
Member

@bobcallaway bobcallaway left a comment

Choose a reason for hiding this comment

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

agreed, we should update and release this ASAP

Copy link
Contributor

@lcarva lcarva left a comment

Choose a reason for hiding this comment

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

I have verified that this fixes the reproducer from #2197

@nkreiger
Copy link
Contributor

looks good, my bad I'll never live that one down...

any reason why the e2e tests didn't catch it?

@cpanato
Copy link
Member

cpanato commented Aug 24, 2022

ok, will prepare the 1.11.1 for today

@asraa
Copy link
Contributor Author

asraa commented Aug 24, 2022

No worries @nkreiger, it was on all of us :)

I just checked the e2e tests and it seems like there's just a SignCmd tlog test. I'll try to do a follow-up e2e test with an attestation.

@cpanato cpanato merged commit b3b6ae2 into sigstore:main Aug 24, 2022
@github-actions github-actions bot added this to the v1.12.0 milestone Aug 24, 2022
@dlorenc
Copy link
Member

dlorenc commented Aug 24, 2022

Looks like there's an attestation test here:

must(attest.AttestCmd(ctx, ko, options.RegistryOptions{}, imgName, "", "", false, attestationPath, false,

Not sure why it didn't catch this... Is it only for tlog cases?

@dlorenc
Copy link
Member

dlorenc commented Aug 24, 2022

Ah - I get it now. That used to be the same code path (same HashedRekord type stored in rekor for both versions), now it's separate so we're missing the verification test for the new code path.

@asraa
Copy link
Contributor Author

asraa commented Aug 24, 2022

Looks like there's an attestation test here:

Hmm I think attestverify is never called with tlog explicit enabled in the tests.

@dlorenc
Copy link
Member

dlorenc commented Aug 24, 2022

Hmm I think attestverify is never called with tlog explicit enabled in the tests.

Yep - we have sign/verify with and without tlog, and attest/verify without tlog. The tlog part used to be shared code, but is separated now.

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.

verify-attestation fails when rekor is enabled
7 participants