OSD-24275: Validate machineCIDR is contained in default ingresscontroller allowedSourceRanges

nephomaniac · nephomaniac · commit 82be7bfcdd47 · 2024-08-13T10:53:34.000-07:00
diff --git a/build/resources.go b/build/resources.go
@@ -32,6 +32,9 @@ const (
 	roleName           string = "validation-webhook"
 	prometheusRoleName string = "prometheus-k8s"
 	repoName           string = "managed-cluster-validating-webhooks"
+	// Role and Binding for reading cluster-config-v1 config map...
+	clusterConfigRole        string = "config-v1-reader-wh"
+	clusterConfigRoleBinding string = "validation-webhook-cluster-config-v1-reader"
 	// Used to define what phase a resource should be deployed in by package-operator
 	pkoPhaseAnnotation string = "package-operator.run/phase"
 	// Defines the 'rbac' package-operator phase for any resources related to RBAC
@@ -211,6 +214,60 @@ func createClusterRoleBinding() *rbacv1.ClusterRoleBinding {
 	}
 }
 
+func createClusterConfigRole() *rbacv1.Role {
+	return &rbacv1.Role{
+		TypeMeta: metav1.TypeMeta{
+			Kind:       "Role",
+			APIVersion: rbacv1.SchemeGroupVersion.String(),
+		},
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      clusterConfigRole,
+			Namespace: "kube-system",
+		},
+		Rules: []rbacv1.PolicyRule{
+			{
+				APIGroups: []string{
+					"",
+				},
+				Resources: []string{
+					"configmaps",
+				},
+				Verbs: []string{
+					"get",
+				},
+				ResourceNames: []string{
+					"cluster-config-v1",
+				},
+			},
+		},
+	}
+}
+
+func createClusterConfigRoleBinding() *rbacv1.RoleBinding {
+	return &rbacv1.RoleBinding{
+		TypeMeta: metav1.TypeMeta{
+			Kind:       "RoleBinding",
+			APIVersion: rbacv1.SchemeGroupVersion.String(),
+		},
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      clusterConfigRoleBinding,
+			Namespace: "kube-system",
+		},
+		Subjects: []rbacv1.Subject{
+			{
+				Kind:      "ServiceAccount",
+				Name:      serviceAccountName,
+				Namespace: *namespace,
+			},
+		},
+		RoleRef: rbacv1.RoleRef{
+			Name:     clusterConfigRole,
+			Kind:     "Role",
+			APIGroup: rbacv1.GroupName,
+		},
+	}
+}
+
 func createPrometheusRole() *rbacv1.Role {
 	return &rbacv1.Role{
 		TypeMeta: metav1.TypeMeta{
@@ -851,6 +908,8 @@ func main() {
 		templateResources.Add(utils.DefaultLabelSelector(), runtime.RawExtension{Object: createClusterRoleBinding()})
 		templateResources.Add(utils.DefaultLabelSelector(), runtime.RawExtension{Object: createPrometheusRole()})
 		templateResources.Add(utils.DefaultLabelSelector(), runtime.RawExtension{Object: createPromethusRoleBinding()})
+		templateResources.Add(utils.DefaultLabelSelector(), runtime.RawExtension{Object: createClusterConfigRole()})
+		templateResources.Add(utils.DefaultLabelSelector(), runtime.RawExtension{Object: createClusterConfigRoleBinding()})
 		templateResources.Add(utils.DefaultLabelSelector(), runtime.RawExtension{Object: createServiceMonitor()})
 		templateResources.Add(utils.DefaultLabelSelector(), runtime.RawExtension{Object: createCACertConfigMap()})
 		templateResources.Add(utils.DefaultLabelSelector(), runtime.RawExtension{Object: createService()})
diff --git a/build/selectorsyncset.yaml b/build/selectorsyncset.yaml
@@ -123,6 +123,35 @@ objects:
       - kind: ServiceAccount
         name: prometheus-k8s
         namespace: openshift-monitoring
+    - apiVersion: rbac.authorization.k8s.io/v1
+      kind: Role
+      metadata:
+        creationTimestamp: null
+        name: config-v1-reader-wh
+        namespace: kube-system
+      rules:
+      - apiGroups:
+        - ""
+        resourceNames:
+        - cluster-config-v1
+        resources:
+        - configmaps
+        verbs:
+        - get
+    - apiVersion: rbac.authorization.k8s.io/v1
+      kind: RoleBinding
+      metadata:
+        creationTimestamp: null
+        name: validation-webhook-cluster-config-v1-reader
+        namespace: kube-system
+      roleRef:
+        apiGroup: rbac.authorization.k8s.io
+        kind: Role
+        name: config-v1-reader-wh
+      subjects:
+      - kind: ServiceAccount
+        name: validation-webhook
+        namespace: openshift-validation-webhook
     - apiVersion: monitoring.coreos.com/v1
       kind: ServiceMonitor
       metadata:
diff --git a/pkg/webhooks/ingresscontroller/ingresscontroller.go b/pkg/webhooks/ingresscontroller/ingresscontroller.go
@@ -1,25 +1,37 @@
 package ingresscontroller
 
 import (
+	"bytes"
+	"context"
 	"fmt"
+	"net"
 	"net/http"
+	"os"
 	"slices"
 	"strings"
 
 	operatorv1 "github.com/openshift/api/operator/v1"
+	"github.com/openshift/managed-cluster-validating-webhooks/pkg/k8sutil"
 	"github.com/openshift/managed-cluster-validating-webhooks/pkg/webhooks/utils"
+	"github.com/pkg/errors"
+	admissionv1 "k8s.io/api/admission/v1"
 	admissionregv1 "k8s.io/api/admissionregistration/v1"
+	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
-	admissionctl "sigs.k8s.io/controller-runtime/pkg/webhook/admission"
-
+	"k8s.io/apimachinery/pkg/util/yaml"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 	logf "sigs.k8s.io/controller-runtime/pkg/log"
+	admissionctl "sigs.k8s.io/controller-runtime/pkg/webhook/admission"
 )
 
 const (
 	WebhookName                     string = "ingresscontroller-validation"
 	docString                       string = `Managed OpenShift Customer may create IngressControllers without necessary taints. This can cause those workloads to be provisioned on master nodes.`
 	legacyIngressSupportFeatureFlag        = "ext-managed.openshift.io/legacy-ingress-support"
+	installConfigMap                       = "cluster-config-v1"
+	installConfigNamespace                 = "kube-system"
+	installConfigKeyName                   = "install-config"
 )
 
 var (
@@ -42,7 +54,22 @@ var (
 )
 
 type IngressControllerWebhook struct {
-	s runtime.Scheme
+	s          runtime.Scheme
+	kubeClient client.Client
+	// Allow caching install config and machineCIDR values...
+	machineCIDRIP  net.IP
+	machineCIDRNet *net.IPNet
+}
+
+// installConfig struct to load the min values needed from the install-config data.
+// Alternative would be to vendor all of github.com/openshift/installer/pkg/types to import the proper struct.
+// Required values: machineCidr info, anything else we gather from this? hcp vs classic, privatelink info, etc?
+type installConfig struct {
+	metav1.TypeMeta   `json:",inline"`
+	metav1.ObjectMeta `json:"metadata"`
+	Networking        struct {
+		MachineCIDR string `json:"machineCIDR"`
+	} `json:"networking"`
 }
 
 // ObjectSelector implements Webhook interface
@@ -149,12 +176,151 @@ func (wh *IngressControllerWebhook) authorized(request admissionctl.Request) adm
 		}
 	}
 
-	ret = admissionctl.Allowed("IngressController operation is allowed")
+	/* TODO:
+	 * 1) ONLY check for privatelink clusters? How?
+	 * 2) HCP vs Classic, etc.. ...any other cluster type this should apply to? Is this handled by the
+	 * HypershiftEnabled vs ClassicEnabled flags set in this module? Currently HCP disabled.
+	 * If HCP is to be enabled for allowed source ranges, should this part of a 2nd ingress validator to
+	 * allow separation of validations between cluster install types? Is there a run time method available
+	 * to this validator to determine classic vs hcp?
+	 * 3) What other specifics should be checked here for this cidr check to be applicable?m
+	 */
+	// Only check for machine cidr in allowed ranges if creating or updating resource...
+	reqOp := request.AdmissionRequest.Operation
+	if reqOp == admissionv1.Create || reqOp == admissionv1.Update {
+		//TODO: Will these need to iterate over more than just the default IngressController config?
+		if ic.ObjectMeta.Name == "default" && ic.ObjectMeta.Namespace == "openshift-ingress-operator" {
+			ret := wh.checkAllowsMachineCIDR(ic.Spec.EndpointPublishingStrategy.LoadBalancer.AllowedSourceRanges)
+			ret.UID = request.AdmissionRequest.UID
+			if !ret.Allowed {
+				log.Info("Error checking minimum AllowedSourceRange", "err", ret.AdmissionResponse.String())
+			}
+			return ret
+		}
+	}
+	log.Info("############# DEBUG LOG: IngressController operation is allowed ###########")
+	ret = admissionctl.Allowed("IngressController operation is allowed, machineCIDR n/a")
 	ret.UID = request.AdmissionRequest.UID
 
 	return ret
 }
 
+func (wh *IngressControllerWebhook) getMachineCIDR() (net.IP, *net.IPNet, error) {
+	if wh.machineCIDRIP == nil || wh.machineCIDRNet == nil {
+		instConf, err := wh.getClusterConfig()
+		if err != nil {
+			log.Error(err, "Failed to fetch machineCIDR", "namespace", installConfigNamespace, "configmap", installConfigMap)
+			return nil, nil, err
+		}
+		if instConf == nil {
+			err := fmt.Errorf("can not fetch machineCIDR from empty '%s' install config", installConfigMap)
+			log.Error(err, "getMachineCIDR failed to find CIDR value")
+			return nil, nil, err
+		}
+		if len(instConf.Networking.MachineCIDR) <= 0 {
+			err := fmt.Errorf("empty machineCIDR string value parsed from '%s' install config", installConfigMap)
+			log.Error(err, "getMachineCIDR found empty CIDR value")
+			return nil, nil, err
+		}
+		machIP, machNet, err := net.ParseCIDR(string(instConf.Networking.MachineCIDR))
+		if err != nil {
+			log.Error(err, "err parsing machineCIDR into network cidr", "machineCIDR", string(instConf.Networking.MachineCIDR))
+			return nil, nil, err
+		}
+		if machIP == nil || machNet == nil {
+			err := fmt.Errorf("failed to parse machineCIDR string:'%s' into network structures", string(instConf.Networking.MachineCIDR))
+			log.Error(err, "failed to parse install-config machineCIDR")
+			return nil, nil, err
+		}
+		// Successfully fetched, parsed, and converted the machineCIDR string into net structures...
+		wh.machineCIDRIP = machIP
+		wh.machineCIDRNet = machNet
+	}
+	return wh.machineCIDRIP, wh.machineCIDRNet, nil
+}
+
+/* Fetch the install-config from the kube-system config map's data.
+ * this requires proper role, rolebinding for this service account's get() request
+ * to succeed. (see toplevel selectorsyncset).  This config should not change during runtime so
+ * this operation should cache this if possible.
+ * TODO: Should it retry fetching the config if there are any failures/errors encountered while
+ * parsing out the the desired values?
+ */
+func (wh *IngressControllerWebhook) getClusterConfig() (*installConfig, error) {
+	var err error
+	if wh.kubeClient == nil {
+		wh.kubeClient, err = k8sutil.KubeClient(&wh.s)
+		if err != nil {
+			log.Error(err, "Fail creating KubeClient for IngressControllerWebhook")
+			return nil, err
+		}
+	}
+	clusterConfig := &corev1.ConfigMap{}
+	err = wh.kubeClient.Get(context.Background(), client.ObjectKey{Name: installConfigMap, Namespace: installConfigNamespace}, clusterConfig)
+	if err != nil {
+		log.Error(err, "Failed to fetch configmap: 'cluster-config-v1' for cluster config")
+		return nil, err
+	}
+	data, ok := clusterConfig.Data[installConfigKeyName]
+	if !ok {
+		return nil, fmt.Errorf("did not find key %s in configmap %s/%s", installConfigKeyName, installConfigNamespace, installConfigMap)
+	}
+	instConf := &installConfig{}
+
+	decoder := yaml.NewYAMLOrJSONDecoder(bytes.NewReader([]byte(data)), 4096)
+	if err := decoder.Decode(instConf); err != nil {
+		return nil, errors.Wrap(err, "failed to decode install config")
+	}
+	return instConf, nil
+}
+
+func (wh *IngressControllerWebhook) checkAllowsMachineCIDR(ipRanges []operatorv1.CIDR) admissionctl.Response {
+	// https://docs.openshift.com/container-platform/4.13/networking/configuring_ingress_cluster_traffic/configuring-ingress-cluster-traffic-load-balancer-allowed-source-ranges.html
+	// Note: From docs it appears a missing ASR value/attr allows all. However...
+	// once ASR values have been added to an ingresscontroller, later deleting all the ASRs can expose an issue
+	// where the IGC will remaining in progressing state indefinitely.
+	// For now return Allowed with a warning?
+	if ipRanges == nil || len(ipRanges) <= 0 {
+		return admissionctl.Allowed("Allowing empty 'AllowedSourceRanges'. Populate this value if operator remains in 'progressing' state")
+	}
+	machIP, machNet, err := wh.getMachineCIDR()
+	if err != nil {
+		// This represents a fault in either the webhook itself, webhook permissions, or install config.
+		// Might be nice to have an env var etc we can set to allow proceeding w/o the immediate need to roll new code?
+		return admissionctl.Errored(http.StatusInternalServerError, err)
+	}
+	machNetSize, machNetBits := machNet.Mask.Size()
+	log.Info("Checking AllowedSourceRanges", "MachineCIDR", fmt.Sprintf("%s/%d", machIP.String(), machNetSize), "NetBits", machNetBits, "AllowedSourceRanges", ipRanges)
+	for _, OpV1CIDR := range ipRanges {
+		// Clean up the operatorV1.CIDR value into trimmed CIDR 'a.b.c.d/x' string
+		ASRstring := strings.TrimSpace(string(OpV1CIDR))
+		log.Info(fmt.Sprintf("Checking allowed source:'%s'", ASRstring))
+		if len(ASRstring) <= 0 {
+			continue
+		}
+		// Parse the Allowed Source Range Cidr entry into network structures...
+		_, ASRNet, err := net.ParseCIDR(ASRstring)
+		if err != nil {
+			log.Info(fmt.Sprintf("failed to parse AllowedSourceRanges value: '%s'. Err: %s", string(ASRstring), err))
+			return admissionctl.Errored(http.StatusBadRequest, fmt.Errorf("failed to parse AllowedSourceRanges value: '%s'. Err: %s", string(ASRstring), err))
+		}
+		// First check if this AlloweSourceRange entry network contains the machine cidr ip...
+		if !ASRNet.Contains(machIP) {
+			log.Info(fmt.Sprintf("AllowedSourceRange:'%s' does not contain machine CIDR:'%s/%d'", ASRstring, machIP.String(), machNetSize))
+			continue
+		}
+		// Check if this AlloweSourceRange entry mask includes the network.
+		ASRNetSize, ASRNetBits := ASRNet.Mask.Size()
+		if machNetBits == ASRNetBits && ASRNetSize <= machNetSize {
+			log.Info(fmt.Sprintf("Found machineCidr:'%s/%d' within AllowedSourceRange:'%s'", machIP.String(), machNetSize, ASRstring))
+			return admissionctl.Allowed(fmt.Sprintf("Found machineCidr:'%s/%d' within AllowedSourceRange:'%s'", machIP.String(), machNetSize, ASRstring))
+			//return admissionctl.Allowed("IngressController operation is allowed. Minimum AllowedSourceRanges are met.")
+		}
+	}
+	log.Info(fmt.Sprintf("machineCidr:'%s/%d' not found within networks provided by AllowedSourceRanges:'%v'", machIP.String(), machNetSize, ipRanges))
+	return admissionctl.Denied(fmt.Sprintf("At least one AllowedSourceRange must allow machine cidr:'%s/%d'", machIP.String(), machNetSize))
+}
+
 // isAllowedUser checks if the user is allowed to perform the action
 func isAllowedUser(request admissionctl.Request) bool {
 	log.Info(fmt.Sprintf("Checking username %s on whitelist", request.UserInfo.Username))
@@ -199,7 +365,13 @@ func (s *IngressControllerWebhook) HypershiftEnabled() bool { return false }
 // NewWebhook creates a new webhook
 func NewWebhook() *IngressControllerWebhook {
 	scheme := runtime.NewScheme()
+	err := corev1.AddToScheme(scheme)
+	if err != nil {
+		log.Error(err, "Fail adding corev1 scheme to IngressControllerWebhook")
+		os.Exit(1)
+	}
 	return &IngressControllerWebhook{
-		s: *scheme,
+		s:          *scheme,
+		kubeClient: nil,
 	}
 }
diff --git a/pkg/webhooks/ingresscontroller/ingresscontroller_test.go b/pkg/webhooks/ingresscontroller/ingresscontroller_test.go
