More Mount Conflict Detection #2919

otakup0pe · 2017-06-24T16:51:41Z

Ensure that we are not creating a "submount" underneath an already
existing mountpoint. This (hopefully) fixes #2878

Note that (locally) I'm seeing intermittent (~25% of the time on the full test suite) build failures 😿

--- FAIL: TestCluster_ForwardRequests (9.16s)
        cluster_test.go:350: err: error during forwarding RPC request

Ensure that we are not creating a "submount" underneath an already existing mountpoint. This (hopefully) fixes hashicorp#2878

otakup0pe · 2017-06-24T17:19:28Z

I'm missing something about the intersection between the router and logical backends... Doing some (manual) smoke testing, I noticed this repeating in the logs...

2017/06/24 10:14:45.630510 [ERROR] rollback: error rolling back: path=foo/ error=unsupported path

jefferai · 2017-06-24T17:58:03Z

The issue is that what's in the router's Mount code is only an extra check; the code in mount.go and auth.go check for an exact match via the router's MatchingMount, but then if that doesn't find a conflict they add the new entry into the table, then mount it on the router. The reason is that once a backend is able to be sent requests, we want it to already be stored so if Vault goes down it can come back up.

The right thing to do would probably be to take the logic you wrote and the check above for a longest prefix and combine it into two functions inside the router, something like AllowedMount and allowedMountInternal. The latter takes that logic and returns an error or not, and can be used inside the router's Mount function. The former does the same thing but gets a read lock (see MatchingMount); this can be called from mount.go and auth.go instead of the existing MatchingMount call.

otakup0pe · 2017-06-25T16:20:58Z

Thanks for the guidance, those changes cleared up the rollback error. Any guidance on why (local) postgres tests are (consistently) failing? Basic test suite (make test TEST=./vault) is fine.

--- FAIL: TestPostgreSQL_RenewUser (8.30s)
        postgresql_test.go:182: Could not connect with new credentials: pq: password authentication failed for user "v-test-test-ZLS5aFHTd2RNsz8eDKV8-1498407266"
--- FAIL: TestPostgreSQL_RevokeUser (7.56s)
        postgresql_test.go:250: Could not connect with new credentials: pq: password authentication failed for user "v-test-test-WzseS2eTjqgckXSBWefZ-1498407274"
FAIL
FAIL    github.com/hashicorp/vault/plugins/database/postgresql  36.679s

jefferai · 2017-06-25T20:50:23Z

vault/auth.go

@@ -73,7 +73,7 @@ func (c *Core) enableCredential(entry *MountEntry) error {
 		return fmt.Errorf("token credential backend cannot be instantiated")
 	}

