Skip to content

Commit

Permalink
Fix DeleteRange when the backend sanitizer is used (#11124)
Browse files Browse the repository at this point in the history 
We no longer require the end key for a DeleteRange operation to be
a valid backend key. This prevented us from deleting resources ending
in 'z', because it would produce an end key that the sanitizer doesn't
like.

Updates #2776
  • Loading branch information
@zmb3
zmb3 committed Mar 15, 2022
1 parent ffbc535 commit 288ae9b
Show file tree
Hide file tree
Showing 5 changed files with 103 additions and 65 deletions.
19 changes: 19 additions & 0 deletions lib/backend/backend_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ package backend

import (
"testing"

"github.com/stretchr/testify/require"
)

func TestParams(t *testing.T) {
Expand All @@ -36,3 +38,20 @@ func TestParams(t *testing.T) {
t.Errorf("expected 'path' to be '%v', got '%v'", expectedPath, path)
}
}

func TestRangeEnd(t *testing.T) {
for _, test := range []struct {
key, expected string
}{
{"abc", "abd"},
{"/foo/bar", "/foo/bas"},
{"/xyz", "/xy{"},
{"\xFF", "\x00"},
{"\xFF\xFF\xFF", "\x00"},
} {
t.Run(test.key, func(t *testing.T) {
end := RangeEnd([]byte(test.key))
require.Equal(t, test.expected, string(end))
})
}
}
16 changes: 8 additions & 8 deletions lib/backend/sanitize.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,16 +30,16 @@ import (
// errorMessage is the error message to return when invalid input is provided by the caller.
const errorMessage = "special characters are not allowed in resource names, please use name composed only from characters, hyphens, dots, and plus signs: %q"

// whitelistPattern is the pattern of allowed characters for each key within
// allowPattern is the pattern of allowed characters for each key within
// the path.
var whitelistPattern = regexp.MustCompile(`^[0-9A-Za-z@_:.\-/+]*$`)
var allowPattern = regexp.MustCompile(`^[0-9A-Za-z@_:.\-/+]*$`)

// blacklistPattern matches some unallowed combinations
var blacklistPattern = regexp.MustCompile(`//`)
// denyPattern matches some unallowed combinations
var denyPattern = regexp.MustCompile(`//`)

// isKeySafe checks if the passed in key conforms to whitelist
func isKeySafe(s []byte) bool {
return whitelistPattern.Match(s) && !blacklistPattern.Match(s) && utf8.Valid(s)
return allowPattern.Match(s) && !denyPattern.Match(s) && utf8.Valid(s)
}

// Sanitizer wraps a Backend implementation to make sure all values requested
Expand Down Expand Up @@ -118,12 +118,12 @@ func (s *Sanitizer) Delete(ctx context.Context, key []byte) error {

// DeleteRange deletes range of items
func (s *Sanitizer) DeleteRange(ctx context.Context, startKey []byte, endKey []byte) error {
// we only validate the start key, since we often compute the end key
// in order to delete a bunch of related entries
if !isKeySafe(startKey) {
return trace.BadParameter(errorMessage, startKey)
}
if !isKeySafe(endKey) {
return trace.BadParameter(errorMessage, endKey)
}

return s.backend.DeleteRange(ctx, startKey, endKey)
}

Expand Down
97 changes: 44 additions & 53 deletions lib/backend/sanitize_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,86 +16,75 @@ package backend

import (
"context"
"fmt"
"testing"
"time"

"github.com/jonboulle/clockwork"
"gopkg.in/check.v1"
"github.com/stretchr/testify/require"
)

type Suite struct {
}

var _ = check.Suite(&Suite{})

func (s *Suite) SetUpSuite(c *check.C) {
}

func (s *Suite) TearDownSuite(c *check.C) {
}

func (s *Suite) TearDownTest(c *check.C) {
}

func (s *Suite) SetUpTest(c *check.C) {
}

func (s *Suite) TestSanitizeBucket(c *check.C) {
func TestSanitize(t *testing.T) {
tests := []struct {
inKey []byte
outError bool
inKey []byte
assert require.ErrorAssertionFunc
}{
{
inKey: []byte("a-b/c:d/.e_f/01"),
outError: false,
inKey: []byte("a-b/c:d/.e_f/01"),
assert: require.NoError,
},
{
inKey: []byte("/namespaces//params"),
outError: true,
inKey: []byte("/namespaces//params"),
assert: require.Error,
},
{
inKey: RangeEnd([]byte("a-b/c:d/.e_f/01")),
outError: false,
inKey: RangeEnd([]byte("a-b/c:d/.e_f/01")),
assert: require.NoError,
},
{
inKey: RangeEnd([]byte("/")),
outError: false,
inKey: RangeEnd([]byte("/")),
assert: require.NoError,
},
{
inKey: RangeEnd([]byte("Malformed \xf0\x90\x28\xbc UTF8")),
outError: true,
inKey: RangeEnd([]byte("Malformed \xf0\x90\x28\xbc UTF8")),
assert: require.Error,
},
{
inKey: []byte("test+subaddr@example.com"),
outError: false,
inKey: []byte("test+subaddr@example.com"),
assert: require.NoError,
},
{
inKey: []byte("xyz"),
assert: require.NoError,
},
}

for i, tt := range tests {
comment := check.Commentf("Test %v, key: %q", i, string(tt.inKey))

safeBackend := NewSanitizer(&nopBackend{})
for _, tt := range tests {
t.Run(fmt.Sprintf("%v", string(tt.inKey)), func(t *testing.T) {
ctx := context.Background()
safeBackend := NewSanitizer(&nopBackend{})

ctx := context.TODO()
_, err := safeBackend.Get(ctx, tt.inKey)
c.Assert(err != nil, check.Equals, tt.outError, comment)
_, err := safeBackend.Get(ctx, tt.inKey)
tt.assert(t, err)

_, err = safeBackend.Create(ctx, Item{Key: tt.inKey})
c.Assert(err != nil, check.Equals, tt.outError, comment)
_, err = safeBackend.Create(ctx, Item{Key: tt.inKey})
tt.assert(t, err)

_, err = safeBackend.Put(ctx, Item{Key: tt.inKey})
c.Assert(err != nil, check.Equals, tt.outError, comment)
_, err = safeBackend.Put(ctx, Item{Key: tt.inKey})
tt.assert(t, err)

_, err = safeBackend.Update(ctx, Item{Key: tt.inKey})
c.Assert(err != nil, check.Equals, tt.outError, comment)
_, err = safeBackend.Update(ctx, Item{Key: tt.inKey})
tt.assert(t, err)

_, err = safeBackend.CompareAndSwap(ctx, Item{Key: tt.inKey}, Item{Key: tt.inKey})
c.Assert(err != nil, check.Equals, tt.outError, comment)
_, err = safeBackend.CompareAndSwap(ctx, Item{Key: tt.inKey}, Item{Key: tt.inKey})
tt.assert(t, err)

err = safeBackend.Delete(ctx, tt.inKey)
c.Assert(err != nil, check.Equals, tt.outError, comment)
err = safeBackend.Delete(ctx, tt.inKey)
tt.assert(t, err)

err = safeBackend.DeleteRange(ctx, tt.inKey, tt.inKey)
c.Assert(err != nil, check.Equals, tt.outError, comment)
err = safeBackend.DeleteRange(ctx, tt.inKey, tt.inKey)
tt.assert(t, err)
})
}
}

Expand All @@ -108,7 +97,9 @@ func (n *nopBackend) Get(_ context.Context, _ []byte) (*Item, error) {
}

func (n *nopBackend) GetRange(_ context.Context, startKey []byte, endKey []byte, limit int) (*GetResult, error) {
return &GetResult{Items: []Item{Item{Key: []byte("foo"), Value: []byte("bar")}}}, nil
return &GetResult{Items: []Item{
{Key: []byte("foo"), Value: []byte("bar")},
}}, nil
}

func (n *nopBackend) Create(_ context.Context, _ Item) (*Lease, error) {
Expand Down
2 changes: 2 additions & 0 deletions lib/services/local/users.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -379,6 +379,8 @@ func (s *IdentityService) DeleteUser(ctx context.Context, user string) error {
if err != nil {
return trace.Wrap(err)
}
// each user has multiple related entries in the backend,
// so use DeleteRange to make sure we get them all
startKey := backend.Key(webPrefix, usersPrefix, user)
err = s.DeleteRange(ctx, startKey, backend.RangeEnd(startKey))
return trace.Wrap(err)
Expand Down
34 changes: 30 additions & 4 deletions lib/services/local/users_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import (
"github.com/google/go-cmp/cmp"
"github.com/google/uuid"
"github.com/gravitational/teleport/api/types"
"github.com/gravitational/teleport/lib/backend"
"github.com/gravitational/teleport/lib/backend/lite"
"github.com/gravitational/teleport/lib/services/local"
"github.com/gravitational/trace"
Expand Down Expand Up @@ -60,7 +61,6 @@ func TestRecoveryCodesCRUD(t *testing.T) {
}

t.Run("upsert, get, delete recovery codes", func(t *testing.T) {
t.Parallel()
username := "someuser"

rc1, err := types.NewRecoveryCodes(mockedCodes, clock.Now(), username)
Expand Down Expand Up @@ -100,7 +100,6 @@ func TestRecoveryCodesCRUD(t *testing.T) {
})

t.Run("deleting user deletes recovery codes", func(t *testing.T) {
t.Parallel()
username := "someuser2"

// Create a user.
Expand All @@ -124,6 +123,35 @@ func TestRecoveryCodesCRUD(t *testing.T) {
_, err = identity.GetRecoveryCodes(ctx, username, true /* withSecrets */)
require.True(t, trace.IsNotFound(err))
})

t.Run("deleting user ending with 'z'", func(t *testing.T) {
// enable the sanitizer, and use a key ending with z,
// which will produce an invalid backend key when we
// compute the end range
username := "xyz"
identity.Backend = backend.NewSanitizer(identity.Backend)

// Create a user.
userResource := &types.UserV2{}
userResource.SetName(username)
err := identity.CreateUser(userResource)
require.NoError(t, err)

// Test codes exist for user.
rc1, err := types.NewRecoveryCodes(mockedCodes, clock.Now(), username)
require.NoError(t, err)
err = identity.UpsertRecoveryCodes(ctx, username, rc1)
require.NoError(t, err)
codes, err := identity.GetRecoveryCodes(ctx, username, true /* withSecrets */)
require.NoError(t, err)
require.ElementsMatch(t, mockedCodes, codes.GetCodes())

// Test deletion of recovery code along with user.
err = identity.DeleteUser(ctx, username)
require.NoError(t, err)
_, err = identity.GetRecoveryCodes(ctx, username, true /* withSecrets */)
require.True(t, trace.IsNotFound(err))
})
}

func TestRecoveryAttemptsCRUD(t *testing.T) {
Expand All @@ -137,7 +165,6 @@ func TestRecoveryAttemptsCRUD(t *testing.T) {
time3 := time1.Add(4 * time.Minute)

t.Run("create, get, and delete recovery attempts", func(t *testing.T) {
t.Parallel()
username := "someuser"

// Test creation of recovery attempt.
Expand Down Expand Up @@ -165,7 +192,6 @@ func TestRecoveryAttemptsCRUD(t *testing.T) {
})

t.Run("deleting user deletes recovery attempts", func(t *testing.T) {
t.Parallel()
username := "someuser2"

// Create a user, to test deletion of recovery attempts with user.
Expand Down

0 comments on commit 288ae9b

Please # to comment.