Transit: Fix bug where locks were not being freed on specific operations - with tests

[cipherboy]

@Gabrielopesantos started this one in #136 (thank you!!!), but I got a little excited with the tests, so I'll take over from here... :-)

This PR includes the fix to a bug happening in certain transit operations, sign and verify, where locks weren't being freed as expected which lead to goroutine deadlocks (operations hitting their timeout).

The deferred call of Unlock has been included in the if block so that it is only called if Lock has been initially called.

EDIT: After doing some additional tests (on other files), it seems there are cases where makes difference between leaving the deferred Unlock call to be in the if block or outside. Are there any known cases where Unlock calls are expected even if Lock isn't called?

Resolves hashicorp/vault#25325.

---

This moves all of Transit and the keysutil.LockManager.GetPolicy(...) helper to a consistent pattern: acquire lock and defer, or, if given an implicit lock and we don't need one, unlock it.

This imposes a slight performance penalty on mixed read/write workload: by holding the lock longer during templating (in the batch encrypt/decrypt or HMAC cases), we can prevent a write from occurring sooner. This is an OK trade off for

---

Notably, this catches a panic caused by concurrent access to an unlocked pointer: when I introduced d52d307, I had forgotten to adjust the locking mechanism for keyPolicyWrite: previously it didn't need any values in the returned policy and so could unlock in the one case it held the lock. After that commit, it read that policy, which (when caching is in use and rotation was occurring) could cause a concurrent read/write to the same map:

fatal error: concurrent map iteration and map write

goroutine 12681 [running]:github.com/openbao/openbao/builtin/logical/transit.(*backend).formatKeyPolicy(0x479ecb0?, 0xc00094dd40, {0x0, 0x0, 0x0})
      /home/cipherboy/GitHub/openbao/openbao/builtin/logical/transit/path_keys.go:364 +0x186cgithub.com/openbao/openbao/builtin/logical/transit.(*backend).pathPolicyWrite(0xc00065b500, {0x479e428, 0xc0031a7f20}, 0xc0037a4fc0, 0xc0014f6b10)
      /home/cipherboy/GitHub/openbao/openbao/builtin/logical/transit/path_keys.go:257 +0xd25github.com/openbao/openbao/sdk/framework.(*Backend).HandleRequest(0xc000f141e0, {0x479e428, 0xc0031a7f20}, 0xc0037a4fc0)
      /home/cipherboy/GitHub/openbao/openbao/sdk/framework/backend.go:300 +0xac8github.com/openbao/openbao/builtin/plugin/v5.(*backend).HandleRequest(0xc0010526c0, {0x479e428, 0xc0031a7f20}, 0xc0037a4fc0)

[Gabrielopesantos]

Interesting, I didn't consider the slight performance trade off that you mention but it should be very minor.

[cipherboy]

Thanks @Gabrielopesantos!

Interesting, I didn't consider the slight performance trade off that you mention but it should be very minor.

This is based on the size of the batch request. A larger batch will result in more time taken to marshal the response, but relative to the encryption operation I'm guessing its minor, yes.

It is a read lock though, so at worst it delays future writes (+ newly inflight reads post-write?) from happening... but it doesn't matter on read-heavy workloads.