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 Signer/Verifier #1595

Merged
merged 4 commits into from Jan 29, 2024

Conversation

ret2libc
Copy link
Contributor

Summary

Add support for Ed25519ph digital signature to be used by other repos like Rekor, Cosign and sigstore-go.

Full e2e tests require changes in cosign and rekor as well.
Tests have been added for the Ed25519ph Signer/Verifier similar to the others.

Release Note

  • Added ED25519phSigner, ED25519phVerifier, and ED25519phSignerVerifier
  • Changed LoadSigner, LoadVerifier, and LoadSignerVerifier to default to the ph version of Ed25519 whenever a ed25519 private/public key is found.
  • Closed Add support for ed25519ph signer/verifier #1594

Documentation

No changes in documentation required, I think.

@ret2libc
Copy link
Contributor Author

I'd appreciate any comment or concern, in particular with regards to the change of behaviour of LoadSigner, LoadVerifier, and LoadSignerVerifier. Would that break anything currently used by rekor/cosign/sigstore-go?

@ret2libc

This comment was marked as outdated.

@ret2libc

This comment was marked as outdated.

@haydentherapper
Copy link
Contributor

@ret2libc I'm going to be taking a look at all PRs later today and tomorrow. I have a bit of a backlog I'm working through.

@ret2libc
Copy link
Contributor Author

@ret2libc I'm going to be taking a look at all PRs later today and tomorrow. I have a bit of a backlog I'm working through.

Thanks! Actually, I have a WIP branch which I think is a bit cleaner. It uses the Options pattern and it looks like this:

func LoadSignerVerifier(privateKey crypto.PrivateKey, hashFunc crypto.Hash, opts ...SignerVerifierOption) (SignerVerifier, error) {
	o := makeSignerVerifierOpts(opts...)

	switch pk := privateKey.(type) {
	case *rsa.PrivateKey:
		if o.rsaPSSOptions != nil {
			return LoadRSAPSSSignerVerifier(pk, hashFunc, o.rsaPSSOptions)
		}
		return LoadRSAPKCS1v15SignerVerifier(pk, hashFunc)
	case *ecdsa.PrivateKey:
		return LoadECDSASignerVerifier(pk, hashFunc)
	case ed25519.PrivateKey:
		if o.useED25519ph {
			return LoadED25519phSignerVerifier(pk)
		}
		return LoadED25519SignerVerifier(pk)
	}
	return nil, errors.New("unsupported public key type")
}

which avoids having to modify all callers of LoadSignerVerifier.
A call would look like this:

sv, err := LoadSignerVerifier(privateKey, crypto.SHA256, WithED25519ph(), WithRSAPSS(opts))

What do you think? You can have a quick look at it at https://github.com/trail-of-forks/sigstore/tree/ed25519ph-2 and I can use that instead if you like that approach more.

@haydentherapper
Copy link
Contributor

haydentherapper commented Jan 17, 2024

@ret2libc I'm a big fan of that API design, that looks great!

Do you think we can create a second set of functions, LoadVerifierWithOpts, etc? I'd like to avoid breaking changes for now. Or can we have the interface use variadic ... arguments like you proposed? I believe this would avoid the breaking change as long as the variadic parameters come last in the API.

Edit: Sorry misread it was a branch, yes, I like that design a lot!

@ret2libc
Copy link
Contributor Author

@ret2libc I'm a big fan of that API design, that looks great!

I'd like to avoid breaking changes for now. Or can we have the interface use variadic ... arguments like you proposed? I believe this would avoid the breaking change as long as the variadic parameters come last in the API.

I think this is still, in theory, a breaking change as it changes the signature of the API. Probably it won't be noticed in practice, but it depends on how others are using the Signature/Verifier API.

Do you think we can create a second set of functions, LoadVerifierWithOpts, etc?

I can, yes!

@ret2libc
Copy link
Contributor Author

Added WithOpts functions. Ready to review.

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.

a couple nits but overall LGTM

pkg/signature/ed25519ph.go Outdated Show resolved Hide resolved
pkg/signature/ed25519ph.go Outdated Show resolved Hide resolved
pkg/signature/ed25519ph.go Outdated Show resolved Hide resolved
pkg/signature/ed25519ph.go Outdated Show resolved Hide resolved
pkg/signature/signer_test.go Outdated Show resolved Hide resolved
pkg/signature/signerverifier_test.go 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.

Looks great!

pkg/signature/ed25519ph.go Outdated Show resolved Hide resolved
pkg/signature/ed25519ph.go Outdated Show resolved Hide resolved
@ret2libc
Copy link
Contributor Author

I think I've addressed all comments, thanks for the reviews!
Let me know if other changes are required :)

pkg/signature/ed25519ph.go Outdated Show resolved Hide resolved
pkg/signature/options.go Outdated Show resolved Hide resolved
pkg/signature/signerverifier_test.go Outdated Show resolved Hide resolved
pkg/signature/options.go Outdated Show resolved Hide resolved
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.

two nits, otherwise LGTM

pkg/signature/signerverifier_options.go Outdated Show resolved Hide resolved
pkg/signature/signerverifier_test.go Outdated Show resolved Hide resolved
pkg/signature/ed25519ph.go Outdated Show resolved Hide resolved
Signed-off-by: Riccardo Schirone <riccardo.schirone@trailofbits.com>
Before this commit, the Signer/Verifier to load was determined
exclusively by the public/private key type, however there may be
multiple Signers/Verifiers available, like in the case of RSA and
ED25519.

This commit adds LoadVerifierWithOpts, LoadSignerWithOpts, and
LoadSignerVerifierWithOpts to give clients more flexibility, allowing
the user of the API to choose between the available options by using
options.

Signed-off-by: Riccardo Schirone <riccardo.schirone@trailofbits.com>
Signed-off-by: Riccardo Schirone <riccardo.schirone@trailofbits.com>
@bobcallaway
Copy link
Member

please fix the linting issues, otherwise LGTM

Signed-off-by: Riccardo Schirone <riccardo.schirone@trailofbits.com>
@ret2libc
Copy link
Contributor Author

Great! Thanks a lot for the review! Should be good now.

@bobcallaway bobcallaway merged commit 20a2db9 into sigstore:main Jan 29, 2024
9 checks passed
@woodruffw woodruffw deleted the ed25519ph branch January 29, 2024 22:08
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 support for ed25519ph signer/verifier
3 participants