From 686a33ca747c561ec73e56d2ec49d5db965bad6a Mon Sep 17 00:00:00 2001 From: joshvanl Date: Thu, 11 Feb 2021 10:20:03 +0000 Subject: [PATCH] Add a section as to _why_ I am proposing this approach Signed-off-by: joshvanl --- .../20210209.certificates.k8s.io-adoption.md | 69 ++++++++++++++----- 1 file changed, 52 insertions(+), 17 deletions(-) diff --git a/design/20210209.certificates.k8s.io-adoption.md b/design/20210209.certificates.k8s.io-adoption.md index 94d0f8c69..47e52d3d9 100644 --- a/design/20210209.certificates.k8s.io-adoption.md +++ b/design/20210209.certificates.k8s.io-adoption.md @@ -22,6 +22,7 @@ status: provisional - [Motivation](#motivation) - [Goals](#goals) - [Non-Goals](#non-goals) +- [Reasons For Approach](#reasons-for-approach) - [Proposal](#proposal) - [Signers](#signers) - [API Changes](#api-changes) @@ -62,11 +63,45 @@ third-party project. - Make changes to upstream Kubernetes to implement controllers in cert-manager +## Reasons for Approach + +Below are a summary of properties of the `CertificateSigningRequest`, and +explanations as to why other approaches to integrate the resource with +cert-manager aren't possible (i.e. completely switching the `CertificateRequest` +resource to `CertificateSigningRequest`: + +1. `CertificateSigningRequests` are cluster scoped resources. This means that + the concept of namespaced `Issuers` doesn't fit with this resource, and any + attempt to integrate it would open up the ability of privilege escalation + (i.e. referencing Secrets in another tenants Namespace). Switching to + `CertificateSigningRequests` would mean the removal of the `Issuer` resource. + +1. As `CertificateSigningRequests` are cluster scoped, they do not provide the + same kind of isolation that `CertificateRequests` provide- preventing tenants + reading others requests, namespace specific issuer authentication (e.g. Vault + Kubernetes auth), namespacing cert-manager. + +1. `CertificateSigningRequests` do not include a duration field. This means that + any attempt to "proxy", or copy `CertificateRequests`, would result in a + regression of this feature. A feature which many users of cert-manager rely + on. + +1. cert-manager has a large number of [external + issuers](https://cert-manager.io/docs/configuration/external/) created by + the community that operate over the `CertificateRequest` resource. No longer + supporting these resources would require these authors to completely re-write + these issuers to continue to be supported by cert-manager. + +Supporting both resource types, but making `CertificateRequests` the "priority" +resource (i.e. the resource created by the `Certificates` controller), means +that we don't have any regression in the features cert-manager offers, whilst +maintaining the same security properties. + ## Proposal -Instead of the concept of an `IssuerRef` for `CertificateRequest`s, -`CertificateSigningRequest`s have the concept of a `SignerName`. Since -`CertificateSigningRequest`s are cluster scoped resources, the signer name can +Instead of the concept of an `IssuerRef` for `CertificateRequests`, +`CertificateSigningRequests` have the concept of a `SignerName`. Since +`CertificateSigningRequests` are cluster scoped resources, the signer name can be directly mapped to the `ClusterIssuer` resource. `ClusterIssuers` will be referenced in the following format: @@ -79,26 +114,26 @@ existing `CertificateRequest` resource, by getting the referenced `ClusterIssuer`, and attempting to sign. If the `ClusterIssuer` type is not managed by this controller, do nothing, else sign. -Each `CertificateSigningRequest` controller will set the +Each `CertificateSigningRequest` controller will automatically set the `CertificateSigningRequest` `RequestCondition` to `Approved` if the request is -for their managed `ClusterIssuer` type, and the `ClusterIssuer` is ready. If -the controller does manage that `ClusterIssuer` type, but the `ClusterIssuer` -doesn't exist or is not ready, the controller will set the condition to -`Failed`. +for their managed `ClusterIssuer` type. If the controller manages that +`ClusterIssuer` type, but the `ClusterIssuer` doesn't exist or is not ready, the +controller will set the condition to `Pending`. If signing fails during +processing, it will set the `CertificateSigningRequest` condition to `Failed`. ### Signers -Some special cases for some `ClusterIssuer`s that need to be addressed: +Some special cases for some `ClusterIssuers` that need to be addressed: - SelfSigned: Makes use of annotations, and so these annotations should also be - present on `CertificateSigningRequest`s. + present on `CertificateSigningRequests`. - Venafi: Makes use of annotations, and so these annotations should also be - present on `CertificateSigningRequest`s. + present on `CertificateSigningRequests`. - ACME: The ACME controller creates sub-resources (`Orders`). Since - `CertificateSigningRequest`s are cluster scoped resources, we should create + `CertificateSigningRequests` are cluster scoped resources, we should create `Orders` in the `Cluster Resource Namespace` (default `cert-manager`). @@ -115,8 +150,8 @@ No effect to upgrades as only additional controllers added. No API changes. ### Risks and Mitigations The controllers need to be aware if the `certificates.k8s.io/v1` -`CertificateSigningRequest` resource exists (pre v1.19 cluster). If they don't -exist, they should gracefully never start. If the Kubernetes API server were to -be upgraded to a version that does support this resource, cert-manager will need -to be restarted to make use of these controllers. This is acceptable- worker -nodes are typically always restarted during a cluster upgrade. +`CertificateSigningRequest` resource exists (e.g. pre v1.19 cluster). If they +don't exist, they should gracefully never start. If the Kubernetes API server +were to be upgraded to a version that does support this resource, cert-manager +will need to be restarted to make use of these controllers. This is acceptable- +worker nodes are typically always restarted during a cluster upgrade.