Skip to content

Update default LTI keys - #2361

Open
nwalters512 wants to merge 3 commits into
instructure:masterfrom
nwalters512:update-default-lti-keys
Open

Update default LTI keys#2361
nwalters512 wants to merge 3 commits into
instructure:masterfrom
nwalters512:update-default-lti-keys

Conversation

@nwalters512

Copy link
Copy Markdown

The default keys used a short key length and outdated algorithm, which made them incompatible with newer LTI tools.

I also updated the instructions for generating new keys; the old instructions don't work (see #1861). I copied the call from here:

def new_key
CanvasSecurity::RSAKeyPair.new.to_jwk.to_json
end

Closes #1861.

@CLAassistant

CLAassistant commented Jun 4, 2024

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@dustin-cowles

Copy link
Copy Markdown
Contributor

@nwalters512 Thanks for your contribution! This looks good to me, if you can resolve the merge conflicts I will pull it into our CR pipeline.

@nwalters512 nwalters512 left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@dustin-cowles I've resolved the merge conflicts! I left a few comments to call out some questions I had.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Both here and in inst-cli/docker-compose/config/dynamic_settings.yml.erb, it looked to be like the only differences between the lti-keys and services-jwt values were the kid fields of each, which had a _a/_b/_c suffix in the services-jwt section. Let me know if you'd like me to generate completely separate keys for services-jwt. I don't know enough about how these are used to make that call myself.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

This file doesn't have a services-jwt: section. Is that intentional?

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR updates the default LTI (and related services JWT) JWKs shipped in the repo to use stronger RSA key material and refreshes the inline instructions for generating replacement keys, addressing incompatibilities with newer LTI tooling and fixing the documented “private key is needed” failure mode described in #1861.

Changes:

  • Replaced the existing default lti-keys and services-jwt JWK blobs with new RSA JWKs that include the required private key fields plus alg/use.
  • Updated the inline “generate a new key” instructions to use CanvasSecurity::RSAKeyPair.new.to_jwk.to_json (which generates a private JWK, not just a public JWK).
  • Normalized/modernized the comment formatting around the key blocks.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
inst-cli/docker-compose/config/dynamic_settings.yml.erb Updates Docker-based dynamic settings defaults for lti-keys and services-jwt and refreshes key-generation instructions.
docker-compose/config/dynamic_settings.yml Updates the non-ERB Docker compose dynamic settings defaults for LTI/services signing keys and the generation instructions.
config/dynamic_settings.yml.example Updates the example dynamic settings to use compatible default JWKs and corrected key-generation instructions.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@nwalters512

Copy link
Copy Markdown
Author

@dustin-cowles I resolved the merge conflicts. Let me know if I can do anything else to help get this in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

These instructions don't create the private key - which is needed for LTI to work.

4 participants