Adding JWK support #273

EricTendian · 2020-02-28T02:34:19Z

This PR introduces changes made in the fork https://github.com/fproject/php-jwt to add support for JWKs, so that a fork is no longer needed. I didn't change the code from that repo apart from updating the docblock at the top of the JWK class, and breaking out the JWK tests into a new file JWKTest.php. Some spacing was also added to those tests to make the arrange/act/assert pattern clear.

I haven't done a ton of manual testing on this, but I assume since the fproject fork has been used, this is a working implementation (and the unit tests pass of course).

Fixes #70.

I know it's a bit weird to make a PR for someone else's code, but since there didn't seem to be any movement on #70 for making one I figured I'd start one so there can be a discussion about what further changes need to be made before this feature can be added to this repo.

googlebot · 2020-02-28T02:34:24Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

googlebot · 2020-02-28T02:34:24Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

EricTendian · 2020-02-28T02:36:21Z

^ Also would appreciate guidance on how we should handle the CLA issue.

hparadiz · 2020-02-28T02:55:33Z

Please merge this. I need this too.

bshaffer · 2020-03-09T23:02:21Z

@EricTendian the CLA issue can be fixed by you and @nguyenbs both signing the CLA, since both your emails are on the commit.

bshaffer

This is a great PR! Thank you for your hard work on this!

I've requested some changes, most of them minor.

src/JWK.php

EricTendian · 2020-03-10T18:36:17Z

@bshaffer thank you for the detailed feedback! Really appreciate it. I think I've responded to almost all of it now. I decided to just have support for $source as an array, to limit the amount of type conversion we're doing.

One issue I did run into was in trying to create an encoded message for the test rather than using the one already encoded - it seems that it's not possible to call JWT::encode without the private key, and a JWKS only contains public keys. If you want, I could add a comment above the $msg lines that just indicate what they payload is, if the goal is only to make the tests more readable.

bshaffer

This is looking really great! A few things related to the typehints and a few code standards fixes, and this will be good to go!

I also added an example of how to make the tests readable.

Thanks again for working with me on this!

bshaffer · 2020-03-10T20:42:39Z

src/JWK.php

+     * @param array $source
+     * @return array an associative array represents the set of keys
+     */
+    public static function parseKeySet($source)


We can add a typehint here so we don't have to check if it's an array or throw the exception below:

public static function parseKeySet(array $source)

src/JWK.php

bshaffer · 2020-03-10T21:02:02Z

tests/JWKTest.php

+        $jsKey = '{"keys":[{"kty":"RSA","e":"AQAB","use":"sig","kid":"s1","n":"kWp2zRA23Z3vTL4uoe8kTFptxBVFunIoP4t_8TDYJrOb7D1iZNDXVeEsYKp6ppmrTZDAgd-cNOTKLd4M39WJc5FN0maTAVKJc7NxklDeKc4dMe1BGvTZNG4MpWBo-taKULlYUu0ltYJuLzOjIrTHfarucrGoRWqM0sl3z2-fv9k"}]}';
+        $key = JWK::parseKeySet($jsKey);
+
+        $msg = 'eyJraWQiOiJzMSIsImFsZyI6IlJTMjU2In0.eyJzY3AiOlsib3BlbmlkIiwiZW1haWwiLCJwcm9maWxlIiwiYWFzIl0sInN1YiI6InRVQ1l0bmZJQlBXY3JTSmY0eUJmdk4xa3d3NEtHY3kzTElQazFHVnpzRTAiLCJjbG0iOlsiITV2OEgiXSwiaXNzIjoiaHR0cDpcL1wvMTMwLjIxMS4yNDMuMTE0OjgwODBcL2MyaWQiLCJleHAiOjE0NDExMjY1MzksInVpcCI6eyJncm91cHMiOlsiYWRtaW4iLCJhdWRpdCJdfSwiY2lkIjoicGstb2lkYy0wMSJ9.PvYrnf3k1Z0wgRwCgq0WXKaoIv1hHtzBFO5cGfCs6bl4suc6ilwCWmJqRxGYkU2fNTGyMOt3OUnnBEwl6v5qN6jv7zbkVAVKVvbQLxhHC2nXe3izvoCiVaMEH6hE7VTWwnPbX_qO72mCwTizHTJTZGLOsyXLYM6ctdOMf7sFPTI';


So you don't actually have to sign this every time. You could make this more readable by writing it like this:

$header = array( 'kid' => 's1', 'alg' => 'RS256', ); $payload = array ( 'scp' => array ('openid', 'email', 'profile', 'aas'), 'sub' => 'tUCYtnfIBPWcrSJf4yBfvN1kww4KGcy3LIPk1GVzsE0', 'clm' => array ('!5v8H'), 'iss' => 'http://130.211.243.114:8080/c2id', 'exp' => 1441126539, 'uip' => array('groups' => array('admin', 'audit')), 'cid' => 'pk-oidc-01', ); $signature = 'PvYrnf3k1Z0wgRwCgq0WXKaoIv1hHtzBFO5cGfCs6bl4suc6ilwCWmJqRxGYkU2fNTGyMOt3OUnnBEwl6v5qN6jv7zbkVAVKVvbQLxhHC2nXe3izvoCiVaMEH6hE7VTWwnPbX_qO72mCwTizHTJTZGLOsyXLYM6ctdOMf7sFPTI'; $msg = sprintf('%s.%s.%s', base64_encode(json_encode($header)), base64_encode(json_encode($payload)), $signature );

Also, that payload is huge. You could make it something much smaller, like $payload = array('foo' => 'bar'), but you'll need to add a sample private key + jwk to this repo for testing. I think this is the way to go, but I can take care of that.

EricTendian · 2020-03-10T23:55:28Z

@bshaffer thanks for the additional feedback! With your help I managed to get a token encoded for use in the tests - I did have to modify base64_encode to urlsafeB64Encode since the header had an = when encoded, but apart from that, no issue.

You'll notice I did a bit more refactoring to the JWK class than what you had commented, particularly cleaning up some of the exceptions (and overall error handling) to match the same style as the JWT class. Hopefully that's a welcome change, particularly when debugging.

I initially tried to add type hints to all functions, but since it seems PHP 5.6 doesn't have support for string or int hints, I removed those, so it's just the ones you commented about.

A follow-up question regarding CLAs: it doesn't look like @nguyenbs may have seen this PR when I first made it and doesn't appear to be active on GitHub so may be unaware of the CLA situation. If he doesn't respond in a week or so, should I remove him as a co-author of the commit to fix the CLA bot when squashing these changes? I've now made quite a lot of changes to his original code.

bshaffer · 2020-03-12T21:31:23Z

@EricTendian I cannot merge any code unless all authors have signed the CLA. That's a pretty strict policy. If you do not know @nguyenbs or have no way to contact him for signing the CLA, we cannot use the commits he has made in this PR. I would suggest creating a new PR without any of his changes and we can go from there.

Cvmcosta · 2020-03-18T14:02:20Z

Any update on this? I'm working with a team at Moodle and the only missing part for a big update is this library's support for JWK.

bshaffer · 2020-03-20T00:41:21Z

@Cvmcosta unfortunately until @EricTendian resubmits the PR without @nguyenbs's commits, or until @nguyenbs comments his consent, there's no way I can merge this PR.

googlebot · 2020-03-20T18:41:28Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

googlebot · 2020-03-20T18:41:28Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

EricTendian · 2020-03-20T19:06:45Z

@bshaffer I was able to get a response over email from @nguyenbs a week ago saying I was able to use the code in his fork, but never did hear back since about explicitly signing the CLA. As a result, I have force pushed new history that does not include his commit (and cleans up the history in general).

Travis CI seems to have passed except for this job which errored out so if you can retry that, all the statuses should be green.

