OCPBUGS-6661: Handle mTLS CRLs in the router

go.mod

@@ -22,7 +22,8 @@ require (
 	github.com/openshift/api v3.9.1-0.20190924102528-32369d4db2ad+incompatible
 	github.com/openshift/client-go v0.0.0-20230120202327-72f107311084
 	github.com/openshift/library-go v0.0.0-20230503125631-c31a6e7b87d4
-	github.com/operator-framework/api v0.3.7-0.20200528122852-759ca0d84007
+	github.com/operator-framework/api v0.15.0
+	github.com/operator-framework/operator-lib v0.11.0
 	github.com/pkg/errors v0.9.1
 	github.com/prometheus/client_golang v1.14.0
 	github.com/prometheus/common v0.37.0
@@ -58,7 +59,6 @@ require (
 	github.com/AzureAD/microsoft-authentication-library-for-go v0.5.1 // indirect
 	github.com/asaskevich/govalidator v0.0.0-20200907205600-7a23bdc65eef // indirect
 	github.com/beorn7/perks v1.0.1 // indirect
-	github.com/blang/semver v3.5.1+incompatible // indirect
 	github.com/blang/semver/v4 v4.0.0 // indirect
 	github.com/cespare/xxhash/v2 v2.1.2 // indirect
 	github.com/emicklei/go-restful/v3 v3.9.0 // indirect

go.sum

@@ -182,7 +182,6 @@ github.com/bitly/go-simplejson v0.5.0/go.mod h1:cXHtHw4XUPsvGaxgjIAn8PhEWG9NfngE
 github.com/bketelsen/crypt v0.0.3-0.20200106085610-5cbc8cc4026c/go.mod h1:MKsuJmJgSg28kpZDP6UIiPt0e0Oz0kqKNGyRaWEPv84=
 github.com/blang/semver v3.1.0+incompatible/go.mod h1:kRBLl5iJ+tD4TcOOxsy/0fnwebNt5EWlYSAyrTnjyyk=
 github.com/blang/semver v3.5.0+incompatible/go.mod h1:kRBLl5iJ+tD4TcOOxsy/0fnwebNt5EWlYSAyrTnjyyk=
-github.com/blang/semver v3.5.1+incompatible h1:cQNTCjp13qL8KC3Nbxr/y2Bqb63oX6wdnnjpJbkM4JQ=
 github.com/blang/semver v3.5.1+incompatible/go.mod h1:kRBLl5iJ+tD4TcOOxsy/0fnwebNt5EWlYSAyrTnjyyk=
 github.com/blang/semver/v4 v4.0.0 h1:1PFHFE6yCCTv8C1TeyNNarDzntLi7wMI5i/pzqYIsAM=
 github.com/blang/semver/v4 v4.0.0/go.mod h1:IbckMUScFkM3pff0VJDNKRiT6TG/YpiHIM2yvyW5YoQ=
@@ -939,8 +938,8 @@ github.com/onsi/ginkgo v1.11.0/go.mod h1:lLunBs/Ym6LB5Z9jYTR76FiuTmxDTDusOGeTQH+
 github.com/onsi/ginkgo v1.12.0/go.mod h1:oUhWkIvk5aDxtKvDDuw8gItl8pKl42LzjC9KZE0HfGg=
 github.com/onsi/ginkgo v1.12.1/go.mod h1:zj2OWP4+oCPe1qIXoGWkgMRwljMUYCdkwsT2108oapk=
 github.com/onsi/ginkgo v1.14.2/go.mod h1:iSB4RoI2tjJc9BBv4NKIKWKya62Rps+oPG/Lv9klQyY=
-github.com/onsi/ginkgo v1.16.4 h1:29JGrr5oVBm5ulCWet69zQkzWipVXIol6ygQUe/EzNc=
 github.com/onsi/ginkgo v1.16.4/go.mod h1:dX+/inL/fNMqNlz0e9LfyB9TswhZpCVdJM/Z6Vvnwo0=
+github.com/onsi/ginkgo v1.16.5 h1:8xi0RTUf59SOSfEtZMvwTvXYMzG4gV23XVHOZiXNtnE=
 github.com/onsi/ginkgo/v2 v2.1.3/go.mod h1:vw5CSIxN1JObi/U8gcbwft7ZxR2dgaR70JSE3/PpL4c=
 github.com/onsi/ginkgo/v2 v2.1.4/go.mod h1:um6tUpWM/cxCK3/FK8BXqEiUMUwRgSM4JXG47RKZmLU=
 github.com/onsi/ginkgo/v2 v2.1.6/go.mod h1:MEH45j8TBi6u9BMogfbp0stKC5cdGjumZj5Y7AG4VIk=
@@ -996,8 +995,11 @@ github.com/opentracing/opentracing-go v1.1.0/go.mod h1:UkNAQd3GIcIGf0SeVgPpRdFSt
 github.com/openzipkin/zipkin-go v0.1.6/go.mod h1:QgAqvLzwWbR/WpD4A3cGpPtJrZXNIiJc5AZX7/PBEpw=
 github.com/operator-framework/api v0.1.1/go.mod h1:yzNYR7qyJqRGOOp+bT6Z/iYSbSPNxeh3Si93Gx/3OBY=
 github.com/operator-framework/api v0.3.4/go.mod h1:TmRmw+8XOUaDPq6SP9gA8cIexNf/Pq8LMFY7YaKQFTs=
-github.com/operator-framework/api v0.3.7-0.20200528122852-759ca0d84007 h1:fbpQelcJmwRofhaRrMOUynmBcT6SBhOZOjSlgaXVyyk=
 github.com/operator-framework/api v0.3.7-0.20200528122852-759ca0d84007/go.mod h1:Xbje9x0SHmh0nihE21kpesB38vk3cyxnE6JdDS8Jo1Q=
+github.com/operator-framework/api v0.15.0 h1:4f9i0drtqHj7ykLoHxv92GR43S7MmQHhmFQkfm5YaGI=
+github.com/operator-framework/api v0.15.0/go.mod h1:scnY9xqSeCsOdtJtNoHIXd7OtHZ14gj1hkDA4+DlgLY=
+github.com/operator-framework/operator-lib v0.11.0 h1:eYzqpiOfq9WBI4Trddisiq/X9BwCisZd3rIzmHRC9Z8=
+github.com/operator-framework/operator-lib v0.11.0/go.mod h1:RpyKhFAoG6DmKTDIwMuO6pI3LRc8IE9rxEYWy476o6g=
 github.com/operator-framework/operator-registry v1.5.3/go.mod h1:agrQlkWOo1q8U1SAaLSS2WQ+Z9vswNT2M2HFib9iuLY=
 github.com/operator-framework/operator-registry v1.12.1/go.mod h1:rf4b/h77GUv1+geiej2KzGRQr8iBLF4dXNwr5AuGkrQ=
 github.com/operator-framework/operator-registry v1.12.4/go.mod h1:JChIivJVLE1wRbgIhDFzYQYT9yosa2wd6qiTyMuG5mg=

manifests/02-deployment-ibm-cloud-managed.yaml

@@ -41,7 +41,7 @@ spec:
             fieldRef:
               fieldPath: metadata.namespace
         - name: IMAGE
-          value: openshift/origin-haproxy-router:v4.0
+          value: quay.io/rfredette/openshift-router:haproxy-router-c0f5c8ef
         - name: CANARY_IMAGE
           value: openshift/origin-cluster-ingress-operator:latest
         image: openshift/origin-cluster-ingress-operator:latest

manifests/02-deployment.yaml

@@ -70,7 +70,7 @@ spec:
                 fieldRef:
                   fieldPath: metadata.namespace
             - name: IMAGE
-              value: openshift/origin-haproxy-router:v4.0
+              value: quay.io/rfredette/openshift-router:haproxy-router-c0f5c8ef
             - name: CANARY_IMAGE
               value: openshift/origin-cluster-ingress-operator:latest
           resources:

pkg/operator/controller/crl/crl_configmap.go

@@ -1,38 +1,17 @@
 package crl
 
 import (
-	"bytes"
 	"context"
-	"crypto/x509"
-	"crypto/x509/pkix"
-	"encoding/asn1"
-	"encoding/hex"
-	"encoding/pem"
 	"fmt"
-	"io/ioutil"
-	"net/http"
-	"reflect"
-	"strings"
-	"time"
 
 	operatorv1 "github.com/openshift/api/operator/v1"
 	"github.com/openshift/cluster-ingress-operator/pkg/operator/controller"
 
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	kerrors "k8s.io/apimachinery/pkg/util/errors"
 )
 
-// authorityKeyIdentifier is a certificate's authority key identifier.
-type authorityKeyIdentifier struct {
-	KeyIdentifier []byte `asn1:"optional,tag:0"`
-}
-
-// authorityKeyIdentifierOID is the ASN.1 object identifier for the authority
-// key identifier extension.
-var authorityKeyIdentifierOID = asn1.ObjectIdentifier{2, 5, 29, 35}
-
 // ensureCRLConfigmap ensures the client CA certificate revocation list
 // configmap exists for a given ingresscontroller if the ingresscontroller
 // specifies a client CA certificate bundle in which any certificates specify
@@ -44,32 +23,9 @@ func (r *reconciler) ensureCRLConfigmap(ctx context.Context, ic *operatorv1.Ingr
 		return false, nil, ctx, err
 	}
 
-	var oldCRLs map[string]*pkix.CertificateList
-	if haveCM {
-		if data, ok := current.Data["crl.pem"]; ok {
-			if crls, err := buildCRLMap([]byte(data)); err != nil {
-				log.Error(err, "failed to parse current client CA configmap", "namespace", current.Namespace, "name", current.Name)
-			} else {
-				oldCRLs = crls
-			}
-		}
-
-	}
-
-	var clientCAData []byte
-	if haveClientCA {
-		clientCABundleFilename := "ca-bundle.pem"
-		if data, ok := clientCAConfigmap.Data[clientCABundleFilename]; !ok {
-			return haveCM, current, ctx, fmt.Errorf("client CA configmap %s/%s is missing %q", clientCAConfigmap.Namespace, clientCAConfigmap.Name, clientCABundleFilename)
-		} else {
-			clientCAData = []byte(data)
-		}
-	}
-
-	wantCM, desired, ctx, err := desiredCRLConfigMap(ctx, ic, ownerRef, clientCAData, oldCRLs)
-	if err != nil {
-		return false, nil, ctx, fmt.Errorf("failed to build configmap: %w", err)
-	}
+	// The CRL management code has been moved into the router, so the CRL configmap is no longer necessary.
+	// TODO: Remove this whole controller after 4.13
+	wantCM := false
 
 	switch {
 	case !wantCM && !haveCM:
@@ -83,191 +39,8 @@ func (r *reconciler) ensureCRLConfigmap(ctx context.Context, ic *operatorv1.Ingr
 			log.Info("deleted configmap", "namespace", current.Namespace, "name", current.Name)
 		}
 		return false, nil, ctx, nil
-	case wantCM && !haveCM:
-		if err := r.client.Create(ctx, desired); err != nil {
-			return false, nil, ctx, fmt.Errorf("failed to create configmap: %w", err)
-		}
-		log.Info("created configmap", "namespace", desired.Namespace, "name", desired.Name)
-		exists, current, err := r.currentCRLConfigMap(ctx, ic)
-		return exists, current, ctx, err
-	case wantCM && haveCM:
-		if updated, err := r.updateCRLConfigMap(ctx, current, desired); err != nil {
-			return true, current, ctx, fmt.Errorf("failed to update configmap: %w", err)
-		} else if updated {
-			log.Info("updated configmap", "namespace", desired.Namespace, "name", desired.Name)
-			exists, current, err := r.currentCRLConfigMap(ctx, ic)
-			return exists, current, ctx, err
-		}
-	}
-
-	return true, current, ctx, nil
-}
-
-// buildCRLMap builds a map of key identifier to certificate list using the
-// provided PEM-encoded certificate revocation list.
-func buildCRLMap(crlData []byte) (map[string]*pkix.CertificateList, error) {
-	crlForKeyId := make(map[string]*pkix.CertificateList)
-	for len(crlData) > 0 {
-		block, data := pem.Decode(crlData)
-		if block == nil {
-			break
-		}
-		crl, err := x509.ParseCRL(block.Bytes)
-		if err != nil {
-			return crlForKeyId, err
-		}
-		for _, ext := range crl.TBSCertList.Extensions {
-			if ext.Id.Equal(authorityKeyIdentifierOID) {
-				var authKeyId authorityKeyIdentifier
-				if _, err := asn1.Unmarshal(ext.Value, &authKeyId); err != nil {
-					return crlForKeyId, err
-				}
-				subjectKeyId := hex.EncodeToString(authKeyId.KeyIdentifier)
-				crlForKeyId[subjectKeyId] = crl
-			}
-		}
-		crlData = data
-	}
-	return crlForKeyId, nil
-}
-
-// desiredCRLConfigMap returns the desired CRL configmap.  Returns a Boolean
-// indicating whether a configmap is desired, the configmap if one is desired,
-// the context (containing the next CRL update time as "nextCRLUpdate"), and an
-// error if one occurred
-func desiredCRLConfigMap(ctx context.Context, ic *operatorv1.IngressController, ownerRef metav1.OwnerReference, clientCAData []byte, crls map[string]*pkix.CertificateList) (bool, *corev1.ConfigMap, context.Context, error) {
-	if len(ic.Spec.ClientTLS.ClientCertificatePolicy) == 0 || len(ic.Spec.ClientTLS.ClientCA.Name) == 0 {
-		return false, nil, ctx, nil
-	}
-
-	if crls == nil {
-		crls = make(map[string]*pkix.CertificateList)
-	}
-
-	var subjectKeyIds []string
-	var nextCRLUpdate time.Time
-	now := time.Now()
-	for len(clientCAData) > 0 {
-		block, data := pem.Decode(clientCAData)
-		if block == nil {
-			break
-		}
-		clientCAData = data
-		cert, err := x509.ParseCertificate(block.Bytes)
-		if err != nil {
-			return false, nil, ctx, fmt.Errorf("client CA configmap has an invalid certificate: %w", err)
-		}
-		subjectKeyId := hex.EncodeToString(cert.SubjectKeyId)
-		if len(cert.CRLDistributionPoints) == 0 {
-			continue
-		}
-		if crl, ok := crls[subjectKeyId]; ok {
-			if crl.HasExpired(now) {
-				log.Info("certificate revocation list has expired", "subject key identifier", subjectKeyId)
-			} else {
-				subjectKeyIds = append(subjectKeyIds, subjectKeyId)
-				if (nextCRLUpdate.IsZero() || crl.TBSCertList.NextUpdate.Before(nextCRLUpdate)) && crl.TBSCertList.NextUpdate.After(now) {
-					nextCRLUpdate = crl.TBSCertList.NextUpdate
-				}
-				continue
-			}
-		}
-		log.Info("retrieving certificate revocation list", "subject key identifier", subjectKeyId)
-		if crl, err := getCRL(cert.CRLDistributionPoints); err != nil {
-			// Creating or updating the configmap with incomplete
-			// data would compromise security by potentially
-			// permitting revoked certificates.
-			return false, nil, ctx, fmt.Errorf("failed to get certificate revocation list for certificate key %s: %w", subjectKeyId, err)
-		} else {
-			crls[subjectKeyId] = crl
-			subjectKeyIds = append(subjectKeyIds, subjectKeyId)
-			log.Info("new certificate revocation list", "subject key identifier", subjectKeyId, "next update", crl.TBSCertList.NextUpdate.String())
-			if (nextCRLUpdate.IsZero() || crl.TBSCertList.NextUpdate.Before(nextCRLUpdate)) && crl.TBSCertList.NextUpdate.After(now) {
-				nextCRLUpdate = crl.TBSCertList.NextUpdate
-			}
-		}
-	}
-
-	if len(subjectKeyIds) == 0 {
-		return false, nil, ctx, nil
-	}
-
-	buf := &bytes.Buffer{}
-	for _, subjectKeyId := range subjectKeyIds {
-		asn1Data, err := asn1.Marshal(*crls[subjectKeyId])
-		if err != nil {
-			return false, nil, ctx, fmt.Errorf("failed to encode ASN.1 for CRL for certificate key %s: %w", subjectKeyId, err)
-		}
-		block := &pem.Block{
-			Type:  "X509 CRL",
-			Bytes: asn1Data,
-		}
-		if err := pem.Encode(buf, block); err != nil {
-			return false, nil, ctx, fmt.Errorf("failed to encode PEM for CRL for certificate key %s: %w", subjectKeyId, err)
-		}
-	}
-	crlData := buf.String()
-
-	crlConfigmapName := controller.CRLConfigMapName(ic)
-	crlConfigmap := corev1.ConfigMap{
-		ObjectMeta: metav1.ObjectMeta{
-			Name:      crlConfigmapName.Name,
-			Namespace: crlConfigmapName.Namespace,
-		},
-		Data: map[string]string{
-			"crl.pem": crlData,
-		},
-	}
-	crlConfigmap.SetOwnerReferences([]metav1.OwnerReference{ownerRef})
-
-	return true, &crlConfigmap, context.WithValue(ctx, "nextCRLUpdate", nextCRLUpdate), nil
-}
-
-// getCRL gets a certificate revocation list using the provided distribution
-// points and returns the certificate list.
-func getCRL(distributionPoints []string) (*pkix.CertificateList, error) {
-	var errs []error
-	for _, distributionPoint := range distributionPoints {
-		// The distribution point is typically a URL with the "http"
-		// scheme.  "https" is generally not used because the
-		// certificate list is signed, and because using TLS to get the
-		// certificate list could introduce a circular dependency
-		// (cannot use TLS without the revocation list, and cannot get
-		// the revocation list without using TLS).
-		//
-		// TODO Support ldap.
-		switch {
-		case strings.HasPrefix(distributionPoint, "http:"):
-			log.Info("retrieving CRL distribution point", "distribution point", distributionPoint)
-			crl, err := getHTTPCRL(distributionPoint)
-			if err != nil {
-				errs = append(errs, fmt.Errorf("error getting %q: %w", distributionPoint, err))
-				continue
-			}
-			return crl, nil
-		default:
-			errs = append(errs, fmt.Errorf("unsupported distribution point type: %s", distributionPoint))
-		}
-	}
-	return nil, kerrors.NewAggregate(errs)
-}
-
-// getHTTPCRL gets a certificate revocation list using the provided HTTP URL.
-func getHTTPCRL(url string) (*pkix.CertificateList, error) {
-	resp, err := http.Get(url)
-	if err != nil {
-		return nil, fmt.Errorf("http.Get failed: %w", err)
-	}
-	defer resp.Body.Close()
-	bytes, err := ioutil.ReadAll(resp.Body)
-	if err != nil {
-		return nil, fmt.Errorf("error reading response: %w", err)
-	}
-	crl, err := x509.ParseCRL(bytes)
-	if err != nil {
-		return nil, fmt.Errorf("error parsing response: %w", err)
 	}
-	return crl, nil
+	return false, nil, ctx, nil
 }
 
 // currentCRLConfigMap returns the current CRL configmap.  Returns a Boolean
@@ -283,30 +56,3 @@ func (r *reconciler) currentCRLConfigMap(ctx context.Context, ic *operatorv1.Ing
 	}
 	return true, cm, nil
 }
-
-// updateCRLConfigMap updates a configmap.  Returns a Boolean indicating whether
-// the configmap was updated, and an error value.
-func (r *reconciler) updateCRLConfigMap(ctx context.Context, current, desired *corev1.ConfigMap) (bool, error) {
-	if crlConfigmapsEqual(current, desired) {
-		return false, nil
-	}
-	updated := current.DeepCopy()
-	updated.Data = desired.Data
-	if err := r.client.Update(ctx, updated); err != nil {
-		if errors.IsAlreadyExists(err) {
-			return false, nil
-		}
-		return false, err
-	}
-	return true, nil
-}
-
-// crlConfigmapsEqual compares two CRL configmaps.  Returns true if the
-// configmaps should be considered equal for the purpose of determining whether
-// an update is necessary, false otherwise
-func crlConfigmapsEqual(a, b *corev1.ConfigMap) bool {
-	if !reflect.DeepEqual(a.Data, b.Data) {
-		return false
-	}
-	return true
-}