RSA Signing & Verification Support #35

liamdennehy · 2018-08-17T22:01:51Z

RSA signing is largely similar to hmac, both requiring a secret (shared in the hmac case), but the difference in key material for signing is significant. Verification is very different, so not trivial to extend.

This PR adds:

Explicitly typing RSA private keys as distinct from secrets (hmac)
Importing keys and certificates into the KeyStore
Detecting whether to use the RSA signing flow depending on the value of algorithm field, and selecting the right hash algorithm (could be replaced on detection of supplied key?)
Detecting whether to use the RSA verification flow depending on the type of key retrieved with keyId and use a X.509 certificate instead of a secret/privateKey.

src/RsaAlgorithm.php

liamdennehy · 2018-08-19T21:22:46Z

OK, that's it. I think this is ready for a thorough review, after handling importing RSA keys & certificates for singing and verification respectively, with a good amount unit tests for all sorts of variations.

src/Algorithm.php

src/Key.php

mtibben · 2018-08-20T00:08:36Z

src/Key.php

+            $this->secret = $item;
+            $publicKey = null;
+            $privateKey = null;
+        };


It looks like you're specifying a very different representation of a key here - if the implementation is so different I'd suggest the concrete types be split out and use a common interface

This took me ages to decide on. I wasn't happy just inventing a new architectural component, and burdening the user with having to understand the various key types with explicit calls. This implementation doesn't break existing flows, and automagically sorts the asymmetric types to handle the sign() and verify() flows (which don't differ in the hmac case).

Acknowledged this is not a complete implementation, but is that preferred over compatibility and user experience?

If we're looking at an overhaul of the Key type and KeyStore, should we incorporate #18 for the KeyStore itself? Or maybe handle the proposed work under that issue in the future and implement this solution for now?

OK so I'd suggest approaching this a little bit differently. Keep the single concrete Key and keep it a simple representation of the Key. Ensure your secret is a string only - mixed use variables are a pain IMO. Remove the public variables but expose a

public function isRSAPrivateKey() public function isX509Certificate()

Then in Verification and *Algorithm put the actual logic

For RSA there is no one "key", there is a signing key (private) and a verification key (public), so we need to handle these. Keep the internal representation but private, expose them via functions as you describe? Oh, and a certificate is just a container for a public key, we should use/expose it directly for consistency.

public function getRSAPublicKey() public function getRSAPrivateKey() public function getHmacSecret()

For EC (in the future) add:

public function getECPublicKey() public function getECPrivateKey()

And with key overhaul to handle rotation in #18 (also future):

public function getRSAPublicKeys($keyId) public function getECPublicKeys($keyId) public function getHmacSecrets($keyId)

Generalised to:

public function getSigningKey() public function getVerifyingKey()

Internal state for a key is now private, and the calls return the right public/private or secret for sign/verify. b9ae276

In future this response can be an array under a single keyId to handle key rotation in #18

@mtibben, can you review the overhaul? I think it's a lot cleaner and tolerant of different input types (e.g. array of possible verification keys or just a single key).
b9ae276

Resolves 99designs#35 (comment)

src/Context.php

src/Signature.php

liamdennehy · 2018-08-29T11:15:07Z

Any further comments here?

mtibben · 2018-10-02T03:40:15Z

Can you rebase and remove the merge commits?

src/Context.php

src/Key.php

src/RsaAlgorithm.php

src/Verification.php

mtibben · 2018-10-04T02:10:23Z

Hey @liamdennehy I'm not sure you're following my request regarding the rebase, so will try be more clear :) We follow a "feature branch" or "Github Flow" workflow. So:

We don't want merge commits. Rebase them out
We want your branch to be from the most recent master commit. Rebase on master

liamdennehy · 2018-10-04T07:44:37Z

@mtibben There's a strong possibility I have no idea what I'm doing ☺️
Thanks for the clarification.

Resolves 99designs#35 (comment)

* Specification does not permit other hash algorithms for RSA signatures

src/Context.php

mtibben · 2018-10-09T08:47:03Z

src/Context.php

@@ -34,9 +37,8 @@ public function __construct($args)

        // algorithm for signing; not necessary for verifying.
        if (isset($args['algorithm'])) {
-            $this->algorithmName = $args['algorithm'];
+            $this->algorithm = Algorithm::create($args['algorithm']);


Any reason for this change?

This old parameter only handled the HMAC case, where a signing and verifying key are the same. Needed a class to discriminate HMAC from RSA, so invoke with a factory that makes the right underlying Algorithm subclass.

@mtibben Still new to OO, is this a good approach?

src/Key.php

mtibben · 2018-10-09T08:54:07Z

src/Key.php

+
+    private function getRSAPrivateKey($object)
+    {
+        if (is_array($object)) {


Under what circumstances is this going to be an array and why? Ideally avoid allowing multiple types in arguments

The user either supplies:

A Public Key/Certificate for verifying as string/object

A Private Key for signing as string/object

Or both together in an array of string/objects - this can now be used for both signing and verifying without having to construct different objects for each case.

Allowing both forms means we can use the same KeyStore object for all three cases to derive a Key object. Alternative is to expose HMAC and Asymmetric-specific Key interfaces which is both a BC and requires more documentation and user education for little gain.

@mtibben Is this ok? I realise the input here is fungible, but I'd rather that than expect the user to do key/certificate handling to a more advanced degree.

src/Key.php

liamdennehy · 2018-10-09T13:46:30Z

@mtibben I'm going to overhaul the Key class to make the flow easier to track, eg

    static function hasX509Certificate($object)

...to explicitly test if an object has a certificate instead of try..catch blocks all over the place.

This will also make a good base to extend the class for #18 (Key Rotation). Will ping you when it's ready.

* Key now handles inputs as X.509, naked Public Key and Private Key (all PEM) * Improved & clearer detection logic with explicit tests for asymmetric key material, falling back on shared secret for HMAC if asymmetric keys are not detected hasX509Certificate() hasPublicKey() hasPrivateKey() * Clearer names for test constants (easier to expand to EC later)

liamdennehy · 2018-10-09T20:27:26Z

@mtibben Hopefully much clearer Key class in b9ae276, and quicker than I thought.

liamdennehy · 2019-03-06T14:22:09Z

Any further questions, or updates on open conversations in this PR?

lahaxearnaud · 2019-07-17T15:55:14Z

Hello,

Can you please merge and tag this PR ?

Regards,

Arnaud

Resolves 99designs#35 (comment)

liamdennehy · 2019-08-02T14:08:02Z

Apologies, accidentally deleted the branch for this PR in my source.

However, I have integrated this feature in my own project along with a number of others, and published this in packagist. Documentation for the entire library is published at Read the Docs: http-signatures-php - incomplete but being expanded regularly.

- Remove unsupported Symfony/PHP version combinations - Add PHP 7.3 support

liamdennehy · 2019-08-15T13:25:13Z

Closing PR due to inactivity.

liamdennehy mentioned this pull request Aug 17, 2018

Support RSA signing #34

Open

liamdennehy changed the title ~~RSA Signatures support, for all SHA2 variants~~ RSA Signing Support Aug 18, 2018

aaronpk reviewed Aug 18, 2018

View reviewed changes

src/RsaAlgorithm.php Outdated Show resolved Hide resolved

liamdennehy changed the title ~~RSA Signing Support~~ RSA Signing & Verification Support Aug 19, 2018

mtibben reviewed Aug 20, 2018

View reviewed changes

src/Algorithm.php Outdated Show resolved Hide resolved

mtibben reviewed Aug 20, 2018

View reviewed changes

src/Algorithm.php Show resolved Hide resolved

mtibben reviewed Aug 20, 2018

View reviewed changes

src/Key.php Outdated Show resolved Hide resolved

mtibben reviewed Aug 20, 2018

View reviewed changes

liamdennehy added a commit to liamdennehy/http-signatures-php that referenced this pull request Aug 20, 2018

Changed static to $this methods for RSA key functions.

c2b1160

Resolves 99designs#35 (comment)

mtibben reviewed Aug 20, 2018

View reviewed changes

src/Context.php Outdated Show resolved Hide resolved

mtibben reviewed Aug 20, 2018

View reviewed changes

src/Signature.php Outdated Show resolved Hide resolved

mtibben reviewed Oct 2, 2018

View reviewed changes

src/Context.php Outdated Show resolved Hide resolved

mtibben reviewed Oct 2, 2018

View reviewed changes

src/Key.php Outdated Show resolved Hide resolved

mtibben reviewed Oct 2, 2018

View reviewed changes

src/Key.php Outdated Show resolved Hide resolved

mtibben reviewed Oct 2, 2018

View reviewed changes

src/Key.php Outdated Show resolved Hide resolved

mtibben reviewed Oct 2, 2018

View reviewed changes

src/RsaAlgorithm.php Outdated Show resolved Hide resolved

mtibben reviewed Oct 2, 2018

View reviewed changes

src/Verification.php Outdated Show resolved Hide resolved

liamdennehy added a commit to liamdennehy/http-signatures-php that referenced this pull request Oct 4, 2018

Changed static to $this methods for RSA key functions.

9e075bd

Resolves 99designs#35 (comment)

liamdennehy added 7 commits October 4, 2018 16:23

RSA Signatures support, for all SHA2 variants

0c417a0

Switch for different SHA2 hash lengths

189c3ea

Only allow rsa-sha1 and rsa-256

07b9967

* Specification does not permit other hash algorithms for RSA signatures

Restrict RSA algorithm selection to those in the standard

473cbad

RSA Context tests and rename default to differentiate HMAC

dc69698

Resolve PHP CS Fixer issues

14e3882

Add RSA to SignatureParametersTest

15b6da9

Typos & Code Style

230b033

liamdennehy force-pushed the rsa branch from 37709ac to 230b033 Compare October 7, 2018 08:05

mtibben reviewed Oct 9, 2018

View reviewed changes

src/Context.php Outdated Show resolved Hide resolved

mtibben reviewed Oct 9, 2018

View reviewed changes

src/Key.php Outdated Show resolved Hide resolved

mtibben reviewed Oct 9, 2018

View reviewed changes

src/Key.php Outdated Show resolved Hide resolved

mtibben reviewed Oct 9, 2018

View reviewed changes

src/Key.php Show resolved Hide resolved

mtibben reviewed Oct 9, 2018

View reviewed changes

src/Key.php Outdated Show resolved Hide resolved

Private variable, remove @throws hints

93c0c45

liamdennehy force-pushed the rsa branch from ecd444d to 99fc46d Compare October 9, 2018 19:16

liamdennehy force-pushed the rsa branch from 99fc46d to b9ae276 Compare October 9, 2018 20:24

phpdoc annotations

67c3d32

liamdennehy force-pushed the rsa branch from c80799e to 67c3d32 Compare October 9, 2018 20:52

liamdennehy mentioned this pull request Oct 11, 2018

Cannot omit "headers" as required by specification #43

Closed

liamdennehy added a commit to liamdennehy/http-signatures-php that referenced this pull request Jul 30, 2019

Changed static to $this methods for RSA key functions.

6cca551

Resolves 99designs#35 (comment)

liamdennehy closed this Jul 31, 2019

liamdennehy deleted the rsa branch July 31, 2019 14:28

liamdennehy restored the rsa branch August 2, 2019 14:05

liamdennehy reopened this Aug 2, 2019

Travis build fixes and include phpunit script

3ee2ea5

- Remove unsupported Symfony/PHP version combinations - Add PHP 7.3 support

liamdennehy closed this Aug 15, 2019

liamdennehy deleted the rsa branch August 15, 2019 13:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RSA Signing & Verification Support #35

RSA Signing & Verification Support #35

liamdennehy commented Aug 17, 2018 •

edited

Loading

liamdennehy commented Aug 19, 2018

mtibben Aug 20, 2018 •

edited

Loading

liamdennehy Aug 20, 2018

liamdennehy Aug 20, 2018

mtibben Aug 20, 2018 •

edited

Loading

liamdennehy Aug 21, 2018

liamdennehy Aug 22, 2018 •

edited

Loading

liamdennehy Nov 21, 2018

liamdennehy commented Aug 29, 2018

mtibben commented Oct 2, 2018

mtibben commented Oct 4, 2018

liamdennehy commented Oct 4, 2018

mtibben Oct 9, 2018

liamdennehy Oct 9, 2018 •

edited

Loading

liamdennehy Nov 21, 2018

mtibben Oct 9, 2018

liamdennehy Oct 9, 2018

liamdennehy Nov 21, 2018

liamdennehy commented Oct 9, 2018

liamdennehy commented Oct 9, 2018

liamdennehy commented Mar 6, 2019

lahaxearnaud commented Jul 17, 2019

liamdennehy commented Aug 2, 2019 •

edited

Loading

liamdennehy commented Aug 15, 2019

RSA Signing & Verification Support #35

RSA Signing & Verification Support #35

Conversation

liamdennehy commented Aug 17, 2018 • edited Loading

liamdennehy commented Aug 19, 2018

mtibben Aug 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtibben Aug 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamdennehy Aug 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamdennehy commented Aug 29, 2018

mtibben commented Oct 2, 2018

mtibben commented Oct 4, 2018

liamdennehy commented Oct 4, 2018

Choose a reason for hiding this comment

liamdennehy Oct 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamdennehy commented Oct 9, 2018

liamdennehy commented Oct 9, 2018

liamdennehy commented Mar 6, 2019

lahaxearnaud commented Jul 17, 2019

liamdennehy commented Aug 2, 2019 • edited Loading

liamdennehy commented Aug 15, 2019

liamdennehy commented Aug 17, 2018 •

edited

Loading

mtibben Aug 20, 2018 •

edited

Loading

mtibben Aug 20, 2018 •

edited

Loading

liamdennehy Aug 22, 2018 •

edited

Loading

liamdennehy Oct 9, 2018 •

edited

Loading

liamdennehy commented Aug 2, 2019 •

edited

Loading