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

fix: use PKCS8 by default for ID token verify #466

Conversation

marickvantuil
Copy link
Contributor

This is (hopefully) a fix for #457.

It happens specifically with phpseclib 3.x.


When using the AccessToken class to decode an ID token, the "Federated Sign On" certificates are stored in the PKCS8 format after which the ID token is decoded with Firebase/JWT:

$payload = $this->callJwtStatic('decode', [
    $token,
    $keys,
]);

Source: AccessToken.php#L246

Part of decoding the JWT is verifying the token which happens with openssl_verify:

$success = \openssl_verify($msg, $signature, $keyMaterial, $algorithm);

Source: JWT.php#L306

I have to be honest that I am completely lacking any knowledge about RSA keys and their encoding, but according to this StackOverflow issue PHP OpenSSL only accepts "public key in X.509 style". I can confirm this as I am getting the error "Supplied key param cannot be coerced into a public key" (same as in the linked issue at the start of this comment). After returning the key in PKCS8 format, the issue was fixed.

I understand my assessment of the issue might be completely wrong but I really like to help resolve this issue! 👍

Copy link
Contributor

@bshaffer bshaffer left a comment

Choose a reason for hiding this comment

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

So, this has to do with the format in which the key is passed to openssl_verify. I suppose it's possible that your implementation doesn't support PKCS1? I find that strange, but I am not sure what the implications are across all our users and platforms.

the "Federated Sign On" certificates are stored in the PKCS8 format

This is a bit misleading - the certs aren't stored in this format, but they are converted to PKCS1 format from the oauth2 certs URL for openssl.

I don't want to just change the format being returned, because it could have a negative affect on some platforms. A better solution would be to:

  1. detect the version of OpenSSL and/or whether or not PKCS1 and PKCS8 are supported, and use the best one
  2. add a $format option to verify to allow modification (not preferred)
  3. confirm with 100% confidence that this update would not break (I doubt this is possible)

@marickvantuil
Copy link
Contributor Author

marickvantuil commented Aug 5, 2023

Thanks for your reply @bshaffer!

I suppose it's possible that your implementation doesn't support PKCS1

After some more investigation, I found it throws this error when OpenSSL 1.1.1 is used. When I bump the PHP version (in my case from 8.1.12 to 8.1.13 using php:8.1.12-cli-alpine Docker image), OpenSSL is updated to 3.x and then it seems to work fine.

So I'm not sure what the implications are! As far as I understand it then, something has changed in OpenSSL 3.x which fixes this issue but some other PHP 8.1 builds (depending on how it was built) ship with 1.1.1 in which this error happens. Hope I'm right here!

Not sure if it helps, but I set up a reproducable example here.

Output:

Screenshot 2023-08-05 at 18 11 07

@bshaffer bshaffer changed the title fix: decode open ID token with phpseclib3 fix: use PKCS8 by default for ID token verify Aug 23, 2023
@bshaffer bshaffer enabled auto-merge (squash) August 23, 2023 08:41
@bshaffer bshaffer merged commit 0c3a1be into googleapis:main Aug 23, 2023
10 checks passed
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.

None yet

2 participants