Add option allowed_domains_template enabling identity templating for issuing PKI certs

[andrejvanderzee]

Implementation of identity templating when issuing a PKI cert using the exported sdk/framework merged in #8088

Duplicate of #6558

Similar to #7548

[andrejvanderzee]

@jefferai Any chance merging the long-awaited closure of security hole of spoofing CNs in issuing x509 certificates?

[tyrannosaurus-becks]

Hi! Thanks for submitting this pull request.

I notice there are some test failures that appear related:

=== Errors
builtin/logical/pki/cert_util_test.go:149:35: not enough arguments in call to generateCreationBundle
	have (*backend, *inputBundle, nil, nil)
	want (*backend, *logical.Request, *inputBundle, *certutil.CAInfoBundle, *x509.CertificateRequest)
builtin/logical/pki/cert_util_test.go:181:35: not enough arguments in call to generateCreationBundle
	have (*backend, *inputBundle, nil, nil)
	want (*backend, *logical.Request, *inputBundle, *certutil.CAInfoBundle, *x509.CertificateRequest)

Happy to take a closer look after the tests are passing.

[qk4l]

@andrejvanderzee , thank you for non stoping decision in implementation this features. As I've counted it's third PR and I hope the last one. =)

Could you please provide status info, if you have time to finish it, so we all can finally use this must-have feature?

Thanks again for you work.

[andrejvanderzee]

@qk4l Thanks, let me fix the comments today.

[andrejvanderzee]

Hi @qk4l and @tyrannosaurus-becks I have pushed fixes for the failing test.

Now test-go-race is failing on the new raft storage. That is not related to this PR I assume?

[andrejvanderzee]

Minimized the PR and pushed again...

[andrejvanderzee]

Hi @qk4l and @tyrannosaurus-becks any update on this MR?

[ncabatoff]

Thank you for working on this feature, it is clearly in high demand. We're sorry it took so long to come to a consensus internally on how to proceed, but we finally have. As you may be aware, there are currently 2 open PRs trying to implement identity templating for PKI, of which yours is one: #7216 and #8509. It would be great if you would coordinate amongst yourselves to see who has the time and inclination to move forward with the proposal.

The proposal:

• Add a boolean role option allowed_domains_template to control whether the allowed_domains option will be interpreted as possibly containing an identity template that should be expanded. This will provide consistency with what was done in the ssh secrets engine with the allowed_users_template parameter (identity propagation in ssh secrets engine #7547 #7548).
• This looks a bit like what Add option allowed_domains_template enabling identity templating for issuing PKI certs #8509 is doing, but in Add option allowed_domains_template enabling identity templating for issuing PKI certs #8509 allowed_domains_template is a string that is given as an alternative to allowed_domains, rather than a toggle that controls how allowed_domains is interpreted.

Other requirements:

• There need to be tests that exercise the new behaviour; there may be other tests as well, but there should be at least one that actually exercises the full behaviour, i.e. create a role with allowed_domains_template=true, then try to generate certs with that role that rely on the expanded template and cover both the happy path (e.g. CN in the cert request matches expanded allowed_domains) and the less happy paths. TestBackend_URI_SANs is a decent example of the pattern we have in mind.
• Use the framework method PopulateIdentityTemplate.

Not a strict requirement but a suggestion: limit the PR to allowed_domains. #7216 includes templating for allowed_uri_sans. This isn't a deal breaker, but the more narrowly focused the PR, the easier it will be to get it approved. We would prefer to see allowed_uri_sans handled in a separate PR (in the same way, with a new allowed_uri_sans_template role option.)

[andrejvanderzee]

@ncabatoff Sorry for the delay it was a busy time for me.

Today I was ably to implement the requirements that you have sent in your last post. Please let me know if something is still missing or not according to standards.

Thank you.

[andrejvanderzee]

Hi @ncabatoff @qk4l @tyrannosaurus-becks
Any update on this MR?

[andrejvanderzee]

@ncabatoff I have added the two testcases and resolved the conversation.

[ncabatoff]

Thanks again for the PR and your patience.

[andrejvanderzee]

Thank you all, this PR has grown a long beard ;-)

[qk4l]

I've noticed that this regex force to use only one pure variable. Additional domain suffices or other variables is not allowed.

Is it a feature or bug?
I suggest to remove this restriction.

What do you think?

[andrejvanderzee]

I've noticed that this regex force to use only one pure variable. Additional domain suffices or other variables is not allowed.

Is it a feature or bug?
I suggest to remove this restriction.

What do you think?

Yes I think that makes sense. Its easy to add a testcase for your suggestion. I can implement it this week or if you have time you could add it, either way fine with me!

[danielpops]

I'm so happy that this feature is merged!! Thanks so much to everyone that worked on this!

[qk4l]

I can implement it this week or if you have time you could add it, either way fine with me!

I did it in #9498