add https redirect to deck #6081

BenTheElder · 2017-12-23T00:07:12Z

/area prow
fixes #5282

move all HTTP handling to a non-default mux
add a flag (disabled by default) to enable injecting an https redirect handler mux over the "real" mux

BenTheElder · 2017-12-23T00:07:28Z

once this looks OK I'll bump deck and then we can merge / deploy

BenTheElder · 2017-12-23T00:16:10Z

We could also muck with the ingress or something, but all we need here is to optionally dissallow non-https in deck (by always redirecting anything else), which gives some nicer guarantees hopefully and isn't much code.

cjwagner

Does this need a test?

cjwagner · 2017-12-23T01:11:18Z

prow/cmd/deck/main.go

+					r.URL.Scheme = "https"
+					http.Redirect(w, r, r.URL.String(), http.StatusMovedPermanently)
+				} else {
+					mux.ServeHTTP(w, r)


s/mux/oldMux/
This would create an infinite recursion loop as is because this mux reference is actually captured so after line 127 mux==redirectMux 😄

whoops, yeah originally I wasn't capturing, and then I realized I couldn't swap them, will fix.

BenTheElder · 2017-12-23T01:54:28Z

Does this need a test?

I don't think so, there's not much useful to assert here, and it's not a critical behavior.

0xmichalis · 2017-12-23T09:16:38Z

This should be an option in Ingress and not something codified in prow. In Openshift, we use Openshift Routes (predecessor to k8s Ingress) for all external traffic to prow and the redirects happen automatically by the router.

spxtr · 2017-12-23T10:03:41Z

prow/cmd/deck/main.go

+	buildCluster    = flag.String("build-cluster", "", "Path to file containing a YAML-marshalled kube.Cluster object. If empty, uses the local cluster.")
+	tideURL         = flag.String("tide-url", "", "Path to tide. If empty, do not serve tide data.")
+	hookURL         = flag.String("hook-url", "", "Path to hook plugin help endpoint.")
+	redirectToHTTPS = flag.Bool("redirect-to-https", false, "Enable http to https redirect.")


I prefer a config option for this, but I'm not 100% sure.

BenTheElder · 2017-12-23T16:54:41Z

It *should*, but I don't want to add another server to our deployment just for such a trivial redirect so I've essentially implemented kubernetes/ingress-gce#51 (comment) and placed it behind a flag.

…

On Sat, Dec 23, 2017, 01:16 Michalis Kargakis ***@***.***> wrote: This should be an option in Ingress and not something codified in prow. In Openshift, we use Openshift Routes (predecessor to k8s Ingress) for all external traffic to prow and the redirects happen automatically by the router. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#6081 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA4Bq5UF3ilE_FnpHqIVAy0B-it5Rqtuks5tDMT3gaJpZM4RLg2m> .

BenTheElder · 2017-12-23T17:08:45Z

/hold
I actually probably need to do x-forwarded-for instead of the scheme 🤦‍♂️
I would still prefer that our single user facing web server in prow be able to require https without an extra router just for this. It's not much code and will be cheaper to run and maintain.

spxtr · 2017-12-25T20:42:29Z

I think it's pretty standard to use something like nginx or openshift routes or whatever your cloud supports for HTTPS redirection. On GCE it seems like nginx is the norm.

I don't want to add another server to our deployment just for such a trivial redirect

I hear you, but I actually prefer it to writing our own code. Because this is a pretty common pattern for k8s/GKE users, we should show how simple it is to do with kubernetes :)

BenTheElder · 2017-12-27T18:32:49Z

I hear you, but I actually prefer it to writing our own code. Because this is a pretty common pattern for k8s/GKE users, we should show how simple it is to do with kubernetes :)

Writing an NGINX rule is code :-) Optionally handling x-forwarded-proto doesn't really justify more latency and operational complexity imho. It's actually going to be more LOC to add an NGINX rule in the middle (including the nginx deployment etc.) ...

GCE-Ingress / Load Balancer -> kube-proxy / iptables -> nginx -> kube-proxy -> kube-proxy (if on another node) -> deck and back again is pretty overkill just to gain a single redirect. If we were using an ingress-nginx it would be simple.

