Set Upgradeable=False if default cert has no SAN

If an ingresscontroller's default certificate has a Common Name (CN)) but
has no Subject Alternative Name (SAN) for the ingress domain, report that
the cluster cannot be upgraded by setting the Upgradeable=False status
condition on the ingresscontroller and clusteroperator.

Clients built using Go 1.17 reject certificates without SANs.  OpenShift
4.10 is built using Go 1.17, which means that various operators that
connect to routes that use the default certificate would reject the
certificate and fail to complete the TLS handshake after upgrading to
OpenShift 4.10 if the ingress operator didn't block the upgrade on a
cluster with a problematic certificate.

The commit is related to bug 2057762.

https://bugzilla.redhat.com/show_bug.cgi?id=2057762

* pkg/operator/controller/ingress/status.go (syncIngressControllerStatus):
Get the default certificate secret and pass it to
computeIngressUpgradeableCondition.
(computeIngressUpgradeableCondition): Add a parameter for the default
certificate secret.  Use the argument value to call the new
checkDefaultCertificate function to check for problematic certificates.
(checkDefaultCertificate): New function.  Return a non-nil error value if
the default certificate in the provided secret has a CN for the ingress
domain and no SAN for the same.
* pkg/operator/controller/ingress/status_test.go
(TestComputeIngressUpgradeableCondition): Verify that
computeIngressUpgradeableCondition reports Upgradeable=False if the default
certificate has a CN and no SAN for the ingress domain and reports
Upgradeable=True otherwise.

pkg/operator/controller/ingress/status.go

