feat(rbac): implement hardened v1alpha1 RoleTemplate/RoleAssignment with admission, ceiling, and VWC bootstrap #11
No reviewers
Labels
No labels
No milestone
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
DevFW-CICD/rbac-controller-manager!11
Loading…
Reference in a new issue
No description provided.
Delete branch "ipceicis-9690-rbac-impl-local"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Controller-util dependency assessment and controller hardening fixes (no controller-util added).
controller-util Assessment
Decision: do not adopt controller-util.
controller-util (
PersistFailure,kcpws,mctest,kcppath) is not adopted in this PR. Rationale:PersistFailurewraps status-patch + error propagation. Our existingpatchStatus+ caller-returns-error pattern is equivalent with fewer transitive dependencies.kcpws/kcppathadd KCP workspace path utilities not yet needed outside the bootstrap path.mctestadds test helpers overenvtest; the current fake-client approach is simpler and has no envtest dependency.Changes
🔒 Security: stale-CRB deletion in
block()must requeue on error (finding 1)block()in the roleassignment reconciler previously logged a Delete error for the staleClusterRoleBindingand continued to patch statusBlocked, returningnil. This meant the work-queue did not retry: a transient API-server failure left the stale CRB alive while the controller believed it had revoked access.Fix:
block()now returns the error whenDeletefails, causing the work-queue to requeue. Status is only patched after successful deletion.Test added:
TestReconcile_BlockDeleteError_RequeuesAndLeavesStale— injects aDeleteinterceptor error, asserts non-nil reconcile error and stale CRB still present.♻️ Refactor: extract
internal/namingpackage (finding 2)Three controllers each contained an identical
sha256-truncation naming helper with only the prefix differing. Extracted tointernal/naming:ClusterRoleName(rtName)—edge-connect-rt-<name>(≤63 chars)AssignmentCRBName(raName)—edge-connect-ra-<name>(≤63 chars)BindingCRBName(crbName)—edge-connect-<name>(≤63 chars)Generated names are unchanged.
roletemplateno longer duplicatesassignmentCRBName(import-cycle workaround now unneeded). Regression tests guard against algorithm drift.Tests added:
naming_test.go— length, exact-boundary, one-beyond-boundary, determinism, no-collision-across-prefixes, and known-name regression tests.♻️ Refactor: replace subset
labelsMatchwithmaps.Equal(finding 3)labelsMatchwas a subset check (only tested that desired keys were present). Stale labels added outside the controller were not removed on update. Replaced withmaps.Equalin roleassignment and clusterrolebinding so the label set is replaced wholesale on drift.Tests added:
TestReconcile_StaleLabel_Removedin both packages.♻️ Refactor: deduplicate condition/label constants (finding 4)
Local
ConditionSynced,ConditionReady,ConditionTemplateResolved,LabelManagedBy,LabelManagedByValue,LabelSourceKind,LabelSourceNameconstants in all three controller packages now reference the canonicalapi/v1alpha1package constants. Per-typesetConditionwrappers retained — they are controller-specific and adding an external dependency solely for them is not warranted.Not changed
controller-util,kcpws,mctest,kcppath, orPersistFailuredependency added.block()security fix.poc-core-deployor published artefacts.Refs: IPCEICIS-9690
Critical: Tenant-authored templates are materialized without permission validation
api/v1alpha1/roletemplate_types.go:90-133internal/controller/roletemplate/roletemplate.go:131-166spec.rulespermits wildcards, and the reconciler copies the rules directly into a nativeClusterRole. No admission webhook or controller-side permission-ceiling validation exists.Any tenant permitted to create
RoleTemplateandRoleAssignmentcan potentially request arbitrary permissions and bind them to arbitrary users or groups. Deferring validation while enabling materialization makes the controller a confused deputy.The controller must reject templates exceeding an enforced permission ceiling before creating or updating a native
ClusterRole.High:
spec.sealedis mutable and spoofableapi/v1alpha1/roletemplate_types.go:107-114internal/controller/roletemplate/roletemplate.go:145-148Any caller allowed to create or update
RoleTemplatecan:The controller then stamps the materialized role with the platform-preset label. There is no creator validation, immutability rule, or admission webhook.
Additionally, sealed templates remain fully mutable. Updating their rules causes the controller to update the native
ClusterRole.Do not treat
spec.sealedas platform provenance. Remove it from user-controlled input or enforce platform-only creation, update, patch, and deletion before materialization is enabled.High: Blocked assignments leave existing authorization active
internal/controller/roleassignment/roleassignment.go:83-106,221-229When a previously ready template becomes missing or not ready,
block()only updates status. It does not delete the existing nativeClusterRoleBinding.If the old
ClusterRoleremains, access remains effective despite the assignment reportingBlocked. If the role is deleted, the dangling binding can silently become effective again when a role with the same name appears.The blocked transition must remove the generated binding and clear its status reference.
High: Blocked assignments do not recover when templates become ready
internal/controller/roleassignment/roleassignment.go:260-265The controller watches
RoleAssignmentand owned native bindings, but notRoleTemplate.An assignment created before its template exists becomes
Blocked. Creating or reconciling the template later does not enqueue the assignment, and the blocked path has no timed requeue.Add a
RoleTemplatewatch mapping templates to referencing assignments, with tests for both readiness and invalidation transitions.High:
forkedFromis accepted but not validatedapi/v1alpha1/roletemplate_types.go:116-123internal/controller/roletemplate/roletemplate.go:150-156The implementation does not:
It merely copies the claimed source into an annotation. This creates misleading provenance for arbitrary permissions. Reject
forkedFromuntil containment enforcement exists, or clearly make it non-authoritative.Medium: Unsupported propagation modes still report
Readyapi/v1alpha1/roleassignment_types.go:15-66internal/controller/roleassignment/roleassignment.go:164-177The CRD accepts
SelectedWorkspacesandAllWorkspaces, but the reconciler applies only the local workspace and still reports the assignment phase asReady.A caller can request global propagation and receive overall success even though the requested state was not achieved. Unsupported modes should be rejected or leave the resource non-ready.
Medium:
capabilityRequirementsis ignoredapi/v1alpha1/roletemplate_types.go:125-133Templates with unmet capability requirements are materialized and marked
Ready. This makes a security-relevant API field purely decorative.Reject the field until implemented, or evaluate requirements and report a non-ready condition.
Medium: Removing
sealedorforkedFromleaves stale metadatainternal/controller/roletemplate/roletemplate.go:204-220labelsMatchandannotationsMatchonly verify desired entries. Extra existing entries are ignored.Changing
sealedfrom true to false does not trigger removal of the existing sealed label. ClearingforkedFromsimilarly leaves the old annotation behind.Compare and reconcile the complete controller-owned metadata set.
Medium:
templateRefis mutableapi/v1alpha1/roleassignment_types.go:71-96Changing
templateRefsilently changes the granted role. Native Kubernetes bindings deliberately makeroleRefimmutable to prevent this class of privilege change.Add CRD validation making
templateRefimmutable. Users should replace the assignment to grant a different role.Testing Gaps
Missing regression coverage includes:
sealed: true.BLOCKER-1 — isPlatformCaller: replace suffix heuristic with exact full-SA username match. Added PlatformServiceAccount string field to WebhookConfig; any caller whose req.UserInfo.Username exactly matches the configured value is treated as the platform identity. Empty string (default) disables the bypass entirely — no caller is platform. Wire --platform-service-account CLI flag in main.go; set it to system:serviceaccount:<ns>:rbac-controller-manager in the Helm Deployment via webhook.platformServiceAccount values key. BLOCKER-2 — Denial formatting: replace bare admission.Denied(formatString) with admission.Denied(fmt.Sprintf(formatString, args...)). Replace json.Marshal itoa with strconv.Itoa. Add handlers_test.go (14 table-driven tests) covering all Handle() branches including suffix-spoofing regression, format-string regression, empty-SA bypass-disabled, and subject-index interpolation verification. BLOCKER-3 — Webhook TLS provisioning: - Add deploy/charts/rbac-controller/templates/webhook-cert.yaml: cert-manager self-signed Issuer + Certificate (conditional on webhook.tls.certManager.enabled, default true); secret name derived from webhookCertSecretName helper. - Update ValidatingWebhookConfiguration: add cert-manager.io/inject-ca-from annotation when certManager.enabled=true so CA-injector keeps caBundle in sync; fall back to manual caBundle when certManager disabled. - Update deployment.yaml: add webhook-certs Secret volumeMount at certDir path, pass --webhook-cert-dir and --webhook-port args, and --platform-service-account. - Add webhookCertSecretName helper to _helpers.tpl; support existingSecret override. - Update values.yaml: add webhook.tls.certManager.{enabled,duration,renewBefore} and webhook.tls.existingSecret; add webhook.platformServiceAccount. BLOCKER-4 — Revoke-first error propagation: replace l.Error(err, ...) log-and- continue with return reconcile.Result{}, fmt.Errorf(..., err) so that a transient CRB deletion failure causes a retry rather than a ClusterRole update with stale CRBs still in place. Add TestReconcile_RevokeFirstError_BlocksClusterRoleUpdate: uses interceptor.Funcs to inject a Delete error on ClusterRoleBinding and asserts (a) reconciler returns a wrapped error, (b) ClusterRole rules are NOT updated, (c) stale CRB still exists. testutil: add NewClientWithInterceptor helper (wraps fake.WithInterceptorFuncs). Test count: 42 → 70 (28 new tests across webhook/handler/roletemplate suites). Ref: IPCEICIS-9690BLOCKER — CEL cost budget fix: - PolicyRule: add MaxItems bounds on all arrays (apiGroups:20, resources:50, verbs:20, resourceNames:50); without bounds the Kubernetes CEL cost-budget estimator cannot assign finite cost to x-kubernetes-validations rules and rejects the CRD/APIResourceSchema with 'CEL cost budget exceeded' - RoleTemplateSpec.rules: add MaxItems=50 - DisplayName/Description: add MaxLength=256/2048 respectively - RoleAssignment: subjects MaxItems=100, templateRef MaxLength=253 (DNS limit), Subject.Name MaxLength=512 - Regenerate config/crd/*.yaml, config/kcp/*.yaml (all three schemas), Helm chart apiresourceschema templates with MaxItems/MaxLength in every layer - Bump schemaNamePrefix s32df7c63f86e → scel50items001 (KCP schema objects are immutable; old schemas orphaned, delete manually after APIExport migration) Schema regression test — internal/schematest/schema_test.go: - Reads generated CRD YAML files at test time and asserts maxItems on spec.rules, PolicyRule.{apiGroups,resources,verbs,resourceNames}, subjects, maxLength on templateRef, displayName, description, and CEL rule count ≥4 - Catches generation drift (controller-gen without markers) and the CEL cost-budget regression before reaching the API server KCP VWC placement fix (observation resolved): - REMOVE ValidatingWebhookConfiguration from rbac-controller chart (management-cluster VWC does NOT protect KCP virtual workspace traffic; deploying it there was misleading and non-functional for APIExport resources) - ADD ValidatingWebhookConfiguration to rbac-controller-kcp chart, deployed in the KCP provider workspace (root:controllers:rbac). KCP's apis.kcp.io/ValidatingWebhook admission plugin dispatches to VWC objects in the provider workspace for virtual workspace requests — this is the correct and only effective placement. - KCP VWC uses url-based clientConfig pointing to the management-cluster webhook Service (accessible same-cluster via svc.cluster.local), not service-reference (which would resolve in the management-cluster apiserver, not KCP). Configurable via webhook.serviceURL in kcp chart values. - webhook.caBundle must be set manually (cert-manager CA-injection unavailable in KCP workspace); instructions added to values.yaml. - When webhook.serviceURL is empty, the template renders a prominent comment warning that admission is inactive rather than a silently-broken VWC. - rbac-controller chart retains cert-manager cert/service/deployment webhook wiring (the server still runs there); only the VWC is moved. Minor: fix ValidateRoleName doc comment (was 'ValidateRuleName'/'callerIsplatform') Test evidence: - go test ./... -race: 68 tests, all PASS - go build, go vet: clean - helm lint both charts: 0 failures - helm template kcp --set webhook.serviceURL=https://...: VWC rendered with url + caBundle and failurePolicy:Fail - helm template kcp (no serviceURL): comment-only output, no broken VWC object - schema_test.go: all three assertions PASS (maxItems present in CRD+KCP yaml) Ref: IPCEICIS-9690Finding 1 — --ceiling-config crash on empty rules: - deployment.yaml: gate --ceiling-config flag on ceiling.rules (not ceiling.configPath). When ceiling.rules=[] (default), no flag is emitted, no ConfigMap is created, no volume is mounted → controller starts fail-closed without a missing-file crash. Previously configPath was always non-empty so the flag was always emitted even when no ConfigMap/volume existed. - Add 3 Helm template regression tests (internal/charttest/chart_test.go): TestHelmTemplate_EmptyCeiling_NoCrash: asserts no --ceiling-config + no volume TestHelmTemplate_NonEmptyCeiling_AllPresent: asserts flag + ConfigMap + volume TestHelmTemplate_ManageVWC_ClusterRole_ExactRules: asserts no delete/wildcard, resourceNames present, correct verbs (get/update/patch/create) Finding 2 — platform ceiling bypass removed (webhook/reconciler parity): - handlers.go: remove if !callerIsPlatform guard around ValidateCeiling; the ceiling now applies to ALL callers including the platform controller. v1alpha1 design: ceiling must cover platform preset rules; an empty ceiling denies everyone. This eliminates the incoherence where the webhook admitted platform writes that the controller's defence-in-depth would then reject. - Only retained platform-specific allowance: exact PlatformUsername match bypasses the reserved-name-prefix guard (can create edge-connect-rt-* names). Hard denials and ceiling apply uniformly. - policy.go: update package doc and WebhookConfig.Ceiling comment to reflect no ceiling bypass; operators must include platform preset rules in the ceiling. - handlers_test.go: TestRTValidator_Platform_BypassesCeiling renamed to TestRTValidator_Platform_AlsoSubjectToCeiling; now asserts DENIED for platform rule outside ceiling (parity test). Added TestRTValidator_Platform_AllowedWhenRuleInCeiling: platform + in-ceiling rule + reserved name → ALLOWED (name bypass still works). Finding 3 — manage-vwc ClusterRole comment accuracy: - clusterrole-content.yaml: replace misleading comment with accurate explanation: Rule 1 (scoped): get/update/patch on resourceNames=[vwcName] — resourceNames scoping works for these verbs. Rule 2 (unscoped create): minimum unavoidable — resourceNames cannot filter create because the object has no name at create time. Documents risk bound: controller only ever creates one named VWC. Removed 'create' from the scoped rule (it was duplicated in both rules; the scoped rule now has only get/update/patch). 93 tests pass — go test ./... -race clean, gofmt clean, go vet clean, helm lint both charts clean, all 3 chart regression tests PASS (not SKIP). Ref: IPCEICIS-9690feat: add RoleTemplate and RoleAssignment RBAC controller prototypeto feat(rbac): implement hardened v1alpha1 RoleTemplate/RoleAssignment with admission, ceiling, and VWC bootstrap2b59caa016toc803d29e09c803d29e09fd17b13fedView command line instructions
Manual merge helper
Use this merge commit message when completing the merge manually.
Checkout
From your project repository, check out a new branch and test the changes.Merge
Merge the changes and update on Forgejo.Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.