search

autolab / Autolab

Course management service that enables auto-graded programming assignments.
http://www.autolabproject.com/
Apache License 2.0
761 stars 218 forks source link

LTI JWK parameters should not be embedded constants #1677

Closed cg2v closed 1 year ago

cg2v commented 1 year ago

https://github.com/autolab/Autolab/blob/7b6b26419ce490b56d9d4ece945bea5a5a238e96/app/controllers/lti_nrps_controller.rb#L19-L25 Most of those parameters in lti_nrps_controller.rb should not be embedded constants.

In particular, kid is an identifier for or hash of the public key, which identifies a single credential. If the tool_private_key changes, then so must the public kid presented in the JWS/JWK signature. e is the public RSA exponent, which generally is a constant (65537), but does not have to be.

Even that leaves aside the possibility that providers other than Instructure (Canvas) might use different key types or algorithms. It seems like jwt-ruby is capable of using real JSON JWK files, so I'm not sure why you switched to openssl PEM files and only support RS256 signatures.

cg2v commented 1 year ago

This does NOT affect the forthcoming CMU deployment. It will prevent any other site from trying to set up LTI, unless they modify the kid value in the code to match their credential.

20wildmanj commented 1 year ago

I agree, ideally we should make it so that we read the JWK from a JSON so that these fields are not embedded. You can specify the kid to be whatever you want, so as long as it matches on both the private and public key it should work on other deployments, but I agree this is a bad idea, so this should be unembedded.

The embedding of JWK parameters and usage of PEM format in configurations was done as a holdover of getting JWK signing to work quickly. I will work on a fix to switch to reading JSON files in the next few weeks.