search

aspnet / BasicMiddleware

[Archived] Basic middleware components for ASP.NET Core. Project moved to https://github.com/aspnet/AspNetCore
Apache License 2.0
169 stars 84 forks source link

Make HttpRedirectionMiddleware not redirect to https if no HttpsPort is provided. #318

Closed jkotalik closed 6 years ago

jkotalik commented 6 years ago

Fundamental issue is that apps will break if deployed to production that have not configured HTTPS (other than antares). At the last step of the middleware port checking, we currently set the HTTPS port to redirect to 443 if no port is found. Instead of that behavior, if no port is found to redirect to, we simply do not redirect the HttpRequest.

Tratcher commented 6 years ago

No, this fails every reverse proxy scenario.

jkotalik commented 6 years ago

For reverse proxy scenarios, we may need people to explicitly set the port (443 for instance).

Tratcher commented 6 years ago

A security feature like this can't silently turn itself off. It's ok to have an off switch, but it can't turn itself off.

@blowdart

blowdart commented 6 years ago

What @Tratcher said, except with more "Dear god no" screaming

jkotalik commented 6 years ago

cc/ @DamianEdwards @davidfowl @javiercn

Tratcher commented 6 years ago

Note the no-https template is the primary mitigation for this issue. If you build an HTTPs app and deploy it to a non-https environment it's supposed to fail.

Eilon commented 6 years ago

Also should log at Debug/Trace if the HTTPS redirect middleware ran but did not redirect.

Tratcher commented 6 years ago

And by did not redirect, you mean because it was already an https request? Or because it was turned off for some reason?

Eilon commented 6 years ago

No logging if it's already HTTPS. We thought it ought to log if it couldn't redirect because it couldn't find the "desired" port in the configuration (or wherever).

Tratcher commented 6 years ago

No, that's a fatal configuration error that should be crashing the app. Edit: unless this is for the default 443 fallback where it redirects anyways?

DamianEdwards commented 6 years ago

Nah, don't agree.

muratg commented 6 years ago

Not doing this in RC1.

Current behavior has some usability issues that can be discovered pretty quickly by our users.

javiercn commented 6 years ago

Current behavior has some usability issues that can be discovered pretty quickly by our users.

@muratg What does this mean?

DamianEdwards commented 6 years ago

Indeed, why is this punted?

Eilon commented 6 years ago

Yeah per our mtg the other day we need to do this in 2.1.0.

henkmollema commented 6 years ago

Perhaps this warrants an announcement? Applications upgrading from preview2 to RC1 suddenly don't redirect to HTTPS anymore with the default settings.

DamianEdwards commented 6 years ago

Agreed. Justin or Chris, could you post an announcement of this change in the RC1 milestone please.

jkotalik commented 6 years ago

I'll go ahead and post one.

Tratcher commented 6 years ago

https://github.com/aspnet/Announcements/issues/301

henkmollema commented 6 years ago

Thanks Justin!