Some commit broke IdentityServer3 compatibility

[svrooij]

This commit broke the ability to use IdentityServer

Could you explain why this code checks for isset($header->kid) ?

IdentityServer doesn't provide the alg parameter in their .well-known/jkws

[jumbojett]

Hi @svrooij can you checkout the jumbojett-patch-1 branch and let me know if it fixes the issue?

[svrooij]

Will check later today, but for what I can see that will solve the issue.

[svrooij]

With this patch it generates a notice Notice: Undefined property: stdClass::$alg in /home/i872953/domains/i872953.iris.fhict.nl/public_html/openid/OpenIDConnectClient.php5 on line 409

If I change line 406/407 to

if (($key->kty == 'RSA') || (isset($header->kid) && $key->alg == $header->alg && $key->kid == $header->kid)) {

It works like a charm

[cicalese]

Unfortunately, although I'm not an expert in the protocol, I think this solution could cause problems for other providers (but I do think there is an alternate solution that will work for both). If you look at the Google provider metadata at https://www.googleapis.com/oauth2/v3/certs, you'll see that there are two keys with $key->kty of 'RSA'. With the code above, it would always return the first key in the array. I believe, if the more detailed information ($header->alg and $header->kid) is available, it should be used to disambiguate, and $key->kty should only be used if one of those $header properties is not set. So, maybe something like the following would work:

if ((!(isset($header->alg) && isset($header->kid)) && $key->kty == 'RSA') || ($key->alg == $header->alg && $key->kid == $header->kid)) {

[cicalese]

Also (again, not being an expert in the protocol), I wonder why $header->alg is not set, since it appears that it is a required header parameter (see https://tools.ietf.org/html/draft-ietf-jose-json-web-signature-41#section-4.1.1), while $header->kid is optional.

[svrooij]

It is not set in the $key

[cicalese]

Ah, then my fix above won't catch it, since I look for it being set in $header.

[cicalese]

So, the check would have to be something like:

if ((!(isset($key->alg) && isset($key->kid) && isset($header->kid)) && $key->kty == 'RSA') || ($key->alg == $header->alg && $key->kid == $header->kid)) {

but that isn't very pretty. It doesn't need to check for $header->alg, since it is a required header parameter and gets checked in the switch statement outside the current function. I haven't had a chance to test this yet, though, and I won't have a chance to until this evening.

[svrooij]

I seem to have fixed it. https://github.com/svrooij/OpenID-Connect-PHP/commit/a894424f7e6090e558100b95a520c31d2cc6c078

[ghost]

@svrooij Hello, how did you manage to connect to identify server?

[svrooij]

Not sure. My last comment was 5 years ago, when there was a need to do OpenID Connect on some php site. Since then I no longer work with php (when possible).

I cannot help you any further.

[ghost]

@svrooij Oh thanks, sometimes I have to work with php while my expertise on Python .d thanks for quick reply

[svrooij]

From what I can read above it did some checking on the wrong values (or identity server didn't supply the right values).

It is just a matter of turning on logging and finding the right combination. Keep in mind that identity server changed a lot in the past 5 years.