Skip to content

Make HttpsRedirectionMiddleware throw if there are multiple ports #29166

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

Merged
merged 1 commit into from
Jan 11, 2021

Conversation

Tratcher
Copy link
Member

@Tratcher Tratcher commented Jan 8, 2021

Fixes #27951

When the HttpsRedirectionMiddleware isn't explicitly configured it will search IServerAddresses to figure out what https port it should redirect to. This doesn't work if there are A) no https address or, B) multiple https addresses with different ports.

When this feature was first designed we decided that case A should no-op to avoid breaking scenarios like development, deployment behind proxies, etc.. Case B was treated the same way, but I don't think we explored that scenario enough.

When IServerAddresses reports multiple distinct https ports it's clear that the site is using https and that the middleware should be active to protect sensitive data, but it's ambiguous which port it should redirect to. The customer feedback for this scenario is that they'd rather fail the request then let it succeed silently and risk data exposure.

#21291 is for follow-up work to let the developer resolve the port ambiguity, but this error would still be needed when that advanced config is not provided.

@Tratcher Tratcher added the breaking-change This issue / pr will introduce a breaking change, when resolved / merged. label Jan 8, 2021
@Tratcher Tratcher added this to the 6.0-preview1 milestone Jan 8, 2021
@Tratcher Tratcher self-assigned this Jan 8, 2021
@Tratcher Tratcher requested a review from jkotalik as a code owner January 8, 2021 23:40
@ghost ghost added the area-middleware label Jan 8, 2021
@Tratcher Tratcher requested a review from halter73 January 8, 2021 23:41
@jkotalik
Copy link
Contributor

Should we bring this to API review?

@Tratcher
Copy link
Member Author

Tratcher commented Jan 11, 2021

Should we bring this to API review?

It's not an API change. I don't think we have a process for reviewing these types of changes beyond code review.

Note I did review it with Barry in advance.

@jkotalik
Copy link
Contributor

Okay, probably an announcement is a good idea. Also, does this break any default scenarios on app service (windows or linux)?

@Tratcher
Copy link
Member Author

Okay, probably an announcement is a good idea.

I'll do that next.

Also, does this break any default scenarios on app service (windows or linux)?

It shouldn't, they either terminate TLS at the proxy or only have one https port. I've only seen self-hosters get into this situation with multiple https ports.

@jkotalik
Copy link
Contributor

Yeah, I think the scenario where this breaks on app services is when there are not HTTPS port.

@Tratcher
Copy link
Member Author

It will continue to default to off when there are no https ports.

Doesn't app service enable https by default now?

@jkotalik
Copy link
Contributor

I don't think so, IIRC the main concern was app service for linux.

@Tratcher
Copy link
Member Author

App service for linux terminates TLS at the proxy. We had lots of issues there with the forwarders, as well as getting the port. We got redirection working by specifying the proxy https port in the config (container?).

@Tratcher Tratcher merged commit 5281367 into dotnet:master Jan 11, 2021
@Tratcher Tratcher deleted the tratcher/https-fail branch January 11, 2021 22:50
@Tratcher
Copy link
Member Author

Announced: aspnet/Announcements#448

@ghost
Copy link

ghost commented Jan 11, 2021

Hi @Tratcher. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

@amcasey amcasey added the area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlewares label Jun 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlewares breaking-change This issue / pr will introduce a breaking change, when resolved / merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HttpsRedirectionMiddleware unsafe fallback behavior when app is misconfigured
4 participants