Skip to content
This repository was archived by the owner on Jan 16, 2021. It is now read-only.

enforce HTTPS #322

Merged
merged 1 commit into from
Sep 28, 2015
Merged

enforce HTTPS #322

merged 1 commit into from
Sep 28, 2015

Conversation

jmhodges
Copy link
Contributor

Redirect HTTP links to HTTPS and set HSTS correctly.

This is specific to the godoc.org set up (with nginx passing a X-Scheme header back) and without fixing up api.godoc.org.

Fixes #304.

@@ -0,0 +1,7 @@
// +build appengine

package main
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, godoc.org doesn't run on app engine anymore, so this is unnecessary.

@jmhodges
Copy link
Contributor Author

I could be convinced that the appengine default code shouldn't be in there, but it followed what was done in the commit linked to in the latest comment on 304.

@@ -898,7 +900,17 @@ func main() {

cacheBusters.Handler = mux

if err := http.ListenAndServe(*httpAddr, hostMux{{"api.", apiMux}, {"", mux}}); err != nil {
var allMux http.Handler = hostMux{{"api.", apiMux}, {"", mux}}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/allMux/mux/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The mux name is already taken by the {"", mux} entry.

I could rename that to webMux or something, but that was a slightly larger change. Totally up for it, though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

allMux is fine

@adg
Copy link
Contributor

adg commented Sep 25, 2015

I think this could all be simpler.

We should just check whether the host is "godoc.org" and enforce TLS in that case only. Then we don't need the flag either.

@adg
Copy link
Contributor

adg commented Sep 25, 2015

Yes, please drop the app engine stuff.

@jmhodges
Copy link
Contributor Author

Okay, so I was hoping to get HTTPS enforced even on a installation of gddo-server that was not on appengine or on godoc.org.

Unfortunately, some clients will include Host headers that have ports in them which the Go HTTP library does not remove. See golang/go#10463. (I'm guessing those ports are removed up by the appengine frontends or else that other bit of code wouldn't behave correctly.)

Also, in order to match all of the domains, this code would have have to go inside the hostMux to work correctly or do a host == "godoc.org" || strings.HasSuffix(host, ".godoc.org") which feels a little silly.

I'm totally willing to accept push back on that, but it would be a bit of a bummer.

@adg
Copy link
Contributor

adg commented Sep 25, 2015

"godoc.org" is the only valid domain for the service. Let's restrict the scope of this issue to making godoc.org HTTPS-only. We can worry about other users another time.

@adg
Copy link
Contributor

adg commented Sep 25, 2015

Hold up. I just realized that since godoc.org is deployed behind nginx that this entire effort is futile. request.TLS will never ben non-nil, as the nginx front end calls the gddo-server instance via plain text HTTP.

I guess I'll have to fix this on the deployment side. Sorry for wasting your time. :-(

@jmhodges
Copy link
Contributor Author

Oh, shoot. Sorry for wasting yours with a bunch of back and forth!

If nginx sets (and clears from outside requests) X-Forward-Proto, we can use that.

@adg
Copy link
Contributor

adg commented Sep 25, 2015

Let me investigate.

@adg
Copy link
Contributor

adg commented Sep 25, 2015

The relevant header is X-Scheme. We should be able to test that against https and do the redirect. Note that we cannot do this for api.godoc.org because our TLS cert does not cover that sub-domain (that's another issue).

@jmhodges
Copy link
Contributor Author

Okay, pushed.

I'd be willing to write code for the api.godoc.org cert in another PR if that's what's needed.

Redirect HTTP links to HTTPS and set HSTS correctly.

This is specific to the godoc.org set up (with nginx passing a X-Scheme
header back) and without fixing up api.godoc.org.

Fixes golang#304.
@jmhodges
Copy link
Contributor Author

I've blown out my timebox for this change for now. Feel free to pick up where I left off.

@adg adg merged commit c291f4d into golang:master Sep 28, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make godoc.org HTTPS-only
2 participants