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

Make sure login errors from hydra are actually handled in the authentication/handlers.go #323

Open
BarcoMasile opened this issue Jun 11, 2024 · 6 comments
Assignees
Labels
enhancement New feature or request

Comments

@BarcoMasile
Copy link
Contributor

BarcoMasile commented Jun 11, 2024

Description

We need to make sure that Admin Service correctly handles cases in which the Hydra login flow ended with an error.
Hydra seems to redirect to Admin UI with error query parameters, we need to return a correct API response with the contextual errors.

In order to simulate this error we need to test this with:

  • a failed login
  • wrong client credentials (client-id + client-secret)
  • wrong redirect URI
    Some of this will probably not even redirect to the Admin Service, in that case we won't be able to do much. But still, we need to return a 400 error in case something goes wrong and Hydra still redirects the useragent to the Admin UI.
@BarcoMasile BarcoMasile added the enhancement New feature or request label Jun 12, 2024
Copy link

Thank you for reporting us your feedback!

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

This message was autogenerated

@BarcoMasile
Copy link
Contributor Author

In case the client-id or client-secret is misconfigured for admin-ui, we end up on the Login UI.
We get redirected to Login UI with the following URL:

http://localhost:4455/ui/oidc_error?error=invalid_client&error_description=Client+authentication+failed+%28e.g.%2C+unknown+client%2C+no+client+authentication+included%2C+or+unsupported+authentication+method%29.+The+requested+OAuth+2.0+Client+does+not+exist.+Unable+to+locate+the+resource

image

@BarcoMasile
Copy link
Contributor Author

Same goes for a misconfigured redirect URI.
We get redirected to Login UI with the following URI:

http://localhost:4455/ui/oidc_error?error=invalid_request&error_description=The+request+is+missing+a+required+parameter%2C+includes+an+invalid+parameter+value%2C+includes+a+parameter+more+than+once%2C+or+is+otherwise+malformed.+The+%27redirect_uri%27+parameter+does+not+match+any+of+the+OAuth+2.0+Client%27s+pre-registered+redirect+urls.

image

@BarcoMasile
Copy link
Contributor Author

Trying out a "failed login flow" with an external provider, in this case it's Github, we never get back to the Admin UI or Login UI for that matter.
The login smiply fails and the user stays on Github login page.

Here are two examples, with a failed first factor authentication method...
image

...and with a failed second factory authentication method
image

@BarcoMasile
Copy link
Contributor Author

Conclusion

Login UI already handles the first two kinds of errors decently, those fall in the misconfiguration scenario, for which we need to rely on the administrator deploying the Bundle or the Admin UI, making sure the login flow works correctly. There is no way to be alerted of a misconfiguration without running the app, unless we provide some sort of dynamic validation hitting hydra APIs, which is way overkill.
Notice how between the first and second case the error message changes, but it's still pretty generic about what went wrong, and that's hydra not revealign too much about the situation, for security reasons.

For the external IdP scenario, I just tested Github, but although other IdPs tend to behave the same way, some may not. We should opportunistically check for other IdPs behavior as a best effort activity and make sure we handle errors in a graceful way. Although different providers may respond with different payloads / query parameters "schemas" so we'll see what we can do about having a generic approach to that.

@BarcoMasile BarcoMasile self-assigned this Oct 1, 2024
@nsklikas nsklikas reopened this Oct 8, 2024
@nsklikas
Copy link
Contributor

nsklikas commented Oct 8, 2024

Reopening this issue. The purpose of the issue was to validate that the admin UI handles the errors returned from the OP, not if the platform can handles errors from the external providers. Per the OAuth2 spec:

If the request fails due to a missing, invalid, or mismatching
redirection URI, or if the client identifier is missing or invalid,
the authorization server SHOULD inform the resource owner of the
error and MUST NOT automatically redirect the user-agent to the
invalid redirection URI.
If the resource owner denies the access request or if the request
fails for reasons other than a missing or invalid redirection URI,
the authorization server informs the client by adding the following
parameters to the query component of the redirection URI using the
"application/x-www-form-urlencoded" format, per Appendix B:

To see that the admin UI does not handle oauth2 errors correctly, you can just visit https://iam.dev.canonical.com/stg-identity-jaas-dev-admin-ui/api/v0/auth/callback?error=error&error_description=The+error+is+unrecognizable&state=fdsafdsa

(this came up after looking into https://warthogs.atlassian.net/browse/IAM-1136)

@nsklikas nsklikas added enhancement New feature or request and removed enhancement New feature or request labels Oct 8, 2024
@shipperizer shipperizer added enhancement New feature or request and removed enhancement New feature or request labels Oct 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants