Merge pull request #71396 from liggitt/forbidden-messages

Improve node authorizer and noderestriction forbidden messages

plugin/pkg/admission/noderestriction/admission.go

@@ -124,7 +124,7 @@ func (c *nodePlugin) Admit(a admission.Attributes) error {
 		case "eviction":
 			return c.admitPodEviction(nodeName, a)
 		default:
-			return admission.NewForbidden(a, fmt.Errorf("unexpected pod subresource %q", a.GetSubresource()))
+			return admission.NewForbidden(a, fmt.Errorf("unexpected pod subresource %q, only 'status' and 'eviction' are allowed", a.GetSubresource()))
 		}
 
 	case nodeResource:
@@ -218,7 +218,7 @@ func (c *nodePlugin) admitPod(nodeName string, a admission.Attributes) error {
 		return nil
 
 	default:
-		return admission.NewForbidden(a, fmt.Errorf("unexpected operation %q", a.GetOperation()))
+		return admission.NewForbidden(a, fmt.Errorf("unexpected operation %q, node %q can only create and delete mirror pods", a.GetOperation(), nodeName))
 	}
 }
 
@@ -280,7 +280,7 @@ func (c *nodePlugin) admitPVCStatus(nodeName string, a admission.Attributes) err
 	switch a.GetOperation() {
 	case admission.Update:
 		if !c.features.Enabled(features.ExpandPersistentVolumes) {
-			return admission.NewForbidden(a, fmt.Errorf("node %q may not update persistentvolumeclaim metadata", nodeName))
+			return admission.NewForbidden(a, fmt.Errorf("node %q is not allowed to update persistentvolumeclaim metadata", nodeName))
 		}
 
 		oldPVC, ok := a.GetOldObject().(*api.PersistentVolumeClaim)
@@ -310,7 +310,7 @@ func (c *nodePlugin) admitPVCStatus(nodeName string, a admission.Attributes) err
 
 		// ensure no metadata changed. nodes should not be able to relabel, add finalizers/owners, etc
 		if !apiequality.Semantic.DeepEqual(oldPVC, newPVC) {
-			return admission.NewForbidden(a, fmt.Errorf("node %q may not update fields other than status.capacity and status.conditions: %v", nodeName, diff.ObjectReflectDiff(oldPVC, newPVC)))
+			return admission.NewForbidden(a, fmt.Errorf("node %q is not allowed to update fields other than status.capacity and status.conditions: %v", nodeName, diff.ObjectReflectDiff(oldPVC, newPVC)))
 		}
 
 		return nil
@@ -331,14 +331,14 @@ func (c *nodePlugin) admitNode(nodeName string, a admission.Attributes) error {
 		// Don't allow a node to create its Node API object with the config source set.
 		// We scope node access to things listed in the Node.Spec, so allowing this would allow a view escalation.
 		if node.Spec.ConfigSource != nil {
-			return admission.NewForbidden(a, fmt.Errorf("cannot create with non-nil configSource"))
+			return admission.NewForbidden(a, fmt.Errorf("node %q is not allowed to create pods with a non-nil configSource", nodeName))
 		}
 
 		// Don't allow a node to register with labels outside the allowed set.
 		// This would allow a node to add or modify its labels in a way that would let it steer privileged workloads to itself.
 		modifiedLabels := getModifiedLabels(node.Labels, nil)
 		if forbiddenLabels := c.getForbiddenCreateLabels(modifiedLabels); len(forbiddenLabels) > 0 {
-			return admission.NewForbidden(a, fmt.Errorf("cannot set labels: %s", strings.Join(forbiddenLabels.List(), ", ")))
+			return admission.NewForbidden(a, fmt.Errorf("node %q is not allowed to set the following labels: %s", nodeName, strings.Join(forbiddenLabels.List(), ", ")))
 		}
 		// check and warn if nodes set labels on create that would have been forbidden on update
 		// TODO(liggitt): in 1.17, expand getForbiddenCreateLabels to match getForbiddenUpdateLabels and drop this
@@ -352,7 +352,7 @@ func (c *nodePlugin) admitNode(nodeName string, a admission.Attributes) error {
 		}
 	}
 	if requestedName != nodeName {
-		return admission.NewForbidden(a, fmt.Errorf("node %q cannot modify node %q", nodeName, requestedName))
+		return admission.NewForbidden(a, fmt.Errorf("node %q is not allowed to modify node %q", nodeName, requestedName))
 	}
 
 	if a.GetOperation() == admission.Update {
@@ -369,20 +369,20 @@ func (c *nodePlugin) admitNode(nodeName string, a admission.Attributes) error {
 		// We scope node access to things listed in the Node.Spec, so allowing this would allow a view escalation.
 		// We only do the check if the new node's configSource is non-nil; old kubelets might drop the field during a status update.
 		if node.Spec.ConfigSource != nil && !apiequality.Semantic.DeepEqual(node.Spec.ConfigSource, oldNode.Spec.ConfigSource) {
-			return admission.NewForbidden(a, fmt.Errorf("node %q cannot update configSource to a new non-nil configSource", nodeName))
+			return admission.NewForbidden(a, fmt.Errorf("node %q is not allowed to update configSource to a new non-nil configSource", nodeName))
 		}
 
 		// Don't allow a node to update its own taints. This would allow a node to remove or modify its
 		// taints in a way that would let it steer disallowed workloads to itself.
 		if !apiequality.Semantic.DeepEqual(node.Spec.Taints, oldNode.Spec.Taints) {
-			return admission.NewForbidden(a, fmt.Errorf("node %q cannot modify taints", nodeName))
+			return admission.NewForbidden(a, fmt.Errorf("node %q is not allowed to modify taints", nodeName))
 		}
 
 		// Don't allow a node to update labels outside the allowed set.
 		// This would allow a node to add or modify its labels in a way that would let it steer privileged workloads to itself.
 		modifiedLabels := getModifiedLabels(node.Labels, oldNode.Labels)
 		if forbiddenUpdateLabels := c.getForbiddenUpdateLabels(modifiedLabels); len(forbiddenUpdateLabels) > 0 {
-			return admission.NewForbidden(a, fmt.Errorf("cannot modify labels: %s", strings.Join(forbiddenUpdateLabels.List(), ", ")))
+			return admission.NewForbidden(a, fmt.Errorf("is not allowed to modify labels: %s", strings.Join(forbiddenUpdateLabels.List(), ", ")))
 		}
 	}
 

plugin/pkg/admission/noderestriction/admission_test.go

@@ -868,7 +868,7 @@ func Test_nodePlugin_Admit(t *testing.T) {
 			name:       "forbid create of my node with forbidden labels",
 			podsGetter: noExistingPods,
 			attributes: admission.NewAttributesRecord(setForbiddenCreateLabels(mynodeObj, ""), nil, nodeKind, mynodeObj.Namespace, "", nodeResource, "", admission.Create, false, mynode),
-			err:        `cannot set labels: foo.node-restriction.kubernetes.io/foo, node-restriction.kubernetes.io/foo`,
+			err:        `is not allowed to set the following labels: foo.node-restriction.kubernetes.io/foo, node-restriction.kubernetes.io/foo`,
 		},
 		{
 			name:       "allow update of my node",
@@ -892,7 +892,7 @@ func Test_nodePlugin_Admit(t *testing.T) {
 			name:       "forbid create of my node with non-nil configSource",
 			podsGetter: noExistingPods,
 			attributes: admission.NewAttributesRecord(mynodeObjConfigA, nil, nodeKind, mynodeObj.Namespace, mynodeObj.Name, nodeResource, "", admission.Create, false, mynode),
-			err:        "create with non-nil configSource",
+			err:        "is not allowed to create pods with a non-nil configSource",
 		},
 		{
 			name:       "forbid update of my node: nil configSource to new non-nil configSource",
@@ -964,37 +964,37 @@ func Test_nodePlugin_Admit(t *testing.T) {
 			name:       "forbid update of my node: add taints",
 			podsGetter: existingPods,
 			attributes: admission.NewAttributesRecord(mynodeObjTaintA, mynodeObj, nodeKind, mynodeObj.Namespace, mynodeObj.Name, nodeResource, "", admission.Update, false, mynode),
-			err:        "cannot modify taints",
+			err:        "is not allowed to modify taints",
 		},
 		{
 			name:       "forbid update of my node: remove taints",
 			podsGetter: existingPods,
 			attributes: admission.NewAttributesRecord(mynodeObj, mynodeObjTaintA, nodeKind, mynodeObj.Namespace, mynodeObj.Name, nodeResource, "", admission.Update, false, mynode),
-			err:        "cannot modify taints",
+			err:        "is not allowed to modify taints",
 		},
 		{
 			name:       "forbid update of my node: change taints",
 			podsGetter: existingPods,
 			attributes: admission.NewAttributesRecord(mynodeObjTaintA, mynodeObjTaintB, nodeKind, mynodeObj.Namespace, mynodeObj.Name, nodeResource, "", admission.Update, false, mynode),
-			err:        "cannot modify taints",
+			err:        "is not allowed to modify taints",
 		},
 		{
 			name:       "forbid update of my node: add labels",
 			podsGetter: existingPods,
 			attributes: admission.NewAttributesRecord(setForbiddenUpdateLabels(mynodeObj, ""), mynodeObj, nodeKind, mynodeObj.Namespace, mynodeObj.Name, nodeResource, "", admission.Update, false, mynode),
-			err:        `cannot modify labels: foo.node-restriction.kubernetes.io/foo, node-restriction.kubernetes.io/foo, other.k8s.io/foo, other.kubernetes.io/foo`,
+			err:        `is not allowed to modify labels: foo.node-restriction.kubernetes.io/foo, node-restriction.kubernetes.io/foo, other.k8s.io/foo, other.kubernetes.io/foo`,
 		},
 		{
 			name:       "forbid update of my node: remove labels",
 			podsGetter: existingPods,
 			attributes: admission.NewAttributesRecord(mynodeObj, setForbiddenUpdateLabels(mynodeObj, ""), nodeKind, mynodeObj.Namespace, mynodeObj.Name, nodeResource, "", admission.Update, false, mynode),
-			err:        `cannot modify labels: foo.node-restriction.kubernetes.io/foo, node-restriction.kubernetes.io/foo, other.k8s.io/foo, other.kubernetes.io/foo`,
+			err:        `is not allowed to modify labels: foo.node-restriction.kubernetes.io/foo, node-restriction.kubernetes.io/foo, other.k8s.io/foo, other.kubernetes.io/foo`,
 		},
 		{
 			name:       "forbid update of my node: change labels",
 			podsGetter: existingPods,
 			attributes: admission.NewAttributesRecord(setForbiddenUpdateLabels(mynodeObj, "new"), setForbiddenUpdateLabels(mynodeObj, "old"), nodeKind, mynodeObj.Namespace, mynodeObj.Name, nodeResource, "", admission.Update, false, mynode),
-			err:        `cannot modify labels: foo.node-restriction.kubernetes.io/foo, node-restriction.kubernetes.io/foo, other.k8s.io/foo, other.kubernetes.io/foo`,
+			err:        `is not allowed to modify labels: foo.node-restriction.kubernetes.io/foo, node-restriction.kubernetes.io/foo, other.k8s.io/foo, other.kubernetes.io/foo`,
 		},
 
 		// Other node object
@@ -1002,31 +1002,31 @@ func Test_nodePlugin_Admit(t *testing.T) {
 			name:       "forbid create of other node",
 			podsGetter: noExistingPods,
 			attributes: admission.NewAttributesRecord(othernodeObj, nil, nodeKind, othernodeObj.Namespace, othernodeObj.Name, nodeResource, "", admission.Create, false, mynode),
-			err:        "cannot modify node",
+			err:        "is not allowed to modify node",
 		},
 		{
 			name:       "forbid create of other node pulling name from object",
 			podsGetter: noExistingPods,
 			attributes: admission.NewAttributesRecord(othernodeObj, nil, nodeKind, othernodeObj.Namespace, "", nodeResource, "", admission.Create, false, mynode),
-			err:        "cannot modify node",
+			err:        "is not allowed to modify node",
 		},
 		{
 			name:       "forbid update of other node",
 			podsGetter: existingPods,
 			attributes: admission.NewAttributesRecord(othernodeObj, othernodeObj, nodeKind, othernodeObj.Namespace, othernodeObj.Name, nodeResource, "", admission.Update, false, mynode),
-			err:        "cannot modify node",
+			err:        "is not allowed to modify node",
 		},
 		{
 			name:       "forbid delete of other node",
 			podsGetter: existingPods,
 			attributes: admission.NewAttributesRecord(nil, nil, nodeKind, othernodeObj.Namespace, othernodeObj.Name, nodeResource, "", admission.Delete, false, mynode),
-			err:        "cannot modify node",
+			err:        "is not allowed to modify node",
 		},
 		{
 			name:       "forbid update of other node status",
 			podsGetter: existingPods,
 			attributes: admission.NewAttributesRecord(othernodeObj, othernodeObj, nodeKind, othernodeObj.Namespace, othernodeObj.Name, nodeResource, "status", admission.Update, false, mynode),
-			err:        "cannot modify node",
+			err:        "is not allowed to modify node",
 		},
 
 		// Service accounts

plugin/pkg/auth/authorizer/node/node_authorizer.go

@@ -196,11 +196,11 @@ func (r *NodeAuthorizer) authorize(nodeName string, startingType vertexType, att
 	ok, err := r.hasPathFrom(nodeName, startingType, attrs.GetNamespace(), attrs.GetName())
 	if err != nil {
 		klog.V(2).Infof("NODE DENY: %v", err)
-		return authorizer.DecisionNoOpinion, "no path found to object", nil
+		return authorizer.DecisionNoOpinion, fmt.Sprintf("no relationship found between node %q and this object", nodeName), nil
 	}
 	if !ok {
 		klog.V(2).Infof("NODE DENY: %q %#v", nodeName, attrs)
-		return authorizer.DecisionNoOpinion, "no path found to object", nil
+		return authorizer.DecisionNoOpinion, fmt.Sprintf("no relationship found between node %q and this object", nodeName), nil
 	}
 	return authorizer.DecisionAllow, "", nil
 }
@@ -221,11 +221,11 @@ func (r *NodeAuthorizer) authorizeCreateToken(nodeName string, startingType vert
 	ok, err := r.hasPathFrom(nodeName, startingType, attrs.GetNamespace(), attrs.GetName())
 	if err != nil {
 		klog.V(2).Infof("NODE DENY: %v", err)
-		return authorizer.DecisionNoOpinion, "no path found to object", nil
+		return authorizer.DecisionNoOpinion, fmt.Sprintf("no relationship found between node %q and this object", nodeName), nil
 	}
 	if !ok {
 		klog.V(2).Infof("NODE DENY: %q %#v", nodeName, attrs)
-		return authorizer.DecisionNoOpinion, "no path found to object", nil
+		return authorizer.DecisionNoOpinion, fmt.Sprintf("no relationship found between node %q and this object", nodeName), nil
 	}
 	return authorizer.DecisionAllow, "", nil
 }
@@ -333,7 +333,7 @@ func (r *NodeAuthorizer) hasPathFrom(nodeName string, startingType vertexType, s
 		return found
 	})
 	if !found {
-		return false, fmt.Errorf("node %q cannot get %s %s/%s, no path was found", nodeName, vertexTypes[startingType], startingNamespace, startingName)
+		return false, fmt.Errorf("node %q cannot get %s %s/%s, no relationship to this object was found in the node authorizer graph", nodeName, vertexTypes[startingType], startingNamespace, startingName)
 	}
 	return true, nil
 }