Merge pull request etcd-io#16020 from tjungblu/putauthshort_3.4

[3.4] Early exit auth check on lease puts

etcdserver/apply_auth.go

@@ -176,18 +176,29 @@ 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
-			}
-		}
+	l := aa.lessor.Lookup(leaseID)
+	if l != nil {
+		return aa.checkLeasePutsKeys(l)
 	}
 
 	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 {

etcdserver/apply_auth_test.go

@@ -0,0 +1,115 @@
+// 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 (
+	"testing"
+	"time"
+
+	"go.etcd.io/etcd/auth"
+	"go.etcd.io/etcd/auth/authpb"
+	pb "go.etcd.io/etcd/etcdserver/etcdserverpb"
+	"go.etcd.io/etcd/lease"
+	"golang.org/x/crypto/bcrypt"
+
+	betesting "go.etcd.io/etcd/mvcc/backend"
+
+	"github.com/stretchr/testify/assert"
+	"go.uber.org/zap/zaptest"
+)
+
+func TestCheckLeasePutsKeys(t *testing.T) {
+	lg := zaptest.NewLogger(t)
+
+	b, _ := betesting.NewDefaultTmpBackend()
+	defer b.Close()
+
+	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",
+		Password: "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()
+}

lease/lessor.go

@@ -282,12 +282,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()
@@ -840,6 +835,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
 }
@@ -863,6 +867,12 @@ func (l *Lease) TTL() int64 {
 	return l.ttl
 }
 
+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 {