Set Organization in Certificates

[Queuecumber]

What this PR does / why we need it:

The CA issuer was previously issuing year-valid certificates with the /O set to "cert-manager". This PR makes those values configurable via the Issuer or ClusterIssuer resource. This allows for

• more customization for your issued certs (it looks unprofessional to issue client-auth certs with someone elses org name for example)
• Ability to provide different QoS or different security levels via different issuers/cluster issuers

Examples of the last case:

• you could have an issuer of client certs for employees that are valid for a year and for consultants that are valid for a month (QoS)
• you could have a highly secure resource that you issuer certificates valid for a few days (security level)

Special notes for your reviewer:

I'm not married to the name "Days" for the field setting the valid duration, but it mirrors the language used by the OpenSSL binary. If you guys have a better suggestion for the name I'm open to it.

Release note:

Add "organization" field to certificate objects

[Queuecumber]

@munnerz @kragniz requesting review

[munnerz]

These fields are no specific to the CA Issuer and as such should be added on the Certificate resource in an issuer-agnostic way.

#520 currently tracks having a custom certificate duration.

I'd be happy to accept a PR for a custom organisation. As far as I am aware, it's possible to set this field on a CSR, so for now it should reside on the Certificate resource as well.

We can explore defaulting mechanisms so that administrators can set a default on issuer resources too, however it may be easier to do that in a separate PR (as it may be more nuanced with certain issuer types)

[Queuecumber]

@munnerz So the suggestion then is to add these fields into the Certificate resource and then ignore them if the issuer doesn't support setting an org?

I think there should definitely be something on the issuer end where it has a defaultOrganization and defaultDays along with a policy that either applies those values if they arent set in a cert or overrides them if the issuer doesnt allow them to be set.

How does that sound for an update?

PR you referenced has languished since June and I can't be issuing year-long certs from someone elses org so I'm happy to work on this.

[Queuecumber]

OK based on the discussion in the file-comments on that PR let's give that guy his few days and see what happens, I'll update this to a PR that sets org in CSR and defaultOrg in issuer with a policy for overwrites. How does that sound?

[munnerz]

@munnerz <https://github.com/munnerz> So the suggestion then is to add these fields into the Certificate resource and then ignore them if the issuer doesn't support setting an org?

Yes, although we have validation that we can use to verify the certificates are valid for their issuers, so we don't simply ignore this case.

I think there should definitely be something on the issuer end where it has a defaultOrganization and defaultDays along with a policy that either applies those values if they arent set in a cert or overrides them if the issuer doesnt allow them to be set on clients.

Agreed - however this sort of defaulting is a more general problem than this one feature, and ties into policy etc. There's a number of different ways it can be solved, and it will require a proposal to consider these options which I think will take longer than just adding the simple Certificate fields for now. Something I've been meaning to put together but have not yet!

…

On Fri, 17 Aug 2018 at 15:21, Max Ehrlich ***@***.***> wrote: @munnerz <https://github.com/munnerz> So the suggestion then is to add these fields into the Certificate resource and then ignore them if the issuer doesn't support setting an org? I think there should definitely be something on the issuer end where it has a defaultOrganization and defaultDays along with a policy that either applies those values if they arent set in a cert or overrides them if the issuer doesnt allow them to be set on clients. How does that sound for an update? PR you referenced has languished since June and I can't be issuing year-long certs from someone elses org so I'm happy to work on this. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#838 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAMbP6QdfkBvk-lSHeTC5Wsu2nX3IEDhks5uRtFOgaJpZM4WBhK1> .

[Queuecumber]

Ok that makes sense, I will reduce the scope of this pr to setting org in the certificate resource, rework it, and wait on everything else

[Queuecumber]

@munnerz Requesting re-review, org is now set in the certificate resource, ACME certificates fail validation if they have this field set, and a test case was added that verifies this behavior. I have confirmed that it correctly issues certificates with organizations set and that it fails to validate ACME certificates with organizations set in a real cluster.

[Queuecumber]

BTW you appear to have a bug on line 59 of pkg/apis/validation/certificate_for_issuer.go I think you copied the validation logic from the block before it and forgot to update the interface field of field.Invalid

[Queuecumber]

@munnerz Now that you're back from vacation can I get an ok to test?

[munnerz]

Hey @Queuecumber - sorry for the delay in getting an ok-to-test on this! I'm going to do a review today, and would love to get this merged this week 😄 again - sorry for the delay!

/ok-to-test

[munnerz]

Ignore these test failures for now - there's a bug in our test infra! 😬 (working on it now)

[munnerz]

Should be working now!

/retest

[Queuecumber]

No problem, I managed to stay busy fighting with Ceph and its obscene memory usage, looking forward to getting these last two merged

[munnerz]

Could you also update the WaitForCertificateIssuedValid function used during e2e tests to include verifying the Organization field is set as expected? (https://github.com/jetstack/cert-manager/blob/master/test/util/util.go#L214)

If you could then also add a new e2e test for the supported providers that verifies that the Org field is set properly when a custom one is specified 😄

Otherwise, all looks good and I'm looking forward to getting this in v0.5!

[Queuecumber]

Just noticed there is support for multiple organizations in the x.509 spec (and that the Go pki organization field is a string array). Probably this should be refactored to support setting multiple orgs.

[jetstack-bot]

@Queuecumber: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
cert-manager-e2e-v1-9	8b6620e	link	/test e2e v1.9

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

[Queuecumber]

OK looks like we're good here. Vault issuer also had a problem with setting O, so handling it in the pki module wasn't going to be a clean solution. Also WaitForCertificateIssuedValid doesnt seem to have an easy way to get at the issuer type that was used to make the cert, so I couldnt really check for that. The solution I came up with was to allow the O field to be empty OR equal to exactly what was asked for in the CSR.

[munnerz]

Vault issuer also had a problem with setting O, so handling it in the pki module wasn't going to be a clean solution

This all looks good, but can you also add validation (plus a test) for this case too? (assuming we are disallowing setting the organisation field for Vault too)

[Queuecumber]

@munnerz I added the validation logic to disallow setting org on vault certificates, although I think you can configure vault to allow that (I guess that's true for CA as well though, which also isn't allowed). There are currently no tests for validation logic for the vault issuer (or at least I didnt see any) so I didnt add that.

[munnerz]

Great, thanks 😄

This should land just in time for the v0.5 release!

/lgtm
/approve

[jetstack-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: munnerz

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [munnerz]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment