Let allowed_users template mix templated and non-templated parts

[phihos]

As outlined in #10388 the regex checking for templates tags in allowed_users when signing SSH certs could be more liberal. By removing ^ and $ templates like username-{{ identity.entity.name }} are also allowed.

[hashicorp-cla]

All committers have signed the CLA.

[phihos]

I also would like to provide a test, but I could not find out how to create a new entity and acquire a token for it with the logicaltest.TestCase. I you could point me in the right direction I will gladly write a test for that.

[candlerb]

A quick scan suggested these may be helpful:

• https://github.com/hashicorp/vault/blob/master/vault/token_store_test.go#L2623-L2635
• helper functions testMakeTokenXXX

[phihos]

@candlerb Thanks for the pointer. I implemented a test now, but I must admit that I am not fully satisfied with the result. logicaltest.TestCase would have been perfect because it is structured nicely, but since it does not allow to declare both a LogicalFactory and a CredentialFactory I was forced to create a Vault instance with NewTestCluster. The test turned out way more verbose than I intended. Maybe you or somebody else knows how to improve this without writing a new vault testing framework.

[pmmukh]

Would it be possible to rewrite the setup code in this test to reuse getSshCaTestCluster ? I think if so, that might address the verbosity issue you've pointed out above.

[phihos]

Thanks, that is exactly what I needed. I changed it accordingly.

[pmmukh]

Another thing, I think it might be nice to add a test that checks just the currently supported {{..}} format. This is really only cause I noticed the prior PR did not have tests in them, so a test similar to what you currently have but for the existing format might be nice, to show there's no regression with this change. But also, I totally understand if that feels a bit out of scope for this PR and am fine skipping this too!

[pmmukh]

Hi @phihos ! If you're still looking to get this PR merged in, could you please take a look at the couple comments I dropped? This is a change we're pretty interested in getting merged in, so would be great if you can!

[phihos]

Hi @pmmukh. Sorry for the late reply, I was pretty caught up in work lately. I will try to implement your comments today or this weekend should that fail. I am happy to hear about your interest to merge this PR.

[phihos]

@pmmukh I changed the existing test and added a second one that tests the old functionality. I extracted the common test code into an additional function.

[pmmukh]

The test changes look great, thanks for the followup and especially for adding the test on the current functionality! Have a couple small nits, but more importantly, this change needs a changelog, with a changelog added this should be good to go!

[pmmukh]

Just another point that I thought of, could you maybe add a line about this behavior here https://www.vaultproject.io/api-docs/secret/ssh#allowed_users ? Maybe something like, "when allowed_users_template is set, this field can contain an identity template, with any prefix or suffix, like ssh-{identity.entity.id}-user" ? Feel free to word it how you want btw, just a suggestion.

[phihos]

I added the doc line you suggested. I am not sure if I did in the correct location though. @pmmukh Can you confirm that I did it correctly?

[pmmukh]

just one note, lgtm!