diff --git a/server/auth/range_perm_cache.go b/server/auth/range_perm_cache.go index 3b2628f4fc8..e096710c32e 100644 --- a/server/auth/range_perm_cache.go +++ b/server/auth/range_perm_cache.go @@ -76,8 +76,10 @@ func checkKeyInterval( cachedPerms *unifiedRangePermissions, key, rangeEnd []byte, permtyp authpb.Permission_Type) bool { - if len(rangeEnd) == 1 && rangeEnd[0] == 0 { + if isOpenEnded(rangeEnd) { rangeEnd = nil + // nil rangeEnd will be converetd to []byte{}, the largest element of BytesAffineComparable, + // in NewBytesAffineInterval(). } ivl := adt.NewBytesAffineInterval(key, rangeEnd) @@ -153,3 +155,51 @@ type unifiedRangePermissions struct { readPerms adt.IntervalTree writePerms adt.IntervalTree } + +// Constraints related to key range +// Assumptions: +// a1. key must be non-nil +// a2. []byte{} (in the case of string, "") is not a valid key of etcd +// For representing an open-ended range, BytesAffineComparable uses []byte{} as the largest element. +// a3. []byte{0x00} is the minimum valid etcd key +// +// Based on the above assumptions, key and rangeEnd must follow below rules: +// b1. for representing a single key point, rangeEnd should be nil or zero length byte array (in the case of string, "") +// Rule a2 guarantees that (X, []byte{}) for any X is not a valid range. So such ranges can be used for representing +// a single key permission. +// +// b2. key range with upper limit, like (X, Y), larger or equal to X and smaller than Y +// +// b3. key range with open-ended, like (X, ), is represented like (X, []byte{0x00}) +// Because of rule a3, if we have (X, []byte{0x00}), such a range represents an empty range and makes no sense to have +// such a permission. So we use []byte{0x00} for representing an open-ended permission. +// Note that rangeEnd with []byte{0x00} will be converted into []byte{} before inserted into the interval tree +// (rule a2 ensures that this is the largest element). +// Special range like key = []byte{0x00} and rangeEnd = []byte{0x00} is treated as a range which matches with all keys. +// +// Treating a range whose rangeEnd with []byte{0x00} as an open-ended comes from the rules of Range() and Watch() API. + +func isOpenEnded(rangeEnd []byte) bool { // check rule b3 + return len(rangeEnd) == 1 && rangeEnd[0] == 0 +} + +func isValidPermissionRange(key, rangeEnd []byte) bool { + if len(key) == 0 { + return false + } + if rangeEnd == nil || len(rangeEnd) == 0 { // ensure rule b1 + return true + } + + begin := adt.BytesAffineComparable(key) + end := adt.BytesAffineComparable(rangeEnd) + if begin.Compare(end) == -1 { // rule b2 + return true + } + + if isOpenEnded(rangeEnd) { + return true + } + + return false +} diff --git a/server/auth/range_perm_cache_test.go b/server/auth/range_perm_cache_test.go index 2f325159309..b8859ee48f4 100644 --- a/server/auth/range_perm_cache_test.go +++ b/server/auth/range_perm_cache_test.go @@ -45,6 +45,26 @@ func TestRangePermission(t *testing.T) { []byte("a"), []byte("f"), true, }, + { + []adt.Interval{adt.NewBytesAffineInterval([]byte("a"), []byte("d")), adt.NewBytesAffineInterval([]byte("a"), []byte("b")), adt.NewBytesAffineInterval([]byte("c"), []byte("f"))}, + []byte("a"), []byte{}, + false, + }, + { + []adt.Interval{adt.NewBytesAffineInterval([]byte("a"), []byte{})}, + []byte("a"), []byte{}, + true, + }, + { + []adt.Interval{adt.NewBytesAffineInterval([]byte{0x00}, []byte{})}, + []byte("a"), []byte{}, + true, + }, + { + []adt.Interval{adt.NewBytesAffineInterval([]byte{0x00}, []byte{})}, + []byte{0x00}, []byte{}, + true, + }, } for i, tt := range tests { @@ -86,6 +106,16 @@ func TestKeyPermission(t *testing.T) { []byte("f"), false, }, + { + []adt.Interval{adt.NewBytesAffineInterval([]byte("a"), []byte("d")), adt.NewBytesAffineInterval([]byte("a"), []byte("b")), adt.NewBytesAffineInterval([]byte("c"), []byte{})}, + []byte("f"), + true, + }, + { + []adt.Interval{adt.NewBytesAffineInterval([]byte("a"), []byte("d")), adt.NewBytesAffineInterval([]byte("a"), []byte("b")), adt.NewBytesAffineInterval([]byte{0x00}, []byte{})}, + []byte("f"), + true, + }, } for i, tt := range tests { @@ -100,3 +130,88 @@ func TestKeyPermission(t *testing.T) { } } } + +func TestRangeCheck(t *testing.T) { + tests := []struct { + name string + key []byte + rangeEnd []byte + want bool + }{ + { + name: "valid single key", + key: []byte("a"), + rangeEnd: []byte(""), + want: true, + }, + { + name: "valid single key", + key: []byte("a"), + rangeEnd: nil, + want: true, + }, + { + name: "valid key range, key < rangeEnd", + key: []byte("a"), + rangeEnd: []byte("b"), + want: true, + }, + { + name: "invalid empty key range, key == rangeEnd", + key: []byte("a"), + rangeEnd: []byte("a"), + want: false, + }, + { + name: "invalid empty key range, key > rangeEnd", + key: []byte("b"), + rangeEnd: []byte("a"), + want: false, + }, + { + name: "invalid key, key must not be \"\"", + key: []byte(""), + rangeEnd: []byte("a"), + want: false, + }, + { + name: "invalid key range, key must not be \"\"", + key: []byte(""), + rangeEnd: []byte(""), + want: false, + }, + { + name: "invalid key range, key must not be \"\"", + key: []byte(""), + rangeEnd: []byte("\x00"), + want: false, + }, + { + name: "valid single key (not useful in practice)", + key: []byte("\x00"), + rangeEnd: []byte(""), + want: true, + }, + { + name: "valid key range, larger or equals to \"a\"", + key: []byte("a"), + rangeEnd: []byte("\x00"), + want: true, + }, + { + name: "valid key range, which includes all keys", + key: []byte("\x00"), + rangeEnd: []byte("\x00"), + want: true, + }, + } + + for i, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := isValidPermissionRange(tt.key, tt.rangeEnd) + if result != tt.want { + t.Errorf("#%d: result=%t, want=%t", i, result, tt.want) + } + }) + } +} diff --git a/server/auth/store.go b/server/auth/store.go index 25e3b01827b..4a11aa45531 100644 --- a/server/auth/store.go +++ b/server/auth/store.go @@ -791,6 +791,9 @@ func (as *authStore) RoleGrantPermission(r *pb.AuthRoleGrantPermissionRequest) ( if r.Perm == nil { return nil, ErrPermissionNotGiven } + if !isValidPermissionRange(r.Perm.Key, r.Perm.RangeEnd) { + return nil, ErrInvalidAuthMgmt + } tx := as.be.BatchTx() tx.LockInsideApply() diff --git a/server/auth/store_test.go b/server/auth/store_test.go index 9daeb4ae0e0..75b5adeab43 100644 --- a/server/auth/store_test.go +++ b/server/auth/store_test.go @@ -17,6 +17,7 @@ package auth import ( "context" "encoding/base64" + "errors" "fmt" "reflect" "strings" @@ -541,6 +542,144 @@ func TestRoleGrantPermission(t *testing.T) { } } +func TestRoleGrantInvalidPermission(t *testing.T) { + as, tearDown := setupAuthStore(t) + defer tearDown(t) + + _, err := as.RoleAdd(&pb.AuthRoleAddRequest{Name: "role-test-1"}) + if err != nil { + t.Fatal(err) + } + + tests := []struct { + name string + perm *authpb.Permission + want error + }{ + { + name: "valid range", + perm: &authpb.Permission{ + PermType: authpb.WRITE, + Key: []byte("Keys"), + RangeEnd: []byte("RangeEnd"), + }, + want: nil, + }, + { + name: "invalid range: nil key", + perm: &authpb.Permission{ + PermType: authpb.WRITE, + Key: nil, + RangeEnd: []byte("RangeEnd"), + }, + want: ErrInvalidAuthMgmt, + }, + { + name: "valid range: single key", + perm: &authpb.Permission{ + PermType: authpb.WRITE, + Key: []byte("Keys"), + RangeEnd: nil, + }, + want: nil, + }, + { + name: "valid range: single key", + perm: &authpb.Permission{ + PermType: authpb.WRITE, + Key: []byte("Keys"), + RangeEnd: []byte{}, + }, + want: nil, + }, + { + name: "invalid range: empty (Key == RangeEnd)", + perm: &authpb.Permission{ + PermType: authpb.WRITE, + Key: []byte("a"), + RangeEnd: []byte("a"), + }, + want: ErrInvalidAuthMgmt, + }, + { + name: "invalid range: empty (Key > RangeEnd)", + perm: &authpb.Permission{ + PermType: authpb.WRITE, + Key: []byte("b"), + RangeEnd: []byte("a"), + }, + want: ErrInvalidAuthMgmt, + }, + { + name: "invalid range: length of key is 0", + perm: &authpb.Permission{ + PermType: authpb.WRITE, + Key: []byte(""), + RangeEnd: []byte("a"), + }, + want: ErrInvalidAuthMgmt, + }, + { + name: "invalid range: length of key is 0", + perm: &authpb.Permission{ + PermType: authpb.WRITE, + Key: []byte(""), + RangeEnd: []byte(""), + }, + want: ErrInvalidAuthMgmt, + }, + { + name: "invalid range: length of key is 0", + perm: &authpb.Permission{ + PermType: authpb.WRITE, + Key: []byte(""), + RangeEnd: []byte{0x00}, + }, + want: ErrInvalidAuthMgmt, + }, + { + name: "valid range: single key permission for []byte{0x00}", + perm: &authpb.Permission{ + PermType: authpb.WRITE, + Key: []byte{0x00}, + RangeEnd: []byte(""), + }, + want: nil, + }, + { + name: "valid range: \"a\" or larger keys", + perm: &authpb.Permission{ + PermType: authpb.WRITE, + Key: []byte("a"), + RangeEnd: []byte{0x00}, + }, + want: nil, + }, + { + name: "valid range: the entire keys", + perm: &authpb.Permission{ + PermType: authpb.WRITE, + Key: []byte{0x00}, + RangeEnd: []byte{0x00}, + }, + want: nil, + }, + } + + for i, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + _, err = as.RoleGrantPermission(&pb.AuthRoleGrantPermissionRequest{ + Name: "role-test-1", + Perm: tt.perm, + }) + + if !errors.Is(err, tt.want) { + t.Errorf("#%d: result=%t, want=%t", i, err, tt.want) + } + }) + } +} + func TestRoleRevokePermission(t *testing.T) { as, tearDown := setupAuthStore(t) defer tearDown(t) diff --git a/tests/integration/v3_auth_test.go b/tests/integration/v3_auth_test.go index 10c5dadb4ef..9a60ab02953 100644 --- a/tests/integration/v3_auth_test.go +++ b/tests/integration/v3_auth_test.go @@ -406,7 +406,7 @@ func TestV3AuthOldRevConcurrent(t *testing.T) { role, user := fmt.Sprintf("test-role-%d", i), fmt.Sprintf("test-user-%d", i) _, err := c.RoleAdd(context.TODO(), role) testutil.AssertNil(t, err) - _, err = c.RoleGrantPermission(context.TODO(), role, "", clientv3.GetPrefixRangeEnd(""), clientv3.PermissionType(clientv3.PermReadWrite)) + _, err = c.RoleGrantPermission(context.TODO(), role, "\x00", clientv3.GetPrefixRangeEnd(""), clientv3.PermissionType(clientv3.PermReadWrite)) testutil.AssertNil(t, err) _, err = c.UserAdd(context.TODO(), user, "123") testutil.AssertNil(t, err)