prevent role and set duplicates (#123)

* prevent duplicate users in static roles and library sets

* do not make redundant calls to storage on init

* changelog

CHANGELOG.md

@@ -1,5 +1,11 @@
 ## Unreleased
 
+## v0.14.3
+
+BUG FIXES:
+
+* fix an edge case where add an LDAP user or service account can be added to more than one role or set (https://github.com/hashicorp/vault-plugin-secrets-openldap/pull/123)
+
 ## v0.14.2
 
 BUG FIXES:

backend.go

@@ -5,6 +5,7 @@ package openldap
 
 import (
 	"context"
+	"fmt"
 	"strings"
 	"sync"
 
@@ -88,7 +89,8 @@ func Backend(client ldapClient) *backend {
 
 func (b *backend) initialize(ctx context.Context, initRequest *logical.InitializationRequest) error {
 	// Load managed LDAP users into memory from storage
-	if err := b.loadManagedUsers(ctx, initRequest.Storage); err != nil {
+	staticRoles, err := b.loadManagedUsers(ctx, initRequest.Storage)
+	if err != nil {
 		return err
 	}
 
@@ -98,7 +100,7 @@ func (b *backend) initialize(ctx context.Context, initRequest *logical.Initializ
 	b.cancelQueue = cancel
 
 	// Load static role queue and kickoff new periodic ticker
-	go b.initQueue(ictx, initRequest)
+	go b.initQueue(ictx, initRequest, staticRoles)
 
 	return nil
 }
@@ -149,10 +151,57 @@ type backend struct {
 	checkOutLocks []*locksutil.LockEntry
 }
 
+// walkfunc type takes a storage path argument and returns true if a storage
+// entry exists, false otherwise
+type walkFunc func(string) (bool, error)
+
+// walkStoragePath performs a non-recursive breadth-first search of the given
+// storage path and applies the walkfunc to each storage entry
+func walkStoragePath(ctx context.Context, s logical.Storage, path string, walker walkFunc) error {
+	keys, err := s.List(ctx, path)
+	if err != nil {
+		return fmt.Errorf("unable to list keys: %w", err)
+	}
+
+	// Storage entries can be defined with hierarchical paths, e.g.
+	// "foo/bar/baz". But the storage.List() call to the top-level key will
+	// only return the top-level keys in the hierarchy. So we perform a
+	// non-recursive breadth-first search through all the keys returned from
+	// storage.List() and apply the walkfunc.
+	for i := 0; i < len(keys); i++ {
+		key := keys[i]
+
+		entryExists, err := walker(key)
+		if err != nil {
+			return fmt.Errorf("unable to read entry %q: %w", key, err)
+		}
+
+		if !entryExists && strings.HasSuffix(key, "/") {
+			// this is a directory
+			subKeys, err := s.List(ctx, path+key)
+			if err != nil {
+				return fmt.Errorf("unable to list keys: %w", err)
+			}
+
+			// append to the keys slice to continue search in the sub-directory
+			for _, subKey := range subKeys {
+				// prevent infinite loop but this should never happen
+				if subKey == "" {
+					continue
+				}
+				subKey = fmt.Sprintf("%s%s", key, subKey)
+				keys = append(keys, subKey)
+			}
+		}
+	}
+	return nil
+}
+
 // loadManagedUsers loads users managed by the secrets engine from storage into
 // the backend's managedUsers set. Users are loaded from both the static role and
 // check-in/check-out systems. Returns an error if one occurs during loading.
-func (b *backend) loadManagedUsers(ctx context.Context, s logical.Storage) error {
+func (b *backend) loadManagedUsers(ctx context.Context, s logical.Storage) (map[string]*roleEntry, error) {
+	log := b.Logger()
 	b.managedUserLock.Lock()
 	defer b.managedUserLock.Unlock()
 
@@ -162,16 +211,26 @@ func (b *backend) loadManagedUsers(ctx context.Context, s logical.Storage) error
 	b.managedUsers = make(map[string]struct{})
 
 	// Load users managed under static roles
-	staticRoles, err := s.List(ctx, staticRolePath)
-	if err != nil {
-		return err
-	}
-	for _, roleName := range staticRoles {
-		staticRole, err := b.staticRole(ctx, s, roleName)
+	roles := map[string]*roleEntry{}
+	roleFunc := func(roleName string) (bool, error) {
+		entry, err := b.staticRole(ctx, s, roleName)
 		if err != nil {
-			return err
+			log.Warn("unable to read static role", "error", err, "role", roleName)
+			return false, err
+		}
+		entryExists := entry != nil && !strings.HasSuffix(roleName, "/")
+		if entryExists {
+			roles[roleName] = entry
 		}
-		if staticRole == nil || staticRole.StaticAccount == nil {
+		return entryExists, nil
+	}
+	err := walkStoragePath(ctx, s, staticRolePath, roleFunc)
+	if err != nil {
+		return nil, err
+	}
+
+	for roleName, role := range roles {
+		if role == nil || role.StaticAccount == nil {
 			// This indicates that a static role returned from the list operation was
 			// deleted before the read operation in this loop. This shouldn't happen
 			// at this point in the plugin lifecycle, so we'll log if it does.
@@ -181,19 +240,28 @@ func (b *backend) loadManagedUsers(ctx context.Context, s logical.Storage) error
 		}
 
 		// Add the static role user to the managed user set
-		b.managedUsers[staticRole.StaticAccount.Username] = struct{}{}
+		b.managedUsers[role.StaticAccount.Username] = struct{}{}
 	}
 
 	// Load users managed under library sets
-	librarySets, err := s.List(ctx, libraryPrefix)
-	if err != nil {
-		return err
-	}
-	for _, setName := range librarySets {
-		set, err := readSet(ctx, s, setName)
+	librarySets := map[string]*librarySet{}
+	setFunc := func(setName string) (bool, error) {
+		entry, err := readSet(ctx, s, setName)
 		if err != nil {
-			return err
+			log.Warn("unable to read library set", "error", err, "set", setName)
+			return false, err
 		}
+		entryExists := entry != nil && !strings.HasSuffix(setName, "/")
+		if entryExists {
+			librarySets[setName] = entry
+		}
+		return entryExists, nil
+	}
+	err = walkStoragePath(ctx, s, libraryPrefix, setFunc)
+	if err != nil {
+		return nil, err
+	}
+	for setName, set := range librarySets {
 		if set == nil {
 			// This indicates that a library set returned from the list operation was
 			// deleted before the read operation in this loop. This shouldn't happen
@@ -209,7 +277,7 @@ func (b *backend) loadManagedUsers(ctx context.Context, s logical.Storage) error
 		}
 	}
 
-	return nil
+	return roles, nil
 }
 
 const backendHelp = `

backend_test.go

@@ -57,7 +57,7 @@ func getBackend(throwsErr bool) (*backend, logical.Storage) {
 	// Load queue and kickoff new periodic ticker
 	b.initQueue(ictx, &logical.InitializationRequest{
 		Storage: config.StorageView,
-	})
+	}, nil)
 
 	return b, config.StorageView
 }

path_checkout_sets_test.go

@@ -129,6 +129,22 @@ func TestCheckOutHierarchicalPaths(t *testing.T) {
 		tSuiteForList = ts
 	}
 
+	// Reset the managed users to simulate startup memory state
+	b.managedUsers = make(map[string]struct{})
+	// Now finish the startup process by populating the managed users
+	b.loadManagedUsers(ctx, s)
+
+	for _, setName := range setNames {
+		ts := testSuite{
+			b:               b,
+			s:               s,
+			name:            setName,
+			svcAccountNames: getTestSvcAccountNames(setName, 2),
+		}
+
+		t.Run("write conflicting set", ts.WriteSetWithConflictingServiceAccounts())
+	}
+
 	tests := []struct {
 		path             string
 		expectedListResp []string
@@ -143,7 +159,7 @@ func TestCheckOutHierarchicalPaths(t *testing.T) {
 	}
 
 	// `LIST /library`
-	// will direct sub-keys split on "/" for the FIRST level (split) only
+	// will return direct sub-keys split on "/" for the FIRST level (split) only
 	t.Run("list library hierarchy", tSuiteForList.ListSets([]string{"foo", "org/"}))
 }
 

rotation.go

@@ -7,7 +7,6 @@ import (
 	"context"
 	"errors"
 	"fmt"
-	"strings"
 	"time"
 
 	"github.com/hashicorp/go-secure-stdlib/base62"
@@ -33,8 +32,7 @@ const (
 // rotations have been processed. It lists the roles from storage and searches
 // for any that have an associated static account, then adds them to the
 // priority queue for rotations.
-
-func (b *backend) populateQueue(ctx context.Context, s logical.Storage) {
+func (b *backend) populateQueue(ctx context.Context, s logical.Storage, roles map[string]*roleEntry) {
 	log := b.Logger()
 	log.Info("populating role rotation queue")
 
@@ -44,25 +42,14 @@ func (b *backend) populateQueue(ctx context.Context, s logical.Storage) {
 		log.Warn("unable to load rotation WALs", "error", err)
 	}
 
-	roles, err := b.getAllStaticRoles(ctx, s)
-	if err != nil {
-		log.Warn("unable to list roles for enqueueing", "error", err)
-		return
-	}
-
-	for _, roleName := range roles {
+	for roleName, role := range roles {
 		select {
 		case <-ctx.Done():
 			log.Info("rotation queue restore cancelled")
 			return
 		default:
 		}
 
-		role, err := b.staticRole(ctx, s, roleName)
-		if err != nil {
-			log.Warn("unable to read static role", "error", err, "role", roleName)
-			continue
-		}
 		if role == nil {
 			log.Error("role not found in storage", "roleName", roleName)
 			continue
@@ -452,7 +439,7 @@ func (b *backend) GeneratePassword(ctx context.Context, cfg *config) (string, er
 // not wait for success or failure of it's tasks before continuing. This is to
 // avoid blocking the mount process while loading and evaluating existing roles,
 // etc.
-func (b *backend) initQueue(ctx context.Context, conf *logical.InitializationRequest) {
+func (b *backend) initQueue(ctx context.Context, conf *logical.InitializationRequest, staticRoles map[string]*roleEntry) {
 	// Verify this mount is on the primary server, or is a local mount. If not, do
 	// not create a queue or launch a ticker. Both processing the WAL list and
 	// populating the queue are done sequentially and before launching a
@@ -464,7 +451,7 @@ func (b *backend) initQueue(ctx context.Context, conf *logical.InitializationReq
 		b.Logger().Info("initializing database rotation queue")
 
 		// Load roles and populate queue with static accounts
-		b.populateQueue(ctx, conf.Storage)
+		b.populateQueue(ctx, conf.Storage, staticRoles)
 
 		// Launch ticker
 		go b.runTicker(ctx, conf.Storage)
@@ -585,50 +572,3 @@ func (b *backend) popFromRotationQueueByKey(name string) (*queue.Item, error) {
 	}
 	return nil, queue.ErrEmpty
 }
-
-// getAllStaticRoles returns a slice of all static roles from storage
-func (b *backend) getAllStaticRoles(ctx context.Context, s logical.Storage) ([]string, error) {
-	var allRoles []string
-
-	roles, err := s.List(ctx, staticRolePath)
-	if err != nil {
-		return allRoles, fmt.Errorf("unable to list roles: %w", err)
-	}
-
-	// Roles can be defined with hierarchical paths, e.g. "foo/bar/baz". But
-	// the storage.List() call to the top-level static role path will only
-	// return the top-level keys in the hierarchy. So we perform a
-	// non-recursive breadth-first search through all the keys returned from
-	// storage.List(). If a given node points to a static role, we append to
-	// the return slice.
-	for i := 0; i < len(roles); i++ {
-		path := roles[i]
-
-		role, err := b.staticRole(ctx, s, path)
-		if err != nil {
-			return allRoles, fmt.Errorf("unable to read static role %q: %w", path, err)
-		}
-
-		if role == nil && strings.HasSuffix(path, "/") {
-			// this is a directory
-			subPaths, err := s.List(ctx, staticRolePath+path)
-			if err != nil {
-				return allRoles, fmt.Errorf("unable to list roles: %w", err)
-			}
-
-			// append to the roles slice to continue search in the sub-directory
-			for _, subPath := range subPaths {
-				// prevent infinite loop but this should never happen
-				if subPath == "" {
-					continue
-				}
-				subPath = fmt.Sprintf("%s%s", path, subPath)
-				roles = append(roles, subPath)
-			}
-		} else {
-			// a role exists at this path, append to the return slice
-			allRoles = append(allRoles, path)
-		}
-	}
-	return allRoles, nil
-}

rotation_test.go

@@ -84,10 +84,16 @@ func TestInitQueueHierarchicalPaths(t *testing.T) {
 			// 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)
 
 			queueLen := b.credRotationQueue.Len()
 			if queueLen != len(tc.roles) {
@@ -409,10 +415,16 @@ func TestStoredWALsCorrectlyProcessed(t *testing.T) {
 			// 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, which should discard the WAL
 			b.initQueue(ictx, &logical.InitializationRequest{
 				Storage: config.StorageView,
-			})
+			}, staticRoles)
 
 			if tc.shouldRotate {
 				requireWALs(t, config.StorageView, 1)