From 0691a83f3fb809616b140043eecd97630878dfa6 Mon Sep 17 00:00:00 2001 From: Vinay Gopalan Date: Thu, 8 Jul 2021 14:53:03 -0700 Subject: [PATCH 1/6] fix: cap token TTL at login time based on default lease TTL --- builtin/credential/aws/path_role.go | 2 + builtin/credential/aws/path_role_test.go | 101 ++++++++++++++++++++++- 2 files changed, 101 insertions(+), 2 deletions(-) diff --git a/builtin/credential/aws/path_role.go b/builtin/credential/aws/path_role.go index 1d248a393b142..6b6e4cedc4ea8 100644 --- a/builtin/credential/aws/path_role.go +++ b/builtin/credential/aws/path_role.go @@ -892,9 +892,11 @@ func (b *backend) pathRoleCreateUpdate(ctx context.Context, req *logical.Request defaultLeaseTTL := b.System().DefaultLeaseTTL() systemMaxTTL := b.System().MaxLeaseTTL() if roleEntry.TokenTTL > defaultLeaseTTL { + roleEntry.TokenTTL = defaultLeaseTTL resp.AddWarning(fmt.Sprintf("Given ttl of %d seconds greater than current mount/system default of %d seconds; ttl will be capped at login time", roleEntry.TokenTTL/time.Second, defaultLeaseTTL/time.Second)) } if roleEntry.TokenMaxTTL > systemMaxTTL { + roleEntry.TokenMaxTTL = systemMaxTTL resp.AddWarning(fmt.Sprintf("Given max ttl of %d seconds greater than current mount/system default of %d seconds; max ttl will be capped at login time", roleEntry.TokenMaxTTL/time.Second, systemMaxTTL/time.Second)) } if roleEntry.TokenMaxTTL != 0 && roleEntry.TokenMaxTTL < roleEntry.TokenTTL { diff --git a/builtin/credential/aws/path_role_test.go b/builtin/credential/aws/path_role_test.go index a46a28a8a8673..931d78d65603a 100644 --- a/builtin/credential/aws/path_role_test.go +++ b/builtin/credential/aws/path_role_test.go @@ -6,6 +6,7 @@ import ( "reflect" "strings" "testing" + "time" "github.com/go-test/deep" "github.com/hashicorp/go-hclog" @@ -762,16 +763,112 @@ func TestAwsEc2_RoleDurationSeconds(t *testing.T) { } if resp.Data["ttl"].(int64) != 10 { - t.Fatalf("bad: period; expected: 10, actual: %d", resp.Data["ttl"]) + t.Fatalf("bad: ttl; expected: 10, actual: %d", resp.Data["ttl"]) } if resp.Data["max_ttl"].(int64) != 20 { - t.Fatalf("bad: period; expected: 20, actual: %d", resp.Data["max_ttl"]) + t.Fatalf("bad: max_ttl; expected: 20, actual: %d", resp.Data["max_ttl"]) } if resp.Data["period"].(int64) != 30 { t.Fatalf("bad: period; expected: 30, actual: %d", resp.Data["period"]) } } +func TestAwsIam_RoleDurationSeconds(t *testing.T) { + config := logical.TestBackendConfig() + storage := &logical.InmemStorage{} + config.StorageView = storage + + b, err := Backend(config) + if err != nil { + t.Fatal(err) + } + + err = b.Setup(context.Background(), config) + if err != nil { + t.Fatal(err) + } + + roleData := map[string]interface{}{ + "auth_type": "iam", + "bound_iam_principal_arn": "arn:aws:iam::123456789012:role/testrole", + "resolve_aws_unique_ids": false, + "ttl": "20m", + "max_ttl": "30m", + } + + roleReq := &logical.Request{ + Operation: logical.CreateOperation, + Storage: storage, + Path: "role/testrole", + Data: roleData, + } + + resp, err := b.HandleRequest(context.Background(), roleReq) + if err != nil || (resp != nil && resp.IsError()) { + t.Fatalf("resp: %#v, err: %v", resp, err) + } + + roleReq.Operation = logical.ReadOperation + + resp, err = b.HandleRequest(context.Background(), roleReq) + if err != nil || (resp != nil && resp.IsError()) { + t.Fatalf("resp: %#v, err: %v", resp, err) + } + + // since default lease TTL for system is 24hr, Token TTL should not be capped + // since max lease TTL for system is 48hr, Token Max TTL should not be capped + if resp.Data["token_ttl"].(int64) != 1200 { + t.Fatalf("bad: token_ttl; expected: 1200, actual: %d", resp.Data["ttl"]) + } + if resp.Data["max_ttl"].(int64) != 1800 { + t.Fatalf("bad: max_ttl; expected: 1800, actual: %d", resp.Data["max_ttl"]) + } + + // set default lease TTL to 10m; Token TTL should be capped at 10m + // set max lease TTL to 20m; Token Max TTL should be capped at 20m + config = &logical.BackendConfig{ + Logger: logging.NewVaultLogger(hclog.Trace), + + System: &logical.StaticSystemView{ + DefaultLeaseTTLVal: time.Minute * 10, + MaxLeaseTTLVal: time.Minute * 20, + }, + StorageView: &logical.InmemStorage{}, + } + + b, err = Backend(config) + if err != nil { + t.Fatal(err) + } + + err = b.Setup(context.Background(), config) + if err != nil { + t.Fatal(err) + } + + roleReq.Operation = logical.CreateOperation + + resp, err = b.HandleRequest(context.Background(), roleReq) + if err != nil || (resp != nil && resp.IsError()) { + t.Fatalf("resp: %#v, err: %v", resp, err) + } + + roleReq.Operation = logical.ReadOperation + + resp, err = b.HandleRequest(context.Background(), roleReq) + if err != nil || (resp != nil && resp.IsError()) { + t.Fatalf("resp: %#v, err: %v", resp, err) + } + + if resp.Data["token_ttl"].(int64) != 600 { + t.Fatalf("bad: token_ttl; expected: 600, actual: %d", resp.Data["ttl"]) + } + + if resp.Data["token_max_ttl"].(int64) != 1200 { + t.Fatalf("bad: token_max_ttl; expected: 1200, actual: %d", resp.Data["ttl"]) + } +} + func TestRoleEntryUpgradeV(t *testing.T) { config := logical.TestBackendConfig() storage := &logical.InmemStorage{} From 72c8f66ac7f89c961b62c335b92b8352019ac9a3 Mon Sep 17 00:00:00 2001 From: Vinay Gopalan Date: Thu, 8 Jul 2021 15:40:37 -0700 Subject: [PATCH 2/6] add changelog file --- changelog/12026.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 changelog/12026.txt diff --git a/changelog/12026.txt b/changelog/12026.txt new file mode 100644 index 0000000000000..ca0ff7a80feac --- /dev/null +++ b/changelog/12026.txt @@ -0,0 +1,3 @@ +```release-note:bug +auth/aws: Fix a bug where the Token TTL for AWS is not capped by Default Lease TTL. +``` \ No newline at end of file From d26ae96f3dbe2deaf7ac74be655fcc3254f6a52b Mon Sep 17 00:00:00 2001 From: Vinay Gopalan Date: Mon, 12 Jul 2021 10:28:55 -0700 Subject: [PATCH 3/6] patch: update warning messages to not include 'at login' --- builtin/credential/aws/path_role.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/builtin/credential/aws/path_role.go b/builtin/credential/aws/path_role.go index 6b6e4cedc4ea8..7abd2f2bbe862 100644 --- a/builtin/credential/aws/path_role.go +++ b/builtin/credential/aws/path_role.go @@ -893,11 +893,11 @@ func (b *backend) pathRoleCreateUpdate(ctx context.Context, req *logical.Request systemMaxTTL := b.System().MaxLeaseTTL() if roleEntry.TokenTTL > defaultLeaseTTL { roleEntry.TokenTTL = defaultLeaseTTL - resp.AddWarning(fmt.Sprintf("Given ttl of %d seconds greater than current mount/system default of %d seconds; ttl will be capped at login time", roleEntry.TokenTTL/time.Second, defaultLeaseTTL/time.Second)) + resp.AddWarning(fmt.Sprintf("Given ttl of %d seconds greater than current mount/system default of %d seconds; ttl will be capped", roleEntry.TokenTTL/time.Second, defaultLeaseTTL/time.Second)) } if roleEntry.TokenMaxTTL > systemMaxTTL { roleEntry.TokenMaxTTL = systemMaxTTL - resp.AddWarning(fmt.Sprintf("Given max ttl of %d seconds greater than current mount/system default of %d seconds; max ttl will be capped at login time", roleEntry.TokenMaxTTL/time.Second, systemMaxTTL/time.Second)) + resp.AddWarning(fmt.Sprintf("Given max ttl of %d seconds greater than current mount/system default of %d seconds; max ttl will be capped", roleEntry.TokenMaxTTL/time.Second, systemMaxTTL/time.Second)) } if roleEntry.TokenMaxTTL != 0 && roleEntry.TokenMaxTTL < roleEntry.TokenTTL { return logical.ErrorResponse("ttl should be shorter than max ttl"), nil From fee12334d715729e3731e46faa36d6fc8f1e4d3f Mon Sep 17 00:00:00 2001 From: Vinay Gopalan Date: Tue, 13 Jul 2021 11:40:51 -0700 Subject: [PATCH 4/6] patch: remove default lease capping and test --- builtin/credential/aws/path_role.go | 6 -- builtin/credential/aws/path_role_test.go | 97 ------------------------ 2 files changed, 103 deletions(-) diff --git a/builtin/credential/aws/path_role.go b/builtin/credential/aws/path_role.go index 7abd2f2bbe862..8a83e54403fba 100644 --- a/builtin/credential/aws/path_role.go +++ b/builtin/credential/aws/path_role.go @@ -889,14 +889,8 @@ func (b *backend) pathRoleCreateUpdate(ctx context.Context, req *logical.Request } } - defaultLeaseTTL := b.System().DefaultLeaseTTL() systemMaxTTL := b.System().MaxLeaseTTL() - if roleEntry.TokenTTL > defaultLeaseTTL { - roleEntry.TokenTTL = defaultLeaseTTL - resp.AddWarning(fmt.Sprintf("Given ttl of %d seconds greater than current mount/system default of %d seconds; ttl will be capped", roleEntry.TokenTTL/time.Second, defaultLeaseTTL/time.Second)) - } if roleEntry.TokenMaxTTL > systemMaxTTL { - roleEntry.TokenMaxTTL = systemMaxTTL resp.AddWarning(fmt.Sprintf("Given max ttl of %d seconds greater than current mount/system default of %d seconds; max ttl will be capped", roleEntry.TokenMaxTTL/time.Second, systemMaxTTL/time.Second)) } if roleEntry.TokenMaxTTL != 0 && roleEntry.TokenMaxTTL < roleEntry.TokenTTL { diff --git a/builtin/credential/aws/path_role_test.go b/builtin/credential/aws/path_role_test.go index 931d78d65603a..5d7a0e313880d 100644 --- a/builtin/credential/aws/path_role_test.go +++ b/builtin/credential/aws/path_role_test.go @@ -6,7 +6,6 @@ import ( "reflect" "strings" "testing" - "time" "github.com/go-test/deep" "github.com/hashicorp/go-hclog" @@ -773,102 +772,6 @@ func TestAwsEc2_RoleDurationSeconds(t *testing.T) { } } -func TestAwsIam_RoleDurationSeconds(t *testing.T) { - config := logical.TestBackendConfig() - storage := &logical.InmemStorage{} - config.StorageView = storage - - b, err := Backend(config) - if err != nil { - t.Fatal(err) - } - - err = b.Setup(context.Background(), config) - if err != nil { - t.Fatal(err) - } - - roleData := map[string]interface{}{ - "auth_type": "iam", - "bound_iam_principal_arn": "arn:aws:iam::123456789012:role/testrole", - "resolve_aws_unique_ids": false, - "ttl": "20m", - "max_ttl": "30m", - } - - roleReq := &logical.Request{ - Operation: logical.CreateOperation, - Storage: storage, - Path: "role/testrole", - Data: roleData, - } - - resp, err := b.HandleRequest(context.Background(), roleReq) - if err != nil || (resp != nil && resp.IsError()) { - t.Fatalf("resp: %#v, err: %v", resp, err) - } - - roleReq.Operation = logical.ReadOperation - - resp, err = b.HandleRequest(context.Background(), roleReq) - if err != nil || (resp != nil && resp.IsError()) { - t.Fatalf("resp: %#v, err: %v", resp, err) - } - - // since default lease TTL for system is 24hr, Token TTL should not be capped - // since max lease TTL for system is 48hr, Token Max TTL should not be capped - if resp.Data["token_ttl"].(int64) != 1200 { - t.Fatalf("bad: token_ttl; expected: 1200, actual: %d", resp.Data["ttl"]) - } - if resp.Data["max_ttl"].(int64) != 1800 { - t.Fatalf("bad: max_ttl; expected: 1800, actual: %d", resp.Data["max_ttl"]) - } - - // set default lease TTL to 10m; Token TTL should be capped at 10m - // set max lease TTL to 20m; Token Max TTL should be capped at 20m - config = &logical.BackendConfig{ - Logger: logging.NewVaultLogger(hclog.Trace), - - System: &logical.StaticSystemView{ - DefaultLeaseTTLVal: time.Minute * 10, - MaxLeaseTTLVal: time.Minute * 20, - }, - StorageView: &logical.InmemStorage{}, - } - - b, err = Backend(config) - if err != nil { - t.Fatal(err) - } - - err = b.Setup(context.Background(), config) - if err != nil { - t.Fatal(err) - } - - roleReq.Operation = logical.CreateOperation - - resp, err = b.HandleRequest(context.Background(), roleReq) - if err != nil || (resp != nil && resp.IsError()) { - t.Fatalf("resp: %#v, err: %v", resp, err) - } - - roleReq.Operation = logical.ReadOperation - - resp, err = b.HandleRequest(context.Background(), roleReq) - if err != nil || (resp != nil && resp.IsError()) { - t.Fatalf("resp: %#v, err: %v", resp, err) - } - - if resp.Data["token_ttl"].(int64) != 600 { - t.Fatalf("bad: token_ttl; expected: 600, actual: %d", resp.Data["ttl"]) - } - - if resp.Data["token_max_ttl"].(int64) != 1200 { - t.Fatalf("bad: token_max_ttl; expected: 1200, actual: %d", resp.Data["ttl"]) - } -} - func TestRoleEntryUpgradeV(t *testing.T) { config := logical.TestBackendConfig() storage := &logical.InmemStorage{} From bc467276bd267c978386d7d8de80f0c6b06a91ed Mon Sep 17 00:00:00 2001 From: Vinay Gopalan Date: Tue, 13 Jul 2021 12:06:42 -0700 Subject: [PATCH 5/6] update changelog --- changelog/12026.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog/12026.txt b/changelog/12026.txt index ca0ff7a80feac..12b6cdda73191 100644 --- a/changelog/12026.txt +++ b/changelog/12026.txt @@ -1,3 +1,3 @@ ```release-note:bug -auth/aws: Fix a bug where the Token TTL for AWS is not capped by Default Lease TTL. +auth/aws: Remove warning stating AWS Token TTL will be capped by the Default Lease TTL. ``` \ No newline at end of file From 72162c4c4796464031df7b5f539b3395d8e4da52 Mon Sep 17 00:00:00 2001 From: Vinay Gopalan Date: Wed, 14 Jul 2021 15:16:36 -0700 Subject: [PATCH 6/6] patch: revert warning message --- builtin/credential/aws/path_role.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/credential/aws/path_role.go b/builtin/credential/aws/path_role.go index 8a83e54403fba..8cca61b3de9e2 100644 --- a/builtin/credential/aws/path_role.go +++ b/builtin/credential/aws/path_role.go @@ -891,7 +891,7 @@ func (b *backend) pathRoleCreateUpdate(ctx context.Context, req *logical.Request systemMaxTTL := b.System().MaxLeaseTTL() if roleEntry.TokenMaxTTL > systemMaxTTL { - resp.AddWarning(fmt.Sprintf("Given max ttl of %d seconds greater than current mount/system default of %d seconds; max ttl will be capped", roleEntry.TokenMaxTTL/time.Second, systemMaxTTL/time.Second)) + resp.AddWarning(fmt.Sprintf("Given max ttl of %d seconds greater than current mount/system default of %d seconds; max ttl will be capped at login time", roleEntry.TokenMaxTTL/time.Second, systemMaxTTL/time.Second)) } if roleEntry.TokenMaxTTL != 0 && roleEntry.TokenMaxTTL < roleEntry.TokenTTL { return logical.ErrorResponse("ttl should be shorter than max ttl"), nil