Add an option to allow redirect of http port 80 to https. #1928

MCF · 2017-06-10T05:17:03Z

This is an "opt in" option (default is to not redirect). It will only redirect
if protocol is https and the new REDIRECT_PORT_80 option is set to true.

Signed-off-by: Mike Fellows [email protected]

This will resolve #1720

bkcsoft · 2017-06-11T07:24:29Z

cmd/web.go

@@ -91,6 +111,9 @@ func runWeb(ctx *cli.Context) error {
 	case setting.HTTP:
 		err = runHTTP(listenAddr, context2.ClearHandler(m))
 	case setting.HTTPS:
+		if setting.RedirectPort80 {
+			go runHTTPRedirector()


Don't fire-n-forget

Can you elaborate on your concern? If the redirector fails to start it will stop the server immediately with the call to log.Fatal right?

Only if runHTTP returns an error. My concern is that this goroutine will stop and then port 80 goes offline. Something needs to keep track of it 🙂

Can you point me at a code example where this sort of thing is being done. I can't see anything like that happening with the other servers binding to ports in web.go. They have the same problem that if they stop then the server will stop working. Presumably in both cases the end users would notice.

The others don't listen in goroutines, so when they error out the whole process errors out. The use of a goroutine here means if/when that listener errors out, it does so silently and without any mechanism for restarting.

Having a loose goroutine like this is risky at best. And will end up with "this **** is broken"-issues that's only solvable by creating a service-handler for tracking long-running goroutines. Which is goind to be a pain :(

Yes I understand that goroutines are threads. And a thread hanging or crashing will cause the service to go away. However if we were to write a thread manager it would presumably required re-engineering the current "main thread" that holds the main gitea web server as well. Even if we did do this you have a similar problem with the actual thread manager. What if it hangs for example and your threads crash for independent reasons?

I don't agree with your assessment that the solution I have chosen for the given feature is risky. It is a exactly 5 lines of code being executed by the redirector service. How much crashing or hanging are we expecting to come out of this? And even if the redirector thread does happen to hang should it be restarted? I suppose something like a DDOS could bring it down, but that seems a stretch. If someone was trying to bring down your site it seems unlikely that they would only be interested in crashing your port 80 to 443 redirector and not attacking your main site on port 443. At a certain point, with these kinds of problems, you need outside tools like pingdom or uptimerobot.

So where does this leave this PR? I've mostly given up on this and accepted that I have to run gitea behind Apache acting as a proxy. It seems odd to have to use a heavyweight server in front of the nice, lightweight gitea web server. If you can give me some specific suggestions about what adjustments are needed to satisfy your concerns please do. Or if you think the feature is not needed, or impossible to implement without radical re-engineering of the main gitea web server, then please close the PR.

bkcsoft · 2017-06-11T07:29:42Z

cmd/web.go

+			target += "?" + r.URL.RawQuery
+		}
+		http.Redirect(w, r, target, http.StatusTemporaryRedirect)
+	})


Just take r.URL and change Schema to https 😕

Thx for the feedback. I had considered the http to https approach but had thought of a configuration with https not on port 443. Then it won't work. Perhaps that particular configuration is so unlikely we don't need to support it?

I for one use it (with a company of 6 people currently). We have multiple https services listening on different ports with IIS redirecting them from port 443 to the appropriate port based on the request's URL, so people don't have to remember the port numbers. We create an additional rule to redirect to https if the service does not have this option, but creating a redirection in IIS is a royal pain, so I would love to be able to do this in Gitea on any possible port.

@bkcsoft your requested change seems to conflict with with the change I just made for @lunny? Can you please re-visit and see what you now think? Also I have an open question above that I was hoping you would comment on.

bkcsoft · 2017-06-15T00:14:33Z

cmd/web.go

