Merge pull request #3637 from aledbf/fix-redirect

Add support for redirect https to https (from-to-www-redirect)

docs/user-guide/nginx-configuration/annotations.md

@@ -465,7 +465,7 @@ When using SSL offloading outside of cluster (e.g. AWS ELB) it may be useful to
 even when there is no TLS certificate available.
 This can be achieved by using the `nginx.ingress.kubernetes.io/force-ssl-redirect: "true"` annotation in the particular resource.
 
-### Redirect from/to www.
+### Redirect from/to www
 
 In some scenarios is required to redirect from `www.domain.com` to `domain.com` or vice versa.
 To enable this feature use the annotation `nginx.ingress.kubernetes.io/from-to-www-redirect: "true"`
@@ -473,6 +473,9 @@ To enable this feature use the annotation `nginx.ingress.kubernetes.io/from-to-w
 !!! attention
     If at some point a new Ingress is created with a host equal to one of the options (like `domain.com`) the annotation will be omitted.
 
+!!! attention
+    For HTTPS to HTTPS redirects is mandatory the SSL Certificate defined in the Secret, located in the TLS section of Ingress, contains both FQDN in the common name of the certificate.
+
 ### Whitelist source range
 
 You can specify allowed client IP source ranges through the `nginx.ingress.kubernetes.io/whitelist-source-range` annotation.

internal/ingress/controller/config/config.go

@@ -721,7 +721,7 @@ type TemplateConfig struct {
 	IsSSLPassthroughEnabled    bool
 	NginxStatusIpv4Whitelist   []string
 	NginxStatusIpv6Whitelist   []string
-	RedirectServers            map[string]string
+	RedirectServers            interface{}
 	ListenPorts                *ListenPorts
 	PublishService             *apiv1.Service
 	DynamicCertificatesEnabled bool

internal/ingress/controller/nginx.go

@@ -38,6 +38,7 @@ import (
 	"github.com/eapache/channels"
 	apiv1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/util/intstr"
+	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/client-go/kubernetes/scheme"
 	v1core "k8s.io/client-go/kubernetes/typed/core/v1"
 	"k8s.io/client-go/tools/record"
@@ -498,39 +499,20 @@ func (n *NGINXController) OnUpdate(ingressCfg ingress.Configuration) error {
 	// https://trac.nginx.org/nginx/ticket/631
 	var longestName int
 	var serverNameBytes int
-	redirectServers := make(map[string]string)
+
 	for _, srv := range ingressCfg.Servers {
 		if longestName < len(srv.Hostname) {
 			longestName = len(srv.Hostname)
 		}
 		serverNameBytes += len(srv.Hostname)
-		if srv.RedirectFromToWWW {
-			var n string
-			if strings.HasPrefix(srv.Hostname, "www.") {
-				n = strings.TrimPrefix(srv.Hostname, "www.")
-			} else {
-				n = fmt.Sprintf("www.%v", srv.Hostname)
-			}
-			klog.V(3).Infof("Creating redirect from %q to %q", srv.Hostname, n)
-			if _, ok := redirectServers[n]; !ok {
-				found := false
-				for _, esrv := range ingressCfg.Servers {
-					if esrv.Hostname == n {
-						found = true
-						break
-					}
-				}
-				if !found {
-					redirectServers[n] = srv.Hostname
-				}
-			}
-		}
 	}
+
 	if cfg.ServerNameHashBucketSize == 0 {
 		nameHashBucketSize := nginxHashBucketSize(longestName)
 		klog.V(3).Infof("Adjusting ServerNameHashBucketSize variable to %d", nameHashBucketSize)
 		cfg.ServerNameHashBucketSize = nameHashBucketSize
 	}
+
 	serverNameHashMaxSize := nextPowerOf2(serverNameBytes)
 	if cfg.ServerNameHashMaxSize < serverNameHashMaxSize {
 		klog.V(3).Infof("Adjusting ServerNameHashMaxSize variable to %d", serverNameHashMaxSize)
@@ -619,7 +601,7 @@ func (n *NGINXController) OnUpdate(ingressCfg ingress.Configuration) error {
 		IsIPV6Enabled:              n.isIPV6Enabled && !cfg.DisableIpv6,
 		NginxStatusIpv4Whitelist:   cfg.NginxStatusIpv4Whitelist,
 		NginxStatusIpv6Whitelist:   cfg.NginxStatusIpv6Whitelist,
-		RedirectServers:            redirectServers,
+		RedirectServers:            buildRedirects(ingressCfg.Servers),
 		IsSSLPassthroughEnabled:    n.cfg.EnableSSLPassthrough,
 		ListenPorts:                n.cfg.ListenPorts,
 		PublishService:             n.GetPublishService(),
@@ -1022,3 +1004,65 @@ func cleanTempNginxCfg() error {
 
 	return nil
 }
+
+type redirect struct {
+	From    string
+	To      string
+	SSLCert ingress.SSLCert
+}
+
+func buildRedirects(servers []*ingress.Server) []*redirect {
+	names := sets.String{}
+	redirectServers := make([]*redirect, 0)
+
+	for _, srv := range servers {
+		if !srv.RedirectFromToWWW {
+			continue
+		}
+
+		to := srv.Hostname
+
+		var from string
+		if strings.HasPrefix(to, "www.") {
+			from = strings.TrimPrefix(to, "www.")
+		} else {
+			from = fmt.Sprintf("www.%v", to)
+		}
+
+		if names.Has(to) {
+			continue
+		}
+
+		klog.V(3).Infof("Creating redirect from %q to %q", from, to)
+		found := false
+		for _, esrv := range servers {
+			if esrv.Hostname == from {
+				found = true
+				break
+			}
+		}
+
+		if found {
+			klog.Warningf("Already exists an Ingress with %q hostname. Skipping creation of redirection from %q to %q.", from, from, to)
+			continue
+		}
+
+		r := &redirect{
+			From: from,
+			To:   to,
+		}
+
+		if srv.SSLCert.PemSHA != "" {
+			if ssl.IsValidHostname(from, srv.SSLCert.CN) {
+				r.SSLCert = srv.SSLCert
+			} else {
+				klog.Warningf("the server %v has SSL configured but the SSL certificate does not contains a CN for %v. Redirects will not work for HTTPS to HTTPS", from, to)
+			}
+		}
+
+		redirectServers = append(redirectServers, r)
+		names.Insert(to)
+	}
+
+	return redirectServers
+}

internal/net/ssl/ssl.go

@@ -30,6 +30,7 @@ import (
 	"math/big"
 	"net"
 	"strconv"
+	"strings"
 	"time"
 
 	"github.com/zakjan/cert-chain-resolver/certUtil"
@@ -508,3 +509,21 @@ func FullChainCert(in string, fs file.Filesystem) ([]byte, error) {
 
 	return certUtil.EncodeCertificates(certs), nil
 }
+
+// IsValidHostname checks if a hostname is valid in a list of common names
+func IsValidHostname(hostname string, commonNames []string) bool {
+	for _, cn := range commonNames {
+		if strings.EqualFold(hostname, cn) {
+			return true
+		}
+
+		labels := strings.Split(hostname, ".")
+		labels[0] = "*"
+		candidate := strings.Join(labels, ".")
+		if strings.EqualFold(candidate, cn) {
+			return true
+		}
+	}
+
+	return false
+}

internal/net/ssl/ssl_test.go

@@ -205,3 +205,39 @@ func newCA(name string) (*keyPair, error) {
 		Cert: cert,
 	}, nil
 }
+
+func TestIsValidHostname(t *testing.T) {
+	cases := map[string]struct {
+		Hostname string
+		CN       []string
+		Valid    bool
+	}{
+		"when there is no common names": {
+			"foo.bar",
+			[]string{},
+			false,
+		},
+		"when there is a match for foo.bar": {
+			"foo.bar",
+			[]string{"foo.bar"},
+			true,
+		},
+		"when there is a wildcard match for foo.bar": {
+			"foo.bar",
+			[]string{"*.bar"},
+			true,
+		},
+		"when there is a wrong wildcard for *.bar": {
+			"invalid.foo.bar",
+			[]string{"*.bar"},
+			false,
+		},
+	}
+
+	for k, tc := range cases {
+		valid := IsValidHostname(tc.Hostname, tc.CN)
+		if valid != tc.Valid {
+			t.Errorf("%s: expected '%v' but returned %v", k, tc.Valid, valid)
+		}
+	}
+}

rootfs/etc/nginx/template/nginx.tmpl

@@ -520,7 +520,8 @@ http {
     {{ end }}
 
     {{/* Build server redirects (from/to www) */}}
-    {{ range $hostname, $to := .RedirectServers }}
+    {{ range $redirect := .RedirectServers }}
+    ## start server {{ $redirect.From }}
     server {
         {{ range $address := $all.Cfg.BindAddressIpv4 }}
         listen {{ $address }}:{{ $all.ListenPorts.HTTP }}{{ if $all.Cfg.UseProxyProtocol }} proxy_protocol{{ end }};
@@ -538,7 +539,25 @@ http {
         listen [::]:{{ if $all.IsSSLPassthroughEnabled }}{{ $all.ListenPorts.SSLProxy }} proxy_protocol{{ else }}{{ $all.ListenPorts.HTTPS }}{{ if $all.Cfg.UseProxyProtocol }} proxy_protocol{{ end }}{{ end }};
         {{ end }}
         {{ end }}
-        server_name {{ $hostname }};
+        server_name {{ $redirect.From }};
+
+        {{ if not (empty $redirect.SSLCert.PemFileName) }}
+        {{/* comment PEM sha is required to detect changes in the generated configuration and force a reload */}}
+        # PEM sha: {{ $redirect.SSLCert.PemSHA }}
+        ssl_certificate                         {{ $redirect.SSLCert.PemFileName }};
+        ssl_certificate_key                     {{ $redirect.SSLCert.PemFileName }};
+        {{ if not (empty $redirect.SSLCert.FullChainPemFileName)}}
+        ssl_trusted_certificate                 {{ $redirect.SSLCert.FullChainPemFileName }};
+        ssl_stapling                            on;
+        ssl_stapling_verify                     on;
+        {{ end }}
+
+        {{ if $all.DynamicCertificatesEnabled}}
+        ssl_certificate_by_lua_block {
+            certificate.call()
+        }
+        {{ end }}
+        {{ end }}
 
         {{ if gt (len $cfg.BlockUserAgents) 0 }}
         if ($block_ua) {
@@ -553,11 +572,12 @@ http {
 
         {{ if ne $all.ListenPorts.HTTPS 443 }}
         {{ $redirect_port := (printf ":%v" $all.ListenPorts.HTTPS) }}
-        return {{ $all.Cfg.HTTPRedirectCode }} $scheme://{{ $to }}{{ $redirect_port }}$request_uri;
+        return {{ $all.Cfg.HTTPRedirectCode }} $scheme://{{ $redirect.To }}{{ $redirect_port }}$request_uri;
         {{ else }}
-        return {{ $all.Cfg.HTTPRedirectCode }} $scheme://{{ $to }}$request_uri;
+        return {{ $all.Cfg.HTTPRedirectCode }} $scheme://{{ $redirect.To }}$request_uri;
         {{ end }}
     }
+    ## end server {{ $redirect.From }}
     {{ end }}
 
     {{ range $server := $servers }}

test/e2e/annotations/authtls.go

@@ -53,8 +53,7 @@ var _ = framework.IngressNginxDescribe("Annotations - AuthTLS", func() {
 			"nginx.ingress.kubernetes.io/auth-tls-secret": nameSpace + "/" + host,
 		}
 
-		ing := framework.NewSingleIngressWithTLS(host, "/", host, nameSpace, "http-svc", 80, &annotations)
-		f.EnsureIngress(ing)
+		f.EnsureIngress(framework.NewSingleIngressWithTLS(host, "/", host, []string{host}, nameSpace, "http-svc", 80, &annotations))
 
 		// Since we can use the same certificate-chain for tls as well as mutual-auth, we will check all values
 		sslCertDirective := fmt.Sprintf("ssl_certificate /etc/ingress-controller/ssl/%s-%s.pem;", nameSpace, host)
@@ -111,8 +110,7 @@ var _ = framework.IngressNginxDescribe("Annotations - AuthTLS", func() {
 			"nginx.ingress.kubernetes.io/auth-tls-verify-depth":  "2",
 		}
 
-		ing := framework.NewSingleIngressWithTLS(host, "/", host, nameSpace, "http-svc", 80, &annotations)
-		f.EnsureIngress(ing)
+		f.EnsureIngress(framework.NewSingleIngressWithTLS(host, "/", host, []string{host}, nameSpace, "http-svc", 80, &annotations))
 
 		// Since we can use the same certificate-chain for tls as well as mutual-auth, we will check all values
 		sslCertDirective := fmt.Sprintf("ssl_certificate /etc/ingress-controller/ssl/%s-%s.pem;", nameSpace, host)
@@ -158,8 +156,7 @@ var _ = framework.IngressNginxDescribe("Annotations - AuthTLS", func() {
 			"nginx.ingress.kubernetes.io/auth-tls-pass-certificate-to-upstream": "true",
 		}
 
-		ing := framework.NewSingleIngressWithTLS(host, "/", host, nameSpace, "http-svc", 80, &annotations)
-		f.EnsureIngress(ing)
+		f.EnsureIngress(framework.NewSingleIngressWithTLS(host, "/", host, []string{host}, nameSpace, "http-svc", 80, &annotations))
 
 		// Since we can use the same certificate-chain for tls as well as mutual-auth, we will check all values
 		sslCertDirective := fmt.Sprintf("ssl_certificate /etc/ingress-controller/ssl/%s-%s.pem;", nameSpace, host)

test/e2e/annotations/fromtowwwredirect.go

@@ -17,6 +17,7 @@ limitations under the License.
 package annotations
 
 import (
+	"crypto/tls"
 	"fmt"
 	"net/http"
 	"time"
@@ -24,20 +25,21 @@ import (
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
 	"github.com/parnurzeal/gorequest"
+
 	"k8s.io/ingress-nginx/test/e2e/framework"
 )
 
-var _ = framework.IngressNginxDescribe("Annotations - Fromtowwwredirect", func() {
+var _ = framework.IngressNginxDescribe("Annotations - from-to-www-redirect", func() {
 	f := framework.NewDefaultFramework("fromtowwwredirect")
 
 	BeforeEach(func() {
-		f.NewEchoDeploymentWithReplicas(2)
+		f.NewEchoDeploymentWithReplicas(1)
 	})
 
 	AfterEach(func() {
 	})
 
-	It("should redirect from www", func() {
+	It("should redirect from www HTTP to HTTP", func() {
 		By("setting up server for redirect from www")
 		host := "fromtowwwredirect.bar.com"
 
@@ -67,4 +69,48 @@ var _ = framework.IngressNginxDescribe("Annotations - Fromtowwwredirect", func()
 		Expect(resp.StatusCode).Should(Equal(http.StatusPermanentRedirect))
 		Expect(resp.Header.Get("Location")).Should(Equal("http://fromtowwwredirect.bar.com/foo"))
 	})
+
+	It("should redirect from www HTTPS to HTTPS", func() {
+		By("setting up server for redirect from www")
+		host := "fromtowwwredirect.bar.com"
+
+		annotations := map[string]string{
+			"nginx.ingress.kubernetes.io/from-to-www-redirect": "true",
+		}
+
+		ing := framework.NewSingleIngressWithTLS(host, "/", host, []string{host, fmt.Sprintf("www.%v", host)}, f.IngressController.Namespace, "http-svc", 80, &annotations)
+		f.EnsureIngress(ing)
+
+		_, err := framework.CreateIngressTLSSecret(f.KubeClientSet,
+			ing.Spec.TLS[0].Hosts,
+			ing.Spec.TLS[0].SecretName,
+			ing.Namespace)
+		Expect(err).ToNot(HaveOccurred())
+
+		f.WaitForNginxServer(fmt.Sprintf("www.%v", host),
+			func(server string) bool {
+				return Expect(server).Should(ContainSubstring(`server_name www.fromtowwwredirect.bar.com;`)) &&
+					Expect(server).Should(ContainSubstring(fmt.Sprintf("/etc/ingress-controller/ssl/%v-fromtowwwredirect.bar.com.pem", f.IngressController.Namespace))) &&
+					Expect(server).Should(ContainSubstring(`return 308 $scheme://fromtowwwredirect.bar.com$request_uri;`))
+			})
+
+		By("sending request to www.fromtowwwredirect.bar.com")
+
+		h := fmt.Sprintf("%s.%s", "www", host)
+
+		resp, _, errs := gorequest.New().
+			TLSClientConfig(&tls.Config{
+				InsecureSkipVerify: true,
+				ServerName:         h,
+			}).
+			Get(f.IngressController.HTTPSURL).
+			Retry(10, 1*time.Second, http.StatusNotFound).
+			RedirectPolicy(noRedirectPolicyFunc).
+			Set("host", h).
+			End()
+
+		Expect(errs).Should(BeEmpty())
+		Expect(resp.StatusCode).Should(Equal(http.StatusPermanentRedirect))
+		Expect(resp.Header.Get("Location")).Should(Equal("https://fromtowwwredirect.bar.com/"))
+	})
 })

