[release-4.15] OCPBUGS-57121: Add validation to avoid conflicts between KubeAPIServer and NamedCertificates SANs

control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go

@@ -1908,7 +1908,7 @@ func (r *HostedControlPlaneReconciler) reconcilePKI(ctx context.Context, hcp *hy
 	// KAS server secret
 	kasServerSecret := manifests.KASServerCertSecret(hcp.Namespace)
 	if _, err := createOrUpdate(ctx, r, kasServerSecret, func() error {
-		return pki.ReconcileKASServerCertSecret(kasServerSecret, rootCASecret, p.OwnerRef, p.ExternalAPIAddress, p.InternalAPIAddress, p.ServiceCIDR)
+		return pki.ReconcileKASServerCertSecret(kasServerSecret, rootCASecret, p.OwnerRef, p.ExternalAPIAddress, p.InternalAPIAddress, p.ServiceCIDR, p.NodeInternalAPIServerIP)
 	}); err != nil {
 		return fmt.Errorf("failed to reconcile kas server secret: %w", err)
 	}

control-plane-operator/controllers/hostedcontrolplane/pki/kas.go

@@ -2,7 +2,6 @@ package pki
 
 import (
 	"fmt"
-	"net"
 
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/client-go/tools/clientcmd"
@@ -14,6 +13,7 @@ import (
 	"github.com/openshift/hypershift/control-plane-operator/controllers/hostedcontrolplane/manifests"
 	"github.com/openshift/hypershift/support/certs"
 	"github.com/openshift/hypershift/support/config"
+	supportpki "github.com/openshift/hypershift/support/pki"
 	"github.com/openshift/hypershift/support/util"
 	utilsnet "k8s.io/utils/net"
 )
@@ -24,43 +24,10 @@ const (
 	ServiceSignerPublicKey  = "service-account.pub"
 )
 
-func ReconcileKASServerCertSecret(secret, ca *corev1.Secret, ownerRef config.OwnerRef, externalAPIAddress, internalAPIAddress string, serviceCIDRs []string) error {
-	svc := manifests.KubeAPIServerService(secret.Namespace)
-	svcAddresses := make([]string, 0)
-
-	for _, serviceCIDR := range serviceCIDRs {
-		serviceIP, err := util.FirstUsableIP(serviceCIDR)
-		if err != nil {
-			return fmt.Errorf("cannot get the first usable IP from CIDR %s: %w", serviceIP, err)
-		}
-		svcAddresses = append(svcAddresses, serviceIP)
-	}
-
-	dnsNames := []string{
-		"localhost",
-		"kubernetes",
-		"kubernetes.default",
-		"kubernetes.default.svc",
-		"kubernetes.default.svc.cluster.local",
-		svc.Name,
-		fmt.Sprintf("%s.%s.svc", svc.Name, svc.Namespace),
-		fmt.Sprintf("%s.%s.svc.cluster.local", svc.Name, svc.Namespace),
-	}
-	apiServerIPs := []string{
-		"127.0.0.1",
-		"0:0:0:0:0:0:0:1",
-	}
-	apiServerIPs = append(apiServerIPs, svcAddresses...)
-
-	if isNumericIP(externalAPIAddress) {
-		apiServerIPs = append(apiServerIPs, externalAPIAddress)
-	} else {
-		dnsNames = append(dnsNames, externalAPIAddress)
-	}
-	if isNumericIP(internalAPIAddress) {
-		apiServerIPs = append(apiServerIPs, internalAPIAddress)
-	} else {
-		dnsNames = append(dnsNames, internalAPIAddress)
+func ReconcileKASServerCertSecret(secret, ca *corev1.Secret, ownerRef config.OwnerRef, externalAPIAddress, internalAPIAddress string, serviceCIDRs []string, nodeInternalAPIServerIP string) error {
+	dnsNames, apiServerIPs, err := supportpki.GetKASServerCertificatesSANs(externalAPIAddress, internalAPIAddress, serviceCIDRs, nodeInternalAPIServerIP, secret.Namespace)
+	if err != nil {
+		return fmt.Errorf("failed to get KAS server certificates SANs: %w", err)
 	}
 	return reconcileSignedCertWithAddresses(secret, ca, ownerRef, "kubernetes", []string{"kubernetes"}, X509UsageServerAuth, dnsNames, apiServerIPs)
 }
@@ -98,10 +65,6 @@ func ReconcileServiceAccountKubeconfig(secret, csrSigner *corev1.Secret, ca *cor
 	return ReconcileKubeConfig(secret, secret, ca, svcURL, "", manifests.KubeconfigScopeLocal, config.OwnerRef{})
 }
 
-func isNumericIP(s string) bool {
-	return net.ParseIP(s) != nil
-}
-
 func ReconcileKubeConfig(secret, cert *corev1.Secret, ca *corev1.ConfigMap, url string, key string, scope manifests.KubeconfigScope, ownerRef config.OwnerRef) error {
 	ownerRef.ApplyTo(secret)
 	caPEM := ca.Data[certs.CASignerCertMapKey]

control-plane-operator/controllers/hostedcontrolplane/pki/konnectivity.go

@@ -4,6 +4,8 @@ import (
 	"fmt"
 
 	"github.com/openshift/hypershift/support/config"
+	supportpki "github.com/openshift/hypershift/support/pki"
+
 	corev1 "k8s.io/api/core/v1"
 )
 
@@ -31,7 +33,7 @@ func ReconcileKonnectivityClusterSecret(secret, ca *corev1.Secret, ownerRef conf
 		fmt.Sprintf("konnectivity-server.%s.svc.cluster.local", secret.Namespace),
 	}
 	ips := []string{}
-	if isNumericIP(externalKconnectivityAddress) {
+	if supportpki.IsNumericIP(externalKconnectivityAddress) {
 		ips = append(ips, externalKconnectivityAddress)
 	} else {
 		dnsNames = append(dnsNames, externalKconnectivityAddress)

go.mod

@@ -56,6 +56,7 @@ require (
 	go.etcd.io/etcd/client/v3 v3.5.10
 	go.etcd.io/etcd/server/v3 v3.5.10
 	go.etcd.io/etcd/tests/v3 v3.5.10
+	go.uber.org/mock v0.4.0
 	go.uber.org/zap v1.25.0
 	golang.org/x/crypto v0.31.0
 	golang.org/x/exp v0.0.0-20231110203233-9a3e6036ecaa

go.sum

@@ -1815,6 +1815,8 @@ go.uber.org/goleak v1.1.10/go.mod h1:8a7PlsEVH3e/a/GLqe5IIrQx6GzcnRmZEufDUTk4A7A
 go.uber.org/goleak v1.1.11/go.mod h1:cwTWslyiVhfpKIDGSZEM2HlOvcqm+tG4zioyIeLoqMQ=
 go.uber.org/goleak v1.2.0/go.mod h1:XJYK+MuIchqpmGmUSAzotztawfKvYLUIgg7guXrwVUo=
 go.uber.org/goleak v1.2.1 h1:NBol2c7O1ZokfZ0LEU9K6Whx/KnwvepVetCUhtKja4A=
+go.uber.org/mock v0.4.0 h1:VcM4ZOtdbR4f6VXfiOpwpVJDL6lCReaZ6mw31wqh7KU=
+go.uber.org/mock v0.4.0/go.mod h1:a6FSlNadKUHUa9IP5Vyt1zh4fC7uAwxMutEAscFbkZc=
 go.uber.org/multierr v1.1.0/go.mod h1:wR5kodmAFQ0UK8QlbwjlSNy0Z68gJhDJUG5sjR94q/0=
 go.uber.org/multierr v1.5.0/go.mod h1:FeouvMocqHpRaaGuG9EjoKcStLC43Zu/fmqdUMPcKYU=
 go.uber.org/multierr v1.6.0/go.mod h1:cdWPpRnG4AhwMwsgIHip0KRBQjJy5kYEpYjJxpXp9iU=

hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go

@@ -87,6 +87,7 @@ import (
 	"github.com/openshift/hypershift/hypershift-operator/controllers/hostedcluster/internal/platform"
 	platformaws "github.com/openshift/hypershift/hypershift-operator/controllers/hostedcluster/internal/platform/aws"
 	hcmetrics "github.com/openshift/hypershift/hypershift-operator/controllers/hostedcluster/metrics"
+	"github.com/openshift/hypershift/hypershift-operator/controllers/hostedcluster/validations"
 	"github.com/openshift/hypershift/hypershift-operator/controllers/manifests"
 	"github.com/openshift/hypershift/hypershift-operator/controllers/manifests/clusterapi"
 	"github.com/openshift/hypershift/hypershift-operator/controllers/manifests/controlplaneoperator"
@@ -3947,6 +3948,10 @@ func (r *HostedClusterReconciler) validateConfigAndClusterCapabilities(ctx conte
 		errs = append(errs, err...)
 	}
 
+	if err := r.validateOCPConfigurations(ctx, hc, r.Client); err != nil {
+		errs = append(errs, err)
+	}
+
 	return utilerrors.NewAggregate(errs)
 }
 
@@ -4256,6 +4261,20 @@ func (r *HostedClusterReconciler) validateNetworks(hc *hyperv1.HostedCluster) er
 	return errs.ToAggregate()
 }
 
+// validateOCPConfigurations validates OpenShift-specific configurations for a HostedCluster.
+// It's worth to abstract this validation to a separate funtion per API to have them organized.
+// Currently validates:
+// - API Server configuration
+//
+// TODO: Add validation for other OpenShift components (e.g. OAuth, Ingress, etc.)
+// Jira: https://issues.redhat.com/browse/CNTRLPLANE-382
+func (r *HostedClusterReconciler) validateOCPConfigurations(ctx context.Context, hc *hyperv1.HostedCluster, client client.Client) error {
+	var errs field.ErrorList
+	errs = append(errs, validations.ValidateOCPAPIServerSANs(ctx, hc, client)...)
+
+	return errs.ToAggregate()
+}
+
 // findAdvertiseAddress function returns a string and an error indicating the AdvertiseAddress for the hostedcluster.
 // if the advertise address is properly set, it will return that value and nil, otherwise will return an error.
 // if the advertise address is not set, it will return the default one based on the network primary stack.

hypershift-operator/controllers/hostedcluster/validations/ocpapiserver.go

@@ -0,0 +1,163 @@
+package validations
+
+import (
+	"context"
+	"encoding/pem"
+	"fmt"
+
+	hyperv1 "github.com/openshift/hypershift/api/hypershift/v1beta1"
+	"github.com/openshift/hypershift/hypershift-operator/controllers/manifests"
+	supportpki "github.com/openshift/hypershift/support/pki"
+
+	corev1 "k8s.io/api/core/v1"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/apimachinery/pkg/util/validation/field"
+
+	"sigs.k8s.io/controller-runtime/pkg/client"
+)
+
+const (
+	KASServerPrivateCertSecretName = "kas-server-private-crt"
+	KASServerCertSecretName        = "kas-server-crt"
+)
+
+func ValidateOCPAPIServerSANs(ctx context.Context, hc *hyperv1.HostedCluster, client client.Client) field.ErrorList {
+	var (
+		errs              field.ErrorList
+		err               error
+		entryCertDNSNames = make([]string, 0)
+		entryCertIPs      = make([]string, 0)
+		kasNames          = make([]string, 0)
+		kasIPs            = make([]string, 0)
+	)
+
+	// At this point, maybe the HCP is not there yet
+	if hc.Spec.Configuration != nil && hc.Spec.Configuration.APIServer != nil && hc.Spec.Configuration.APIServer.ServingCerts.NamedCertificates != nil {
+		for _, cert := range hc.Spec.Configuration.APIServer.ServingCerts.NamedCertificates {
+			entryCertDNSNames = append(entryCertDNSNames, cert.Names...)
+			if len(cert.ServingCertificate.Name) > 0 {
+				secret := &corev1.Secret{}
+				err = client.Get(ctx, types.NamespacedName{Namespace: hc.Namespace, Name: cert.ServingCertificate.Name}, secret)
+				if err != nil {
+					errs = append(errs, field.Invalid(field.NewPath("NamedCertificates get secret"), cert.ServingCertificate.Name, err.Error()))
+					return errs
+				}
+				entryCertDNSNames, entryCertIPs, err = getSANsFromSecretCert(entryCertDNSNames, entryCertIPs, secret)
+				if err != nil {
+					errs = append(errs, field.Invalid(field.NewPath("KAS TLS private cert decrypt"), KASServerPrivateCertSecretName, err.Error()))
+					return errs
+				}
+			}
+		}
+
+		kasNames, kasIPs, err = supportpki.GetKASServerCertificatesSANs("", fmt.Sprintf("api.%s.hypershift.local", hc.Name), []string{}, "", hc.Namespace)
+		if err != nil {
+			errs = append(errs, field.Invalid(field.NewPath("Hypershift KAS SANs"), entryCertDNSNames, err.Error()))
+		}
+
+		if err := checkConflictingSANs(entryCertDNSNames, kasNames, "DNS names"); err != nil {
+			errs = append(errs, field.Invalid(field.NewPath("conflicting entries with KAS SANs"), entryCertDNSNames, err.Error()))
+			return errs
+		}
+
+		if err := checkConflictingSANs(entryCertIPs, kasIPs, "IP addresses"); err != nil {
+			errs = append(errs, field.Invalid(field.NewPath("conflicting entries with KAS SANs"), entryCertIPs, err.Error()))
+			return errs
+		}
+	}
+
+	hcpNamespace := manifests.HostedControlPlaneNamespace(hc.Namespace, hc.Name)
+
+	// Check the KAS TLS private secret
+	kasServerPrivateSecret := &corev1.Secret{}
+	err = client.Get(ctx, types.NamespacedName{Namespace: hcpNamespace, Name: KASServerPrivateCertSecretName}, kasServerPrivateSecret)
+	if err != nil {
+		if !apierrors.IsNotFound(err) {
+			errs = append(errs, field.Invalid(field.NewPath("KAS TLS secret"), "error grabbing KAS TLS secret", err.Error()))
+		}
+		// return early, we can assume that the KAS is not there yet
+		return errs
+	}
+	kasNames, kasIPs, err = getSANsFromSecretCert(kasNames, kasIPs, kasServerPrivateSecret)
+	if err != nil {
+		errs = append(errs, field.Invalid(field.NewPath("KAS TLS cert decrypt"), KASServerPrivateCertSecretName, err.Error()))
+	}
+
+	// Check the KAS TLS certificate secret
+	kasServerCertSecret := &corev1.Secret{}
+	err = client.Get(ctx, types.NamespacedName{Namespace: hcpNamespace, Name: KASServerCertSecretName}, kasServerCertSecret)
+	if err != nil {
+		if !apierrors.IsNotFound(err) {
+			errs = append(errs, field.Invalid(field.NewPath("KAS TLS secret"), "error grabbing KAS TLS secret", err.Error()))
+		}
+		// return early, we can assume that the KAS is not there yet
+		return errs
+	}
+
+	kasNames, kasIPs, err = getSANsFromSecretCert(kasNames, kasIPs, kasServerCertSecret)
+	if err != nil {
+		errs = append(errs, field.Invalid(field.NewPath("KAS TLS cert decrypt"), KASServerCertSecretName, err.Error()))
+	}
+
+	if err := checkConflictingSANs(entryCertDNSNames, kasNames, "DNS names"); err != nil {
+		errs = append(errs, field.Invalid(field.NewPath("custom serving cert"), entryCertDNSNames, err.Error()))
+		return errs
+	}
+
+	if err := checkConflictingSANs(entryCertIPs, kasIPs, "IP addresses"); err != nil {
+		errs = append(errs, field.Invalid(field.NewPath("custom serving cert"), entryCertIPs, err.Error()))
+		return errs
+	}
+
+	return errs
+}
+
+func getSANsFromSecretCert(entryCertDNSNames []string, entryCertIPs []string, secretCert *corev1.Secret) ([]string, []string, error) {
+	if secretCert == nil || secretCert.Data == nil || len(secretCert.Data["tls.crt"]) == 0 {
+		return nil, nil, fmt.Errorf("TLS secret or certificate entries are empty")
+	}
+
+	// Try to parse the certificate as PEM
+	block, _ := pem.Decode(secretCert.Data["tls.crt"])
+	if block == nil {
+		return nil, nil, fmt.Errorf("failed to decode PEM block from certificate")
+	}
+
+	certSANsDNS, certSANsIPs, err := supportpki.GetSANsFromCertificate(block.Bytes)
+	if err != nil {
+		return nil, nil, fmt.Errorf("error decrypting TLS certificate: %w", err)
+	}
+
+	tempEntryCertDNSNames := appendEntriesIfNotExists(entryCertDNSNames, certSANsDNS)
+	tempEntryCertIPs := appendEntriesIfNotExists(entryCertIPs, certSANsIPs)
+
+	return tempEntryCertDNSNames, tempEntryCertIPs, nil
+}
+
+func containsString(slice []string, s string) bool {
+	for _, v := range slice {
+		if v == s {
+			return true
+		}
+	}
+	return false
+}
+
+func appendEntriesIfNotExists(slice []string, entries []string) []string {
+	for _, entry := range entries {
+		if !containsString(slice, entry) {
+			slice = append(slice, entry)
+		}
+	}
+	return slice
+}
+
+func checkConflictingSANs(customEntries []string, kasSANEntries []string, entryType string) error {
+	for _, customEntry := range customEntries {
+		if containsString(kasSANEntries, customEntry) {
+			return fmt.Errorf("conflicting %s found in KAS SANs. Configuration is invalid", entryType)
+		}
+	}
+	return nil
+}