Skip to content

Commit d671fd0

Browse files
ahrtrahrtr
authored
Merge pull request #21685 from ahrtr/20260429_auth_3.6
[release-3.6] Fix read access via PrevKv or lease attachment in a Put request in etcd transactions bypass RBAC authorization checks
2 parents 16a8a36 + 633de82 commit d671fd0

4 files changed

Lines changed: 117 additions & 13 deletions

File tree

‎server/etcdserver/apply/apply_auth.go‎

Lines changed: 9 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -114,29 +114,29 @@ func (aa *authApplierV3) DeleteRange(r *pb.DeleteRangeRequest) (*pb.DeleteRangeR
114114
}
115115

116116
func (aa *authApplierV3) Txn(rt *pb.TxnRequest) (*pb.TxnResponse, *traceutil.Trace, error) {
117-
if err := CheckTxnAuth(aa.as, &aa.authInfo, rt); err != nil {
117+
if err := CheckTxnAuth(aa.as, &aa.authInfo, aa.lessor, rt); err != nil {
118118
return nil, nil, err
119119
}
120120
return aa.applierV3.Txn(rt)
121121
}
122122

123-
func CheckTxnAuth(as auth.AuthStore, ai *auth.AuthInfo, rt *pb.TxnRequest) error {
124-
return checkTxnPermission(as, ai, rt)
123+
func CheckTxnAuth(as auth.AuthStore, ai *auth.AuthInfo, lessor lease.Lessor, rt *pb.TxnRequest) error {
124+
return checkTxnPermission(as, ai, lessor, rt)
125125
}
126126

127-
func checkTxnPermission(as auth.AuthStore, ai *auth.AuthInfo, rt *pb.TxnRequest) error {
127+
func checkTxnPermission(as auth.AuthStore, ai *auth.AuthInfo, lessor lease.Lessor, rt *pb.TxnRequest) error {
128128
for _, c := range rt.Compare {
129129
if err := as.IsRangePermitted(ai, c.Key, c.RangeEnd); err != nil {
130130
return err
131131
}
132132
}
133-
if err := checkTxnReqsPermission(as, ai, rt.Success); err != nil {
133+
if err := checkTxnReqsPermission(as, ai, lessor, rt.Success); err != nil {
134134
return err
135135
}
136-
return checkTxnReqsPermission(as, ai, rt.Failure)
136+
return checkTxnReqsPermission(as, ai, lessor, rt.Failure)
137137
}
138138

139-
func checkTxnReqsPermission(as auth.AuthStore, ai *auth.AuthInfo, reqs []*pb.RequestOp) error {
139+
func checkTxnReqsPermission(as auth.AuthStore, ai *auth.AuthInfo, lessor lease.Lessor, reqs []*pb.RequestOp) error {
140140
for _, requ := range reqs {
141141
switch tv := requ.Request.(type) {
142142
case *pb.RequestOp_RequestRange:
@@ -153,10 +153,9 @@ func checkTxnReqsPermission(as auth.AuthStore, ai *auth.AuthInfo, reqs []*pb.Req
153153
continue
154154
}
155155

156-
if err := as.IsPutPermitted(ai, tv.RequestPut.Key); err != nil {
156+
if err := checkPutAuth(as, ai, lessor, tv.RequestPut); err != nil {
157157
return err
158158
}
159-
160159
case *pb.RequestOp_RequestDeleteRange:
161160
if tv.RequestDeleteRange == nil {
162161
continue
@@ -178,7 +177,7 @@ func checkTxnReqsPermission(as auth.AuthStore, ai *auth.AuthInfo, reqs []*pb.Req
178177
continue
179178
}
180179

181-
err := checkTxnPermission(as, ai, tv.RequestTxn)
180+
err := checkTxnPermission(as, ai, lessor, tv.RequestTxn)
182181
if err != nil {
183182
return err
184183
}

‎server/etcdserver/apply/apply_auth_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1048,7 +1048,7 @@ func TestCheckTxnAuth(t *testing.T) {
10481048

10491049
for _, tt := range tests {
10501050
t.Run(tt.name, func(t *testing.T) {
1051-
err := CheckTxnAuth(as, &auth.AuthInfo{Username: "foo", Revision: 8}, tt.txnRequest)
1051+
err := CheckTxnAuth(as, &auth.AuthInfo{Username: "foo", Revision: 8}, &lease.FakeLessor{}, tt.txnRequest)
10521052
assert.Equal(t, tt.err, err)
10531053
})
10541054
}

‎server/etcdserver/v3_server.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -174,7 +174,7 @@ func (s *EtcdServer) Txn(ctx context.Context, r *pb.TxnRequest) (*pb.TxnResponse
174174
var resp *pb.TxnResponse
175175
var err error
176176
chk := func(ai *auth.AuthInfo) error {
177-
return apply2.CheckTxnAuth(s.authStore, ai, r)
177+
return apply2.CheckTxnAuth(s.authStore, ai, s.lessor, r)
178178
}
179179

180180
defer func(start time.Time) {

‎tests/integration/v3_auth_test.go‎

Lines changed: 106 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ package integration
1717
import (
1818
"context"
1919
"fmt"
20+
"strings"
2021
"sync"
2122
"testing"
2223
"time"
@@ -188,6 +189,7 @@ type user struct {
188189
name string
189190
password string
190191
role string
192+
perm string
191193
key string
192194
end string
193195
}
@@ -379,8 +381,15 @@ func authSetupUsers(t *testing.T, auth pb.AuthClient, users []user) {
379381
continue
380382
}
381383

384+
permType := authpb.READWRITE
385+
if len(user.perm) > 0 {
386+
val, ok := authpb.Permission_Type_value[strings.ToUpper(user.perm)]
387+
if ok {
388+
permType = authpb.Permission_Type(val)
389+
}
390+
}
382391
perm := &authpb.Permission{
383-
PermType: authpb.READWRITE,
392+
PermType: permType,
384393
Key: []byte(user.key),
385394
RangeEnd: []byte(user.end),
386395
}
@@ -486,6 +495,102 @@ func TestV3AuthNestedTxnPermissionDenied(t *testing.T) {
486495
require.Equal(t, resp.Kvs[0].Value, []byte("bar"))
487496
}
488497

498+
func TestReadWithPrevKvInTXN(t *testing.T) {
499+
integration.BeforeTest(t)
500+
clus := integration.NewCluster(t, &integration.ClusterConfig{Size: 1})
501+
defer clus.Terminate(t)
502+
503+
users := []user{
504+
{
505+
name: "user1",
506+
password: "user1-123",
507+
role: "role1",
508+
perm: "write",
509+
key: "foo",
510+
end: "zoo",
511+
},
512+
}
513+
anonCli := integration.ToGRPC(clus.Client(0))
514+
authSetupUsers(t, anonCli.Auth, users)
515+
authSetupRoot(t, anonCli.Auth)
516+
517+
rootc, err := integration.NewClient(t, clientv3.Config{
518+
Endpoints: clus.Client(0).Endpoints(),
519+
Username: "root",
520+
Password: "123",
521+
})
522+
require.NoError(t, err)
523+
defer rootc.Close()
524+
525+
userc, err := integration.NewClient(t, clientv3.Config{
526+
Endpoints: clus.Client(0).Endpoints(),
527+
Username: "user1",
528+
Password: "user1-123",
529+
})
530+
require.NoError(t, err)
531+
defer userc.Close()
532+
533+
_, err = rootc.Put(t.Context(), "foo", "bar")
534+
require.NoError(t, err)
535+
536+
_, err = userc.Txn(t.Context()).
537+
Then(clientv3.OpPut("foo", "new", clientv3.WithPrevKV())).
538+
Commit()
539+
540+
require.Error(t, err)
541+
require.Truef(t, eqErrGRPC(err, rpctypes.ErrGRPCPermissionDenied), "got %v, expected %v", err, rpctypes.ErrGRPCPermissionDenied)
542+
}
543+
544+
func TestPutWithLeaseInTXN(t *testing.T) {
545+
integration.BeforeTest(t)
546+
clus := integration.NewCluster(t, &integration.ClusterConfig{Size: 1})
547+
defer clus.Terminate(t)
548+
549+
users := []user{
550+
{
551+
name: "user1",
552+
password: "user1-123",
553+
role: "role1",
554+
perm: "write",
555+
key: "foo",
556+
end: "fop",
557+
},
558+
}
559+
anonCli := integration.ToGRPC(clus.Client(0))
560+
authSetupUsers(t, anonCli.Auth, users)
561+
authSetupRoot(t, anonCli.Auth)
562+
563+
rootc, err := integration.NewClient(t, clientv3.Config{
564+
Endpoints: clus.Client(0).Endpoints(),
565+
Username: "root",
566+
Password: "123",
567+
})
568+
require.NoError(t, err)
569+
defer rootc.Close()
570+
571+
userc, err := integration.NewClient(t, clientv3.Config{
572+
Endpoints: clus.Client(0).Endpoints(),
573+
Username: "user1",
574+
Password: "user1-123",
575+
})
576+
require.NoError(t, err)
577+
defer userc.Close()
578+
579+
t.Log("Create a lease and attach it to a key which the user1 doesn't have permission to write")
580+
leaseResp, err := rootc.Grant(t.Context(), 90)
581+
require.NoError(t, err)
582+
leaseID := leaseResp.ID
583+
_, err = rootc.Put(t.Context(), "eoo", "bar", clientv3.WithLease(leaseID))
584+
require.NoError(t, err)
585+
586+
_, err = userc.Txn(t.Context()).
587+
Then(clientv3.OpPut("foo", "new", clientv3.WithLease(leaseID))).
588+
Commit()
589+
590+
require.Error(t, err)
591+
require.Truef(t, eqErrGRPC(err, rpctypes.ErrGRPCPermissionDenied), "got %v, expected %v", err, rpctypes.ErrGRPCPermissionDenied)
592+
}
593+
489594
func TestV3AuthOldRevConcurrent(t *testing.T) {
490595
integration.BeforeTest(t)
491596
clus := integration.NewCluster(t, &integration.ClusterConfig{Size: 1})

0 commit comments

Comments
 (0)