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 support for ed25519ph user keys in hashedrekord #1945

Merged
merged 3 commits into from
Mar 4, 2024

Conversation

ret2libc
Copy link
Contributor

@ret2libc ret2libc commented Jan 11, 2024

Summary

Add support for Ed25519ph user keys in hashedrekord type, so we can test #1724 better with 2 options (ecdsa+sha256 and ed25519ph).

This change requires sigstore/sigstore#1595 and #1959 .

Release Note

  • Added x509.NewSignatureWithOpts to support ED25519ph
  • Allow ED25519 public keys when validating hashedrekord entries

Documentation

@ret2libc
Copy link
Contributor Author

cc @woodruffw @tetsuo-cpp

I'm not sure if we need a new version for the change in hashedrekor which allows sha512 as hash algorithm and not only sha256.

@woodruffw
Copy link
Member

Thanks @ret2libc!

Yeah, I think we may need a 0.0.2 here, which means we'll probably want to hold off on making the complete hashedrekord changes until we have a full list of hashes we want to support. But I'm not 100% sure about that.

CCing @haydentherapper for opinions + @laurentsimon since I know you've been looking at similar changes to Rekor/hashedrekord 🙂

@@ -160,11 +159,6 @@ func (v *V001Entry) validate() (pki.Signature, pki.PublicKey, error) {
return nil, nil, types.ValidationError(err)
}

_, isEd25519 := keyObj.CryptoPubKey().(ed25519.PublicKey)
Copy link
Contributor

Choose a reason for hiding this comment

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

Just for context, when I wrote up #1325, my intention was to support ed25519ph only for hashedrekord, and ed25519 for all other types. This might mean making some changes to the 'pkg/pki/x509/x509' library to support loading in a custom verifier like on

verifier, err := sigsig.LoadVerifier(p, crypto.SHA256)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you think about this way of customizing the verifier?

Copy link

codecov bot commented Jan 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 48.93%. Comparing base (488eb97) to head (4831505).
Report is 41 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #1945       +/-   ##
===========================================
- Coverage   66.46%   48.93%   -17.53%     
===========================================
  Files          92       80       -12     
  Lines        9258     6641     -2617     
===========================================
- Hits         6153     3250     -2903     
- Misses       2359     2987      +628     
+ Partials      746      404      -342     
Flag Coverage Δ
e2etests ?
unittests 48.93% <100.00%> (+1.25%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ret2libc ret2libc marked this pull request as ready for review January 17, 2024 14:22
@ret2libc ret2libc requested review from bobcallaway and a team as code owners January 17, 2024 14:22
@ret2libc ret2libc changed the title Add support for ed25519ph user keys Add support for ed25519ph user keys in hashedrekord Jan 17, 2024
@bobcallaway
Copy link
Member

I threw this together as the smallest change to add sha384 and sha512 to the rekor type. I'm happy to use your PR as the definitive change, but wanted to compare/contrast what was strictly needed to extend the enum, VS what was needed for the ed25519ph key types.

i don't think we need a 0.0.2 version of hashedrekord for this, since we're not actually changing the structure or canonicalization logic of the type.

go.mod Outdated
@@ -204,3 +204,5 @@ require (
gopkg.in/yaml.v2 v2.4.0
gopkg.in/yaml.v3 v3.0.1 // indirect
)

replace github.com/sigstore/sigstore => github.com/trail-of-forks/sigstore v0.0.0-20240117103256-3095d93bafe8
Copy link
Member

Choose a reason for hiding this comment

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

this would need to be removed before merging.

Copy link
Member

Choose a reason for hiding this comment

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

Yep! I believe this is here until sigstore/sigstore#1595 gets merged (correct me if I'm wrong @ret2libc), so it probably makes sense to merge #1958 first and then we can rebase here 🙂

Copy link
Contributor

@haydentherapper haydentherapper left a comment

Choose a reason for hiding this comment

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

LGTM, this looks good.

While this might be obvious from the change since it only modifies hashedrekord, i wanted to double check that this won't affect other types. rekord also uses x509.NewSignature indirectly through a factory, but given that we've added a new signature function rather than modify the existing one, this has no impact on other types.

Let's merge #1958 first (or split this into two) if that works for you, just to make it easier to see the two different changes.

@ret2libc
Copy link
Contributor Author

I threw this together as the smallest change to add sha384 and sha512 to the rekor type. I'm happy to use your PR as the definitive change, but wanted to compare/contrast what was strictly needed to extend the enum, VS what was needed for the ed25519ph key types.

i don't think we need a 0.0.2 version of hashedrekord for this, since we're not actually changing the structure or canonicalization logic of the type.

I have included the changes in your PR into mine, and mentioned you in the commit 2577342 . If that's not the proper way to "borrow" code let me know how I should note your help!

By the way, @haydentherapper I'm going to split this into SHA* additions and ED25519ph support.

@haydentherapper
Copy link
Contributor

LGTM, thanks for splitting. Will approve once the sigstore/sigstore PR is merged.

Copy link
Contributor

@haydentherapper haydentherapper left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for splitting this PR up. Will approve once we merge s/s and remove the replace.

@ret2libc ret2libc force-pushed the support-ed25519ph branch 2 times, most recently from 3007336 to e5112b6 Compare January 26, 2024 13:29
go.mod Outdated Show resolved Hide resolved
Copy link
Contributor

@haydentherapper haydentherapper left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for your patience and for all the discussion on the underlying verifier interface. This came out really clean!

Can you update the test for this (https://github.com/sigstore/rekor/blob/main/pkg/types/hashedrekord/v0.0.1/entry_test.go#L116-L120 and https://github.com/sigstore/rekor/blob/main/pkg/types/hashedrekord/v0.0.1/entry_test.go#L195-L211), which I would have expected to be failing now (so it might not have been testing the right thing)?

haydentherapper
haydentherapper previously approved these changes Feb 6, 2024
@haydentherapper
Copy link
Contributor

@ret2libc can you rebase?

@ret2libc
Copy link
Contributor Author

@ret2libc can you rebase?

@haydentherapper rebased! Sorry for the delay

@haydentherapper
Copy link
Contributor

No worries! Sorry, need one more rebase

go.mod Outdated
@@ -26,7 +26,7 @@ require (
github.com/rs/cors v1.10.1
github.com/sassoftware/relic v7.2.1+incompatible
github.com/secure-systems-lab/go-securesystemslib v0.8.0
github.com/sigstore/sigstore v1.8.1
github.com/sigstore/sigstore v1.8.2-0.20240215213640-922069a3bc39
Copy link
Contributor

Choose a reason for hiding this comment

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

can you also bump to v1.8.2, we just cut a release

Copy link
Member

Choose a reason for hiding this comment

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

Sure, doing now!

bobcallaway
bobcallaway previously approved these changes Feb 28, 2024
@bobcallaway
Copy link
Member

other than fixing the merge conflict, I think this is ready to go

- Made X509 Signatures configurable with LoadOptions
- Removed existing check that limited the use of Ed25519 keys in
  HashedRekord entries
- Used Ed25519ph Signer/Verifier for HashedRekord entries

Signed-off-by: Riccardo Schirone <[email protected]>
@bobcallaway bobcallaway merged commit 92742be into sigstore:main Mar 4, 2024
14 checks passed
@github-actions github-actions bot added this to the v1.2.2 milestone Mar 4, 2024
@woodruffw woodruffw deleted the support-ed25519ph branch March 4, 2024 16:47
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.

4 participants