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..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 @@ -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 { - w.responseCache.Add(string(key), r.Status, w.authorizedTTL) - } else { - w.responseCache.Add(string(key), r.Status, w.unauthorizedTTL) + 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) + } } } 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 +} 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..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 @@ -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 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) + } + }) + } }