From 461e50deccc95618f123508759f01689d48bab4c Mon Sep 17 00:00:00 2001 From: Tom Hayward Date: Mon, 12 Jul 2021 23:08:29 -0700 Subject: [PATCH] Fix forwarding of auth-response-headers to gRPC backends (#7331) * add e2e test for auth-response-headers annotation * add e2e test for grpc with auth-response-headers * fix forwarding of auth header to GRPC backends * add test case for proxySetHeader(nil) --- .../ingress/controller/template/template.go | 6 +- .../controller/template/template_test.go | 51 ++++++++----- rootfs/etc/nginx/template/nginx.tmpl | 4 +- test/e2e/annotations/auth.go | 23 ++++++ test/e2e/annotations/grpc.go | 74 +++++++++++++++++++ 5 files changed, 136 insertions(+), 22 deletions(-) diff --git a/internal/ingress/controller/template/template.go b/internal/ingress/controller/template/template.go index 7a248938f9..522407a2fa 100644 --- a/internal/ingress/controller/template/template.go +++ b/internal/ingress/controller/template/template.go @@ -563,7 +563,7 @@ func shouldApplyGlobalAuth(input interface{}, globalExternalAuthURL string) bool return false } -func buildAuthResponseHeaders(headers []string) []string { +func buildAuthResponseHeaders(proxySetHeader string, headers []string) []string { res := []string{} if len(headers) == 0 { @@ -574,7 +574,7 @@ func buildAuthResponseHeaders(headers []string) []string { hvar := strings.ToLower(h) hvar = strings.NewReplacer("-", "_").Replace(hvar) res = append(res, fmt.Sprintf("auth_request_set $authHeader%v $upstream_http_%v;", i, hvar)) - res = append(res, fmt.Sprintf("proxy_set_header '%v' $authHeader%v;", h, i)) + res = append(res, fmt.Sprintf("%s '%v' $authHeader%v;", proxySetHeader, h, i)) } return res } @@ -668,7 +668,7 @@ func buildProxyPass(host string, b interface{}, loc interface{}) string { var xForwardedPrefix string if len(location.XForwardedPrefix) > 0 { - xForwardedPrefix = fmt.Sprintf("proxy_set_header X-Forwarded-Prefix \"%s\";\n", location.XForwardedPrefix) + xForwardedPrefix = fmt.Sprintf("%s X-Forwarded-Prefix \"%s\";\n", proxySetHeader(location), location.XForwardedPrefix) } return fmt.Sprintf(` diff --git a/internal/ingress/controller/template/template_test.go b/internal/ingress/controller/template/template_test.go index ad0635adfc..abe7049b01 100644 --- a/internal/ingress/controller/template/template_test.go +++ b/internal/ingress/controller/template/template_test.go @@ -506,7 +506,7 @@ func TestBuildAuthResponseHeaders(t *testing.T) { "proxy_set_header 'H-With-Caps-And-Dashes' $authHeader1;", } - headers := buildAuthResponseHeaders(externalAuthResponseHeaders) + headers := buildAuthResponseHeaders(proxySetHeader(nil), externalAuthResponseHeaders) if !reflect.DeepEqual(expected, headers) { t.Errorf("Expected \n'%v'\nbut returned \n'%v'", expected, headers) @@ -1182,23 +1182,40 @@ func TestBuildCustomErrorLocationsPerServer(t *testing.T) { } func TestProxySetHeader(t *testing.T) { - invalidType := &ingress.Ingress{} - expected := "proxy_set_header" - actual := proxySetHeader(invalidType) - - if expected != actual { - t.Errorf("Expected '%v' but returned '%v'", expected, actual) - } - - grpcBackend := &ingress.Location{ - BackendProtocol: "GRPC", + tests := []struct { + name string + loc interface{} + expected string + }{ + { + name: "nil", + loc: nil, + expected: "proxy_set_header", + }, + { + name: "invalid type", + loc: &ingress.Ingress{}, + expected: "proxy_set_header", + }, + { + name: "http backend", + loc: &ingress.Location{}, + expected: "proxy_set_header", + }, + { + name: "gRPC backend", + loc: &ingress.Location{ + BackendProtocol: "GRPC", + }, + expected: "grpc_set_header", + }, } - - expected = "grpc_set_header" - actual = proxySetHeader(grpcBackend) - - if expected != actual { - t.Errorf("Expected '%v' but returned '%v'", expected, actual) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := proxySetHeader(tt.loc); got != tt.expected { + t.Errorf("proxySetHeader() = %v, expected %v", got, tt.expected) + } + }) } } diff --git a/rootfs/etc/nginx/template/nginx.tmpl b/rootfs/etc/nginx/template/nginx.tmpl index e6f516f739..4bb5fe18c6 100755 --- a/rootfs/etc/nginx/template/nginx.tmpl +++ b/rootfs/etc/nginx/template/nginx.tmpl @@ -1178,7 +1178,7 @@ stream { auth_request {{ $authPath }}; auth_request_set $auth_cookie $upstream_http_set_cookie; add_header Set-Cookie $auth_cookie; - {{- range $line := buildAuthResponseHeaders $externalAuth.ResponseHeaders }} + {{- range $line := buildAuthResponseHeaders $proxySetHeader $externalAuth.ResponseHeaders }} {{ $line }} {{- end }} {{ end }} @@ -1196,7 +1196,7 @@ stream { auth_digest {{ $location.BasicDigestAuth.Realm | quote }}; auth_digest_user_file {{ $location.BasicDigestAuth.File }}; {{ end }} - proxy_set_header Authorization ""; + {{ $proxySetHeader }} Authorization ""; {{ end }} {{ end }} diff --git a/test/e2e/annotations/auth.go b/test/e2e/annotations/auth.go index 781a32b1b5..fac0658c08 100644 --- a/test/e2e/annotations/auth.go +++ b/test/e2e/annotations/auth.go @@ -453,6 +453,29 @@ http { Expect(). Status(http.StatusOK) }) + + ginkgo.It("should overwrite Foo header with auth response", func() { + var ( + rewriteHeader = "Foo" + rewriteVal = "bar" + ) + annotations["nginx.ingress.kubernetes.io/auth-response-headers"] = rewriteHeader + f.UpdateIngress(ing) + + f.WaitForNginxServer(host, func(server string) bool { + return strings.Contains(server, fmt.Sprintf("proxy_set_header '%s' $authHeader0;", rewriteHeader)) + }) + + f.HTTPTestClient(). + GET("/"). + WithHeader("Host", host). + WithHeader(rewriteHeader, rewriteVal). + WithBasicAuth("user", "password"). + Expect(). + Status(http.StatusOK). + Body(). + NotContainsFold(fmt.Sprintf("%s=%s", rewriteHeader, rewriteVal)) + }) }) ginkgo.Context("when external authentication is configured with a custom redirect param", func() { diff --git a/test/e2e/annotations/grpc.go b/test/e2e/annotations/grpc.go index 003277885d..9da6fdc152 100644 --- a/test/e2e/annotations/grpc.go +++ b/test/e2e/annotations/grpc.go @@ -27,6 +27,7 @@ import ( "golang.org/x/net/context" "google.golang.org/grpc" "google.golang.org/grpc/credentials" + "google.golang.org/grpc/metadata" core "k8s.io/api/core/v1" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -121,6 +122,79 @@ var _ = framework.DescribeAnnotation("backend-protocol - GRPC", func() { assert.Equal(ginkgo.GinkgoT(), metadata["content-type"].Values[0], "application/grpc") }) + ginkgo.It("authorization metadata should be overwritten by external auth response headers", func() { + f.NewGRPCBinDeployment() + f.NewHttpbinDeployment() + + host := "echo" + + svc := &core.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "grpcbin-test", + Namespace: f.Namespace, + }, + Spec: corev1.ServiceSpec{ + ExternalName: fmt.Sprintf("grpcbin.%v.svc.cluster.local", f.Namespace), + Type: corev1.ServiceTypeExternalName, + Ports: []corev1.ServicePort{ + { + Name: host, + Port: 9000, + TargetPort: intstr.FromInt(9000), + Protocol: "TCP", + }, + }, + }, + } + f.EnsureService(svc) + + err := framework.WaitForEndpoints(f.KubeClientSet, framework.DefaultTimeout, framework.HTTPBinService, f.Namespace, 1) + assert.Nil(ginkgo.GinkgoT(), err) + + e, err := f.KubeClientSet.CoreV1().Endpoints(f.Namespace).Get(context.TODO(), framework.HTTPBinService, metav1.GetOptions{}) + assert.Nil(ginkgo.GinkgoT(), err) + + assert.GreaterOrEqual(ginkgo.GinkgoT(), len(e.Subsets), 1, "expected at least one endpoint") + assert.GreaterOrEqual(ginkgo.GinkgoT(), len(e.Subsets[0].Addresses), 1, "expected at least one address ready in the endpoint") + + httpbinIP := e.Subsets[0].Addresses[0].IP + + annotations := map[string]string{ + "nginx.ingress.kubernetes.io/auth-url": fmt.Sprintf("http://%s/response-headers?authorization=foo", httpbinIP), + "nginx.ingress.kubernetes.io/auth-response-headers": "Authorization", + "nginx.ingress.kubernetes.io/backend-protocol": "GRPC", + } + + ing := framework.NewSingleIngressWithTLS(host, "/", host, []string{host}, f.Namespace, "grpcbin-test", 9000, annotations) + + f.EnsureIngress(ing) + + f.WaitForNginxServer(host, + func(server string) bool { + return strings.Contains(server, "grpc_pass grpc://upstream_balancer;") + }) + + conn, _ := grpc.Dial(f.GetNginxIP()+":443", + grpc.WithTransportCredentials( + credentials.NewTLS(&tls.Config{ + ServerName: "echo", + InsecureSkipVerify: true, + }), + ), + ) + defer conn.Close() + + client := pb.NewGRPCBinClient(conn) + ctx := metadata.AppendToOutgoingContext(context.Background(), + "authorization", "bar") + + res, err := client.HeadersUnary(ctx, &pb.EmptyMessage{}) + assert.Nil(ginkgo.GinkgoT(), err) + + metadata := res.GetMetadata() + assert.Equal(ginkgo.GinkgoT(), "foo", metadata["authorization"].Values[0]) + }) + ginkgo.It("should return OK for service with backend protocol GRPCS", func() { f.NewGRPCBinDeployment()