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

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

Merged
merged 3 commits into from Dec 25, 2017

Conversation

@MCF
Contributor

MCF commented Jun 10, 2017

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 mike.fellows@shaw.ca

This will resolve #1720

@MCF MCF force-pushed the MCF:redirect_port_80 branch 2 times, most recently from 0336833 to 00261ee Jun 10, 2017

@appleboy appleboy added this to the 1.2.0 milestone Jun 10, 2017

cmd/web.go Outdated
@@ -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()

This comment has been minimized.

@bkcsoft

bkcsoft Jun 11, 2017

Member

Don't fire-n-forget

This comment has been minimized.

@MCF

MCF Jun 11, 2017

Contributor

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?

This comment has been minimized.

@bkcsoft

bkcsoft Jun 15, 2017

Member

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 🙂

This comment has been minimized.

@MCF

MCF Jun 22, 2017

Contributor

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.

This comment has been minimized.

@rmg

rmg Nov 19, 2017

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.

This comment has been minimized.

@bkcsoft

bkcsoft Nov 23, 2017

Member

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 :(

This comment has been minimized.

@MCF

MCF Dec 17, 2017

Contributor

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.

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

This comment has been minimized.

@bkcsoft

bkcsoft Jun 11, 2017

Member

Just take r.URL and change Schema to https 😕

This comment has been minimized.

@MCF

MCF Jun 11, 2017

Contributor

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?

This comment has been minimized.

@Avalancs

Avalancs Jun 11, 2017

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.

This comment has been minimized.

@MCF

MCF Dec 19, 2017

Contributor

@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.

cmd/web.go Outdated
@@ -48,6 +48,26 @@ and it takes care of all the other things for you`,
},
}
func runHTTPRedirector() {
source := setting.HTTPAddr + ":80"

This comment has been minimized.

@bkcsoft

bkcsoft Jun 15, 2017

Member

Also don't assume port 80 😉

This comment has been minimized.

@MCF

MCF Jun 22, 2017

Contributor

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.

This comment has been minimized.

@lunny

lunny Dec 19, 2017

Member

The port could be another config on app.ini

@lunny

This comment has been minimized.

Member

lunny commented Jun 15, 2017

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

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

@MCF

This comment has been minimized.

Contributor

MCF commented Jun 22, 2017

I'll get some tests together for this.

@MCF MCF force-pushed the MCF:redirect_port_80 branch from 00261ee to f2cfa26 Jun 23, 2017

@MCF MCF force-pushed the MCF:redirect_port_80 branch from f2cfa26 to f15f321 Jul 1, 2017

@MCF MCF force-pushed the MCF:redirect_port_80 branch 2 times, most recently from 45c0f3e to 28df9a1 Aug 6, 2017

@andreynering

This comment has been minimized.

Contributor

andreynering commented Aug 13, 2017

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

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

@MCF MCF force-pushed the MCF:redirect_port_80 branch from 28df9a1 to ea729d7 Oct 17, 2017

@codecov-io

This comment has been minimized.

codecov-io commented Oct 17, 2017

Codecov Report

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

Impacted file tree graph

@@            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.

@MCF MCF force-pushed the MCF:redirect_port_80 branch from ea729d7 to fc0ff22 Dec 17, 2017

cmd/web.go Outdated
@@ -48,6 +48,26 @@ and it takes care of all the other things for you`,
},
}
func runHTTPRedirector() {
source := setting.HTTPAddr + ":80"

This comment has been minimized.

@lunny

lunny Dec 19, 2017

Member

The port could be another config on app.ini

Add an option to allow redirect of http port 80 to https.
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 <mike.fellows@shaw.ca>

@MCF MCF force-pushed the MCF:redirect_port_80 branch from fc0ff22 to c14bd84 Dec 19, 2017

@lunny

lunny approved these changes Dec 20, 2017

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

This comment has been minimized.

@lunny

lunny Dec 20, 2017

Member

Wrong port in error hint.

@lunny

This comment has been minimized.

Member

lunny commented Dec 20, 2017

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

Change redirector setting name, add a port to redirect setting
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.

@MCF MCF force-pushed the MCF:redirect_port_80 branch from c14bd84 to 929f1b0 Dec 20, 2017

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

@lunny lunny added this to the 1.4.0 milestone Dec 23, 2017

@lunny

This comment has been minimized.

Member

lunny commented Dec 23, 2017

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

@tboerger tboerger added lgtm/need 1 and removed lgtm/need 2 labels Dec 23, 2017

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

This comment has been minimized.

@bkcsoft

bkcsoft Dec 24, 2017

Member

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

This comment has been minimized.

@MCF

MCF Dec 24, 2017

Contributor

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.

This comment has been minimized.

@bkcsoft

bkcsoft Dec 25, 2017

Member

Makes sense to me. LGTM

@bkcsoft

This comment has been minimized.

Member

bkcsoft commented Dec 25, 2017

LGTM

@tboerger tboerger added lgtm/done and removed lgtm/need 1 labels Dec 25, 2017

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

3 checks passed

Codacy/PR Quality Review Good work! A positive pull request.
Details
approvals/lgtm this commit looks good
continuous-integration/drone/pr the build was successful
Details

@MCF MCF deleted the MCF:redirect_port_80 branch Dec 27, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment