-
Notifications
You must be signed in to change notification settings - Fork 537
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
adds tsa cert chain check for env var or tuf targets. #3600
Conversation
bb39074
to
873791f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks pretty good to me, though I'm not a TSA or TUF expert.
It looks like the merge commit accidentally pulled in an unused import so this doesn't build.
This will need some tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, sorry for the delay. Just a couple comments.
+1 to tests.
thanks for the feedback/comments, will try to implement in the next few weeks. |
72c5b77
to
c1bef74
Compare
i've added some of the feedback, just working on tests and returning |
c1bef74
to
cfda46f
Compare
@ianhundere Can you tell me the approximate time of making all the improvements? |
@Meeki1l if not today, by weds. |
c4fc8d7
to
4d6a0d0
Compare
@haydentherapper / @cmurphy okay, i think that about does it. |
Thanks @ianhundere! Will take a closer look tomorrow. Can you take a look at failing tests? |
@haydentherapper no problem, thanks for the quick 👀 / feedback. 🙇 this commit fixes the failing units tests / lint errors:
|
dc62ff0
to
4bb0da1
Compare
ah, had a couple more issues:
now running |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3600 +/- ##
==========================================
+ Coverage 40.10% 40.70% +0.60%
==========================================
Files 155 159 +4
Lines 10044 10225 +181
==========================================
+ Hits 4028 4162 +134
- Misses 5530 5558 +28
- Partials 486 505 +19 ☔ View full report in Codecov by Sentry. |
4bb0da1
to
466bef2
Compare
47be48e
to
679d7a5
Compare
this one is failing due to some infra/network issues:
|
One last thing, need to run |
done / done 🙂 |
@haydentherapper thanks for the 👀 / lemme know if that satisfies everything. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few more places to update! Sorry for all the duplication across verify_* functions
ah, i'll get those / thanks again. edit: @haydentherapper all done |
19a74cc
to
470139b
Compare
470139b
to
83ea3d0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for all of your work on this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error handling lgtm now 👍
@cpanato would you be able to take a look at the test failures? I’m uncertain why they’re failing |
I will take a look; just bear with me :) i am traveling back home today |
looks like @bobcallaway merged a fix / updated metallb as per the sigstore slack (#general): |
Can you rebase? |
sorry for the delay, a rebase might fix |
Signed-off-by: ianhundere <138915+ianhundere@users.noreply.github.com>
…ogic. Signed-off-by: ianhundere <138915+ianhundere@users.noreply.github.com>
83ea3d0
to
3fee9d8
Compare
@haydentherapper done/done 🙂 |
closes #3563
Summary
Creates parity between Cosign / TSA (e.g. TSA values are handled similarly to ctlog, fulcio, and rekor creds now) since sigstore/sigstore TUF client was recently updated to support the "TSA" usage type.
Currently, the TSA cert chain is required via Cosign's cli flag, though, as per #3563, Cosign can support reading the cert chain from either an environment variable or the TUF targets, similar to Fulcio certs, Rekor keys or the CTLog public key that can be provided on verification. I looked at RekorPubKeys and GetCTLogPubs as an example.
huge thanks to @aalsabag for helping w/ unit tests.
Release Note
SIGSTORE_TSA_CERTIFICATE_FILE
, and TUF targets