Merge pull request #2811 from takonomura/escape-request-uri

Escape $request_uri for external auth

docs/examples/auth/oauth-external-auth/README.md

@@ -25,7 +25,7 @@ metadata:
   name: application
   annotations:
     nginx.ingress.kubernetes.io/auth-url: "https://$host/oauth2/auth"
-    nginx.ingress.kubernetes.io/auth-signin: "https://$host/oauth2/start?rd=$request_uri"
+    nginx.ingress.kubernetes.io/auth-signin: "https://$host/oauth2/start?rd=$escaped_request_uri"
 ...
 ```
 

docs/examples/auth/oauth-external-auth/dashboard-ingress.yaml

@@ -3,7 +3,7 @@ kind: Ingress
 metadata:
   annotations:
     nginx.ingress.kubernetes.io/auth-url: "https://$host/oauth2/auth"
-    nginx.ingress.kubernetes.io/auth-signin: "https://$host/oauth2/start?rd=$request_uri"
+    nginx.ingress.kubernetes.io/auth-signin: "https://$host/oauth2/start?rd=$escaped_request_uri"
   name: external-auth-oauth2
   namespace: kube-system
 spec:

internal/ingress/controller/template/template.go

@@ -815,14 +815,14 @@ func buildAuthSignURL(input interface{}) string {
 	u, _ := url.Parse(s)
 	q := u.Query()
 	if len(q) == 0 {
-		return fmt.Sprintf("%v?rd=$pass_access_scheme://$http_host$request_uri", s)
+		return fmt.Sprintf("%v?rd=$pass_access_scheme://$http_host$escaped_request_uri", s)
 	}
 
 	if q.Get("rd") != "" {
 		return s
 	}
 
-	return fmt.Sprintf("%v&rd=$pass_access_scheme://$http_host$request_uri", s)
+	return fmt.Sprintf("%v&rd=$pass_access_scheme://$http_host$escaped_request_uri", s)
 }
 
 var letters = []rune("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ")

internal/ingress/controller/template/template_test.go

@@ -697,8 +697,8 @@ func TestBuildAuthSignURL(t *testing.T) {
 	cases := map[string]struct {
 		Input, Output string
 	}{
-		"default url":       {"http://google.com", "http://google.com?rd=$pass_access_scheme://$http_host$request_uri"},
+		"default url":       {"http://google.com", "http://google.com?rd=$pass_access_scheme://$http_host$escaped_request_uri"},
-		"with random field": {"http://google.com?cat=0", "http://google.com?cat=0&rd=$pass_access_scheme://$http_host$request_uri"},
+		"with random field": {"http://google.com?cat=0", "http://google.com?cat=0&rd=$pass_access_scheme://$http_host$escaped_request_uri"},
 		"with rd field":     {"http://google.com?cat&rd=$request", "http://google.com?cat&rd=$request"},
 	}
 	for k, tc := range cases {

rootfs/etc/nginx/template/nginx.tmpl

@@ -977,6 +977,7 @@ stream {
             {{ end }}
 
             {{ if $location.ExternalAuth.SigninURL }}
+            set_escape_uri $escaped_request_uri $request_uri;
             error_page 401 = {{ buildAuthSignURL $location.ExternalAuth.SigninURL }};
             {{ end }}
 

test/e2e/annotations/auth.go

@@ -19,6 +19,7 @@ package annotations
 import (
 	"fmt"
 	"net/http"
+	"net/url"
 	"os/exec"
 	"time"
 
@@ -27,7 +28,9 @@ import (
 	"github.com/parnurzeal/gorequest"
 
 	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/util/wait"
 
 	"k8s.io/ingress-nginx/test/e2e/framework"
 )
@@ -265,6 +268,77 @@ var _ = framework.IngressNginxDescribe("Annotations - Auth", func() {
 		Expect(len(errs)).Should(BeNumerically("==", 0))
 		Expect(resp.StatusCode).Should(Equal(http.StatusInternalServerError))
 	})
+
+	Context("when external authentication is configured", func() {
+		host := "auth"
+
+		BeforeEach(func() {
+			err := f.NewHttpbinDeployment()
+			Expect(err).NotTo(HaveOccurred())
+
+			var httpbinIP string
+			err = wait.PollImmediate(time.Second, time.Minute, func() (bool, error) {
+				e, err := f.KubeClientSet.CoreV1().Endpoints(f.IngressController.Namespace).Get("httpbin", metav1.GetOptions{})
+				if errors.IsNotFound(err) {
+					return false, nil
+				}
+				if err != nil {
+					return false, err
+				}
+				if len(e.Subsets) < 1 || len(e.Subsets[0].Addresses) < 1 {
+					return false, nil
+				}
+				httpbinIP = e.Subsets[0].Addresses[0].IP
+				return true, nil
+			})
+			Expect(err).NotTo(HaveOccurred())
+
+			bi, err := f.EnsureIngress(framework.NewSingleIngress(host, "/", host, f.IngressController.Namespace, "http-svc", 80, &map[string]string{
+				"nginx.ingress.kubernetes.io/auth-url":    fmt.Sprintf("http://%s/basic-auth/user/password", httpbinIP),
+				"nginx.ingress.kubernetes.io/auth-signin": "http://$host/auth/start",
+			}))
+			Expect(err).NotTo(HaveOccurred())
+			Expect(bi).NotTo(BeNil())
+
+			err = f.WaitForNginxServer(host, func(server string) bool {
+				return Expect(server).ShouldNot(ContainSubstring("return 503"))
+			})
+			Expect(err).NotTo(HaveOccurred())
+		})
+
+		It("should return status code 200 when signed in", func() {
+			resp, _, errs := gorequest.New().
+				Get(f.IngressController.HTTPURL).
+				Retry(10, 1*time.Second, http.StatusNotFound).
+				Set("Host", host).
+				SetBasicAuth("user", "password").
+				End()
+
+			for _, err := range errs {
+				Expect(err).NotTo(HaveOccurred())
+			}
+			Expect(resp.StatusCode).Should(Equal(http.StatusOK))
+		})
+
+		It("should redirect to signin url when not signed in", func() {
+			resp, _, errs := gorequest.New().
+				Get(f.IngressController.HTTPURL).
+				Retry(10, 1*time.Second, http.StatusNotFound).
+				Set("Host", host).
+				RedirectPolicy(func(req gorequest.Request, via []gorequest.Request) error {
+					return http.ErrUseLastResponse
+				}).
+				Param("a", "b").
+				Param("c", "d").
+				End()
+
+			for _, err := range errs {
+				Expect(err).NotTo(HaveOccurred())
+			}
+			Expect(resp.StatusCode).Should(Equal(http.StatusFound))
+			Expect(resp.Header.Get("Location")).Should(Equal(fmt.Sprintf("http://%s/auth/start?rd=http://%s%s", host, host, url.QueryEscape("/?a=b&c=d"))))
+		})
+	})
 })
 
 // TODO: test Digest Auth

test/e2e/framework/deployment.go

@@ -37,35 +37,45 @@ func (f *Framework) NewEchoDeployment() error {
 // NewEchoDeploymentWithReplicas creates a new deployment of the echoserver image in a particular namespace. Number of
 // replicas is configurable
 func (f *Framework) NewEchoDeploymentWithReplicas(replicas int32) error {
+	return f.NewDeployment("http-svc", "gcr.io/google_containers/echoserver:1.10", 8080, replicas)
+}
+
+// NewHttpbinDeployment creates a new single replica deployment of the httpbin image in a particular namespace.
+func (f *Framework) NewHttpbinDeployment() error {
+	return f.NewDeployment("httpbin", "kennethreitz/httpbin", 80, 1)
+}
+
+// NewDeployment creates a new deployment in a particular namespace.
+func (f *Framework) NewDeployment(name, image string, port int32, replicas int32) error {
 	deployment := &extensions.Deployment{
 		ObjectMeta: metav1.ObjectMeta{
-			Name:      "http-svc",
+			Name:      name,
 			Namespace: f.IngressController.Namespace,
 		},
 		Spec: extensions.DeploymentSpec{
 			Replicas: NewInt32(replicas),
 			Selector: &metav1.LabelSelector{
 				MatchLabels: map[string]string{
-					"app": "http-svc",
+					"app": name,
 				},
 			},
 			Template: corev1.PodTemplateSpec{
 				ObjectMeta: metav1.ObjectMeta{
 					Labels: map[string]string{
-						"app": "http-svc",
+						"app": name,
 					},
 				},
 				Spec: corev1.PodSpec{
 					TerminationGracePeriodSeconds: NewInt64(0),
 					Containers: []corev1.Container{
 						{
-							Name:  "http-svc",
+							Name:  name,
-							Image: "gcr.io/google_containers/echoserver:1.10",
+							Image: image,
 							Env:   []corev1.EnvVar{},
 							Ports: []corev1.ContainerPort{
 								{
 									Name:          "http",
-									ContainerPort: 8080,
+									ContainerPort: port,
 								},
 							},
 						},
@@ -81,7 +91,7 @@ func (f *Framework) NewEchoDeploymentWithReplicas(replicas int32) error {
 	}
 
 	if d == nil {
-		return fmt.Errorf("unexpected error creating deployement for echoserver")
+		return fmt.Errorf("unexpected error creating deployement %s", name)
 	}
 
 	err = WaitForPodsReady(f.KubeClientSet, 5*time.Minute, int(replicas), f.IngressController.Namespace, metav1.ListOptions{
@@ -93,7 +103,7 @@ func (f *Framework) NewEchoDeploymentWithReplicas(replicas int32) error {
 
 	service := &corev1.Service{
 		ObjectMeta: metav1.ObjectMeta{
-			Name:      "http-svc",
+			Name:      name,
 			Namespace: f.IngressController.Namespace,
 		},
 		Spec: corev1.ServiceSpec{
@@ -101,12 +111,12 @@ func (f *Framework) NewEchoDeploymentWithReplicas(replicas int32) error {
 				{
 					Name:       "http",
 					Port:       80,
-					TargetPort: intstr.FromInt(8080),
+					TargetPort: intstr.FromInt(int(port)),
 					Protocol:   "TCP",
 				},
 			},
 			Selector: map[string]string{
-				"app": "http-svc",
+				"app": name,
 			},
 		},
 	}
@@ -117,7 +127,7 @@ func (f *Framework) NewEchoDeploymentWithReplicas(replicas int32) error {
 	}
 
 	if s == nil {
-		return fmt.Errorf("unexpected error creating service for echoserver deployment")
+		return fmt.Errorf("unexpected error creating service %s", name)
 	}
 
 	return nil