RSA-based and SHA-256-involving signature methods #194
Conversation
- added support for the following signature methods: RSA-SHA1, RSA-SHA256, HMAC-SHA256 (the third and second methods are non-standard extensions)
tmhOAuth.php
Outdated
| * @return binary signature | ||
| */ | ||
| private function sign_with_rsa($algorithm) { | ||
| openssl_sign($this->request_settings['basestring'], $signature, $this->config['private_key_pem'], $algorithm); |
There was a problem hiding this comment.
thinking this might need a function_exists check on openssl_sign.
There was a problem hiding this comment.
Added a check for whether 'openssl_sign' function exists. If not, an exception is thrown.
tmhOAuth.php
Outdated
| $signature = $this->sign_with_rsa('sha1'); | ||
| break; | ||
| case 'RSA-SHA256': | ||
| $signature = $this->sign_with_rsa('sha256'); |
There was a problem hiding this comment.
i know 'sha256' is supported but what about using OPENSSL_ALGO_SHA256 instead?
There was a problem hiding this comment.
Replaced 'sha256' with OPENSSL_ALGO_SHA256
tmhOAuth.php
Outdated
| $signature = $this->sign_with_hmac('sha256'); | ||
| break; | ||
| case 'RSA-SHA1': | ||
| $signature = $this->sign_with_rsa('sha1'); |
There was a problem hiding this comment.
i know 'sha1' is supported but what about using OPENSSL_ALGO_SHA1 instead?
There was a problem hiding this comment.
Replaced 'sha1' with OPENSSL_ALGO_SHA1
tmhOAuth.php
Outdated
| @@ -45,6 +45,9 @@ public function reconfigure($config=array()) { | |||
| 'token' => '', | |||
| 'secret' => '', | |||
|
|
|||
| // RSA private key (for RSA-SHA1 and RSA-SHA256 methods) | |||
| 'private_key_pem' => '', | |||
There was a problem hiding this comment.
openssl_sign is a messy function as it overloads the types that can be used for the key and algorithm variables.
it's possible to send a resource instead of a string as the private_key_pem but the implementation you are using assumes it's string. wanna call that out in the comment - that if used this is expecting a string representation of a private key and not a file resource?
tmhOAuth.php
Outdated
| * @return binary signature | ||
| */ | ||
| private function sign_with_rsa($algorithm) { | ||
| openssl_sign($this->request_settings['basestring'], $signature, $this->config['private_key_pem'], $algorithm); |
There was a problem hiding this comment.
thanks for contributing this, love it. just a couple of thoughts on improvements before a pull it in.
also - wanna bump the version number (0.8.5) and date?
… to be used with RSA signature computation
… a string containing a PEM-formatted key
|
Thanks for your review! I'm currently working on implementing the changes you suggested. |
- version is set to 0.8.5 - date is set to 09 Jun 2017
|
I've made the changes you suggested. Also, openssl_sign error handling is added. Could you please review this change specifically? I'm not very familiar with Openssl extension in PHP. |
|
Hi Matt! Is there anything missing from my side on this pull request? |
|
@themattharris just bumping |
RSA-SHA1 is a standard signature method defined by OAuth 1.0a. It is nice to have it implemented.
As for RSA-SHA256, it is non-standard, but now, when SHA1 is compromised in practice, RSA-SHA1 does not seem to provide enough security. Hence RSA-SHA256 may be useful for clients and servers that will to use asymmetric signatures and not suffer from a weak signature algorithm.
HMAC-SHA1 does not seem to be compromised, but HMAC-SHA256 (again, a non-standard signature method) support was added for completeness.