Cubbyhole cleanup

[vishalnayak]

Fixes #5959

[briankassouf]

Could we add a test that will make sure cubbyhole entries are cleaned up once a token is revoked?

[vishalnayak]

@briankassouf Test for cubbyhole deletion is added.

[tyrannosaurus-becks]

@vishalnayak when I pull this branch and go through the steps to reproduce in the linked ticket, I'm still seeing that the count returned doesn't return to the original one.

postgres=# select count(*) from vault_kv_store;
 count 
-------
    34
(1 row)

postgres=# select count(*) from vault_kv_store;
 count 
-------
    39
(1 row)

postgres=# select count(*) from vault_kv_store;
 count 
-------
    35
(1 row)

[vishalnayak]

@tyrannosaurus-becks Can you please see which storage key is getting dangled?

[tyrannosaurus-becks]

@vishalnayak when I start with a fresh, initialized postgres db with no other secrets (in my previous example it wasn't fresh), I initially have 17 records in the vault_kv_store table. When I do the steps to duplicate, after 2 minutes pass, I have 18 records in the store. This is the record that's left behind:

• parent_path: /logical/cc6663b3-030f-b04f-10b0-25f072df2f69/5dUZAnWbsL0HtuMtDvq37EaO/path/to/short/
• path: /logical/cc6663b3-030f-b04f-10b0-25f072df2f69/5dUZAnWbsL0HtuMtDvq37EaO/path/to/short/lived/
• key: bucket

[briankassouf]

I wonder if we can inject some legacy style tokens to make sure we are not accidentally removing salted cubbyhole IDs?

[vishalnayak]

I'll think if I can do this tomorrow morning.

[vishalnayak]

If the token IDs are supplied, we force SHA1 hashing. I've added such tokens into the sample space for both tests.

[briankassouf]

@tyrannosaurus-becks That might be an issue with the MySQL backend, maybe try with the file or consul backend?

[briankassouf]

Just one comment around further testing, otherwise looks good

[vishalnayak]

@tyrannosaurus-becks That is really surprising. I am really clueless as to what could be happening with postgres. I have tested this with the file backend and it is working. You would have done the right thing, but I am tempted to ask you to recheck if the binary has the fix :-)
I'll hold merging this until we dig more on what you have found.

[jefferai]

Keep in mind that there may be two separate problems. It's the responsibility of the storage backend to remove a prefix no longer in use. Investigating that may have identified a totally different issue that affects more than just postgres.

I don't know for sure that this is what's going on but we've had other issues in the past related to various storage backends not cleaning up prefixes appropriately.

[vishalnayak]

@tyrannosaurus-becks I have verified the fix to be working in both file and consul backends.

[tyrannosaurus-becks]

@vishalnayak I re-tested and you were exactly right! I'd been testing with a different binary than the branch's. This is working for me! I verified the key count returned to the original.

[briankassouf]

Thanks for making those test changes, looks good!