@@ -48,6 +48,26 @@ and it takes care of all the other things for you`,
 	},
 }

+func runHTTPRedirector() {
+	source := setting.HTTPAddr + ":80"


Also don't assume port 80 😉

I think that port 80 is a requirement with this feature.

My intention was to address the simple case of someone typing a bare domain name into their web browser location bar, hitting return and ending up at the right place even when gitea is being served on https.

For example a company has an internal server on their intranet with the name: git.example.com. The actually URL is https://git.example.com - but end users may type git.example.com into their location bar and hit return. Without the redirect the browser gives them an error, with the redirect they end up at the right URL.

The port could be another config on app.ini

lunny · 2017-06-15T05:13:03Z

This needs some integration tests and I prefer move this to v1.3.

MCF · 2017-06-22T05:35:41Z

I'll get some tests together for this.

andreynering · 2017-08-13T17:27:30Z

@lunny I don't think this need an integration test. It's a simple feature, and not easy to test, IMHO.

codecov-io · 2017-10-17T17:28:01Z

Codecov Report

Merging #1928 into master will increase coverage by 0.18%.
The diff coverage is 9.52%.

@@            Coverage Diff             @@
##           master    #1928      +/-   ##
==========================================
+ Coverage    34.7%   34.89%   +0.18%     
==========================================
  Files         277      277              
  Lines       40116    40137      +21     
==========================================
+ Hits        13922    14005      +83     
+ Misses      24159    24081      -78     
- Partials     2035     2051      +16

Impacted Files	Coverage Δ
cmd/web.go	`0% <0%> (ø)`	⬆️
modules/setting/setting.go	`47.06% <100%> (+0.13%)`	⬆️
models/lfs.go	`28.26% <0%> (+2.17%)`	⬆️
modules/process/manager.go	`81.15% <0%> (+4.34%)`	⬆️
modules/lfs/server.go	`35.01% <0%> (+14.32%)`	⬆️
modules/lfs/content_store.go	`43.75% <0%> (+35.93%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f5155b9...7c71d19. Read the comment docs.

lunny · 2017-12-19T02:01:44Z

cmd/web.go

@@ -48,6 +48,26 @@ and it takes care of all the other things for you`,
 	},
 }

+func runHTTPRedirector() {
+	source := setting.HTTPAddr + ":80"


The port could be another config on app.ini

This is an "opt in" option (default is to not redirect). It will only redirect if protocol is https and the new REDIRECT_PORT_80 option is set to true. Signed-off-by: Mike Fellows <[email protected]>

lunny · 2017-12-20T02:24:46Z

cmd/web.go

+	var err = runHTTP(source, context2.ClearHandler(handler))
+
+	if err != nil {
+		log.Fatal(4, "Failed to start port 80 redirector: %v", err)


Wrong port in error hint.

lunny · 2017-12-20T02:25:18Z

And I didn't run successfully local. It's strange.

The Port to redirect in previous commit was hardcoded to 80, now it can be specified in the app.ini, defaulting to 80. The boolean option to turn redirection on has been changed to REDIRECT_OTHER_PORT to be logically consistent with the new port option.

lunny · 2017-12-23T06:26:47Z

It works well. LGTM. @bkcsoft needs your review.

bkcsoft · 2017-12-24T05:34:13Z

cmd/web.go

+		if len(r.URL.RawQuery) > 0 {
+			target += "?" + r.URL.RawQuery
+		}
+		http.Redirect(w, r, target, http.StatusTemporaryRedirect)


Why Temporary and not Permanent? Then webbrowsers can go to https automagically afterwards even if they type http 🤔

Temporary redirect also supports POST. I based the redirect logic on this gist: https://gist.github.com/d-schmidt/587ceec34ce1334a5e60

I don't have strong feelings about this either way. It felt more complete to support any type of request. Chrome seems to treat temporary in a similar manner as permanent. Presumably it is doing so with some kind of cache timeout, so you would go back to the http address after a period of time.

Makes sense to me. LGTM

bkcsoft · 2017-12-25T22:16:03Z

LGTM

MCF force-pushed the redirect_port_80 branch 2 times, most recently from 0336833 to 00261ee Compare June 10, 2017 05:48

appleboy added the type/enhancement An improvement of existing functionality label Jun 10, 2017

appleboy added this to the 1.2.0 milestone Jun 10, 2017

bkcsoft suggested changes Jun 11, 2017

View reviewed changes

bkcsoft reviewed Jun 15, 2017

View reviewed changes

tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jun 15, 2017

lunny modified the milestones: 1.3.0, 1.2.0 Jun 18, 2017

MCF force-pushed the redirect_port_80 branch from 00261ee to f2cfa26 Compare June 23, 2017 13:33

MCF force-pushed the redirect_port_80 branch from f2cfa26 to f15f321 Compare July 1, 2017 18:38

MCF force-pushed the redirect_port_80 branch 2 times, most recently from 45c0f3e to 28df9a1 Compare August 13, 2017 16:17

lunny modified the milestones: 1.3.0, 1.x.x Oct 11, 2017

MCF force-pushed the redirect_port_80 branch from 28df9a1 to ea729d7 Compare October 17, 2017 17:20

MCF force-pushed the redirect_port_80 branch from ea729d7 to fc0ff22 Compare December 17, 2017 05:47

lunny requested changes Dec 19, 2017

View reviewed changes

Add an option to allow redirect of http port 80 to https.

d318310

This is an "opt in" option (default is to not redirect). It will only redirect if protocol is https and the new REDIRECT_PORT_80 option is set to true. Signed-off-by: Mike Fellows <[email protected]>

MCF force-pushed the redirect_port_80 branch from fc0ff22 to c14bd84 Compare December 19, 2017 23:33

lunny approved these changes Dec 20, 2017

View reviewed changes

lunny reviewed Dec 20, 2017

View reviewed changes

MCF force-pushed the redirect_port_80 branch from c14bd84 to 929f1b0 Compare December 20, 2017 02:55

lunny removed this from the 1.x.x milestone Dec 23, 2017

lunny added this to the 1.4.0 milestone Dec 23, 2017

tboerger added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Dec 23, 2017

bkcsoft suggested changes Dec 24, 2017

View reviewed changes

bkcsoft approved these changes Dec 25, 2017

View reviewed changes

tboerger added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Dec 25, 2017

Merge branch 'master' into redirect_port_80

7c71d19

bkcsoft merged commit fabf3f2 into go-gitea:master Dec 25, 2017

MCF deleted the redirect_port_80 branch December 27, 2017 02:02

MCF mentioned this pull request Dec 27, 2017

Add docs for REDIRECT_OTHER_PORT and PORT_TO_REDIRECT config options #3262

Merged

go-gitea locked and limited conversation to collaborators Nov 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add an option to allow redirect of http port 80 to https. #1928

Add an option to allow redirect of http port 80 to https. #1928

MCF commented Jun 10, 2017

bkcsoft Jun 11, 2017

MCF Jun 11, 2017

bkcsoft Jun 15, 2017

MCF Jun 22, 2017

rmg Nov 19, 2017

bkcsoft Nov 23, 2017

MCF Dec 17, 2017

bkcsoft Jun 11, 2017

MCF Jun 11, 2017

Avalancs Jun 11, 2017

MCF Dec 19, 2017

bkcsoft Jun 15, 2017

MCF Jun 22, 2017

lunny Dec 19, 2017

lunny commented Jun 15, 2017

MCF commented Jun 22, 2017

andreynering commented Aug 13, 2017

codecov-io commented Oct 17, 2017 •

edited

Loading

lunny Dec 19, 2017

lunny Dec 20, 2017

lunny commented Dec 20, 2017

lunny commented Dec 23, 2017

bkcsoft Dec 24, 2017

MCF Dec 24, 2017

bkcsoft Dec 25, 2017

bkcsoft commented Dec 25, 2017

Add an option to allow redirect of http port 80 to https. #1928

Add an option to allow redirect of http port 80 to https. #1928

Conversation

MCF commented Jun 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lunny commented Jun 15, 2017

MCF commented Jun 22, 2017

andreynering commented Aug 13, 2017

codecov-io commented Oct 17, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lunny commented Dec 20, 2017

lunny commented Dec 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkcsoft commented Dec 25, 2017

codecov-io commented Oct 17, 2017 •

edited

Loading