Don't call LeaseExtend on login renewal paths when period is provided (…

…#3803)

* Don't call LeaseExtend on login renewal paths when period is provided

* WIP tests

* NoopBackend accept backend ttl values

* Test period value on credentials backend

* Use t.Fatalf instead

* Remove mockCoreExpiration

* Add login renewal test for approle backend

* Add resp.Auth.Period check on aws and cert backend tests

* Pass in approle's period via role's period

* Correctly set period in valid-role's role

* Add period renewal test using TestCluster and approle backend

* Check for ttl values after renewals on test

builtin/credential/approle/path_login.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"fmt"
 	"strings"
+	"time"
 
 	"github.com/hashicorp/vault/logical"
 	"github.com/hashicorp/vault/logical/framework"
@@ -100,12 +101,17 @@ func (b *backend) pathLoginRenew(ctx context.Context, req *logical.Request, data
 		return nil, fmt.Errorf("role %s does not exist during renewal", roleName)
 	}
 
-	resp, err := framework.LeaseExtend(role.TokenTTL, role.TokenMaxTTL, b.System())(ctx, req, data)
-	if err != nil {
-		return nil, err
+	// If a period is provided, set that as part of resp.Auth.Period and return a
+	// response immediately. Let expiration manager handle renewal from there on.
+	if role.Period > time.Duration(0) {
+		resp := &logical.Response{
+			Auth: req.Auth,
+		}
+		resp.Auth.Period = role.Period
+		return resp, nil
 	}
-	resp.Auth.Period = role.Period
-	return resp, nil
+
+	return framework.LeaseExtend(role.TokenTTL, role.TokenMaxTTL, b.System())(ctx, req, data)
 }
 
 const pathLoginHelpSys = "Issue a token based on the credentials supplied"

builtin/credential/approle/path_login_test.go

@@ -3,6 +3,7 @@ package approle
 import (
 	"context"
 	"testing"
+	"time"
 
 	"github.com/hashicorp/vault/logical"
 )
@@ -48,12 +49,106 @@ func TestAppRole_RoleLogin(t *testing.T) {
 			RemoteAddr: "127.0.0.1",
 		},
 	}
-	resp, err = b.HandleRequest(context.Background(), loginReq)
+	loginResp, err := b.HandleRequest(context.Background(), loginReq)
+	if err != nil || (loginResp != nil && loginResp.IsError()) {
+		t.Fatalf("err:%v resp:%#v", err, loginResp)
+	}
+
+	if loginResp.Auth == nil {
+		t.Fatalf("expected a non-nil auth object in the response")
+	}
+
+	// Test renewal
+	renewReq := generateRenewRequest(storage, loginResp.Auth)
+
+	resp, err = b.HandleRequest(context.Background(), renewReq)
+	if err != nil || (resp != nil && resp.IsError()) {
+		t.Fatalf("err:%v resp:%#v", err, resp)
+	}
+
+	if resp.Auth.TTL != 400*time.Second {
+		t.Fatalf("expected period value from response to be 400s, got: %s", resp.Auth.TTL)
+	}
+
+	///
+	// Test renewal with period
+	///
+
+	// Create role
+	period := 600 * time.Second
+	roleData := map[string]interface{}{
+		"policies": "a,b,c",
+		"period":   period.String(),
+	}
+	roleReq := &logical.Request{
+		Operation: logical.CreateOperation,
+		Path:      "role/" + "role-period",
+		Storage:   storage,
+		Data:      roleData,
+	}
+	resp, err = b.HandleRequest(context.Background(), roleReq)
+	if err != nil || (resp != nil && resp.IsError()) {
+		t.Fatalf("err:%v resp:%#v", err, resp)
+	}
+
+	roleRoleIDReq = &logical.Request{
+		Operation: logical.ReadOperation,
+		Path:      "role/role-period/role-id",
+		Storage:   storage,
+	}
+	resp, err = b.HandleRequest(context.Background(), roleRoleIDReq)
+	if err != nil || (resp != nil && resp.IsError()) {
+		t.Fatalf("err:%v resp:%#v", err, resp)
+	}
+	roleID = resp.Data["role_id"]
+
+	roleSecretIDReq = &logical.Request{
+		Operation: logical.UpdateOperation,
+		Path:      "role/role-period/secret-id",
+		Storage:   storage,
+	}
+	resp, err = b.HandleRequest(context.Background(), roleSecretIDReq)
 	if err != nil || (resp != nil && resp.IsError()) {
 		t.Fatalf("err:%v resp:%#v", err, resp)
 	}
+	secretID = resp.Data["secret_id"]
+
+	loginData["role_id"] = roleID
+	loginData["secret_id"] = secretID
+
+	loginResp, err = b.HandleRequest(context.Background(), loginReq)
+	if err != nil || (loginResp != nil && loginResp.IsError()) {
+		t.Fatalf("err:%v resp:%#v", err, loginResp)
+	}
 
-	if resp.Auth == nil {
+	if loginResp.Auth == nil {
 		t.Fatalf("expected a non-nil auth object in the response")
 	}
+
+	renewReq = generateRenewRequest(storage, loginResp.Auth)
+
+	resp, err = b.HandleRequest(context.Background(), renewReq)
+	if err != nil || (resp != nil && resp.IsError()) {
+		t.Fatalf("err:%v resp:%#v", err, resp)
+	}
+
+	if resp.Auth.Period != period {
+		t.Fatalf("expected period value of %d in the response, got: %s", period, resp.Auth.Period)
+	}
+}
+
+func generateRenewRequest(s logical.Storage, auth *logical.Auth) *logical.Request {
+	renewReq := &logical.Request{
+		Operation: logical.RenewOperation,
+		Storage:   s,
+		Auth:      &logical.Auth{},
+	}
+	renewReq.Auth.InternalData = auth.InternalData
+	renewReq.Auth.Metadata = auth.Metadata
+	renewReq.Auth.LeaseOptions = auth.LeaseOptions
+	renewReq.Auth.Policies = auth.Policies
+	renewReq.Auth.IssueTime = time.Now()
+	renewReq.Auth.Period = auth.Period
+
+	return renewReq
 }

builtin/credential/aws/backend_test.go

@@ -1615,6 +1615,40 @@ func TestBackendAcc_LoginWithCallerIdentity(t *testing.T) {
 	if cachedArn == "" {
 		t.Errorf("got empty ARN back from user ID cache; expected full arn")
 	}
+
+	// Test for renewal with period
+	period := 600 * time.Second
+	roleData["period"] = period.String()
+	roleRequest.Path = "role/" + testValidRoleName
+	resp, err = b.HandleRequest(context.Background(), roleRequest)
+	if err != nil || (resp != nil && resp.IsError()) {
+		t.Fatalf("bad: failed to create wildcard role: resp:%#v\nerr:%v", resp, err)
+	}
+
+	loginData["role"] = testValidRoleName
+	resp, err = b.HandleRequest(context.Background(), loginRequest)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if resp == nil || resp.Auth == nil || resp.IsError() {
+		t.Fatalf("bad: expected valid login: resp:%#v", resp)
+	}
+
+	renewReq = generateRenewRequest(storage, resp.Auth)
+	resp, err = b.pathLoginRenew(context.Background(), renewReq, empty_login_fd)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if resp == nil {
+		t.Fatal("got nil response from renew")
+	}
+	if resp.IsError() {
+		t.Fatalf("got error when renewing: %#v", *resp)
+	}
+
+	if resp.Auth.Period != period {
+		t.Fatalf("expected a period value of %s in the response, got: %s", period, resp.Auth.Period)
+	}
 }
 
 func generateRenewRequest(s logical.Storage, auth *logical.Auth) *logical.Request {
@@ -1627,6 +1661,7 @@ func generateRenewRequest(s logical.Storage, auth *logical.Auth) *logical.Reques
 	renewReq.Auth.LeaseOptions = auth.LeaseOptions
 	renewReq.Auth.Policies = auth.Policies
 	renewReq.Auth.IssueTime = time.Now()
+	renewReq.Auth.Period = auth.Period
 
 	return renewReq
 }

builtin/credential/aws/path_login.go

@@ -975,12 +975,17 @@ func (b *backend) pathLoginRenewIam(ctx context.Context, req *logical.Request, d
 		}
 	}
 
