Cleaning up logical and auth unmount functions (#2994)

vault/auth.go

@@ -131,7 +131,7 @@ func (c *Core) enableCredential(entry *MountEntry) error {
 
 // disableCredential is used to disable an existing credential backend; the
 // boolean indicates if it existed
-func (c *Core) disableCredential(path string) (bool, error) {
+func (c *Core) disableCredential(path string) error {
 	// Ensure we end the path in a slash
 	if !strings.HasSuffix(path, "/") {
 		path += "/"
@@ -139,29 +139,29 @@ func (c *Core) disableCredential(path string) (bool, error) {
 
 	// Ensure the token backend is not affected
 	if path == "token/" {
-		return true, fmt.Errorf("token credential backend cannot be disabled")
+		return fmt.Errorf("token credential backend cannot be disabled")
 	}
 
 	// Store the view for this backend
 	fullPath := credentialRoutePrefix + path
 	view := c.router.MatchingStorageView(fullPath)
 	if view == nil {
-		return false, fmt.Errorf("no matching backend %s", fullPath)
+		return fmt.Errorf("no matching backend %s", fullPath)
 	}
 
 	// Mark the entry as tainted
 	if err := c.taintCredEntry(path); err != nil {
-		return true, err
+		return err
 	}
 
 	// Taint the router path to prevent routing
 	if err := c.router.Taint(fullPath); err != nil {
-		return true, err
+		return err
 	}
 
 	// Revoke credentials from this path
 	if err := c.expiration.RevokePrefix(fullPath); err != nil {
-		return true, err
+		return err
 	}
 
 	// Call cleanup function if it exists
@@ -172,24 +172,24 @@ func (c *Core) disableCredential(path string) (bool, error) {
 
 	// Unmount the backend
 	if err := c.router.Unmount(fullPath); err != nil {
-		return true, err
+		return err
 	}
 
 	// Clear the data in the view
 	if view != nil {
 		if err := logical.ClearView(view); err != nil {
-			return true, err
+			return err
 		}
 	}
 
 	// Remove the mount table entry
 	if err := c.removeCredEntry(path); err != nil {
-		return true, err
+		return err
 	}
 	if c.logger.IsInfo() {
 		c.logger.Info("core: disabled credential backend", "path", path)
 	}
-	return true, nil
+	return nil
 }
 
 // removeCredEntry is used to remove an entry in the auth table

vault/auth_test.go

@@ -217,9 +217,9 @@ func TestCore_DisableCredential(t *testing.T) {
 		return &NoopBackend{}, nil
 	}
 
-	existed, err := c.disableCredential("foo")
+	err := c.disableCredential("foo")
-	if existed || (err != nil && !strings.HasPrefix(err.Error(), "no matching backend")) {
+	if err != nil && !strings.HasPrefix(err.Error(), "no matching backend") {
-		t.Fatalf("existed: %v; err: %v", existed, err)
+		t.Fatalf("err: %v", err)
 	}
 
 	me := &MountEntry{
@@ -232,9 +232,9 @@ func TestCore_DisableCredential(t *testing.T) {
 		t.Fatalf("err: %v", err)
 	}
 
-	existed, err = c.disableCredential("foo")
+	err = c.disableCredential("foo")
-	if !existed || err != nil {
+	if err != nil {
-		t.Fatalf("existed: %v; err: %v", existed, err)
+		t.Fatalf("err: %v", err)
 	}
 
 	match := c.router.MatchingMount("auth/foo/bar")
@@ -268,9 +268,9 @@ func TestCore_DisableCredential(t *testing.T) {
 
 func TestCore_DisableCredential_Protected(t *testing.T) {
 	c, _, _ := TestCoreUnsealed(t)
-	existed, err := c.disableCredential("token")
+	err := c.disableCredential("token")
-	if !existed || err.Error() != "token credential backend cannot be disabled" {
+	if err.Error() != "token credential backend cannot be disabled" {
-		t.Fatalf("existed: %v; err: %v", existed, err)
+		t.Fatalf("err: %v", err)
 	}
 }
 
@@ -324,9 +324,9 @@ func TestCore_DisableCredential_Cleanup(t *testing.T) {
 	}
 
 	// Disable should cleanup
-	existed, err := c.disableCredential("foo")
+	err = c.disableCredential("foo")
-	if !existed || err != nil {
+	if err != nil {
-		t.Fatalf("existed: %v; err: %v", existed, err)
+		t.Fatalf("err: %v", err)
 	}
 
 	// Token should be revoked

vault/logical_system.go

@@ -1293,25 +1293,25 @@ func handleError(
 // handleUnmount is used to unmount a path
 func (b *SystemBackend) handleUnmount(
 	req *logical.Request, data *framework.FieldData) (*logical.Response, error) {
-	b.Core.clusterParamsLock.RLock()
+	path := data.Get("path").(string)
-	repState := b.Core.replicationState
+	path = sanitizeMountPath(path)
-	b.Core.clusterParamsLock.RUnlock()
 
-	suffix := strings.TrimPrefix(req.Path, "mounts/")
+	repState := b.Core.ReplicationState()
-	if len(suffix) == 0 {
+	entry := b.Core.router.MatchingMountEntry(path)
-		return logical.ErrorResponse("path cannot be blank"), logical.ErrInvalidRequest
-	}
-
-	suffix = sanitizeMountPath(suffix)
-
-	entry := b.Core.router.MatchingMountEntry(suffix)
 	if entry != nil && !entry.Local && repState == consts.ReplicationSecondary {
 		return logical.ErrorResponse("cannot unmount a non-local mount on a replication secondary"), nil
 	}
 
+	// We return success when the mount does not exists to not expose if the
+	// mount existed or not
+	match := b.Core.router.MatchingMount(path)
+	if match == "" || path != match {
+		return nil, nil
+	}
+
 	// Attempt unmount
-	if existed, err := b.Core.unmount(suffix); existed && err != nil {
+	if err := b.Core.unmount(path); err != nil {
-		b.Backend.Logger().Error("sys: unmount failed", "path", suffix, "error", err)
+		b.Backend.Logger().Error("sys: unmount failed", "path", path, "error", err)
 		return handleError(err)
 	}
 
@@ -1714,16 +1714,26 @@ func (b *SystemBackend) handleEnableAuth(
 // handleDisableAuth is used to disable a credential backend
 func (b *SystemBackend) handleDisableAuth(
 	req *logical.Request, data *framework.FieldData) (*logical.Response, error) {
-	suffix := strings.TrimPrefix(req.Path, "auth/")
+	path := data.Get("path").(string)
-	if len(suffix) == 0 {
+	path = sanitizeMountPath(path)
-		return logical.ErrorResponse("path cannot be blank"), logical.ErrInvalidRequest
+	fullPath := credentialRoutePrefix + path
+
+	repState := b.Core.ReplicationState()
+	entry := b.Core.router.MatchingMountEntry(fullPath)
+	if entry != nil && !entry.Local && repState == consts.ReplicationSecondary {
+		return logical.ErrorResponse("cannot unmount a non-local mount on a replication secondary"), nil
 	}
 
-	suffix = sanitizeMountPath(suffix)
+	// We return success when the mount does not exists to not expose if the
+	// mount existed or not
+	match := b.Core.router.MatchingMount(fullPath)
+	if match == "" || fullPath != match {
+		return nil, nil
+	}
 
 	// Attempt disable
-	if existed, err := b.Core.disableCredential(suffix); existed && err != nil {
+	if err := b.Core.disableCredential(path); err != nil {
-		b.Backend.Logger().Error("sys: disable auth mount failed", "path", suffix, "error", err)
+		b.Backend.Logger().Error("sys: disable auth mount failed", "path", path, "error", err)
 		return handleError(err)
 	}
 	return nil, nil

vault/mount.go

@@ -169,26 +169,6 @@ type MountConfig struct {
 	ForceNoCache    bool          `json:"force_no_cache" structs:"force_no_cache" mapstructure:"force_no_cache"`          // Override for global default
 }
 
-// Returns a deep copy of the mount entry
-func (e *MountEntry) Clone() *MountEntry {
-	optClone := make(map[string]string)
-	for k, v := range e.Options {
-		optClone[k] = v
-	}
-	return &MountEntry{
-		Table:       e.Table,
-		Path:        e.Path,
-		Type:        e.Type,
-		Description: e.Description,
-		UUID:        e.UUID,
-		Accessor:    e.Accessor,
-		Config:      e.Config,
-		Options:     optClone,
-		Local:       e.Local,
-		Tainted:     e.Tainted,
-	}
-}
-
 // Mount is used to mount a new backend to the mount table.
 func (c *Core) mount(entry *MountEntry) error {
 	// Ensure we end the path in a slash
@@ -271,7 +251,7 @@ func (c *Core) mount(entry *MountEntry) error {
 
 // Unmount is used to unmount a path. The boolean indicates whether the mount
 // was found.
-func (c *Core) unmount(path string) (bool, error) {
+func (c *Core) unmount(path string) error {
 	// Ensure we end the path in a slash
 	if !strings.HasSuffix(path, "/") {
 		path += "/"
@@ -280,14 +260,14 @@ func (c *Core) unmount(path string) (bool, error) {
 	// Prevent protected paths from being unmounted
 	for _, p := range protectedMounts {
 		if strings.HasPrefix(path, p) {
-			return true, fmt.Errorf("cannot unmount '%s'", path)
+			return fmt.Errorf("cannot unmount '%s'", path)
 		}
 	}
 
 	// Verify exact match of the route
 	match := c.router.MatchingMount(path)
 	if match == "" || path != match {
-		return false, fmt.Errorf("no matching mount")
+		return fmt.Errorf("no matching mount")
 	}
 
 	// Get the view for this backend
@@ -295,23 +275,23 @@ func (c *Core) unmount(path string) (bool, error) {
 
 	// Mark the entry as tainted
 	if err := c.taintMountEntry(path); err != nil {
-		return true, err
+		return err
 	}
 
 	// Taint the router path to prevent routing. Note that in-flight requests
 	// are uncertain, right now.
 	if err := c.router.Taint(path); err != nil {
-		return true, err
+		return err
 	}
 
 	// Invoke the rollback manager a final time
 	if err := c.rollback.Rollback(path); err != nil {
-		return true, err
+		return err
 	}
 
 	// Revoke all the dynamic keys
 	if err := c.expiration.RevokePrefix(path); err != nil {
-		return true, err
+		return err
 	}
 
 	// Call cleanup function if it exists
@@ -322,22 +302,22 @@ func (c *Core) unmount(path string) (bool, error) {
 
 	// Unmount the backend entirely
 	if err := c.router.Unmount(path); err != nil {
-		return true, err
+		return err
 	}
 
 	// Clear the data in the view
 	if err := logical.ClearView(view); err != nil {
-		return true, err
+		return err
 	}
 
 	// Remove the mount table entry
 	if err := c.removeMountEntry(path); err != nil {
-		return true, err
+		return err
 	}
 	if c.logger.IsInfo() {
 		c.logger.Info("core: successfully unmounted", "path", path)
 	}
-	return true, nil
+	return nil
 }
 
 // removeMountEntry is used to remove an entry from the mount table

vault/mount_test.go

@@ -181,9 +181,9 @@ func TestCore_Mount_Local(t *testing.T) {
 
 func TestCore_Unmount(t *testing.T) {
 	c, keys, _ := TestCoreUnsealed(t)
-	existed, err := c.unmount("secret")
+	err := c.unmount("secret")
-	if !existed || err != nil {
+	if err != nil {
-		t.Fatalf("existed: %v; err: %v", existed, err)
+		t.Fatalf("err: %v", err)
 	}
 
 	match := c.router.MatchingMount("secret/foo")
@@ -272,8 +272,8 @@ func TestCore_Unmount_Cleanup(t *testing.T) {
 	}
 
 	// Unmount, this should cleanup
-	if existed, err := c.unmount("test/"); !existed || err != nil {
+	if err := c.unmount("test/"); err != nil {
-		t.Fatalf("existed: %v; err: %v", existed, err)
+		t.Fatalf("err: %v", err)
 	}
 
 	// Rollback should be invoked