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.

Sign up for GitHub

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

Jump to bottom

Fixes issue #1064 #1097

Closed
wants to merge 3 commits into from
Closed

Fixes issue #1064 #1097

wants to merge 3 commits into from

Conversation

JessieWadman
Copy link

Saml2Handler for ASP.NET Core uses private property CurrentUri to determine where the application is running, and uses this as return url for SignIn command.

This does not work if the application is behind a reverse proxy, or running in Kubernetes or any other solution where you have TLS termination before reaching the application.

In Kubernetes, for example, the requests will be:

Browser -> TLS terminating load balancer (https://domain/path) -> Pod (http://domain/path)
The application running inside the pod will determine that its URL is http://domain/path instead of https://domain/path causing a https to http downgrade. In reverse proxy scenarios, this will also affect the domain, and replace it with the reverse proxy address, instead of the public url (see issue #1064).

The fix (and to not break backwards compatibility) is to simply provide a Func to rewrite the redirect URL.
In most scenarios, the return url should be relative, not absolute. But so as not to break backwards compatibility, the Func can be used. Optionally, a flag could be put on options to indicate if we should strip the return url down to a relative URL instead. I still think this PR provides a good hook for other uses, so it shouldn't be replaced with the flag, but the default behavior of the Notifications Func could be made to respect the flag, so we don't need to manually convert the return url to a relative URL.

services.AddAuthentication(sharedOptions =>
{
    ...
}).AddSaml2(options =>
{
    // Converts http://pod:port/path to /path
    options.Notifications.RewriteRedirectUrl = (redirectUrl, publicUrl) => new Uri(redirectUrl.PathAndQuery, UriKind.Relative);
}

@AndersAbel
Copy link
Member

Have you checked the PublicOrigin setting? It is meant for scenarios like this.

In ASP.NET Core another option is to add a separate middleware before UseAuthentication that rewrites the request host/protocol to the one that is visible to the client. I think that is how it is done when kestrel is hosted behind IIS and IIS does the TLS termination.

@JessieWadman
Copy link
Author

JessieWadman commented Jun 3, 2019 •
edited

Have you checked the PublicOrigin setting? It is meant for scenarios like this.

In ASP.NET Core another option is to add a separate middleware before UseAuthentication that rewrites the request host/protocol to the one that is visible to the client. I think that is how it is done when kestrel is hosted behind IIS and IIS does the TLS termination.

@AndersAbel I have checked it, yes, and it has no effect, since the Saml2Handler injects the returnUrl based on it's current address, and that takes precedence over PublicOrigin (kind of a returnUrl ?? PublicOrigin statement). In the Func, I'm passing the PublicOrigin in as a parameter, so you can rewrite the detected return url to a relative path beneath the PublicOrigin, if you prefer, over a relative URL. The default Func implementation just throws the original return url back, to maintain backwards compatibility.

The middleware suggestion won't work in the case of https to http downgrade, if the application isn't exposed over http. Since after Sign-in you are redirected to the detected url (a http url), and if there's nothing there answering, then you just get connection failure in your web browser.

Like so:
Browser -> https://domain -> External sign-in -> https://domain/Acs -> http://domain/dashboard
If http://domain isn't exposed, you just get a connection error. You never reach the middleware.
You would need to put the middleware at the very end, and rewrite outgoing redirects instead, for that to work, and that appears very backwards :-)

@AndersAbel AndersAbel linked an issue Apr 21, 2020 that may be closed by this pull request
Redirecting to internal url after login instead of public origin #1064
Open
@philchuang
Copy link

Any ETA on this PR?

@snypenet
Copy link

snypenet commented Jan 24, 2022 •
edited

Sorry to bump an old PR, but is there an ETA on this? I just now ran into this problem and I was able to fix it by updating the GetLocation in the AcsCommand method to swap out the BaseUrl that is found with the configured PublicOrigin, if one is configured. Similar to the fix described here. So for now, we are working off of a forked version of this repository. Would love to update to use the mainline again :)

@AndersAbel
Copy link
Member

This shows one of the many reasons why it is time for a redesign, as planned for v3. The current target-all-platforms-all-frameworks architecture is brittle.

I'm closing this as won't fix as it won't be fixed in the current architecture.

@AndersAbel AndersAbel closed this Jan 30, 2022
@snypenet
Copy link

@AndersAbel i appreciate updating this PR. Hopefully it'll help someone else if they run into this as well. I look forward to v3. I really appreciate all the work that has been done on this project!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
wontfix
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Redirecting to internal url after login instead of public origin
4 participants
@JessieWadman @AndersAbel @philchuang @snypenet