From 6784fb09d4eade055e49479762dd22e01bc5b4a0 Mon Sep 17 00:00:00 2001 From: Kay Craig Date: Wed, 4 Oct 2023 12:27:31 -0400 Subject: [PATCH 01/10] put in the fix we talked about --- builtin/logical/aws/path_static_roles.go | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/builtin/logical/aws/path_static_roles.go b/builtin/logical/aws/path_static_roles.go index 52f1eaf3efba..ab2a20cb42c6 100644 --- a/builtin/logical/aws/path_static_roles.go +++ b/builtin/logical/aws/path_static_roles.go @@ -212,6 +212,21 @@ func (b *backend) pathStaticRolesWrite(ctx context.Context, req *logical.Request if err != nil { return nil, fmt.Errorf("failed to add item into the rotation queue for role %q: %w", config.Name, err) } + } else { + // creds already exist, so all we need to do is update the rotation + // what here stays the same and what changes? Can we change the name? + i, err := b.credRotationQueue.PopByKey(config.Name) + if err != nil { + return nil, fmt.Errorf("expected an item with name %q, but got an error: %w", config.Name, err) + } + i.Value = config + // a few options for updating the priority - the most straightforward is to just say it's rotation period + now. + // this might not be ideal since it would allow infinite postponing of the deadline. + i.Priority = time.Now().Add(config.RotationPeriod).Unix() + err = b.credRotationQueue.Push(i) + if err != nil { + return nil, fmt.Errorf("failed to add updated item into the rotation queue for role %q: %w", config.Name, err) + } } return &logical.Response{ From baf7b85f5b962b046e3013fa3664a66f6f5eb91e Mon Sep 17 00:00:00 2001 From: Kay Craig Date: Thu, 5 Oct 2023 13:14:46 -0400 Subject: [PATCH 02/10] add changelog --- changelog/23528.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 changelog/23528.txt diff --git a/changelog/23528.txt b/changelog/23528.txt new file mode 100644 index 000000000000..ad9ec4f4b7bd --- /dev/null +++ b/changelog/23528.txt @@ -0,0 +1,3 @@ +```release-note:bug +secrets/aws: update credential rotation deadline when static role rotation period is updated +``` From cd8a69ed5fd625286d272aec09733566376e7d41 Mon Sep 17 00:00:00 2001 From: Kay Craig Date: Mon, 9 Oct 2023 11:08:58 -0400 Subject: [PATCH 03/10] add single update case in writeuser tests --- builtin/logical/aws/path_static_roles_test.go | 43 ++++++++++++++++++- 1 file changed, 41 insertions(+), 2 deletions(-) diff --git a/builtin/logical/aws/path_static_roles_test.go b/builtin/logical/aws/path_static_roles_test.go index f46981d6cee1..074e7e9f4a33 100644 --- a/builtin/logical/aws/path_static_roles_test.go +++ b/builtin/logical/aws/path_static_roles_test.go @@ -6,6 +6,7 @@ package aws import ( "context" "errors" + "fmt" "testing" "time" @@ -130,6 +131,7 @@ func TestStaticRolesWrite(t *testing.T) { expectedError bool findUser bool isUpdate bool + newPriority int64 // update time of new item in queue, skip if isUpdate false }{ { name: "happy path", @@ -187,9 +189,11 @@ func TestStaticRolesWrite(t *testing.T) { "name": "johnny", "rotation_period": "19m", }, - findUser: true, - isUpdate: true, + findUser: true, + isUpdate: true, + newPriority: time.Now().Add(19 * time.Minute).Unix(), }, + {}, } // if a user exists (user doesn't exist is tested in validation) @@ -269,6 +273,11 @@ func TestStaticRolesWrite(t *testing.T) { expectedData = staticRole } + var actualItem *queue.Item + if c.isUpdate { + actualItem, _ = b.credRotationQueue.PopByKey(expectedData.Name) + } + if u, ok := fieldData.GetOk("username"); ok { expectedData.Username = u.(string) } @@ -289,6 +298,13 @@ func TestStaticRolesWrite(t *testing.T) { if en, an := expectedData.Name, actualData.Name; en != an { t.Fatalf("mismatched role name, expected %q, but got %q", en, an) } + + if c.isUpdate { + fmt.Printf("%d vs %d\n", c.newPriority, actualItem.Priority) + if ep, ap := c.newPriority, actualItem.Priority; ep != ap { + t.Fatalf("mismatched updated prioirt, expected %d but got %d", ep, ap) + } + } }) } } @@ -362,6 +378,29 @@ func TestStaticRoleRead(t *testing.T) { } } +// TestStaticRoleUpdate validates that an update, i.e., a create when a role with that name already exists, will +// properly modify the queue. +func TestStaticRoleUpdate(t *testing.T) { + // bgCTX := context.Background() + + cases := []struct { + name string + data map[string]interface{} + }{ + { + name: "increase duration", + }, + { + name: "decrease duration", + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + }) + } +} + // TestStaticRoleDelete validates that we correctly remove a role on a delete request, and that we correctly do not // remove anything if a role does not exist with that name. func TestStaticRoleDelete(t *testing.T) { From 377440e54c3c3a16a796ac2eff5f035d3c2a49b0 Mon Sep 17 00:00:00 2001 From: Kay Craig Date: Mon, 9 Oct 2023 11:19:06 -0400 Subject: [PATCH 04/10] add some more test cases for duration updates --- builtin/logical/aws/path_static_roles_test.go | 69 +++++++++++-------- 1 file changed, 39 insertions(+), 30 deletions(-) diff --git a/builtin/logical/aws/path_static_roles_test.go b/builtin/logical/aws/path_static_roles_test.go index 074e7e9f4a33..357cdf16e4aa 100644 --- a/builtin/logical/aws/path_static_roles_test.go +++ b/builtin/logical/aws/path_static_roles_test.go @@ -7,6 +7,7 @@ import ( "context" "errors" "fmt" + "math" "testing" "time" @@ -125,13 +126,21 @@ func TestStaticRolesWrite(t *testing.T) { bgCTX := context.Background() cases := []struct { - name string - opts []awsutil.MockIAMOption + name string + // objects to return from mock IAM. + // You'll need a GetUserOutput (to validate the existence of the user being written, + // the keys the user has already been assigned, + // and the new key vault requests. + opts []awsutil.MockIAMOption // objects to return from the mock IAM + // the name, username if updating, and rotation_period of the user. This is the inbound request the cod would get. data map[string]interface{} expectedError bool findUser bool - isUpdate bool - newPriority int64 // update time of new item in queue, skip if isUpdate false + // if data is sent the name "johnny", then we'll match an existing user with rotation period 24 hours. + isUpdate bool + newPriority int64 // update time of new item in queue, skip if isUpdate false. There is a wiggle room of 5 seconds + // so the deltas between the old and the new update time should be larger than that to ensure the difference + // can be detected. }{ { name: "happy path", @@ -170,7 +179,7 @@ func TestStaticRolesWrite(t *testing.T) { expectedError: true, }, { - name: "update existing user", + name: "update existing user, decreased rotation duration", opts: []awsutil.MockIAMOption{ awsutil.WithGetUserOutput(&iam.GetUserOutput{User: &iam.User{UserName: aws.String("john-doe"), UserId: aws.String("unique-id")}}), awsutil.WithListAccessKeysOutput(&iam.ListAccessKeysOutput{ @@ -193,7 +202,30 @@ func TestStaticRolesWrite(t *testing.T) { isUpdate: true, newPriority: time.Now().Add(19 * time.Minute).Unix(), }, - {}, + { + name: "update existing user, increased rotation duration", + opts: []awsutil.MockIAMOption{ + awsutil.WithGetUserOutput(&iam.GetUserOutput{User: &iam.User{UserName: aws.String("john-doe"), UserId: aws.String("unique-id")}}), + awsutil.WithListAccessKeysOutput(&iam.ListAccessKeysOutput{ + AccessKeyMetadata: []*iam.AccessKeyMetadata{}, + IsTruncated: aws.Bool(false), + }), + awsutil.WithCreateAccessKeyOutput(&iam.CreateAccessKeyOutput{ + AccessKey: &iam.AccessKey{ + AccessKeyId: aws.String("abcdefghijklmnopqrstuvwxyz"), + SecretAccessKey: aws.String("zyxwvutsrqponmlkjihgfedcba"), + UserName: aws.String("john-doe"), + }, + }), + }, + data: map[string]interface{}{ + "name": "johnny", + "rotation_period": "40h", + }, + findUser: true, + isUpdate: true, + newPriority: time.Now().Add(40 * time.Hour).Unix(), + }, } // if a user exists (user doesn't exist is tested in validation) @@ -301,7 +333,7 @@ func TestStaticRolesWrite(t *testing.T) { if c.isUpdate { fmt.Printf("%d vs %d\n", c.newPriority, actualItem.Priority) - if ep, ap := c.newPriority, actualItem.Priority; ep != ap { + if ep, ap := c.newPriority, actualItem.Priority; math.Abs(float64(ep-ap)) > 5 { t.Fatalf("mismatched updated prioirt, expected %d but got %d", ep, ap) } } @@ -378,29 +410,6 @@ func TestStaticRoleRead(t *testing.T) { } } -// TestStaticRoleUpdate validates that an update, i.e., a create when a role with that name already exists, will -// properly modify the queue. -func TestStaticRoleUpdate(t *testing.T) { - // bgCTX := context.Background() - - cases := []struct { - name string - data map[string]interface{} - }{ - { - name: "increase duration", - }, - { - name: "decrease duration", - }, - } - - for _, c := range cases { - t.Run(c.name, func(t *testing.T) { - }) - } -} - // TestStaticRoleDelete validates that we correctly remove a role on a delete request, and that we correctly do not // remove anything if a role does not exist with that name. func TestStaticRoleDelete(t *testing.T) { From e1958e3aaf9e3eb41b21b0c7c4c5c17d6fc108b3 Mon Sep 17 00:00:00 2001 From: Kay Craig Date: Mon, 9 Oct 2023 11:26:18 -0400 Subject: [PATCH 05/10] document update rotation period behavior --- website/content/api-docs/secret/aws.mdx | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/website/content/api-docs/secret/aws.mdx b/website/content/api-docs/secret/aws.mdx index 54cfdbbfab33..cf55fad8a014 100644 --- a/website/content/api-docs/secret/aws.mdx +++ b/website/content/api-docs/secret/aws.mdx @@ -586,7 +586,7 @@ $ curl \ } ``` -## Create static role +## Create/Update static role This endpoint creates or updates static role definitions. A static role is a 1-to-1 mapping with an AWS IAM User, which will be adopted and managed by Vault, including rotating it according to the configured `rotation_period`. @@ -612,7 +612,8 @@ is specified as part of the URL. - `rotation_period` `(string/int: )` – Specifies the amount of time Vault should wait before rotating the password. The minimum is 1 minute. Can be -specified in either `24h` or `86400` format (see [duration format strings](/vault/docs/concepts/duration-format)). +specified in either `24h` or `86400` format (see [duration format strings](/vault/docs/concepts/duration-format)). On an update, +this will 'reset' the next rotation to occur at `now` + `rotation_period`. ### Sample payload From b69468096c09259367fde97afe3e8ec394981e1b Mon Sep 17 00:00:00 2001 From: Kay Craig Date: Mon, 9 Oct 2023 16:35:56 -0400 Subject: [PATCH 06/10] update documentation to avoid using 'this' as a subject --- website/content/api-docs/secret/aws.mdx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/website/content/api-docs/secret/aws.mdx b/website/content/api-docs/secret/aws.mdx index cf55fad8a014..9c3b4946a8e0 100644 --- a/website/content/api-docs/secret/aws.mdx +++ b/website/content/api-docs/secret/aws.mdx @@ -612,8 +612,8 @@ is specified as part of the URL. - `rotation_period` `(string/int: )` – Specifies the amount of time Vault should wait before rotating the password. The minimum is 1 minute. Can be -specified in either `24h` or `86400` format (see [duration format strings](/vault/docs/concepts/duration-format)). On an update, -this will 'reset' the next rotation to occur at `now` + `rotation_period`. +specified in either `24h` or `86400` format (see [duration format strings](/vault/docs/concepts/duration-format)). +Updating the rotation period will 'reset' the next rotation to occur at `now` + `rotation_period`. ### Sample payload From e2a19db8c566099e5d82d2919b5dbc07ebba0ce6 Mon Sep 17 00:00:00 2001 From: Kay Craig Date: Mon, 9 Oct 2023 17:29:54 -0400 Subject: [PATCH 07/10] remove extra print --- builtin/logical/aws/path_static_roles_test.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/builtin/logical/aws/path_static_roles_test.go b/builtin/logical/aws/path_static_roles_test.go index 357cdf16e4aa..feef935f2103 100644 --- a/builtin/logical/aws/path_static_roles_test.go +++ b/builtin/logical/aws/path_static_roles_test.go @@ -6,7 +6,6 @@ package aws import ( "context" "errors" - "fmt" "math" "testing" "time" @@ -332,7 +331,6 @@ func TestStaticRolesWrite(t *testing.T) { } if c.isUpdate { - fmt.Printf("%d vs %d\n", c.newPriority, actualItem.Priority) if ep, ap := c.newPriority, actualItem.Priority; math.Abs(float64(ep-ap)) > 5 { t.Fatalf("mismatched updated prioirt, expected %d but got %d", ep, ap) } From 641ae09f6d4ae4d00d9ff584eb7f4d5f082443f8 Mon Sep 17 00:00:00 2001 From: Kay Craig Date: Mon, 9 Oct 2023 17:35:54 -0400 Subject: [PATCH 08/10] remove import of math is this worth it? who can say --- builtin/logical/aws/path_static_roles_test.go | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/builtin/logical/aws/path_static_roles_test.go b/builtin/logical/aws/path_static_roles_test.go index feef935f2103..0bad32a4d4d0 100644 --- a/builtin/logical/aws/path_static_roles_test.go +++ b/builtin/logical/aws/path_static_roles_test.go @@ -6,7 +6,6 @@ package aws import ( "context" "errors" - "math" "testing" "time" @@ -330,8 +329,16 @@ func TestStaticRolesWrite(t *testing.T) { t.Fatalf("mismatched role name, expected %q, but got %q", en, an) } + // one-off to avoid importing/casting + abs := func(x int64) int64 { + if x < 0 { + return -x + } + return x + } + if c.isUpdate { - if ep, ap := c.newPriority, actualItem.Priority; math.Abs(float64(ep-ap)) > 5 { + if ep, ap := c.newPriority, actualItem.Priority; abs(ep-ap) > 5 { // 5 second wiggle room for how long the test takes t.Fatalf("mismatched updated prioirt, expected %d but got %d", ep, ap) } } From b90b1d7541890e56d902dc16810a87438c914dfa Mon Sep 17 00:00:00 2001 From: Kay Craig Date: Tue, 10 Oct 2023 10:15:01 -0400 Subject: [PATCH 09/10] fix typo in test alert --- builtin/logical/aws/path_static_roles_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/logical/aws/path_static_roles_test.go b/builtin/logical/aws/path_static_roles_test.go index 0bad32a4d4d0..a56225b57580 100644 --- a/builtin/logical/aws/path_static_roles_test.go +++ b/builtin/logical/aws/path_static_roles_test.go @@ -339,7 +339,7 @@ func TestStaticRolesWrite(t *testing.T) { if c.isUpdate { if ep, ap := c.newPriority, actualItem.Priority; abs(ep-ap) > 5 { // 5 second wiggle room for how long the test takes - t.Fatalf("mismatched updated prioirt, expected %d but got %d", ep, ap) + t.Fatalf("mismatched updated priority, expected %d but got %d", ep, ap) } } }) From 9abdaefe38c523b03cfa1011d69e1259b3ad4966 Mon Sep 17 00:00:00 2001 From: Kay Craig Date: Wed, 11 Oct 2023 12:52:30 -0400 Subject: [PATCH 10/10] clean up comment on rotation update --- builtin/logical/aws/path_static_roles.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/builtin/logical/aws/path_static_roles.go b/builtin/logical/aws/path_static_roles.go index ab2a20cb42c6..f07eab54ab18 100644 --- a/builtin/logical/aws/path_static_roles.go +++ b/builtin/logical/aws/path_static_roles.go @@ -220,8 +220,7 @@ func (b *backend) pathStaticRolesWrite(ctx context.Context, req *logical.Request return nil, fmt.Errorf("expected an item with name %q, but got an error: %w", config.Name, err) } i.Value = config - // a few options for updating the priority - the most straightforward is to just say it's rotation period + now. - // this might not be ideal since it would allow infinite postponing of the deadline. + // update the next rotation to occur at now + the new rotation period i.Priority = time.Now().Add(config.RotationPeriod).Unix() err = b.credRotationQueue.Push(i) if err != nil {