From 696356b702050ccebd29b630913fcf793cfa2dc0 Mon Sep 17 00:00:00 2001 From: joshvanl Date: Mon, 26 Jul 2021 14:20:18 +0100 Subject: [PATCH] Update certificates.k8s.io-adoption design to include changes which have been implemented Signed-off-by: joshvanl --- .../20210209.certificates.k8s.io-adoption.md | 198 ++++++++++-------- 1 file changed, 107 insertions(+), 91 deletions(-) diff --git a/design/20210209.certificates.k8s.io-adoption.md b/design/20210209.certificates.k8s.io-adoption.md index dd3818480..20fa913a4 100644 --- a/design/20210209.certificates.k8s.io-adoption.md +++ b/design/20210209.certificates.k8s.io-adoption.md @@ -9,8 +9,8 @@ approvers: - @jetstack/team-cert-manager editor: "@joshvanl" creation-date: 2021-02-09 -last-updated: 2021-02-25 -status: provisional +last-updated: 2021-07-26 +status: implemented --- # certificates.k8s.io Adoption @@ -20,11 +20,15 @@ status: provisional - [Summary](#summary) - [Motivation](#motivation) + * [Third Party Projects](#third-party-projects) + * [Security](#security) + * [cert-manager Webhook Dependency](#cert-manager-webhook-dependency) + * [CertificateSigningRequest Migration Path](#certificatesigningrequest-migration-path) * [Goals](#goals) * [Non-Goals](#non-goals) - [Reasons for Approach](#reasons-for-approach) - * [Read all access to `CertificateSigningRequests`](#read-all-access-to-certificatesigningrequests) - * [Namespaced issuers referenced by `CertificateSigningRequest`](#namespaced-issuers-referenced-by-certificatesigningrequest) + * [Read all access to CertificateSigningRequests](#read-all-access-to-certificatesigningrequests) + * [Namespaced issuers referenced by CertificateSigningRequest](#namespaced-issuers-referenced-by-certificatesigningrequest) * [Duration Field](#duration-field) * [CA Field](#ca-field) * [External Issuers](#external-issuers) @@ -68,12 +72,12 @@ than a configurable webhook, ### cert-manager Webhook Dependency Consumers of the certificates.k8s.io `CertificateRequest` do not require -cert-manager as a hard dependency. cert-manager can cause issues with order of -operation scenarios during installation or upgrades. For example, the +cert-manager as a hard dependency. cert-manager can cause issues with the order +of operation scenarios during installation or upgrades. For example, the `CertificateRequest` resource needs to be installed and the webhook ready for consumers/clients of this API to make requests. -In some setups, the cert-manager installation is handled by two separate teams: +In some set-ups, the cert-manager installation is handled by two separate teams: - The cert-manager CRDs and webhook are installed and managed by the operations team, which allows the operations team to enforce cluster @@ -82,12 +86,11 @@ In some setups, the cert-manager installation is handled by two separate teams: managed by the developer team. By supporting the built-in `CertificateSigningRequest` resource, we decrease the -coupling between the operations team and the developer team. Since the +coupling between the operations team and the developer team. Since the `CertificateSigningRequest` resource is built-in, it does not need any `CustomResourceDefinition` installation, and the webhook for this resource is integrated to the API server. - ### CertificateSigningRequest Migration Path Support for the `CertificateSigningRequest` resource in cert-manager is a @@ -120,14 +123,10 @@ resource. `CertificateRequests` are not. This means that in order for users to have the same debugging and usage capabilities of the `CertificateRequest`, users will have the ability to list all requests in the cluster, rather than being scoped to -specific namespaces. This is a regression in security. - -A solution to this problem could be given to users by providing examples or -guides to external tooling (e.g. [Open Policy Agent -(OPA)](https://github.com/open-policy-agent/opa), whereby cluster administrators -can limit users access to certain `CertificateSigningRequests` based on label -selectors. +specific namespaces. This is a regression in certificate privacy. +Whilst this is a regression in privacy, it could be an accepted limitation where +the real world consequences are minimal. ### Namespaced issuers referenced by CertificateSigningRequest @@ -140,36 +139,57 @@ escalation (Alice references an issuer in Bob's namespace). cert-manager will enforce an [RBAC](https://kubernetes.io/docs/reference/access-authn-authz/rbac/) noun and -verb whereby the requester must have this role bound to them, in order for the +verb whereby the requester must have this role bound to them in order for the `CertificateSigningRequest` referencing a namespaced `Issuer` be approved by the cert-manager controller. See [here](#conditions). This will be done via a [`SubjectAccessReview`](https://github.com/kubernetes/api/blob/4a626d306b987a4096cf0784ec01af1be2f6d67f/authorization/v1/types.go#L52) -against the `UserInfo` fields on the `CertificateSigningRequest`. The required -role must be of `certificates.k8s.io/CertificateSigningRequest` with the verb -`create` within the target namespace. +against the `UserInfo` fields on the `CertificateSigningRequest`. The user must +hold the following permissions to reference a namespaced signer: -Since `Certificates` are also namespaced resources, cert-manager validates that -created `Certificate` resources may only reference `Issuers` in the same -namespace, or `ClusterIssuers`. Any resulting `CertificateSigningRequests` will -reference `Issuers` in the same namespace. +```yaml +apiVersion: rbac.authorization.k8s.io/v1 +kind: Role +metadata: + name: cert-manager-referencer:my-issuer + namespace: sandbox +rules: +- apiGroups: ["cert-manager.io"] + resources: ["signers"] + verbs: ["reference"] + resourceNames: + - "my-issuer" # To give permission to _only_ reference Issuers with the name 'my-issuer' + - "*" # To give permission to reference Issuers with any name in this namespace +``` ### Duration Field -`CertificateSigningRequests` do not include a `duration` field. To have parity -with the `CertificateRequest` resource, the duration field will be moved to an -annotation as `cert-manager.io/duration` which includes a [Go time duration -string](https://golang.org/pkg/time/#Duration.String). This annotation may not -be changed after creation of the `CertificateSigningRequest`. +[Until 1.22](https://github.com/kubernetes/kubernetes/pull/99494) +`CertificateSigningRequests` did not include a `duration` field. To have parity +with the `CertificateRequest` resource, the duration field will be moved to the +annotation `experimental.cert-manager.io/request-duration` who's value is a [Go +time duration string](https://golang.org/pkg/time/#Duration.String). + +When 1.22 is released, cert-manager can optimistically read the +`expirationSeconds` `CertificateSigningRequest` field to discover the requested +duration. If this field hasn't been set or the user is using an older version of +Kubernetes, cert-manager can fallback to this annotation. ### CA Field -`CertificateSigningRequests` do not include a `ca` field. To have parity with -the `CertificateRequest` resource, the `ca` field will be moved to an annotation -as `cert-manager.io/ca`. This annotation value contains the base 64 PEM encoded -CA certificate (if one was returned by the signer). This annotation may not be -changed after creation of the `CertificateSigningRequest`. +`CertificateSigningRequests` do not include a `ca` field. This field has been +proven to be problematic in cert-manager and controversial as it can encourage +TLS anti-patterns. Attempting to add a `ca` style field to +`CertificateSigningRequests` in the form of an annotation also causes issues in +controller reconciliation, since updating the status and metadata fields are +separate endpoints; if a successful status update occurs but the annotation +update fails, the `CertificateSigningRequest` is left in a bad state with no way +of recovery. + +For these reasons, the `ca` field has been omitted with no intention of +including it. Users are advised to share CA data out of band to the +`CertificateSigningRequest` resource. ### External Issuers @@ -188,35 +208,31 @@ signers if and when the migration to `CertificateSigningRequests` occurs. cert-manager will support both `CertificateRequest` and `CertificateSigningRequest` types until, and if, the project completely migrates to the `CertificateSigningRequest` resource. The `Certificates` controller will -continue to create `CertificateRequest` resources, unless a flag is changed on -the cert-manager controller. This default can be switched in future. +continue to create `CertificateRequest` resources. The `Certificates` controller +_could_ be changed in future to be optionally configured to create +`CertificateSigningRequests`. The cert-manager controller will not enable the `CertificateSigningRequest` -controllers unless the resource is available in the running cluster. This can be -checked via the [discovery endpoint of the API -server](https://github.com/kubernetes/client-go/blob/7279fc64d8478bd5f9d38122143b5e6294f06e75/discovery/discovery_client.go#L55). -If the resource is not available, the `CertificateSigningRequest` controllers -should gracefully never start. If the Kubernetes API server were to be upgraded -to a version that does support this resource, cert-manager would need to be -restarted to make use of these controllers. This is acceptable—worker nodes are -typically always restarted during a cluster upgrade. - +controllers unless the `ExperimentalCertificateSigningRequestControllers=true` +environment variable is present on the cert-manager controller. This could be +changed in future by optimistically discovering whether the +`CertificateSigningRequest` resource is available and starting the controllers. 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 must -include the namespace, if the referenced `Issuer` is namespaced. The signer name +include the namespace if the referenced `Issuer` is namespaced. The signer name for cert-manager signers will be prefixed with `cert-manager.io` to prevent conflicts with other external signer projects. ```yaml # Namespaced issuer reference # e.g. `issuers.cert-manager.io/my-namespace.my-issuer - signerName: .cert-manager.io/. + signerName: issuers.cert-manager.io/. # Cluster scoped issuer reference - # e.g. `clusterissuers.cert-manager.iomy-issuer - signerName: .cert-manager.io/ + # e.g. `clusterissuers.cert-manager.io/my-issuer + signerName: clusterissuers.cert-manager.io/ ``` Using the same approach of referencing by _just_ name, rather than issuer type @@ -233,71 +249,71 @@ nothing, else sign. Some special cases for some `[Cluster]Issuers` that need to be addressed: -- SelfSigned: Makes use of annotations, and so these annotations should also be - present on `CertificateSigningRequests`. +- SelfSigned: The `experimental.cert-manager.io/private-key-secret-name` + annotation is used to reference a secret containing the private key of the + self-signed certificate. The Secret must reside in either the namespace of + the reference Issuer, or the cluster resource namespace in the case of a + ClusterIssuer. -- Venafi: Makes use of annotations, and so these annotations should also be - present on `CertificateSigningRequests`. - -- ACME: Makes use of annotations, and so these annotations should also be - present on `CertificateSigningRequests`. +- Venafi: + - The `venafi.experimental.cert-manager.io/custom-fields` annotation is set by + the user to request a Venafi certificate with custom fields. + - The `venafi.experimental.cert-manager.io/pickup-id` annotation is used by + the cert-manager Venafi signer to keep track of the request against the + Venafi API. - ACME: The ACME controller creates sub-resources (`Orders`). Since `CertificateSigningRequests` are cluster scoped resources, when referencing - a `ClusterIssuer` we should create `Orders` in the `Cluster Resource - Namespace` (default `cert-manager`), else the namespace of the referenced - `Issuer`. - + a ClusterIssuer we should create Orders in the cluster resource namespace, + else the namespace of the referenced Issuer. ### Conditions The `CertificateSigningRequest` has [well-known condition types](https://github.com/kubernetes/api/blob/4a626d306b987a4096cf0784ec01af1be2f6d67f/certificates/v1/types.go#L222) -of `Approved`, `Denied`, and `Failed`. All `CertificateSigningRequest` signer +of `Approved`, `Denied`, and `Failed`. `CertificateSigningRequest` signer controllers should not begin computation of the request until the resource has the `Approved` condition set to `True`. -In the absence of further policy, the `Approved` condition is always set t -`True`, _unless_, the `CertificateSigningRequest` references a namespaced -`Issuer` and the requester does not have create permissions for -`certificates.k8s.io/CertificateSigningRequests` in that namespace. In the case -of this namespace validation failing, the `CertificateSigningRequest` `Denied` -condition should be set to `True`, and no signer controllers should process the -request. +It was found that nearly all users testing out these signers forgot about +needing to manually add the Approved condition. An event is fired on +`CertificateSigningRequest` which have neither a Denied or Approved condition. +This is to aid consumers in knowing that the resource _has_ been observed, but +is waiting for the Approved condition. If a signer fails when processing a `CertificateSigningRequest`, the well-known `Failed` condition should be set to `True`. No signer controllers should process the request again. -If the referenced issuer resource doesn't exist or is not in a ready state, the -corresponding `CertificateSigningRequest` controller should set the `Pending` -condition to `True`. - -When the request has been successfully signed, the [cert-manager `Ready` -condition](https://github.com/jetstack/cert-manager/blob/969b678f330c68a6429b7a71b271761c59651a85/pkg/apis/certmanager/v1/types_certificaterequest.go#L139) -should be set to `True` on the resource, to match the same behaviour as -`CertificateRequests`. - +When the request has been successfully signed, the `status.Certificate` field +will be updated with the sign certificate. ### API Changes -* The `cert-manager.io/duration` annotation on `CertificateSigningRequests` - denotes the requested duration as a Go duration string. +- IsCA: The `experimental.cert-manager.io/request-is-ca` annotation is used to + request a certificate with the X.509 `IsCA` flag set. -* The `cert-manager.io/ca` annotation on `CertificateSigningRequests` contains - the returned base 64 PEM encoded CA certificate from the signer. +- Duration: The `experimental.cert-manager.io/request-duration` annotation is used to + request a certificate with a particular duration. Accepts a Go time duration + string. -* The requester of a `CertificateSigningRequest` must have `create` permissions - for `certificates.k8s.io/CertificateSigningRequests` in the same namespace - the referenced issuer resides. - -* The [Approval - Condition](https://github.com/kubernetes/api/blob/4a626d306b987a4096cf0784ec01af1be2f6d67f/certificates/v1/types.go#L222) - will be replicated on the `CertificateRequest` resource. This condition will - _always_ be set to true, in the absence of policy. All `CertificateRequest` - controllers must wait until the `Approved` condition is set to `True` before - beginning computation. +- Namespaced issuers: requesters must have the following RBAC in the same + namespace to allow them to request from a namespaced issuer: +```yaml +apiVersion: rbac.authorization.k8s.io/v1 +kind: Role +metadata: + name: cert-manager-referencer:my-issuer + namespace: sandbox +rules: +- apiGroups: ["cert-manager.io"] + resources: ["signers"] + verbs: ["reference"] + resourceNames: + - "my-issuer" # To give permission to _only_ reference Issuers with the name 'my-issuer' + - "*" # To give permission to reference Issuers with any name in this namespace +``` ### Upgrading