JWK must contain an "alg" parameter (Moodle 4.3.7+)

JWK must contain an "alg" parameter (Moodle 4.3.7+)

by Tom Doesburg -
Number of replies: 16

This bug was apparently also found in older 4.x version (https://moodle.org/mod/forum/discuss.php?d=443389) and was supposed to be fixed in https://tracker.moodle.org/browse/MDL-77077 but im seeing this still when testing with Moodle 4.3.7+. I cloned the MOODLE_403_STABLE branch.

My JWKS come from an Azure Key Vault and technically the key is not bound to a single algorithm. (RS256 or RS512, work both) so if i can prevent it I would not want to add the alg to the JWK.

Is this something that can be fixed again? 

Below an example of a key from my keyset url and the stacktrace in moodle
image%20%281%29.png

image.png

Average of ratings: -
In reply to Tom Doesburg

Re: JWK must contain an "alg" parameter (Moodle 4.3.7+)

by Jake Dallimore -
Picture of Core developers Picture of Moodle HQ Picture of Particularly helpful Moodlers Picture of Peer reviewers Picture of Plugin developers Picture of Testers
Hi Tom,

I've just checked and the original issue is still being fixed by the current patch. I can step through the code on 4.3.7+ here and see it setting the missing alg on the key, as it runs the code inside fix_jwks_alg(), at least with the sites we test with (per that MDL). So, at the very least, we know that it can work. You may need to debug and step through the content item return from your tool. There are paths which will skip the key fix, such as if the kid in the JWT header doesn't match the kid in the key. Basically, I think we're going to need to dig into this and find out exactly why the fix isn't working for your specific key/launch. If you could provide any further information, e.g. the JWT headers or any information you glean from debugging further, that would be helpful.

Many thanks
Jake
Average of ratings: -
In reply to Jake Dallimore

Re: JWK must contain an "alg" parameter (Moodle 4.3.7+)

by Tom Doesburg -
Hi Jake, 

Thanks for the quick response, I checked the implementation in Moodle (im not a php dev) but I couldn't figure out at first glance what could cause this.

Here an example of a header with the corresponding key on jwks endpoint. As you can see the the kid matches. Note that this key is one of multiple that are given back as an array. So maybe check if it can handle the array of jwks correctly? Our LTI implementation has been live in an brightspace setting for years so I'm pretty confident that the code is sound at its core.

{
      "kty": "RSA",
      "e": "AQAB",
      "kid": "https://slimstampen-vault-dev.vault.azure.net/keys/lti-signing-key/0edc5c112e024c5598203201e30cd3d5",
      "key_ops": [
        "sign",
        "verify",
        "wrapKey",
        "unwrapKey",
        "encrypt",
        "decrypt"
      ],
      "n": "x0d5B1dp8GgZ7mGvEdIeiJK3EKaAxinwKFlAQy_DpcNDnDfjfS3bIUmAfM6dQcGsLg9GBo2eTGkYe0iNHE68wtMbkZvAEUUiB3cHhkw2QVZYE005vbrqeNKlJPgXVWf78XScEt3Nsg6LarEkWaawL1KlsP0vDnmxu-rN-iORB9HXWoPEAf83uoq6H83RNmY3DXefI4uJNDE1SP5_fv1wmeCUwj8HylLABbDsatZBLIlRAQ-UT-QZSsisv-JI6U5cWk5oRb8Jo6ap6OnH3KrNTX6RbW2ylwk2tQOPsfQbEUmhLpNy0KZBiXjSfxQvnPUGtSphNMdrm0iFA8KlrWGWKQ"
    },

image.png 
Average of ratings: -
In reply to Tom Doesburg

Re: JWK must contain an "alg" parameter (Moodle 4.3.7+)

by Jake Dallimore -
Picture of Core developers Picture of Moodle HQ Picture of Particularly helpful Moodlers Picture of Peer reviewers Picture of Plugin developers Picture of Testers
Thanks Tom. I think that's almost ruled that one out then. Was worth a shot. I'll have to try to take what you've got above and step through it locally when I get a few mins spare (currently one week out from major release, so time poor). Hopefully, it'll be something obvious then and we can get started on a fix.

Any chance you can share the public JWKS URL here? Would be great to pipe in the exact same data you're using.

Cheers
Average of ratings: -
In reply to Jake Dallimore

Re: JWK must contain an "alg" parameter (Moodle 4.3.7+)

by Tom Doesburg -
Any luck finding some time to look into this? Deeplinking is currently the only thing that doesn't work in Moodle, so it would be great if we could resolve this.
Average of ratings: -
In reply to Tom Doesburg

Re: JWK must contain an "alg" parameter (Moodle 4.3.7+)

by Liam Moran -
Picture of Core developers
I can't figure out why it wasn't failing the unit tests, but is it that fix_jwks_keys is only adding the alg parameter to the key with the matching kid and skipping the others in the jwks' keys array, while parseKeySet is calling parseKey on all of them? The keys that didn't have alg added are throwing the reported exception.

In any case, when php-jwt was upgraded to 6.4.0, the call to parseKeySet in the exception block could have made use of that function's newly added second parameter of $defaultAlg (that wasn't available in Moodle until 2 months after MDL-77077 was patched). The discussion around the upstream pull request for this parameter and later asking why alg is required is consistent with this as the intent of that parameter. A function like fix_jwks_keys could simply return the header alg and use that if it's supplied or else fall back on 'RS256' as the presumed php-jwt default algorithm as per the comment in locallib.php when calling parseKeySet added when that library was upgraded to 6.0.0. 
 
