From 7b5f467d9b55857f12c82c3676667c3355b3a876 Mon Sep 17 00:00:00 2001 From: Pawel Rozlach Date: Tue, 4 Oct 2016 15:26:07 +0200 Subject: [PATCH 1/3] Add tests for nested/prefixed secrets removal. Current tests were not checking if backends are properly removing nested secrets. We follow here the behaviour of Consul backend, where empty "directories/prefixes" are automatically removed by Consul itself. --- physical/physical_test.go | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/physical/physical_test.go b/physical/physical_test.go index a2f0bc1914ae8..6b0d468f1bede 100644 --- a/physical/physical_test.go +++ b/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) From d13c10ffe335cca6f54ac43e54ef63e7941f2c7d Mon Sep 17 00:00:00 2001 From: Pawel Rozlach Date: Tue, 4 Oct 2016 15:28:48 +0200 Subject: [PATCH 2/3] Fix zookeeper backend so that properly deletes/lists secrets. This patch fixes two bugs in Zookeeper backends: * backend was determining if the node is a leaf or not basing on the number of the childer given node has. This is incorrect if you consider the fact that deleteing nested node can leave empty prefixes/dirs behind which have neither children nor data inside. The fix changes this situation by testing if the node has any data set - if not then it is not a leaf. * zookeeper does not delete nodes that do not have childern just like consul does and this leads to leaving empty nodes behind. In order to fix it, we scan the logical path of a secret being deleted for empty dirs/prefixes and remove them up until first non-empty one. --- physical/zookeeper.go | 62 +++++++++++++++++++++++++++++++++----- physical/zookeeper_test.go | 3 ++ 2 files changed, 58 insertions(+), 7 deletions(-) diff --git a/physical/zookeeper.go b/physical/zookeeper.go index 5e1e0a5fadc67..86c73319a9b5e 100644 --- a/physical/zookeeper.go +++ b/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:]) diff --git a/physical/zookeeper_test.go b/physical/zookeeper_test.go index 9c8ffc94e999a..b9969aed2781f 100644 --- a/physical/zookeeper_test.go +++ b/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) From 3156098a5bf47e402043f4c3a8c4dcd0ecb17ba7 Mon Sep 17 00:00:00 2001 From: Pawel Rozlach Date: Tue, 4 Oct 2016 15:33:54 +0200 Subject: [PATCH 3/3] Fix file backend so that it properly removes nested secrets. This patch makes file backend properly remove nested secrets, without leaving empty directory artifacts, no matter how nested directories were. --- physical/file.go | 65 ++++++++++++++++++++++++++---------------------- 1 file changed, 35 insertions(+), 30 deletions(-) diff --git a/physical/file.go b/physical/file.go index 335c75e992667..3037c06689546 100644 --- a/physical/file.go +++ b/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