Fix: non-login requests shouldn't care about login roles for lease quotas. Also fix a potential deadlock

[ncabatoff]

I'm working on some code in ent that messes with how we use these locks, so I turned on deadlock detection, and I found a pre-existing issue. Creating this PR to highlight it.

[ncabatoff]

This exposed:

POTENTIAL DEADLOCK: Duplicate locking, saw callers this locks in one goroutine:
current goroutine 39049 lock &{{{0 0} 0 0 {{} 1} {{} 0}}} 
all callers to this lock in the goroutine
vault/core.go:3786 vault.(*Core).DetermineRoleFromLoginRequest { c.authLock.RLock() } <<<<<
vault/request_handling.go:1024 vault.(*Core).handleRequest { loginRole := c.DetermineRoleFromLoginRequest(req.MountPoint, req.Data, ctx) }
vault/request_handling.go:681 vault.(*Core).handleCancelableRequest { resp, auth, err = c.handleRequest(ctx, req) }
vault/request_handling.go:488 vault.(*Core).switchedLockHandleRequest { resp, err = c.handleCancelableRequest(ctx, req) }
vault/request_handling.go:449 plugin_test.TestSystemBackend_Plugin_auth.func1 { return c.switchedLockHandleRequest(httpCtx, req, true) }
vault/external_tests/plugin/plugin_test.go:133 plugin_test.TestSystemBackend_Plugin_auth.func1 { resp, err := core.HandleRequest(namespace.RootContext(testCtx), req) }

vault/core.go:3786 vault.(*Core).DetermineRoleFromLoginRequest { c.authLock.RLock() } <<<<<
vault/wrapping.go:331 vault.(*Core).wrapInCubbyhole { if err := c.expiration.RegisterAuth(ctx, &te, wAuth, c.DetermineRoleFromLoginRequest(req.MountPoint, req.Data, ctx)); err != nil { }
vault/dynamic_system_view.go:214 vault.dynamicSystemView.ResponseWrapData { _, err := d.core.wrapInCubbyhole(ctx, req, resp, nil) }
sdk/helper/pluginutil/tls.go:100 pluginutil.wrapServerConfig { wrapInfo, err := sys.ResponseWrapData(ctx, map[string]interface{}{ }
sdk/helper/pluginutil/run_config.go:82 pluginutil.runConfig.makeConfig { wrapToken, err := wrapServerConfig(ctx, rc.Wrapper, certBytes, key) }
sdk/helper/pluginutil/run_config.go:113 pluginutil.runConfig.run { clientConfig, err := rc.makeConfig(ctx) }
sdk/helper/pluginutil/run_config.go:184 pluginutil.(*PluginRunner).RunConfig { return rc.run(ctx) }
sdk/helper/pluginutil/runner.go:68 pluginutil.(*PluginRunner).Run { return r.RunConfig(ctx, }
sdk/plugin/plugin.go:106 plugin.NewPluginClient { client, err = pluginRunner.Run(ctx, sys, pluginSet, HandshakeConfig, []string{}, namedLogger) }
sdk/plugin/plugin.go:63 plugin.NewBackendWithVersion { backend, err = NewPluginClient(ctx, sys, pluginRunner, conf.Logger, isMetadataMode) }
builtin/plugin/backend.go:130 plugin.(*PluginBackend).startBackend { nb, err := bplugin.NewBackendWithVersion(ctx, pluginName, pluginType, b.config.System, b.config, false, b.config.Config["plugin_version"]) }
builtin/plugin/backend.go:176 plugin.(*PluginBackend).lazyLoadBackend { err := b.startBackend(ctx, storage) }
builtin/plugin/backend.go:221 plugin.(*PluginBackend).HandleRequest { err = b.lazyLoadBackend(ctx, req.Storage, func() error { }
vault/core.go:3794 vault.(*Core).DetermineRoleFromLoginRequest { resp, err := matchingBackend.HandleRequest(ctx, &logical.Request{ }
vault/request_handling.go:1024 vault.(*Core).handleRequest { loginRole := c.DetermineRoleFromLoginRequest(req.MountPoint, req.Data, ctx) }
vault/request_handling.go:681 vault.(*Core).handleCancelableRequest { resp, auth, err = c.handleRequest(ctx, req) }
vault/request_handling.go:488 vault.(*Core).switchedLockHandleRequest { resp, err = c.handleCancelableRequest(ctx, req) }
vault/request_handling.go:449 plugin_test.TestSystemBackend_Plugin_auth.func1 { return c.switchedLockHandleRequest(httpCtx, req, true) }
vault/external_tests/plugin/plugin_test.go:133 plugin_test.TestSystemBackend_Plugin_auth.func1 { resp, err := core.HandleRequest(namespace.RootContext(testCtx), req) }

[VioletHynes]

Looks good! This should make sense, as we still call DetermineRoleFromLoginRequest in handleLoginRequest.

Our quota tests in ent are a lot more robust than they are in OSS (e.g. quotas_ent_test.go), so you might want to make sure they all pass, just to be safe, before merging.

[ncabatoff]

Our quota tests in ent are a lot more robust than they are in OSS (e.g. quotas_ent_test.go), so you might want to make sure they all pass, just to be safe, before merging.

Good idea, did so in https://github.com/hashicorp/vault-enterprise/pull/4164.