Merge pull request #70302 from tallclair/authzcache

Don't cache rediculous subject access reviews

staging/src/k8s.io/apiserver/plugin/pkg/authorizer/webhook/webhook.go

@@ -39,7 +39,11 @@ var (
 	groupVersions = []schema.GroupVersion{authorization.SchemeGroupVersion}
 )
 
-const retryBackoff = 500 * time.Millisecond
+const (
+	retryBackoff = 500 * time.Millisecond
+	// The maximum length of requester-controlled attributes to allow caching.
+	maxControlledAttrCacheSize = 10000
+)
 
 // Ensure Webhook implements the authorizer.Authorizer interface.
 var _ authorizer.Authorizer = (*WebhookAuthorizer)(nil)
@@ -193,10 +197,12 @@ func (w *WebhookAuthorizer) Authorize(attr authorizer.Attributes) (decision auth
 			return w.decisionOnError, "", err
 		}
 		r.Status = result.Status
-		if r.Status.Allowed {
+		if shouldCache(attr) {
-			w.responseCache.Add(string(key), r.Status, w.authorizedTTL)
+			if r.Status.Allowed {
-		} else {
+				w.responseCache.Add(string(key), r.Status, w.authorizedTTL)
-			w.responseCache.Add(string(key), r.Status, w.unauthorizedTTL)
+			} else {
+				w.responseCache.Add(string(key), r.Status, w.unauthorizedTTL)
+			}
 		}
 	}
 	switch {
@@ -262,3 +268,17 @@ func (t *subjectAccessReviewClient) Create(subjectAccessReview *authorization.Su
 	err := t.w.RestClient.Post().Body(subjectAccessReview).Do().Into(result)
 	return result, err
 }
+
+// shouldCache determines whether it is safe to cache the given request attributes. If the
+// requester-controlled attributes are too large, this may be a DoS attempt, so we skip the cache.
+func shouldCache(attr authorizer.Attributes) bool {
+	controlledAttrSize := int64(len(attr.GetNamespace())) +
+		int64(len(attr.GetVerb())) +
+		int64(len(attr.GetAPIGroup())) +
+		int64(len(attr.GetAPIVersion())) +
+		int64(len(attr.GetResource())) +
+		int64(len(attr.GetSubresource())) +
+		int64(len(attr.GetName())) +
+		int64(len(attr.GetPath()))
+	return controlledAttrSize < maxControlledAttrCacheSize
+}

staging/src/k8s.io/apiserver/plugin/pkg/authorizer/webhook/webhook_test.go

@@ -28,6 +28,7 @@ import (
 	"os"
 	"path/filepath"
 	"reflect"
+	"strings"
 	"testing"
 	"text/template"
 	"time"
@@ -542,41 +543,6 @@ func TestWebhook(t *testing.T) {
 	}
 }
 
-type webhookCacheTestCase struct {
-	attr authorizer.AttributesRecord
-
-	allow      bool
-	statusCode int
-
-	expectedErr        bool
-	expectedAuthorized bool
-	expectedCalls      int
-}
-
-func testWebhookCacheCases(t *testing.T, serv *mockService, wh *WebhookAuthorizer, tests []webhookCacheTestCase) {
-	for i, test := range tests {
-		serv.called = 0
-		serv.allow = test.allow
-		serv.statusCode = test.statusCode
-		authorized, _, err := wh.Authorize(test.attr)
-		if test.expectedErr && err == nil {
-			t.Errorf("%d: Expected error", i)
-			continue
-		} else if !test.expectedErr && err != nil {
-			t.Errorf("%d: unexpected error: %v", i, err)
-			continue
-		}
-
-		if test.expectedAuthorized != (authorized == authorizer.DecisionAllow) {
-			t.Errorf("%d: expected authorized=%v, got %v", i, test.expectedAuthorized, authorized)
-		}
-
-		if test.expectedCalls != serv.called {
-			t.Errorf("%d: expected %d calls, got %d", i, test.expectedCalls, serv.called)
-		}
-	}
-}
-
 // TestWebhookCache verifies that error responses from the server are not
 // cached, but successful responses are.
 func TestWebhookCache(t *testing.T) {
@@ -595,27 +561,86 @@ func TestWebhookCache(t *testing.T) {
 
 	aliceAttr := authorizer.AttributesRecord{User: &user.DefaultInfo{Name: "alice"}}
 	bobAttr := authorizer.AttributesRecord{User: &user.DefaultInfo{Name: "bob"}}
+	aliceRidiculousAttr := authorizer.AttributesRecord{
+		User:            &user.DefaultInfo{Name: "alice"},
+		ResourceRequest: true,
+		Verb:            strings.Repeat("v", 2000),
+		APIGroup:        strings.Repeat("g", 2000),
+		APIVersion:      strings.Repeat("a", 2000),
+		Resource:        strings.Repeat("r", 2000),
+		Name:            strings.Repeat("n", 2000),
+	}
+	bobRidiculousAttr := authorizer.AttributesRecord{
+		User:            &user.DefaultInfo{Name: "bob"},
+		ResourceRequest: true,
+		Verb:            strings.Repeat("v", 2000),
+		APIGroup:        strings.Repeat("g", 2000),
+		APIVersion:      strings.Repeat("a", 2000),
+		Resource:        strings.Repeat("r", 2000),
+		Name:            strings.Repeat("n", 2000),
+	}
+
+	type webhookCacheTestCase struct {
+		name string
+
+		attr authorizer.AttributesRecord
+
+		allow      bool
+		statusCode int
+
+		expectedErr        bool
+		expectedAuthorized bool
+		expectedCalls      int
+	}
 
 	tests := []webhookCacheTestCase{
 		// server error and 429's retry
-		{attr: aliceAttr, allow: false, statusCode: 500, expectedErr: true, expectedAuthorized: false, expectedCalls: 5},
+		{name: "server errors retry", attr: aliceAttr, allow: false, statusCode: 500, expectedErr: true, expectedAuthorized: false, expectedCalls: 5},
-		{attr: aliceAttr, allow: false, statusCode: 429, expectedErr: true, expectedAuthorized: false, expectedCalls: 5},
+		{name: "429s retry", attr: aliceAttr, allow: false, statusCode: 429, expectedErr: true, expectedAuthorized: false, expectedCalls: 5},
 		// regular errors return errors but do not retry
-		{attr: aliceAttr, allow: false, statusCode: 404, expectedErr: true, expectedAuthorized: false, expectedCalls: 1},
+		{name: "404 doesnt retry", attr: aliceAttr, allow: false, statusCode: 404, expectedErr: true, expectedAuthorized: false, expectedCalls: 1},
-		{attr: aliceAttr, allow: false, statusCode: 403, expectedErr: true, expectedAuthorized: false, expectedCalls: 1},
+		{name: "403 doesnt retry", attr: aliceAttr, allow: false, statusCode: 403, expectedErr: true, expectedAuthorized: false, expectedCalls: 1},
-		{attr: aliceAttr, allow: false, statusCode: 401, expectedErr: true, expectedAuthorized: false, expectedCalls: 1},
+		{name: "401 doesnt retry", attr: aliceAttr, allow: false, statusCode: 401, expectedErr: true, expectedAuthorized: false, expectedCalls: 1},
 		// successful responses are cached
-		{attr: aliceAttr, allow: true, statusCode: 200, expectedErr: false, expectedAuthorized: true, expectedCalls: 1},
+		{name: "alice successful request", attr: aliceAttr, allow: true, statusCode: 200, expectedErr: false, expectedAuthorized: true, expectedCalls: 1},
 		// later requests within the cache window don't hit the backend
-		{attr: aliceAttr, allow: false, statusCode: 500, expectedErr: false, expectedAuthorized: true, expectedCalls: 0},
+		{name: "alice cached request", attr: aliceAttr, allow: false, statusCode: 500, expectedErr: false, expectedAuthorized: true, expectedCalls: 0},
 
 		// a request with different attributes doesn't hit the cache
-		{attr: bobAttr, allow: false, statusCode: 500, expectedErr: true, expectedAuthorized: false, expectedCalls: 5},
+		{name: "bob failed request", attr: bobAttr, allow: false, statusCode: 500, expectedErr: true, expectedAuthorized: false, expectedCalls: 5},
 		// successful response for other attributes is cached
-		{attr: bobAttr, allow: true, statusCode: 200, expectedErr: false, expectedAuthorized: true, expectedCalls: 1},
+		{name: "bob unauthorized request", attr: bobAttr, allow: false, statusCode: 200, expectedErr: false, expectedAuthorized: false, expectedCalls: 1},
 		// later requests within the cache window don't hit the backend
-		{attr: bobAttr, allow: false, statusCode: 500, expectedErr: false, expectedAuthorized: true, expectedCalls: 0},
+		{name: "bob unauthorized cached request", attr: bobAttr, allow: false, statusCode: 500, expectedErr: false, expectedAuthorized: false, expectedCalls: 0},
+		// ridiculous unauthorized requests are not cached.
+		{name: "ridiculous unauthorized request", attr: bobRidiculousAttr, allow: false, statusCode: 200, expectedErr: false, expectedAuthorized: false, expectedCalls: 1},
+		// later ridiculous requests within the cache window still hit the backend
+		{name: "ridiculous unauthorized request again", attr: bobRidiculousAttr, allow: false, statusCode: 200, expectedErr: false, expectedAuthorized: false, expectedCalls: 1},
+		// ridiculous authorized requests are not cached.
+		{name: "ridiculous authorized request", attr: aliceRidiculousAttr, allow: true, statusCode: 200, expectedErr: false, expectedAuthorized: true, expectedCalls: 1},
+		// later ridiculous requests within the cache window still hit the backend
+		{name: "ridiculous authorized request again", attr: aliceRidiculousAttr, allow: true, statusCode: 200, expectedErr: false, expectedAuthorized: true, expectedCalls: 1},
 	}
 
-	testWebhookCacheCases(t, serv, wh, tests)
+	for i, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			serv.called = 0
+			serv.allow = test.allow
+			serv.statusCode = test.statusCode
+			authorized, _, err := wh.Authorize(test.attr)
+			if test.expectedErr && err == nil {
+				t.Fatalf("%d: Expected error", i)
+			} else if !test.expectedErr && err != nil {
+				t.Fatalf("%d: unexpected error: %v", i, err)
+			}
+
+			if test.expectedAuthorized != (authorized == authorizer.DecisionAllow) {
+				t.Errorf("%d: expected authorized=%v, got %v", i, test.expectedAuthorized, authorized)
+			}
+
+			if test.expectedCalls != serv.called {
+				t.Errorf("%d: expected %d calls, got %d", i, test.expectedCalls, serv.called)
+			}
+		})
+	}
 }