-
Notifications
You must be signed in to change notification settings - Fork 7
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
Verify entry signatures #146
Conversation
beb3b54
to
8a922d0
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #146 +/- ##
==========================================
+ Coverage 26.84% 31.32% +4.47%
==========================================
Files 29 35 +6
Lines 1892 2442 +550
==========================================
+ Hits 508 765 +257
- Misses 1346 1606 +260
- Partials 38 71 +33 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Can rebase on #144 if that goes in first |
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.
seems ok to me (one question in code though):
- please decide merge order with @loosebazooka
- let's have a good look at both test coverage (with an eye for more than code line coverage) and potentially simplifying these critical verification methods after these big PRs are in
I can merge last. I'd like to talk through the new protos in our sync today |
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.
Seems fine to me if we're mostly just copying over from rekor
pkg/pki/x509/x509.go
Outdated
"io" | ||
"strings" | ||
|
||
"github.com/asaskevich/govalidator" |
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.
It looks like this hasn't had a release since 2021, maybe that's okay? It doesn't look abandoned.
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.
Actually I can remove this, it looks like it's only used to verify EmailAddresses and there's a note saying that was deprecated https://github.com/sigstore/rekor/blob/73dba7c07d0747f00119417fc0ff994a393f97b2/pkg/pki/pki.go#L28
This copies in in parts of github.com/sigstore/rekor/pkg/pki to help with parsing the public key and checking the signature. Signed-off-by: Colleen Murphy <colleenmurphy@google.com>
8a922d0
to
5bbc4cc
Compare
Removed the deprecated x509 interface method and added tests for x.509 verifiers. |
Quick comment, this looks good. There's a few things I'd like to change in the copied files, such as cleaning up hardcoded sha256 refs and dropping support for cert chains, but I'd rather this get merged with minimal changes to the copied files and we follow up later. Feel free to create an issue and assign it to me for following up. |
This copies in in parts of github.com/sigstore/rekor/pkg/pki to help with parsing the public key and checking the signature.
Fixes #10
Summary
Release Note
Documentation