Really the GCE ingress / load balancer should support this :(

cblecker · 2017-12-30T01:13:36Z

prow/cmd/deck/main.go

+	buildCluster    = flag.String("build-cluster", "", "Path to file containing a YAML-marshalled kube.Cluster object. If empty, uses the local cluster.")
+	tideURL         = flag.String("tide-url", "", "Path to tide. If empty, do not serve tide data.")
+	hookURL         = flag.String("hook-url", "", "Path to hook plugin help endpoint.")
+	redirectToHTTPS = flag.Bool("redirect-to-https", false, "Enable x-forwarded-for http to https redirect.")


x-forwarded-for != x-forwarded-proto

oops, thanks.

cjwagner

/lgtm

k8s-ci-robot · 2018-01-02T19:58:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: BenTheElder, cjwagner

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~prow/OWNERS~~ [BenTheElder,cjwagner]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

BenTheElder · 2018-01-02T21:06:13Z

This has a key (and rather stupid) mistake: it needs to redirect to the external domain, not the one in the forwarded in the request. This is an easy fix.

BenTheElder requested review from cjwagner and 0xmichalis as code owners December 23, 2017 00:07

BenTheElder requested a review from spxtr December 23, 2017 00:13

cjwagner reviewed Dec 23, 2017

View reviewed changes

BenTheElder force-pushed the secure-the-deck branch from 45fde52 to d042d11 Compare December 23, 2017 01:51

spxtr reviewed Dec 23, 2017

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 23, 2017

BenTheElder force-pushed the secure-the-deck branch from d042d11 to fac9917 Compare December 27, 2017 18:26

BenTheElder removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 27, 2017

cblecker reviewed Dec 30, 2017

View reviewed changes

BenTheElder force-pushed the secure-the-deck branch from fac9917 to 3f973b4 Compare January 2, 2018 19:55

BenTheElder added 2 commits January 2, 2018 11:56

add https redirect to deck

b219a25

bump deck

5904f77

BenTheElder force-pushed the secure-the-deck branch from 3f973b4 to 5904f77 Compare January 2, 2018 19:58

cjwagner approved these changes Jan 2, 2018

View reviewed changes

k8s-ci-robot assigned cjwagner Jan 2, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 2, 2018

k8s-ci-robot merged commit 43b0134 into kubernetes:master Jan 2, 2018

BenTheElder deleted the secure-the-deck branch January 2, 2018 20:24

BenTheElder mentioned this pull request Jan 2, 2018

Revert "add https redirect to deck" #6120

Merged

BenTheElder mentioned this pull request Jan 2, 2018

add https redirect to deck #6122

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add https redirect to deck #6081

add https redirect to deck #6081

BenTheElder commented Dec 23, 2017 •

edited

Loading

BenTheElder commented Dec 23, 2017

BenTheElder commented Dec 23, 2017

cjwagner left a comment

cjwagner Dec 23, 2017

BenTheElder Dec 23, 2017

BenTheElder Dec 23, 2017

BenTheElder commented Dec 23, 2017

0xmichalis commented Dec 23, 2017

spxtr Dec 23, 2017

BenTheElder commented Dec 23, 2017 via email

BenTheElder commented Dec 23, 2017

spxtr commented Dec 25, 2017

BenTheElder commented Dec 27, 2017

cblecker Dec 30, 2017

BenTheElder Jan 2, 2018

cjwagner left a comment

k8s-ci-robot commented Jan 2, 2018

BenTheElder commented Jan 2, 2018

add https redirect to deck #6081

add https redirect to deck #6081

Conversation

BenTheElder commented Dec 23, 2017 • edited Loading

BenTheElder commented Dec 23, 2017

BenTheElder commented Dec 23, 2017

cjwagner left a comment

Choose a reason for hiding this comment

cjwagner Dec 23, 2017

Choose a reason for hiding this comment

BenTheElder Dec 23, 2017

Choose a reason for hiding this comment

BenTheElder Dec 23, 2017

Choose a reason for hiding this comment

BenTheElder commented Dec 23, 2017

0xmichalis commented Dec 23, 2017

spxtr Dec 23, 2017

Choose a reason for hiding this comment

BenTheElder commented Dec 23, 2017 via email

BenTheElder commented Dec 23, 2017

spxtr commented Dec 25, 2017

BenTheElder commented Dec 27, 2017

cblecker Dec 30, 2017

Choose a reason for hiding this comment

BenTheElder Jan 2, 2018

Choose a reason for hiding this comment

cjwagner left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jan 2, 2018

BenTheElder commented Jan 2, 2018

BenTheElder commented Dec 23, 2017 •

edited

Loading