From d512173c86708ca83983c4307edd817a6bf109d5 Mon Sep 17 00:00:00 2001
From: Tim Allclair <tallclair@google.com>
Date: Thu, 24 Jan 2019 13:37:30 -0800
Subject: [PATCH] Apply caching limits to authorized requests too

---
 .../plugin/pkg/authorizer/webhook/webhook.go  | 19 +++++++++++--------
 .../pkg/authorizer/webhook/webhook_test.go    |  6 +++---
 2 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/staging/src/k8s.io/apiserver/plugin/pkg/authorizer/webhook/webhook.go b/staging/src/k8s.io/apiserver/plugin/pkg/authorizer/webhook/webhook.go
index 299f30b948a..52da85980a4 100644
--- a/staging/src/k8s.io/apiserver/plugin/pkg/authorizer/webhook/webhook.go
+++ b/staging/src/k8s.io/apiserver/plugin/pkg/authorizer/webhook/webhook.go
@@ -41,8 +41,8 @@ var (
 
 const (
 	retryBackoff = 500 * time.Millisecond
-	// The maximum key length for unauthorized request keys to qualify for caching.
-	maxUnauthorizedCachedKeySize = 10000
+	// The maximum length of requester-controlled attributes to allow caching.
+	maxControlledAttrCacheSize = 10000
 )
 
 // Ensure Webhook implements the authorizer.Authorizer interface.
@@ -197,10 +197,10 @@ func (w *WebhookAuthorizer) Authorize(attr authorizer.Attributes) (decision auth
 			return w.decisionOnError, "", err
 		}
 		r.Status = result.Status
-		if r.Status.Allowed {
-			w.responseCache.Add(string(key), r.Status, w.authorizedTTL)
-		} else {
-			if callerControlledAttributeSize(attr) < maxUnauthorizedCachedKeySize {
+		if shouldCache(attr) {
+			if r.Status.Allowed {
+				w.responseCache.Add(string(key), r.Status, w.authorizedTTL)
+			} else {
 				w.responseCache.Add(string(key), r.Status, w.unauthorizedTTL)
 			}
 		}
@@ -269,8 +269,10 @@ func (t *subjectAccessReviewClient) Create(subjectAccessReview *authorization.Su
 	return result, err
 }
 
-func callerControlledAttributeSize(attr authorizer.Attributes) int64 {
-	return int64(len(attr.GetNamespace())) +
+// 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())) +
@@ -278,4 +280,5 @@ func callerControlledAttributeSize(attr authorizer.Attributes) int64 {
 		int64(len(attr.GetSubresource())) +
 		int64(len(attr.GetName())) +
 		int64(len(attr.GetPath()))
+	return controlledAttrSize < maxControlledAttrCacheSize
 }
diff --git a/staging/src/k8s.io/apiserver/plugin/pkg/authorizer/webhook/webhook_test.go b/staging/src/k8s.io/apiserver/plugin/pkg/authorizer/webhook/webhook_test.go
index 440587d1ec4..18f0e5868fb 100644
--- a/staging/src/k8s.io/apiserver/plugin/pkg/authorizer/webhook/webhook_test.go
+++ b/staging/src/k8s.io/apiserver/plugin/pkg/authorizer/webhook/webhook_test.go
@@ -616,10 +616,10 @@ func TestWebhookCache(t *testing.T) {
 		{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 still cached.
+		// 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 don't hit the backend
-		{name: "ridiculous authorized request again", attr: aliceRidiculousAttr, allow: false, statusCode: 500, expectedErr: false, expectedAuthorized: true, expectedCalls: 0},
+		// 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},
 	}
 
 	for i, test := range tests {