VAULT-18307 Vault incorrectly requeues credentials when the rotation period is updated

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{

builtin/logical/aws/path_static_roles_test.go

@@ -6,6 +6,8 @@ package aws
 import (
 	"context"
 	"errors"
+	"fmt"
+	"math"
 	"testing"
 	"time"
 
@@ -124,12 +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
+		// 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",
@@ -168,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{
@@ -187,8 +198,33 @@ 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(),
+		},
+		{
+			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(),
 		},
 	}
 
@@ -269,6 +305,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 +330,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; math.Abs(float64(ep-ap)) > 5 {
+					t.Fatalf("mismatched updated prioirt, expected %d but got %d", ep, ap)
+				}
+			}
 		})
 	}
 }

changelog/23528.txt

@@ -0,0 +1,3 @@
+```release-note:bug
+secrets/aws: update credential rotation deadline when static role rotation period is updated
+```

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: <required>)` – 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