Skip to content

Commit 0b9f132

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request kubernetes#70302 from tallclair/authzcache
Don't cache rediculous subject access reviews
2 parents b96378c + d512173 commit 0b9f132

File tree

2 files changed

+96
-51
lines changed

2 files changed

+96
-51
lines changed

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

Lines changed: 25 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,11 @@ var (
3939
groupVersions = []schema.GroupVersion{authorization.SchemeGroupVersion}
4040
)
4141

42-
const retryBackoff = 500 * time.Millisecond
42+
const (
43+
retryBackoff = 500 * time.Millisecond
44+
// The maximum length of requester-controlled attributes to allow caching.
45+
maxControlledAttrCacheSize = 10000
46+
)
4347

4448
// Ensure Webhook implements the authorizer.Authorizer interface.
4549
var _ authorizer.Authorizer = (*WebhookAuthorizer)(nil)
@@ -193,10 +197,12 @@ func (w *WebhookAuthorizer) Authorize(attr authorizer.Attributes) (decision auth
193197
return w.decisionOnError, "", err
194198
}
195199
r.Status = result.Status
196-
if r.Status.Allowed {
197-
w.responseCache.Add(string(key), r.Status, w.authorizedTTL)
198-
} else {
199-
w.responseCache.Add(string(key), r.Status, w.unauthorizedTTL)
200+
if shouldCache(attr) {
201+
if r.Status.Allowed {
202+
w.responseCache.Add(string(key), r.Status, w.authorizedTTL)
203+
} else {
204+
w.responseCache.Add(string(key), r.Status, w.unauthorizedTTL)
205+
}
200206
}
201207
}
202208
switch {
@@ -262,3 +268,17 @@ func (t *subjectAccessReviewClient) Create(subjectAccessReview *authorization.Su
262268
err := t.w.RestClient.Post().Body(subjectAccessReview).Do().Into(result)
263269
return result, err
264270
}
271+
272+
// shouldCache determines whether it is safe to cache the given request attributes. If the
273+
// requester-controlled attributes are too large, this may be a DoS attempt, so we skip the cache.
274+
func shouldCache(attr authorizer.Attributes) bool {
275+
controlledAttrSize := int64(len(attr.GetNamespace())) +
276+
int64(len(attr.GetVerb())) +
277+
int64(len(attr.GetAPIGroup())) +
278+
int64(len(attr.GetAPIVersion())) +
279+
int64(len(attr.GetResource())) +
280+
int64(len(attr.GetSubresource())) +
281+
int64(len(attr.GetName())) +
282+
int64(len(attr.GetPath()))
283+
return controlledAttrSize < maxControlledAttrCacheSize
284+
}

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

Lines changed: 71 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@ import (
2828
"os"
2929
"path/filepath"
3030
"reflect"
31+
"strings"
3132
"testing"
3233
"text/template"
3334
"time"
@@ -542,41 +543,6 @@ func TestWebhook(t *testing.T) {
542543
}
543544
}
544545

545-
type webhookCacheTestCase struct {
546-
attr authorizer.AttributesRecord
547-
548-
allow bool
549-
statusCode int
550-
551-
expectedErr bool
552-
expectedAuthorized bool
553-
expectedCalls int
554-
}
555-
556-
func testWebhookCacheCases(t *testing.T, serv *mockService, wh *WebhookAuthorizer, tests []webhookCacheTestCase) {
557-
for i, test := range tests {
558-
serv.called = 0
559-
serv.allow = test.allow
560-
serv.statusCode = test.statusCode
561-
authorized, _, err := wh.Authorize(test.attr)
562-
if test.expectedErr && err == nil {
563-
t.Errorf("%d: Expected error", i)
564-
continue
565-
} else if !test.expectedErr && err != nil {
566-
t.Errorf("%d: unexpected error: %v", i, err)
567-
continue
568-
}
569-
570-
if test.expectedAuthorized != (authorized == authorizer.DecisionAllow) {
571-
t.Errorf("%d: expected authorized=%v, got %v", i, test.expectedAuthorized, authorized)
572-
}
573-
574-
if test.expectedCalls != serv.called {
575-
t.Errorf("%d: expected %d calls, got %d", i, test.expectedCalls, serv.called)
576-
}
577-
}
578-
}
579-
580546
// TestWebhookCache verifies that error responses from the server are not
581547
// cached, but successful responses are.
582548
func TestWebhookCache(t *testing.T) {
@@ -595,27 +561,86 @@ func TestWebhookCache(t *testing.T) {
595561

596562
aliceAttr := authorizer.AttributesRecord{User: &user.DefaultInfo{Name: "alice"}}
597563
bobAttr := authorizer.AttributesRecord{User: &user.DefaultInfo{Name: "bob"}}
564+
aliceRidiculousAttr := authorizer.AttributesRecord{
565+
User: &user.DefaultInfo{Name: "alice"},
566+
ResourceRequest: true,
567+
Verb: strings.Repeat("v", 2000),
568+
APIGroup: strings.Repeat("g", 2000),
569+
APIVersion: strings.Repeat("a", 2000),
570+
Resource: strings.Repeat("r", 2000),
571+
Name: strings.Repeat("n", 2000),
572+
}
573+
bobRidiculousAttr := authorizer.AttributesRecord{
574+
User: &user.DefaultInfo{Name: "bob"},
575+
ResourceRequest: true,
576+
Verb: strings.Repeat("v", 2000),
577+
APIGroup: strings.Repeat("g", 2000),
578+
APIVersion: strings.Repeat("a", 2000),
579+
Resource: strings.Repeat("r", 2000),
580+
Name: strings.Repeat("n", 2000),
581+
}
582+
583+
type webhookCacheTestCase struct {
584+
name string
585+
586+
attr authorizer.AttributesRecord
587+
588+
allow bool
589+
statusCode int
590+
591+
expectedErr bool
592+
expectedAuthorized bool
593+
expectedCalls int
594+
}
598595

599596
tests := []webhookCacheTestCase{
600597
// server error and 429's retry
601-
{attr: aliceAttr, allow: false, statusCode: 500, expectedErr: true, expectedAuthorized: false, expectedCalls: 5},
602-
{attr: aliceAttr, allow: false, statusCode: 429, expectedErr: true, expectedAuthorized: false, expectedCalls: 5},
598+
{name: "server errors retry", attr: aliceAttr, allow: false, statusCode: 500, expectedErr: true, expectedAuthorized: false, expectedCalls: 5},
599+
{name: "429s retry", attr: aliceAttr, allow: false, statusCode: 429, expectedErr: true, expectedAuthorized: false, expectedCalls: 5},
603600
// regular errors return errors but do not retry
604-
{attr: aliceAttr, allow: false, statusCode: 404, expectedErr: true, expectedAuthorized: false, expectedCalls: 1},
605-
{attr: aliceAttr, allow: false, statusCode: 403, expectedErr: true, expectedAuthorized: false, expectedCalls: 1},
606-
{attr: aliceAttr, allow: false, statusCode: 401, expectedErr: true, expectedAuthorized: false, expectedCalls: 1},
601+
{name: "404 doesnt retry", attr: aliceAttr, allow: false, statusCode: 404, expectedErr: true, expectedAuthorized: false, expectedCalls: 1},
602+
{name: "403 doesnt retry", attr: aliceAttr, allow: false, statusCode: 403, expectedErr: true, expectedAuthorized: false, expectedCalls: 1},
603+
{name: "401 doesnt retry", attr: aliceAttr, allow: false, statusCode: 401, expectedErr: true, expectedAuthorized: false, expectedCalls: 1},
607604
// successful responses are cached
608-
{attr: aliceAttr, allow: true, statusCode: 200, expectedErr: false, expectedAuthorized: true, expectedCalls: 1},
605+
{name: "alice successful request", attr: aliceAttr, allow: true, statusCode: 200, expectedErr: false, expectedAuthorized: true, expectedCalls: 1},
609606
// later requests within the cache window don't hit the backend
610-
{attr: aliceAttr, allow: false, statusCode: 500, expectedErr: false, expectedAuthorized: true, expectedCalls: 0},
607+
{name: "alice cached request", attr: aliceAttr, allow: false, statusCode: 500, expectedErr: false, expectedAuthorized: true, expectedCalls: 0},
611608

612609
// a request with different attributes doesn't hit the cache
613-
{attr: bobAttr, allow: false, statusCode: 500, expectedErr: true, expectedAuthorized: false, expectedCalls: 5},
610+
{name: "bob failed request", attr: bobAttr, allow: false, statusCode: 500, expectedErr: true, expectedAuthorized: false, expectedCalls: 5},
614611
// successful response for other attributes is cached
615-
{attr: bobAttr, allow: true, statusCode: 200, expectedErr: false, expectedAuthorized: true, expectedCalls: 1},
612+
{name: "bob unauthorized request", attr: bobAttr, allow: false, statusCode: 200, expectedErr: false, expectedAuthorized: false, expectedCalls: 1},
616613
// later requests within the cache window don't hit the backend
617-
{attr: bobAttr, allow: false, statusCode: 500, expectedErr: false, expectedAuthorized: true, expectedCalls: 0},
614+
{name: "bob unauthorized cached request", attr: bobAttr, allow: false, statusCode: 500, expectedErr: false, expectedAuthorized: false, expectedCalls: 0},
615+
// ridiculous unauthorized requests are not cached.
616+
{name: "ridiculous unauthorized request", attr: bobRidiculousAttr, allow: false, statusCode: 200, expectedErr: false, expectedAuthorized: false, expectedCalls: 1},
617+
// later ridiculous requests within the cache window still hit the backend
618+
{name: "ridiculous unauthorized request again", attr: bobRidiculousAttr, allow: false, statusCode: 200, expectedErr: false, expectedAuthorized: false, expectedCalls: 1},
619+
// ridiculous authorized requests are not cached.
620+
{name: "ridiculous authorized request", attr: aliceRidiculousAttr, allow: true, statusCode: 200, expectedErr: false, expectedAuthorized: true, expectedCalls: 1},
621+
// later ridiculous requests within the cache window still hit the backend
622+
{name: "ridiculous authorized request again", attr: aliceRidiculousAttr, allow: true, statusCode: 200, expectedErr: false, expectedAuthorized: true, expectedCalls: 1},
618623
}
619624

620-
testWebhookCacheCases(t, serv, wh, tests)
625+
for i, test := range tests {
626+
t.Run(test.name, func(t *testing.T) {
627+
serv.called = 0
628+
serv.allow = test.allow
629+
serv.statusCode = test.statusCode
630+
authorized, _, err := wh.Authorize(test.attr)
631+
if test.expectedErr && err == nil {
632+
t.Fatalf("%d: Expected error", i)
633+
} else if !test.expectedErr && err != nil {
634+
t.Fatalf("%d: unexpected error: %v", i, err)
635+
}
636+
637+
if test.expectedAuthorized != (authorized == authorizer.DecisionAllow) {
638+
t.Errorf("%d: expected authorized=%v, got %v", i, test.expectedAuthorized, authorized)
639+
}
640+
641+
if test.expectedCalls != serv.called {
642+
t.Errorf("%d: expected %d calls, got %d", i, test.expectedCalls, serv.called)
643+
}
644+
})
645+
}
621646
}

0 commit comments

Comments
 (0)