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

Implement EdDSA Signer #605

Merged
merged 2 commits into from Dec 16, 2020
Merged

Implement EdDSA Signer #605

merged 2 commits into from Dec 16, 2020

Conversation

Slamdunk
Copy link
Collaborator

@Slamdunk Slamdunk commented Dec 10, 2020

Hope to see this in 4.1.0 release

@Slamdunk Slamdunk marked this pull request as ready for review December 10, 2020 07:09
@Slamdunk Slamdunk changed the title Implement EDDSA Signer Implement EdDSA Signer Dec 10, 2020
Copy link
Owner

@lcobucci lcobucci left a comment

Choose a reason for hiding this comment

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

@Slamdunk this looks quite good, thanks 👍

I've got a few small requests, though 😬

src/Signer/Eddsa.php Outdated Show resolved Hide resolved
test/functional/EddsaTokenTest.php Outdated Show resolved Hide resolved
test/unit/Signer/EddsaTest.php Show resolved Hide resolved
test/performance/EddsaBench.php Outdated Show resolved Hide resolved
@Slamdunk
Copy link
Collaborator Author

Ed25519 is awesome, among the asymmetric signing is the fastest, and its keys are so easy to store and generate:

+-------------+-----------+
| benchmark   | diff      |   
+-------------+-----------+
| Sha384Bench | 460.14x   |   
| Sha384Bench | 892.40x   |   
| Sha256Bench | 459.90x   |   
| Sha256Bench | 764.18x   |   
| Sha512Bench | 466.50x   |   
| Sha512Bench | 957.14x   |   
| EddsaBench  | 123.00x   |   
| EddsaBench  | 303.83x   |   
| NoneBench   | 1.00x     |   
| NoneBench   | 1.17x     |   
| Sha384Bench | 5,163.65x |
| Sha384Bench | 313.29x   |   
| Sha256Bench | 5,258.72x |
| Sha256Bench | 383.07x   |   
| Sha512Bench | 5,492.08x |
| Sha512Bench | 312.46x   |   
+-------------+-----------+

try {
return sodium_crypto_sign_detached($payload, $key->contents());
} catch (SodiumException $sodiumException) {
throw new InvalidKeyProvided($sodiumException->getMessage(), 0, $sodiumException);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've used InvalidKeyProvided exception but not its factory methods because SodiumException has much wider scope range

Copy link
Owner

Choose a reason for hiding this comment

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

We could introduce a named constructor there but this works too.

@lcobucci
Copy link
Owner

@Slamdunk I'm just wondering about the optional dependency on sodium... Do you have anything against just requiring it?

We added mbstring requirement in the past (which is only there for ECDSA), so the precedent is there.

Apart from that, sodium is pretty stable and works really well - I have it enabled in most of the projects I work.

That would allow for us to remove the checks and move the "breakage" to build time instead of runtime.

I had the idea in the past to break the signers into satellite repositories but that creates more complexity for very little benefit.

What's your take?

@Slamdunk
Copy link
Collaborator Author

I had the idea in the past to break the signers into satellite repositories but that creates more complexity for very little benefit.

Sub-packages are a good idea for non-core features that require external libraries, you don't want to be slowed down in the mainline by a dependency.
However, extensions are always ready when a new PHP version is ready, so you're not slowed down by them, so features that require them can make into main-package.

I'm just wondering about the optional dependency on sodium... Do you have anything against just requiring it?

No: often maintainers prefer PR that do not force anything to users, so I went with a runtime check, since we haven't had a prior discussion.

Here comes the updated push

@lcobucci
Copy link
Owner

@Slamdunk I just rebase to add the GPG signature on commits and tidy the history a bit. Will merge once CI is done.

Thanks a lot!

@lcobucci lcobucci merged commit 7ae299e into lcobucci:4.1.x Dec 16, 2020
@lcobucci lcobucci self-assigned this Dec 16, 2020
@lcobucci lcobucci added this to the 4.1.0 milestone Dec 16, 2020
@Slamdunk Slamdunk deleted the eddsa branch December 16, 2020 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants