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

Allowed urls do not include return_to #259

Open
natalian98 opened this issue Sep 9, 2024 · 8 comments · May be fixed by canonical/identity-platform-login-ui#284
Open

Allowed urls do not include return_to #259

natalian98 opened this issue Sep 9, 2024 · 8 comments · May be fixed by canonical/identity-platform-login-ui#284
Labels
bug Something isn't working

Comments

@natalian98
Copy link
Contributor

If I deploy the bundle, create an admin account via juju CLI, setup the authenticator and try login via Hydra:
image

I will end up in the totp page:
image

Now if I go back (without entering the otp), I will be asked only for my password (username is cached):
image

If I now try to reset my password, I will be asked to input my email:
image

But then I will be redirected to an error page:
image

Originally posted by @nsklikas in canonical/iam-bundle#273 (comment)

@natalian98
Copy link
Contributor Author

I'm able to reproduce the issue deploying kratos rev 471 and login ui rev 117.
The allowed_return_urls parameter does not include all required return_to urls:

{
  "code": 400,
  "debug": "Allowed domains are: https://iam.dev.canonical.com/, https://iam.dev.canonical.com/stg-identity-jaas-dev-kratos/self-service",
  "id": "self_service_flow_return_to_forbidden",
  "message": "The request was malformed or contained invalid parameters",
  "reason": "Requested return_to URL \"http://kratos.stg-identity-jaas-dev.svc.cluster.local:4433/self-service/settings/browser?return_to=https%3A%2F%2Fiam.dev.canonical.com%2Fstg-identity-jaas-dev-login-ui%2Fui%2Fsetup_complete\" is not allowed.",
  "status": "Bad Request"
}

@natalian98 natalian98 added the bug Something isn't working label Sep 9, 2024
Copy link

Thank you for reporting us your feedback!

The internal ticket has been created: https://warthogs.atlassian.net/browse/IAM-1044.

This message was autogenerated

@natalian98
Copy link
Contributor Author

natalian98 commented Sep 9, 2024

It is only reproducible with the provided steps but not when following a standard process for password reset.
In the second case the user is correctly redirected to the setup_complete page:

  "request_url": "http://kratos.stg-identity-jaas-dev.svc.cluster.local:4433/self-service/settings/browser?return_to=https%3A%2F%2Fiam.dev.canonical.com%2Fstg-identity-jaas-dev-login-ui%2Fui%2Fsetup_complete",
  "return_to": "https://iam.dev.canonical.com/stg-identity-jaas-dev-login-ui/ui/setup_complete",

@natalian98
Copy link
Contributor Author

In this specific case, when user starts the recovery flow (in the "enter email to reset password" step), it doesn't ask for a one-time code sent by email because the user has completed authentication with password and thus already has a session with aal1.
It attempts to redirect to password settings:
https://iam.dev.canonical.com/stg-identity-jaas-dev-login-ui/api/kratos/self-service/settings/browser
but accessing settings requires aal2, so it calls the login endpoint to complete 2fa:

https://iam.dev.canonical.com/stg-identity-jaas-dev-kratos/self-service/login/browser?aal=aal2
&return_to=http%3A%2F%2Fkratos.stg-identity-jaas-dev.svc.cluster.local%3A4433%2Fself-service%2Fsettings%2Fbrowser
%3Freturn_to%3Dhttps%253A%252F%252Fiam.dev.canonical.com%252Fstg-identity-jaas-dev-login-ui%252Fui%252Fsetup_complete

This call fails because the return_to is not allowed (the internal service dns record is used instead of kratos public ingress url). There is a doubled return_to parameter, because we first need to redirect to insert totp and then to settings, which at the end will bring the user to https://iam.dev.canonical.com/stg-identity-jaas-dev-login-ui/ui/setup_complete.

Requested return_to URL "http://kratos.stg-identity-jaas-dev.svc.cluster.local:4433/self-service/settings/browser?return_to=https%3A%2F%2Fiam.dev.canonical.com%2Fstg-identity-jaas-dev-login-ui%2Fui%2Fsetup_complete" is not allowed.

I found that this nested return url is calculated by kratos sdk here (log message from a localhost setup):

{"severity":"debug","@timestamp":"2024-09-13T12:35:07.83228994+02:00","message":"Redirect to: http://localhost:4433/self-service/login/browser?aal=aal2&return_to=http%3A%2F%2Flocalhost%3A4433%2Fself-service%2Fsettings%2Fbrowser%3Freturn_to%3Dhttp%253A%252F%252Flocalhost%253A4455%252Fui%252Fsetup_complete"}

I think the issue is that kratos sends the internal service dns in relation data and login ui uses it as KRATOS_PUBLIC_URL.

Note: Kratos public API is normally called during login flow, for example when user needs to complete 2fa (e.g. https://iam.dev.canonical.com/stg-identity-jaas-dev-kratos/self-service/login/browser?aal=aal2&login_challenge=<...>&return_to=<login-ui-url>), but for some reason the correct public url is called instead of the k8s service.

@shipperizer @nsklikas

@shipperizer
Copy link
Contributor

https://github.com/canonical/kratos-operator/blob/main/src%2Fcharm.py#L538-L545

I think something is lost in the UI endpoint relation as I cannot explain why the login URL is fine but the others are not

@nsklikas
Copy link
Contributor

I think that the issue is that we don't configure the URL under which Kratos needs to serve (public.base_url). This means that it does not know where to redirect the user to perform the 2fa. So it simply returns that url that was used to it. If we set the public.base_url to the public ingress' URL, then we won't be able to call kratos through the k8s API

The login UI calls Kratos always through the internal ingress:
image

We need to either:

  1. Start calling the Kratos public APIs through the public ingress, then we can also set the public.base_url
  2. Convert the URL on the fly on the login-ui side

@nsklikas
Copy link
Contributor

After thinking about this a little further, I realize that I didn’t really understand the issue at first. This may seem like an edge case, but it will become a more frequent case if (when) we introduce a side bar (or some kind of navigation menu). I think that the problem is that we have mixed up the recovery and the settings flows.

Currently to reset a user’s password what happens is:

  1. We initialize a recovery flow, this is needed because we assume that the user does not remember their password.
  2. The user provides their email
  3. We update the recovery flow, this sends a recovery code to the user’s email
  4. The user provides the recovery code that was sent to their email
  5. The user is redirected to the settings page, this is handled by Kratos as it always redirects the user to the settings page after the recovery flow
  6. We initialize a settings flow, no the user can reset their password

This bug happens when the user already has an active session, in that case what happens is:

  1. We initialize a recovery flow
  2. The user provides their email
  3. We try to update the recovery flow, but a 500 is returned because a login session is already active
  4. The frontend redirects the user to the reset_password page (see https://github.com/canonical/identity-platform-login-ui/blob/main/ui/util/handleFlowError.ts#L30 , this line does not make much sense from a higher level as we assume that an existing session error means that the user wants to change their password, we need to fix this or at least document it)
  5. We try to create (or get?) a settings flow, but not enough aal is provided so a 403 is returned
  6. The user is redirected to login to update their aal
  7. The user logs in
  8. The user is redirected back to the reset_password page and a settings flow is initialized

The difference between these 2 cases is that on the 1st case the user logs in using the recovery via email method and on the second case the user logs in using user name and password. It appears that our intention was to support only the first case.

IMHO an IdP should support both of these flows, one is for resetting the password (aka forgot my password) and the other is changing the password. An easy fix for this would be to just log out the user before performing the recovery flow (or after we got the error), this could be triggered by either the backend or the frontend.

The proper solution would be to untangle the 2 flows. We need to:

  1. have a forgot password button that would trigger the recovery flow to create a login session to the user (and once that is done the user will be redirected to the settings page to force password resetting, this is handled by Kratos)
  2. have a menu that would allow the user to change their password after logging in (ie do the settings flow)

But the question is, from a UX/flow perspective what is our intention here? (cc @natalian98 @lukasSerelis)

@natalian98
Copy link
Contributor Author

The frontend redirects the user to the reset_password page (see https://github.com/canonical/identity-platform-login-ui/blob/main/ui/util/handleFlowError.ts#L30 , this line does not make much sense from a higher level as we assume that an existing session error means that the user wants to change their password, we need to fix this or at least document it)

My assumption was that if a user starts a recovery flow but is already logged in, he intents to reset the password, hence the redirection.

If a user with a valid session wants to reset pwd, going to ui/reset_password starts a settings flow directly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants