tsugiproject / tsugi

Tsugi Admin, Developer, and Management Console (pls join the dev list)
http://www.tsugi.org
Apache License 2.0
346 stars 257 forks source link

CANVAS: Handle same issuer for different(almost all) sites(deployments) #83

Closed ziaulrehman40 closed 4 years ago

ziaulrehman40 commented 4 years ago

NOTE: This issue is more like an issue with canvas's implementation of LTI, but canvas being such a big player in the space, we need to work out to see what we can do to support their implementation in this regard.

Problem in canvas (iss, issuer):

Issue is, as described in the docs:

This requests contains an issuer identifier (iss) to recognize Canvas is launching the tool. As the issuer, Instructure-hosted Canvas intances all use the following, regardless of the specific account domain(s) that the tool was launched from:

https://canvas.instructure.com (Production environment launches) https://canvas.beta.instructure.com (Beta environment launches) https://canvas.test.instructure.com (Test environment launches)

And as a matter of fact, i recently did a canvas deployment, and on our xyz.com domain where canvas in hosted, we are getting https://canvas.instructure.com as issuer instead of xyz.com. I know we can change the issuer to anything doing some config, but thats only us, we can't force everyone to set proper and unique issuer ids in their canvas systems. And also, instructor's hosted canvas instances would still have this same issuer despite whatever the domain/subdomain is.

Related links: 1- https://community.canvaslms.com/thread/36682-lti13-how-to-identify-clientid-and-deploymentid-on-launch#comment-153280 2- https://community.canvaslms.com/message/202373-issuer-identifieriss-issues-finding-iss-and-uniqueness-of-iss

Problem in TSUGI because of above:

Because of the uniqueness validation on issuer Id in TSUGI(first field at /admin/key/issuer-add form), we can really just add 1 canvas site as issuer, we can't really support multiple sites because no matter what the domain of the site is, with default configs, it will give iss(issuer id) as https://canvas.instructure.com.

I hope i have explained the problem correctly, if not let me know and i can re-phrase.

We want to work this out in tsugi codebase and share the solution with community. We are just looking for some hints as to what can be a good approach to fix this issue(in timely manner).

csev commented 4 years ago

We might need to do a Zoom on this. But I think that Tsugi already supports this. This is the essence of the difference in Tsugi between “LTI 1.3 Issuers” and “Tenant Keys”.

To make this work - you need to add three issuers - one for each of the possible canvas issuers. Then when you need to add a tenant, you will need to add three tenants, one each pointing to each of the issuers.

This way, the data in Tsugi will be completely isolated between production, test, and dev. Which is what you want. In Tsugi the “Tenant” is the “silo” - the Issuer is just a way to check signatures.

I have never set this all up - so we might want to do a Zoom so I can see how Canvas works - what Issuer / tenant data they give you, etc. I never set up a Canvas as an admin - I just gave Canvas a Tsugi instance and they set it up :)

/Chuck

On Jul 6, 2020, at 1:27 PM, Zia Ul Rehman notifications@github.com wrote:

NOTE: This issue is more like an issue with canvas's implementation of LTI, but canvas being such a big player in the space, we need to work out to see what we can do to support their implementation in this regard.

Problem in canvas (iss, issuer):

Issue is, as described in the docs https://canvas.instructure.com/doc/api/file.lti_dev_key_config.html:

This requests contains an issuer identifier (iss) to recognize Canvas is launching the tool. As the issuer, Instructure-hosted Canvas intances all use the following, regardless of the specific account domain(s) that the tool was launched from:

https://canvas.instructure.com https://canvas.instructure.com/ (Production environment launches) https://canvas.beta.instructure.com https://canvas.beta.instructure.com/ (Beta environment launches) https://canvas.test.instructure.com https://canvas.test.instructure.com/ (Test environment launches)

And as a matter of fact, i recently did a canvas deployment, and on our xyz.com domain where canvas in hosted, we are getting https://canvas.instructure.com as issuer instead of xyz.com. I know we can change the issuer to anything doing some config, but thats only us, we can't force everyone to set proper and unique issuer ids in their canvas systems. And also, instructor's hosted canvas instances would still have this same issuer despite whatever the domain/subdomain is.

