From ea1b4eb2394a1ee5a3847f92382b30e32eee4d47 Mon Sep 17 00:00:00 2001 From: Tim Allclair Date: Fri, 26 Oct 2018 13:18:06 -0700 Subject: [PATCH] Don't cache rediculous subject access reviews --- .../plugin/pkg/authorizer/webhook/webhook.go | 10 +- .../pkg/authorizer/webhook/webhook_test.go | 117 +++++++++++------- 2 files changed, 79 insertions(+), 48 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 e05ef503f22..b9cc4c65d98 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 @@ -39,7 +39,11 @@ var ( groupVersions = []schema.GroupVersion{authorization.SchemeGroupVersion} ) -const retryBackoff = 500 * time.Millisecond +const ( + retryBackoff = 500 * time.Millisecond + // The maximum key length for unauthorized request keys to qualify for caching. + maxUnauthorizedCachedKeySize = 10000 +) // Ensure Webhook implements the authorizer.Authorizer interface. var _ authorizer.Authorizer = (*WebhookAuthorizer)(nil) @@ -196,7 +200,9 @@ func (w *WebhookAuthorizer) Authorize(attr authorizer.Attributes) (decision auth if r.Status.Allowed { w.responseCache.Add(string(key), r.Status, w.authorizedTTL) } else { - w.responseCache.Add(string(key), r.Status, w.unauthorizedTTL) + if len(key) <= maxUnauthorizedCachedKeySize { + w.responseCache.Add(string(key), r.Status, w.unauthorizedTTL) + } } } switch { 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 7fecf7796f8..440587d1ec4 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 @@ -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}, - {attr: aliceAttr, allow: false, statusCode: 429, expectedErr: true, expectedAuthorized: false, expectedCalls: 5}, + {name: "server errors retry", attr: aliceAttr, allow: false, statusCode: 500, 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}, - {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: "404 doesnt retry", attr: aliceAttr, allow: false, statusCode: 404, expectedErr: true, expectedAuthorized: false, expectedCalls: 1}, + {name: "403 doesnt retry", attr: aliceAttr, allow: false, statusCode: 403, 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 still 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}, } - 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) + } + }) + } }