Repository navigation
Vcsim authz fidelity - #4138
Draft
hickeng wants to merge 4 commits into
Draft
Vcsim authz fidelity#4138hickeng wants to merge 4 commits into
hickeng wants to merge 4 commits into
Conversation
Symptom: object.AuthorizationManager.AddRole against vcsim always returned role id 0, because the AddAuthorizationRole response carried no Returnval. Callers that record the id to grant or later update the role got an id that matched no role (or the first custom role). Fix: set Returnval to the id assigned to the new role. Custom role ids now start at 1, or after the highest id already in RoleList when a model is loaded with custom roles, so an id is never 0 (the zero value callers see on error) and never collides with an existing role. The built-in roles keep their negative ids. Verified: TestAuthorizationManagerAddRole fails before the change (id=0 for every role) and passes after. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: George Hicken <george.hicken@broadcom.com>
Symptom: SetEntityPermissions replaced the entity's whole permission list with the request's. Granting one principal a role on the root folder erased the default Admin permissions there, and each later grant on the same entity erased the one before it. vCenter instead adds each permission, or updates the rule already present for the same user or group on the entity. Fix: match each requested permission on (principal, group) against the entity's list, replacing a match and appending otherwise. Stored permissions also carry the entity they were set on, as the default root folder permissions already do and as vCenter returns them. Verified: TestAuthorizationManagerSetEntityPermissions fails before the change (default and earlier permissions removed) and passes after. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: George Hicken <george.hicken@broadcom.com>
Symptom: AddAuthorizationRole and UpdateAuthorizationRole fault with InvalidArgument on privIds for ManagementServiceAccessGrants.Configure and ManagementServices.Configure, which vCenter defines and which the vSphere Supervisor observability role requests. vcsim only accepts privileges in its Admin role, and the catalogue lacked these two. Fix: add both privileges to Admin, and to NoTrustedAdmin and NoCryptoAdmin, which hold every Admin privilege outside their excluded groups. Verified: TestAuthorizationManagerAddRolePrivileges fails before the change (InvalidArgument) and passes after. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: George Hicken <george.hicken@broadcom.com>
Symptom: AddAuthorizationRole faults with InvalidArgument on privIds for the vSphere Supervisor namespace operator's RBAC controller role, which requests Namespaces.View, Namespaces.Edit and Namespaces.Owner. vCenter defines these, with Namespaces.Observe, IPAM.Edit, Namespaces.ConfigureAdmissionPolicy and Namespaces.ViewAdmissionPolicy, when it registers the Supervisor RBAC operator's privileges. vcsim only accepts privileges in its Admin role, and the catalogue lacked all seven. Fix: add the seven privileges to Admin, and to NoTrustedAdmin and NoCryptoAdmin, which hold every Admin privilege outside their excluded groups. Verified: TestAuthorizationManagerAddRolePrivileges fails before the change (InvalidArgument) and passes after. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: George Hicken <george.hicken@broadcom.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Three AuthorizationManager fidelity fixes, found while simulating a vSphere solution that creates its own roles and grants them on inventory objects:
AddAuthorizationRolenow returns the new role's id (Returnvalwas never set, so every created role read as id 0). Custom role ids start at 1, or after the highest id already in the model, so they cannot collide with the built-in negative ids.SetEntityPermissionsnow upserts by (principal, group) instead of replacing the entity's whole permission list, matching vCenter's documented behaviour ("defines permission rules or updates rules already present for the user or group"). Previously, granting one principal on the root folder erased the default admin permissions. Stored permissions also record their entity, as the default root permissions already did.ManagementServiceAccessGrants.ConfigureandManagementServices.Configureto the Admin, NoTrustedAdmin and NoCryptoAdmin privilege lists, so roles that use them can be created.Closes: n/a (no upstream issue yet)
How Has This Been Tested?
simulator/authorization_manager_test.go:TestAuthorizationManagerAddRole,TestAuthorizationManagerSetEntityPermissions,TestAuthorizationManagerAddRolePrivileges. Each fails before its fix and passes after.go test ./simulator/... ./object/...passes;make lintreports 0 issues.govc/test/role.batswas not run (no bats locally); by reading, its permissions test still holds under upsert.