Let me know if there's any other changes you want made.

bshaffer

Just one small change left - let's remove the UnsupportedAlgorithmException, and this will be good to go!

bshaffer · 2020-03-21T00:33:26Z

src/JWK.php

+                break;
+        }
+
+        throw new UnsupportedAlgorithmException('Algorithm not supported');


There's no real reason to throw this exception since it's a private method and we catch it anyway.

bshaffer · 2020-03-21T00:33:33Z

src/UnsupportedAlgorithmException.php

+<?php
+namespace Firebase\JWT;
+
+class UnsupportedAlgorithmException extends \UnexpectedValueException


Let's remove this class

src/JWK.php

EricTendian · 2020-03-21T04:57:01Z

@bshaffer thanks, good points - updated that. Looks like Travis is under maintenance when I'm posting this so can't see if CI will pass, but tests pass locally.

bshaffer · 2020-03-21T20:06:42Z

@EricTendian @Cvmcosta

Please test the code in master via composer and I can tag a new version once this feature has baked a bit:

composer require firebase/php-jwt:dev-master

Cvmcosta · 2020-03-23T04:58:55Z

@bshaffer Just tested the new functionalities and everything is working fine. Let me know if u'll be tagging a new version so i can update Moodle's libraries file. Thanks for the great work!

MikeBeas · 2020-03-23T15:17:21Z

I’m also awaiting a tag so we can remove the extra JWT library we were going to use for JWK handling in an upcoming release at my job. Is there a timeframe for how long it usually takes to tag a version?

EricTendian · 2020-03-23T18:00:09Z

@bshaffer I also tested this and it's working well. Thanks for taking care of #283 BTW.

bshaffer · 2020-03-23T19:06:08Z

@MikeBeas I wanted to give the new functionality at least a few days to "bake" in master to catch potential bugs before tagging a release. I will tag 5.2.0 within the next few days.

@EricTendian @Cvmcosta great news! Thank you for testing this!

MikeBeas · 2020-03-23T19:08:56Z

@MikeBeas I wanted to give the new functionality at least a few days to "bake" in master to catch potential bugs before tagging a release. I will tag 5.2.0 within the next few days.

@EricTendian @Cvmcosta great news! Thank you for testing this!

That works perfectly for me, thanks!

bshaffer · 2020-03-25T18:53:10Z

@MikeBeas @EricTendian @Cvmcosta The new release is out and ready to be pulled in via composer!

https://github.com/firebase/php-jwt/releases/tag/v5.2.0

compose require firebase/php-jwt:^5.2

Thanks again for all your help on this!

MikeBeas · 2020-03-25T18:55:52Z

Awesome! Unfortunately my excitement was a bit short-lived when I found out it doesn’t support EC keys at this time. 😞😬

I will keep an eye in case that ever gets added, but unfortunately for the webooks I’m verifying I have to use EC. We are still using this library for all our other JWT needs.

bshaffer · 2020-03-25T19:00:14Z

@MikeBeas This is the next step! Adding this shouldn't be too difficult. I had support for this ,but removed it in 0c693a4 (see ECPublicKey.php)

I've added #286 to track adding this. Please +1 it and we'll see about getting it in sometime soon.

Thanks again!

MikeBeas · 2020-03-25T19:01:06Z

Sounds great! Thanks!

googlebot added the cla: no label Feb 28, 2020

bshaffer requested changes Mar 9, 2020

View reviewed changes

EricTendian requested a review from bshaffer March 10, 2020 18:36

bshaffer reviewed Mar 10, 2020

View reviewed changes

EricTendian requested a review from bshaffer March 11, 2020 00:01

googlebot added cla: yes and removed cla: no labels Mar 20, 2020

bshaffer reviewed Mar 21, 2020

View reviewed changes

feat: Add JWK support

dc09c99

bshaffer mentioned this pull request Mar 21, 2020

feat: Adds JWK set #282