test/e2e/framework/framework.go

@@ -20,7 +20,7 @@ import (
 	"time"
 
 	appsv1beta1 "k8s.io/api/apps/v1beta1"
-	"k8s.io/api/core/v1"
+	v1 "k8s.io/api/core/v1"
 	extensions "k8s.io/api/extensions/v1beta1"
 	apiextcs "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -388,16 +388,16 @@ func UpdateIngress(kubeClientSet kubernetes.Interface, namespace string, name st
 }
 
 // NewSingleIngressWithTLS creates a simple ingress rule with TLS spec included
-func NewSingleIngressWithTLS(name, path, host, ns, service string, port int, annotations *map[string]string) *extensions.Ingress {
-	return newSingleIngressWithRules(name, path, host, ns, service, port, annotations, true)
+func NewSingleIngressWithTLS(name, path, host string, tlsHosts []string, ns, service string, port int, annotations *map[string]string) *extensions.Ingress {
+	return newSingleIngressWithRules(name, path, host, ns, service, port, annotations, tlsHosts)
 }
 
 // NewSingleIngress creates a simple ingress rule
 func NewSingleIngress(name, path, host, ns, service string, port int, annotations *map[string]string) *extensions.Ingress {
-	return newSingleIngressWithRules(name, path, host, ns, service, port, annotations, false)
+	return newSingleIngressWithRules(name, path, host, ns, service, port, annotations, nil)
 }
 
-func newSingleIngressWithRules(name, path, host, ns, service string, port int, annotations *map[string]string, withTLS bool) *extensions.Ingress {
+func newSingleIngressWithRules(name, path, host, ns, service string, port int, annotations *map[string]string, tlsHosts []string) *extensions.Ingress {
 
 	spec := extensions.IngressSpec{
 		Rules: []extensions.IngressRule{
@@ -420,10 +420,10 @@ func newSingleIngressWithRules(name, path, host, ns, service string, port int, a
 		},
 	}
 
-	if withTLS {
+	if len(tlsHosts) > 0 {
 		spec.TLS = []extensions.IngressTLS{
 			{
-				Hosts:      []string{host},
+				Hosts:      tlsHosts,
 				SecretName: host,
 			},
 		}

test/e2e/lua/dynamic_certificates.go

@@ -80,7 +80,7 @@ var _ = framework.IngressNginxDescribe("Dynamic Certificate", func() {
 	})
 
 	It("picks up the previously missing secret for a given ingress without reloading", func() {
-		ing := framework.NewSingleIngressWithTLS(host, "/", host, f.IngressController.Namespace, "http-svc", 80, nil)
+		ing := framework.NewSingleIngressWithTLS(host, "/", host, []string{host}, f.IngressController.Namespace, "http-svc", 80, nil)
 		f.EnsureIngress(ing)
 
 		time.Sleep(waitForLuaSync)
@@ -120,7 +120,7 @@ var _ = framework.IngressNginxDescribe("Dynamic Certificate", func() {
 
 	Context("given an ingress with TLS correctly configured", func() {
 		BeforeEach(func() {
-			ing := f.EnsureIngress(framework.NewSingleIngressWithTLS(host, "/", host, f.IngressController.Namespace, "http-svc", 80, nil))
+			ing := f.EnsureIngress(framework.NewSingleIngressWithTLS(host, "/", host, []string{host}, f.IngressController.Namespace, "http-svc", 80, nil))
 
 			time.Sleep(waitForLuaSync)
 

test/e2e/settings/tls.go

@@ -48,7 +48,11 @@ var _ = framework.IngressNginxDescribe("Settings - TLS)", func() {
 		// https://www.openssl.org/docs/man1.1.0/apps/ciphers.html - "CIPHER SUITE NAMES"
 		testCiphers := "ECDHE-RSA-AES256-GCM-SHA384:ECDHE-RSA-AES256-SHA"
 
-		tlsConfig, err := tlsEndpoint(f, host)
+		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)
@@ -97,7 +101,11 @@ var _ = framework.IngressNginxDescribe("Settings - TLS)", func() {
 		hstsIncludeSubdomains := "hsts-include-subdomains"
 		hstsPreload := "hsts-preload"
 
-		tlsConfig, err := tlsEndpoint(f, host)
+		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)
@@ -157,11 +165,3 @@ var _ = framework.IngressNginxDescribe("Settings - TLS)", func() {
 		Expect(resp.Header.Get("Strict-Transport-Security")).Should(ContainSubstring("preload"))
 	})
 })
-
-func tlsEndpoint(f *framework.Framework, host string) (*tls.Config, error) {
-	ing := f.EnsureIngress(framework.NewSingleIngressWithTLS(host, "/", host, f.IngressController.Namespace, "http-svc", 80, nil))
-	return framework.CreateIngressTLSSecret(f.KubeClientSet,
-		ing.Spec.TLS[0].Hosts,
-		ing.Spec.TLS[0].SecretName,
-		ing.Namespace)
-}

test/e2e/ssl/secret_update.go

@@ -24,7 +24,7 @@ import (
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
 
-	"k8s.io/api/core/v1"
+	v1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/ingress-nginx/test/e2e/framework"
 )
@@ -52,8 +52,7 @@ var _ = framework.IngressNginxDescribe("SSL", func() {
 			},
 		})
 
-		ing := f.EnsureIngress(framework.NewSingleIngressWithTLS(host, "/", host, f.IngressController.Namespace, "http-svc", 80, nil))
-
+		ing := f.EnsureIngress(framework.NewSingleIngressWithTLS(host, "/", host, []string{host}, f.IngressController.Namespace, "http-svc", 80, nil))
 		_, err := framework.CreateIngressTLSSecret(f.KubeClientSet,
 			ing.Spec.TLS[0].Hosts,
 			ing.Spec.TLS[0].SecretName,