Merge pull request #3787 from aledbf/port-in-redirect

Use UsePortInRedirects only if enabled
kubernetes · Feb 21, 2019 · f04361c · f04361c
2 parents fb1df21 + 8b6e4d4
commit f04361c
Show file tree

Hide file tree

Showing 3 changed files with 81 additions and 5 deletions.
diff --git a/rootfs/etc/nginx/template/nginx.tmpl b/rootfs/etc/nginx/template/nginx.tmpl
@@ -1126,14 +1126,25 @@ stream {
             {{ if not (isLocationInLocationList $location $all.Cfg.NoTLSRedirectLocations) }}
             # enforce ssl on server side
             if ($redirect_to_https) {
+                set_by_lua_block $redirect_host {
+                    local ngx_re = require "ngx.re"
+
+                    local host_port, err = ngx_re.split(ngx.var.best_http_host, ":")
+                    if err then
+                        ngx.log(ngx.ERR, "could not parse variable: ", err)
+                        return ngx.var.best_http_host;
+                    end
+
+                    return host_port[1];
+                }
+
                 {{ if $location.UsePortInRedirects }}
                 # using custom ports require a different rewrite directive
-                {{ $redirect_port := (printf ":%v" $all.ListenPorts.HTTPS) }}
-                error_page 497 ={{ $all.Cfg.HTTPRedirectCode }} https://$host{{ $redirect_port }}$request_uri;
-
+                # https://forum.nginx.org/read.php?2,155978,155978#msg-155978
+                error_page 497 ={{ $all.Cfg.HTTPRedirectCode }} https://$redirect_host{{ printf ":%v" $all.ListenPorts.HTTPS }}$request_uri;
                 return 497;
                 {{ else }}
-                return {{ $all.Cfg.HTTPRedirectCode }} https://$best_http_host$request_uri;
+                return {{ $all.Cfg.HTTPRedirectCode }} https://$redirect_host$request_uri;
                 {{ end }}
             }
             {{ end }}

diff --git a/test/e2e/annotations/forcesslredirect.go b/test/e2e/annotations/forcesslredirect.go
@@ -49,7 +49,7 @@ var _ = framework.IngressNginxDescribe("Annotations - Forcesslredirect", func()
 		f.WaitForNginxServer(host,
 			func(server string) bool {
 				return Expect(server).Should(ContainSubstring(`if ($redirect_to_https) {`)) &&
-					Expect(server).Should(ContainSubstring(`return 308 https://$best_http_host$request_uri;`))
+					Expect(server).Should(ContainSubstring(`return 308 https://$redirect_host$request_uri;`))
 			})
 
 		resp, _, errs := gorequest.New().

diff --git a/test/e2e/settings/tls.go b/test/e2e/settings/tls.go
@@ -21,6 +21,7 @@ import (
 	"fmt"
 	"net/http"
 	"strings"
+	"time"
 
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
@@ -29,12 +30,17 @@ import (
 	"k8s.io/ingress-nginx/test/e2e/framework"
 )
 
+func noRedirectPolicyFunc(gorequest.Request, []gorequest.Request) error {
+	return http.ErrUseLastResponse
+}
+
 var _ = framework.IngressNginxDescribe("Settings - TLS)", func() {
 	f := framework.NewDefaultFramework("settings-tls")
 	host := "settings-tls"
 
 	BeforeEach(func() {
 		f.NewEchoDeployment()
+		f.UpdateNginxConfigMapData("use-forwarded-headers", "false")
 	})
 
 	AfterEach(func() {
@@ -164,4 +170,63 @@ var _ = framework.IngressNginxDescribe("Settings - TLS)", func() {
 		Expect(resp.StatusCode).Should(Equal(http.StatusOK))
 		Expect(resp.Header.Get("Strict-Transport-Security")).Should(ContainSubstring("preload"))
 	})
+
+	It("should not use ports during the HTTP to HTTPS redirection", func() {
+		ing := f.EnsureIngress(framework.NewSingleIngressWithTLS(host, "/", host, []string{host}, f.IngressController.Namespace, "http-svc", 80, nil))
+		tlsConfig, err := framework.CreateIngressTLSSecret(f.KubeClientSet,
+			ing.Spec.TLS[0].Hosts,
+			ing.Spec.TLS[0].SecretName,
+			ing.Namespace)
+		Expect(err).NotTo(HaveOccurred())
+
+		framework.WaitForTLS(f.IngressController.HTTPSURL, tlsConfig)
+
+		f.WaitForNginxServer(host,
+			func(server string) bool {
+				return Expect(server).Should(ContainSubstring(`if ($redirect_to_https) {`)) &&
+					Expect(server).Should(ContainSubstring(`return 308 https://$redirect_host$request_uri;`))
+			})
+
+		resp, _, errs := gorequest.New().
+			Get(fmt.Sprintf(f.IngressController.HTTPURL)).
+			Retry(10, 1*time.Second, http.StatusNotFound).
+			RedirectPolicy(noRedirectPolicyFunc).
+			Set("Host", host).
+			End()
+
+		Expect(errs).Should(BeEmpty())
+		Expect(resp.StatusCode).Should(Equal(http.StatusPermanentRedirect))
+		Expect(resp.Header.Get("Location")).Should(Equal(fmt.Sprintf("https://%v/", host)))
+	})
+
+	It("should not use ports or X-Forwarded-Host during the HTTP to HTTPS redirection", func() {
+		f.UpdateNginxConfigMapData("use-forwarded-headers", "true")
+
+		ing := f.EnsureIngress(framework.NewSingleIngressWithTLS(host, "/", host, []string{host}, f.IngressController.Namespace, "http-svc", 80, nil))
+		tlsConfig, err := framework.CreateIngressTLSSecret(f.KubeClientSet,
+			ing.Spec.TLS[0].Hosts,
+			ing.Spec.TLS[0].SecretName,
+			ing.Namespace)
+		Expect(err).NotTo(HaveOccurred())
+
+		framework.WaitForTLS(f.IngressController.HTTPSURL, tlsConfig)
+
+		f.WaitForNginxServer(host,
+			func(server string) bool {
+				return Expect(server).Should(ContainSubstring(`if ($redirect_to_https) {`)) &&
+					Expect(server).Should(ContainSubstring(`return 308 https://$redirect_host$request_uri;`))
+			})
+
+		resp, _, errs := gorequest.New().
+			Get(fmt.Sprintf(f.IngressController.HTTPURL)).
+			Retry(10, 1*time.Second, http.StatusNotFound).
+			RedirectPolicy(noRedirectPolicyFunc).
+			Set("Host", host).
+			Set("X-Forwarded-Host", "example.com:80").
+			End()
+
+		Expect(errs).Should(BeEmpty())
+		Expect(resp.StatusCode).Should(Equal(http.StatusPermanentRedirect))
+		Expect(resp.Header.Get("Location")).Should(Equal("https://example.com/"))
+	})
 })