Merge pull request #15294 from mitake/range-check

server/auth: disallow creating empty permission ranges

server/auth/range_perm_cache.go

@@ -75,8 +75,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)
@@ -155,3 +157,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, <open ended>), 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
+}

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)
+			}
+		})
+	}
+}

server/auth/store.go

@@ -796,6 +796,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.Lock()

server/auth/store_test.go

@@ -17,6 +17,7 @@ package auth
 import (
 	"context"
 	"encoding/base64"
+	"errors"
 	"fmt"
 	"strings"
 	"sync"
@@ -547,6 +548,144 @@ func TestRoleGrantPermission(t *testing.T) {
 	assert.Equal(t, perm, r.Perm[0])
 }
 
+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 TestRootRoleGrantPermission(t *testing.T) {
 	as, tearDown := setupAuthStore(t)
 	defer tearDown(t)

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)