Merge pull request #708 from Miciah/BZ2057762-set-Upgradeable-equals-…

…False-if-default-cert-has-no-SAN

Bug 2057762: Set Upgradeable=False if default cert has no SAN

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)
 			}