Merge pull request #1964 from mesosphere/prozlach/nested_secrets_hand…

…ling_fix

Nested secrets handling fix for zookeeper and file based backend.

physical/file.go

@@ -6,6 +6,7 @@ import (
 	"io"
 	"os"
 	"path/filepath"
+	"strings"
 	"sync"
 
 	log "github.com/mgutz/logxi/v1"
@@ -39,48 +40,52 @@ func newFileBackend(conf map[string]string, logger log.Logger) (Backend, error)
 	}, nil
 }
 
-func (b *FileBackend) Delete(k string) error {
+func (b *FileBackend) Delete(path string) error {
 	b.l.Lock()
 	defer b.l.Unlock()
 
-	path, key := b.path(k)
-	fullPath := filepath.Join(path, key)
+	basePath, key := b.path(path)
+	fullPath := filepath.Join(basePath, key)
 
-	// If the path doesn't exist return success; this is in line with Vault's
-	// expected behavior and we don't want to check for an empty directory if
-	// we couldn't even find the path in the first place.
 	err := os.Remove(fullPath)
-	if err != nil {
-		if os.IsNotExist(err) {
-			return nil
-		} else {
-			return err
-		}
+	if err != nil && !os.IsNotExist(err) {
+		return fmt.Errorf("Failed to remove `%s`: %v", fullPath, err)
 	}
 
-	// Check for the directory being empty and remove if so, with another
-	// additional guard for the path not existing
-	dir, err := os.Open(path)
-	if err != nil {
-		if os.IsNotExist(err) {
-			return nil
-		} else {
-			return err
-		}
-	}
+	err = b.cleanupLogicalPath(path)
 
-	list, err := dir.Readdir(1)
-	dir.Close()
-	if err != nil && err != io.EOF {
-		return err
-	}
+	return err
+}
 
-	// If we have no entries, it's an empty directory; remove it
-	if err == io.EOF || list == nil || len(list) == 0 {
-		err = os.Remove(path)
+// cleanupLogicalPath is used to remove all empty nodes, begining with deepest
+// one, aborting on first non-empty one, up to top-level node.
+func (b *FileBackend) cleanupLogicalPath(path string) error {
+	nodes := strings.Split(path, "/")
+	for i := len(nodes) - 1; i > 0; i-- {
+		fullPath := b.Path + "/" + strings.Join(nodes[:i], "/")
+
+		dir, err := os.Open(fullPath)
 		if err != nil {
+			if os.IsNotExist(err) {
+				return nil
+			} else {
+				return err
+			}
+		}
+
+		list, err := dir.Readdir(1)
+		dir.Close()
+		if err != nil && err != io.EOF {
 			return err
 		}
+
+		// If we have no entries, it's an empty directory; remove it
+		if err == io.EOF || list == nil || len(list) == 0 {
+			err = os.Remove(fullPath)
+			if err != nil {
+				return err
+			}
+		}
 	}
 
 	return nil

physical/physical_test.go

@@ -151,6 +151,32 @@ func testBackend(t *testing.T, b Backend) {
 		t.Fatalf("missing child")
 	}
 
+	// Removal of nested secret should not leave artifacts
+	e = &Entry{Key: "foo/nested1/nested2/nested3", Value: []byte("baz")}
+	err = b.Put(e)
+	if err != nil {
+		t.Fatalf("err: %v", err)
+	}
+
+	err = b.Delete("foo/nested1/nested2/nested3")
+	if err != nil {
+		t.Fatalf("failed to remove nested secret: %v", err)
+	}
+
+	keys, err = b.List("foo/")
+	if err != nil {
+		t.Fatalf("err: %v", err)
+	}
+
+	if len(keys) != 1 {
+		t.Fatalf("there should be only one key left after deleting nested "+
+			"secret: %v", keys)
+	}
+
+	if keys[0] != "bar" {
+		t.Fatalf("bad keys after deleting nested: %v", keys)
+	}
+
 	// Make a second nested entry to test prefix removal
 	e = &Entry{Key: "foo/zip", Value: []byte("zap")}
 	err = b.Put(e)

physical/zookeeper.go

@@ -10,7 +10,7 @@ import (
 
 	log "github.com/mgutz/logxi/v1"
 
-	"github.com/armon/go-metrics"
+	metrics "github.com/armon/go-metrics"
 	"github.com/samuel/go-zookeeper/zk"
 )
 
@@ -159,7 +159,39 @@ func (c *ZookeeperBackend) ensurePath(path string, value []byte) error {
 	return nil
 }
 
-// nodePath returns an etcd filepath based on the given key.
+// cleanupLogicalPath is used to remove all empty nodes, begining with deepest one,
+// aborting on first non-empty one, up to top-level node.
+func (c *ZookeeperBackend) cleanupLogicalPath(path string) error {
+	nodes := strings.Split(path, "/")
+	for i := len(nodes) - 1; i > 0; i-- {
+		fullPath := c.path + strings.Join(nodes[:i], "/")
+
+		_, stat, err := c.client.Exists(fullPath)
+		if err != nil {
+			return fmt.Errorf("Failed to acquire node data: %s", err)
+		}
+
+		if stat.DataLength > 0 && stat.NumChildren > 0 {
+			msgFmt := "Node %s is both of data and leaf type ??"
+			panic(fmt.Sprintf(msgFmt, fullPath))
+		} else if stat.DataLength > 0 {
+			msgFmt := "Node %s is a data node, this is either a bug or " +
+				"backend data is corrupted"
+			panic(fmt.Sprintf(msgFmt, fullPath))
+		} else if stat.NumChildren > 0 {
+			return nil
+		} else {
+			// Empty node, lets clean it up!
+			if err := c.client.Delete(fullPath, -1); err != nil {
+				msgFmt := "Removal of node `%s` failed: `%v`"
+				return fmt.Errorf(msgFmt, fullPath, err)
+			}
+		}
+	}
+	return nil
+}
+
+// nodePath returns an zk path based on the given key.
 func (c *ZookeeperBackend) nodePath(key string) string {
 	return filepath.Join(c.path, filepath.Dir(key), ZKNodeFilePrefix+filepath.Base(key))
 }
@@ -215,9 +247,12 @@ func (c *ZookeeperBackend) Delete(key string) error {
 	err := c.client.Delete(fullPath, -1)
 
 	// Mask if the node does not exist
-	if err == zk.ErrNoNode {
-		err = nil
+	if err != nil && err != zk.ErrNoNode {
+		return fmt.Errorf("Failed to remove `%s`: %v", fullPath, err)
 	}
+
+	err = c.cleanupLogicalPath(key)
+
 	return err
 }
 
@@ -233,15 +268,28 @@ func (c *ZookeeperBackend) List(prefix string) ([]string, error) {
 	// If the path nodes are missing, no children!
 	if err == zk.ErrNoNode {
 		return []string{}, nil
+	} else if err != nil {
+		return []string{}, err
 	}
 
 	children := []string{}
 	for _, key := range result {
-		// Check if this entry has any child entries,
+		childPath := fullPath + "/" + key
+		_, stat, err := c.client.Exists(childPath)
+		if err != nil {
+			// Node is ought to exists, so it must be something different
+			return []string{}, err
+		}
+
+		// Check if this entry is a leaf of a node,
 		// and append the slash which is what Vault depends on
 		// for iteration
-		nodeChildren, _, _ := c.client.Children(fullPath + "/" + key)
-		if nodeChildren != nil && len(nodeChildren) > 0 {
+		if stat.DataLength > 0 && stat.NumChildren > 0 {
+			msgFmt := "Node %s is both of data and leaf type ??"
+			panic(fmt.Sprintf(msgFmt, childPath))
+		} else if stat.DataLength == 0 {
+			// No, we cannot differentiate here on number of children as node
+			// can have all it leafs remoed, and it still is a node.
 			children = append(children, key+"/")
 		} else {
 			children = append(children, key[1:])

physical/zookeeper_test.go

@@ -33,6 +33,9 @@ func TestZookeeperBackend(t *testing.T) {
 	}
 
 	defer func() {
+		client.Delete(randPath+"/foo/nested1/nested2/nested3", -1)
+		client.Delete(randPath+"/foo/nested1/nested2", -1)
+		client.Delete(randPath+"/foo/nested1", -1)
 		client.Delete(randPath+"/foo/bar/baz", -1)
 		client.Delete(randPath+"/foo/bar", -1)
 		client.Delete(randPath+"/foo", -1)