From 15aebc43b217c73b12c8e67248de2ae6a74e2423 Mon Sep 17 00:00:00 2001
From: "W. Trevor King" <wking@tremily.us>
Date: Mon, 17 Apr 2023 15:44:32 -0700
Subject: [PATCH] pkg/cvo/availableupdates: Clear TLS config for
 *.cluster.local Cincinnati

Sometimes folks point their ClusterVersion spec.upstream at a local
OpenShift Update Service running in the same cluster.

a. Our docs recommend using the UpdateService status.policyEngineURI
   as the ClusterVersion spec.upstream [1].

b. The cluster-version operator loads Proxy information, as described
   in [2].  Before this commit, it assumed that the set of trusted CAs
   in the trusted-ca-bundle ConfigMap was the complete trust set, both
   for requests through the proxy and requests to no-proxy endpoints.

c. The network operator's Proxy controller combined any user-request
   trustedCA content with the system default trust bundle [3], but
   this did not include the current Ingress wildcard certificate CA.

d. From our docs [4,5]:

     The Ingress Operator generates a default certificate for an
     Ingress Controller to serve as a placeholder until you configure
     a custom default certificate. Do not use Operator-generated
     default certificates in production clusters.

When cluster admins:

* Neglect the docs-recommended trusted Ingress certificate for (d),
  e.g. because they are testing in a non-production channel, and
* Use the docs-recommended status.policyEngineURI for (a).

neither (c) nor (d) would recover for them, and the Cincinnati request
would fail on missing TLS/X.509 trust [6].

It seems like this would be a generic issue that could be sorted at
the network operator's (c), for all Proxy-consuming resources who
might want to reach back into the cluster via the Ingress router.  But
as a cheap hack until something like that happens, users can use:

  http://${POLICY_ENGINE_SERVICE_NAME}.${NAMESPACE}.svc.cluster.local/api/upgrades_info/graph

to bypass TLS.  Or, if they'd rather use encrypted communication, this
commit allows them to use:

  https://${POLICY_ENGINE_SERVICE_NAME}.${NAMESPACE}.svc.cluster.local/api/upgrades_info/graph

This commit breaks a few layers of abstraction:

* Hard-coding .cluster.local as a known no-proxy path, although that's
  formally documented in [3].
* Neglecting similar handling for release signatures, because
  getTransport -> HTTPClient -> loadConfigMapVerifierDataFromUpdate
  doesn't currently allow for "but if the target URI is noProxy, skip
  the TLS customization, and fall back to default trust stores".
* Requiring consumers to reach around status.policyEngineURI and talk
  directly to the underlying local Service.

But it's only a few lines, even if it's incomplete, awkward, and
brittle.  Folks concerned about any of those limitations can advocate
for changes to the network operator's (c) so there's a generic
decision around how to handle this kind of problem.  Or they can
explain why the network operator's current handling is appropriate,
and only the cluster-version operator needs Ingress CA injection (and
if so, whether that should happen only for Cincinnati requests, or
also for signature requests).

[1]: https://docs.openshift.com/container-platform/4.12/updating/updating-restricted-network-cluster/restricted-network-update-osus.html#update-service-create-service-cli_updating-restricted-network-cluster-osus
[2]: https://github.com/openshift/enhancements/blob/6b3209fa18ab3161429743550eed36391efc785f/enhancements/proxy/global-cluster-egress-proxy.md
[3]: https://github.com/openshift/api/blob/1b2161d23365fb5918167b2ba73e90ff80ca1805/config/v1/types_proxy.go#L50-L58
[4]: https://docs.openshift.com/container-platform/4.11/security/certificate_types_descriptions/ingress-certificates.html#location
[5]: https://github.com/openshift/openshift-docs/blame/53aa6335eb28cdc9ac0888b05002b374342f7b1e/security/certificate_types_descriptions/ingress-certificates.adoc#L24
[6]: https://issues.redhat.com/browse/OCPBUGS-1694
---
 pkg/cvo/availableupdates.go | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/pkg/cvo/availableupdates.go b/pkg/cvo/availableupdates.go
index 317f21b6cb..6f8de5c644 100644
--- a/pkg/cvo/availableupdates.go
+++ b/pkg/cvo/availableupdates.go
@@ -234,6 +234,10 @@ func calculateAvailableUpdatesStatus(ctx context.Context, clusterID string, tran
 		}
 	}
 
+	if strings.HasSuffix(upstreamURI.Hostname(), ".cluster.local") {
+		transport.TLSClientConfig = nil
+	}
+
 	uuid, err := uuid.Parse(string(clusterID))
 	if err != nil {
 		return cvoCurrent, nil, nil, configv1.ClusterOperatorStatusCondition{