Related links: 1- https://community.canvaslms.com/thread/36682-lti13-how-to-identify-clientid-and-deploymentid-on-launch#comment-153280 https://community.canvaslms.com/thread/36682-lti13-how-to-identify-clientid-and-deploymentid-on-launch#comment-153280 2- https://community.canvaslms.com/message/202373-issuer-identifieriss-issues-finding-iss-and-uniqueness-of-iss https://community.canvaslms.com/message/202373-issuer-identifieriss-issues-finding-iss-and-uniqueness-of-iss Problem in TSUGI because of above:

Because of the uniqueness validation on issuer Id in TSUGI(first field at /admin/key/issuer-add form), we can really just add 1 canvas site as issuer, we can't really support multiple sites because no matter what the domain of the site is, with default configs, it will give iss(issuer id) as https://canvas.instructure.com.

I hope i have explained the problem correctly, if not let me know and i can re-phrase.

We want to work this out in tsugi codebase and share the solution with community. We are just looking for some hints as to what can be a good approach to fix this issue(in timely manner).

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/tsugiproject/tsugi/issues/83, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJEJJRN55J6SWIRZW47JYLR2ICR5ANCNFSM4OR2CD5Q.

ziaulrehman40 commented 4 years ago

@csev thankyou so much for the quick response.

We would love to do that. We have our own canvas instance and tsugi instance, with a WIP "mod". So we really just have to connect dots here.

You must have received a direct email from Leo (lcunha@curriki.org). Lets plan a zoom call whenever possible.

Thanks again for all the work around tsugi and quick response here.

ziaulrehman40 commented 4 years ago

Just for the sake of completion(and the record for community) here. Let me put out a reply to what i understood of your suggestion above.

I think i was not able to explain the problem correctly or i didn't understand your response correctly, i will try again to explain the issue:

Lets say we have 2 canvas instances,

  1. https://canvas1.mydomain.com
  2. https://canvas2.mydomain.com

These two instances are both independently hosted somewhere in the wild(cloud), and both have default config for issuer Id, and default config is that issuer is set to https://canvas.instructure.com for both of these instances of canvas.(i have tested this on our own deployments and it holds true, and is also mentioned in above linked docs and issues)

Now, we want to install tsugi app in both of these instances, which means we want to add issuers in tsugi admin.

We go to https://canvas1.mydomain.com generate an LTI key(lets say it gives client_id 12345678), we go in tsugi add issuer form, and we fill it with:

Issuer Id: https://canvas.instructure.com
client Id: 12345678
LTI 1.3 Platform OAuth2 Well-Known/KeySet URL (from the platform): https://canvas1.mydomain.com/api/lti/security/jwks
LTI 1.3 Platform OAuth2 Bearer Token Retrieval URL (from the platform): https://canvas1.mydomain.com/login/oauth2/token
LTI 1.3 Platform OIDC Authentication URL (from the Platform): https://canvas1.mydomain.com/api/lti/authorize_redirect

We hit save and everything works, we copy json for canvas over in canvas, and complete rest of the process, all good for https://canvas1.mydomain.com

But now, we have to add app for https://canvas2.mydomain.com, so we generate an LTI key in this canvas instance, and as this is an independently hosted canvas instance, we get a client id (12345678), we go in tsugi admin and try to add an issuer for this canvas instance., we fill the form:

Issuer Id: https://canvas.instructure.com
client Id: 12345678
LTI 1.3 Platform OAuth2 Well-Known/KeySet URL (from the platform): https://canvas2.mydomain.com/api/lti/security/jwks
LTI 1.3 Platform OAuth2 Bearer Token Retrieval URL (from the platform): https://canvas2.mydomain.com/login/oauth2/token
LTI 1.3 Platform OIDC Authentication URL (from the Platform): https://canvas2.mydomain.com/api/lti/authorize_redirect

And when we save, we get error that issuer is duplicate, why? Because issuer is same for all canvas instances, no matter what the domain is. At-least with default configs it is so. So now we know issuer is not going to be unique for canvas instances, but tsugi expects these to be unique.

