server: redirect http to https in secure mode

[tamird]

This change is

[knz]

LGTM however I do not see an explicit test for the http redirect itself. Probably doesn't matter too much since you're testing reachability both with and without.

---

Reviewed 10 of 14 files at r1.
Review status: 10 of 14 files reviewed at latest revision, 1 unresolved discussion.

---

resource/test_certs/README.md, line 24 [r1] (raw file):
This seems to belong to a different PR.

---

Comments from the review on Reviewable.io

[tamird]

Yeah the default http client automagically follows redirects. Not sure it matters.

---

Review status: 10 of 14 files reviewed at latest revision, 1 unresolved discussion.

---

resource/test_certs/README.md, line 24 [r1] (raw file):
Meh, it's in a separate commit, and I ended up here because it's the last remaining in-repo resource on generating certificates.

---

Comments from the review on Reviewable.io

[bdarnell]

Review status: 10 of 14 files reviewed at latest revision, 2 unresolved discussions.

---

resource/test_certs/README.md, line 24 [r1] (raw file):
@knz++. I'd much rather have three separate PRs for the three commits in this branch.

---

server/server.go, line 298 [r1] (raw file):
StatusMovedPermanently (302) will only work as expected in this context for GET requests. Non-GETs should use 308 instead. (actually even GETs should use 308, but it's so new that I'm unsure of browser support and so it's safer to stick with 302 for GETs)

---

Comments from the review on Reviewable.io

[tamird]

Review status: 10 of 14 files reviewed at latest revision, 2 unresolved discussions.

---

server/server.go, line 298 [r1] (raw file):
What's the concrete suggestion here? AFAICT, RFC 7538 is not yet standard, and 308 is definitely not yet in the standard library as a constant.

Should I just not redirect on GET? Leave a TODO? Use 308 as a literal?

---

Comments from the review on Reviewable.io

[bdarnell]

Review status: 10 of 14 files reviewed at latest revision, 2 unresolved discussions.

---

server/server.go, line 298 [r1] (raw file):
Once something gets an RFC number, it is for all practical purposes standard (it says "proposed standard" at https://tools.ietf.org/html/rfc7538, but that's just IETF language - it's very slow-moving, and the last RFC to be promoted to a full standard was 5234 in 2008).

I would use 308 as a literal. Or, if you don't want to trust that, return a 400 error for non-GETs.

---

Comments from the review on Reviewable.io

[tamird]

Review status: 10 of 14 files reviewed at latest revision, 2 unresolved discussions.

---

server/server.go, line 298 [r1] (raw file):
Turns out 308 as a literal doesn't work (Go's http client doesn't support it). Added a TODO.

Actually, why is 302 wrong for non-GETs? Seems like clients can follow the redirect with a GET or HEAD, which is better than getting a 400.

---

Comments from the review on Reviewable.io

[petermattis]

Is this meant to fix #5741 (should mention in commit message)? @jseldess This should take care of the "TLS handshake" errors that you see when switching a cluster from insecure to secure.

[tamird]

@petermattis it's in the commit message, just not the PR description.

[petermattis]

@tamird Ok. I wish github would include all of the commit messages in the PR message. Reviewable does a better job at this, but I didn't click through to reviewable.

[tamird]

Still need an lgtm on this one.

[bdarnell]

Review status: 10 of 14 files reviewed at latest revision, 2 unresolved discussions.

---

server/server.go, line 298 [r1] (raw file):
How much do we care about the fact that go's http client doesn't support 308? The main purpose here is to get browsers to do something reasonable. For testing, we don't need the client to follow the redirect, we can just verify that we get status code 308 with an appropriate Location.

302 is bad for non-GETs because it can throw away the original request method and its body (depending on whether the client interprets 302 in the way that the spec says or the way browsers have always done it). There is no guarantee that the path will support both GET and POST (it's common in go's http server interface, but it's rare in some other frameworks). It's better to give a clear error rather than to do something different from what the user asked.

---

Comments from the review on Reviewable.io

[tamird]

Review status: 10 of 14 files reviewed at latest revision, 2 unresolved discussions.

---

server/server.go, line 298 [r1] (raw file):
I was able to find a history of the incorrectness of 302 implementations in the wild, but this is current using 301, not 302. Does that matter?

---

Comments from the review on Reviewable.io

[bdarnell]

Review status: 10 of 14 files reviewed at latest revision, 2 unresolved discussions.

---

server/server.go, line 298 [r1] (raw file):
https://tools.ietf.org/html/rfc7231#section-6.4 says that 301 and 302 have the same method-rewriting behavior in at least some user agents (if it didn't, there would be no need to introduce 308).

I'd also be fine with using 301 for GET and 307 for non-GET. The permanent/temporary distinction matters less than the method preservation.

---

Comments from the review on Reviewable.io

[tamird]

Review status: 10 of 15 files reviewed at latest revision, 2 unresolved discussions.

---

server/server.go, line 298 [r1] (raw file):
OK, I've changed it to 308 and made the appropriate test changes. PTAL

---

Comments from the review on Reviewable.io

[bdarnell]

LGTM

---

Review status: 10 of 15 files reviewed at latest revision, 2 unresolved discussions.

---

Comments from the review on Reviewable.io

[tamird]

@petermattis @jseldess this isn't guaranteed to take care of the "TLS handshake" errors you're seeing - many of those are triggered by XHRs which don't honor redirects. Also, since client browsers do not trust our CA, a "TLS handshake" error (or two) will be generated on that first redirect when chrome kills the connection upon discovering the untrusted certificate.

[jseldess]

Thanks for the heads up, @tamird. Once this is merged, I'll retest our getting started steps and figure out if/how they should be updated.

[jseldess]

Still seeing "TLS handshake error" when switching cluster from insecure to secure with the admin ui up. I guess I'll just leave the docs as they are (suggesting to close the admin ui before restarting an insecure cluster as secure).

[tamird]

Yes, that's what I meant by XHRs - if you have the admin UI up through a
"switch" you'll definitely continue to see errors.

On Thu, Mar 31, 2016 at 10:02 PM, jseldess notifications@github.com wrote:

Still seeing "TLS handshake error" still there when switching cluster from
insecure to secure with the admin ui up. I guess I'll just leave the docs
as they are (suggesting to close the admin ui before restarting an insecure
cluster as secure).

—
You are receiving this because you modified the open/close state.
Reply to this email directly or view it on GitHub
#5746 (comment)