Merge pull request #9399 from yudai/maxleasettl

*: enforce max lease TTL with 9,000,000,000 seconds
etcd-io · Mar 8, 2018 · 6fd4138 · 6fd4138
2 parents 4704232 + 83a9684
commit 6fd4138
Show file tree

Hide file tree

Showing 9 changed files with 60 additions and 11 deletions.
diff --git a/CHANGELOG-3.2.md b/CHANGELOG-3.2.md
@@ -13,6 +13,11 @@ See [code changes](https://github.com/coreos/etcd/compare/v3.2.16...v3.2.17) and
 - Fix [server panic on invalid Election Proclaim/Resign HTTP(S) requests](https://github.com/coreos/etcd/pull/9379).
   - Previously, wrong-formatted HTTP requests to Election API could trigger panic in etcd server.
   - e.g. `curl -L http://localhost:2379/v3/election/proclaim -X POST -d '{"value":""}'`, `curl -L http://localhost:2379/v3/election/resign -X POST -d '{"value":""}'`.
+- Prevent [overflow by large `TTL` values for `Lease` `Grant`](https://github.com/coreos/etcd/pull/9399).
+  - `TTL` parameter to `Grant` request is unit of second.
+  - Leases with too large `TTL` values exceeding `math.MaxInt64` [expire in unexpected ways](https://github.com/coreos/etcd/issues/9374).
+  - Server now returns `rpctypes.ErrLeaseTTLTooLarge` to client, when the requested `TTL` is larger than 9,000,000,000 (which is >285 years).
+  - Again, etcd `Lease` is meant for short-periodic keepalives or sessions, in the range of seconds or minutes. Not for hours or days!
 - Enable etcd server [`raft.Config.CheckQuorum` when starting with `ForceNewCluster`](https://github.com/coreos/etcd/pull/9347).
 
 ### Security

diff --git a/CHANGELOG-3.3.md b/CHANGELOG-3.3.md
@@ -16,6 +16,11 @@ See [code changes](https://github.com/coreos/etcd/compare/v3.3.1...v3.3.2) and [
 - Fix [revision-based compaction retention parsing](https://github.com/coreos/etcd/pull/9339).
   - Previously, `--auto-compaction-mode revision --auto-compaction-retention 1` was [translated to revision retention 3600000000000](https://github.com/coreos/etcd/issues/9337).
   - Now, `--auto-compaction-mode revision --auto-compaction-retention 1` is correctly parsed as revision retention 1.
+- Prevent [overflow by large `TTL` values for `Lease` `Grant`](https://github.com/coreos/etcd/pull/9399).
+  - `TTL` parameter to `Grant` request is unit of second.
+  - Leases with too large `TTL` values exceeding `math.MaxInt64` [expire in unexpected ways](https://github.com/coreos/etcd/issues/9374).
+  - Server now returns `rpctypes.ErrLeaseTTLTooLarge` to client, when the requested `TTL` is larger than 9,000,000,000 (which is >285 years).
+  - Again, etcd `Lease` is meant for short-periodic keepalives or sessions, in the range of seconds or minutes. Not for hours or days!
 - Enable etcd server [`raft.Config.CheckQuorum` when starting with `ForceNewCluster`](https://github.com/coreos/etcd/pull/9347).
 
 

diff --git a/CHANGELOG-3.4.md b/CHANGELOG-3.4.md
@@ -123,4 +123,9 @@ See [code changes](https://github.com/coreos/etcd/compare/v3.3.0...v3.4.0) and [
 - Fix [revision-based compaction retention parsing](https://github.com/coreos/etcd/pull/9339).
   - Previously, `etcd --auto-compaction-mode revision --auto-compaction-retention 1` was [translated to revision retention 3600000000000](https://github.com/coreos/etcd/issues/9337).
   - Now, `etcd --auto-compaction-mode revision --auto-compaction-retention 1` is correctly parsed as revision retention 1.
+- Prevent [overflow by large `TTL` values for `Lease` `Grant`](https://github.com/coreos/etcd/pull/9399).
+  - `TTL` parameter to `Grant` request is unit of second.
+  - Leases with too large `TTL` values exceeding `math.MaxInt64` [expire in unexpected ways](https://github.com/coreos/etcd/issues/9374).
+  - Server now returns `rpctypes.ErrLeaseTTLTooLarge` to client, when the requested `TTL` is larger than 9,000,000,000 (which is >285 years).
+  - Again, etcd `Lease` is meant for short-periodic keepalives or sessions, in the range of seconds or minutes. Not for hours or days!
 - Enable etcd server [`raft.Config.CheckQuorum` when starting with `ForceNewCluster`](https://github.com/coreos/etcd/pull/9347).
diff --git a/clientv3/integration/lease_test.go b/clientv3/integration/lease_test.go
@@ -55,6 +55,11 @@ func TestLeaseGrant(t *testing.T) {
 
 	kv := clus.RandClient()
 
+	_, merr := lapi.Grant(context.Background(), clientv3.MaxLeaseTTL+1)
+	if merr != rpctypes.ErrLeaseTTLTooLarge {
+		t.Fatalf("err = %v, want %v", merr, rpctypes.ErrLeaseTTLTooLarge)
+	}
+
 	resp, err := lapi.Grant(context.Background(), 10)
 	if err != nil {
 		t.Errorf("failed to create lease %v", err)

diff --git a/clientv3/grpc_options.go → clientv3/options.go b/clientv3/grpc_options.go → clientv3/options.go
@@ -44,3 +44,6 @@ var (
 // Some options are exposed to "clientv3.Config".
 // Defaults will be overridden by the settings in "clientv3.Config".
 var defaultCallOpts = []grpc.CallOption{defaultFailFast, defaultMaxCallSendMsgSize, defaultMaxCallRecvMsgSize}
+
+// MaxLeaseTTL is the maximum lease TTL value
+const MaxLeaseTTL = 9000000000
diff --git a/etcdserver/api/v3rpc/rpctypes/error.go b/etcdserver/api/v3rpc/rpctypes/error.go
@@ -31,8 +31,9 @@ var (
 	ErrGRPCFutureRev     = status.New(codes.OutOfRange, "etcdserver: mvcc: required revision is a future revision").Err()
 	ErrGRPCNoSpace       = status.New(codes.ResourceExhausted, "etcdserver: mvcc: database space exceeded").Err()
 
-	ErrGRPCLeaseNotFound = status.New(codes.NotFound, "etcdserver: requested lease not found").Err()
-	ErrGRPCLeaseExist    = status.New(codes.FailedPrecondition, "etcdserver: lease already exists").Err()
+	ErrGRPCLeaseNotFound    = status.New(codes.NotFound, "etcdserver: requested lease not found").Err()
+	ErrGRPCLeaseExist       = status.New(codes.FailedPrecondition, "etcdserver: lease already exists").Err()
+	ErrGRPCLeaseTTLTooLarge = status.New(codes.OutOfRange, "etcdserver: too large lease TTL").Err()
 
 	ErrGRPCMemberExist            = status.New(codes.FailedPrecondition, "etcdserver: member ID already exist").Err()
 	ErrGRPCPeerURLExist           = status.New(codes.FailedPrecondition, "etcdserver: Peer URLs already exists").Err()
@@ -80,8 +81,9 @@ var (
 		ErrorDesc(ErrGRPCFutureRev):    ErrGRPCFutureRev,
 		ErrorDesc(ErrGRPCNoSpace):      ErrGRPCNoSpace,
 
-		ErrorDesc(ErrGRPCLeaseNotFound): ErrGRPCLeaseNotFound,
-		ErrorDesc(ErrGRPCLeaseExist):    ErrGRPCLeaseExist,
+		ErrorDesc(ErrGRPCLeaseNotFound):    ErrGRPCLeaseNotFound,
+		ErrorDesc(ErrGRPCLeaseExist):       ErrGRPCLeaseExist,
+		ErrorDesc(ErrGRPCLeaseTTLTooLarge): ErrGRPCLeaseTTLTooLarge,
 
 		ErrorDesc(ErrGRPCMemberExist):            ErrGRPCMemberExist,
 		ErrorDesc(ErrGRPCPeerURLExist):           ErrGRPCPeerURLExist,
@@ -131,8 +133,9 @@ var (
 	ErrFutureRev     = Error(ErrGRPCFutureRev)
 	ErrNoSpace       = Error(ErrGRPCNoSpace)
 
-	ErrLeaseNotFound = Error(ErrGRPCLeaseNotFound)
-	ErrLeaseExist    = Error(ErrGRPCLeaseExist)
+	ErrLeaseNotFound    = Error(ErrGRPCLeaseNotFound)
+	ErrLeaseExist       = Error(ErrGRPCLeaseExist)
+	ErrLeaseTTLTooLarge = Error(ErrGRPCLeaseTTLTooLarge)
 
 	ErrMemberExist            = Error(ErrGRPCMemberExist)
 	ErrPeerURLExist           = Error(ErrGRPCPeerURLExist)

diff --git a/etcdserver/api/v3rpc/util.go b/etcdserver/api/v3rpc/util.go
@@ -52,8 +52,9 @@ var toGRPCErrorMap = map[error]error{
 	etcdserver.ErrKeyNotFound:                rpctypes.ErrGRPCKeyNotFound,
 	etcdserver.ErrCorrupt:                    rpctypes.ErrGRPCCorrupt,
 
-	lease.ErrLeaseNotFound: rpctypes.ErrGRPCLeaseNotFound,
-	lease.ErrLeaseExists:   rpctypes.ErrGRPCLeaseExist,
+	lease.ErrLeaseNotFound:    rpctypes.ErrGRPCLeaseNotFound,
+	lease.ErrLeaseExists:      rpctypes.ErrGRPCLeaseExist,
+	lease.ErrLeaseTTLTooLarge: rpctypes.ErrGRPCLeaseTTLTooLarge,
 
 	auth.ErrRootUserNotExist:     rpctypes.ErrGRPCRootUserNotExist,
 	auth.ErrRootRoleNotExist:     rpctypes.ErrGRPCRootRoleNotExist,

diff --git a/lease/lessor.go b/lease/lessor.go
@@ -29,6 +29,9 @@ import (
 // NoLease is a special LeaseID representing the absence of a lease.
 const NoLease = LeaseID(0)
 
+// MaxLeaseTTL is the maximum lease TTL value
+const MaxLeaseTTL = 9000000000
+
 var (
 	forever = time.Time{}
 
@@ -37,9 +40,10 @@ var (
 	// maximum number of leases to revoke per second; configurable for tests
 	leaseRevokeRate = 1000
 
-	ErrNotPrimary    = errors.New("not a primary lessor")
-	ErrLeaseNotFound = errors.New("lease not found")
-	ErrLeaseExists   = errors.New("lease already exists")
+	ErrNotPrimary       = errors.New("not a primary lessor")
+	ErrLeaseNotFound    = errors.New("lease not found")
+	ErrLeaseExists      = errors.New("lease already exists")
+	ErrLeaseTTLTooLarge = errors.New("too large lease TTL")
 )
 
 // TxnDelete is a TxnWrite that only permits deletes. Defined here
@@ -198,6 +202,10 @@ func (le *lessor) Grant(id LeaseID, ttl int64) (*Lease, error) {
 		return nil, ErrLeaseNotFound
 	}
 
+	if ttl > MaxLeaseTTL {
+		return nil, ErrLeaseTTLTooLarge
+	}
+
 	// TODO: when lessor is under high load, it should give out lease
 	// with longer TTL to reduce renew load.
 	l := &Lease{

diff --git a/lease/lessor_test.go b/lease/lessor_test.go
@@ -451,6 +451,20 @@ func TestLessorExpireAndDemote(t *testing.T) {
 	}
 }
 
+func TestLessorMaxTTL(t *testing.T) {
+	dir, be := NewTestBackend(t)
+	defer os.RemoveAll(dir)
+	defer be.Close()
+
+	le := newLessor(be, minLeaseTTL)
+	defer le.Stop()
+
+	_, err := le.Grant(1, MaxLeaseTTL+1)
+	if err != ErrLeaseTTLTooLarge {
+		t.Fatalf("grant unexpectedly succeeded")
+	}
+}
+
 type fakeDeleter struct {
 	deleted []string
 	tx      backend.BatchTx