Merge pull request #363 from JoelSpeed/prevent-node-daemon-escalation

openshift-merge-bot[bot] · web-flow · commit b601c17fea26 · 2024-08-28T14:12:37.000Z
OCPBUGS-22190: Add Node Admission restriction for Azure node Manager
diff --git a/cmd/cluster-cloud-controller-manager-operator/main.go b/cmd/cluster-cloud-controller-manager-operator/main.go
@@ -35,7 +35,6 @@ import (
 	"k8s.io/component-base/config"
 	"k8s.io/component-base/config/options"
 	"k8s.io/klog/v2"
-	"k8s.io/klog/v2/textlogger"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/cache"
 	"sigs.k8s.io/controller-runtime/pkg/healthz"
@@ -77,8 +76,7 @@ func init() {
 }
 
 func main() {
-	textLoggerCfg := textlogger.NewConfig()
-	textLoggerCfg.AddFlags(flag.CommandLine)
+	klog.InitFlags(flag.CommandLine)
 
 	metricsAddr := flag.String(
 		"metrics-bind-address",
@@ -109,7 +107,7 @@ func main() {
 	options.BindLeaderElectionFlags(&leaderElectionConfig, pflag.CommandLine)
 	pflag.Parse()
 
-	ctrl.SetLogger(textlogger.NewLogger(textLoggerCfg).WithName("CCMOperator"))
+	ctrl.SetLogger(klog.NewKlogr().WithName("CCMOperator"))
 
 	restConfig := ctrl.GetConfigOrDie()
 	le := util.GetLeaderElectionDefaults(restConfig, configv1.LeaderElection{
diff --git a/manifests/0000_26_cloud-controller-manager-operator_02_rbac_operator.yaml b/manifests/0000_26_cloud-controller-manager-operator_02_rbac_operator.yaml
@@ -75,6 +75,19 @@ rules:
   - update
   - patch
 
+- apiGroups:
+  - admissionregistration.k8s.io
+  resources:
+  - validatingadmissionpolicies
+  - validatingadmissionpolicybindings
+  verbs:
+  - get
+  - list
+  - watch
+  - create
+  - update
+  - patch
+
 # vSphere has a separate node manager that uses the service account kube-system/vsphere-cloud-controller-manager.
 # The operator must have these permissions to then grant them to the vSphere node manager.
 - apiGroups:
diff --git a/pkg/cloud/azure/assets/validating-admission-policy-binding.yaml b/pkg/cloud/azure/assets/validating-admission-policy-binding.yaml
@@ -0,0 +1,7 @@
+apiVersion: admissionregistration.k8s.io/v1
+kind: ValidatingAdmissionPolicyBinding
+metadata:
+  name: openshift-cloud-controller-manager-cloud-provider-azure-node-admission
+spec:
+  policyName: openshift-cloud-controller-manager-cloud-provider-azure-node-admission
+  validationActions: ["Deny"]
diff --git a/pkg/cloud/azure/assets/validating-admission-policy.yaml b/pkg/cloud/azure/assets/validating-admission-policy.yaml
@@ -0,0 +1,23 @@
+apiVersion: admissionregistration.k8s.io/v1beta1
+kind: ValidatingAdmissionPolicy
+metadata:
+  name: openshift-cloud-controller-manager-cloud-provider-azure-node-admission
+spec:
+  failurePolicy: Fail
+  matchConstraints:
+    resourceRules:
+    - apiGroups:   [""]
+      apiVersions: ["v1"]
+      operations:  ["UPDATE"]
+      resources:   ["nodes"]
+  validations:
+    # all requests should have a node-name claim, this prevents impersonation of the SA.
+    - expression: "has(request.userInfo.extra) && ('authentication.kubernetes.io/node-name' in request.userInfo.extra)"
+      message: "this user must have a \"authentication.kubernetes.io/node-name\" claim"
+    # all requests should originate from the MCN owner's node
+    - expression: "object.metadata.name == request.userInfo.extra[\"authentication.kubernetes.io/node-name\"][0]"
+      messageExpression: "'updates to Node ' + string(object.metadata.name) + ' may only be effected from the cloud node manager running on the same node'"      
+  matchConditions:
+    # Only check requests from Azure Cloud Node Manager SA, this allows all other SAs with the correct RBAC to modify Nodes.
+    - name: "check-only-machine-config-daemon-requests"
+      expression: "request.userInfo.username == 'system:serviceaccount:openshift-cloud-controller-manager:cloud-node-manager'"
diff --git a/pkg/cloud/azure/azure.go b/pkg/cloud/azure/azure.go
@@ -9,6 +9,7 @@ import (
 
 	"github.com/asaskevich/govalidator"
 	configv1 "github.com/openshift/api/config/v1"
+	admissionregistrationv1 "k8s.io/api/admissionregistration/v1"
 	appsv1 "k8s.io/api/apps/v1"
 	rbacv1 "k8s.io/api/rbac/v1"
 	"k8s.io/apimachinery/pkg/util/validation/field"
@@ -31,6 +32,8 @@ var (
 		{ReferenceObject: &appsv1.DaemonSet{}, EmbedFsPath: "assets/cloud-node-manager-daemonset.yaml"},
 		{ReferenceObject: &rbacv1.ClusterRole{}, EmbedFsPath: "assets/azure-cloud-controller-manager-clusterrole.yaml"},
 		{ReferenceObject: &rbacv1.ClusterRoleBinding{}, EmbedFsPath: "assets/azure-cloud-controller-manager-clusterrolebinding.yaml"},
+		{ReferenceObject: &admissionregistrationv1.ValidatingAdmissionPolicy{}, EmbedFsPath: "assets/validating-admission-policy.yaml"},
+		{ReferenceObject: &admissionregistrationv1.ValidatingAdmissionPolicyBinding{}, EmbedFsPath: "assets/validating-admission-policy-binding.yaml"},
 	}
 )
 
diff --git a/pkg/cloud/azure/azure_test.go b/pkg/cloud/azure/azure_test.go
@@ -90,7 +90,7 @@ func TestResourcesRenderingSmoke(t *testing.T) {
 			}
 
 			resources := assets.GetRenderedResources()
-			assert.Len(t, resources, 4)
+			assert.Len(t, resources, 6)
 		})
 	}
 }
diff --git a/pkg/cloud/cloud_test.go b/pkg/cloud/cloud_test.go
@@ -149,24 +149,28 @@ func TestGetResources(t *testing.T) {
 	}, {
 		name:                  "Azure resources returned as expected",
 		testPlatform:          platformsMap[string(configv1.AzurePlatformType)],
-		expectedResourceCount: 5,
+		expectedResourceCount: 7,
 		expectedResourcesKindName: []string{
 			"Deployment/azure-cloud-controller-manager",
 			"DaemonSet/azure-cloud-node-manager",
 			"ClusterRole/azure-cloud-controller-manager",
 			"ClusterRoleBinding/cloud-controller-manager:azure-cloud-controller-manager",
+			"ValidatingAdmissionPolicy/openshift-cloud-controller-manager-cloud-provider-azure-node-admission",
+			"ValidatingAdmissionPolicyBinding/openshift-cloud-controller-manager-cloud-provider-azure-node-admission",
 			"PodDisruptionBudget/azure-cloud-controller-manager",
 		},
 	}, {
 		name:                  "Azure resources returned as expected with single node cluster",
 		testPlatform:          platformsMap[string(configv1.AzurePlatformType)],
-		expectedResourceCount: 4,
+		expectedResourceCount: 6,
 		singleReplica:         true,
 		expectedResourcesKindName: []string{
 			"Deployment/azure-cloud-controller-manager",
 			"DaemonSet/azure-cloud-node-manager",
 			"ClusterRole/azure-cloud-controller-manager",
 			"ClusterRoleBinding/cloud-controller-manager:azure-cloud-controller-manager",
+			"ValidatingAdmissionPolicy/openshift-cloud-controller-manager-cloud-provider-azure-node-admission",
+			"ValidatingAdmissionPolicyBinding/openshift-cloud-controller-manager-cloud-provider-azure-node-admission",
 		},
 	}, {
 		name:                  "Azure Stack resources returned as expected",
diff --git a/pkg/controllers/resourceapply/resourceapply.go b/pkg/controllers/resourceapply/resourceapply.go
@@ -8,6 +8,7 @@ import (
 	"fmt"
 	"reflect"
 
+	admissionregistrationv1 "k8s.io/api/admissionregistration/v1"
 	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
 	policyv1 "k8s.io/api/policy/v1"
@@ -22,6 +23,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	coreclientv1 "sigs.k8s.io/controller-runtime/pkg/client"
 
+	"github.com/openshift/library-go/pkg/operator/resource/resourceapply"
 	"github.com/openshift/library-go/pkg/operator/resource/resourcemerge"
 )
 
@@ -83,6 +85,10 @@ func ApplyResource(ctx context.Context, client coreclientv1.Client, recorder rec
 		return applyRoleBinding(ctx, client, recorder, t)
 	case *rbacv1.ClusterRoleBinding:
 		return applyClusterRoleBinding(ctx, client, recorder, t)
+	case *admissionregistrationv1.ValidatingAdmissionPolicy:
+		return applyValidatingAdmissionPolicy(ctx, client, recorder, t)
+	case *admissionregistrationv1.ValidatingAdmissionPolicyBinding:
+		return applyValidatingAdmissionPolicyBinding(ctx, client, recorder, t)
 	default:
 		return false, fmt.Errorf("unhandled type %T", resource)
 	}
@@ -560,3 +566,87 @@ func applyClusterRoleBinding(ctx context.Context, client coreclientv1.Client, re
 	recorder.Event(required, corev1.EventTypeNormal, ResourceUpdateSuccessEvent, "Resource was successfully updated")
 	return true, nil
 }
+
+func applyValidatingAdmissionPolicy(ctx context.Context, client coreclientv1.Client, recorder record.EventRecorder,
+	requiredOriginal *admissionregistrationv1.ValidatingAdmissionPolicy) (bool, error) {
+	required := requiredOriginal.DeepCopy()
+
+	existing := &admissionregistrationv1.ValidatingAdmissionPolicy{}
+	err := client.Get(ctx, coreclientv1.ObjectKeyFromObject(requiredOriginal), existing)
+	if apierrors.IsNotFound(err) {
+		required := requiredOriginal.DeepCopy()
+		if err := client.Create(ctx, required); err != nil {
+			recorder.Event(required, corev1.EventTypeWarning, ResourceCreateFailedEvent, err.Error())
+			return false, fmt.Errorf("validatingadmissionpolicy creation failed: %v", err)
+		}
+		recorder.Event(required, corev1.EventTypeNormal, ResourceCreateSuccessEvent, "Resource was successfully created")
+		return true, nil
+	} else if err != nil {
+		recorder.Event(required, corev1.EventTypeWarning, ResourceUpdateFailedEvent, err.Error())
+		return false, fmt.Errorf("failed to get validatingadmissionpolicy for update: %v", err)
+	}
+
+	modified := false
+	existingCopy := existing.DeepCopy()
+
+	resourcemerge.EnsureObjectMeta(&modified, &existingCopy.ObjectMeta, required.ObjectMeta)
+	specEquivalent := equality.Semantic.DeepDerivative(required.Spec, existingCopy.Spec)
+	if specEquivalent && !modified {
+		return false, nil
+	}
+	// at this point we know that we're going to perform a write.  We're just trying to get the object correct
+	toWrite := existingCopy // shallow copy so the code reads easier
+	toWrite.Spec = required.Spec
+
+	klog.V(2).Infof("ValidatingAdmissionPolicyConfiguration %q changes: %v", required.GetNamespace()+"/"+required.GetName(), resourceapply.JSONPatchNoError(existing, toWrite))
+
+	if err := client.Update(ctx, existingCopy); err != nil {
+		recorder.Event(required, corev1.EventTypeWarning, ResourceUpdateFailedEvent, err.Error())
+		return false, err
+	}
+	recorder.Event(required, corev1.EventTypeNormal, ResourceUpdateSuccessEvent, "Resource was successfully updated")
+
+	return true, nil
+}
+
+func applyValidatingAdmissionPolicyBinding(ctx context.Context, client coreclientv1.Client, recorder record.EventRecorder,
+	requiredOriginal *admissionregistrationv1.ValidatingAdmissionPolicyBinding) (bool, error) {
+	required := requiredOriginal.DeepCopy()
+
+	existing := &admissionregistrationv1.ValidatingAdmissionPolicyBinding{}
+	err := client.Get(ctx, coreclientv1.ObjectKeyFromObject(requiredOriginal), existing)
+	if apierrors.IsNotFound(err) {
+		required := requiredOriginal.DeepCopy()
+		if err := client.Create(ctx, required); err != nil {
+			recorder.Event(required, corev1.EventTypeWarning, ResourceCreateFailedEvent, err.Error())
+			return false, fmt.Errorf("validatingadmissionpolicybinding creation failed: %v", err)
+		}
+		recorder.Event(required, corev1.EventTypeNormal, ResourceCreateSuccessEvent, "Resource was successfully created")
+		return true, nil
+	} else if err != nil {
+		recorder.Event(required, corev1.EventTypeWarning, ResourceUpdateFailedEvent, err.Error())
+		return false, fmt.Errorf("failed to get validatingadmissionpolicybinding for update: %v", err)
+	}
+
+	modified := false
+	existingCopy := existing.DeepCopy()
+
+	resourcemerge.EnsureObjectMeta(&modified, &existingCopy.ObjectMeta, required.ObjectMeta)
+	specEquivalent := equality.Semantic.DeepDerivative(required.Spec, existingCopy.Spec)
+	if specEquivalent && !modified {
+		return false, nil
+	}
+	// at this point we know that we're going to perform a write.  We're just trying to get the object correct
+	toWrite := existingCopy // shallow copy so the code reads easier
+	toWrite.Spec = required.Spec
+
+	klog.V(2).Infof("ValidatingAdmissionPolicyBindingConfiguration %q changes: %v", required.GetNamespace()+"/"+required.GetName(), resourceapply.JSONPatchNoError(existing, toWrite))
+
+	if err := client.Update(ctx, existingCopy); err != nil {
+		recorder.Event(required, corev1.EventTypeWarning, ResourceUpdateFailedEvent, err.Error())
+		return false, err
+	}
+	recorder.Event(required, corev1.EventTypeNormal, ResourceUpdateSuccessEvent, "Resource was successfully updated")
+
+	return true, nil
+}

Original file line number	Diff line number	Diff line change
`@@ -9,6 +9,7 @@ import (`
`9`	`9`
`10`	`10`	`"github.com/asaskevich/govalidator"`
`11`	`11`	`configv1 "github.com/openshift/api/config/v1"`
	`12`	`+ admissionregistrationv1 "k8s.io/api/admissionregistration/v1"`
`12`	`13`	`appsv1 "k8s.io/api/apps/v1"`
`13`	`14`	`rbacv1 "k8s.io/api/rbac/v1"`
`14`	`15`	`"k8s.io/apimachinery/pkg/util/validation/field"`
`@@ -31,6 +32,8 @@ var (`
`31`	`32`	`{ReferenceObject: &appsv1.DaemonSet{}, EmbedFsPath: "assets/cloud-node-manager-daemonset.yaml"},`
`32`	`33`	`{ReferenceObject: &rbacv1.ClusterRole{}, EmbedFsPath: "assets/azure-cloud-controller-manager-clusterrole.yaml"},`
`33`	`34`	`{ReferenceObject: &rbacv1.ClusterRoleBinding{}, EmbedFsPath: "assets/azure-cloud-controller-manager-clusterrolebinding.yaml"},`
	`35`	`+ {ReferenceObject: &admissionregistrationv1.ValidatingAdmissionPolicy{}, EmbedFsPath: "assets/validating-admission-policy.yaml"},`
	`36`	`+ {ReferenceObject: &admissionregistrationv1.ValidatingAdmissionPolicyBinding{}, EmbedFsPath: "assets/validating-admission-policy-binding.yaml"},`
`34`	`37`	`}`
`35`	`38`	`)`
`36`	`39`
Original file line number	Diff line number	Diff line change
`@@ -90,7 +90,7 @@ func TestResourcesRenderingSmoke(t *testing.T) {`
`90`	`90`	`}`
`91`	`91`
`92`	`92`	`resources := assets.GetRenderedResources()`
`93`		`- assert.Len(t, resources, 4)`
	`93`	`+ assert.Len(t, resources, 6)`
`94`	`94`	`})`
`95`	`95`	`}`
`96`	`96`	`}`