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

Add Verifier to get public key/cert and identities for entry type #1210

Merged
merged 11 commits into from
Jan 11, 2023

Conversation

haydentherapper
Copy link
Contributor

@haydentherapper haydentherapper commented Nov 18, 2022

Looking for feedback before adding tests

Fixes #1173

Signed-off-by: Hayden Blauzvern hblauzvern@google.com

Summary

Release Note

Documentation

@haydentherapper haydentherapper marked this pull request as draft November 18, 2022 01:04
@haydentherapper
Copy link
Contributor Author

haydentherapper commented Nov 18, 2022

cc @asraa @priyawadhwa - Looking for some feedback before adding more tests! The motivation for this work is to make it easy for users to access the public key or certificate of the entry type - Particularly useful for monitoring.

The only thing I don't love is exposing pgp in the function, not sure a way around that though.

For RFC3161, we can't implement support for public keys. For TUF, I chose to not implement support for now at least, because it's not clear how best to return a set of public keys - Just from the root? From targets too? For minisign, I added a TODO.

@haydentherapper
Copy link
Contributor Author

One thing I’ll change is setting the key in unmarshal. I’ll just read the key field in in Verifier. More usable that way

pkg/types/entries.go Outdated Show resolved Hide resolved
@haydentherapper
Copy link
Contributor Author

Still needs tests, but this looks a lot better I think @asraa

Main changes:

  • EntryImpl needs a Verifier() that returns a pki.PublicKey
  • PublicKey now requires an Identities() that returns a list of subjects from either pgp keys, ssh keys, or certs, and for keys, returns either the PEM encoding (pkcs7/tuf/x509) or a specific encoding (minisign/pgp)

@codecov-commenter
Copy link

codecov-commenter commented Nov 18, 2022

Codecov Report

Merging #1210 (3906cf0) into main (eb97335) will increase coverage by 0.02%.
The diff coverage is 58.88%.

@@            Coverage Diff             @@
##             main    #1210      +/-   ##
==========================================
+ Coverage   63.16%   63.19%   +0.02%     
==========================================
  Files          82       82              
  Lines        7670     7847     +177     
==========================================
+ Hits         4845     4959     +114     
- Misses       2212     2258      +46     
- Partials      613      630      +17     
Flag Coverage Δ
e2etests 46.71% <1.66%> (-1.03%) ⬇️
unittests 42.62% <58.88%> (+0.60%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/types/entries.go 47.45% <ø> (ø)
pkg/types/rfc3161/v0.0.1/entry.go 63.15% <0.00%> (-0.97%) ⬇️
pkg/types/test_util.go 0.00% <0.00%> (ø)
pkg/types/jar/v0.0.1/entry.go 46.72% <25.00%> (-0.85%) ⬇️
pkg/types/rekord/v0.0.1/entry.go 66.77% <26.66%> (-0.34%) ⬇️
pkg/types/intoto/v0.0.2/entry.go 69.38% <33.33%> (-1.14%) ⬇️
pkg/types/tuf/v0.0.1/entry.go 49.21% <33.33%> (-0.59%) ⬇️
pkg/pki/tuf/tuf.go 53.76% <40.00%> (-1.66%) ⬇️
pkg/types/intoto/v0.0.1/entry.go 70.79% <40.00%> (-0.70%) ⬇️
pkg/pki/x509/x509.go 69.56% <51.51%> (-3.95%) ⬇️
... and 12 more

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

Copy link
Contributor

@asraa asraa left a comment

Choose a reason for hiding this comment

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

Nice change! Some quick first comments

pkg/pki/pkcs7/pkcs7.go Outdated Show resolved Hide resolved
pkg/pki/ssh/ssh.go Outdated Show resolved Hide resolved
pkg/pki/tuf/tuf.go Show resolved Hide resolved
pkg/types/cose/v0.0.1/entry.go Show resolved Hide resolved
Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>
Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>
Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>
Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>
Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>
Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>
Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>
Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>
@haydentherapper haydentherapper marked this pull request as ready for review December 20, 2022 23:14
@haydentherapper
Copy link
Contributor Author

haydentherapper commented Dec 20, 2022

@asraa @bobcallaway It's ready for another review. I've added tests for all types.

There's one potential bug I've found that I'm filing issues for:

  • For PGP, CanonicalValue re-encodes the PGP key such that the PEM encoding does not match the input
  • For JARs, the test JAR fails the test (that's commented out) because it says that a CERTIFICATE block, rather than a PKCS7 block, was found. I'm not sure if the JAR is invalid or if we need to allow different certificate formats. I also think there might be a bug in that we pass both the same pkcs7 signature to the NewPublicKey and NewSignature functions, unless this is WAI? (Because the PEM decoder will drop everything before the first PEM block it finds) EDIT: That was my error

Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>
@haydentherapper haydentherapper changed the title [WIP] Add Verifier to get public key/cert for entry type Add Verifier to get public key/cert and identities for entry type Dec 21, 2022
@haydentherapper
Copy link
Contributor Author

@asraa @bobcallaway Bumping for review

Copy link
Contributor

@asraa asraa left a comment

Choose a reason for hiding this comment

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

Thanks for revamping this! It was an easy review!

pkg/pki/pkcs7/pkcs7.go Outdated Show resolved Hide resolved
pkg/pki/pkcs7/pkcs7.go Outdated Show resolved Hide resolved
pkg/pki/pkcs7/pkcs7.go Outdated Show resolved Hide resolved
pkg/pki/x509/sans.go Outdated Show resolved Hide resolved
pkg/pki/x509/x509.go Outdated Show resolved Hide resolved
@haydentherapper
Copy link
Contributor Author

@asraa ready for another review!

* Use sigstore/sigstore method for SAN extraction
* Included PEM certificate in pkcs7/x509 PKI types

Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>
Copy link
Contributor

@asraa asraa left a comment

Choose a reason for hiding this comment

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

Just one final nit!

pkg/pki/x509/x509.go Outdated Show resolved Hide resolved
Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>
@bobcallaway bobcallaway merged commit ee83776 into sigstore:main Jan 11, 2023
@github-actions github-actions bot added this to the v1.1.0 milestone Jan 11, 2023
@haydentherapper haydentherapper deleted the extractcerts branch January 11, 2023 20:20
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.

Add public key method to EntryImpl
5 participants