-	if match := c.router.MatchingMount(credentialRoutePrefix + entry.Path); match != "" {
+	if match := c.router.MountConflict(credentialRoutePrefix + entry.Path); match != "" {


For ease of understanding, can you change match here to conflict and in the error message say "conflicting mount at ..."?

jefferai · 2017-06-25T20:58:30Z

vault/router.go

+	// with an existing mountpoint
+	if prefix != "" {
+		if match := r.MatchingPrefix(prefix); match != "" {
+			return fmt.Errorf("cannot mount under existing mount '%s'", match)


Isn't this checking for mounting over an existing mount?

jefferai · 2017-06-25T20:59:01Z

vault/router.go

+func (r *Router) MatchingPrefix(path string) string {
+	var existing string = ""
+	fn := func(existing_path string, _v interface{}) bool {
+		fmt.Println(fmt.Sprintf("comparing %v %v", path, existing_path))


I'm guessing this was a debug line you meant to take out

How embarrassing 😬

Mostly it was like, um, that's going to spam logs a LOT :-)

Which is why I'm feeling a touch embarrassed 👼

New commit incorporates the feedback and I added a little blurb to the docs about mount conflicts.

jefferai · 2017-06-29T03:10:18Z

vault/router.go

@@ -174,6 +181,31 @@ func (r *Router) MatchingMount(path string) string {
 	return mount
 }

+//MatchingPrefix returns a mount prefix that a path may be a part of
+func (r *Router) MatchingPrefix(path string) string {


This function doesn't get a read lock but should, as it can be called externally (via MountConflict from the mount code). However, it is called with a lock already held from the router's Mount function.

The way to deal with this would be to have MatchingPrefix grab a read lock, then call matchingPrefixInternal which has the actual logic. The router's Mount function would then just call matchingPrefixInternal directly.

jefferai · 2017-06-29T03:12:37Z

website/source/docs/secrets/index.html.md

@@ -59,6 +59,13 @@ Once a secret backend is mounted, you can interact with it directly
 at its mount point according to its own API. You can use the `vault path-help`
 system to determine the paths it responds to.

+Note that mount point cannot conflict with each other in Vault. There are


s/point/points

jefferai · 2017-06-29T03:17:21Z

vault/router.go

+//MatchingPrefix returns a mount prefix that a path may be a part of
+func (r *Router) MatchingPrefix(path string) string {
+	var existing string = ""
+	fn := func(existing_path string, _v interface{}) bool {


If this function executes at all wouldn't it mean that we've found a prefix? Is the if check unnecessary here?

otakup0pe · 2017-07-11T01:08:41Z

Added the read lock and fixed a typo but wasn't clear on the third statement. The if in what is now matchingPrefixInternal checks each entry in the radix tree... If you meant the if on line 67 - that is there because some entities (like policies iirc) didn't seem to have a prefix set...

otakup0pe · 2017-07-11T18:08:52Z

Just noticed tests failed (they are passing locally). Will take a look this afternoon/evening... Unless someone can restart the build 👼

I see the build is passing now, but I'm seeing the following error consistently locally...

--- FAIL: TestPostgreSQL_RenewUser (8.98s)
	postgresql_test.go:182: Could not connect with new credentials: pq: password authentication failed for user "v-test-test-PyffKa0TNLrdv8zX1sGc-1499798498"
--- FAIL: TestPostgreSQL_RevokeUser (7.84s)
	postgresql_test.go:250: Could not connect with new credentials: pq: password authentication failed for user "v-test-test-9c8dmq3SMXLPFhYK2q4Z-1499798506"

jefferai · 2017-07-17T15:27:00Z

vault/router.go

@@ -62,6 +62,13 @@ func (r *Router) Mount(backend logical.Backend, prefix string, mountEntry *Mount
 	if existing, _, ok := r.root.LongestPrefix(prefix); ok && existing != "" {
 		return fmt.Errorf("cannot mount under existing mount '%s'", existing)
 	}
+	// If this is a secret backend, check to see if the prefix conflicts
+	// with an existing mountpoint
+	if prefix != "" {


I'm pretty sure that we don't allow empty prefixes in the API calls or core mount function. Have you seen this? This if seems unnecessary.

Yes, this check appears unnecessary.

jefferai · 2017-07-17T15:27:43Z

vault/router.go

+
+// MountConflict determines if there are potential path conflicts
+func (r *Router) MountConflict(path string) string {
+	if exact_match := r.MatchingMount(path); exact_match != "" {


Really this function should grab a single read lock for the entire function and then call matchingMountInternal and matchingPrefixInternal. That way nothing can change between the two functions.

jefferai · 2017-07-17T15:30:25Z

Regarding the if -- both, actually. I left a comment on the one at line 67. The other one was about the use of HasPrefix in the walking function. Wouldn't we never even hit that part of the walk function if HasPrefix wasn't true?

otakup0pe · 2017-07-27T06:07:23Z

Shuffled around the "internal" functions. When I removed the prefix matching code in Mount, the build fails and I see a bunch of warnings/errors about audit logs (not sure why I thought policies). They might be a red herring, there is a lot of output to parse when the vault test suite fails.

2017/07/26 22:54:14.121014 [ERROR] audit: backend failed to log request: backend=foo error=failed
2017/07/26 22:54:14.121028 [ERROR] audit: backend failed to log request: backend=foo error=failed
2017/07/26 22:54:14.121037 [ERROR] audit: backend failed to log request: backend=bar error=failed
2017/07/26 22:54:14.121166 [ERROR] audit: backend failed to log response: backend=foo error=failed
2017/07/26 22:54:14.121178 [ERROR] audit: backend failed to log response: backend=foo error=failed
2017/07/26 22:54:14.121185 [ERROR] audit: backend failed to log response: backend=bar error=failed
2017/07/26 22:54:14.121376 [ERROR] audit: backend failed to log request: backend=foo error=failed
2017/07/26 22:54:14.121403 [ERROR] audit: backend failed to log request: backend=foo error=failed
2017/07/26 22:54:14.121430 [ERROR] audit: backend failed to log request: backend=bar error=failed

2017/07/26 22:54:14.078248 [ERROR] core: failed to create audit entry: path=noop/ error=failing enabling
2017/07/26 22:54:14.078259 [ERROR] core: failed to create audit entry: path=noop/ error=failing enabling
2017/07/26 22:54:14.078268 [ERROR] core: failed to create audit entry: path=noop2/ error=failing enabling

jefferai · 2017-07-27T16:31:10Z

@otakup0pe Your build works fine -- can you explain what you mean about "when I removed the prefix matching code in Mount"?

jefferai · 2017-08-30T19:29:03Z

@otakup0pe Just wanted to check on on this and see if you were going to address the remaining review items.

otakup0pe · 2017-09-06T19:04:17Z

Hi @jefferai I was off grid for several weeks. Will pick wrap this up now that I'm back!

Changed the tests around to better capture this now does. Existing paths paths are still supported from the perspective of the router, but not from the perspective of new mounts.

otakup0pe · 2017-09-18T00:22:46Z

I think this addresses the feedback. I adjusted the tests to account for the new pattern.

vishalnayak · 2017-10-23T13:28:41Z

vault/router_test.go

+		t.Fatalf("bad: prod/aws/")
+	}
+
+	err = r.Mount(n, "prod/", subMountEntry, view)


Can we have a comment here as to why this should pass?

vishalnayak · 2017-10-23T13:28:46Z

vault/router_test.go

@@ -118,6 +118,11 @@ func TestRouter_Mount(t *testing.T) {
 		t.Fatalf("err: %v", err)
 	}

+	meUUID, err = uuid.GenerateUUID()


Can we move this to be placed just above the definition of subMountEntry?

vishalnayak · 2017-10-23T13:28:47Z

vault/router.go

@@ -201,14 +200,46 @@ func (r *Router) MatchingMountByAccessor(mountAccessor string) *MountEntry {
 // MatchingMount returns the mount prefix that would be used for a path
 func (r *Router) MatchingMount(path string) string {
 	r.l.RLock()
-	mount, _, ok := r.root.LongestPrefix(path)
+	var mount = r.matchingMountInternal(path)


[Nitpick][Optional] We could do a defer r.l.RUnlock() and return r.matchingMountInternal(path) and avoid the local var mount.

vishalnayak

Very minor comments. This is looking good to me.

* oss/master: (30 commits) Handle 'not supplied' case for field type TypeNameString (#3546) Fix deprecated cassandra backend tests (#3543) changelog++ auth/aws: Make disallow_reauthentication and allow_instance_migration mutually exclusive (#3291) changelog++ More Mount Conflict Detection (#2919) Fix swallowed errors in TestRollbackManager_Join() (#3327) changelog++ added AWS enpoint handling (#3416) Seal wrap all root tokens and their leases (#3540) Return group memberships of entity during read (#3526) Add note on support for using rec keys on /sys/rekey (#3517) Add third party tools list to website (#3488) Minor client refactoring (#3539) changelog++ Add PKCS8 marshaling to PKI (#3518) Update SSH list roles docs (#3536) Update gocql dep changelog++ Return role info for each role on pathRoleList (#3532) ...

More Mount Conflict Detection

ff650e5

Ensure that we are not creating a "submount" underneath an already existing mountpoint. This (hopefully) fixes hashicorp#2878

jefferai added this to the 0.7.4 milestone Jun 24, 2017

Check for mount conflicts in more places and test helper explicitly

af30967

jefferai reviewed Jun 25, 2017

View reviewed changes

Jonathan Freedman added 2 commits June 25, 2017 14:27

Incorporate feedback and add a blurb to the docs

d848929

Incorporate feedback in proper file tho

cf07549

jefferai reviewed Jun 29, 2017

View reviewed changes

Jonathan Freedman added 2 commits July 10, 2017 18:00

Merge branch 'master' of github.com:hashicorp/vault into mount-conflict

e8f330e

Use a read lock when appropriate

f3136f0

otakup0pe mentioned this pull request Jul 12, 2017

Improve Ordering of Operations Autodesk/aomi#114

Merged

jefferai reviewed Jul 17, 2017

View reviewed changes

jefferai modified the milestones: 0.7.4, 0.8.0 Jul 24, 2017

Merge branch 'master' of github.com:hashicorp/vault into mount-conflict

b0fbfe8

shuffle around internal router/mount functions

bf02212

defer is kinda cool 🕶

6836698

jefferai modified the milestones: next-release, 0.8.2 Aug 18, 2017

jefferai modified the milestones: 0.8.2, 0.8.3 Aug 31, 2017

Jonathan Freedman and others added 3 commits September 17, 2017 12:35

Merge branch 'master' of github.com:hashicorp/vault into mount-conflict

314a032

Merge branch 'master' of github.com:hashicorp/vault into mount-conflict

23aa611

Only block path stomping on new mounts

c03b662

Changed the tests around to better capture this now does. Existing paths paths are still supported from the perspective of the router, but not from the perspective of new mounts.

jefferai modified the milestones: 0.8.3, 0.8.4 Sep 25, 2017

jefferai requested a review from vishalnayak October 19, 2017 16:56

vishalnayak reviewed Oct 23, 2017

View reviewed changes

vishalnayak previously approved these changes Oct 23, 2017

View reviewed changes

Update router.go

e2c0f63

jefferai dismissed vishalnayak’s stale review via e2c0f63 November 6, 2017 20:24

jefferai added 3 commits November 6, 2017 15:26

Update router_test.go

5b60f6b

Merge branch 'master' into mount-conflict

51c948f

Merge branch 'master' into mount-conflict

d70bcd5

jefferai merged commit a40f8c4 into hashicorp:master Nov 6, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More Mount Conflict Detection #2919

More Mount Conflict Detection #2919

otakup0pe commented Jun 24, 2017 •

edited

Loading

otakup0pe commented Jun 24, 2017

jefferai commented Jun 24, 2017

otakup0pe commented Jun 25, 2017 •

edited

Loading

jefferai Jun 25, 2017

jefferai Jun 25, 2017

jefferai Jun 25, 2017

otakup0pe Jun 25, 2017

jefferai Jun 25, 2017

otakup0pe Jun 25, 2017 •

edited

Loading

jefferai Jun 29, 2017

jefferai Jun 29, 2017

jefferai Jun 29, 2017

otakup0pe commented Jul 11, 2017

otakup0pe commented Jul 11, 2017 •

edited

Loading

jefferai Jul 17, 2017

chrishoffman Jul 27, 2017

jefferai Jul 17, 2017

jefferai commented Jul 17, 2017

otakup0pe commented Jul 27, 2017 •

edited

Loading

jefferai commented Jul 27, 2017

jefferai commented Aug 30, 2017

otakup0pe commented Sep 6, 2017

otakup0pe commented Sep 18, 2017

vishalnayak Oct 23, 2017

vishalnayak Oct 23, 2017

vishalnayak Oct 23, 2017

vishalnayak left a comment

More Mount Conflict Detection #2919

More Mount Conflict Detection #2919

Conversation

otakup0pe commented Jun 24, 2017 • edited Loading

otakup0pe commented Jun 24, 2017

jefferai commented Jun 24, 2017

otakup0pe commented Jun 25, 2017 • 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

otakup0pe Jun 25, 2017 • 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

otakup0pe commented Jul 11, 2017

otakup0pe commented Jul 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jefferai commented Jul 17, 2017

otakup0pe commented Jul 27, 2017 • edited Loading

jefferai commented Jul 27, 2017

jefferai commented Aug 30, 2017

otakup0pe commented Sep 6, 2017

otakup0pe commented Sep 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vishalnayak left a comment

Choose a reason for hiding this comment

otakup0pe commented Jun 24, 2017 •

edited

Loading

otakup0pe commented Jun 25, 2017 •

edited

Loading

otakup0pe Jun 25, 2017 •

edited

Loading

otakup0pe commented Jul 11, 2017 •

edited

Loading

otakup0pe commented Jul 27, 2017 •

edited

Loading