Skip to content

Commit 3fb8935

Browse files
tniermantnierman
committed
SREP-769 - Only allow 'openshift.io/cluster-monitoring: true' label to be removed from namespaces
1 parent 85ffc1f commit 3fb8935

File tree

2 files changed

+155
-65
lines changed

2 files changed

+155
-65
lines changed

‎pkg/webhooks/namespace/namespace.go‎

Lines changed: 79 additions & 65 deletions
Original file line numberDiff line numberDiff line change
@@ -2,10 +2,12 @@ package namespace
22

33
import (
44
"fmt"
5+
"maps"
56
"net/http"
67
"os"
78
"regexp"
89
"slices"
10+
"strings"
911
"sync"
1012

1113
hookconfig "github.com/openshift/managed-cluster-validating-webhooks/pkg/config"
@@ -37,13 +39,16 @@ var (
3739
sreAdminGroups = []string{"system:serviceaccounts:openshift-backplane-srep"}
3840
privilegedServiceAccountsRe = regexp.MustCompile(utils.PrivilegedServiceAccountGroups)
3941
layeredProductNamespaceRe = regexp.MustCompile(layeredProductNamespace)
40-
// protectedLabels are labels which managed customers should not be allowed
41-
// change by dedicated-admins.
42+
// protectedLabels are labels which managed customers should not be allowed change
4243
protectedLabels = []string{
4344
// https://github.com/openshift/managed-cluster-config/tree/master/deploy/resource-quotas
4445
"managed.openshift.io/storage-pv-quota-exempt",
4546
"managed.openshift.io/service-lb-quota-exempt",
4647
}
48+
// removableProtectedLabels defines labels that unprivileged users can remove (but not add!) to unprivileged namespaces
49+
removableProtectedLabels = []string{
50+
"openshift.io/cluster-monitoring",
51+
}
4752

4853
log = logf.Log.WithName(WebhookName)
4954

@@ -181,57 +186,53 @@ func (s *NamespaceWebhook) Authorized(request admissionctl.Request) admissionctl
181186
// Is the request authorized?
182187
func (s *NamespaceWebhook) authorized(request admissionctl.Request) admissionctl.Response {
183188
var ret admissionctl.Response
184-
185-
// Picking OldObject or Object will suffice for most validation concerns
186-
ns, err := s.renderNamespace(request)
187-
if err != nil {
188-
log.Error(err, "Couldn't render a Namespace from the incoming request")
189-
return admissionctl.Errored(http.StatusBadRequest, err)
189+
// Admins are allowed to perform any operation
190+
if amIAdmin(request) {
191+
ret = admissionctl.Allowed("Cluster and SRE admins may access")
192+
ret.UID = request.AdmissionRequest.UID
193+
return ret
190194
}
191-
// service accounts making requests will include their name in the group
195+
// Privileged ServiceAccounts are allowed to perform any operation
192196
for _, group := range request.UserInfo.Groups {
193197
if privilegedServiceAccountsRe.Match([]byte(group)) {
194198
ret = admissionctl.Allowed("Privileged service accounts may access")
195199
ret.UID = request.AdmissionRequest.UID
196200
return ret
197201
}
198202
}
199-
// This must be prior to privileged namespace check
203+
204+
ns, err := s.renderNamespace(request)
205+
if err != nil {
206+
log.Error(err, "Couldn't render a Namespace from the incoming request")
207+
return admissionctl.Errored(http.StatusBadRequest, err)
208+
}
209+
210+
// Layered Product SRE can access their own namespaces
200211
if slices.Contains(request.UserInfo.Groups, layeredProductAdminGroupName) &&
201212
layeredProductNamespaceRe.Match([]byte(ns.GetName())) {
202213
ret = admissionctl.Allowed("Layered product admins may access")
203214
ret.UID = request.AdmissionRequest.UID
204215
return ret
205216
}
206217

218+
// Unprivileged users cannot modify privileged namespaces
207219
// L64-73
208220
if hookconfig.IsPrivilegedNamespace(ns.GetName()) {
209-
210-
if amIAdmin(request) {
211-
ret = admissionctl.Allowed("Cluster and SRE admins may access")
212-
ret.UID = request.AdmissionRequest.UID
213-
return ret
214-
}
215221
log.Info("Non-admin attempted to access a privileged namespace matching a regex from this list", "list", hookconfig.PrivilegedNamespaces, "request", request.AdmissionRequest)
216222
ret = admissionctl.Denied(fmt.Sprintf("Prevented from accessing Red Hat managed namespaces. Customer workloads should be placed in customer namespaces, and should not match an entry in this list of regular expressions: %v", hookconfig.PrivilegedNamespaces))
217223
ret.UID = request.AdmissionRequest.UID
218224
return ret
219225
}
226+
// Unprivileged users cannot create namespaces with certain names
220227
if BadNamespaceRe.Match([]byte(ns.GetName())) {
221-
222-
if amIAdmin(request) {
223-
ret = admissionctl.Allowed("Cluster and SRE admins may access")
224-
ret.UID = request.AdmissionRequest.UID
225-
return ret
226-
}
227228
log.Info("Non-admin attempted to access a potentially harmful namespace (eg matching this regex)", "regex", badNamespace, "request", request.AdmissionRequest)
228229
ret = admissionctl.Denied(fmt.Sprintf("Prevented from creating a potentially harmful namespace. Customer namespaces should not match this regular expression, as this would impact DNS resolution: %s", badNamespace))
229230
ret.UID = request.AdmissionRequest.UID
230231
return ret
231232
}
232-
// Check labels.
233+
// Unprivileged users cannot modify certain labels on unprivileged namespaces
233234
unauthorized, err := s.unauthorizedLabelChanges(request)
234-
if !amIAdmin(request) && unauthorized {
235+
if unauthorized {
235236
ret = admissionctl.Denied(fmt.Sprintf("Denied. Err %+v", err))
236237
ret.UID = request.AdmissionRequest.UID
237238
return ret
@@ -254,57 +255,70 @@ func (s *NamespaceWebhook) unauthorizedLabelChanges(req admissionctl.Request) (b
254255
return true, err
255256
}
256257
if req.Operation == admissionv1.Create {
257-
// For creations, we look to newNamespace and ensure no protectedLabels are set
258-
// We don't care about oldNamespace.
259-
protectedLabelsFound := doesNamespaceContainProtectedLabels(newNamespace)
260-
if len(protectedLabelsFound) == 0 {
258+
// Ensure no protected labels are set at creation-time
259+
protectedLabelsFound := protectedLabelsOnNamespace(newNamespace)
260+
removableProtectedLabelsFound := removableProtectedLabelsOnNamespace(newNamespace)
261+
262+
if len(protectedLabelsFound) == 0 && len(removableProtectedLabelsFound) == 0 {
261263
return false, nil
262264
}
263-
// There were some found
264-
return true, fmt.Errorf("Managed OpenShift customers may not directly set certain protected labels (%s) on Namespaces", protectedLabels)
265-
} else if req.Operation == admissionv1.Update {
266-
// For Updates we must see if the new object is making a change to the old one for any protected labels.
267-
// First, let's see if the old object had any protected labels we ought to
268-
// care about. If it has, then we can use that resulting list to compare to
269-
// the newNamespace for any changes. However, just because the oldNamespace
270-
// did not have any protected labels doesn't necessarily mean that we can
271-
// ignore potential setting of those labels' values in the newNamespace.
272-
273-
// protectedLabelsFoundInOld is a slice of all instances of protectedLabels
274-
// that appeared in the oldNamespace that we need to be sure have not
275-
// changed.
276-
protectedLabelsFoundInOld := doesNamespaceContainProtectedLabels(oldNamespace)
277-
// protectedLabelsFoundInNew is a slice of all instances of protectedLabels
278-
// that appeared in the newNamespace that we need to be sure do not have a
279-
// value different than oldNamespace.
280-
protectedLabelsFoundInNew := doesNamespaceContainProtectedLabels(newNamespace)
281-
282-
// First check: Were any protectedLabels deleted?
283-
if len(protectedLabelsFoundInOld) != len(protectedLabelsFoundInNew) {
284-
// If we have x protectedLabels in the oldNamespace then we expect to also
285-
// have x protectedLabels in the newNamespace. Any difference is a removal or addition
286-
return true, fmt.Errorf("Managed OpenShift customers may not add or remove protected labels (%s) from Namespaces", protectedLabels)
265+
return true, fmt.Errorf("Managed OpenShift customers may not directly set certain protected labels (%s) on Namespaces", strings.Join(append(protectedLabels, removableProtectedLabels...), ", "))
266+
}
267+
if req.Operation == admissionv1.Update {
268+
// Check whether protected labels had their key or value altered
269+
protectedLabelsFoundInOld := protectedLabelsOnNamespace(oldNamespace)
270+
protectedLabelsFoundInNew := protectedLabelsOnNamespace(newNamespace)
271+
protectedLabelsUnchanged := maps.Equal(protectedLabelsFoundInOld, protectedLabelsFoundInNew)
272+
if !protectedLabelsUnchanged {
273+
return true, fmt.Errorf("Managed OpenShift customers may not add or remove the following protected labels from Namespaces: (%s)", protectedLabels)
287274
}
288-
// Next check: Compare values to ensure there are no changes in the protected labels
289-
for _, labelKey := range protectedLabelsFoundInOld {
290-
if oldNamespace.Labels[labelKey] != newNamespace.ObjectMeta.Labels[labelKey] {
291-
return true, fmt.Errorf("Managed OpenShift customers may not change the value or certain protected labels (%s) on Namespaces. %s changed from %s to %s", protectedLabels, labelKey, oldNamespace.Labels[labelKey], newNamespace.ObjectMeta.Labels[labelKey])
292-
}
275+
276+
// Check whether a removableProtectedLabel was added
277+
removableProtectedLabelsFoundInOld := removableProtectedLabelsOnNamespace(oldNamespace)
278+
removableProtectedLabelsFoundInNew := removableProtectedLabelsOnNamespace(newNamespace)
279+
removableProtectedLabelsAdded := unauthorizedRemovableProtectedLabelChange(removableProtectedLabelsFoundInOld, removableProtectedLabelsFoundInNew)
280+
281+
if removableProtectedLabelsAdded {
282+
return true, fmt.Errorf("Managed OpenShift customers may only remove the following protected labels from Namespaces: (%s)", removableProtectedLabels)
293283
}
294284
}
295285
return false, nil
296286
}
297287

298-
// doesNamespaceContainProtectedLabels checks the namespace for any instances of
299-
// protectedLabels and returns a slice of any instances of matches
300-
func doesNamespaceContainProtectedLabels(ns *corev1.Namespace) []string {
301-
foundLabelNames := make([]string, 0)
302-
for _, label := range protectedLabels {
303-
if _, found := ns.ObjectMeta.Labels[label]; found {
304-
foundLabelNames = append(foundLabelNames, label)
288+
// unauthorizedRemovableProtectedLabelChange returns true if a protectedRemovableLabel was added or had it's value changed
289+
func unauthorizedRemovableProtectedLabelChange(oldLabels, newLabels map[string]string) bool {
290+
// All we need to validate is that every given new label was present in the set of old labels
291+
for key, newValue := range newLabels {
292+
oldValue, found := oldLabels[key]
293+
if !found {
294+
return true
295+
}
296+
if newValue != oldValue {
297+
return true
298+
}
299+
}
300+
return false
301+
}
302+
303+
// protectedLabelsInNamespace returns any protectedLabels present in the namespace object
304+
func protectedLabelsOnNamespace(ns *corev1.Namespace) map[string]string {
305+
return labelSetInNamespace(ns, protectedLabels)
306+
}
307+
308+
// removableProtectedLabelsInNamespace returns any removableProtectedLabels in the namespace object
309+
func removableProtectedLabelsOnNamespace(ns *corev1.Namespace) map[string]string {
310+
return labelSetInNamespace(ns, removableProtectedLabels)
311+
}
312+
313+
func labelSetInNamespace(ns *corev1.Namespace, labels []string) map[string]string {
314+
foundLabels := map[string]string{}
315+
for _, label := range labels {
316+
value, found := ns.Labels[label]
317+
if found {
318+
foundLabels[label] = value
305319
}
306320
}
307-
return foundLabelNames
321+
return foundLabels
308322
}
309323

310324
// SyncSetLabelSelector returns the label selector to use in the SyncSet.

‎pkg/webhooks/namespace/namespace_test.go‎

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1026,6 +1026,82 @@ func TestLabellingUpdates(t *testing.T) {
10261026
},
10271027
shouldBeAllowed: true,
10281028
},
1029+
{
1030+
testID: "user-can-remove-removable-label-from-unpriv-ns",
1031+
targetNamespace: "my-customer-ns",
1032+
username: "test@user",
1033+
userGroups: []string{"system:authenticated", "system:authenticated:oauth"},
1034+
operation: admissionv1.Update,
1035+
oldObject: createOldObject("my-customer-ns", "user-can-remove-removable-label-from-unpriv-ns", map[string]string{
1036+
"openshift.io/cluster-monitoring": "true",
1037+
}),
1038+
labels: map[string]string{},
1039+
shouldBeAllowed: true,
1040+
},
1041+
{
1042+
testID: "user-cant-alter-removable-label-key-unpriv-ns",
1043+
targetNamespace: "my-customer-ns",
1044+
username: "test@user",
1045+
userGroups: []string{"system:authenticated", "system:authenticated:oauth"},
1046+
operation: admissionv1.Update,
1047+
oldObject: createOldObject("my-customer-ns", "user-can-remove-removable-label-from-unpriv-ns", map[string]string{
1048+
"openshift.io/cluster-monitoring": "true",
1049+
}),
1050+
labels: map[string]string{"openshift.io/cluster-monitoring": "false"},
1051+
shouldBeAllowed: false,
1052+
},
1053+
{
1054+
testID: "user-cant-add-removable-label-on-unpriv-ns",
1055+
targetNamespace: "my-customer-ns",
1056+
username: "test@user",
1057+
userGroups: []string{"system:authenticated", "system:authenticated:oauth"},
1058+
operation: admissionv1.Update,
1059+
oldObject: createOldObject("my-customer-ns", "user-can-remove-removable-label-from-unpriv-ns", map[string]string{}),
1060+
labels: map[string]string{"openshift.io/cluster-monitoring": "true"},
1061+
shouldBeAllowed: false,
1062+
},
1063+
{
1064+
testID: "cluster-admin-cant-add-removable-label-on-unpriv-ns",
1065+
targetNamespace: "my-customer-ns",
1066+
username: "test@user",
1067+
userGroups: []string{"cluster-admins", "system:authenticated", "system:authenticated:oauth"},
1068+
operation: admissionv1.Update,
1069+
oldObject: createOldObject("my-customer-ns", "user-can-remove-removable-label-from-unpriv-ns", map[string]string{}),
1070+
labels: map[string]string{"openshift.io/cluster-monitoring": "true"},
1071+
shouldBeAllowed: false,
1072+
},
1073+
{
1074+
testID: "backplane-cluster-admin-can-add-removable-label-on-unpriv-ns",
1075+
targetNamespace: "my-customer-ns",
1076+
username: "backplane-cluster-admin",
1077+
userGroups: []string{"system:authenticated", "system:authenticated:oauth"},
1078+
operation: admissionv1.Update,
1079+
oldObject: createOldObject("my-customer-ns", "user-can-remove-removable-label-from-unpriv-ns", map[string]string{}),
1080+
labels: map[string]string{"openshift.io/cluster-monitoring": "true"},
1081+
shouldBeAllowed: true,
1082+
},
1083+
{
1084+
testID: "backplane-cluster-admin-can-add-removable-label-on-priv-ns",
1085+
targetNamespace: "openshift-kube-apiserver",
1086+
username: "backplane-cluster-admin",
1087+
userGroups: []string{"system:authenticated", "system:authenticated:oauth"},
1088+
operation: admissionv1.Update,
1089+
oldObject: createOldObject("my-customer-ns", "user-can-remove-removable-label-from-unpriv-ns", map[string]string{}),
1090+
labels: map[string]string{"openshift.io/cluster-monitoring": "true"},
1091+
shouldBeAllowed: true,
1092+
},
1093+
{
1094+
testID: "backplane-cluster-admin-can-remove-removable-label-on-priv-ns",
1095+
targetNamespace: "openshift-kube-apiserver",
1096+
username: "backplane-cluster-admin",
1097+
userGroups: []string{"system:authenticated", "system:authenticated:oauth"},
1098+
operation: admissionv1.Update,
1099+
oldObject: createOldObject("my-customer-ns", "user-can-remove-removable-label-from-unpriv-ns", map[string]string{
1100+
"openshift.io/cluster-monitoring": "true",
1101+
}),
1102+
labels: map[string]string{},
1103+
shouldBeAllowed: true,
1104+
},
10291105
}
10301106
runNamespaceTests(t, tests)
10311107
}

0 commit comments

Comments
 (0)