Other than issuer, everything can be different, especially signatures will be different, all the urls for LTI flows are different. So we really need to fix this uniqueness constraint to work with above mentioned scenario. (Which is common for canvas, as per default configs which define issuer to be https://canvas.instructure.com no matter domain/sub domain).

And again, we want to contribute in tsugi to fix this, as we are aiming for a release our plugin within a couple of day, things are working with moodle but with canvas we have hit this issue. So we are only seeking some guidance in finding and building a solution, we will definitely make a PR here once fixed.

ziaulrehman40 commented 4 years ago

Followup updates as per our investigations so far:

So we are looking into how to make this all work is a cross LMS way.

LTI launch params from different LMSs

Moodle:

So we looked in moodle's LTI launch params, they recently added extra params which are coming to v3.9, it sends these values to OIDC launch:

array(6) {
  ["iss"]=>
  string(27) "http://moodlemac.local:8282"
  ["target_link_uri"]=>
  string(35) "http://localhost/tsugi/mod/curriki/"
  ["login_hint"]=>
  string(1) "2"
  ["lti_message_hint"]=>
  string(2) "50"
  ["client_id"]=>
  string(15) "1KM8AZ6FFfQmSbf"
  ["lti_deployment_id"]=>
  string(1) "3"
}

Our interest is with iss, client_id and deployment_id. This along with the fact that moodle reports unique iss as per domain where moodle is hosted, we have no issue with moodle. GREAT!

Canvas:

But with canvas, we get something like:

array(6) {
 ["iss"]=>
 string(30) "https://canvas.instructure.com"
 ["login_hint"]=>
 string(40) "e20984648b488fb11b323741425126dca07aa300"
 ["client_id"]=>
 string(17) "00000000000000000"
 ["target_link_uri"]=>
 string(86) "https://OurLtiApplication/someEndpoint"
 ["lti_message_hint"]=>
 string(423) "eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJ2ZXJpZmllciI6IjhhOGFjNDQ0NDJjNjkxMDFjYjRiMzVkZjQyNTA5NzhhNjUyYzhmNTY0YWU3MDhjOTcxMGE1OTQ2ODAzMTY3MzUyMjA3YTczNjQ2YjRlOTA1NTYyNzM1MDMxNDM1ZGY1MzU3MDQ5Y2Q1YzMyNWYwMDA5N2Y0YjQ4YTA5MWQwMTA5IiwiY2FudmFzX2RvbWFpbiI6ImNhbnZhcy5zdGdlb3JnZXMuYmMuY2EiLCJjb250ZXh0X3R5cGUiOiJDb3Vyc2UiLCJjb250ZXh0X2lkIjo0OTcwMDAwMDAwMDAwMDg2MSwiZXhwIjoxNTkyMjQ5NzY4fQ.KmI75uFTAZBWe9F1mZ6nXnpabWVKDUm4HS6HjP-jExw"
}

Additionally, decoding lti_message_hint we have in it:

{
 "verifier": "8a8ac44442c69101cb4b35df4250978a652c8f564ae708c9710a5946803167352207a73646b4e905562735031435df5357049cd5c325f00097f4b48a091d0109",
 "canvas_domain": "canvas.curriki.org",
 "context_type": "Course",
 "context_id": 49700000000000860,
 "exp": 1592249768
}

So all in all, we have iss, client_id and canvas_domain to our interest.

We can use these values combined to uniquely identify which issuer to use.

Solution(v1?)

So it seems like we will need to take an extra input(canvas_domain) on issuer form, and bind that and client_id and combination of all 3 should be unique. And will allow us to uniquely identify issuers for any LTI launch.

Path from here?

So now we are analyzing the tsugi codebase to figure out how we can implement above mentioned solution, and how much effort it will be.

In codebase it seems uniqueness is assumed only on iss, not any combination of it with other values: image

What we need?

So now we are looking for any possible input on the above investigation, and if the route we have discovered is right path to go? Because we don't just want to do it for our self, we want this merged in tsugi so it is fixed for once and for all.

So we are seeking advice on if this approach is right, if so, to what extent would you say the application architecture will change. And what strategy would you recommend to implement this.

csev commented 4 years ago

I would like a Zoom to look this over. Please send me an email at the tsugi dev list so we can set a time up. I am in the US Eastern time zone - I have from 9-11 and 2-5 available tomorrow.

/Chuck

On Jul 7, 2020, at 9:34 AM, Zia Ul Rehman notifications@github.com wrote:

Followup updates as per our investigations so far:

So we are looking into how to make this all work is a cross LMS way.

LTI launch params from different LMSs

Moodle:

So we looked in moodle's LTI launch params, they recently added extra params which are coming to v3.9, https://tracker.moodle.org/browse/MDL-67072 it sends these values to OIDC launch:

array(6) { ["iss"]=> string(27) "http://moodlemac.local:8282" ["target_link_uri"]=> string(35) "http://localhost/tsugi/mod/curriki/" ["login_hint"]=> string(1) "2" ["lti_message_hint"]=> string(2) "50" ["client_id"]=> string(15) "1KM8AZ6FFfQmSbf" ["lti_deployment_id"]=> string(1) "3" } Our interest is with iss, client_id and deployment_id. This along with the fact that moodle reports unique iss as per domain where moodle is hosted, we have no issue with moodle. GREAT!

Canvas:

But with canvas, we get something like https://community.canvaslms.com/thread/52665-what-key-must-we-validate-ltimessagehint-the-encoded-jwt-against:

array(6) { ["iss"]=> string(30) "https://canvas.instructure.com" ["login_hint"]=> string(40) "e20984648b488fb11b323741425126dca07aa300" ["client_id"]=> string(17) "00000000000000000" ["target_link_uri"]=> string(86) "https://OurLtiApplication/someEndpoint" ["lti_message_hint"]=> string(423) "eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJ2ZXJpZmllciI6IjhhOGFjNDQ0NDJjNjkxMDFjYjRiMzVkZjQyNTA5NzhhNjUyYzhmNTY0YWU3MDhjOTcxMGE1OTQ2ODAzMTY3MzUyMjA3YTczNjQ2YjRlOTA1NTYyNzM1MDMxNDM1ZGY1MzU3MDQ5Y2Q1YzMyNWYwMDA5N2Y0YjQ4YTA5MWQwMTA5IiwiY2FudmFzX2RvbWFpbiI6ImNhbnZhcy5zdGdlb3JnZXMuYmMuY2EiLCJjb250ZXh0X3R5cGUiOiJDb3Vyc2UiLCJjb250ZXh0X2lkIjo0OTcwMDAwMDAwMDAwMDg2MSwiZXhwIjoxNTkyMjQ5NzY4fQ.KmI75uFTAZBWe9F1mZ6nXnpabWVKDUm4HS6HjP-jExw" } Additionally, decoding lti_message_hint we have in it:

{ "verifier": "8a8ac44442c69101cb4b35df4250978a652c8f564ae708c9710a5946803167352207a73646b4e905562735031435df5357049cd5c325f00097f4b48a091d0109", "canvas_domain": "canvas.curriki.org", "context_type": "Course", "context_id": 49700000000000860, "exp": 1592249768 } So all in all, we have iss, client_id and canvas_domain to our interest.

We can use these values combined to uniquely identify which issuer to use.

Solution(v1?)

So it seems like we will need to take an extra input(canvas_domain) on issuer form, and bind that and client_id and combination of all 3 should be unique. And will allow us to uniquely identify issuers for any LTI launch.

Path from here?

So now we are analyzing the tsugi codebase to figure out how we can implement above mentioned solution, and how much effort it will be.

In codebase it seems uniqueness is assumed only on iss, not any combination of it with other values: https://user-images.githubusercontent.com/22441482/86788852-0cb8b580-c080-11ea-98ea-589b4decb0fe.png What we need?

So now we are looking for any possible input on the above investigation, and if the route we have discovered is right path to go? Because we don't just want to do it for our self, we want this merged in tsugi so it is fixed for once and for all.

So we are seeking advice on if this approach is right, if so, to what extent would you say the application architecture will change. And what strategy would you recommend to implement this.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/tsugiproject/tsugi/issues/83#issuecomment-654863479, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJEJJTOHVZNJCAPOBHMWS3R2MP53ANCNFSM4OR2CD5Q.

ziaulrehman40 commented 4 years ago

Alright let me send an email for the 9-11 slot. Thanks again.

csev commented 4 years ago

This has now been fixed.