@@ -2,6 +2,8 @@ package ingress
 
 import (
 	"context"
+	"crypto/x509"
+	"encoding/pem"
 	"errors"
 	"fmt"
 	"reflect"
@@ -12,6 +14,7 @@ import (
 	"github.com/google/go-cmp/cmp/cmpopts"
 
 	"github.com/openshift/cluster-ingress-operator/pkg/manifests"
+	"github.com/openshift/cluster-ingress-operator/pkg/operator/controller"
 	"github.com/openshift/cluster-ingress-operator/pkg/util/retryableerror"
 
 	configv1 "github.com/openshift/api/config/v1"
@@ -23,6 +26,7 @@ import (
 	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
 
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
 	utilclock "k8s.io/apimachinery/pkg/util/clock"
@@ -57,6 +61,12 @@ func (r *reconciler) syncIngressControllerStatus(ic *operatorv1.IngressControlle
 		return fmt.Errorf("failed to determine infrastructure platform status for ingresscontroller %s/%s: %w", ic.Namespace, ic.Name, err)
 	}
 
+	secret := &corev1.Secret{}
+	secretName := controller.RouterEffectiveDefaultCertificateSecretName(ic, deployment.Namespace)
+	if err := r.client.Get(context.TODO(), secretName, secret); err != nil && !apierrors.IsNotFound(err) {
+		return fmt.Errorf("failed to get the default certificate secret %s for ingresscontroller %s/%s: %w", secretName, ic.Namespace, ic.Name, err)
+	}
+
 	var errs []error
 
 	updated := ic.DeepCopy()
@@ -74,7 +84,7 @@ func (r *reconciler) syncIngressControllerStatus(ic *operatorv1.IngressControlle
 	errs = append(errs, err)
 	updated.Status.Conditions = MergeConditions(updated.Status.Conditions, computeIngressProgressingCondition(updated.Status.Conditions, ic, service, platform))
 	updated.Status.Conditions = MergeConditions(updated.Status.Conditions, degradedCondition)
-	updated.Status.Conditions = MergeConditions(updated.Status.Conditions, computeIngressUpgradeableCondition(ic, deploymentRef, service, platform))
+	updated.Status.Conditions = MergeConditions(updated.Status.Conditions, computeIngressUpgradeableCondition(ic, deploymentRef, service, platform, secret))
 
 	updated.Status.Conditions = PruneConditions(updated.Status.Conditions)
 
@@ -539,9 +549,11 @@ func computeIngressDegradedCondition(conditions []operatorv1.OperatorCondition,
 }
 
 // computeIngressUpgradeableCondition computes the IngressController's "Upgradeable" status condition.
-func computeIngressUpgradeableCondition(ic *operatorv1.IngressController, deploymentRef metav1.OwnerReference, service *corev1.Service, platform *configv1.PlatformStatus) operatorv1.OperatorCondition {
+func computeIngressUpgradeableCondition(ic *operatorv1.IngressController, deploymentRef metav1.OwnerReference, service *corev1.Service, platform *configv1.PlatformStatus, secret *corev1.Secret) operatorv1.OperatorCondition {
 	var errs []error
 
+	errs = append(errs, checkDefaultCertificate(secret, "*."+ic.Status.Domain))
+
 	if service != nil {
 		errs = append(errs, loadBalancerServiceIsUpgradeable(ic, deploymentRef, service, platform))
 	}
@@ -563,6 +575,48 @@ func computeIngressUpgradeableCondition(ic *operatorv1.IngressController, deploy
 	}
 }
 
+// checkDefaultCertificate returns an error value indicating whether the default
+// certificate is safe for upgrades.  In particular, if the current default
+// certificate specifies a Subject Alternative Name (SAN) for the ingress
+// domain, then it is safe to upgrade, and the return value is nil.  Otherwise,
+// if the certificate has a legacy Common Name (CN) and no SAN, then the return
+// value is an error indicating that the certificate must be replaced by one
+// with a SAN before upgrading is allowed.  This check is necessary because
+// OpenShift 4.10 and newer are built using Go 1.17, which rejects certificates
+// without SANs.  Note that this function only checks the validity of the
+// certificate insofar as it affects upgrades.
+func checkDefaultCertificate(secret *corev1.Secret, domain string) error {
+	var certData []byte
+	if v, ok := secret.Data["tls.crt"]; !ok {
+		return nil
+	} else {
+		certData = v
+	}
+
+	for len(certData) > 0 {
+		block, data := pem.Decode(certData)
+		if block == nil {
+			break
+		}
+		certData = data
+		cert, err := x509.ParseCertificate(block.Bytes)
+		if err != nil {
+			continue
+		}
+		foundSAN := false
+		for i := range cert.DNSNames {
+			if cert.DNSNames[i] == domain {
+				foundSAN = true
+			}
+		}
+		if cert.Subject.CommonName == domain && !foundSAN {
+			return fmt.Errorf("certificate in secret %s/%s has legacy Common Name (CN) but has no Subject Alternative Name (SAN) for domain: %s", secret.Namespace, secret.Name, domain)
+		}
+	}
+
+	return nil
+}
+
 func formatConditions(conditions []*operatorv1.OperatorCondition) string {
 	var formatted string
 	if len(conditions) == 0 {

pkg/operator/controller/ingress/status_test.go

@@ -1,6 +1,12 @@
 package ingress
 
 import (
+	"crypto/rand"
+	"crypto/rsa"
+	"crypto/x509"
+	"crypto/x509/pkix"
+	"encoding/pem"
+	"math/big"
 	"strings"
 	"testing"
 	"time"
@@ -1360,9 +1366,39 @@ func TestZoneInConfig(t *testing.T) {
 }
 
 func TestComputeIngressUpgradeableCondition(t *testing.T) {
+	makeDefaultCertificateSecret := func(cn string, sans []string) *corev1.Secret {
+		key, err := rsa.GenerateKey(rand.Reader, 2048)
+		if err != nil {
+			t.Fatalf("failed to generate key: %v", err)
+		}
+
+		certTemplate := &x509.Certificate{
+			SerialNumber: big.NewInt(1),
+			Subject:      pkix.Name{CommonName: cn},
+			DNSNames:     sans,
+		}
+		cert, err := x509.CreateCertificate(rand.Reader, certTemplate, certTemplate, &key.PublicKey, key)
+		if err != nil {
+			t.Fatalf("failed to generate certificate: %v", err)
+		}
+
+		certData := pem.EncodeToMemory(&pem.Block{
+			Type:  "CERTIFICATE",
+			Bytes: cert,
+		})
+
+		return &corev1.Secret{
+			Data: map[string][]byte{"tls.crt": certData},
+		}
+	}
+	const (
+		ingressDomain  = "apps.foo.com"
+		wildcardDomain = "*." + ingressDomain
+	)
 	testCases := []struct {
 		description string
 		mutate      func(*corev1.Service)
+		secret      *corev1.Secret
 		expect      bool
 	}{
 		{
@@ -1379,6 +1415,16 @@ func TestComputeIngressUpgradeableCondition(t *testing.T) {
 			},
 			expect: false,
 		},
+		{
+			description: "if the default certificate has a SAN",
+			secret:      makeDefaultCertificateSecret(wildcardDomain, []string{wildcardDomain}),
+			expect:      true,
+		},
+		{
+			description: "if the default certificate has no SAN",
+			secret:      makeDefaultCertificateSecret(wildcardDomain, []string{}),
+			expect:      false,
+		},
 	}
 	for _, tc := range testCases {
 		t.Run(tc.description, func(t *testing.T) {
@@ -1390,6 +1436,7 @@ func TestComputeIngressUpgradeableCondition(t *testing.T) {
 					EndpointPublishingStrategy: &operatorv1.EndpointPublishingStrategy{
 						Type: operatorv1.LoadBalancerServiceStrategyType,
 					},
+					Domain: ingressDomain,
 				},
 			}
 			trueVar := true
@@ -1420,14 +1467,20 @@ func TestComputeIngressUpgradeableCondition(t *testing.T) {
 				t.Errorf("%q: unexpected false value from desiredLoadBalancerService", tc.description)
 				return
 			}
-			tc.mutate(service)
+			if tc.mutate != nil {
+				tc.mutate(service)
+			}
+			secret := tc.secret
+			if secret == nil {
+				secret = makeDefaultCertificateSecret("", []string{wildcardDomain})
+			}
 
 			expectedStatus := operatorv1.ConditionFalse
 			if tc.expect {
 				expectedStatus = operatorv1.ConditionTrue
 			}
 
-			actual := computeIngressUpgradeableCondition(ic, deploymentRef, service, platformStatus)
+			actual := computeIngressUpgradeableCondition(ic, deploymentRef, service, platformStatus, secret)
 			if actual.Status != expectedStatus {
 				t.Errorf("%q: expected Upgradeable to be %q, got %q", tc.description, expectedStatus, actual.Status)
 			}