From aeda0f23c0c4b8d80df86fdd88d149f76e145cb8 Mon Sep 17 00:00:00 2001 From: joshvanl Date: Thu, 11 Feb 2021 17:22:12 +0000 Subject: [PATCH] Updates design document from comments Signed-off-by: joshvanl --- .../20210203.certificate-request-identity.md | 64 +++++++++++++------ 1 file changed, 44 insertions(+), 20 deletions(-) diff --git a/design/20210203.certificate-request-identity.md b/design/20210203.certificate-request-identity.md index 728615da0..70c336b08 100644 --- a/design/20210203.certificate-request-identity.md +++ b/design/20210203.certificate-request-identity.md @@ -55,11 +55,11 @@ anti-pattern to make runtime decisions on historical audit logs. The upstream [Kubernetes certificates `CertificateSigningRequest`](https://github.com/kubernetes/api/blob/master/certificates/v1/types.go#L41) -API has identity as part of the resource. As the project intends to align and -likely make this resource compatible with cert-manager in some way, the -`CertificateRequest` should expose identical identity details. This means -extensions or additions to cert-manager (policy, auditing) can have the same -guarantees about what information is available from both resources. +API has the identity of the requesting Kubernetes user. As the project intends +to transition to this resource as part of the project, the `CertificateRequest` +should strive to match 1:1 wherever possible. This means extensions or additions +to cert-manager (policy, auditing) can have the same guarantees about what +information is available from both resources, while this transition takes place. ### Goals @@ -73,37 +73,49 @@ guarantees about what information is available from both resources. ### Non-Goals -- Discuss how this identity can be used (policy, auditing) -- Make changes to upstream Kubernetes to implement identiy in cert-manager -- Although considered below, "passed down" identity is not part of this design +- Dictate any kind of means through which identity or policy evaluation should + be performed (rather, only the building blocks to enable others to build + evaluation systems is a goal) +- Make changes to upstream Kubernetes to implement identity in cert-manager +- Although considered below, ["passed down"](#certificate-identity-pass-down) + identity is not part of this design ## Proposal Kubernetes does [not currently support immutable fields](https://github.com/kubernetes/enhancements/blob/8b9b994136371f1bc938aabf012f4c45535d684c/keps/sig-api-machinery/20190603-immutable-fields.md) for CRDs. The cert-manager webhook will be responsible for populating and -enforcing identity fields which are present on the spec of `CertificateRequest` +enforcing user info fields which are present on the spec of `CertificateRequest` resources. The webhook will be responsible for enforcing the following during a CREATE -operation: -- No identity fields have been set by the user creating the resource -- Set identity fields to exactly what is received from the API server in the +operation. We will not reject requests which populate these fields, but instead +simply override them. +- No user info fields have been set by the [user creating the resource]( + https://github.com/kubernetes/kubernetes/blob/7a94debba5f8c21bbf8b42b2a7f1d5e974ddb837/pkg/registry/certificates/certificates/strategy.go#L63-L79) +- Set user info fields to exactly what is received from the API server in the [UserInfo](https://github.com/kubernetes/api/blob/master/authentication/v1/types.go#L102) The webhook will also responsible for enforcing the following during an UPDATE -operation: -- No changes to identity fields are allowed to be made +operation. Any attempt to changes these fields will result in a rejected +request. +- No changes to user info fields are allowed to be made ### API Changes -In order to expose the identity of who created `CertificateRequest` resources, +In order to expose the user info of who created `CertificateRequest` resources, these resources must be updated to have parity with the upstream Kubernetes certificates API. This means that the `CertificateRequest` API type be updated to include the following fields in spec, for all API versions: ```go +type CertificateRequestSpec { + // EXISTING FIELDS + // ... + + // NEW FIELDS + // Username contains the name of the user that created the CertificateRequest. // Populated by the cert-manager webhook on creation and immutable. // +optional @@ -123,15 +135,23 @@ following fields in spec, for all API versions: Extra map[string][]string `json:"extra,omitempty"` ``` +All new fields here are marked as optional. It is likely for a number of them to +be empty for a given request, depending on the requester's identity. It is up to +any component consuming the user info fields to make appropriate decisions about +fields which are not populated. + ### Upgrading Any `CertificateRequest`s that are present in the cluster at the time of -upgrade, won't and will never have their identity fields populated. This should +upgrade, won't and will never have their user info fields populated. This should be acceptable. All subsequent `CertificateRequest`s created after this upgrade will have their identities populated. +When the CRDs are upgraded, there will be a brief period where the user info +fields will not be populated until the webhook is also upgraded. + ### Test Plan Unit tests will be created is ensure that the [properties](#proposal) are @@ -142,19 +162,23 @@ resources have their respective identities populated in the fields. ### Risks and Mitigations -There are large security implications if the identity fields that other +There are large security implications if the user info fields that other components rely on are wrong, or could be made fraudulent. Special care when testing needs to be given to ensure the properties described above are correct and enforced. +If the validating or mutating webhooks were not installed properly, either by +being out of date or deleted, then this would invalidate the security guarantees +of the user info fields. + ### Certificate Identity Pass Down `CertificateRequest`s created and managed via `Certificate` resources will have -the identity of the cert-manager controller. Though this design doesn't provide -a solution to components that need to traverse the identity to the original +the user info of the cert-manager controller. Though this design doesn't provide +a solution to components that need to traverse the user info to the original user who created the `Certificate` resource, some possible further design options are; -- cert-manager would override the identity fields with the of the identity that +- cert-manager would override the user info fields with the of the identity that created the `Certificate` resource, if cert-manager is the creator. - Consumers of the identity will need to be aware that the `CertificateRequest` may be managed by a `Certificate` resource if it is created by cert-manager,