Skip to content

Implement EdDSA Signer#605

Merged
lcobucci merged 2 commits into
lcobucci:4.1.xfrom
Slamdunk:eddsa
Dec 16, 2020
Merged

Implement EdDSA Signer#605
lcobucci merged 2 commits into
lcobucci:4.1.xfrom
Slamdunk:eddsa

Conversation

@Slamdunk

@Slamdunk Slamdunk commented Dec 10, 2020

Copy link
Copy Markdown
Collaborator

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

@lcobucci lcobucci left a comment

Copy link
Copy Markdown
Owner

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 😬

Comment thread src/Signer/Eddsa.php Outdated
Comment thread test/functional/EddsaTokenTest.php Outdated
Comment thread test/unit/Signer/EddsaTest.php
Comment thread test/performance/EddsaBench.php Outdated
@Slamdunk

Copy link
Copy Markdown
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   |   
+-------------+-----------+

Comment thread src/Signer/Eddsa.php
try {
return sodium_crypto_sign_detached($payload, $key->contents());
} catch (SodiumException $sodiumException) {
throw new InvalidKeyProvided($sodiumException->getMessage(), 0, $sodiumException);

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

2 participants