Skip to content

fix(coderd/rbac): do not cache context cancellation errors #11840

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jan 26, 2024
Merged

fix(coderd/rbac): do not cache context cancellation errors #11840

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 11 additions & 6 deletions coderd/rbac/authz.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"crypto/sha256"
_ "embed"
"encoding/json"
"errors"
"strings"
"sync"
"time"
Expand Down Expand Up @@ -653,10 +654,10 @@ type authCache struct {
authz Authorizer
}

// Cacher returns an Authorizer that can use a cache stored on a context
// to short circuit duplicate calls to the Authorizer. This is useful when
// multiple calls are made to the Authorizer for the same subject, action, and
// object. The cache is on each `ctx` and is not shared between requests.
// Cacher returns an Authorizer that can use a cache to short circuit duplicate
// calls to the Authorizer. This is useful when multiple calls are made to the
// Authorizer for the same subject, action, and object.
// This is a GLOBAL cache shared between all requests.
// If no cache is found on the context, the Authorizer is called as normal.
//
// Cacher is safe for multiple actors.
Expand All @@ -676,8 +677,12 @@ func (c *authCache) Authorize(ctx context.Context, subject Subject, action Actio
err, _, ok := c.cache.Get(authorizeCacheKey)
if !ok {
err = c.authz.Authorize(ctx, subject, action, object)
// In case there is a caching bug, bound the TTL to 1 minute.
c.cache.Set(authorizeCacheKey, err, time.Minute)
// If there is a transient error such as a context cancellation, do not
// cache it.
if !errors.Is(err, context.Canceled) {
// In case there is a caching bug, bound the TTL to 1 minute.
c.cache.Set(authorizeCacheKey, err, time.Minute)
}
}

return err
Expand Down
43 changes: 43 additions & 0 deletions coderd/rbac/authz_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,12 @@ import (

"github.com/google/uuid"
"github.com/prometheus/client_golang/prometheus"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

"github.com/coder/coder/v2/coderd/coderdtest"
"github.com/coder/coder/v2/coderd/rbac"
"github.com/coder/coder/v2/testutil"
)

type benchmarkCase struct {
Expand Down Expand Up @@ -351,6 +353,47 @@ func TestCacher(t *testing.T) {
require.NoError(t, rec.AllAsserted(), "all assertions should have been made")
})

t.Run("DontCacheTransientErrors", func(t *testing.T) {
t.Parallel()

var (
ctx = testutil.Context(t, testutil.WaitShort)
authOut = make(chan error, 1) // buffered to not block
authorizeFunc = func(ctx context.Context, subject rbac.Subject, action rbac.Action, object rbac.Object) error {
// Just return what you're told.
return testutil.RequireRecvCtx(ctx, t, authOut)
}
ma = &rbac.MockAuthorizer{AuthorizeFunc: authorizeFunc}
rec = &coderdtest.RecordingAuthorizer{Wrapped: ma}
authz = rbac.Cacher(rec)
subj, obj, action = coderdtest.RandomRBACSubject(), coderdtest.RandomRBACObject(), coderdtest.RandomRBACAction()
)

// First call will result in a transient error. This should not be cached.
testutil.RequireSendCtx(ctx, t, authOut, context.Canceled)
err := authz.Authorize(ctx, subj, action, obj)
assert.ErrorIs(t, err, context.Canceled)

// A subsequent call should still hit the authorizer.
testutil.RequireSendCtx(ctx, t, authOut, nil)
err = authz.Authorize(ctx, subj, action, obj)
assert.NoError(t, err)
// This should be cached and not hit the wrapped authorizer again.
err = authz.Authorize(ctx, subj, action, obj)
assert.NoError(t, err)

// Let's change the subject.
subj, obj, action = coderdtest.RandomRBACSubject(), coderdtest.RandomRBACObject(), coderdtest.RandomRBACAction()

// A third will be a legit error
testutil.RequireSendCtx(ctx, t, authOut, assert.AnError)
err = authz.Authorize(ctx, subj, action, obj)
assert.EqualError(t, err, assert.AnError.Error())
// This should be cached and not hit the wrapped authorizer again.
err = authz.Authorize(ctx, subj, action, obj)
assert.EqualError(t, err, assert.AnError.Error())
})

t.Run("MultipleSubjects", func(t *testing.T) {
t.Parallel()

Expand Down