backport/vault-1.18: honor static role ttl across restarts (#144)

* fix: ensure TTL is honored across restarts when skip import rotation is enabled (#140)

* use NextVaultRotation to determine prioirty across restarts

* add tests

* refactor some existing tests

* rely fully on NextVaultRotation for computing TTL and queue priority

* typo

* changelog

* update deps

.go-version

@@ -1 +1 @@
-1.23.4
+1.23.6

CHANGELOG.md

@@ -1,5 +1,13 @@
 ## Unreleased
 
+## v0.14.5
+
+BUG FIXES:
+
+* Fix a bug where static role passwords are erroneously rotated across backend restarts when using skip import rotation. (https://github.com/hashicorp/vault-plugin-secrets-openldap/pull/140)
+
+## v0.14.4
+
 BUG FIXES:
 
 * Update static role rotation to generate a new password after 2 failed attempts (https://github.com/hashicorp/vault-plugin-secrets-openldap/pull/125)

backend_test.go

@@ -25,8 +25,43 @@ var (
 	testPasswordFromPolicy2 = "TestPolicy2Password"
 )
 
+// getBackend returns an initialized test backend with InmemStorage
 func getBackend(throwsErr bool) (*backend, logical.Storage) {
-	config := &logical.BackendConfig{
+	b, config := getBackendWithConfig(testBackendConfig(), throwsErr)
+	return b, config.StorageView
+}
+
+// getBackendWithConfig returns an initialized test backend for the given
+// config
+func getBackendWithConfig(c *logical.BackendConfig, throwsErr bool) (*backend, *logical.BackendConfig) {
+	b := Backend(&fakeLdapClient{throwErrs: throwsErr})
+	b.Setup(context.Background(), c)
+
+	b.credRotationQueue = queue.New()
+	// Create a context with a cancel method for processing any WAL entries and
+	// populating the queue
+	initCtx := context.Background()
+	ictx, cancel := context.WithCancel(initCtx)
+	b.cancelQueue = cancel
+
+	// Load managed LDAP users into memory from storage
+	staticRoles, err := b.loadManagedUsers(ictx, c.StorageView)
+	if err != nil {
+		// TODO: make this fatal? Requires refactoring all tests to pass in a testing.T
+		b.Logger().Error("error configuring backend: could not read roles from storage")
+	}
+
+	// Load queue and kickoff new periodic ticker
+	b.initQueue(ictx, &logical.InitializationRequest{
+		Storage: c.StorageView,
+	}, staticRoles)
+
+	return b, c
+}
+
+// testBackendConfig returns a backend config with inmem storage
+func testBackendConfig() *logical.BackendConfig {
+	return &logical.BackendConfig{
 		Logger: logging.NewVaultLogger(log.Debug),
 
 		System: &logical.StaticSystemView{
@@ -43,23 +78,6 @@ func getBackend(throwsErr bool) (*backend, logical.Storage) {
 		},
 		StorageView: &logical.InmemStorage{},
 	}
-
-	b := Backend(&fakeLdapClient{throwErrs: throwsErr})
-	b.Setup(context.Background(), config)
-
-	b.credRotationQueue = queue.New()
-	// Create a context with a cancel method for processing any WAL entries and
-	// populating the queue
-	initCtx := context.Background()
-	ictx, cancel := context.WithCancel(initCtx)
-	b.cancelQueue = cancel
-
-	// Load queue and kickoff new periodic ticker
-	b.initQueue(ictx, &logical.InitializationRequest{
-		Storage: config.StorageView,
-	}, nil)
-
-	return b, config.StorageView
 }
 
 var _ ldapClient = (*fakeLdapClient)(nil)

go.mod

@@ -16,7 +16,7 @@ require (
 	github.com/mitchellh/mapstructure v1.5.0
 	github.com/ory/dockertest/v3 v3.10.0
 	github.com/stretchr/testify v1.9.0
-	golang.org/x/text v0.18.0
+	golang.org/x/text v0.21.0
 )
 
 require (
@@ -97,9 +97,9 @@ require (
 	go.opentelemetry.io/otel/sdk v1.29.0 // indirect
 	go.opentelemetry.io/otel/trace v1.29.0 // indirect
 	go.uber.org/atomic v1.9.0 // indirect
-	golang.org/x/crypto v0.26.0 // indirect
-	golang.org/x/net v0.28.0 // indirect
-	golang.org/x/sys v0.24.0 // indirect
+	golang.org/x/crypto v0.31.0 // indirect
+	golang.org/x/net v0.33.0 // indirect
+	golang.org/x/sys v0.28.0 // indirect
 	golang.org/x/time v0.3.0 // indirect
 	google.golang.org/genproto/googleapis/api v0.0.0-20240903143218-8af14fe29dc1 // indirect
 	google.golang.org/genproto/googleapis/rpc v0.0.0-20240827150818-7e3bb234dfed // indirect

go.sum

@@ -336,8 +336,8 @@ golang.org/x/crypto v0.0.0-20210921155107-089bfa567519/go.mod h1:GvvjBRRGRdwPK5y
 golang.org/x/crypto v0.6.0/go.mod h1:OFC/31mSvZgRz0V1QTNCzfAI1aIRzbiufJtkMIlEp58=
 golang.org/x/crypto v0.19.0/go.mod h1:Iy9bg/ha4yyC70EfRS8jz+B6ybOBKMaSxLj6P6oBDfU=
 golang.org/x/crypto v0.21.0/go.mod h1:0BP7YvVV9gBbVKyeTG0Gyn+gZm94bibOW5BjDEYAOMs=
-golang.org/x/crypto v0.26.0 h1:RrRspgV4mU+YwB4FYnuBoKsUapNIL5cohGAmSH3azsw=
-golang.org/x/crypto v0.26.0/go.mod h1:GY7jblb9wI+FOo5y8/S2oY4zWP07AkOJ4+jxCqdqn54=
+golang.org/x/crypto v0.31.0 h1:ihbySMvVjLAeSH1IbfcRTkD/iNscyz8rGzjF/E5hV6U=
+golang.org/x/crypto v0.31.0/go.mod h1:kDsLvtWBEx7MV9tJOj9bnXsPbxwJQ6csT/x4KIN4Ssk=
 golang.org/x/exp v0.0.0-20240222234643-814bf88cf225 h1:LfspQV/FYTatPTr/3HzIcmiUFH7PGP+OQ6mgDYo3yuQ=
 golang.org/x/exp v0.0.0-20240222234643-814bf88cf225/go.mod h1:CxmFvTBINI24O/j8iY7H1xHzx2i4OsyguNBmN/uPtqc=
 golang.org/x/mod v0.2.0/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA=
@@ -358,8 +358,8 @@ golang.org/x/net v0.7.0/go.mod h1:2Tu9+aMcznHK/AK1HMvgo6xiTLG5rD5rZLDS+rp2Bjs=
 golang.org/x/net v0.10.0/go.mod h1:0qNGK6F8kojg2nk9dLZ2mShWaEBan6FAoqfSigmmuDg=
 golang.org/x/net v0.21.0/go.mod h1:bIjVDfnllIU7BJ2DNgfnXvpSvtn8VRwhlsaeUTyUS44=
 golang.org/x/net v0.22.0/go.mod h1:JKghWKKOSdJwpW2GEx0Ja7fmaKnMsbu+MWVZTokSYmg=
-golang.org/x/net v0.28.0 h1:a9JDOJc5GMUJ0+UDqmLT86WiEy7iWyIhz8gz8E4e5hE=
-golang.org/x/net v0.28.0/go.mod h1:yqtgsTWOOnlGLG9GFRrK3++bGOUEkNBoHZc8MEDWPNg=
+golang.org/x/net v0.33.0 h1:74SYHlV8BIgHIFC/LrYkOGIwL19eTYXQ5wc6TBuO36I=
+golang.org/x/net v0.33.0/go.mod h1:HXLR5J+9DxmrqMwG9qjGCxZ+zKXxBru04zlTvWlWuN4=
 golang.org/x/sync v0.0.0-20181108010431-42b317875d0f/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
 golang.org/x/sync v0.0.0-20181221193216-37e7f081c4d4/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
 golang.org/x/sync v0.0.0-20190423024810-112230192c58/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
@@ -391,8 +391,8 @@ golang.org/x/sys v0.6.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 golang.org/x/sys v0.8.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 golang.org/x/sys v0.17.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA=
 golang.org/x/sys v0.18.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA=
-golang.org/x/sys v0.24.0 h1:Twjiwq9dn6R1fQcyiK+wQyHWfaz/BJB+YIpzU/Cv3Xg=
-golang.org/x/sys v0.24.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA=
+golang.org/x/sys v0.28.0 h1:Fksou7UEQUWlKvIdsqzJmUmCX3cZuD2+P3XyyzwMhlA=
+golang.org/x/sys v0.28.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA=
 golang.org/x/term v0.0.0-20201126162022-7de9c90e9dd1/go.mod h1:bj7SfCRtBDWHUb9snDiAeCFNEtKQo2Wmx5Cou7ajbmo=
 golang.org/x/term v0.0.0-20210927222741-03fcf44c2211/go.mod h1:jbD1KX2456YbFQfuXm/mYQcufACuNUgVhRMnK/tPxf8=
 golang.org/x/term v0.5.0/go.mod h1:jMB1sMXY+tzblOD4FWmEbocvup2/aLOaQEp7JmGp78k=
@@ -405,8 +405,8 @@ golang.org/x/text v0.3.7/go.mod h1:u+2+/6zg+i71rQMx5EYifcz6MCKuco9NR6JIITiCfzQ=
 golang.org/x/text v0.7.0/go.mod h1:mrYo+phRRbMaCq/xk9113O4dZlRixOauAjOtrjsXDZ8=
 golang.org/x/text v0.9.0/go.mod h1:e1OnstbJyHTd6l/uOt8jFFHp6TRDWZR/bV3emEE/zU8=
 golang.org/x/text v0.14.0/go.mod h1:18ZOQIKpY8NJVqYksKHtTdi31H5itFRjB5/qKTNYzSU=
-golang.org/x/text v0.18.0 h1:XvMDiNzPAl0jr17s6W9lcaIhGUfUORdGCNsuLmPG224=
-golang.org/x/text v0.18.0/go.mod h1:BuEKDfySbSR4drPmRPG/7iBdf8hvFMuRexcpahXilzY=
+golang.org/x/text v0.21.0 h1:zyQAAkrwaneQ066sspRyJaG9VNi/YJ1NfzcGB3hZ/qo=
+golang.org/x/text v0.21.0/go.mod h1:4IBbMaMmOPCJ8SecivzSH54+73PCFmPWxNTLm+vZkEQ=
 golang.org/x/time v0.3.0 h1:rg5rLMjNzMS1RkNLzCG38eapWhnYLFYXDXj2gOlr8j4=
 golang.org/x/time v0.3.0/go.mod h1:tRJNPiyCQ0inRvYxbN9jk5I+vvW/OXSQhTDSoE431IQ=
 golang.org/x/tools v0.0.0-20180917221912-90fa682c2a6e/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ=

path_static_roles.go

@@ -326,16 +326,30 @@ func (b *backend) pathStaticRoleCreateUpdate(ctx context.Context, req *logical.R
 		skipRotation = c.SkipStaticRoleImportRotation
 	}
 
-	// lvr represents the role's LastVaultRotation
-	lvr := role.StaticAccount.LastVaultRotation
+	lastVaultRotation := role.StaticAccount.LastVaultRotation
 
 	// Only call setStaticAccountPassword if we're creating the role for the first time
 	var item *queue.Item
 	switch req.Operation {
 	case logical.CreateOperation:
-		// if we were asked to not rotate, just add the entry - this essentially becomes an update operation, except
-		// the item is new
 		if skipRotation {
+			b.Logger().Debug("skipping static role import rotation", "role", name)
+
+			// Synthetically set lastVaultRotation to now so that it gets
+			// queued correctly.
+			// NOTE: We intentionally do not set role.StaticAccount.LastVaultRotation
+			// because the zero value indicates Vault has not rotated the
+			// password yet.
+			lastVaultRotation = time.Now()
+
+			// NextVaultRotation allows calculating the TTL on GET /static-creds
+			// requests and to calculate the queue priority in populateQueue()
+			// across restarts. We can't rely on LastVaultRotation in these
+			// cases because, when import rotation is skipped, LastVaultRotation
+			// is set to a zero value in storage.
+			role.StaticAccount.SetNextVaultRotation(lastVaultRotation)
+
+			// we were told not to rotate, just add the entry
 			entry, err := logical.StorageEntryJSON(staticRolePath+name, role)
 			if err != nil {
 				return nil, err
@@ -348,8 +362,6 @@ func (b *backend) pathStaticRoleCreateUpdate(ctx context.Context, req *logical.R
 			item = &queue.Item{
 				Key: name,
 			}
-			// synthetically set lvr to now, so that it gets queued correctly
-			lvr = time.Now()
 			break
 		} else {
 			// setStaticAccountPassword calls Storage.Put and saves the role to storage
@@ -372,7 +384,7 @@ func (b *backend) pathStaticRoleCreateUpdate(ctx context.Context, req *logical.R
 				return nil, err
 			}
 			// guard against RotationTime not being set or zero-value
-			lvr = resp.RotationTime
+			lastVaultRotation = resp.RotationTime
 			item = &queue.Item{
 				Key: name,
 			}
@@ -397,7 +409,7 @@ func (b *backend) pathStaticRoleCreateUpdate(ctx context.Context, req *logical.R
 		}
 	}
 
-	item.Priority = lvr.Add(role.StaticAccount.RotationPeriod).Unix()
+	item.Priority = lastVaultRotation.Add(role.StaticAccount.RotationPeriod).Unix()
 
 	// Add their rotation to the queue
 	if err := b.pushItem(item); err != nil {
@@ -429,9 +441,14 @@ type staticAccount struct {
 	// This is returned on credential requests if it exists.
 	LastPassword string `json:"last_password"`
 
-	// LastVaultRotation represents the last time Vault rotated the password
+	// LastVaultRotation represents the last time Vault rotated the password. A
+	// zero value indicates the the password has never been rotated by Vault.
 	LastVaultRotation time.Time `json:"last_vault_rotation"`
 
+	// NextVaultRotation represents the next time Vault is expected to rotate
+	// the password
+	NextVaultRotation time.Time `json:"next_vault_rotation"`
+
 	// RotationPeriod is number in seconds between each rotation, effectively a
 	// "time to live". This value is compared to the LastVaultRotation to
 	// determine if a password needs to be rotated
@@ -441,7 +458,13 @@ type staticAccount struct {
 // NextRotationTime calculates the next rotation by adding the Rotation Period
 // to the last known vault rotation
 func (s *staticAccount) NextRotationTime() time.Time {
-	return s.LastVaultRotation.Add(s.RotationPeriod)
+	return s.NextVaultRotation
+}
+
+// SetNextVaultRotation sets the next vault rotation to time t plus the role's
+// rotation period.
+func (s *staticAccount) SetNextVaultRotation(t time.Time) {
+	s.NextVaultRotation = t.Add(s.RotationPeriod)
 }
 
 // PasswordTTL calculates the approximate time remaining until the password is

path_static_roles_test.go

@@ -486,7 +486,6 @@ func TestRoles_NewPasswordGeneration(t *testing.T) {
 		if resp.Data["password"] == initialPassword {
 			t.Fatalf("expected password to be different after second retry")
 		}
-
 	})
 
 	t.Run("updating password policy should generate new password", func(t *testing.T) {
@@ -538,7 +537,6 @@ func TestRoles_NewPasswordGeneration(t *testing.T) {
 		// Ensure WAL is flushed
 		walIDs = requireWALs(t, storage, 0)
 	})
-
 }
 
 func TestListRoles(t *testing.T) {

rotation.go

@@ -403,6 +403,7 @@ func (b *backend) setStaticAccountPassword(ctx context.Context, s logical.Storag
 	// lvr is the known LastVaultRotation
 	lvr := time.Now()
 	input.Role.StaticAccount.LastVaultRotation = lvr
+	input.Role.StaticAccount.SetNextVaultRotation(lvr)
 	input.Role.StaticAccount.LastPassword = input.Role.StaticAccount.Password
 	input.Role.StaticAccount.Password = newPassword
 	output.RotationTime = lvr

rotation_test.go

@@ -55,45 +55,18 @@ func TestInitQueueHierarchicalPaths(t *testing.T) {
 		},
 	} {
 		t.Run(tc.name, func(t *testing.T) {
-			ctx := context.Background()
-			config := &logical.BackendConfig{
-				Logger: logging.NewVaultLogger(log.Debug),
-
-				System: &logical.StaticSystemView{
-					DefaultLeaseTTLVal: defaultLeaseTTLVal,
-					MaxLeaseTTLVal:     maxLeaseTTLVal,
-				},
-				StorageView: &logical.InmemStorage{},
-			}
+			b, config := getBackendWithConfig(testBackendConfig(), false)
+			defer b.Cleanup(context.Background())
+			storage := config.StorageView
 
-			b := Backend(&fakeLdapClient{throwErrs: false})
-			b.Setup(context.Background(), config)
-
-			b.credRotationQueue = queue.New()
-			initCtx := context.Background()
-			ictx, cancel := context.WithCancel(initCtx)
-			b.cancelQueue = cancel
-
-			defer b.Cleanup(ctx)
-			configureOpenLDAPMount(t, b, config.StorageView)
+			configureOpenLDAPMount(t, b, storage)
 
 			for _, r := range tc.roles {
 				createRole(t, b, config.StorageView, r)
 			}
 
-			// Reset the rotation queue to simulate startup memory state
-			b.credRotationQueue = queue.New()
-
-			// Load managed LDAP users into memory from storage
-			staticRoles, err := b.loadManagedUsers(ictx, config.StorageView)
-			if err != nil {
-				t.Fatal(err)
-			}
-
-			// Now finish the startup process by populating the queue
-			b.initQueue(ictx, &logical.InitializationRequest{
-				Storage: config.StorageView,
-			}, staticRoles)
+			// Reload backend to similate a Vault restart/startup memory state
+			getBackendWithConfig(config, false)
 
 			queueLen := b.credRotationQueue.Len()
 			if queueLen != len(tc.roles) {
@@ -203,6 +176,78 @@ func TestAutoRotate(t *testing.T) {
 			t.Fatal("expected last_password to be equal to old password after auto rotation")
 		}
 	})
+
+	t.Run("skip_import_rotation is true and rotates after ttl expiration", func(t *testing.T) {
+		b, storage := getBackend(false)
+		defer b.Cleanup(context.Background())
+
+		configureOpenLDAPMount(t, b, storage)
+
+		data := map[string]interface{}{
+			"username":             "hashicorp",
+			"dn":                   "uid=hashicorp,ou=users,dc=hashicorp,dc=com",
+			"rotation_period":      "5s",
+			"skip_import_rotation": true,
+		}
+
+		roleName := "hashicorp"
+		createStaticRoleWithData(t, b, storage, roleName, data)
+		resp := readStaticCred(t, b, storage, roleName)
+
+		if resp.Data["password"] != "" {
+			t.Fatal("expected password to be empty, it wasn't: skip_import_rotation was enabled, password should not be rotated on import")
+		}
+
+		// Wait for auto rotation (5s) + 1 second for breathing room
+		time.Sleep(time.Second * 6)
+
+		resp = readStaticCred(t, b, storage, roleName)
+
+		if resp.Data["password"] == "" {
+			t.Fatal("expected password to be set after auto rotation, it wasn't")
+		}
+		if resp.Data["last_password"] != "" {
+			t.Fatal("expected last_password to be empty after auto rotation, it wasn't")
+		}
+	})
+
+	t.Run("skip_import_rotation is true and does not rotate after backend reload", func(t *testing.T) {
+		b, config := getBackendWithConfig(testBackendConfig(), false)
+		defer b.Cleanup(context.Background())
+		storage := config.StorageView
+
+		configureOpenLDAPMount(t, b, storage)
+
+		data := map[string]interface{}{
+			"username":             "hashicorp",
+			"dn":                   "uid=hashicorp,ou=users,dc=hashicorp,dc=com",
+			"rotation_period":      "10m",
+			"skip_import_rotation": true,
+		}
+
+		roleName := "hashicorp"
+		createStaticRoleWithData(t, b, storage, roleName, data)
+		resp := readStaticCred(t, b, storage, roleName)
+
+		if resp.Data["password"] != "" {
+			t.Fatal("expected password to be empty, it wasn't: skip_import_rotation was enabled, password should not be rotated on import")
+		}
+		if resp.Data["last_password"] != "" {
+			t.Fatal("expected last_password to be empty, it wasn't")
+		}
+
+		// Reload backend to similate a Vault restart/startup memory state
+		getBackendWithConfig(config, false)
+
+		resp = readStaticCred(t, b, storage, roleName)
+
+		if resp.Data["password"] != "" {
+			t.Fatal("expected password to be empty after backend reload, it wasn't: skip_import_rotation was enabled, password should not be rotated yet")
+		}
+		if resp.Data["last_password"] != "" {
+			t.Fatal("expected last_password to be empty after backend reload, it wasn't")
+		}
+	})
 }
 
 // TestPasswordPolicyModificationInvalidatesWAL tests that modification of the