Merge pull request #253 from maxsokolovsky/add-docs-and-tests

Add docs and tests

README.md

@@ -27,7 +27,7 @@ It handles TLS certificates and the management of associated Secrets for secure
 Documentation on each of the resources that are validated or mutated can be found in `docs.md`. It is recommended to review the [kubernetes docs on CRDs](https://kubernetes.io/docs/concepts/extend-kubernetes/api-extension/custom-resources/#customresourcedefinitions) as well.
 
 Docs are added by creating a resource-specific readme in the directory of your mutator/validator (e.x. `pkg/resources/$GROUP/$GROUP_VERSION/$RESOURCE/$READABLE_RESOURCE.MD`). 
-These files should be named with a human-readable version of the resource's name.
+These files should be named with a human-readable version of the resource's name. For example, `GlobalRole.md`.
 Running `go generate` will then aggregate these into the user-facing docs in the `docs.md` file.
 
 ## Webhooks

docs.md

@@ -4,13 +4,44 @@
 
 ### Validation Checks
 
-Note: The kube-system namespace, unlike other namespaces, has a "failPolicy" of "ignore" on update calls.
+Note: The `kube-system` namespace, unlike other namespaces, has a `failPolicy` of `ignore` on update calls.
+
+#### Project annotation
+Verifies that the annotation `field.cattle.io/projectId` value can only be updated by users with the `manage-namespaces` 
+verb on the project specified in the annotation.
 
 #### PSA Label Validation
 
-Validates that users who create or edit a PSA enforcement label on a namespace have the `updatepsa` verb on `projects` in `management.cattle.io/v3`. See the [upstream docs](https://kubernetes.io/docs/concepts/security/pod-security-admission/) for more information on the effect of these labels.
+Validates that users who create or edit a PSA enforcement label on a namespace have the `updatepsa` verb on `projects` 
+in `management.cattle.io/v3`. See the [upstream docs](https://kubernetes.io/docs/concepts/security/pod-security-admission/) 
+for more information on the effect of these labels.
+
+The following labels are considered relevant for PSA enforcement: 
+- pod-security.kubernetes.io/enforce
+- pod-security.kubernetes.io/enforce-version 
+- pod-security.kubernetes.io/audit 
+- pod-security.kubernetes.io/audit-version 
+- pod-security.kubernetes.io/warn
+- pod-security.kubernetes.io/warn-version
+
+## Secret 
+
+### Validation Checks
+
+A secret cannot be deleted if its deletion request has an orphan policy,
+and the secret has roles or role bindings dependent on it.
+
+### Mutation Checks
+
+#### On create
+
+For all secrets of type `provisioning.cattle.io/cloud-credential`, 
+places a `field.cattle.io/creatorId` annotation with the name of the user as the value.
 
-The following labels are considered relevant labels for PSA enforcement: `"pod-security.kubernetes.io/enforce", "pod-security.kubernetes.io/enforce-version", "pod-security.kubernetes.io/audit", "pod-security.kubernetes.io/audit-version", "pod-security.kubernetes.io/warn", "pod-security.kubernetes.io/warn-version"`.
+#### On delete
+
+Checks if there are any RoleBindings owned by this secret which provide access to a role granting access to this secret.
+If yes, the webhook redacts the role, so that it only grants a deletion permission.
 
 # management.cattle.io/v3 
 
@@ -25,10 +56,10 @@ Users can only create/update ClusterRoleTemplateBindings which grant permissions
 #### Invalid Fields - Create
 
 Users cannot create ClusterRoleTemplateBindings which violate the following constraints:
-- Either a user subject (through "UserName" or "UserPrincipalName") or a group subject (through "GroupName" or "GroupPrincipalName") must be specified; both a user subject and group subject cannot be specified
-- A "ClusterName" must be specified
-- The roleTemplate indicated in "RoleTemplateName" must be:
-  - Valid (i.e. is an existing `roleTemplate` object in the `management.cattle.io/v3` apiGroup)
+- Either a user subject (through `UserName` or `UserPrincipalName`) or a group subject (through `GroupName` or `GroupPrincipalName`) must be specified; both a user subject and a group subject cannot be specified
+- `ClusterName` must be specified
+- The roleTemplate indicated in `RoleTemplateName` must be:
+  - Valid (i.e. is an existing `roleTemplate` object in the `management.cattle.io/v3` API group)
   - Not locked (i.e. `roleTemplate.Locked` must be `false`)
 
 #### Invalid Fields - Update
@@ -45,11 +76,22 @@ Users can update the following fields if they have not been set, but after they
 
 In addition, as in the create validation, both a user subject and a group subject cannot be specified.
 
+## Feature 
+
+### Validation Checks
+
+#### On update
+
+The desired value must not change on new spec unless it's equal to the `lockedValue` or `lockedValue` is nil.
+
 ## GlobalRole 
 
 ### Validation Checks
 
-Note: all checks are bypassed if the GlobalRole is being deleted
+Note: all checks are bypassed if the GlobalRole is being deleted.
+
+#### Invalid Fields - Create and Update
+When a GlobalRole is created or updated, the webhook checks that each rule has at least one verb.
 
 #### Escalation Prevention
 
@@ -59,7 +101,7 @@ Users can only change GlobalRoles with rights less than or equal to those they c
 
 ### Validation Checks
 
-Note: all checks are bypassed if the GlobalRoleBinding is being deleted
+Note: all checks are bypassed if the GlobalRoleBinding is being deleted.
 
 #### Escalation Prevention
 
@@ -90,10 +132,10 @@ Users can only create/update ProjectRoleTemplateBindings with rights less than o
 #### Invalid Fields - Create
 
 Users cannot create ProjectRoleTemplateBindings which violate the following constraints:
-- Either a user subject (through "UserName" or "UserPrincipalName") or a group subject (through "GroupName" or "GroupPrincipalName") must be specified; both a user subject and group subject cannot be specified
-- A "ProjectName" must be specified
-- The roleTemplate indicated in "RoleTemplateName" must be:
-  - Valid (i.e. is an existing `roleTemplate` object in the `management.cattle.io/v3` apiGroup)
+- Either a user subject (through `UserName` or `UserPrincipalName`) or a group subject (through `GroupName` or `GroupPrincipalName`) must be specified; both a user subject and a group subject cannot be specified
+- `ProjectName` must be specified
+- The roleTemplate indicated in `RoleTemplateName` must be:
+  - Valid (i.e. is an existing `roleTemplate` object in the `management.cattle.io/v3` API group)
   - Not locked (i.e. `roleTemplate.Locked` must be `false`)
 
 #### Invalid Fields - Update
@@ -102,7 +144,7 @@ Users cannot update the following fields after creation:
 - RoleTemplateName
 - ProjectName
 
-Users can update the following fields if they have not been set, but after they have been set they cannot be changed:
+Users can update the following fields if they have not been set, but after they have been set, they cannot be changed:
 - UserName
 - UserPrincipalName
 - GroupName

pkg/codegen/docs.go

@@ -75,9 +75,10 @@ func generateDocs(resourcesBaseDir, outputFilePath string) (err error) {
 			if err != nil {
 				return fmt.Errorf("unable to write content for %s/%s.%s: %w", docFile.group, docFile.version, docFile.resource, err)
 			}
-
 		}
-
+		if scanner.Err() != nil {
+			return fmt.Errorf("got an error scanning content for %s/%s.%s: %w", docFile.group, docFile.version, docFile.resource, err)
+		}
 	}
 	return nil
 }

pkg/resources/core/v1/namespace/Namespace.md

@@ -1,10 +1,22 @@
 ## Validation Checks
 
-Note: The kube-system namespace, unlike other namespaces, has a "failPolicy" of "ignore" on update calls.
+Note: The `kube-system` namespace, unlike other namespaces, has a `failPolicy` of `ignore` on update calls.
+
+### Project annotation
+Verifies that the annotation `field.cattle.io/projectId` value can only be updated by users with the `manage-namespaces` 
+verb on the project specified in the annotation.
 
 ### PSA Label Validation
 
-Validates that users who create or edit a PSA enforcement label on a namespace have the `updatepsa` verb on `projects` in `management.cattle.io/v3`. See the [upstream docs](https://kubernetes.io/docs/concepts/security/pod-security-admission/) for more information on the effect of these labels.
+Validates that users who create or edit a PSA enforcement label on a namespace have the `updatepsa` verb on `projects` 
+in `management.cattle.io/v3`. See the [upstream docs](https://kubernetes.io/docs/concepts/security/pod-security-admission/) 
+for more information on the effect of these labels.
 
-The following labels are considered relevant labels for PSA enforcement: `"pod-security.kubernetes.io/enforce", "pod-security.kubernetes.io/enforce-version", "pod-security.kubernetes.io/audit", "pod-security.kubernetes.io/audit-version", "pod-security.kubernetes.io/warn", "pod-security.kubernetes.io/warn-version"`.
+The following labels are considered relevant for PSA enforcement: 
+- pod-security.kubernetes.io/enforce
+- pod-security.kubernetes.io/enforce-version 
+- pod-security.kubernetes.io/audit 
+- pod-security.kubernetes.io/audit-version 
+- pod-security.kubernetes.io/warn
+- pod-security.kubernetes.io/warn-version
 

pkg/resources/core/v1/secret/Secret.md

@@ -0,0 +1,16 @@
+## Validation Checks
+
+A secret cannot be deleted if its deletion request has an orphan policy,
+and the secret has roles or role bindings dependent on it.
+
+## Mutation Checks
+
+### On create
+
+For all secrets of type `provisioning.cattle.io/cloud-credential`, 
+places a `field.cattle.io/creatorId` annotation with the name of the user as the value.
+
+### On delete
+
+Checks if there are any RoleBindings owned by this secret which provide access to a role granting access to this secret.
+If yes, the webhook redacts the role, so that it only grants a deletion permission.

pkg/resources/core/v1/secret/mutator.go

@@ -123,8 +123,8 @@ func (m *Mutator) admitCreate(secret *corev1.Secret, request *admission.Request)
 	return response, nil
 }
 
-// admitDelete checks to see if there are any roleBindings owned by this secret which provide access to a role granting access to this secret
-// if so, it redacts the role so that it only grants delete access. This handles cases where users were given owner access to an individual secret
+// admitDelete checks if there are any roleBindings owned by this secret which provide access to a role granting access to this secret.
+// If yes, it redacts the role, so that it only grants a deletion permission. This handles cases where users were given owner access to an individual secret
 // through a controller (like cloud-credentials), and delete the secret but keep the rbac
 func (m *Mutator) admitDelete(secret *corev1.Secret) (*admissionv1.AdmissionResponse, error) {
 	roleBindings, err := m.roleBindingController.Cache().GetByIndex(mutatorRoleBindingOwnerIndex, fmt.Sprintf(ownerFormat, secret.Namespace, secret.Name))

pkg/resources/core/v1/secret/mutator_test.go

@@ -19,6 +19,11 @@ import (
 	"k8s.io/apimachinery/pkg/runtime/schema"
 )
 
+var (
+	secretGVR = metav1.GroupVersionResource{Group: "", Version: "v1", Resource: "secrets"}
+	secretGVK = metav1.GroupVersionKind{Group: "", Version: "v1", Kind: "Secret"}
+)
+
 func Test_roleBindingIndexer(t *testing.T) {
 	testNamespace := "test-ns"
 	createBinding := func(roleRefKind string, ownerRefs ...metav1.OwnerReference) rbacv1.RoleBinding {
@@ -116,9 +121,7 @@ func Test_roleBindingIndexer(t *testing.T) {
 	}
 }
 
-func TestMutatorAdmit(t *testing.T) {
-	secretGVR := metav1.GroupVersionResource{Group: "", Version: "v1", Resource: "secrets"}
-	secretGVK := metav1.GroupVersionKind{Group: "", Version: "v1", Kind: "Secret"}
+func TestMutatorAdmitOnDelete(t *testing.T) {
 	const secretName = "test-secret"
 	const secretNamespace = "test-ns"
 
@@ -363,6 +366,71 @@ func TestMutatorAdmit(t *testing.T) {
 	}
 }
 
+func TestMutatorAdmitOnCreate(t *testing.T) {
+	t.Parallel()
+	tests := []struct {
+		name      string
+		secret    corev1.Secret
+		wantAdmit bool
+		wantErr   bool
+	}{
+		{
+			name:      "create secret",
+			secret:    corev1.Secret{},
+			wantAdmit: true,
+		},
+		{
+			name: "create cloud credential secret",
+			secret: corev1.Secret{
+				Type: "provisioning.cattle.io/cloud-credential",
+			},
+			wantAdmit: true,
+		},
+	}
+
+	ctrl := gomock.NewController(t)
+	roleBindingController := fakes.NewMockRoleBindingController(ctrl)
+	roleController := fakes.NewMockRoleController(ctrl)
+	roleBindingCache := fakes.NewMockRoleBindingCache(ctrl)
+	roleBindingController.EXPECT().Cache().Return(roleBindingCache).AnyTimes()
+	roleBindingCache.EXPECT().AddIndexer(gomock.Any(), gomock.Any())
+
+	mutator := NewMutator(roleController, roleBindingController)
+
+	for _, test := range tests {
+		test := test
+		t.Run(test.name, func(t *testing.T) {
+			t.Parallel()
+			req := admission.Request{
+				AdmissionRequest: admissionv1.AdmissionRequest{
+					UID:             "2",
+					Kind:            secretGVK,
+					Resource:        secretGVR,
+					RequestKind:     &secretGVK,
+					RequestResource: &secretGVR,
+					Name:            "my-secret",
+					Namespace:       "test-ns",
+					Operation:       admissionv1.Create,
+					UserInfo:        authenicationv1.UserInfo{Username: "test-user", UID: ""},
+					Object:          runtime.RawExtension{},
+					OldObject:       runtime.RawExtension{},
+				},
+			}
+			var err error
+			req.Object.Raw, err = json.Marshal(test.secret)
+			require.NoError(t, err)
+
+			resp, err := mutator.Admit(&req)
+			if test.wantErr {
+				require.Error(t, err)
+			} else {
+				require.NoError(t, err)
+				require.Equal(t, test.wantAdmit, resp.Allowed)
+			}
+		})
+	}
+}
+
 func addRoleRefToBinding(role rbacv1.Role, binding rbacv1.RoleBinding) *rbacv1.RoleBinding {
 	newBinding := binding.DeepCopy()
 	newBinding.RoleRef = rbacv1.RoleRef{

pkg/resources/core/v1/secret/validator.go

@@ -3,7 +3,6 @@ package secret
 import (
 	"encoding/json"
 	"fmt"
-	"net/http"
 
 	"github.com/rancher/webhook/pkg/admission"
 	objectsv1 "github.com/rancher/webhook/pkg/generated/objects/core/v1"
@@ -99,13 +98,15 @@ func (a *admitter) Admit(request *admission.Request) (*admissionv1.AdmissionResp
 	// we are only concerned with requests that attempt to orphan resources
 	if !hasOrphanDependents && !hasOrphanPolicy {
 		return admission.ResponseAllowed(), nil
-
 	}
 	secret, err := objectsv1.SecretFromRequest(&request.AdmissionRequest)
 	if err != nil {
 		return nil, fmt.Errorf("unable to read secret from request: %w", err)
 	}
 	roles, roleBindings, err := a.getRbacRefs(secret)
+	if err != nil {
+		return nil, fmt.Errorf("unable to determine if secret has rbac refs: %w", err)
+	}
 	if logrus.IsLevelEnabled(logrus.DebugLevel) {
 		roleNames := make([]string, len(roles))
 		roleBindingNames := make([]string, len(roleBindings))
@@ -117,23 +118,12 @@ func (a *admitter) Admit(request *admission.Request) (*admissionv1.AdmissionResp
 		}
 		logrus.Debugf("[%s] secret %s owns roles: %v and roleBindings %v", logPrefix, secret.Name, roleNames, roleBindingNames)
 	}
-	if err != nil {
-		return nil, fmt.Errorf("unable to determine if secret has rbac refs: %w", err)
-	}
 	// requests which orphan non-rbac resources are allowed
 	if len(roles) == 0 && len(roleBindings) == 0 {
 		return admission.ResponseAllowed(), nil
 	}
 	// secret orphans rbac resources, deny the request
-	return &admissionv1.AdmissionResponse{
-		Allowed: false,
-		Result: &metav1.Status{
-			Status:  "Failure",
-			Message: "A secret which owns RBAC objects cannot be deleted with OrphanDependents: true or PropagationPolicy: Orphan",
-			Reason:  metav1.StatusReasonBadRequest,
-			Code:    http.StatusBadRequest,
-		},
-	}, nil
+	return admission.ResponseBadRequest("A secret which owns RBAC objects cannot be deleted with OrphanDependents: true or PropagationPolicy: Orphan"), nil
 }
 
 // getRbacRefs checks to see if there are any existing rbac resources which could be orphaned by this delete call

pkg/resources/management.cattle.io/v3/clusterroletemplatebinding/ClusterRoleTemplateBinding.md

@@ -7,10 +7,10 @@ Users can only create/update ClusterRoleTemplateBindings which grant permissions
 ### Invalid Fields - Create
 
 Users cannot create ClusterRoleTemplateBindings which violate the following constraints:
-- Either a user subject (through "UserName" or "UserPrincipalName") or a group subject (through "GroupName" or "GroupPrincipalName") must be specified; both a user subject and group subject cannot be specified
-- A "ClusterName" must be specified
-- The roleTemplate indicated in "RoleTemplateName" must be:
-  - Valid (i.e. is an existing `roleTemplate` object in the `management.cattle.io/v3` apiGroup)
+- Either a user subject (through `UserName` or `UserPrincipalName`) or a group subject (through `GroupName` or `GroupPrincipalName`) must be specified; both a user subject and a group subject cannot be specified
+- `ClusterName` must be specified
+- The roleTemplate indicated in `RoleTemplateName` must be:
+  - Valid (i.e. is an existing `roleTemplate` object in the `management.cattle.io/v3` API group)
   - Not locked (i.e. `roleTemplate.Locked` must be `false`)
 
 ### Invalid Fields - Update

pkg/resources/management.cattle.io/v3/feature/Feature.md

@@ -0,0 +1,5 @@
+## Validation Checks
+
+### On update
+
+The desired value must not change on new spec unless it's equal to the `lockedValue` or `lockedValue` is nil.