Closed

bshaffer approved these changes Mar 21, 2020

View reviewed changes

bshaffer merged commit b0def5f into firebase:master Mar 21, 2020

bshaffer mentioned this pull request Mar 21, 2020

test: use keys for testing JWK #283

Merged

EricTendian mentioned this pull request Mar 25, 2020

Switching to upstream version of JWT package with JWK classes added 1EdTech/lti-1-3-php-library#44

Closed

kiridaruma mentioned this pull request Jun 5, 2020

update README.md #299

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding JWK support #273

Adding JWK support #273

EricTendian commented Feb 28, 2020

googlebot commented Feb 28, 2020

googlebot commented Feb 28, 2020

EricTendian commented Feb 28, 2020

hparadiz commented Feb 28, 2020

bshaffer commented Mar 9, 2020 •

edited

Loading

bshaffer left a comment

EricTendian commented Mar 10, 2020

bshaffer left a comment

bshaffer Mar 10, 2020

bshaffer Mar 10, 2020 •

edited

Loading

EricTendian commented Mar 10, 2020

bshaffer commented Mar 12, 2020 •

edited

Loading

Cvmcosta commented Mar 18, 2020

bshaffer commented Mar 20, 2020

googlebot commented Mar 20, 2020

googlebot commented Mar 20, 2020

EricTendian commented Mar 20, 2020 •

edited

Loading

bshaffer left a comment

bshaffer Mar 21, 2020

bshaffer Mar 21, 2020

EricTendian commented Mar 21, 2020

bshaffer commented Mar 21, 2020

Cvmcosta commented Mar 23, 2020

MikeBeas commented Mar 23, 2020

EricTendian commented Mar 23, 2020

bshaffer commented Mar 23, 2020

MikeBeas commented Mar 23, 2020

bshaffer commented Mar 25, 2020

MikeBeas commented Mar 25, 2020

bshaffer commented Mar 25, 2020

MikeBeas commented Mar 25, 2020

Adding JWK support #273

Adding JWK support #273

Conversation

EricTendian commented Feb 28, 2020

googlebot commented Feb 28, 2020

googlebot commented Feb 28, 2020

EricTendian commented Feb 28, 2020

hparadiz commented Feb 28, 2020

bshaffer commented Mar 9, 2020 • edited Loading

bshaffer left a comment

Choose a reason for hiding this comment

EricTendian commented Mar 10, 2020

bshaffer left a comment

Choose a reason for hiding this comment

bshaffer Mar 10, 2020

Choose a reason for hiding this comment

bshaffer Mar 10, 2020 • edited Loading

Choose a reason for hiding this comment

EricTendian commented Mar 10, 2020

bshaffer commented Mar 12, 2020 • edited Loading

Cvmcosta commented Mar 18, 2020

bshaffer commented Mar 20, 2020

googlebot commented Mar 20, 2020

googlebot commented Mar 20, 2020

EricTendian commented Mar 20, 2020 • edited Loading

bshaffer left a comment

Choose a reason for hiding this comment

bshaffer Mar 21, 2020

Choose a reason for hiding this comment

bshaffer Mar 21, 2020

Choose a reason for hiding this comment

EricTendian commented Mar 21, 2020

bshaffer commented Mar 21, 2020

Cvmcosta commented Mar 23, 2020

MikeBeas commented Mar 23, 2020

EricTendian commented Mar 23, 2020

bshaffer commented Mar 23, 2020

MikeBeas commented Mar 23, 2020

bshaffer commented Mar 25, 2020

MikeBeas commented Mar 25, 2020

bshaffer commented Mar 25, 2020

MikeBeas commented Mar 25, 2020

bshaffer commented Mar 9, 2020 •

edited

Loading

bshaffer Mar 10, 2020 •

edited

Loading

bshaffer commented Mar 12, 2020 •

edited

Loading

EricTendian commented Mar 20, 2020 •

edited

Loading