From 8976d8826b938fcf4b9b4c2c67bb9e6b0740fdd9 Mon Sep 17 00:00:00 2001
From: Alexander Yastrebov <alexander.yastrebov@zalando.de>
Date: Thu, 18 Apr 2024 19:52:37 +0200
Subject: [PATCH] filters/auth: implement opt-out support for jwtMetrics

Extend configuration of `jwtMetrics` (#3020) to support opt-out -
disable metrics collection when any of the configured
route annotations (#3022) is present.

This can be used to collect data about missing/invalid JWT tokens per hostname
in multitenant ingress setup.

Add `jwtMetrics` filter to all routes using `-default-filters-append` flag and
allow users to annotate routes that do not require JWT token.

Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>
---
 docs/reference/filters.md        |   6 +-
 filters/auth/jwt_metrics.go      |  13 ++-
 filters/auth/jwt_metrics_test.go | 149 ++++++++++++++++++++-----------
 3 files changed, 114 insertions(+), 54 deletions(-)

diff --git a/docs/reference/filters.md b/docs/reference/filters.md
index cf702a2784..e1f17bceac 100644
--- a/docs/reference/filters.md
+++ b/docs/reference/filters.md
@@ -1583,7 +1583,11 @@ For convenience use [flow style format](https://yaml.org/spec/1.2.2/#chapter-7-f
 Examples:
 
 ```
-jwtMetrics("{issuers: ['https://example.com', 'https://example.org']}")
+jwtMetrics("{issuers: ['https://example.com', 'https://example.org']")
+
+// opt-out
+annotate("oauth.disabled", "this endpoint is public") ->
+jwtMetrics("{issuers: ['https://example.com', 'https://example.org'], optOutAnnotations: [oauth.disabled]}")
 ```
 
 
diff --git a/filters/auth/jwt_metrics.go b/filters/auth/jwt_metrics.go
index 71e093b6c3..48a66226bf 100644
--- a/filters/auth/jwt_metrics.go
+++ b/filters/auth/jwt_metrics.go
@@ -8,6 +8,7 @@ import (
 
 	"github.com/ghodss/yaml"
 	"github.com/zalando/skipper/filters"
+	"github.com/zalando/skipper/filters/annotate"
 	"github.com/zalando/skipper/jwt"
 )
 
@@ -15,7 +16,8 @@ type (
 	jwtMetricsSpec struct{}
 
 	jwtMetricsFilter struct {
-		Issuers []string `json:"issuers,omitempty"`
+		Issuers           []string `json:"issuers,omitempty"`
+		OptOutAnnotations []string `json:"optOutAnnotations,omitempty"`
 	}
 )
 
@@ -46,6 +48,15 @@ func (s *jwtMetricsSpec) CreateFilter(args []interface{}) (filters.Filter, error
 func (f *jwtMetricsFilter) Request(ctx filters.FilterContext) {}
 
 func (f *jwtMetricsFilter) Response(ctx filters.FilterContext) {
+	if len(f.OptOutAnnotations) > 0 {
+		annotations := annotate.GetAnnotations(ctx)
+		for _, annotation := range f.OptOutAnnotations {
+			if _, ok := annotations[annotation]; ok {
+				return // opt-out
+			}
+		}
+	}
+
 	response := ctx.Response()
 
 	if response.StatusCode >= 400 && response.StatusCode < 500 {
diff --git a/filters/auth/jwt_metrics_test.go b/filters/auth/jwt_metrics_test.go
index c1f5535fdf..d50e20c4be 100644
--- a/filters/auth/jwt_metrics_test.go
+++ b/filters/auth/jwt_metrics_test.go
@@ -3,155 +3,200 @@ package auth_test
 import (
 	"encoding/base64"
 	"encoding/json"
+	"fmt"
 	"net/http"
+	"net/url"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"github.com/zalando/skipper/eskip"
 	"github.com/zalando/skipper/filters/auth"
-	"github.com/zalando/skipper/filters/filtertest"
+	"github.com/zalando/skipper/filters/builtin"
+	"github.com/zalando/skipper/metrics"
 	"github.com/zalando/skipper/metrics/metricstest"
+	"github.com/zalando/skipper/proxy/proxytest"
+	"github.com/zalando/skipper/routing"
 )
 
 func TestJwtMetrics(t *testing.T) {
-	spec := auth.NewJwtMetrics()
-
 	for _, tc := range []struct {
 		name     string
-		def      string
+		filters  string
 		request  *http.Request
-		response *http.Response
+		status   int
 		expected map[string]int64
 	}{
 		{
 			name:     "ignores 401 response",
-			def:      `jwtMetrics("{issuers: [foo, bar]}")`,
+			filters:  `jwtMetrics("{issuers: [foo, bar]}")`,
 			request:  &http.Request{Method: "GET", Host: "foo.test"},
-			response: &http.Response{StatusCode: http.StatusUnauthorized},
+			status:   http.StatusUnauthorized,
 			expected: map[string]int64{},
 		},
 		{
 			name:     "ignores 403 response",
-			def:      `jwtMetrics("{issuers: [foo, bar]}")`,
+			filters:  `jwtMetrics("{issuers: [foo, bar]}")`,
 			request:  &http.Request{Method: "GET", Host: "foo.test"},
-			response: &http.Response{StatusCode: http.StatusForbidden},
+			status:   http.StatusForbidden,
 			expected: map[string]int64{},
 		},
 		{
 			name:     "ignores 404 response",
-			def:      `jwtMetrics("{issuers: [foo, bar]}")`,
+			filters:  `jwtMetrics("{issuers: [foo, bar]}")`,
 			request:  &http.Request{Method: "GET", Host: "foo.test"},
-			response: &http.Response{StatusCode: http.StatusNotFound},
+			status:   http.StatusNotFound,
 			expected: map[string]int64{},
 		},
 		{
-			name:     "missing-token",
-			def:      `jwtMetrics("{issuers: [foo, bar]}")`,
-			request:  &http.Request{Method: "GET", Host: "foo.test"},
-			response: &http.Response{StatusCode: http.StatusOK},
+			name:    "missing-token",
+			filters: `jwtMetrics("{issuers: [foo, bar]}")`,
+			request: &http.Request{Method: "GET", Host: "foo.test"},
+			status:  http.StatusOK,
 			expected: map[string]int64{
-				"GET.foo_test.200.missing-token": 1,
+				"jwtMetrics.custom.GET.foo_test.200.missing-token": 1,
 			},
 		},
 		{
-			name: "invalid-token-type",
-			def:  `jwtMetrics("{issuers: [foo, bar]}")`,
+			name:    "invalid-token-type",
+			filters: `jwtMetrics("{issuers: [foo, bar]}")`,
 			request: &http.Request{Method: "GET", Host: "foo.test",
 				Header: http.Header{"Authorization": []string{"Basic foobarbaz"}},
 			},
-			response: &http.Response{StatusCode: http.StatusOK},
+			status: http.StatusOK,
 			expected: map[string]int64{
-				"GET.foo_test.200.invalid-token-type": 1,
+				"jwtMetrics.custom.GET.foo_test.200.invalid-token-type": 1,
 			},
 		},
 		{
-			name: "invalid-token",
-			def:  `jwtMetrics("{issuers: [foo, bar]}")`,
+			name:    "invalid-token",
+			filters: `jwtMetrics("{issuers: [foo, bar]}")`,
 			request: &http.Request{Method: "GET", Host: "foo.test",
 				Header: http.Header{"Authorization": []string{"Bearer invalid-token"}},
 			},
-			response: &http.Response{StatusCode: http.StatusOK},
+			status: http.StatusOK,
 			expected: map[string]int64{
-				"GET.foo_test.200.invalid-token": 1,
+				"jwtMetrics.custom.GET.foo_test.200.invalid-token": 1,
 			},
 		},
 		{
-			name: "missing-issuer",
-			def:  `jwtMetrics("{issuers: [foo, bar]}")`,
+			name:    "missing-issuer",
+			filters: `jwtMetrics("{issuers: [foo, bar]}")`,
 			request: &http.Request{Method: "GET", Host: "foo.test",
 				Header: http.Header{"Authorization": []string{
 					"Bearer header." + marshalBase64JSON(t, map[string]any{"sub": "baz"}) + ".signature",
 				}},
 			},
-			response: &http.Response{StatusCode: http.StatusOK},
+			status: http.StatusOK,
 			expected: map[string]int64{
-				"GET.foo_test.200.missing-issuer": 1,
+				"jwtMetrics.custom.GET.foo_test.200.missing-issuer": 1,
 			},
 		},
 		{
-			name: "invalid-issuer",
-			def:  `jwtMetrics("{issuers: [foo, bar]}")`,
+			name:    "invalid-issuer",
+			filters: `jwtMetrics("{issuers: [foo, bar]}")`,
 			request: &http.Request{Method: "GET", Host: "foo.test",
 				Header: http.Header{"Authorization": []string{
 					"Bearer header." + marshalBase64JSON(t, map[string]any{"iss": "baz"}) + ".signature",
 				}},
 			},
-			response: &http.Response{StatusCode: http.StatusOK},
+			status: http.StatusOK,
 			expected: map[string]int64{
-				"GET.foo_test.200.invalid-issuer": 1,
+				"jwtMetrics.custom.GET.foo_test.200.invalid-issuer": 1,
 			},
 		},
 		{
-			name: "no invalid-issuer for empty issuers",
-			def:  `jwtMetrics()`,
+			name:    "no invalid-issuer for empty issuers",
+			filters: `jwtMetrics()`,
 			request: &http.Request{Method: "GET", Host: "foo.test",
 				Header: http.Header{"Authorization": []string{
 					"Bearer header." + marshalBase64JSON(t, map[string]any{"iss": "baz"}) + ".signature",
 				}},
 			},
-			response: &http.Response{StatusCode: http.StatusOK},
+			status:   http.StatusOK,
 			expected: map[string]int64{},
 		},
 		{
-			name: "no invalid-issuer when matches first",
-			def:  `jwtMetrics("{issuers: [foo, bar]}")`,
+			name:    "no invalid-issuer when matches first",
+			filters: `jwtMetrics("{issuers: [foo, bar]}")`,
 			request: &http.Request{Method: "GET", Host: "foo.test",
 				Header: http.Header{"Authorization": []string{
 					"Bearer header." + marshalBase64JSON(t, map[string]any{"iss": "foo"}) + ".signature",
 				}},
 			},
-			response: &http.Response{StatusCode: http.StatusOK},
+			status:   http.StatusOK,
 			expected: map[string]int64{},
 		},
 		{
-			name: "no invalid-issuer when matches second",
-			def:  `jwtMetrics("{issuers: [foo, bar]}")`,
+			name:    "no invalid-issuer when matches second",
+			filters: `jwtMetrics("{issuers: [foo, bar]}")`,
 			request: &http.Request{Method: "GET", Host: "foo.test",
 				Header: http.Header{"Authorization": []string{
 					"Bearer header." + marshalBase64JSON(t, map[string]any{"iss": "bar"}) + ".signature",
 				}},
 			},
-			response: &http.Response{StatusCode: http.StatusOK},
+			status:   http.StatusOK,
+			expected: map[string]int64{},
+		},
+		{
+			name:    "missing-token without opt-out",
+			filters: `jwtMetrics("{issuers: [foo, bar], optOutAnnotations: [oauth.disabled]}")`,
+			request: &http.Request{Method: "GET", Host: "foo.test"},
+			status:  http.StatusOK,
+			expected: map[string]int64{
+				"jwtMetrics.custom.GET.foo_test.200.missing-token": 1,
+			},
+		},
+		{
+			name: "no metrics when opted-out",
+			filters: `
+				annotate("oauth.disabled", "this endpoint is public") ->
+				jwtMetrics("{issuers: [foo, bar], optOutAnnotations: [oauth.disabled, jwtMetrics.disabled]}")
+			`,
+			request:  &http.Request{Method: "GET", Host: "foo.test"},
+			status:   http.StatusOK,
+			expected: map[string]int64{},
+		},
+		{
+			name: "no metrics when opted-out by alternative annotation",
+			filters: `
+				annotate("jwtMetrics.disabled", "skip jwt metrics collection") ->
+				jwtMetrics("{issuers: [foo, bar], optOutAnnotations: [oauth.disabled, jwtMetrics.disabled]}")
+			`,
+			request:  &http.Request{Method: "GET", Host: "foo.test"},
+			status:   http.StatusOK,
 			expected: map[string]int64{},
 		},
 	} {
 		t.Run(tc.name, func(t *testing.T) {
-			args := eskip.MustParseFilters(tc.def)[0].Args
+			dm := metrics.Default
+			t.Cleanup(func() { metrics.Default = dm })
+			m := &metricstest.MockMetrics{}
+			metrics.Default = m
+			defer m.Close()
+
+			fr := builtin.MakeRegistry()
+			fr.Register(auth.NewJwtMetrics())
+			p := proxytest.Config{
+				RoutingOptions: routing.Options{
+					FilterRegistry: fr,
+				},
+				Routes: eskip.MustParse(fmt.Sprintf(`* -> %s -> status(%d) -> <shunt>`, tc.filters, tc.status)),
+			}.Create()
+			defer p.Close()
+
+			u, err := url.Parse(p.URL)
+			require.NoError(t, err)
+			tc.request.URL = u
 
-			filter, err := spec.CreateFilter(args)
+			resp, err := p.Client().Do(tc.request)
 			require.NoError(t, err)
+			resp.Body.Close()
 
-			metrics := &metricstest.MockMetrics{}
-			ctx := &filtertest.Context{
-				FRequest: tc.request,
-				FMetrics: metrics,
-			}
-			filter.Request(ctx)
-			ctx.FResponse = tc.response
-			filter.Response(ctx)
+			m.WithCounters(func(counters map[string]int64) {
+				// add incoming counter to simplify comparison
+				tc.expected["incoming.HTTP/1.1"] = 1
 
-			metrics.WithCounters(func(counters map[string]int64) {
 				assert.Equal(t, tc.expected, counters)
 			})
 		})