-	resp, err := framework.LeaseExtend(roleEntry.TTL, roleEntry.MaxTTL, b.System())(ctx, req, data)
-	if err != nil {
-		return nil, err
+	// If a period is provided, set that as part of resp.Auth.Period and return a
+	// response immediately. Let expiration manager handle renewal from there on.
+	if roleEntry.Period > time.Duration(0) {
+		resp := &logical.Response{
+			Auth: req.Auth,
+		}
+		resp.Auth.Period = roleEntry.Period
+		return resp, nil
 	}
-	resp.Auth.Period = roleEntry.Period
-	return resp, nil
+
+	return framework.LeaseExtend(roleEntry.TTL, roleEntry.MaxTTL, b.System())(ctx, req, data)
 }
 
 func (b *backend) pathLoginRenewEc2(ctx context.Context, req *logical.Request, data *framework.FieldData) (*logical.Response, error) {
@@ -1060,12 +1065,17 @@ func (b *backend) pathLoginRenewEc2(ctx context.Context, req *logical.Request, d
 		return nil, err
 	}
 
-	resp, err := framework.LeaseExtend(roleEntry.TTL, shortestMaxTTL, b.System())(ctx, req, data)
-	if err != nil {
-		return nil, err
+	// If a period is provided, set that as part of resp.Auth.Period and return a
+	// response immediately. Let expiration manager handle renewal from there on.
+	if roleEntry.Period > time.Duration(0) {
+		resp := &logical.Response{
+			Auth: req.Auth,
+		}
+		resp.Auth.Period = roleEntry.Period
+		return resp, nil
 	}
-	resp.Auth.Period = roleEntry.Period
-	return resp, nil
+
+	return framework.LeaseExtend(roleEntry.TTL, shortestMaxTTL, b.System())(ctx, req, data)
 }
 
 func (b *backend) pathLoginUpdateIam(ctx context.Context, req *logical.Request, data *framework.FieldData) (*logical.Response, error) {

builtin/credential/cert/backend_test.go

@@ -1195,6 +1195,7 @@ func Test_Renew(t *testing.T) {
 	req.Auth.LeaseOptions = resp.Auth.LeaseOptions
 	req.Auth.Policies = resp.Auth.Policies
 	req.Auth.IssueTime = time.Now()
+	req.Auth.Period = resp.Auth.Period
 
 	// Normal renewal
 	resp, err = b.pathLoginRenew(context.Background(), req, empty_login_fd)
@@ -1238,6 +1239,29 @@ func Test_Renew(t *testing.T) {
 		t.Fatalf("got error: %#v", *resp)
 	}
 
+	// Add period value to cert entry
+	period := 350 * time.Second
+	fd.Raw["period"] = period.String()
+	resp, err = b.pathCertWrite(context.Background(), req, fd)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	resp, err = b.pathLoginRenew(context.Background(), req, empty_login_fd)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if resp == nil {
+		t.Fatal("got nil response from renew")
+	}
+	if resp.IsError() {
+		t.Fatalf("got error: %#v", *resp)
+	}
+
+	if resp.Auth.Period != period {
+		t.Fatalf("expected a period value of %s in the response, got: %s", period, resp.Auth.Period)
+	}
+
 	// Delete CA, make sure we can't renew
 	resp, err = b.pathCertDelete(context.Background(), req, fd)
 	if err != nil {

builtin/credential/cert/path_login.go

@@ -175,12 +175,17 @@ func (b *backend) pathLoginRenew(ctx context.Context, req *logical.Request, d *f
 		return nil, fmt.Errorf("policies have changed, not renewing")
 	}
 
-	resp, err := framework.LeaseExtend(cert.TTL, cert.MaxTTL, b.System())(ctx, req, d)
-	if err != nil {
-		return nil, err
+	// If a period is provided, set that as part of resp.Auth.Period and return a
+	// response immediately. Let expiration manager handle renewal from there on.
+	if cert.Period > time.Duration(0) {
+		resp := &logical.Response{
+			Auth: req.Auth,
+		}
+		resp.Auth.Period = cert.Period
+		return resp, nil
 	}
-	resp.Auth.Period = cert.Period
-	return resp, nil
+
+	return framework.LeaseExtend(cert.TTL, cert.MaxTTL, b.System())(ctx, req, d)
 }
 
 func (b *backend) verifyCredentials(req *logical.Request, d *framework.FieldData) (*ParsedCert, *logical.Response, error) {