From b2fb75d1470a196fe399ba3649454a053afbea23 Mon Sep 17 00:00:00 2001 From: Thomas Jungblut Date: Fri, 16 Jun 2023 09:14:41 +0200 Subject: [PATCH 1/2] Early exit auth check on lease puts Mitigates etcd-io#15993 by not checking each key individually for permission when auth is entirely disabled or admin user is calling the method. Signed-off-by: Thomas Jungblut --- server/etcdserver/apply_auth.go | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/server/etcdserver/apply_auth.go b/server/etcdserver/apply_auth.go index 74fd2b4fc..bb12923d1 100644 --- a/server/etcdserver/apply_auth.go +++ b/server/etcdserver/apply_auth.go @@ -179,16 +179,27 @@ func (aa *authApplierV3) LeaseRevoke(lc *pb.LeaseRevokeRequest) (*pb.LeaseRevoke func (aa *authApplierV3) checkLeasePuts(leaseID lease.LeaseID) error { lease := aa.lessor.Lookup(leaseID) if lease != nil { - for _, key := range lease.Keys() { - if err := aa.as.IsPutPermitted(&aa.authInfo, []byte(key)); err != nil { - return err - } - } + return aa.checkLeasePutsKeys(lease) } return nil } +func (aa *authApplierV3) checkLeasePutsKeys(l *lease.Lease) error { + // early return for most-common scenario of either disabled auth or admin user. + // IsAdminPermitted also checks whether auth is enabled + if err := aa.as.IsAdminPermitted(&aa.authInfo); err == nil { + return nil + } + + for _, key := range l.Keys() { + if err := aa.as.IsPutPermitted(&aa.authInfo, []byte(key)); err != nil { + return err + } + } + return nil +} + func (aa *authApplierV3) UserGet(r *pb.AuthUserGetRequest) (*pb.AuthUserGetResponse, error) { err := aa.as.IsAdminPermitted(&aa.authInfo) if err != nil && r.Name != aa.authInfo.Username { From 423f9514092472a471a677e7c9ccf461ac33dd16 Mon Sep 17 00:00:00 2001 From: Thomas Jungblut Date: Fri, 16 Jun 2023 09:42:09 +0200 Subject: [PATCH 2/2] Add first unit test for authApplierV3 This contains a slight refactoring to expose enough information to write meaningful tests for auth applier v3. Signed-off-by: Thomas Jungblut --- server/etcdserver/apply_auth.go | 6 +- server/etcdserver/apply_auth_test.go | 122 +++++++++++++++++++++++++++ server/lease/lessor.go | 23 +++-- 3 files changed, 142 insertions(+), 9 deletions(-) create mode 100644 server/etcdserver/apply_auth_test.go diff --git a/server/etcdserver/apply_auth.go b/server/etcdserver/apply_auth.go index bb12923d1..beafa967b 100644 --- a/server/etcdserver/apply_auth.go +++ b/server/etcdserver/apply_auth.go @@ -177,9 +177,9 @@ func (aa *authApplierV3) LeaseRevoke(lc *pb.LeaseRevokeRequest) (*pb.LeaseRevoke } func (aa *authApplierV3) checkLeasePuts(leaseID lease.LeaseID) error { - lease := aa.lessor.Lookup(leaseID) - if lease != nil { - return aa.checkLeasePutsKeys(lease) + l := aa.lessor.Lookup(leaseID) + if l != nil { + return aa.checkLeasePutsKeys(l) } return nil diff --git a/server/etcdserver/apply_auth_test.go b/server/etcdserver/apply_auth_test.go new file mode 100644 index 000000000..8d8c27587 --- /dev/null +++ b/server/etcdserver/apply_auth_test.go @@ -0,0 +1,122 @@ +// Copyright 2023 The etcd Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package etcdserver + +import ( + "encoding/base64" + "testing" + "time" + + "golang.org/x/crypto/bcrypt" + + betesting "go.etcd.io/etcd/server/v3/mvcc/backend/testing" + + "github.com/stretchr/testify/assert" + "go.uber.org/zap/zaptest" + + "go.etcd.io/etcd/api/v3/authpb" + pb "go.etcd.io/etcd/api/v3/etcdserverpb" + "go.etcd.io/etcd/server/v3/auth" + "go.etcd.io/etcd/server/v3/lease" +) + +func TestCheckLeasePutsKeys(t *testing.T) { + lg := zaptest.NewLogger(t) + + b, _ := betesting.NewDefaultTmpBackend(t) + defer betesting.Close(t, b) + + simpleTokenTTLDefault := 300 * time.Second + tokenTypeSimple := "simple" + dummyIndexWaiter := func(index uint64) <-chan struct{} { + ch := make(chan struct{}, 1) + go func() { + ch <- struct{}{} + }() + return ch + } + + tp, _ := auth.NewTokenProvider(zaptest.NewLogger(t), tokenTypeSimple, dummyIndexWaiter, simpleTokenTTLDefault) + as := auth.NewAuthStore(lg, b, tp, bcrypt.MinCost) + + aa := authApplierV3{as: as} + assert.NoError(t, aa.checkLeasePutsKeys(lease.NewLease(lease.LeaseID(1), 3600)), "auth is disabled, should allow puts") + assert.NoError(t, enableAuthAndCreateRoot(aa.as), "error while enabling auth") + aa.authInfo = auth.AuthInfo{Username: "root"} + assert.NoError(t, aa.checkLeasePutsKeys(lease.NewLease(lease.LeaseID(1), 3600)), "auth is enabled, should allow puts for root") + + l := lease.NewLease(lease.LeaseID(1), 3600) + l.SetLeaseItem(lease.LeaseItem{Key: "a"}) + aa.authInfo = auth.AuthInfo{Username: "bob", Revision: 0} + assert.ErrorIs(t, aa.checkLeasePutsKeys(l), auth.ErrUserEmpty, "auth is enabled, should not allow bob, non existing at rev 0") + aa.authInfo = auth.AuthInfo{Username: "bob", Revision: 1} + assert.ErrorIs(t, aa.checkLeasePutsKeys(l), auth.ErrAuthOldRevision, "auth is enabled, old revision") + + aa.authInfo = auth.AuthInfo{Username: "bob", Revision: aa.as.Revision()} + assert.ErrorIs(t, aa.checkLeasePutsKeys(l), auth.ErrPermissionDenied, "auth is enabled, bob does not have permissions, bob does not exist") + _, err := aa.as.UserAdd(&pb.AuthUserAddRequest{Name: "bob", Options: &authpb.UserAddOptions{NoPassword: true}}) + assert.NoError(t, err, "bob should be added without error") + aa.authInfo = auth.AuthInfo{Username: "bob", Revision: aa.as.Revision()} + assert.ErrorIs(t, aa.checkLeasePutsKeys(l), auth.ErrPermissionDenied, "auth is enabled, bob exists yet does not have permissions") + + // allow bob to access "a" + _, err = aa.as.RoleAdd(&pb.AuthRoleAddRequest{Name: "bobsrole"}) + assert.NoError(t, err, "bobsrole should be added without error") + _, err = aa.as.RoleGrantPermission(&pb.AuthRoleGrantPermissionRequest{ + Name: "bobsrole", + Perm: &authpb.Permission{ + PermType: authpb.READWRITE, + Key: []byte("a"), + RangeEnd: nil, + }, + }) + assert.NoError(t, err, "bobsrole should be granted permissions without error") + _, err = aa.as.UserGrantRole(&pb.AuthUserGrantRoleRequest{ + User: "bob", + Role: "bobsrole", + }) + assert.NoError(t, err, "bob should be granted bobsrole without error") + + aa.authInfo = auth.AuthInfo{Username: "bob", Revision: aa.as.Revision()} + assert.NoError(t, aa.checkLeasePutsKeys(l), "bob should be able to access key 'a'") + +} + +func enableAuthAndCreateRoot(as auth.AuthStore) error { + _, err := as.UserAdd(&pb.AuthUserAddRequest{ + Name: "root", + HashedPassword: encodePassword("root"), + Options: &authpb.UserAddOptions{NoPassword: false}}) + if err != nil { + return err + } + + _, err = as.RoleAdd(&pb.AuthRoleAddRequest{Name: "root"}) + if err != nil { + return err + } + + _, err = as.UserGrantRole(&pb.AuthUserGrantRoleRequest{User: "root", Role: "root"}) + if err != nil { + return err + } + + return as.AuthEnable() +} + +func encodePassword(s string) string { + hashedPassword, _ := bcrypt.GenerateFromPassword([]byte(s), bcrypt.MinCost) + return base64.StdEncoding.EncodeToString(hashedPassword) +} diff --git a/server/lease/lessor.go b/server/lease/lessor.go index 4c6bf3c3c..ff9cb2ca5 100644 --- a/server/lease/lessor.go +++ b/server/lease/lessor.go @@ -281,12 +281,7 @@ func (le *lessor) Grant(id LeaseID, ttl int64) (*Lease, error) { // TODO: when lessor is under high load, it should give out lease // with longer TTL to reduce renew load. - l := &Lease{ - ID: id, - ttl: ttl, - itemSet: make(map[LeaseItem]struct{}), - revokec: make(chan struct{}), - } + l := NewLease(id, ttl) le.mu.Lock() defer le.mu.Unlock() @@ -838,6 +833,15 @@ type Lease struct { revokec chan struct{} } +func NewLease(id LeaseID, ttl int64) *Lease { + return &Lease{ + ID: id, + ttl: ttl, + itemSet: make(map[LeaseItem]struct{}), + revokec: make(chan struct{}), + } +} + func (l *Lease) expired() bool { return l.Remaining() <= 0 } @@ -861,6 +865,13 @@ func (l *Lease) TTL() int64 { return l.ttl } +// SetLeaseItem sets the given lease item, this func is thread-safe +func (l *Lease) SetLeaseItem(item LeaseItem) { + l.mu.Lock() + defer l.mu.Unlock() + l.itemSet[item] = struct{}{} +} + // RemainingTTL returns the last checkpointed remaining TTL of the lease. // TODO(jpbetz): do not expose this utility method func (l *Lease) RemainingTTL() int64 {