From 16e355791f2c3438862cb09bf6f5213caaded996 Mon Sep 17 00:00:00 2001 From: Jordan Liggitt Date: Sat, 24 Nov 2018 09:31:10 -0500 Subject: [PATCH] Improve node authorizer and noderestriction forbidden messages --- .../admission/noderestriction/admission.go | 20 +++++++------- .../noderestriction/admission_test.go | 26 +++++++++---------- .../auth/authorizer/node/node_authorizer.go | 10 +++---- 3 files changed, 28 insertions(+), 28 deletions(-) diff --git a/plugin/pkg/admission/noderestriction/admission.go b/plugin/pkg/admission/noderestriction/admission.go index 222156a100..e44f026392 100644 --- a/plugin/pkg/admission/noderestriction/admission.go +++ b/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(), ", "))) } } diff --git a/plugin/pkg/admission/noderestriction/admission_test.go b/plugin/pkg/admission/noderestriction/admission_test.go index b271109e2a..1ebadaf9f9 100644 --- a/plugin/pkg/admission/noderestriction/admission_test.go +++ b/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 diff --git a/plugin/pkg/auth/authorizer/node/node_authorizer.go b/plugin/pkg/auth/authorizer/node/node_authorizer.go index 757b386861..1e541ee22a 100644 --- a/plugin/pkg/auth/authorizer/node/node_authorizer.go +++ b/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 }