Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extending generateCreationBundle to allow for specifying PKIX subject… #3999

Closed
wants to merge 1 commit into from

Conversation

robison
Copy link
Contributor

@robison robison commented Feb 17, 2018

… data in CSR/issuance requests, and verifying that data against configured role data, if present.

The use case is to allow users to specify PKIX data when making a request, but if their role has PKIX fields pre-configured, then test to see if the request data is a subset of the role data. I believe that this has utility beyond my particular needs; would love to receive feedback.

… data in CSR/issuance requests, and verifying that data against configured role data, if present
@jefferai
Copy link
Member

Thanks for submitting this but it's already in Vault! :-)

@jefferai jefferai closed this Feb 17, 2018
@robison
Copy link
Contributor Author

robison commented Feb 17, 2018

I’m sorry, was it added recently?

@jefferai
Copy link
Member

Yep, a day or two ago.

@robison
Copy link
Contributor Author

robison commented Feb 17, 2018

As of the submission of this PR, this functionality did not exist. Can you point out a merged commit or issue for it? Thanks!

@jefferai
Copy link
Member

Sure, #3992

@robison
Copy link
Contributor Author

robison commented Feb 17, 2018

I believe that the commit you referenced is only applicable to handling CA issuance, not non-CA issuance - and also does not immediately appear to set CN properly.

@jefferai
Copy link
Member

It's for non CA issuance via roles. What do you mean by doesn't set CN properly?

@robison
Copy link
Contributor Author

robison commented Feb 17, 2018

CommonName does not appear to be set in the subject in master/HEAD for cert issuance. It should be clear in the diff for this PR

@jefferai
Copy link
Member

Ah, thanks. I hadn't yet dug into what was causing Travis failures currently, but that does explain it. :-)

I took a look at the diff. What's the use-case for the client specifying values? It seemed to us that specifying at a role level should be sufficient, and not allowing the client to choose means you don't need to perform validation. Generally we don't see so much variance in OUs, much less O/L/C/etc. that creating a new role seemed like a significant lift.

@robison
Copy link
Contributor Author

robison commented Feb 18, 2018

Allowing variable OUs would be a good thing for an org that would like to have a single role (or maybe multiple roles) for an intermediate CA that would issue end-entity certificates for different units within said org. Constraining the allowable PKIX field values based on the assigned role also seems reasonable. The only problem that I don’t believe I fully addressed is considering adding a flag in the role to allow supplicants to specify PKIX values. And thanks for reading all of my comments!

@robison
Copy link
Contributor Author

robison commented Feb 18, 2018

(n.b.: adding multiple roles is a possible solution to this, but I believe that adds complexity to the supplicant process)

@jefferai
Copy link
Member

that would like to have a single role (or maybe multiple roles)

Multiple roles is already the flow! :-) And yes -- it adds some complexity to the supplicant process. But once you make something a client driven option you start having to think about the different ways people will want to control it, so pretty soon you end up with e.g. a set of OUs that support globbing that the client can pick from. And that's still supplicant complexity, plus more code complexity.

Taking a step back from there, it starts feeling like different OUs should reasonably correspond to different roles anyways. I'd rather take a wait and see approach here and see what demand is like. Sorry!

@robison
Copy link
Contributor Author

robison commented Feb 18, 2018

No problem. I’m going to take CN fix and the tests that I’ve added here and submit them in a different PR, if that’s alright - it seems that #3992 didn’t precisely add full test coverage for the added features.

@robison
Copy link
Contributor Author

robison commented Feb 18, 2018

Would that be a reasonable thing, at this point? And thanks for the feedback - looking forward to contributing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants