MH-13082: Fix LTI security vulnerability and refactor LTI and OAuth classes #440
Conversation
|
@gregorydlogan I will prepare a new PR for Opencast 3, without the ability for multiple OAuth consumers. As discussed in #414, this solution has some shortcomings. I will close #414 in favor of the new PR. As for this PR, still hope we find a reviewer for 6.x 😬. |
mtneug
force-pushed
the
t/MH-13082-refactor-lit-to-osgi-service
branch
from
1af95fb
to
cddbce4
Compare
modules/kernel/pom.xml
Outdated
| @@ -300,6 +300,8 @@ | |||
| OSGI-INF/pingback.xml, | |||
| OSGI-INF/rest.xml, | |||
| OSGI-INF/security-filter.xml, | |||
| OSGI-INF/security-lit.xml, | |||
There was a problem hiding this comment.
This is the right filename, but the wrong spelling. Fix in a follow up PR.
| # OAuth consumer keys with should be highly trusted. | ||
| # | ||
| # By default OAuth consumer are regarded as untrusted and a user authenticating via such | ||
| # systems receives a rewritten username in the form of "lit:{ltiConsumerGUID}:{ltiUserID}". |
There was a problem hiding this comment.
is it lit:{... or lti:{...?
| # blacklist below instead. | ||
| # | ||
| # Default: false | ||
| #lti.allow_system_administrator=true |
There was a problem hiding this comment.
Default and comment-out value should match.
| # even if a trusted OAuth consumer key is used. | ||
| # | ||
| # Default: false | ||
| #lti.allow_digest_user=true |
There was a problem hiding this comment.
Default and commented-out value should match
| # | ||
| # Multiple users can be configured, by incrementing the counter. The list is read sequentially | ||
| # incrementing the counter. If you miss any numbers it will stop looking for further users. | ||
| #lti.blacklist.user.1=myAdminUser |
There was a problem hiding this comment.
Should include the default (ie, no user is blacklisted).
There was a problem hiding this comment.
How would that look like? Just #lti.blacklist.user.1=?
There was a problem hiding this comment.
Yes, I think so. And something like Default: no blacklisted users?
| # A blacklist of users not allowed to authenticate via LTI as themselves. | ||
| # | ||
| # Note that these users may still authenticate via LTI, but their username will be rewritten, | ||
| # even if a trusted OAuth consumer key is used. |
There was a problem hiding this comment.
I feel like I'm not 100% sure what this paragraph means. They can't authenticate as themselves via LTI, but their username is rewritten, as it is in the other cases as well. Maybe I'm just not familiar enough with how LTI works to get this, but it seems like this description needs to be fleshed out more.
There was a problem hiding this comment.
Well their username is rewritten (e.g. lti:lmsng.school.edu:1234 instead of mtneug). Opencast's role provider will see the new username and therefore do not load roles associated with mtneug. So more accurately: they can't authenticate as themselves because their username is rewritten.
There was a problem hiding this comment.
Ok, so this is down to my not having worked with LTI. We're good here.
| ``` | ||
|
|
||
| > **Notice:** Marking a consumer key as highly trusted can be a security risk! If the usernames of sensitive Opencast | ||
| > users are not blacklisted, the LMS administrator could create LMS users with the same username and use LTI to grant |
There was a problem hiding this comment.
Can we list these usernames? I'd think it would only be the two, and those are not likely to change, correct?
There was a problem hiding this comment.
By default yes, but there might be many more (e.g. custom admin or API users). However, I can list the default usernames.
mtneug
force-pushed
the
t/MH-13082-refactor-lit-to-osgi-service
branch
3 times, most recently
from
2f06c11
to
0457492
Compare
|
I have had some problems yesterday in our test infrastructure, which I think should now be fixed. Will report back. |
|
Is fixed now and working as intended. Originally I missed to include |
mtneug
force-pushed
the
t/MH-13082-refactor-lit-to-osgi-service
branch
from
0457492
to
a33f078
Compare
|
@gregorydlogan I created #449 for older releases. |
This refactors LTI and OAuth classes to OSGi services and implements the LTI user blacklist as discussed in #414 and during the developer meeting.
There is a new functionality in that this changes allows to configure multiple OAuth consumers instead of just one, so this might be seen as feature instead of a bug fix. Although, this might be small enough?
@gregorydlogan note that I have a branch ready for 3.x, however since
SecurityUtildid not have that method back then, it is marginally different. Should I file separately?