-
-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
Conversation
0336833
to
00261ee
Compare
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() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't fire-n-forget
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just take r.URL
and change Schema to https
😕
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also don't assume port 80 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The port could be another config on app.ini
This needs some integration tests and I prefer move this to v1.3. |
I'll get some tests together for this. |
45c0f3e
to
28df9a1
Compare
@lunny I don't think this need an integration test. It's a simple feature, and not easy to test, IMHO. |
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
ea729d7
to
fc0ff22
Compare
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" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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]>
fc0ff22
to
c14bd84
Compare
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrong port in error hint.
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.
c14bd84
to
929f1b0
Compare
It works well. LGTM. @bkcsoft needs your review. |
if len(r.URL.RawQuery) > 0 { | ||
target += "?" + r.URL.RawQuery | ||
} | ||
http.Redirect(w, r, target, http.StatusTemporaryRedirect) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why Temporary and not Permanent? Then webbrowsers can go to https automagically afterwards even if they type http 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to me. LGTM
LGTM |
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