celtic-project / LTI-PHP

PHP class library for building LTI integrations
GNU Lesser General Public License v3.0
48 stars 38 forks source link

Seemingly erroneous Missing login_hint parameter failure #77

Open hisuwh opened 8 months ago

hisuwh commented 8 months ago

First of all, thanks a lot for the saLTIre test tool - this has been really useful both understanding and implementing LTI.

I implemented an initial rough platform in a standalone application to understand how to approach this and managed to get this validated by your application:

image

I have then implemented essentially the same code into my application but I'm now encountering this issue: image

(Also not sure why the second JWT is not showing as signed - this code is all identical).

Looking in chrome dev tools the login_hint is present.

Good Request image

Failing request image

The only difference between these two requests is the lti_message_hint.

According to the spec:

Similarly to the login_hint parameter, lti_message_hint value is opaque to the tool. If present in the login initiation request, the tool MUST include it back in the authentication request unaltered.

So this shouldn't be a factor (in fact according to the above the tool shouldn't care if the login_hint is there either).

The tool clearly does know the login_hint is there because it is including it in authorization request back to my application: image

spvickers commented 8 months ago

It sounds like you are reporting an issue with the saLTIre application (available at https://saltire.lti.app) rather than with this library. If so, for future reference, I would recommend using the feedback option in the app to report issues.

However, in the meantime, are you able to confirm whether your platform received an authentication request in response to your initiate login request? If so, what response did you return?

hisuwh commented 8 months ago

Sorry yes I am. I did notice the source code for the app appears to be in this repository so I thought it still appropriate to report here.

Yes the screenshot above is the authentication request being sent back to my application and as you can see it is including the login_hint.

spvickers commented 8 months ago

The saLTIre app is not open source; its code is not available in this, or any other, repository.

So, as I understand it, the authentication request being sent to your app is correct; what response are you sending in return to this?

hisuwh commented 8 months ago

We then send a post request back with the id_token and state in the payload

spvickers commented 8 months ago

Are the id_token and state the only parameters in your request message?

spvickers commented 6 months ago

Closing this issue as no reply received, so assuming it has been resolved. Please open a new issue if this it not the case.

hisuwh commented 2 weeks ago

@spvickers this is still an issue btw. In fact it is not working at all now.

Are the id_token and state the only parameters in your request message?

Interesting that you ask this. No iss is also included in my request message - this is the only discernible difference between the version that works and the version that doesn't.

hisuwh commented 2 weeks ago

I managed to work out how to remove this parameter through the OIDC library I am using. However, this parameter ideally should exist to prevent "mix-up attacks": https://datatracker.ietf.org/doc/html/draft-ietf-oauth-iss-auth-resp-05

spvickers commented 2 weeks ago

The document you reference appears to be a draft and I am not an expert on how widespread it has been implemented or is expected to be. This library follows the LTI spec from 1EdTech which does not include the iss parameter in the response (or in the Openid documents it references, as far as I can see). The iss value is included as a claim in the JWT passed in the id_token parameter; do you think duplicating it is useful? Have you asked 1EdTech about this, or seen any other documents or implementations which duplicate the iss value as a form parameter?

hisuwh commented 2 weeks ago

Ah looks like that draft is old and this has now been published: https://datatracker.ietf.org/doc/rfc9207/ More of an explanation here: https://www.hackmanit.de/en/blog-en/132-how-to-protect-your-oauth-client-against-mix-up-attacks

This is part of the OAuth2.0 spec so may not be mentioned explicitly by the LTI spec. Regardless of whether or not saLTIre needs or uses it, including this parameter should not break it. In fact from what I can tell it is perfectly valid to send additional parameters to those specified by the spec (with some providers defining additional required parameters).

Our workaround for now is to remove this additional parameter - though with the OAuth library we are using we have to do this for all clients. We are currently only using OAuth for LTI so I'm comfortable with doing this for now. However, I would like to expand our OAuth usage and at which point I would like the additional protection this parameter provides.

It would be nice to see saLTIre support this as we are relying on this excellent tool to validate our LTI implementation. Thanks.

spvickers commented 2 weeks ago

Thanks for the additional information. I agree that it would be better for the library to ignore additional parameters and I will investigate this. The current code is written to allow a single endpoint to be used for different message types, and inspecting the parameters which are present is used to do this. Since LTI only uses id_token parameters in its Openid/OAuth 2 connections, I do not think the mix-up attack applies here; the iss in the id_token is used to verify that the state is coming from the expected platform. Or am I misunderstanding this issue? If so, have you raised this with 1EdTech to have them make the iss parameter a requirement?

hisuwh commented 2 weeks ago

Yes I don't think the mix-up attack applies here. However, we have other use cases for OAuth so I do not want to open ourselves up to vulnerability long term just to support LTI.

I have not raised this with 1EdTech. It is my understanding that the LTI standard is built on top of the OAuth standard so this should be implicit. Though as you point out for the flow used here I guess the iss parameter wouldn't be required but the whole request shouldn't fail if present.

spvickers commented 2 weeks ago

Thanks for the inpur; I will try to make a change to avoid this issue.