Certificate Signing Request Issuer CA

[JoshVanL]

What this PR does / why we need it:
This PR adds an optional CertificateSigningRequest CA issuer. The controller is only enabled if it is explicitly done so via the --feature-gates flag (i.e. --feature-gates=ExperimentalCertificateSigningRequestControllers=true)

This begins the work to integrating kubernetes CertificateSigningRequests as part of cert-manager. (ref #3646).

Users who request a certificate referencing a namespaced issuer, must have permissions to do so in the following form:

group: cert-manager.io
resource: signers
verb: reference
namespace: <referenced signer namespace>
name: <either the name of the signer or '*' for all signer names in that namespace>

A SubjectAccessReview is performed during processing to check whether the user has sufficient permissions to reference the Issuer encoded into the signer name.

Special notes for your reviewer:

This work is based on the design in #3646

Release note:

Adds an option for a Kubernetes CertificateSigningRequest controller to implement the CA Issuer.

/cc @munnerz

[JoshVanL]

flake
/retest

[irbekrm]

/test pull-cert-manager-e2e-v1-21

[SgtCoDFish]

a few comments from me; a lot of these are small but there are a couple of bigger ones.

as this is quite a big PR[1], I can't be fully confident that I've been over everything - a second set of eyes would be good here I think.

[1] not a criticism; it's hard to imagine how this could have been cut down much.

[SgtCoDFish]

nitpick: this doc comment is incorrect, I think?

Suggested change

	// Package certmanager is the internal version of the API.
	// Package experimental ...

[SgtCoDFish]

suggestion: should these default values come from https://github.com/jetstack/cert-manager/blob/a80198c03d108e4ceb7e5aceb0f2fd78be5db39d/pkg/apis/certmanager/v1/types.go#L203?

(I recognise that will require converting a certmangerv1.KeyUsage to a certificatesv1.KeyUsage, but I think it'd be good to re-use that function)

[JoshVanL]

Not sure I understand the intention with reusing this function. We are using a separate API group and are different types.

[SgtCoDFish]

"Default cert-manager key usages for certificates" is a useful concept and if we're going to have it as a concept we might as well have one place to change it rather than multiple places where we can forget one of them. I suppose we'd have, say, in pseudocode:

func DefaultKeyUsages() []x509.KeyUsage

in pkg/internal/ somewhere. I don't think it's required for this PR to be merged.

[JoshVanL]

Let's leave this for now. We can always add the Default to the API at a later date.

[irbekrm]

Looks good to me- I have read through the code and ran the tests.
I've added a couple nits, but generally happy to approve.

[JoshVanL]

Thanks @SgtCoDFish @irbekrm

I have addressed your comments in the latest commit or responded. Good for another review and (hopefully) merge 🙂

[JoshVanL]

/assign @irbekrm @SgtCoDFish

[irbekrm]

Looks good to me- I could have spent more time actually trying it out, but I guess since it's an experimental controller it'd be easier to change things later.

[SgtCoDFish]

just one test looks incorrect to me

[SgtCoDFish]

one style suggestion from me, but non-blocking

[SgtCoDFish]

good spot on the clusterissuers special case!

suggestion (non-blocking): I know this is splitting hairs a little, but would this be better as an if statement? A switch with a single case and a default doesn't feel right; at the very least, we can save a level of indentation here.

Suggested change

	switch len(signerNameSplit) {
	case 1:
	return SignerIssuerRef{
	Namespace: "",
	Name: signerNameSplit[0],
	Type: signerTypeSplit[0],
	Group: signerTypeSplit[1],
	}, true
	default:
	// ClusterIssuers do not have Namespaces
	if signerTypeSplit[0] == "clusterissuers" {
	return SignerIssuerRef{
	Namespace: "",
	Name: strings.Join(signerNameSplit[0:], "."),
	Type: signerTypeSplit[0],
	Group: signerTypeSplit[1],
	}, true
	}
	// Non Cluster Scoped issuers always have Namespaces
	return SignerIssuerRef{
	Namespace: signerNameSplit[0],
	Name: strings.Join(signerNameSplit[1:], "."),
	Type: signerTypeSplit[0],
	Group: signerTypeSplit[1],
	}, true
	}
	if len(signerNameSplit) == 1 {
	return SignerIssuerRef{
	Namespace: "",
	Name: signerNameSplit[0],
	Type: signerTypeSplit[0],
	Group: signerTypeSplit[1],
	}, true
	}
	// ClusterIssuers do not have Namespaces
	if signerTypeSplit[0] == "clusterissuers" {
	return SignerIssuerRef{
	Namespace: "",
	Name: strings.Join(signerNameSplit[0:], "."),
	Type: signerTypeSplit[0],
	Group: signerTypeSplit[1],
	}, true
	}
	// Non Cluster Scoped issuers always have Namespaces
	return SignerIssuerRef{
	Namespace: signerNameSplit[0],
	Name: strings.Join(signerNameSplit[1:], "."),
	Type: signerTypeSplit[0],
	Group: signerTypeSplit[1],
	}, true

[JoshVanL]

I'm a big proponent for switch statements personally 🤷

[JoshVanL]

Happy to change it to an if statement though.

[SgtCoDFish]

/lgtm

[jetstack-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: irbekrm, JoshVanL, SgtCoDFish

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [JoshVanL,SgtCoDFish,irbekrm]

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

[SgtCoDFish]

/test pull-cert-manager-e2e-v1-21

[jetstack-bot]

@JoshVanL: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-cert-manager-e2e-v1-21	36bd7a4	link	/test pull-cert-manager-e2e-v1-21

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.

Details

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.