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

Compressing logic for verification #28

Merged
merged 1 commit into from
Jan 14, 2016
Merged

Compressing logic for verification #28

merged 1 commit into from
Jan 14, 2016

Conversation

jumbojett
Copy link
Owner

@cicalese
@alexedelsburg
@zmon

I compressed some of the logic from your individual pull requests that were submitted. I know several of you are working with various OIDC providers. Would you be kind enough to verify this patch on your provider?

@zmon
Copy link
Contributor

zmon commented Apr 29, 2015

Google Worked!

SalesForce failed - Fatal error: Uncaught exception 'OpenIDConnectClientException' with message 'Unable to verify signature' in /var/www/openid-connect-php.savagesoft.com/web/OpenIDConnectClient.php5:207 Stack trace: #0 /var/www/openid-connect-php.savagesoft.com/web/index.php(46): OpenIDConnectClient->authenticate() #1 {main} thrown in /var/www/openid-connect-php.savagesoft.com/web/OpenIDConnectClient.php5 on line 207

Looks like it is somewhere down in verifyRSAJWTsignature(), sorry that is the best I can do at this time.

@@ -403,28 +403,14 @@ class OpenIDConnectClient
*/
private function get_key_for_header($keys, $header) {
foreach ($keys as $key) {
if ($key->alg == $header->alg && $key->kid == $header->kid) {
if ((isset($header->kid) && $key->alg == $header->alg && $key->kid == $header->kid)
|| ($key->kty == 'RSA')) {
return $key;
}
}
throw new OpenIDConnectClientException('Unable to find a key for (algorithm, kid):' . $header->alg . ', ' . $header->kid . ')');
Copy link
Collaborator

Choose a reason for hiding this comment

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

This patch works great for me, but it needs one final enhancement. Please change line 411 from:

     throw new OpenIDConnectClientException('Unable to find a key for (algorithm, kid):' . $header->alg . ', ' . $header->kid . ')');

to

     if (isset($header->kid)) {
         throw new OpenIDConnectClientException('Unable to find a key for (algorithm, kid):' . $header->alg . ', ' . $header->kid . ')');
     } else {
         throw new OpenIDConnectClientException('Unable to find a key for RSA');
     }

Thanks!

@cicalese
Copy link
Collaborator

cicalese commented May 1, 2015

I am intermittently getting the same exception described by @zmon above, but I'm using Google. I've traced through the code, and it is coming from the test at line 444 ($rsa->verify($payload, $signature)). I have repeated the authentication attempt with the same credentials over and over (repeatedly clicking my login link); sometimes the test returns true, and sometimes it returns false. When it returns false, the exception shown by @zmon above is thrown. What would cause the signature verification to fail intermittently for the same credentials?
When I remove the phpseclib library, all works fine, as it is not traversing the signature verification code. But, I get an unisghtly error in the Apache log. Should signature verification be optional, and, if so, could there be a flag that would prevent the attempt to include the phpseclib library if signature verification is not desired?

@bpteague
Copy link

Hi - I can add another provider to the list. I'm authenticating on MIT's OpenID provider, which is running MITREid Connect: https://github.com/mitreid-connect/ I am having the same difficulty as @cicalese - the JWT signature doesn't verify. And of course, the JWK JSON is a little different; you can find it at https://oidc.mit.edu/jwk .

@bpteague
Copy link

@cicalese I think there are two bugs interacting here. The first bug is in key selection (the logic that's the subject of this pull request). When I try to authenticate with Google, it fails initially because Google has two public keys listed; their JWK JSON is structured such that ($key->kty == $alg) always returns TRUE, which means the first key is always used even if the response was signed with the second key.

I fiddled with the logic to make it pick the right key, but even when it's using the right key $rsa->verify() doesn't return TRUE. As I mentioned in #35, I can verify the signature with the online tool at http://jwt.io, so something is working; but I'm not enough of a crypto nerd to push any further.

@stgnet
Copy link

stgnet commented Jun 10, 2015

I just tried the jumbojett-patch-1 branch, and I'm still getting the error:

Uncaught exception 'OpenIDConnectClientException' with message 'Unable to verify signature'

Compatibility with IdentityServer3
@jumbojett jumbojett merged commit f3f5226 into master Jan 14, 2016
@DeepDiver1975 DeepDiver1975 deleted the jumbojett-patch-1 branch July 5, 2022 10:20
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.

5 participants