Putting that new parameter to use would fix Tom's problem in this instance, I'm pretty sure.

To verify, on your test environment, change this line:
$keys = JWK::parseKeySet($keysetarr);
to:
$keys = JWK::parseKeySet($keysetarr,'RS256');
Average of ratings: -
In reply to Liam Moran

Re: JWK must contain an "alg" parameter (Moodle 4.3.7+)

by Jake Dallimore -
Picture of Core developers Picture of Moodle HQ Picture of Particularly helpful Moodlers Picture of Peer reviewers Picture of Plugin developers Picture of Testers
See my other comment here. Yes, that's the cause, but that solution isn't one I'm overly fond of.
Average of ratings: -
In reply to Tom Doesburg

Re: JWK must contain an "alg" parameter (Moodle 4.3.7+)

by Jake Dallimore -
Picture of Core developers Picture of Moodle HQ Picture of Particularly helpful Moodlers Picture of Peer reviewers Picture of Plugin developers Picture of Testers
Thanks for the bump. I did indeed, this morning.

It's a case of a missing alg, post-fixing, for only those keys not matching the kid in the JWT header. You can see this exact case being verified in the test data provider, however, since there's no coverage of the actual decoding (along with a lot of stuff in mod_lti, frankly), we missed this one. 

I've put up a patch for this at MDL-83423. At this point in the code, we know that we're only going to decode using one matched key (based on kid in JWT header), and either:
- that key has an alg in the JWKS (not fixed) or;
- that key didn't have an alg in the JWKS (and we fixed it).
All other keys can just be dropped at this point in time since they won't be used during decode anyway.

Note also: 4.3.x is now security support only, so won't get any fix. It's a candidate for 4.4, 4.5 and main only.

It's also worth noting that while we're migrating a huge amount of our mod_lti code to a core subsystem, we're only accepting very small numbers of changes to mod_lti right now (read: basically a mod_lti specific code freeze). I've marked this one major, however, and will raise getting it into the upcoming minors, given the impact. 
Average of ratings:Useful (1)
In reply to Jake Dallimore

Re: JWK must contain an "alg" parameter (Moodle 4.3.7+)

by Tom Doesburg -
Thanks for looking into this, glad my lti integration can be left untouched wink Ill upgrade my development version and wait for the patch.
Average of ratings: -
In reply to Jake Dallimore

Re: JWK must contain an "alg" parameter (Moodle 4.3.7+)

by Liam Moran -
Picture of Core developers
Filtering out the unused keys is a good strategy.

I checked the jwt RFCs for guidance on whether an informative error should be thrown if the jwt doesn't include alg signing metadata either and conclude "Not a plausible error", and also note this section from IETF's RFC-8725 that supports your reluctance to fall back onto a default algorithm.
Average of ratings: -
In reply to Jake Dallimore

Re: JWK must contain an "alg" parameter (Moodle 4.3.7+)

by Tom Doesburg -
Can you give me a heads up when I can try it in 4.4 or 4.5?
Average of ratings: -
In reply to Tom Doesburg

Re: JWK must contain an "alg" parameter (Moodle 4.3.7+)

by Jake Dallimore -
Picture of Core developers Picture of Moodle HQ Picture of Particularly helpful Moodlers Picture of Peer reviewers Picture of Plugin developers Picture of Testers
Our next minors are due to be released on Monday December 9th, 2024. This fix will be included there.
Average of ratings: -
In reply to Jake Dallimore

Re: JWK must contain an "alg" parameter (Moodle 4.3.7+)

by Tom Doesburg -

Is there anyway to get access to this earlier, because this is almost 2 months away..

Average of ratings: -
In reply to Tom Doesburg

Re: JWK must contain an "alg" parameter (Moodle 4.3.7+)

by Jake Dallimore -
Picture of Core developers Picture of Moodle HQ Picture of Particularly helpful Moodlers Picture of Peer reviewers Picture of Plugin developers Picture of Testers
You'll need to pull the patch and apply it to your codebase. You can see the code linked in the issue - MDL-83423
Average of ratings: -
In reply to Jake Dallimore

Re: JWK must contain an "alg" parameter (Moodle 4.3.7+)

by Tom Doesburg -

Thanks, I can now confirm that this fixes my issue

Average of ratings: -
In reply to Tom Doesburg

Re: JWK must contain an "alg" parameter (Moodle 4.3.7+)

by Jake Dallimore -
Picture of Core developers Picture of Moodle HQ Picture of Particularly helpful Moodlers Picture of Peer reviewers Picture of Plugin developers Picture of Testers
Great to hear. Thanks for confirming, Tom!
Average of ratings: -