Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Seemingly erroneous Missing login_hint parameter failure #77

Open
hisuwh opened this issue Feb 20, 2024 · 13 comments
Open

Seemingly erroneous Missing login_hint parameter failure #77

hisuwh opened this issue Feb 20, 2024 · 13 comments

Comments

@hisuwh
Copy link

hisuwh commented Feb 20, 2024

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
Copy link
Contributor

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
Copy link
Author

hisuwh commented Mar 4, 2024

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
Copy link
Contributor

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
Copy link
Author

hisuwh commented Mar 6, 2024

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

@spvickers
Copy link
Contributor

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

@spvickers
Copy link
Contributor

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
Copy link
Author

hisuwh commented Oct 21, 2024

@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
Copy link
Author

hisuwh commented Oct 21, 2024

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 spvickers reopened this Oct 21, 2024
@spvickers
Copy link
Contributor

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
Copy link
Author

hisuwh commented Oct 21, 2024

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
Copy link
Contributor

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
Copy link
Author

hisuwh commented Oct 22, 2024

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
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants