From f53ff6e2950e192a0be07feb64393ee3f4a574e1 Mon Sep 17 00:00:00 2001 From: Thejas N Date: Fri, 23 Dec 2022 15:13:19 +0530 Subject: [PATCH 01/11] CFE-704: Add secret injection to Route API for external certificate management This enhancement extends the Route API to provide support for providing a secret reference instead of adding certificate data directly into the Route spec. --- ...ion-for-external-certificate-management.md | 441 ++++++++++++++++++ 1 file changed, 441 insertions(+) create mode 100644 enhancements/ingress/route-secret-injection-for-external-certificate-management.md diff --git a/enhancements/ingress/route-secret-injection-for-external-certificate-management.md b/enhancements/ingress/route-secret-injection-for-external-certificate-management.md new file mode 100644 index 0000000000..99dbb96848 --- /dev/null +++ b/enhancements/ingress/route-secret-injection-for-external-certificate-management.md @@ -0,0 +1,441 @@ +--- +title: route-secret-injection-for-external-certificate-management +authors: + - '@thejasn' +reviewers: + - '@Miciah' + - '@alebedev87' + - '@tgeer' + - '@joelspeed' + - '@deads2k' +approvers: + - '@Miciah' +api-approvers: # In case of new or modified APIs or API extensions (CRDs, aggregated apiservers, webhooks, finalizers). If there is no API change, use "None" + - '@joelspeed' +creation-date: 2022-12-13 +last-updated: 2023-01-27 +tracking-link: # link to the tracking ticket (for example: Jira Feature or Epic ticket) that corresponds to this enhancement + - https://issues.redhat.com/browse/CFE-704 +--- + +# Route Secret Injection For External Certificate Management + +## Summary + +Currently, users of OpenShift cannot very easily integrate OpenShift Routes +with third-party certificate management solutions like [cert-manager](https://github.com/cert-manager/cert-manager). +And this is mainly due to the design of Routes API which deliberately requires the certificate +data to be present in the Route object as opposed to having a reference. This is especially +problematic when third-party solutions also manage the life cycle (create/renew/delete) +of the generated certificates which OpenShift Routes does not support and requires +manual intervention to manage certificate life cycle. + +This enhancement aims to provide a solution where OpenShift Routes can support +integration with third-party certificate management solutions like cert-manager and +avoid manual certificate management by the user which is more error prone. + +## Motivation + +OpenShift customers currently manually manage certificates for user workloads +by updating OpenShift Routes with the updated certificate data during expiry/renew +workflow. This is cumbersome activity if users have a huge number of workloads and +is also error prone. + +This enhancement adds the support to OpenShift Routes for third-party certificate +management solutions like cert-manager. + +### User Stories + +- As an end user of Route API, I want to be able to provide a tls secret reference on + OpenShift Routes so that I can integrate with third-party certificate management solution + +- As an OpenShift cluster administrator, I want to use third-party solutions like cert-manager + for certificate management of user workloads on OpenShift so that no manual process is + required to renew expired certificates. + +- As an end user of Route API, I want OpenShift Routes to support both manual and managed + mode of operation for certification management so that I can switch between manual certificate + management and third-party certificate management. + +- As an OpenShift engineer, I want to be able to update Route API so that I can integrate + OpenShift Routes with third-party certificate management solutions like cert-manager. + +- An an OpenShift engineer, I want to be able to watch routes and process routes having valid + annotations so that the certificate data is injected into the route CR. + +- As an OpenShift engineer, I want to be able to run e2e tests as part of openshift/origin + so that testcases are executed to signal feature health by CI executions. + +### Goals + +- Provide users with a configurable option in Route API to provide externally managed certificate data. +- Provide users with a mechanism to switch between using externally managed certificates and + manually managed certificates on OpenShift routes and vice-versa. +- Provide smooth roll out of new certificates on OpenShift router when managed certificates + are renewed. +- Provide latest certificate type information on the Route. + +### Non-Goals + +- Provide certificate life cycle management controls on the Route API (expiryAfter, renewBefore, etc). +- Provide migration of certificates from current solution. + +## Proposal + +This enhancement proposes introducing a new controller (secret-injector) in [route-controller-manager](https://github.com/openshift/route-controller-manager) +to manage a new annotation (secret-reference) on the Route object. This annotation +enables the users to provide a reference to a Secret containing the serving cert/key +pair that will be injected to `.spec.tls` and will be served by OpenShift router. +This annotation will be given a higher preference if route CR also has `.spec.tls.certificate` +and `.spec.tls.key` fields set. + +### Workflow Description + +End users have 2 possible variations for the creation of the route: + +- Create a route for the user workload and this is completely managed by the end user. +- Create an ingress for the user workload and a managed route is created automatically + by the ingress-to-route controller. + +Both these workflows will support integrating with third party certificate management +solutions like cert-manager with the secret-reference annotation described under [API Extensions](#api-extensions). + +**When a Route is directly used to expose user workload** + +- The end user must have generated the serving certificate as a pre requisite + using third-party systems like cert-manager. +- In cert-manager's case, the [Certificate](https://cert-manager.io/docs/usage/certificate/#creating-certificate-resources) CR must be created in the same namespace + where the Route is going to be created. +- To expose a user workload, the user would create a new Route with the + secret-reference annotation referencing the generated secret that was created + in the previous step. +- If the secret that is referenced exists and has a successfully generated + cert/key pair, the secret-injector controller copies the certificate data + into the route at `.spec.tls.certificate` and `.spec.tls.key`. + +**When an Ingress is used to expose user workload** + +- The end user must have generated the serving certificate as a pre requisite + using third-party systems like cert-manager. +- In cert-manager's case, the [Certificate](https://cert-manager.io/docs/usage/certificate/#creating-certificate-resources) CR must be created in the same namespace + where the Ingress is going to be created. +- To expose a user workload, a new Ingress with the generated secret + referenced in `.spec.tls.secretName` needs to be created. +- If the secret CR that is referenced exists and has a successfully generated + cert/key pair, the ingress-to-route controller copies this certificate data + into the route at `.spec.tls.certificate` and `.spec.tls.key`. + +_Note_: The same workflow would apply to updating an existing Route to integrate +with cert-manager. The new certificate data will overwrite the `.spec.tls.certificate` +and `.spec.tls.key` in the route CR since the secret-reference annotation gets a +higher preference. + +#### Certificate Life cycle + +Post integrating with third-party solutions like cert-manager, the end user can also +disable automatic certificate management. This can be done in 2 ways, + +- The end user can delete the secret-reference annotation on the Route. +- The end user deletes the secret that was associated with the Certificate CR. + +**Deleting the secret-reference annotation on the Route** +The end user can delete the annotation on the Route, this will result +in clearing `.spec.tls.certificate` and `.spec.tls.key` and resort to +using the default certificates that are generated by the cluster-ingress-operator. + +**Deleting the generated secret that contains the serving cert/key pair** +The end user can delete the secret containing the certificate data, this will result +in clearing `.spec.tls.certificate` and `.spec.tls.key` and resort to using the +default certificates that are generated by the cluster-ingress-operator. Also the secret-reference +annotation that was added by the user will not be deleted by the secret-injector controller. + +#### Variation [optional] + +N.A + +### API Extensions + +A secret-reference annotation of type `string` to be introduced as part of the integration +to support third-party certificate management systems, + +```yaml +annotations: + route.openshift.io/tls-secret-name: +``` + +_Note_: The default value would be N/A. The secret is required to be created +in the same namespace as that of the Route. The secret must be of type +`kubernetes.io/tls` and the tls.key and the tls.crt key must be provided in +the `data` (or `stringData`) field of the Secret configuration. + +This annotation can be applied by the end user on Route. The controller that +will be introduced as part of this enhancement will be responsible for the +processing of this annotation on the Route. + +The Route API will also be updated to denote certificate status under `.status`, + +```go + +// RouteStatus provides relevant info about the status of a route, including which routers +// acknowledge it. +type RouteStatus struct { + // ... + + // CertificateConditions describes the type of certificate that is being served + // by the router(s) associated with this route. + Certificate []RouteCertificateCondition `json:"certificate,omitempty" protobuf:"bytes,2,rep,name=certificate"` +} + +// RouteCertificateConditionType is a valid value forRouteCertificateCondition +type RouteCertificateConditionType string + +// RouteCertificateCondition contains details of the certificate being served by routers associated with +// this route. +type RouteCertificateCondition struct { + // Type is the type of the condition. + // Possible values include Default, Custom and Managed. + Type RouteCertificateConditionType `json:"type" protobuf:"bytes,1,opt,name=type,casttype=RouteCertificateConditionType"` + // Status is the status of the condition. + // Can be True, False, Unknown. + Status corev1.ConditionStatus `json:"status" protobuf:"bytes,2,opt,name=status,casttype=k8s.io/api/core/v1.ConditionStatus"` + // (brief) reason for the condition's last transition, and is usually a machine and human + // readable constant + Reason string `json:"reason,omitempty" protobuf:"bytes,3,opt,name=reason"` + // Human readable message indicating details about last transition. + Message string `json:"message,omitempty" protobuf:"bytes,4,opt,name=message"` + // RFC 3339 date and time when this condition last transitioned + LastTransitionTime *metav1.Time `json:"lastTransitionTime,omitempty" protobuf:"bytes,5,opt,name=lastTransitionTime"` +} + +const ( + // DefaultCertificate denotes that the user has not provided + // any custom certificate and the default generated certificate is being + // served on the route. + DefaultCertificate RouteCertificateConditionType = "Default" + + // CustomCertificate denotes that there is a custom certificate + // being served on the route. + CustomCertificate RouteCertificateConditionType = "Custom" + + // ManagedCertificate denotes that the route.openshift.io/tls-secret-name + // annotation is applied and is used to inject a third-party managed secret + // containing the certificate data that is being served on the route. + ManagedCertificate RouteCertificateConditionType = "Managed" +) + +``` + +#### Variation + +##### Alternative to the tls-secret-reference annotation + +**_Note_**: This idea has been dropped in favor of a secret reference through an annotation. + +The alternative to the annotation was to provide a new API field in the Route API, +through which the user could provide the certificate reference. + +```go +type CertificateReference struct { + // certificate resource name. + // +optional + Name string `json:"name,omitempty" protobuf:"bytes,1,opt,name=name"` +} + +Type RouteSpec struct { + // ... + + CertificateRef CertificateReference `json:"certificateRef,omitempty" protobuf:"bytes,5,opt,name=certificateRef"` +} +``` + +The reasoning for the field was mainly for future proofing the API to handle integrations +with other third-party certificate management systems. But this API introduces inconsistency +between how Ingresses are supported by cert-manager via `.spec.tls.secretName` and when using +Routes, the user would have to provide the Certificate CR reference. + +This also adds coupling with external third-party API(s) and limits future enhancement to be +a more generic solution for integrating with third-party certificate management systems. + +##### Alternative to new status condition types + +An alternative to introducing 3 new condition types, would be to use `metadata.managedFields` +on Route to denote to the user which fields are managed by the secret-injector controller and +server-side apply would be used by the controller during updates to ensure that `.spec.tls.certificate` +and `.spec.tls.secret` are always managed by the controller as long as the annotation is present. + +This although helps in avoiding addition of new condition types will result in a few inconsistencies, + +- When transitioning from using managed certificates to manually managing custom certificates, + the `.managedFields` will still depict `.spec.tls` as managed by the controller since the update + operation done by the user is only on the `.metadata.annotations`. +- Also since `oc` defaults to `--server-side=false`, mixing CSA and SSA results in ambiguous `.managedFields`. + +### Implementation Details/Notes/Constraints [optional] + +The new controller (secret-injector) will be responsible for watching routes and processing those +that have the valid annotation for `tls-secret-name`. This controller will also update +`.status` of the route to contain the latest information on the certificate in use. + +Since the secrets are the primary resource for the secret-injector controller, the new controller +will also set up watches and re-sync routes in the same namespace as that of the secret (similar +to ingress-to-route controller). + +The new controller will watch all routes and process all routes in order to update +`.status.certificate`. The ones with the annotation will have additional +logic to update `.spec.tls.certificate` and `.spec.tls.key` based on the following pre-conditions, + +- The secret-reference annotation is present. +- Secret mentioned in the annotation exists and is valid (contains the required fields mentioned [here](#api-extensions)) + +In scenarios where the user deletes the annotation, it is the controller's +responsibility to reset `.spec.tls` and this will be driven based on `.status` i.e. +the latest certificate status condition has to be `ManagedCertificate` and since the required annotation is +not present this results in the `.spec.tls.certificate` and `.spec.tls.key` being cleared +out and the `.status` updated to `DefaultCertificate`. + +If the user deletes the secret associated with the Route without deleting the +`route.openshift.io/tls-secret-name` annotation, the secret-injector controller will +clear out `.spec.tls.certificate` and `.spec.tls.key` since the following pre-conditions +are met, + +- The secret-reference annotation is present. +- Secret mentioned in the annotation doesn't exist +- The latest condition on Route's `.status.certificate` has `ManagedCertificate`=`True` + +This results in the router using the default certificates that are generated and +the `.status` updated as well. + +As a follow up to the above scenario when the secret-reference annotation added by the user is retained +by the secret-injector controller and because the referenced secret is not present, +the controller will publish an `Event` so that the end user is notified regarding this +switch to using the default generated certificates. The `.status` is also updated to `DefaultCertificate`. + +When transitioning from using a managed certificate to manually managed certificate, the user +is expected to delete the secret-reference annotation and provide new certificate/key pair under `.spec.tls`. +If the previously used `.spec.tls.certificate` and `.spec.tls.key` is not replaced +by the user, the secret-injector controller will clear `.spec.tls.certificate` and `.spec.tls.key` +and route will serve the default certificates that are generated by cluster-ingress-operator. +If the `.spec.tls.certificate` and `.spec.tls.key` are replaced as well, then the +controller will update the `.status.certificate` to `CustomCertificate` + +The new secret-injector controller will not reconcile Routes that are owned by the +ingress-to-route controller. + +### Risks and Mitigations + +N.A + +### Drawbacks + +This introduces an inconsistency between Ingress and Route CRD with respect to how +a secret reference can be provided with the certificate data. The ingress has a field +`.spec.tls.secretName` where as the Route will have an annotation. + +## Design Details + +### Open Questions [optional] + +1. Does this warrant making changes to the ingress-to-route controller in how + `.spec.tls.secretName` is processed? Currently, the controller reads the secret + data and copies it over to the Route that is created. Since this enhancement + introduces a difference between managed certificate provided via secrets and + manually provided secrets, this distinction isn't possible with Routes created + via Ingresses. [CLOSED] + + - Proposed change + The ingress-to-route controller will also process the secret-reference annotation and if + present copies it over to the route that is created. This annotation will take + precedence over `.spec.tls.secretName` and this offers enough distinction between + third-party/manual certificate management. + +**Answer**: The behaviour of the ingress-to-route controller will be retained as is and +no changes will be done. + +2. How/where to implement e2e tests for [route-controller-manager](https://github.com/openshift/route-controller-manager)? + [CLOSED] + +**Answer**: The e2e tests will be added to openshift/origin. + +### Test Plan + +This enhancement will be tested in isolation of cert-manager-operator as part of +core OCP payload. This will also be tested with cert-manager-operator as part of the +operators e2e test suite. + +1. Test Route API without interfacing with Ingresses + + a. Create a edge terminated route that is using default certificates. + b. Update route with the secret-reference annotation and ensure the `.spec.tls` and `.status` + is updated. + c. Verify that the associated openshift routers are also serving the updated + certificates and not the default certificates. + d. Update certificate data and verify if the certificates served by the router + are updated as well. + d. Delete the annotation and ensure that the route `.spec.tls` and `.status` + denotes the usage of default certificates. Also verify the associated + openshift router is serving the default certificates. + +Various other transitions, `Custom`->`Managed`-`Custom` will also be tested. + +### Graduation Criteria + +This is a user facing change and will directly go to GA. This feature requires an +update to Openshift Docs. + +#### Dev Preview -> Tech Preview + +N/A. This feature will go directly to GA. + +#### Tech Preview -> GA + +N/A. This feature will go directly to GA. + +#### Removing a deprecated feature + +N/A. + +### Upgrade / Downgrade Strategy + +On downgrades, all routes using `route.openshift.io/tls-secret-name` annotation will continue to use the custom certificates +indefinitely unless the route is manually edited and the `.spec.tls` is updated. + +### Version Skew Strategy + +This enhancement isn't affected by version skew of core Kubernetes components +during updates/upgrades. The new controller will be capable to re-synchronize +the required components if required and without this controller the annotation +would basically do nothing. + +### Operational Aspects of API Extensions + +N.A + +#### Failure Modes + +When using routes with third-party certificate management solutions like cert-manager, this +adds a hard dependency in order of operation. In scenarios where the secret has not been created +by third-party solutions like cert-manager, the route would not be successfully created due +to the dependency on the route. + +> A solution to this could be that upon failure to use the secret due to various reasons, +> the controller defaults to using the default generated certificates and updating the route +> status appropriately. The controller also publishes an event indicating the switch has been made. + +#### Support Procedures + +The new introduced `RouteCertificateConditionType` types will provide the required information into +the current state of route objects with respect to certificates. While using third-party solutions +like cert-manager for certificate management the `RouteCertificateCondition` will indicate which type +of certificate is associated with the route. + +## Implementation History + +N.A + +## Alternatives + +N.A + +## Infrastructure Needed [optional] + +N.A From 4134d5f2b347f721f02f43ffc97f0740693c18b2 Mon Sep 17 00:00:00 2001 From: Thejas N Date: Wed, 26 Apr 2023 13:20:44 +0530 Subject: [PATCH 02/11] Updated proposal to use a secret refernce field --- ...ion-for-external-certificate-management.md | 365 ++++++------------ 1 file changed, 108 insertions(+), 257 deletions(-) diff --git a/enhancements/ingress/route-secret-injection-for-external-certificate-management.md b/enhancements/ingress/route-secret-injection-for-external-certificate-management.md index 99dbb96848..da7c67b9c5 100644 --- a/enhancements/ingress/route-secret-injection-for-external-certificate-management.md +++ b/enhancements/ingress/route-secret-injection-for-external-certificate-management.md @@ -13,9 +13,9 @@ approvers: api-approvers: # In case of new or modified APIs or API extensions (CRDs, aggregated apiservers, webhooks, finalizers). If there is no API change, use "None" - '@joelspeed' creation-date: 2022-12-13 -last-updated: 2023-01-27 +last-updated: 2023-04-24 tracking-link: # link to the tracking ticket (for example: Jira Feature or Epic ticket) that corresponds to this enhancement - - https://issues.redhat.com/browse/CFE-704 + - https://issues.redhat.com/browse/CM-16 --- # Route Secret Injection For External Certificate Management @@ -42,7 +42,8 @@ workflow. This is cumbersome activity if users have a huge number of workloads a is also error prone. This enhancement adds the support to OpenShift Routes for third-party certificate -management solutions like cert-manager. +management solutions like cert-manager by extending the Route API to read the serving +certificate data via a secret reference. ### User Stories @@ -57,37 +58,32 @@ management solutions like cert-manager. mode of operation for certification management so that I can switch between manual certificate management and third-party certificate management. +- As an Openshift engineer, I want to be update the router so that it is able read secrets directly + if all the preconditions have been met by the router serviceaccount. + - As an OpenShift engineer, I want to be able to update Route API so that I can integrate OpenShift Routes with third-party certificate management solutions like cert-manager. -- An an OpenShift engineer, I want to be able to watch routes and process routes having valid - annotations so that the certificate data is injected into the route CR. - - As an OpenShift engineer, I want to be able to run e2e tests as part of openshift/origin so that testcases are executed to signal feature health by CI executions. ### Goals -- Provide users with a configurable option in Route API to provide externally managed certificate data. -- Provide users with a mechanism to switch between using externally managed certificates and - manually managed certificates on OpenShift routes and vice-versa. -- Provide smooth roll out of new certificates on OpenShift router when managed certificates - are renewed. -- Provide latest certificate type information on the Route. +- Provide users with a configurable option in Route API to reference externally managed certificates via secrets. ### Non-Goals - Provide certificate life cycle management controls on the Route API (expiryAfter, renewBefore, etc). -- Provide migration of certificates from current solution. +- Provide smooth roll out of new certificates on OpenShift router when referenced certificates + are renewed (secret containing the certificate is updated). ## Proposal -This enhancement proposes introducing a new controller (secret-injector) in [route-controller-manager](https://github.com/openshift/route-controller-manager) -to manage a new annotation (secret-reference) on the Route object. This annotation -enables the users to provide a reference to a Secret containing the serving cert/key -pair that will be injected to `.spec.tls` and will be served by OpenShift router. -This annotation will be given a higher preference if route CR also has `.spec.tls.certificate` -and `.spec.tls.key` fields set. +This enhancement proposes extending the openshift/router to read serving certificate data either +from the Route `.spec.tls.certificate` and `.spec.tls.key` or from a new field `.spec.tls.certificateRef` +which is a `kubernetes.io/tls` type secret reference. This `certificateRef` field will enables the +users to provide a reference to a Secret containing the serving cert/key pair that will be parsed +and served by OpenShift router. ### Workflow Description @@ -95,7 +91,7 @@ End users have 2 possible variations for the creation of the route: - Create a route for the user workload and this is completely managed by the end user. - Create an ingress for the user workload and a managed route is created automatically - by the ingress-to-route controller. + by the ingress-to-route controller. [Depends on the open question] Both these workflows will support integrating with third party certificate management solutions like cert-manager with the secret-reference annotation described under [API Extensions](#api-extensions). @@ -104,14 +100,22 @@ solutions like cert-manager with the secret-reference annotation described under - The end user must have generated the serving certificate as a pre requisite using third-party systems like cert-manager. -- In cert-manager's case, the [Certificate](https://cert-manager.io/docs/usage/certificate/#creating-certificate-resources) CR must be created in the same namespace - where the Route is going to be created. +- In cert-manager's case, the [Certificate](https://cert-manager.io/docs/usage/certificate/#creating-certificate-resources) + CR must be created in the same namespace where the Route is going to be created. +- The end user must create a role and in the same namespace as the secret containing the certificate from earlier, + ```bash + oc create role secret-reader --verb=get,list,watch --resource=secrets --resourceName= + ``` +- The end user must create a rolebinding in the same namespace as the secret + and bind the router serviceaccount to the above created role. + ```bash + oc create rolebinding foo-secret-reader --role=secret-reader --serviceaccount=openshift-ingress:router --namespace= + ``` - To expose a user workload, the user would create a new Route with the - secret-reference annotation referencing the generated secret that was created + `.spec.tls.certificateRef` referencing the generated secret that was created in the previous step. - If the secret that is referenced exists and has a successfully generated - cert/key pair, the secret-injector controller copies the certificate data - into the route at `.spec.tls.certificate` and `.spec.tls.key`. + cert/key pair, the router will serve this certificate if all preconditions are met. **When an Ingress is used to expose user workload** @@ -122,32 +126,8 @@ solutions like cert-manager with the secret-reference annotation described under - To expose a user workload, a new Ingress with the generated secret referenced in `.spec.tls.secretName` needs to be created. - If the secret CR that is referenced exists and has a successfully generated - cert/key pair, the ingress-to-route controller copies this certificate data - into the route at `.spec.tls.certificate` and `.spec.tls.key`. - -_Note_: The same workflow would apply to updating an existing Route to integrate -with cert-manager. The new certificate data will overwrite the `.spec.tls.certificate` -and `.spec.tls.key` in the route CR since the secret-reference annotation gets a -higher preference. - -#### Certificate Life cycle - -Post integrating with third-party solutions like cert-manager, the end user can also -disable automatic certificate management. This can be done in 2 ways, - -- The end user can delete the secret-reference annotation on the Route. -- The end user deletes the secret that was associated with the Certificate CR. - -**Deleting the secret-reference annotation on the Route** -The end user can delete the annotation on the Route, this will result -in clearing `.spec.tls.certificate` and `.spec.tls.key` and resort to -using the default certificates that are generated by the cluster-ingress-operator. - -**Deleting the generated secret that contains the serving cert/key pair** -The end user can delete the secret containing the certificate data, this will result -in clearing `.spec.tls.certificate` and `.spec.tls.key` and resort to using the -default certificates that are generated by the cluster-ingress-operator. Also the secret-reference -annotation that was added by the user will not be deleted by the secret-injector controller. + cert/key pair, the ingress-to-route controller adds this secret name to + the created route `.spec.tls.certificateRef`. #### Variation [optional] @@ -155,240 +135,111 @@ N.A ### API Extensions -A secret-reference annotation of type `string` to be introduced as part of the integration -to support third-party certificate management systems, - -```yaml -annotations: - route.openshift.io/tls-secret-name: -``` - -_Note_: The default value would be N/A. The secret is required to be created -in the same namespace as that of the Route. The secret must be of type -`kubernetes.io/tls` and the tls.key and the tls.crt key must be provided in -the `data` (or `stringData`) field of the Secret configuration. - -This annotation can be applied by the end user on Route. The controller that -will be introduced as part of this enhancement will be responsible for the -processing of this annotation on the Route. - -The Route API will also be updated to denote certificate status under `.status`, +A `.spec.tls.certificateRef` field is added to Route `.spec.tls` which can be used to provide a secret name +containing the certificate data instead of using `.spec.tls.certificate` and `spec.tls.key`. ```go +type TLSConfig struct { + // ... -// RouteStatus provides relevant info about the status of a route, including which routers -// acknowledge it. -type RouteStatus struct { - // ... - - // CertificateConditions describes the type of certificate that is being served - // by the router(s) associated with this route. - Certificate []RouteCertificateCondition `json:"certificate,omitempty" protobuf:"bytes,2,rep,name=certificate"` -} - -// RouteCertificateConditionType is a valid value forRouteCertificateCondition -type RouteCertificateConditionType string - -// RouteCertificateCondition contains details of the certificate being served by routers associated with -// this route. -type RouteCertificateCondition struct { - // Type is the type of the condition. - // Possible values include Default, Custom and Managed. - Type RouteCertificateConditionType `json:"type" protobuf:"bytes,1,opt,name=type,casttype=RouteCertificateConditionType"` - // Status is the status of the condition. - // Can be True, False, Unknown. - Status corev1.ConditionStatus `json:"status" protobuf:"bytes,2,opt,name=status,casttype=k8s.io/api/core/v1.ConditionStatus"` - // (brief) reason for the condition's last transition, and is usually a machine and human - // readable constant - Reason string `json:"reason,omitempty" protobuf:"bytes,3,opt,name=reason"` - // Human readable message indicating details about last transition. - Message string `json:"message,omitempty" protobuf:"bytes,4,opt,name=message"` - // RFC 3339 date and time when this condition last transitioned - LastTransitionTime *metav1.Time `json:"lastTransitionTime,omitempty" protobuf:"bytes,5,opt,name=lastTransitionTime"` -} - -const ( - // DefaultCertificate denotes that the user has not provided - // any custom certificate and the default generated certificate is being - // served on the route. - DefaultCertificate RouteCertificateConditionType = "Default" - - // CustomCertificate denotes that there is a custom certificate - // being served on the route. - CustomCertificate RouteCertificateConditionType = "Custom" - - // ManagedCertificate denotes that the route.openshift.io/tls-secret-name - // annotation is applied and is used to inject a third-party managed secret - // containing the certificate data that is being served on the route. - ManagedCertificate RouteCertificateConditionType = "Managed" -) - -``` - -#### Variation - -##### Alternative to the tls-secret-reference annotation - -**_Note_**: This idea has been dropped in favor of a secret reference through an annotation. - -The alternative to the annotation was to provide a new API field in the Route API, -through which the user could provide the certificate reference. + // certificateRef provides certificate contents as a secret reference. + // This should be a single serving certificate, not a certificate + // chain. Do not include a CA certificate. -```go -type CertificateReference struct { - // certificate resource name. + // + // +kubebuilder:validation:Optional + // +openshift:enable:FeatureSets=TechPreviewNoUpgrade // +optional - Name string `json:"name,omitempty" protobuf:"bytes,1,opt,name=name"` -} - -Type RouteSpec struct { - // ... - - CertificateRef CertificateReference `json:"certificateRef,omitempty" protobuf:"bytes,5,opt,name=certificateRef"` + CertificateRef *corev1.LocalObjectReference `json:"certificateRef,omitempty" protobuf:"bytes,7,opt,name=certificateRef"` } ``` -The reasoning for the field was mainly for future proofing the API to handle integrations -with other third-party certificate management systems. But this API introduces inconsistency -between how Ingresses are supported by cert-manager via `.spec.tls.secretName` and when using -Routes, the user would have to provide the Certificate CR reference. - -This also adds coupling with external third-party API(s) and limits future enhancement to be -a more generic solution for integrating with third-party certificate management systems. +_Note_: The default value would be `nil`. The secret is required to be created +in the same namespace as that of the Route. The secret must be of type +`kubernetes.io/tls` and the tls.key and the tls.crt key must be provided in +the `data` (or `stringData`) field of the Secret configuration. -##### Alternative to new status condition types +If neither `.spec.tls.certificateRef` or `.spec.tls.certificate` and `.spec.tls.key` are +provided the router will serve the default generated secret. -An alternative to introducing 3 new condition types, would be to use `metadata.managedFields` -on Route to denote to the user which fields are managed by the secret-injector controller and -server-side apply would be used by the controller during updates to ensure that `.spec.tls.certificate` -and `.spec.tls.secret` are always managed by the controller as long as the annotation is present. +All valid and invalid scenarios will be depicted via the existing `RouteIngressCondition`. -This although helps in avoiding addition of new condition types will result in a few inconsistencies, +#### Variation -- When transitioning from using managed certificates to manually managing custom certificates, - the `.managedFields` will still depict `.spec.tls` as managed by the controller since the update - operation done by the user is only on the `.metadata.annotations`. -- Also since `oc` defaults to `--server-side=false`, mixing CSA and SSA results in ambiguous `.managedFields`. +N.A ### Implementation Details/Notes/Constraints [optional] -The new controller (secret-injector) will be responsible for watching routes and processing those -that have the valid annotation for `tls-secret-name`. This controller will also update -`.status` of the route to contain the latest information on the certificate in use. - -Since the secrets are the primary resource for the secret-injector controller, the new controller -will also set up watches and re-sync routes in the same namespace as that of the secret (similar -to ingress-to-route controller). - -The new controller will watch all routes and process all routes in order to update -`.status.certificate`. The ones with the annotation will have additional -logic to update `.spec.tls.certificate` and `.spec.tls.key` based on the following pre-conditions, - -- The secret-reference annotation is present. -- Secret mentioned in the annotation exists and is valid (contains the required fields mentioned [here](#api-extensions)) +The router will read the secret referenced in `.spec.tls.certificateRef` if present and +if the following pre-conditions are met it uses this certificate us configure haproxy. -In scenarios where the user deletes the annotation, it is the controller's -responsibility to reset `.spec.tls` and this will be driven based on `.status` i.e. -the latest certificate status condition has to be `ManagedCertificate` and since the required annotation is -not present this results in the `.spec.tls.certificate` and `.spec.tls.key` being cleared -out and the `.status` updated to `DefaultCertificate`. +- The secret created should be in the same namespace as that of the route. +- The secret created is of type `kubernetes.io/tls`. +- The router serviceaccount must have permission to read this secret particular secret. + - The role and rolebinding to provide this access must be provided by the user. -If the user deletes the secret associated with the Route without deleting the -`route.openshift.io/tls-secret-name` annotation, the secret-injector controller will -clear out `.spec.tls.certificate` and `.spec.tls.key` since the following pre-conditions -are met, +The router will not have any active watches on the secret and will only +do a single look up when a route has been updated. The router will maintain +a secret hash in order to be able to reload if the secret content has changed. -- The secret-reference annotation is present. -- Secret mentioned in the annotation doesn't exist -- The latest condition on Route's `.status.certificate` has `ManagedCertificate`=`True` +The `ServiceAliasConfig` creation logic will be updated in the router to also parse +the secret referenced in `.spec.tls.certificateRef`. The router will +use the default certificates only when neither `.spec.tls.certificate` or `.spec.tls.certificateRef` +are provided. -This results in the router using the default certificates that are generated and -the `.status` updated as well. - -As a follow up to the above scenario when the secret-reference annotation added by the user is retained -by the secret-injector controller and because the referenced secret is not present, -the controller will publish an `Event` so that the end user is notified regarding this -switch to using the default generated certificates. The `.status` is also updated to `DefaultCertificate`. - -When transitioning from using a managed certificate to manually managed certificate, the user -is expected to delete the secret-reference annotation and provide new certificate/key pair under `.spec.tls`. -If the previously used `.spec.tls.certificate` and `.spec.tls.key` is not replaced -by the user, the secret-injector controller will clear `.spec.tls.certificate` and `.spec.tls.key` -and route will serve the default certificates that are generated by cluster-ingress-operator. -If the `.spec.tls.certificate` and `.spec.tls.key` are replaced as well, then the -controller will update the `.status.certificate` to `CustomCertificate` - -The new secret-injector controller will not reconcile Routes that are owned by the -ingress-to-route controller. +The route validating admission webhook will verify if the `router` serviceaccount +route has permissions to read the secret that is referenced at `.spec.tls.certificateRef`. +This is only performed if `.spec.tls.certificateRef` is non-nil and non-empty. +In addition to the rbac validation, the admission webhook will also validate if only one +of the certificate fields (`.spec.tls.certificate` and `.spec.tls.certificateRef`) is specified. ### Risks and Mitigations -N.A +The TechPreview feature will not handle secret updates, meaning upon certificate renewal/rotation +the router will not load the new certificates until the route is updated. ### Drawbacks -This introduces an inconsistency between Ingress and Route CRD with respect to how -a secret reference can be provided with the certificate data. The ingress has a field -`.spec.tls.secretName` where as the Route will have an annotation. +The user will need to manually create, provide and maintain the rbac required by the +router so that it can access secrets securely. This becomes tedious when users have +1000s of Routes. ## Design Details ### Open Questions [optional] -1. Does this warrant making changes to the ingress-to-route controller in how - `.spec.tls.secretName` is processed? Currently, the controller reads the secret - data and copies it over to the Route that is created. Since this enhancement - introduces a difference between managed certificate provided via secrets and - manually provided secrets, this distinction isn't possible with Routes created - via Ingresses. [CLOSED] - - - Proposed change - The ingress-to-route controller will also process the secret-reference annotation and if - present copies it over to the route that is created. This annotation will take - precedence over `.spec.tls.secretName` and this offers enough distinction between - third-party/manual certificate management. - -**Answer**: The behaviour of the ingress-to-route controller will be retained as is and -no changes will be done. - -2. How/where to implement e2e tests for [route-controller-manager](https://github.com/openshift/route-controller-manager)? - [CLOSED] - -**Answer**: The e2e tests will be added to openshift/origin. +- Performance testing of openshift-router? +- What does taking Tech Preview to GA look like? +- Do we make changes to the ingress-to-route controller as well? ### Test Plan -This enhancement will be tested in isolation of cert-manager-operator as part of -core OCP payload. This will also be tested with cert-manager-operator as part of the -operators e2e test suite. - -1. Test Route API without interfacing with Ingresses +Update router tests in openshift/origin and supplement all existing certificate related tests +with new tests utilizing `.spec.tls.certificateRef`. Ensure the tests cover the following scenarios, - a. Create a edge terminated route that is using default certificates. - b. Update route with the secret-reference annotation and ensure the `.spec.tls` and `.status` - is updated. - c. Verify that the associated openshift routers are also serving the updated - certificates and not the default certificates. - d. Update certificate data and verify if the certificates served by the router - are updated as well. - d. Delete the annotation and ensure that the route `.spec.tls` and `.status` - denotes the usage of default certificates. Also verify the associated - openshift router is serving the default certificates. - -Various other transitions, `Custom`->`Managed`-`Custom` will also be tested. +- Updating routes from default certificates to certificate referenced via secrets and vice-versa. ### Graduation Criteria -This is a user facing change and will directly go to GA. This feature requires an -update to Openshift Docs. +This feature will initially be released as Tech Preview only. #### Dev Preview -> Tech Preview -N/A. This feature will go directly to GA. +N/A. This feature will go directly to Tech Preview. #### Tech Preview -> GA -N/A. This feature will go directly to GA. +The router will need additional logic to handle secret updates using single item +list/watch for every referenced secret. This pattern needs to be brought over +from kubelet's [secret_manager.go](https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/secret/secret_manager.go) + +Once this pattern is added to the router, the test plan needs to be updated +to cover all scenarios involving updating referenced secrets. + +The ingress-to-route controller in the route-controller-manager will need to +be updated to ensure that the created routes use `.spec.tls.certificateRef` +instead of `.spec.tls.certificate`. Additional tests will need to be added into +o/origin for this scenario. #### Removing a deprecated feature @@ -396,15 +247,14 @@ N/A. ### Upgrade / Downgrade Strategy -On downgrades, all routes using `route.openshift.io/tls-secret-name` annotation will continue to use the custom certificates -indefinitely unless the route is manually edited and the `.spec.tls` is updated. +On downgrades, all routes specifying `.spec.tls.certificateRef` will switch over to use the default certificates +unless the route is manually edited and the `.spec.tls` is updated. + +Upgrade strategy not considered since this feature is going to be added as TechPreviewNoUpgrade. ### Version Skew Strategy -This enhancement isn't affected by version skew of core Kubernetes components -during updates/upgrades. The new controller will be capable to re-synchronize -the required components if required and without this controller the annotation -would basically do nothing. +This feature will be added as TechPreviewNoUprade. ### Operational Aspects of API Extensions @@ -417,16 +267,9 @@ adds a hard dependency in order of operation. In scenarios where the secret has by third-party solutions like cert-manager, the route would not be successfully created due to the dependency on the route. -> A solution to this could be that upon failure to use the secret due to various reasons, -> the controller defaults to using the default generated certificates and updating the route -> status appropriately. The controller also publishes an event indicating the switch has been made. - #### Support Procedures -The new introduced `RouteCertificateConditionType` types will provide the required information into -the current state of route objects with respect to certificates. While using third-party solutions -like cert-manager for certificate management the `RouteCertificateCondition` will indicate which type -of certificate is associated with the route. +N.A ## Implementation History @@ -434,7 +277,15 @@ N.A ## Alternatives -N.A +An alternative proposal is to introduce a new controller (secret-injector) in [route-controller-manager](https://github.com/openshift/route-controller-manager) +to manage a new annotation (secret-reference) on the Route object. This annotation +enables the users to provide a reference to a Secret containing the serving cert/key +pair that will be injected to `.spec.tls` and will be served by OpenShift router. +This annotation will be given a higher preference if route CR also has `.spec.tls.certificate` +and `.spec.tls.key` fields set. + +This approach was dropped after much deliberation as it introduces a confused deputy problem +as well as opens a security flaw where a user could read the contents of an arbitrary secret. ## Infrastructure Needed [optional] From 7f200c23ec1a5fd82dcb6696dc028692039909ad Mon Sep 17 00:00:00 2001 From: Thejas N Date: Wed, 3 May 2023 17:28:53 +0530 Subject: [PATCH 03/11] Update techpreview scope/goal Update techpreview scope to include secret updates and add more implementation detail for validation and failure modes. --- ...ion-for-external-certificate-management.md | 169 +++++++++++------- 1 file changed, 108 insertions(+), 61 deletions(-) diff --git a/enhancements/ingress/route-secret-injection-for-external-certificate-management.md b/enhancements/ingress/route-secret-injection-for-external-certificate-management.md index da7c67b9c5..4688635876 100644 --- a/enhancements/ingress/route-secret-injection-for-external-certificate-management.md +++ b/enhancements/ingress/route-secret-injection-for-external-certificate-management.md @@ -13,9 +13,9 @@ approvers: api-approvers: # In case of new or modified APIs or API extensions (CRDs, aggregated apiservers, webhooks, finalizers). If there is no API change, use "None" - '@joelspeed' creation-date: 2022-12-13 -last-updated: 2023-04-24 +last-updated: 2023-05-03 tracking-link: # link to the tracking ticket (for example: Jira Feature or Epic ticket) that corresponds to this enhancement - - https://issues.redhat.com/browse/CM-16 + - https://issues.redhat.com/browse/CM-815 --- # Route Secret Injection For External Certificate Management @@ -61,6 +61,9 @@ certificate data via a secret reference. - As an Openshift engineer, I want to be update the router so that it is able read secrets directly if all the preconditions have been met by the router serviceaccount. +- As an OpenShift engineer, I want to update the route admission webhook to add new validations + required for `.spec.tls.certificateRef`. + - As an OpenShift engineer, I want to be able to update Route API so that I can integrate OpenShift Routes with third-party certificate management solutions like cert-manager. @@ -70,12 +73,14 @@ certificate data via a secret reference. ### Goals - Provide users with a configurable option in Route API to reference externally managed certificates via secrets. +- Provide smooth roll out of new certificates on OpenShift router when referenced certificates + are renewed (secret containing the certificate is updated). ### Non-Goals - Provide certificate life cycle management controls on the Route API (expiryAfter, renewBefore, etc). -- Provide smooth roll out of new certificates on OpenShift router when referenced certificates - are renewed (secret containing the certificate is updated). +- Modify ingress-to-route controller behaviour to use `.spec.tls.certificateRef` +- Extend this feature to cover CA certificate or destination CA certificate in the Route API. ## Proposal @@ -87,19 +92,12 @@ and served by OpenShift router. ### Workflow Description -End users have 2 possible variations for the creation of the route: - -- Create a route for the user workload and this is completely managed by the end user. -- Create an ingress for the user workload and a managed route is created automatically - by the ingress-to-route controller. [Depends on the open question] - -Both these workflows will support integrating with third party certificate management -solutions like cert-manager with the secret-reference annotation described under [API Extensions](#api-extensions). - -**When a Route is directly used to expose user workload** +The following workflow describes the integration with third party +certificate management solutions like cert-manager with the certificate +reference field described under [API Extensions](#api-extensions). -- The end user must have generated the serving certificate as a pre requisite - using third-party systems like cert-manager. +- The end user must have generated the serving certificate generated + as a pre requisite using third-party systems like cert-manager. - In cert-manager's case, the [Certificate](https://cert-manager.io/docs/usage/certificate/#creating-certificate-resources) CR must be created in the same namespace where the Route is going to be created. - The end user must create a role and in the same namespace as the secret containing the certificate from earlier, @@ -117,18 +115,6 @@ solutions like cert-manager with the secret-reference annotation described under - If the secret that is referenced exists and has a successfully generated cert/key pair, the router will serve this certificate if all preconditions are met. -**When an Ingress is used to expose user workload** - -- The end user must have generated the serving certificate as a pre requisite - using third-party systems like cert-manager. -- In cert-manager's case, the [Certificate](https://cert-manager.io/docs/usage/certificate/#creating-certificate-resources) CR must be created in the same namespace - where the Ingress is going to be created. -- To expose a user workload, a new Ingress with the generated secret - referenced in `.spec.tls.secretName` needs to be created. -- If the secret CR that is referenced exists and has a successfully generated - cert/key pair, the ingress-to-route controller adds this secret name to - the created route `.spec.tls.certificateRef`. - #### Variation [optional] N.A @@ -144,12 +130,11 @@ type TLSConfig struct { // certificateRef provides certificate contents as a secret reference. // This should be a single serving certificate, not a certificate - // chain. Do not include a CA certificate. - + // chain. Do not include a CA certificate. // // +kubebuilder:validation:Optional - // +openshift:enable:FeatureSets=TechPreviewNoUpgrade - // +optional + // +openshift:enable:FeatureSets=TechPreviewNoUpgrade + // +optional CertificateRef *corev1.LocalObjectReference `json:"certificateRef,omitempty" protobuf:"bytes,7,opt,name=certificateRef"` } ``` @@ -171,53 +156,78 @@ N.A ### Implementation Details/Notes/Constraints [optional] The router will read the secret referenced in `.spec.tls.certificateRef` if present and -if the following pre-conditions are met it uses this certificate us configure haproxy. +if the following pre-conditions (validated in the admission webhook) are met it uses +this certificate us configure haproxy. - The secret created should be in the same namespace as that of the route. - The secret created is of type `kubernetes.io/tls`. - The router serviceaccount must have permission to read this secret particular secret. - The role and rolebinding to provide this access must be provided by the user. -The router will not have any active watches on the secret and will only -do a single look up when a route has been updated. The router will maintain -a secret hash in order to be able to reload if the secret content has changed. +The router will bootstrap a watch based secret manager to ensure it can keep the +certificate/secret up-to-date. This means the router pod will maintain active watches +for every secret that is referenced by a route. + +Every active watch will be linked to a route, meaning the watch based secret manager +will be linked to the lifecycle of the route. For every new route that is created, +the secret manager will start a watch if the route uses `.spec.tls.certificateRef`. +For every update route event, the secret manager only increments the reference count. +If a route is deleted, the secret manager will unregister the route and teardown the +watch associated with it. The `ServiceAliasConfig` creation logic will be updated in the router to also parse the secret referenced in `.spec.tls.certificateRef`. The router will use the default certificates only when neither `.spec.tls.certificate` or `.spec.tls.certificateRef` are provided. -The route validating admission webhook will verify if the `router` serviceaccount -route has permissions to read the secret that is referenced at `.spec.tls.certificateRef`. -This is only performed if `.spec.tls.certificateRef` is non-nil and non-empty. -In addition to the rbac validation, the admission webhook will also validate if only one -of the certificate fields (`.spec.tls.certificate` and `.spec.tls.certificateRef`) is specified. +Extend the `ExtendedValidateRoute()` in the router to also cover validating secret and +its content. In addition to this, the route validating admission webhook will verify if +the `router` serviceaccount route has permissions to read the secret that is +referenced at `.spec.tls.certificateRef`. This is only performed if `.spec.tls.certificateRef` +is non-nil and non-empty. In addition to the rbac validation, the admission webhook +will also validate if only one of the certificate fields (`.spec.tls.certificate` +and `.spec.tls.certificateRef`) is specified. ### Risks and Mitigations -The TechPreview feature will not handle secret updates, meaning upon certificate renewal/rotation -the router will not load the new certificates until the route is updated. +TBD ### Drawbacks The user will need to manually create, provide and maintain the rbac required by the router so that it can access secrets securely. This becomes tedious when users have -1000s of Routes. +100s of Routes. + +The workaround for this is to document various levels of rbac that can be provided, + +- Grant router service account access to secret by secret-name (explicit rbac) +- Grant router service account access to all secrets in a fixed namespace (implicit rbac) + +The above variations need to be documented for the end user as part of OpenShift documentation. ## Design Details ### Open Questions [optional] -- Performance testing of openshift-router? -- What does taking Tech Preview to GA look like? +- Performance testing of openshift-router in tech-preview? Is there + a workflow present where we can gather some early metrics (memory, cpu)? This will help in + preemptively addressing performance concerns before going GA. + - Do we make changes to the ingress-to-route controller as well? + > The ingress-to-route behaviour will remain as is i.e. it will not make use of + > the newly introduced field. ### Test Plan Update router tests in openshift/origin and supplement all existing certificate related tests with new tests utilizing `.spec.tls.certificateRef`. Ensure the tests cover the following scenarios, -- Updating routes from default certificates to certificate referenced via secrets and vice-versa. +- Updating routes from default certificates to certificate referenced via + secrets and vice-versa. +- Updating secret/certificate referenced in routes and verify serving + certificate has been updated. +- Updating secret/certificate with incorrect information and verify route + is not admitted due to validation failure. (eg: mismatched hostnames) ### Graduation Criteria @@ -227,20 +237,20 @@ This feature will initially be released as Tech Preview only. N/A. This feature will go directly to Tech Preview. -#### Tech Preview -> GA - -The router will need additional logic to handle secret updates using single item -list/watch for every referenced secret. This pattern needs to be brought over -from kubelet's [secret_manager.go](https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/secret/secret_manager.go) +#### Tech Preview -> GA (Future work) -Once this pattern is added to the router, the test plan needs to be updated -to cover all scenarios involving updating referenced secrets. +The router will need to undergo performance testing as part of OCP payload +to ensure the memory implications of creating and maintains all the active watches +is verified to be efficient. The ingress-to-route controller in the route-controller-manager will need to be updated to ensure that the created routes use `.spec.tls.certificateRef` instead of `.spec.tls.certificate`. Additional tests will need to be added into o/origin for this scenario. +This behaviour should be extended to both `.spec.tls.caCertificate` and +`.spec.tls.destinationCACertificate` to ensure uniformity and improve security. + #### Removing a deprecated feature N/A. @@ -258,14 +268,42 @@ This feature will be added as TechPreviewNoUprade. ### Operational Aspects of API Extensions -N.A +Route admission webhook will be modified to validate the following scenarios, + +- Check if secret/certificate referenced under `.spec.tls.certificateRef` exists. +- Check if secret/certificate referenced under `.spec.tls.certificateRef` is of the correct type. +- Check if router service account has permissions to read referenced secret. +- Check if route only one of the fields set, + - `.spec.tls.certificate` and `.spec.tls.key` + - `.spec.tls.certificateRef` + +SLOs TBD #### Failure Modes -When using routes with third-party certificate management solutions like cert-manager, this -adds a hard dependency in order of operation. In scenarios where the secret has not been created -by third-party solutions like cert-manager, the route would not be successfully created due -to the dependency on the route. +##### Referenced secret not present + +In scenarios where the secret has not been created by third-party solutions +like cert-manager, the route would not be successfully created due to the +dependency. This route will be rejected by the route admission webhook with an +`FieldValueNotFound` error and will contain the reason as `referenced secret not present`. + +In addition to this validation, the router will also validate the same to +cover an edge case. If a route fails this validation, it is not processed +further and the error will be reflected on the route `.status` with the same reason. + +##### Insufficient router permission + +As part of the route admission webhook, if the router does not have permission +to read the secret referenced under `.spec.tls.certificateRef`, the route is +rejected with an `FieldValueForbidden` error and reason as `insufficient permission +to read resource`. + +##### Incorrect secret type + +As part of `ExtendedValidateRoute()`, the router validates the content of the secret +that is referenced under `.spec.tls.certificateRef`. Failure will result in the route +not being admitted and this will reflect under route `.status` as `FieldValueInvalid`. #### Support Procedures @@ -277,6 +315,8 @@ N.A ## Alternatives +### Secret Injector + An alternative proposal is to introduce a new controller (secret-injector) in [route-controller-manager](https://github.com/openshift/route-controller-manager) to manage a new annotation (secret-reference) on the Route object. This annotation enables the users to provide a reference to a Secret containing the serving cert/key @@ -284,8 +324,15 @@ pair that will be injected to `.spec.tls` and will be served by OpenShift router This annotation will be given a higher preference if route CR also has `.spec.tls.certificate` and `.spec.tls.key` fields set. -This approach was dropped after much deliberation as it introduces a confused deputy problem -as well as opens a security flaw where a user could read the contents of an arbitrary secret. +> This approach was dropped after much deliberation as it introduces a confused deputy problem +> as well as opens a security flaw where a user could read the contents of an arbitrary secret. + +### Extend oc CLI + +As an alternative to requiring the user create the role and rolebinding to grant the router +access to the secrets, this behaviour can be baked into `oc create route`. This will reduce +the number of manual steps and will be less error prone. But here's the catch, how widely +is `oc create route` used and do users who manage 100s of routes really use it. ## Infrastructure Needed [optional] From ce7e82a07e717b269ec3cb15c961f850d68f19c3 Mon Sep 17 00:00:00 2001 From: Thejas N Date: Thu, 11 May 2023 18:30:43 +0530 Subject: [PATCH 04/11] Updated API and implementation details --- ...ion-for-external-certificate-management.md | 75 ++++++++++++------- 1 file changed, 48 insertions(+), 27 deletions(-) diff --git a/enhancements/ingress/route-secret-injection-for-external-certificate-management.md b/enhancements/ingress/route-secret-injection-for-external-certificate-management.md index 4688635876..b0adb803ed 100644 --- a/enhancements/ingress/route-secret-injection-for-external-certificate-management.md +++ b/enhancements/ingress/route-secret-injection-for-external-certificate-management.md @@ -61,7 +61,7 @@ certificate data via a secret reference. - As an Openshift engineer, I want to be update the router so that it is able read secrets directly if all the preconditions have been met by the router serviceaccount. -- As an OpenShift engineer, I want to update the route admission webhook to add new validations +- As an OpenShift engineer, I want to update the route validation in the api-server to add new validations required for `.spec.tls.certificateRef`. - As an OpenShift engineer, I want to be able to update Route API so that I can integrate @@ -125,18 +125,33 @@ A `.spec.tls.certificateRef` field is added to Route `.spec.tls` which can be us containing the certificate data instead of using `.spec.tls.certificate` and `spec.tls.key`. ```go + +// TLSConfig defines config used to secure a route and provide termination +// +// +kubebuilder:validation:XValidation:rule="has(self.termination) && has(self.insecureEdgeTerminationPolicy) ? !((self.termination=='passthrough') && (self.insecureEdgeTerminationPolicy=='Allow')) : true", message="cannot have both spec.tls.termination: passthrough and spec.tls.insecureEdgeTerminationPolicy: Allow" +// +kubebuilder:validation:XValidation:rule="has(self.certificate) && has(self.certificateRef) ? false : true", message="cannot have both spec.tls.certificate and spec.tls.certificateRef" type TLSConfig struct { // ... - // certificateRef provides certificate contents as a secret reference. - // This should be a single serving certificate, not a certificate - // chain. Do not include a CA certificate. - // - // +kubebuilder:validation:Optional - // +openshift:enable:FeatureSets=TechPreviewNoUpgrade - // +optional + // certificateRef provides certificate contents as a secret reference. + // This should be a single serving certificate, not a certificate + // chain. Do not include a CA certificate. The secret referenced should + // be present in the same namespace as that of the Route. + // + // +openshift:enable:FeatureSets=TechPreviewNoUpgrade + // +optional CertificateRef *corev1.LocalObjectReference `json:"certificateRef,omitempty" protobuf:"bytes,7,opt,name=certificateRef"` } + +// LocalObjectReference contains enough information to let you locate the +// referenced object inside the same namespace. +// +structType=atomic +type LocalObjectReference struct { + // Name of the referent. + // More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names + // +optional + Name string `json:"name,omitempty" protobuf:"bytes,1,opt,name=name"` +} ``` _Note_: The default value would be `nil`. The secret is required to be created @@ -156,14 +171,9 @@ N.A ### Implementation Details/Notes/Constraints [optional] The router will read the secret referenced in `.spec.tls.certificateRef` if present and -if the following pre-conditions (validated in the admission webhook) are met it uses +if the following pre-conditions (validated in the API server) are met it uses this certificate us configure haproxy. -- The secret created should be in the same namespace as that of the route. -- The secret created is of type `kubernetes.io/tls`. -- The router serviceaccount must have permission to read this secret particular secret. - - The role and rolebinding to provide this access must be provided by the user. - The router will bootstrap a watch based secret manager to ensure it can keep the certificate/secret up-to-date. This means the router pod will maintain active watches for every secret that is referenced by a route. @@ -180,17 +190,28 @@ the secret referenced in `.spec.tls.certificateRef`. The router will use the default certificates only when neither `.spec.tls.certificate` or `.spec.tls.certificateRef` are provided. -Extend the `ExtendedValidateRoute()` in the router to also cover validating secret and -its content. In addition to this, the route validating admission webhook will verify if -the `router` serviceaccount route has permissions to read the secret that is -referenced at `.spec.tls.certificateRef`. This is only performed if `.spec.tls.certificateRef` -is non-nil and non-empty. In addition to the rbac validation, the admission webhook -will also validate if only one of the certificate fields (`.spec.tls.certificate` -and `.spec.tls.certificateRef`) is specified. +Validations done by the router as part of [ExtendedValidateRoute()](https://github.com/openshift/router/blob/c407ebbc5d8d85daea2ef2d1ba539444a06f4d25/pkg/router/routeapihelpers/validation.go#L158) (contents of secret), + +- Verify certificate and key (PEM encode/decode) +- Verify private key matches public certificate + +Validations done by API server as part of [ValidateRoute()](https://github.com/openshift/openshift-apiserver/blob/aac3dd5bf0547e928103a0f718ca104b1bb13930/pkg/route/apis/route/validation/validation.go#L21), + +- The secret created should be in the same namespace as that of the route. +- The secret created is of type `kubernetes.io/tls`. +- The router serviceaccount must have permission to read this secret particular secret. + - The role and rolebinding to provide this access must be provided by the user. +- CEL validations will enforce that both `.spec.tls.certificate` and `.spec.tls.certificateRef` + are not specified on the route. ### Risks and Mitigations -TBD +There is a possibility of an invalid route being processed by the router (edge case), +if any changes are done to the rbac or the referenced secret is deleted after the API +server validation but before router has processed the route (maybe router pod is not running) +then this can lead to the router processing this incorrect route. + +> Will need to duplicate the validations present on the API server to the router. ### Drawbacks @@ -227,7 +248,7 @@ with new tests utilizing `.spec.tls.certificateRef`. Ensure the tests cover the - Updating secret/certificate referenced in routes and verify serving certificate has been updated. - Updating secret/certificate with incorrect information and verify route - is not admitted due to validation failure. (eg: mismatched hostnames) + is not admitted due to validation failure. (eg: mismatched public and private key, etc) ### Graduation Criteria @@ -268,7 +289,7 @@ This feature will be added as TechPreviewNoUprade. ### Operational Aspects of API Extensions -Route admission webhook will be modified to validate the following scenarios, +Route validation in the API server will be modified to validate the following scenarios, - Check if secret/certificate referenced under `.spec.tls.certificateRef` exists. - Check if secret/certificate referenced under `.spec.tls.certificateRef` is of the correct type. @@ -277,7 +298,7 @@ Route admission webhook will be modified to validate the following scenarios, - `.spec.tls.certificate` and `.spec.tls.key` - `.spec.tls.certificateRef` -SLOs TBD +TODO: SLOs #### Failure Modes @@ -285,7 +306,7 @@ SLOs TBD In scenarios where the secret has not been created by third-party solutions like cert-manager, the route would not be successfully created due to the -dependency. This route will be rejected by the route admission webhook with an +dependency. This route will be rejected by the API server with an `FieldValueNotFound` error and will contain the reason as `referenced secret not present`. In addition to this validation, the router will also validate the same to @@ -294,7 +315,7 @@ further and the error will be reflected on the route `.status` with the same rea ##### Insufficient router permission -As part of the route admission webhook, if the router does not have permission +As part of the API server validation, if the router does not have permission to read the secret referenced under `.spec.tls.certificateRef`, the route is rejected with an `FieldValueForbidden` error and reason as `insufficient permission to read resource`. From 942ee8cc68a2399a1d4467bbbf29fb0dd781147b Mon Sep 17 00:00:00 2001 From: Thejas N Date: Tue, 4 Jul 2023 10:11:17 +0530 Subject: [PATCH 05/11] Update API field to externalCertificate --- ...ion-for-external-certificate-management.md | 71 +++++++++---------- 1 file changed, 35 insertions(+), 36 deletions(-) diff --git a/enhancements/ingress/route-secret-injection-for-external-certificate-management.md b/enhancements/ingress/route-secret-injection-for-external-certificate-management.md index b0adb803ed..33c3043373 100644 --- a/enhancements/ingress/route-secret-injection-for-external-certificate-management.md +++ b/enhancements/ingress/route-secret-injection-for-external-certificate-management.md @@ -54,15 +54,11 @@ certificate data via a secret reference. for certificate management of user workloads on OpenShift so that no manual process is required to renew expired certificates. -- As an end user of Route API, I want OpenShift Routes to support both manual and managed - mode of operation for certification management so that I can switch between manual certificate - management and third-party certificate management. - - As an Openshift engineer, I want to be update the router so that it is able read secrets directly if all the preconditions have been met by the router serviceaccount. - As an OpenShift engineer, I want to update the route validation in the api-server to add new validations - required for `.spec.tls.certificateRef`. + required for `.spec.tls.externalCertificate`. - As an OpenShift engineer, I want to be able to update Route API so that I can integrate OpenShift Routes with third-party certificate management solutions like cert-manager. @@ -79,14 +75,14 @@ certificate data via a secret reference. ### Non-Goals - Provide certificate life cycle management controls on the Route API (expiryAfter, renewBefore, etc). -- Modify ingress-to-route controller behaviour to use `.spec.tls.certificateRef` +- Modify ingress-to-route controller behaviour to use `.spec.tls.externalCertificate` - Extend this feature to cover CA certificate or destination CA certificate in the Route API. ## Proposal This enhancement proposes extending the openshift/router to read serving certificate data either -from the Route `.spec.tls.certificate` and `.spec.tls.key` or from a new field `.spec.tls.certificateRef` -which is a `kubernetes.io/tls` type secret reference. This `certificateRef` field will enables the +from the Route `.spec.tls.certificate` and `.spec.tls.key` or from a new field `.spec.tls.externalCertificate` +which is a `kubernetes.io/tls` type secret reference. This `externalCertificate` field will enables the users to provide a reference to a Secret containing the serving cert/key pair that will be parsed and served by OpenShift router. @@ -110,7 +106,7 @@ reference field described under [API Extensions](#api-extensions). oc create rolebinding foo-secret-reader --role=secret-reader --serviceaccount=openshift-ingress:router --namespace= ``` - To expose a user workload, the user would create a new Route with the - `.spec.tls.certificateRef` referencing the generated secret that was created + `.spec.tls.externalCertificate` referencing the generated secret that was created in the previous step. - If the secret that is referenced exists and has a successfully generated cert/key pair, the router will serve this certificate if all preconditions are met. @@ -121,7 +117,7 @@ N.A ### API Extensions -A `.spec.tls.certificateRef` field is added to Route `.spec.tls` which can be used to provide a secret name +A `.spec.tls.externalCertificate` field is added to Route `.spec.tls` which can be used to provide a secret name containing the certificate data instead of using `.spec.tls.certificate` and `spec.tls.key`. ```go @@ -129,25 +125,26 @@ containing the certificate data instead of using `.spec.tls.certificate` and `sp // TLSConfig defines config used to secure a route and provide termination // // +kubebuilder:validation:XValidation:rule="has(self.termination) && has(self.insecureEdgeTerminationPolicy) ? !((self.termination=='passthrough') && (self.insecureEdgeTerminationPolicy=='Allow')) : true", message="cannot have both spec.tls.termination: passthrough and spec.tls.insecureEdgeTerminationPolicy: Allow" -// +kubebuilder:validation:XValidation:rule="has(self.certificate) && has(self.certificateRef) ? false : true", message="cannot have both spec.tls.certificate and spec.tls.certificateRef" +// +openshift:validation:FeatureSetAwareXValidation:featureSet=TechPreviewNoUpgrade;CustomNoUpgrade,rule="!(has(self.certificate) && has(self.externalCertificate))", message="cannot have both spec.tls.certificate and spec.tls.externalCertificate" type TLSConfig struct { // ... - // certificateRef provides certificate contents as a secret reference. + // externalCertificate provides certificate contents as a secret reference. // This should be a single serving certificate, not a certificate // chain. Do not include a CA certificate. The secret referenced should // be present in the same namespace as that of the Route. + // Forbidden when `certificate` is set. // - // +openshift:enable:FeatureSets=TechPreviewNoUpgrade + // +openshift:enable:FeatureSets=CustomNoUpgrade;TechPreviewNoUpgrade // +optional - CertificateRef *corev1.LocalObjectReference `json:"certificateRef,omitempty" protobuf:"bytes,7,opt,name=certificateRef"` + ExternalCertificate LocalObjectReference `json:"externalCertificate,omitempty" protobuf:"bytes,7,opt,name=externalCertificate"` } // LocalObjectReference contains enough information to let you locate the // referenced object inside the same namespace. // +structType=atomic type LocalObjectReference struct { - // Name of the referent. + // name of the referent. // More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names // +optional Name string `json:"name,omitempty" protobuf:"bytes,1,opt,name=name"` @@ -159,8 +156,8 @@ in the same namespace as that of the Route. The secret must be of type `kubernetes.io/tls` and the tls.key and the tls.crt key must be provided in the `data` (or `stringData`) field of the Secret configuration. -If neither `.spec.tls.certificateRef` or `.spec.tls.certificate` and `.spec.tls.key` are -provided the router will serve the default generated secret. +If neither `.spec.tls.externalCertificate` or `.spec.tls.certificate` and `.spec.tls.key` are +provided the router will serve the default generated certificates. All valid and invalid scenarios will be depicted via the existing `RouteIngressCondition`. @@ -170,7 +167,7 @@ N.A ### Implementation Details/Notes/Constraints [optional] -The router will read the secret referenced in `.spec.tls.certificateRef` if present and +The router will read the secret referenced in `.spec.tls.externalCertificate` if present and if the following pre-conditions (validated in the API server) are met it uses this certificate us configure haproxy. @@ -180,15 +177,15 @@ for every secret that is referenced by a route. Every active watch will be linked to a route, meaning the watch based secret manager will be linked to the lifecycle of the route. For every new route that is created, -the secret manager will start a watch if the route uses `.spec.tls.certificateRef`. +the secret manager will start a watch if the route uses `.spec.tls.externalCertificate`. For every update route event, the secret manager only increments the reference count. If a route is deleted, the secret manager will unregister the route and teardown the watch associated with it. The `ServiceAliasConfig` creation logic will be updated in the router to also parse -the secret referenced in `.spec.tls.certificateRef`. The router will -use the default certificates only when neither `.spec.tls.certificate` or `.spec.tls.certificateRef` -are provided. +the secret referenced in `.spec.tls.externalCertificate`. The router will +use the default certificates only when `.spec.tls.certificate` or `.spec.tls.externalCertificate` +are not provided. Validations done by the router as part of [ExtendedValidateRoute()](https://github.com/openshift/router/blob/c407ebbc5d8d85daea2ef2d1ba539444a06f4d25/pkg/router/routeapihelpers/validation.go#L158) (contents of secret), @@ -201,7 +198,7 @@ Validations done by API server as part of [ValidateRoute()](https://github.com/o - The secret created is of type `kubernetes.io/tls`. - The router serviceaccount must have permission to read this secret particular secret. - The role and rolebinding to provide this access must be provided by the user. -- CEL validations will enforce that both `.spec.tls.certificate` and `.spec.tls.certificateRef` +- CEL validations will enforce that both `.spec.tls.certificate` and `.spec.tls.externalCertificate` are not specified on the route. ### Risks and Mitigations @@ -241,7 +238,7 @@ The above variations need to be documented for the end user as part of OpenShift ### Test Plan Update router tests in openshift/origin and supplement all existing certificate related tests -with new tests utilizing `.spec.tls.certificateRef`. Ensure the tests cover the following scenarios, +with new tests utilizing `.spec.tls.externalCertificate`. Ensure the tests cover the following scenarios, - Updating routes from default certificates to certificate referenced via secrets and vice-versa. @@ -252,7 +249,8 @@ with new tests utilizing `.spec.tls.certificateRef`. Ensure the tests cover the ### Graduation Criteria -This feature will initially be released as Tech Preview only. +This feature will initially be released as Tech Preview only. The e2e tests +in openshift/origin will only be added when graduating this feature to GA. #### Dev Preview -> Tech Preview @@ -260,17 +258,20 @@ N/A. This feature will go directly to Tech Preview. #### Tech Preview -> GA (Future work) +The e2e tests as part of openshift/origin should be consistently passing. The router will need to undergo performance testing as part of OCP payload to ensure the memory implications of creating and maintains all the active watches is verified to be efficient. +##### Future work + The ingress-to-route controller in the route-controller-manager will need to -be updated to ensure that the created routes use `.spec.tls.certificateRef` +be updated to ensure that the created routes use `.spec.tls.externalCertificate` instead of `.spec.tls.certificate`. Additional tests will need to be added into o/origin for this scenario. -This behaviour should be extended to both `.spec.tls.caCertificate` and -`.spec.tls.destinationCACertificate` to ensure uniformity and improve security. +Current implementation does not use `caCert` in the secret to populate +`.spec.tls.caCertificate`, this can be added in the future. #### Removing a deprecated feature @@ -278,7 +279,7 @@ N/A. ### Upgrade / Downgrade Strategy -On downgrades, all routes specifying `.spec.tls.certificateRef` will switch over to use the default certificates +On downgrades, all routes specifying `.spec.tls.externalCertificate` will switch over to use the default certificates unless the route is manually edited and the `.spec.tls` is updated. Upgrade strategy not considered since this feature is going to be added as TechPreviewNoUpgrade. @@ -291,14 +292,12 @@ This feature will be added as TechPreviewNoUprade. Route validation in the API server will be modified to validate the following scenarios, -- Check if secret/certificate referenced under `.spec.tls.certificateRef` exists. -- Check if secret/certificate referenced under `.spec.tls.certificateRef` is of the correct type. +- Check if secret/certificate referenced under `.spec.tls.externalCertificate` exists. +- Check if secret/certificate referenced under `.spec.tls.externalCertificate` is of the correct type. - Check if router service account has permissions to read referenced secret. - Check if route only one of the fields set, - `.spec.tls.certificate` and `.spec.tls.key` - - `.spec.tls.certificateRef` - -TODO: SLOs + - `.spec.tls.externalCertificate` #### Failure Modes @@ -316,14 +315,14 @@ further and the error will be reflected on the route `.status` with the same rea ##### Insufficient router permission As part of the API server validation, if the router does not have permission -to read the secret referenced under `.spec.tls.certificateRef`, the route is +to read the secret referenced under `.spec.tls.externalCertificate`, the route is rejected with an `FieldValueForbidden` error and reason as `insufficient permission to read resource`. ##### Incorrect secret type As part of `ExtendedValidateRoute()`, the router validates the content of the secret -that is referenced under `.spec.tls.certificateRef`. Failure will result in the route +that is referenced under `.spec.tls.externalCertificate`. Failure will result in the route not being admitted and this will reflect under route `.status` as `FieldValueInvalid`. #### Support Procedures From 425f99fe650995d0f30ff7edf27c945f8d5a84e0 Mon Sep 17 00:00:00 2001 From: Thejas N Date: Tue, 4 Jul 2023 15:42:00 +0530 Subject: [PATCH 06/11] Apply review suggestions - Update secret monitor implementation details - Update validation list --- ...ion-for-external-certificate-management.md | 96 +++++++++++-------- 1 file changed, 57 insertions(+), 39 deletions(-) diff --git a/enhancements/ingress/route-secret-injection-for-external-certificate-management.md b/enhancements/ingress/route-secret-injection-for-external-certificate-management.md index 33c3043373..88e6ff0598 100644 --- a/enhancements/ingress/route-secret-injection-for-external-certificate-management.md +++ b/enhancements/ingress/route-secret-injection-for-external-certificate-management.md @@ -10,10 +10,11 @@ reviewers: - '@deads2k' approvers: - '@Miciah' + - '@alebedev87' api-approvers: # In case of new or modified APIs or API extensions (CRDs, aggregated apiservers, webhooks, finalizers). If there is no API change, use "None" - '@joelspeed' creation-date: 2022-12-13 -last-updated: 2023-05-03 +last-updated: 2023-07-05 tracking-link: # link to the tracking ticket (for example: Jira Feature or Epic ticket) that corresponds to this enhancement - https://issues.redhat.com/browse/CM-815 --- @@ -54,11 +55,16 @@ certificate data via a secret reference. for certificate management of user workloads on OpenShift so that no manual process is required to renew expired certificates. -- As an Openshift engineer, I want to be update the router so that it is able read secrets directly - if all the preconditions have been met by the router serviceaccount. +- As an Openshift engineer, I want to update the router so that it is able read + secrets directly if all the preconditions have been met by the router serviceaccount. + + - The router serviceaccount must have permission to read this secret particular secret. + - The role and rolebinding to provide this access must be provided by the user. + +- As an OpenShift engineer, I want to update the route validation in openshift/library-go + in order to validate the updated Route API. -- As an OpenShift engineer, I want to update the route validation in the api-server to add new validations - required for `.spec.tls.externalCertificate`. + - Both Openshift and Microshift both run the o/library-go validations as part of admission plugin - As an OpenShift engineer, I want to be able to update Route API so that I can integrate OpenShift Routes with third-party certificate management solutions like cert-manager. @@ -93,10 +99,11 @@ certificate management solutions like cert-manager with the certificate reference field described under [API Extensions](#api-extensions). - The end user must have generated the serving certificate generated - as a pre requisite using third-party systems like cert-manager. + as a prerequisite using third-party systems like cert-manager. - In cert-manager's case, the [Certificate](https://cert-manager.io/docs/usage/certificate/#creating-certificate-resources) CR must be created in the same namespace where the Route is going to be created. -- The end user must create a role and in the same namespace as the secret containing the certificate from earlier, +- The end user must create a role in the same namespace as the secret containing + the certificate which was generated by the cert-manager from earlier. ```bash oc create role secret-reader --verb=get,list,watch --resource=secrets --resourceName= ``` @@ -109,7 +116,7 @@ reference field described under [API Extensions](#api-extensions). `.spec.tls.externalCertificate` referencing the generated secret that was created in the previous step. - If the secret that is referenced exists and has a successfully generated - cert/key pair, the router will serve this certificate if all preconditions are met. + cert/key pair, the router will serve this certificate if all preconditions (listed [below](#implementation-detailsnotesconstraints-optional)) are met. #### Variation [optional] @@ -151,13 +158,14 @@ type LocalObjectReference struct { } ``` -_Note_: The default value would be `nil`. The secret is required to be created -in the same namespace as that of the Route. The secret must be of type -`kubernetes.io/tls` and the tls.key and the tls.crt key must be provided in -the `data` (or `stringData`) field of the Secret configuration. +_Note_: The secret is required to be created in the same namespace as that of the Route. +The secret must be of type `kubernetes.io/tls` and the tls.key and the tls.crt key must be +provided in the `data` (or `stringData`) field of the Secret configuration. If neither `.spec.tls.externalCertificate` or `.spec.tls.certificate` and `.spec.tls.key` are -provided the router will serve the default generated certificates. +provided the router will serve the default generated certificates. User's will not be able to +provide both `.spec.tls.certificate/key` and `.spec.tls.externalCerificate`. API server +admission validation will enforce this. All valid and invalid scenarios will be depicted via the existing `RouteIngressCondition`. @@ -167,40 +175,45 @@ N.A ### Implementation Details/Notes/Constraints [optional] -The router will read the secret referenced in `.spec.tls.externalCertificate` if present and -if the following pre-conditions (validated in the API server) are met it uses -this certificate us configure haproxy. +The router will read the secret referenced in `.spec.tls.externalCertificate` and will use +the certificate inside to configure HAProxy if the secret is present and if the +following pre-conditions are met: + +- Validations done by the router as part of [ExtendedValidateRoute()](https://github.com/openshift/router/blob/c407ebbc5d8d85daea2ef2d1ba539444a06f4d25/pkg/router/routeapihelpers/validation.go#L158) (contents of secret), + + - Verify certificate and key (PEM encode/decode) + - Verify private key matches public certificate + +- Validations done by API server as part of [ValidateRoute()](https://github.com/openshift/openshift-apiserver/blob/aac3dd5bf0547e928103a0f718ca104b1bb13930/pkg/route/apis/route/validation/validation.go#L21), + + - The secret created should be in the same namespace as that of the route. + - The secret created is of type `kubernetes.io/tls`. + - The router serviceaccount must have permission to read this secret particular secret. + - The role and rolebinding to provide this access must be provided by the user. + - CEL validations and o/library-go will enforce that both `.spec.tls.certificate` and `.spec.tls.externalCertificate` + are not specified on the route. -The router will bootstrap a watch based secret manager to ensure it can keep the +A watch based secret monitor will be introduced in the router in order to keep +track of all the secrets referenced by the routes. This component is solely +responsible for maintaining all the single item list-watch functions required +to cache the referenced secrets. + +The router will bootstrap the secret monitor to ensure it can keep the certificate/secret up-to-date. This means the router pod will maintain active watches for every secret that is referenced by a route. -Every active watch will be linked to a route, meaning the watch based secret manager +Every active watch will be linked to a route, meaning the secret monitor will be linked to the lifecycle of the route. For every new route that is created, -the secret manager will start a watch if the route uses `.spec.tls.externalCertificate`. -For every update route event, the secret manager only increments the reference count. -If a route is deleted, the secret manager will unregister the route and teardown the -watch associated with it. +the secret monitor will start a watch if the route uses `.spec.tls.externalCertificate`. +For every route created/updated referencing the same secret, the secret monitor increments +the reference count. If a route is deleted, the secret monitor will deregister the +route and teardown the watch associated with it only if reference count is zero. The `ServiceAliasConfig` creation logic will be updated in the router to also parse the secret referenced in `.spec.tls.externalCertificate`. The router will -use the default certificates only when `.spec.tls.certificate` or `.spec.tls.externalCertificate` +use the default certificates only when `.spec.tls.certificate/key` or `.spec.tls.externalCertificate` are not provided. -Validations done by the router as part of [ExtendedValidateRoute()](https://github.com/openshift/router/blob/c407ebbc5d8d85daea2ef2d1ba539444a06f4d25/pkg/router/routeapihelpers/validation.go#L158) (contents of secret), - -- Verify certificate and key (PEM encode/decode) -- Verify private key matches public certificate - -Validations done by API server as part of [ValidateRoute()](https://github.com/openshift/openshift-apiserver/blob/aac3dd5bf0547e928103a0f718ca104b1bb13930/pkg/route/apis/route/validation/validation.go#L21), - -- The secret created should be in the same namespace as that of the route. -- The secret created is of type `kubernetes.io/tls`. -- The router serviceaccount must have permission to read this secret particular secret. - - The role and rolebinding to provide this access must be provided by the user. -- CEL validations will enforce that both `.spec.tls.certificate` and `.spec.tls.externalCertificate` - are not specified on the route. - ### Risks and Mitigations There is a possibility of an invalid route being processed by the router (edge case), @@ -214,7 +227,7 @@ then this can lead to the router processing this incorrect route. The user will need to manually create, provide and maintain the rbac required by the router so that it can access secrets securely. This becomes tedious when users have -100s of Routes. +many of Routes. The workaround for this is to document various levels of rbac that can be provided, @@ -263,6 +276,11 @@ The router will need to undergo performance testing as part of OCP payload to ensure the memory implications of creating and maintains all the active watches is verified to be efficient. +Update API godoc to document that manual intervention is required for using +`.spec.tls.externalCerificate`. + +Update implementation details to cover internal working of secret monitor. + ##### Future work The ingress-to-route controller in the route-controller-manager will need to @@ -295,7 +313,7 @@ Route validation in the API server will be modified to validate the following sc - Check if secret/certificate referenced under `.spec.tls.externalCertificate` exists. - Check if secret/certificate referenced under `.spec.tls.externalCertificate` is of the correct type. - Check if router service account has permissions to read referenced secret. -- Check if route only one of the fields set, +- Check if route has only one of the fields set, - `.spec.tls.certificate` and `.spec.tls.key` - `.spec.tls.externalCertificate` From 8229b046862b13c3d59dcfe315c350045f134096 Mon Sep 17 00:00:00 2001 From: Thejas N Date: Wed, 5 Jul 2023 17:19:08 +0530 Subject: [PATCH 07/11] Update field validations in apiserver Move secret validations out of apiserver to openshift-router as the router is a much better domain for this. --- ...te-secret-injection-for-external-certificate-management.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/enhancements/ingress/route-secret-injection-for-external-certificate-management.md b/enhancements/ingress/route-secret-injection-for-external-certificate-management.md index 88e6ff0598..27037f81ad 100644 --- a/enhancements/ingress/route-secret-injection-for-external-certificate-management.md +++ b/enhancements/ingress/route-secret-injection-for-external-certificate-management.md @@ -181,13 +181,13 @@ following pre-conditions are met: - Validations done by the router as part of [ExtendedValidateRoute()](https://github.com/openshift/router/blob/c407ebbc5d8d85daea2ef2d1ba539444a06f4d25/pkg/router/routeapihelpers/validation.go#L158) (contents of secret), + - The secret created should be in the same namespace as that of the route. + - The secret created is of type `kubernetes.io/tls`. - Verify certificate and key (PEM encode/decode) - Verify private key matches public certificate - Validations done by API server as part of [ValidateRoute()](https://github.com/openshift/openshift-apiserver/blob/aac3dd5bf0547e928103a0f718ca104b1bb13930/pkg/route/apis/route/validation/validation.go#L21), - - The secret created should be in the same namespace as that of the route. - - The secret created is of type `kubernetes.io/tls`. - The router serviceaccount must have permission to read this secret particular secret. - The role and rolebinding to provide this access must be provided by the user. - CEL validations and o/library-go will enforce that both `.spec.tls.certificate` and `.spec.tls.externalCertificate` From 8a6c19e3968e9d015d37971b2aaeb9f110e30efd Mon Sep 17 00:00:00 2001 From: Thejas N Date: Mon, 17 Jul 2023 11:55:58 +0530 Subject: [PATCH 08/11] Add aditional secret monitor implementation details Update validations to introduce new check on host update --- ...ion-for-external-certificate-management.md | 28 +++++++++++++++---- 1 file changed, 23 insertions(+), 5 deletions(-) diff --git a/enhancements/ingress/route-secret-injection-for-external-certificate-management.md b/enhancements/ingress/route-secret-injection-for-external-certificate-management.md index 27037f81ad..40a079ec4e 100644 --- a/enhancements/ingress/route-secret-injection-for-external-certificate-management.md +++ b/enhancements/ingress/route-secret-injection-for-external-certificate-management.md @@ -164,7 +164,7 @@ provided in the `data` (or `stringData`) field of the Secret configuration. If neither `.spec.tls.externalCertificate` or `.spec.tls.certificate` and `.spec.tls.key` are provided the router will serve the default generated certificates. User's will not be able to -provide both `.spec.tls.certificate/key` and `.spec.tls.externalCerificate`. API server +provide both `.spec.tls.certificate/key` and `.spec.tls.externalCertificate`. API server admission validation will enforce this. All valid and invalid scenarios will be depicted via the existing `RouteIngressCondition`. @@ -193,6 +193,21 @@ following pre-conditions are met: - CEL validations and o/library-go will enforce that both `.spec.tls.certificate` and `.spec.tls.externalCertificate` are not specified on the route. +- `ValidateHostUpdate()` will be updated to introduce a new check on the usage of `.spec.tls.externalCertificate` + - User will not allowed to update `spec.host` or `spec.subDomain` when + `externalCertificate` is non-empty. This is because if all the below conditions + are satisfied, we cannot validate point 3 as we don't have the older state of the + secret referenced in `externalCertificate`. + 1. If the user does not have `update` permission on `custom-host` sub-resource + 2. If the user does not have `create` permission on `custom-host` sub-resource + 3. The user has not updated `spec.host` or `spec.subDomain`, but has updated the + contents of the secret referenced in `spec.tls.externalCertificate` + +The router being an edge component, from a security standpoint is more prone to +being compromised. In order to avoid providing the router with escalated privileges +to read all secrets, the router will implement a single item list/watch for secrets (secret monitor). +This uses name-scoped rbac (created by the user) to access the particular secrets. + A watch based secret monitor will be introduced in the router in order to keep track of all the secrets referenced by the routes. This component is solely responsible for maintaining all the single item list-watch functions required @@ -205,15 +220,18 @@ for every secret that is referenced by a route. Every active watch will be linked to a route, meaning the secret monitor will be linked to the lifecycle of the route. For every new route that is created, the secret monitor will start a watch if the route uses `.spec.tls.externalCertificate`. -For every route created/updated referencing the same secret, the secret monitor increments -the reference count. If a route is deleted, the secret monitor will deregister the -route and teardown the watch associated with it only if reference count is zero. +If a route is deleted, the secret monitor will deregister the route and teardown +the watch associated with it. The `ServiceAliasConfig` creation logic will be updated in the router to also parse the secret referenced in `.spec.tls.externalCertificate`. The router will use the default certificates only when `.spec.tls.certificate/key` or `.spec.tls.externalCertificate` are not provided. +The cluster-ingress-operator will propagate the relevant Tech-Preview feature gate down to the +router. This feature gate will be added as a command-line argument called `ROUTER_EXTERNAL_CERTIFICATE` +to the router and will not be user configurable. + ### Risks and Mitigations There is a possibility of an invalid route being processed by the router (edge case), @@ -277,7 +295,7 @@ to ensure the memory implications of creating and maintains all the active watch is verified to be efficient. Update API godoc to document that manual intervention is required for using -`.spec.tls.externalCerificate`. +`.spec.tls.externalCertificate`. Update implementation details to cover internal working of secret monitor. From 079d8f7a03dd57c5c899833e9c5780321d54ef2c Mon Sep 17 00:00:00 2001 From: Thejas N Date: Mon, 17 Jul 2023 14:26:54 +0530 Subject: [PATCH 09/11] Update open questions Move ValidateHostUpdate() validation as an open question --- ...jection-for-external-certificate-management.md | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/enhancements/ingress/route-secret-injection-for-external-certificate-management.md b/enhancements/ingress/route-secret-injection-for-external-certificate-management.md index 40a079ec4e..08d5fb4286 100644 --- a/enhancements/ingress/route-secret-injection-for-external-certificate-management.md +++ b/enhancements/ingress/route-secret-injection-for-external-certificate-management.md @@ -193,16 +193,6 @@ following pre-conditions are met: - CEL validations and o/library-go will enforce that both `.spec.tls.certificate` and `.spec.tls.externalCertificate` are not specified on the route. -- `ValidateHostUpdate()` will be updated to introduce a new check on the usage of `.spec.tls.externalCertificate` - - User will not allowed to update `spec.host` or `spec.subDomain` when - `externalCertificate` is non-empty. This is because if all the below conditions - are satisfied, we cannot validate point 3 as we don't have the older state of the - secret referenced in `externalCertificate`. - 1. If the user does not have `update` permission on `custom-host` sub-resource - 2. If the user does not have `create` permission on `custom-host` sub-resource - 3. The user has not updated `spec.host` or `spec.subDomain`, but has updated the - contents of the secret referenced in `spec.tls.externalCertificate` - The router being an edge component, from a security standpoint is more prone to being compromised. In order to avoid providing the router with escalated privileges to read all secrets, the router will implement a single item list/watch for secrets (secret monitor). @@ -262,10 +252,15 @@ The above variations need to be documented for the end user as part of OpenShift a workflow present where we can gather some early metrics (memory, cpu)? This will help in preemptively addressing performance concerns before going GA. + > Will be addressed when taking feature from TP -> GA. + - Do we make changes to the ingress-to-route controller as well? + > The ingress-to-route behaviour will remain as is i.e. it will not make use of > the newly introduced field. +- What should be the behaviour of `ValidationHostUpdate()` when using `externalCertificate`? + ### Test Plan Update router tests in openshift/origin and supplement all existing certificate related tests From 1f6063ba2c590c1e71eb458518d77f732abf614f Mon Sep 17 00:00:00 2001 From: Thejas N Date: Fri, 21 Jul 2023 12:07:08 +0530 Subject: [PATCH 10/11] Add ValidateHostExternalCertificate to implementation details Updates validations done as part of API server. Addresses open questions and other nits raised during review. Also add new `Drawback/Risk` due to validation gap. --- ...ion-for-external-certificate-management.md | 97 +++++++++++++------ 1 file changed, 66 insertions(+), 31 deletions(-) diff --git a/enhancements/ingress/route-secret-injection-for-external-certificate-management.md b/enhancements/ingress/route-secret-injection-for-external-certificate-management.md index 08d5fb4286..423c064a84 100644 --- a/enhancements/ingress/route-secret-injection-for-external-certificate-management.md +++ b/enhancements/ingress/route-secret-injection-for-external-certificate-management.md @@ -64,7 +64,7 @@ certificate data via a secret reference. - As an OpenShift engineer, I want to update the route validation in openshift/library-go in order to validate the updated Route API. - - Both Openshift and Microshift both run the o/library-go validations as part of admission plugin + - Both Openshift and Microshift run the openshift/library-go validations as part of admission plugin - As an OpenShift engineer, I want to be able to update Route API so that I can integrate OpenShift Routes with third-party certificate management solutions like cert-manager. @@ -81,15 +81,15 @@ certificate data via a secret reference. ### Non-Goals - Provide certificate life cycle management controls on the Route API (expiryAfter, renewBefore, etc). -- Modify ingress-to-route controller behaviour to use `.spec.tls.externalCertificate` +- Modify ingress-to-route controller behaviour to use the external managed certificate reference from Route API. - Extend this feature to cover CA certificate or destination CA certificate in the Route API. ## Proposal This enhancement proposes extending the openshift/router to read serving certificate data either from the Route `.spec.tls.certificate` and `.spec.tls.key` or from a new field `.spec.tls.externalCertificate` -which is a `kubernetes.io/tls` type secret reference. This `externalCertificate` field will enables the -users to provide a reference to a Secret containing the serving cert/key pair that will be parsed +which is a `kubernetes.io/tls` type secret reference. This `externalCertificate` field will enable the +users to provide a reference to a secret containing the serving cert/key pair that will be parsed and served by OpenShift router. ### Workflow Description @@ -144,7 +144,7 @@ type TLSConfig struct { // // +openshift:enable:FeatureSets=CustomNoUpgrade;TechPreviewNoUpgrade // +optional - ExternalCertificate LocalObjectReference `json:"externalCertificate,omitempty" protobuf:"bytes,7,opt,name=externalCertificate"` + ExternalCertificate *LocalObjectReference `json:"externalCertificate,omitempty" protobuf:"bytes,7,opt,name=externalCertificate"` } // LocalObjectReference contains enough information to let you locate the @@ -158,9 +158,11 @@ type LocalObjectReference struct { } ``` -_Note_: The secret is required to be created in the same namespace as that of the Route. -The secret must be of type `kubernetes.io/tls` and the tls.key and the tls.crt key must be -provided in the `data` (or `stringData`) field of the Secret configuration. +_Note_: The default value would be `nil`, both `nil` and empty `LocalObjectReference{}` +are treated as though the field is unset. The secret is required to be created in the +same namespace as that of the Route. The secret must be of type `kubernetes.io/tls` and +the tls.key and the tls.crt key must be provided in the `data` (or `stringData`) field +of the Secret configuration. If neither `.spec.tls.externalCertificate` or `.spec.tls.certificate` and `.spec.tls.key` are provided the router will serve the default generated certificates. User's will not be able to @@ -179,6 +181,35 @@ The router will read the secret referenced in `.spec.tls.externalCertificate` an the certificate inside to configure HAProxy if the secret is present and if the following pre-conditions are met: +- Validations done by API server as part of [ValidateRoute()](https://github.com/openshift/openshift-apiserver/blob/aac3dd5bf0547e928103a0f718ca104b1bb13930/pkg/route/apis/route/validation/validation.go#L21), + + - The router serviceaccount must have permission to read this secret particular secret. + - The role and rolebinding to provide this access must be provided by the user. + - CEL validations and openshfit/library-go will enforce that both `.spec.tls.certificate` and `.spec.tls.externalCertificate` + are not specified on the route at the same time. Since CEL validations are not run on Openshift API server, the same + validation will be done as part of `ValidationRoute()`. + +- New validation added to the API server as `ValidationHostExternalCertificate()` + + - Any route that is updated or created which has a non-empty `.spec.tls.externalCertificate`, + will need require additional permission checks done as changing the certificate also affects + `.spec.host` or `.spec.subdomain`. Meaning any user that is updating the certificate must also + have `create` and `update` permission on the `custom-host` sub-resource. + - Any user that does not have both these permissions will not be allowed to update/create routes + that use `.spec.tls.externalCertificate`. + - This validation is intentionally done only on the API server and not the router as well, this + will result in not having this validation for events that are generated by the secret monitor + directly to the route controller in the router. + - This validation function will be invoked before `ValidationHostUpdate()`. + +- Validations done by API server as part of [ValidationHostUpdate()](https://github.com/openshift/openshift-apiserver/blob/bd2a35e58172010c658f4d8f4dff8f9f0eac187d/pkg/route/apiserver/registry/route/strategy.go#L96) + + - If the old route or the new route uses `.spec.tls.externalCertificate` this validation will always + have the precondition [certificateChangeRequiresAuth()](https://github.com/openshift/library-go/blob/d8d3f3f8a9e4a82c110a89a13229ce1412a88e4a/pkg/route/hostassignment/assignment.go#L123C29-L123C29) return `true` since we cannot definitively + verify if the content of the secret that is referenced has been modified. Since the previous validation + func (`ValidationHostExternalCertificate()`) would have already validation user permissions, we can + safely make this assumption. + - Validations done by the router as part of [ExtendedValidateRoute()](https://github.com/openshift/router/blob/c407ebbc5d8d85daea2ef2d1ba539444a06f4d25/pkg/router/routeapihelpers/validation.go#L158) (contents of secret), - The secret created should be in the same namespace as that of the route. @@ -186,13 +217,6 @@ following pre-conditions are met: - Verify certificate and key (PEM encode/decode) - Verify private key matches public certificate -- Validations done by API server as part of [ValidateRoute()](https://github.com/openshift/openshift-apiserver/blob/aac3dd5bf0547e928103a0f718ca104b1bb13930/pkg/route/apis/route/validation/validation.go#L21), - - - The router serviceaccount must have permission to read this secret particular secret. - - The role and rolebinding to provide this access must be provided by the user. - - CEL validations and o/library-go will enforce that both `.spec.tls.certificate` and `.spec.tls.externalCertificate` - are not specified on the route. - The router being an edge component, from a security standpoint is more prone to being compromised. In order to avoid providing the router with escalated privileges to read all secrets, the router will implement a single item list/watch for secrets (secret monitor). @@ -204,16 +228,16 @@ responsible for maintaining all the single item list-watch functions required to cache the referenced secrets. The router will bootstrap the secret monitor to ensure it can keep the -certificate/secret up-to-date. This means the router pod will maintain active watches +certificate up-to-date. This means the router pod will maintain active watches for every secret that is referenced by a route. Every active watch will be linked to a route, meaning the secret monitor will be linked to the lifecycle of the route. For every new route that is created, -the secret monitor will start a watch if the route uses `.spec.tls.externalCertificate`. -If a route is deleted, the secret monitor will deregister the route and teardown -the watch associated with it. +the secret monitor will start a sharedinformer if the route uses `.spec.tls.externalCertificate`. +If a route is deleted, the secret monitor will stop the sharedinformer associated +with the route. -The `ServiceAliasConfig` creation logic will be updated in the router to also parse +The [createServiceAliasConfig()](https://github.com/openshift/router/blob/6117b7ba414c7073274e2d19c43082031393ccd7/pkg/router/template/router.go#L927) creation logic will be updated in the router to also parse the secret referenced in `.spec.tls.externalCertificate`. The router will use the default certificates only when `.spec.tls.certificate/key` or `.spec.tls.externalCertificate` are not provided. @@ -234,8 +258,8 @@ then this can lead to the router processing this incorrect route. ### Drawbacks The user will need to manually create, provide and maintain the rbac required by the -router so that it can access secrets securely. This becomes tedious when users have -many of Routes. +router so that it can access secrets. This becomes tedious when users have +many routes. The workaround for this is to document various levels of rbac that can be provided, @@ -244,6 +268,17 @@ The workaround for this is to document various levels of rbac that can be provid The above variations need to be documented for the end user as part of OpenShift documentation. +#### Exception in Validations between API server and the router + +The new `ValidationHostExternalCertificate()` is intentionally done only on the API server +and not the router as well, this will result in not having this validation for events that +are generated by the secret monitor directly to the route controller in the router. So if +a user who has the `create` and `update` permission on `custom-host` creates a route +that sets `.spec.tls.externalCertificate` the validation on the API server will pass and +the route is successfully created. Post creation of the route if the users permissions are +revoked and the user edits the contents of the secret, this would successfully reload the +certificate on the route. + ## Design Details ### Open Questions [optional] @@ -260,6 +295,8 @@ The above variations need to be documented for the end user as part of OpenShift > the newly introduced field. - What should be the behaviour of `ValidationHostUpdate()` when using `externalCertificate`? + > Addressed by introducing `ValidationHostExternalCertificate()` and which will execute + > prior to the `ValidationHostUpdate()` function. ### Test Plan @@ -290,7 +327,9 @@ to ensure the memory implications of creating and maintains all the active watch is verified to be efficient. Update API godoc to document that manual intervention is required for using -`.spec.tls.externalCertificate`. +`.spec.tls.externalCertificate`. Something simple like: "The Router service account +needs to be granted with read-only access to this secret, please refer to openshift docs +for additional details." Update implementation details to cover internal working of secret monitor. @@ -334,14 +373,10 @@ Route validation in the API server will be modified to validate the following sc ##### Referenced secret not present -In scenarios where the secret has not been created by third-party solutions -like cert-manager, the route would not be successfully created due to the -dependency. This route will be rejected by the API server with an -`FieldValueNotFound` error and will contain the reason as `referenced secret not present`. - -In addition to this validation, the router will also validate the same to -cover an edge case. If a route fails this validation, it is not processed -further and the error will be reflected on the route `.status` with the same reason. +As part of `ExtendedValidateRoute()`, the router will validate if the secret +referenced in the router exists. If a route fails this validation, it is not +processed further and the error will be reflected on the route `.status` +with the same reason. ##### Insufficient router permission From d4c7701193646105e2c60b2b996047ddfed083a4 Mon Sep 17 00:00:00 2001 From: Thejas N Date: Fri, 28 Jul 2023 15:17:20 +0530 Subject: [PATCH 11/11] Apply review suggestions --- ...ion-for-external-certificate-management.md | 38 ++++++++++--------- 1 file changed, 20 insertions(+), 18 deletions(-) diff --git a/enhancements/ingress/route-secret-injection-for-external-certificate-management.md b/enhancements/ingress/route-secret-injection-for-external-certificate-management.md index 423c064a84..efb528b781 100644 --- a/enhancements/ingress/route-secret-injection-for-external-certificate-management.md +++ b/enhancements/ingress/route-secret-injection-for-external-certificate-management.md @@ -14,7 +14,7 @@ approvers: api-approvers: # In case of new or modified APIs or API extensions (CRDs, aggregated apiservers, webhooks, finalizers). If there is no API change, use "None" - '@joelspeed' creation-date: 2022-12-13 -last-updated: 2023-07-05 +last-updated: 2023-07-28 tracking-link: # link to the tracking ticket (for example: Jira Feature or Epic ticket) that corresponds to this enhancement - https://issues.redhat.com/browse/CM-815 --- @@ -105,7 +105,7 @@ reference field described under [API Extensions](#api-extensions). - The end user must create a role in the same namespace as the secret containing the certificate which was generated by the cert-manager from earlier. ```bash - oc create role secret-reader --verb=get,list,watch --resource=secrets --resourceName= + oc create role secret-reader --verb=get,list,watch --resource=secrets --resource-name= ``` - The end user must create a rolebinding in the same namespace as the secret and bind the router serviceaccount to the above created role. @@ -183,31 +183,29 @@ following pre-conditions are met: - Validations done by API server as part of [ValidateRoute()](https://github.com/openshift/openshift-apiserver/blob/aac3dd5bf0547e928103a0f718ca104b1bb13930/pkg/route/apis/route/validation/validation.go#L21), - - The router serviceaccount must have permission to read this secret particular secret. + - The router serviceaccount must have permission to read this secret. - The role and rolebinding to provide this access must be provided by the user. - CEL validations and openshfit/library-go will enforce that both `.spec.tls.certificate` and `.spec.tls.externalCertificate` are not specified on the route at the same time. Since CEL validations are not run on Openshift API server, the same - validation will be done as part of `ValidationRoute()`. + validation will be done as part of `ValidateRoute()`. -- New validation added to the API server as `ValidationHostExternalCertificate()` +- New validation added to the API server as `ValidateHostExternalCertificate()` - Any route that is updated or created which has a non-empty `.spec.tls.externalCertificate`, - will need require additional permission checks done as changing the certificate also affects + will need additional permission checks done as changing the certificate also affects `.spec.host` or `.spec.subdomain`. Meaning any user that is updating the certificate must also have `create` and `update` permission on the `custom-host` sub-resource. - - Any user that does not have both these permissions will not be allowed to update/create routes + - Any user that does not have both of these permissions will not be allowed to update/create routes that use `.spec.tls.externalCertificate`. - - This validation is intentionally done only on the API server and not the router as well, this - will result in not having this validation for events that are generated by the secret monitor - directly to the route controller in the router. - - This validation function will be invoked before `ValidationHostUpdate()`. + - This validation function will be invoked before `ValidateHostUpdate()`. + - Refer to [Drawbacks](#drawbacks) for additional details. -- Validations done by API server as part of [ValidationHostUpdate()](https://github.com/openshift/openshift-apiserver/blob/bd2a35e58172010c658f4d8f4dff8f9f0eac187d/pkg/route/apiserver/registry/route/strategy.go#L96) +- Validations done by API server as part of [ValidateHostUpdate()](https://github.com/openshift/openshift-apiserver/blob/bd2a35e58172010c658f4d8f4dff8f9f0eac187d/pkg/route/apiserver/registry/route/strategy.go#L96) - If the old route or the new route uses `.spec.tls.externalCertificate` this validation will always have the precondition [certificateChangeRequiresAuth()](https://github.com/openshift/library-go/blob/d8d3f3f8a9e4a82c110a89a13229ce1412a88e4a/pkg/route/hostassignment/assignment.go#L123C29-L123C29) return `true` since we cannot definitively verify if the content of the secret that is referenced has been modified. Since the previous validation - func (`ValidationHostExternalCertificate()`) would have already validation user permissions, we can + func (`ValidateHostExternalCertificate()`) would have already validation user permissions, we can safely make this assumption. - Validations done by the router as part of [ExtendedValidateRoute()](https://github.com/openshift/router/blob/c407ebbc5d8d85daea2ef2d1ba539444a06f4d25/pkg/router/routeapihelpers/validation.go#L158) (contents of secret), @@ -253,7 +251,11 @@ if any changes are done to the rbac or the referenced secret is deleted after th server validation but before router has processed the route (maybe router pod is not running) then this can lead to the router processing this incorrect route. -> Will need to duplicate the validations present on the API server to the router. +> Will need to duplicate the following rbac validations present on the API server to +> the router as part of `ExtendedValidateRoute()`. +> +> - The router serviceaccount must have permission to get/list/watch this secret. +> - The role and rolebinding to provide this access must be provided by the user. ### Drawbacks @@ -275,9 +277,9 @@ and not the router as well, this will result in not having this validation for e are generated by the secret monitor directly to the route controller in the router. So if a user who has the `create` and `update` permission on `custom-host` creates a route that sets `.spec.tls.externalCertificate` the validation on the API server will pass and -the route is successfully created. Post creation of the route if the users permissions are -revoked and the user edits the contents of the secret, this would successfully reload the -certificate on the route. +the route is successfully created. Post creation of the route if the permissions for `custom-host` +are revoked and the user edits the contents of the secret, the route would still be +able successfully reload the certificate on the route. ## Design Details @@ -360,7 +362,7 @@ This feature will be added as TechPreviewNoUprade. ### Operational Aspects of API Extensions -Route validation in the API server will be modified to validate the following scenarios, +Route validation in the API server and router will be modified to validate the following scenarios, - Check if secret/certificate referenced under `.spec.tls.externalCertificate` exists. - Check if secret/certificate referenced under `.spec.tls.externalCertificate` is of the correct type.