Add an idle timeout for the server

builtin/credential/approle/path_tidy_user_id.go

@@ -26,141 +26,153 @@ func pathTidySecretID(b *backend) *framework.Path {
 }
 
 // tidySecretID is used to delete entries in the whitelist that are expired.
-func (b *backend) tidySecretID(ctx context.Context, s logical.Storage) error {
-	grabbed := atomic.CompareAndSwapUint32(b.tidySecretIDCASGuard, 0, 1)
-	if grabbed {
-		defer atomic.StoreUint32(b.tidySecretIDCASGuard, 0)
-	} else {
-		return fmt.Errorf("SecretID tidy operation already running")
+func (b *backend) tidySecretID(ctx context.Context, s logical.Storage) (*logical.Response, error) {
+	if !atomic.CompareAndSwapUint32(b.tidySecretIDCASGuard, 0, 1) {
+		resp := &logical.Response{}
+		resp.AddWarning("Tidy operation already in progress.")
+		return resp, nil
 	}
 
-	var result error
+	go func() {
+		defer atomic.StoreUint32(b.tidySecretIDCASGuard, 0)
 
-	tidyFunc := func(secretIDPrefixToUse, accessorIDPrefixToUse string) error {
-		roleNameHMACs, err := s.List(ctx, secretIDPrefixToUse)
-		if err != nil {
-			return err
-		}
+		var result error
 
-		// List all the accessors and add them all to a map
-		accessorHashes, err := s.List(ctx, accessorIDPrefixToUse)
-		if err != nil {
-			return err
-		}
-		accessorMap := make(map[string]bool, len(accessorHashes))
-		for _, accessorHash := range accessorHashes {
-			accessorMap[accessorHash] = true
-		}
+		// Don't cancel when the original client request goes away
+		ctx = context.Background()
 
-		secretIDCleanupFunc := func(secretIDHMAC, roleNameHMAC, secretIDPrefixToUse string) error {
-			lock := b.secretIDLock(secretIDHMAC)
-			lock.Lock()
-			defer lock.Unlock()
+		logger := b.Logger().Named("tidy")
 
-			entryIndex := fmt.Sprintf("%s%s%s", secretIDPrefixToUse, roleNameHMAC, secretIDHMAC)
-			secretIDEntry, err := s.Get(ctx, entryIndex)
+		tidyFunc := func(secretIDPrefixToUse, accessorIDPrefixToUse string) error {
+			roleNameHMACs, err := s.List(ctx, secretIDPrefixToUse)
 			if err != nil {
-				return errwrap.Wrapf(fmt.Sprintf("error fetching SecretID %q: {{err}}", secretIDHMAC), err)
+				return err
 			}
 
-			if secretIDEntry == nil {
-				result = multierror.Append(result, fmt.Errorf("entry for SecretID %q is nil", secretIDHMAC))
-				return nil
+			// List all the accessors and add them all to a map
+			accessorHashes, err := s.List(ctx, accessorIDPrefixToUse)
+			if err != nil {
+				return err
 			}
-
-			if secretIDEntry.Value == nil || len(secretIDEntry.Value) == 0 {
-				return fmt.Errorf("found entry for SecretID %q but actual SecretID is empty", secretIDHMAC)
+			accessorMap := make(map[string]bool, len(accessorHashes))
+			for _, accessorHash := range accessorHashes {
+				accessorMap[accessorHash] = true
 			}
 
-			var result secretIDStorageEntry
-			if err := secretIDEntry.DecodeJSON(&result); err != nil {
-				return err
-			}
+			secretIDCleanupFunc := func(secretIDHMAC, roleNameHMAC, secretIDPrefixToUse string) error {
+				lock := b.secretIDLock(secretIDHMAC)
+				lock.Lock()
+				defer lock.Unlock()
 
-			// If a secret ID entry does not have a corresponding accessor
-			// entry, revoke the secret ID immediately
-			accessorEntry, err := b.secretIDAccessorEntry(ctx, s, result.SecretIDAccessor, secretIDPrefixToUse)
-			if err != nil {
-				return errwrap.Wrapf("failed to read secret ID accessor entry: {{err}}", err)
-			}
-			if accessorEntry == nil {
-				if err := s.Delete(ctx, entryIndex); err != nil {
-					return errwrap.Wrapf(fmt.Sprintf("error deleting secret ID %q from storage: {{err}}", secretIDHMAC), err)
+				entryIndex := fmt.Sprintf("%s%s%s", secretIDPrefixToUse, roleNameHMAC, secretIDHMAC)
+				secretIDEntry, err := s.Get(ctx, entryIndex)
+				if err != nil {
+					return errwrap.Wrapf(fmt.Sprintf("error fetching SecretID %q: {{err}}", secretIDHMAC), err)
+				}
+
+				if secretIDEntry == nil {
+					result = multierror.Append(result, fmt.Errorf("entry for SecretID %q is nil", secretIDHMAC))
+					return nil
+				}
+
+				if secretIDEntry.Value == nil || len(secretIDEntry.Value) == 0 {
+					return fmt.Errorf("found entry for SecretID %q but actual SecretID is empty", secretIDHMAC)
+				}
+
+				var result secretIDStorageEntry
+				if err := secretIDEntry.DecodeJSON(&result); err != nil {
+					return err
 				}
-				return nil
-			}
 
-			// ExpirationTime not being set indicates non-expiring SecretIDs
-			if !result.ExpirationTime.IsZero() && time.Now().After(result.ExpirationTime) {
-				// Clean up the accessor of the secret ID first
-				err = b.deleteSecretIDAccessorEntry(ctx, s, result.SecretIDAccessor, secretIDPrefixToUse)
+				// If a secret ID entry does not have a corresponding accessor
+				// entry, revoke the secret ID immediately
+				accessorEntry, err := b.secretIDAccessorEntry(ctx, s, result.SecretIDAccessor, secretIDPrefixToUse)
 				if err != nil {
-					return errwrap.Wrapf("failed to delete secret ID accessor entry: {{err}}", err)
+					return errwrap.Wrapf("failed to read secret ID accessor entry: {{err}}", err)
+				}
+				if accessorEntry == nil {
+					if err := s.Delete(ctx, entryIndex); err != nil {
+						return errwrap.Wrapf(fmt.Sprintf("error deleting secret ID %q from storage: {{err}}", secretIDHMAC), err)
+					}
+					return nil
+				}
+
+				// ExpirationTime not being set indicates non-expiring SecretIDs
+				if !result.ExpirationTime.IsZero() && time.Now().After(result.ExpirationTime) {
+					// Clean up the accessor of the secret ID first
+					err = b.deleteSecretIDAccessorEntry(ctx, s, result.SecretIDAccessor, secretIDPrefixToUse)
+					if err != nil {
+						return errwrap.Wrapf("failed to delete secret ID accessor entry: {{err}}", err)
+					}
+
+					if err := s.Delete(ctx, entryIndex); err != nil {
+						return errwrap.Wrapf(fmt.Sprintf("error deleting SecretID %q from storage: {{err}}", secretIDHMAC), err)
+					}
+
+					return nil
 				}
 
-				if err := s.Delete(ctx, entryIndex); err != nil {
-					return errwrap.Wrapf(fmt.Sprintf("error deleting SecretID %q from storage: {{err}}", secretIDHMAC), err)
+				// At this point, the secret ID is not expired and is valid. Delete
+				// the corresponding accessor from the accessorMap. This will leave
+				// only the dangling accessors in the map which can then be cleaned
+				// up later.
+				salt, err := b.Salt(ctx)
+				if err != nil {
+					return err
 				}
+				delete(accessorMap, salt.SaltID(result.SecretIDAccessor))
 
 				return nil
 			}
 
-			// At this point, the secret ID is not expired and is valid. Delete
-			// the corresponding accessor from the accessorMap. This will leave
-			// only the dangling accessors in the map which can then be cleaned
-			// up later.
-			salt, err := b.Salt(ctx)
-			if err != nil {
-				return err
+			for _, roleNameHMAC := range roleNameHMACs {
+				secretIDHMACs, err := s.List(ctx, fmt.Sprintf("%s%s", secretIDPrefixToUse, roleNameHMAC))
+				if err != nil {
+					return err
+				}
+				for _, secretIDHMAC := range secretIDHMACs {
+					err = secretIDCleanupFunc(secretIDHMAC, roleNameHMAC, secretIDPrefixToUse)
+					if err != nil {
+						return err
+					}
+				}
 			}
-			delete(accessorMap, salt.SaltID(result.SecretIDAccessor))
 
-			return nil
-		}
-
-		for _, roleNameHMAC := range roleNameHMACs {
-			secretIDHMACs, err := s.List(ctx, fmt.Sprintf("%s%s", secretIDPrefixToUse, roleNameHMAC))
-			if err != nil {
-				return err
-			}
-			for _, secretIDHMAC := range secretIDHMACs {
-				err = secretIDCleanupFunc(secretIDHMAC, roleNameHMAC, secretIDPrefixToUse)
+			// Accessor indexes were not getting cleaned up until 0.9.3. This is a fix
+			// to clean up the dangling accessor entries.
+			for accessorHash, _ := range accessorMap {
+				// Ideally, locking should be performed here. But for that, accessors
+				// are required in plaintext, which are not available. Hence performing
+				// a racy cleanup.
+				err = s.Delete(ctx, secretIDAccessorPrefix+accessorHash)
 				if err != nil {
 					return err
 				}
 			}
-		}
 
-		// Accessor indexes were not getting cleaned up until 0.9.3. This is a fix
-		// to clean up the dangling accessor entries.
-		for accessorHash, _ := range accessorMap {
-			// Ideally, locking should be performed here. But for that, accessors
-			// are required in plaintext, which are not available. Hence performing
-			// a racy cleanup.
-			err = s.Delete(ctx, secretIDAccessorPrefix+accessorHash)
-			if err != nil {
-				return err
-			}
+			return nil
 		}
 
-		return nil
-	}
-
-	err := tidyFunc(secretIDPrefix, secretIDAccessorPrefix)
-	if err != nil {
-		return err
-	}
-	err = tidyFunc(secretIDLocalPrefix, secretIDAccessorLocalPrefix)
-	if err != nil {
-		return err
-	}
+		err := tidyFunc(secretIDPrefix, secretIDAccessorPrefix)
+		if err != nil {
+			logger.Error("error tidying global secret IDs", "error", err)
+			return
+		}
+		err = tidyFunc(secretIDLocalPrefix, secretIDAccessorLocalPrefix)
+		if err != nil {
+			logger.Error("error tidying local secret IDs", "error", err)
+			return
+		}
+	}()
 
-	return result
+	resp := &logical.Response{}
+	resp.AddWarning("Tidy operation successfully started. Any information from the operation will be printed to Vault's server logs.")
+	return resp, nil
 }
 
 // pathTidySecretIDUpdate is used to delete the expired SecretID entries
 func (b *backend) pathTidySecretIDUpdate(ctx context.Context, req *logical.Request, data *framework.FieldData) (*logical.Response, error) {
-	return nil, b.tidySecretID(ctx, req.Storage)
+	return b.tidySecretID(ctx, req.Storage)
 }
 
 const pathTidySecretIDSyn = "Trigger the clean-up of expired SecretID entries."

builtin/credential/approle/path_tidy_user_id_test.go

@@ -3,6 +3,7 @@ package approle
 import (
 	"context"
 	"testing"
+	"time"
 
 	"github.com/hashicorp/vault/logical"
 )
@@ -64,11 +65,14 @@ func TestAppRole_TidyDanglingAccessors(t *testing.T) {
 		t.Fatalf("bad: len(accessorHashes); expect 3, got %d", len(accessorHashes))
 	}
 
-	err = b.tidySecretID(context.Background(), storage)
+	_, err = b.tidySecretID(context.Background(), storage)
 	if err != nil {
 		t.Fatal(err)
 	}
 
+	// It runs async so we give it a bit of time to run
+	time.Sleep(10 * time.Second)
+
 	accessorHashes, err = storage.List(context.Background(), "accessor/")
 	if err != nil {
 		t.Fatal(err)

builtin/credential/aws/path_tidy_identity_whitelist.go

@@ -33,53 +33,72 @@ expiration, before it is removed from the backend storage.`,
 }
 
 // tidyWhitelistIdentity is used to delete entries in the whitelist that are expired.
-func (b *backend) tidyWhitelistIdentity(ctx context.Context, s logical.Storage, safety_buffer int) error {
-	grabbed := atomic.CompareAndSwapUint32(b.tidyWhitelistCASGuard, 0, 1)
-	if grabbed {
-		defer atomic.StoreUint32(b.tidyWhitelistCASGuard, 0)
-	} else {
-		return fmt.Errorf("identity whitelist tidy operation already running")
+func (b *backend) tidyWhitelistIdentity(ctx context.Context, s logical.Storage, safety_buffer int) (*logical.Response, error) {
+	if !atomic.CompareAndSwapUint32(b.tidyWhitelistCASGuard, 0, 1) {
+		resp := &logical.Response{}
+		resp.AddWarning("Tidy operation already in progress.")
+		return resp, nil
 	}
 
-	bufferDuration := time.Duration(safety_buffer) * time.Second
+	go func() {
+		defer atomic.StoreUint32(b.tidyWhitelistCASGuard, 0)
 
-	identities, err := s.List(ctx, "whitelist/identity/")
-	if err != nil {
-		return err
-	}
+		// Don't cancel when the original client request goes away
+		ctx = context.Background()
 
-	for _, instanceID := range identities {
-		identityEntry, err := s.Get(ctx, "whitelist/identity/"+instanceID)
-		if err != nil {
-			return errwrap.Wrapf(fmt.Sprintf("error fetching identity of instanceID %q: {{err}}", instanceID), err)
-		}
+		logger := b.Logger().Named("wltidy")
 
-		if identityEntry == nil {
-			return fmt.Errorf("identity entry for instanceID %q is nil", instanceID)
-		}
+		bufferDuration := time.Duration(safety_buffer) * time.Second
 
-		if identityEntry.Value == nil || len(identityEntry.Value) == 0 {
-			return fmt.Errorf("found identity entry for instanceID %q but actual identity is empty", instanceID)
-		}
+		doTidy := func() error {
+			identities, err := s.List(ctx, "whitelist/identity/")
+			if err != nil {
+				return err
+			}
+
+			for _, instanceID := range identities {
+				identityEntry, err := s.Get(ctx, "whitelist/identity/"+instanceID)
+				if err != nil {
+					return errwrap.Wrapf(fmt.Sprintf("error fetching identity of instanceID %q: {{err}}", instanceID), err)
+				}
+
+				if identityEntry == nil {
+					return fmt.Errorf("identity entry for instanceID %q is nil", instanceID)
+				}
+
+				if identityEntry.Value == nil || len(identityEntry.Value) == 0 {
+					return fmt.Errorf("found identity entry for instanceID %q but actual identity is empty", instanceID)
+				}
+
+				var result whitelistIdentity
+				if err := identityEntry.DecodeJSON(&result); err != nil {
+					return err
+				}
+
+				if time.Now().After(result.ExpirationTime.Add(bufferDuration)) {
+					if err := s.Delete(ctx, "whitelist/identity"+instanceID); err != nil {
+						return errwrap.Wrapf(fmt.Sprintf("error deleting identity of instanceID %q from storage: {{err}}", instanceID), err)
+					}
+				}
+			}
 
-		var result whitelistIdentity
-		if err := identityEntry.DecodeJSON(&result); err != nil {
-			return err
+			return nil
 		}
 
-		if time.Now().After(result.ExpirationTime.Add(bufferDuration)) {
-			if err := s.Delete(ctx, "whitelist/identity"+instanceID); err != nil {
-				return errwrap.Wrapf(fmt.Sprintf("error deleting identity of instanceID %q from storage: {{err}}", instanceID), err)
-			}
+		if err := doTidy(); err != nil {
+			logger.Error("error running whitelist tidy", "error", err)
+			return
 		}
-	}
+	}()
 
-	return nil
+	resp := &logical.Response{}
+	resp.AddWarning("Tidy operation successfully started. Any information from the operation will be printed to Vault's server logs.")
+	return resp, nil
 }
 
 // pathTidyIdentityWhitelistUpdate is used to delete entries in the whitelist that are expired.
 func (b *backend) pathTidyIdentityWhitelistUpdate(ctx context.Context, req *logical.Request, data *framework.FieldData) (*logical.Response, error) {
-	return nil, b.tidyWhitelistIdentity(ctx, req.Storage, data.Get("safety_buffer").(int))
+	return b.tidyWhitelistIdentity(ctx, req.Storage, data.Get("safety_buffer").(int))
 }
 
 const pathTidyIdentityWhitelistSyn = `