Fix SSH zero address OTP delete

[mbamber]

Fixed bug where SSH OTP roles could not be deleted if a zero-address role
previously existed, and there currently exist no zero-address roles.

Fixes #6382

The bug states the bug can be reproduced using the following sequence of commands:

vault secrets enable ssh
vault write ssh/roles/everyone default_user=ubuntu key_type=otp
vault write ssh/config/zeroaddress roles=everyone
vault delete ssh/roles/everyone
vault write ssh/roles/everyone default_user=ubuntu key_type=otp
vault delete ssh/roles/everyone

The result of running these with this change is below:

Success! Enabled the ssh secrets engine at: ssh/
Success! Data written to: ssh/roles/everyone
Success! Data written to: ssh/config/zeroaddress
Success! Data deleted (if it existed) at: ssh/roles/everyone
Success! Data written to: ssh/roles/everyone
Success! Data deleted (if it existed) at: ssh/roles/everyone

[hashicorp-cla]

All committers have signed the CLA.

[mbamber]

@meirish - Could you take a look at the failing ember test please; I don't believe this has any relevance to my PR. Indeed the tests are failing on master anyway.

I think this is the bit that is failing, but I could be wrong:

ok 64 Chrome 72.0 - [884 ms] - Acceptance | secrets/secret/create: version 2 with restricted policy still allows creation
2019-03-11T09:58:07.602Z [ERROR] secrets.system.system_30a74536: mount failed: path=kv-v2/ error="existing mount at kv-v2/"

ok 65 Chrome 72.0 - [1038 ms] - Acceptance | secrets/secret/create: version 2 with restricted policy still allows edit
2019-03-11T09:58:08.642Z [INFO]  core: successful mount: namespace= path=kv/ type=kv

ok 66 Chrome 72.0 - [2627 ms] - Acceptance | secrets/secret/create: paths are properly encoded
2019-03-11T09:58:11.277Z [ERROR] secrets.system.system_30a74536: mount failed: path=kv/ error="existing mount at kv/"

TIA

[kalafut]

@mbamber Thanks for finding this. Your PR does suppress the error, but zeroaddress is left with roles still configured with a value, which isn't ideal. I think an update to zeroAddressRoles.remove() might work better. If that returns nil when the list is empty, and perhaps if the role isn't found, the empty roles list will be written to zeroaddress.

[kalafut]

@mbamber We'd like to close this issue in the next few days. If you're able to make the changes to remove() that'd be great, otherwise I can update this PR too.

[mbamber]

Sure I can have a go at this - thanks for the help :)

[mbamber]

@kalafut - Looking over this again this morning I'm not quite sure I understand :/

AFAICT, zeroAddressRoles.remove() does return nil when the list is empty:

// If slice has zero or one item, remove the item by setting slice to nil.
if length < 2 {
	r.Roles = nil
	return nil
}

I think the issue with this, is that backend.putZeroAddressRoles() writes an empty list into the backend (via logical.StorageEntryJSON), which is different to when there were no ZeroAddressRoles in the first place.

Perhaps you could take a look and update the PR for me?

[kalafut]

@mbamber The error was was due to if index >= length.... The code I just pushed is much simpler thanks to a helper function that didn't exist in 2015 🙂, and the entire remove() function has been pulled. Please give it a try and see if it works for you too.

[mbamber]

@kalafut - lgtm! Thanks for the help :)

[kalafut]

@mbamber Thanks for raising this issue.