Nested secrets handling fix for zookeeper and file based backend. #1964

vespian · 2016-10-04T15:20:38Z

Hi!

Some time ago we discovered that nested secrets (e.g. "foo/bar/baz") are inproperly handled in zookeeper backend, after some research it turned out that:

zookeeper's backend List method improperly calssifies nodes, basing only on the number of children given node has. This was causing "ghost" entries in List call results for ZK nodes which were not removed when all their children were
the reason for for not removing empty node is that Zookeeper was based on officially supported consul backend. Consul takes care of removing "empty" nodes itself, without the need of extra logic in the code.

While rebasing, I have noticed that there was already some patching in file based backend (https://github.com/hashicorp/vault/pull/1821/files). Unfortunately neither the fix nor the extra tests are not recursive. Lets assume that we have following hierarchy:

foo
  |- suz -> val=="ffz"

now let's create secret foo/bar/baz/gaz/naz with value test1234, elements bar, baz, gaz are empty, foo/suz and naz hold a secret. We have following hierarchy now:

foo
  |- suz -> val=="ffz"
  |- bar
      |- baz
         |- gaz
            |- naz -> val == `test1234`

Removing foo/bar/baz/gaz/naz will result in directories bar, baz, gaz left on the disk. In case of the Zookeeper, user would get additional entry of foo/bar/baz/az in the result of List call (first character is stripped as the backend thinks this is a data node with _ prefix).

So this PR refactors tests in order to cover mentioned corner-cases and adds recursive deletion of directoires/nodes that are empty. It has been verified/tested on Zookeeper, Consul and File backend. I am not sure if other backends do not share this bug (i.e. PSQL, MySQL, ETCd, S3, etc...).

Thanks in advance for feedback.
pr

jefferai · 2016-10-04T18:06:47Z

physical/physical_test.go

@@ -151,43 +151,31 @@ func testBackend(t *testing.T, b Backend) {
 		t.Fatalf("missing child")
 	}

-	// Make a second nested entry to test prefix removal


Hi,

This looks pretty good but one request: you changed the tests here but actually took out some functionality. The previous version was testing that removing one key from a prefix with two keys under it would not remove the prefix. You should add your test in addition to that, not instead of.

Fixed. Please have another look.

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.

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.

This patch makes file backend properly remove nested secrets, without leaving empty directory artifacts, no matter how nested directories were.

vishalnayak · 2016-10-04T20:55:30Z

physical/file.go

-			return err
-		}
+	if err != nil && !os.IsNotExist(err) {
+		return fmt.Errorf("Failed to remove `%s`: %v", fullPath, err)


%s can be replaced with %q

vishalnayak · 2016-10-04T20:55:57Z

physical/file.go

-			return err
-		}
-	}
+	err = b.cleanupLogicalPath(path)


Can we make it return b.cleanupLogicalPath(path)?

Would it be a problem if I leave it as-is ? Two things:
a) debugging - while stepping a program, you can inspect the output of err before escaping the function
b) there is clear separation between program/logic (doSomeStuff...) and the return statement

What do you think ?

vishalnayak · 2016-10-04T20:56:19Z

physical/file.go

+// 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, "/")


Can we use os.PathSeparator here?

vishalnayak · 2016-10-04T20:59:19Z

physical/file.go

@@ -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 {


Can we check if path is empty here and return nil?

vishalnayak · 2016-10-04T21:01:15Z

physical/file.go

+// 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, "/")


If we check for path being empty in the Delete method above (left a comment there as well), then we don't let i becoming -1 here. Otherwise there is a risk for a panic here.

I am not sure I understand - if i becomes -1 then the loop will not be executed: https://play.golang.org/p/TH1DHWW3h6

vishalnayak · 2016-10-04T21:03:04Z

physical/file.go

+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], "/")


Can we employ a combination of os.PathSeparator and filepath.Join() to achieve this?

vishalnayak · 2016-10-04T21:08:03Z

@vespian I left a few comments on the file backend. Please also assume the same comments for ZK wherever applicable.

vishalnayak · 2016-10-05T02:44:26Z

Thank you @vespian for submitting this! I'll be merging this in hopes to try to get this into our next release.

vespian · 2016-10-05T10:59:12Z

@vishalnayak I have have made changes you requested in #1969 Could you please have a look and merge if applicable ? Thanks!

jefferai requested changes Oct 4, 2016

View reviewed changes

jefferai added this to the 0.6.2 milestone Oct 4, 2016

vespian force-pushed the prozlach/nested_secrets_handling_fix branch 2 times, most recently from e016013 to a6a8da8 Compare October 4, 2016 19:50

Pawel Rozlach added 3 commits October 4, 2016 21:55

Add tests for nested/prefixed secrets removal.

7b5f467

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.

Fix file backend so that it properly removes nested secrets.

3156098

This patch makes file backend properly remove nested secrets, without leaving empty directory artifacts, no matter how nested directories were.

vespian force-pushed the prozlach/nested_secrets_handling_fix branch from a6a8da8 to 3156098 Compare October 4, 2016 19:56

vishalnayak suggested changes Oct 4, 2016

View reviewed changes

vishalnayak merged commit 2711249 into hashicorp:master Oct 5, 2016

vespian pushed a commit to d2iq-archive/vault that referenced this pull request Oct 5, 2016

Post-review fixes for hashicorp#1964

70058ef

vespian pushed a commit to d2iq-archive/vault that referenced this pull request Oct 5, 2016

Post-review fixes for hashicorp#1964

7d8b638

adamdecaf mentioned this pull request Jan 10, 2017

physical/zk: Ignore ErrNoNode when deleting znodes #2256

Merged

tsaarni mentioned this pull request Jul 11, 2021

Update Go client libraries for etcd #11980

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nested secrets handling fix for zookeeper and file based backend. #1964

Nested secrets handling fix for zookeeper and file based backend. #1964

vespian commented Oct 4, 2016

jefferai Oct 4, 2016

vespian Oct 4, 2016

vishalnayak Oct 4, 2016

vespian Oct 5, 2016

vishalnayak Oct 4, 2016

vespian Oct 5, 2016 •

edited

Loading

vishalnayak Oct 4, 2016

vespian Oct 5, 2016

vishalnayak Oct 4, 2016

vespian Oct 5, 2016

vishalnayak Oct 4, 2016

vespian Oct 5, 2016

vishalnayak Oct 4, 2016 •

edited

Loading

vespian Oct 5, 2016

vishalnayak commented Oct 4, 2016

vishalnayak commented Oct 5, 2016

vespian commented Oct 5, 2016

Nested secrets handling fix for zookeeper and file based backend. #1964

Nested secrets handling fix for zookeeper and file based backend. #1964

Conversation

vespian commented Oct 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vespian Oct 5, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vishalnayak Oct 4, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vishalnayak commented Oct 4, 2016

vishalnayak commented Oct 5, 2016

vespian commented Oct 5, 2016

vespian Oct 5, 2016 •

edited

Loading

vishalnayak Oct 4, 2016 •

edited

Loading