generated from AMRC-FactoryPlus/acs-template
-
Notifications
You must be signed in to change notification settings - Fork 6
Granular ConfigDB object permissions #559
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Draft
amrc-benmorrow
wants to merge
19
commits into
main
Choose a base branch
from
bmz/cdb-perms
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Conversation
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
The previous convention with underscores is from a very long time ago and is inconsistent with everything else.
Far too much was handled by a blanket ManageObj permission. Create more granular permissions. Permissions for editing relations come in pairs; the client must have permission for both ends of the relation. A grant with a structured target would be better here as it would be more specific, but we don't have those in the Auth service yet. Disable some compat endpoints that are no longer useful. They now always return 403.
When a dump loads grants for a given principal, it specifies the full set of grants for that principal. All other existing grants are removed. This means it's not possible to grant new permissions to users and groups which are created by ACS but I think that can be worked around.
If we are going to remove existing grants then the permission checks for loading a dump depend on the existing data; this means they must be performed in the model. We still have a race condition between freezing our permitted lists and the DB transaction but I can't see how to avoid that.
These have been replaced with more restricted grants where the service account can now only manipulate memberships of its own objects. This means we need a fixup step to find and reset membership on the appropriate objects. Auth dumps are now authoritative about the permissions granted to principals they mention, so each prinicipal must have all its permissions set in one place.
The new permission grants only give the krbkeys and cluster manager permission to manipulate their own objects. This means we need to set ownership on existing objects which were created by those services.
The new API appears to be different.
We have no good reason to allow cross-realm principals here.
Loading a dump now needs lots of permissions, but that's expected. The relations table is needed from the notify interface as well; this has actually removed a lot of the duplication between the HTTP and notify endpoints.
An Edge Agent is created by a human user, and then the edge krbkeys needs to place it in the correct groups. For now grant all edge krbkeys access to write memberships of all Edge Agents; this is still more restricted than before. Ideally we want a class of agents per cluster and an individual grant for each edge krbkeys.
This is even broader, in principle, because it will be cross-deployment (if we ever get those links). But the only initial classification of a new Edge Agent is Auth.Class.EdgeAgent: the Active role is added and removed by krbkeys.
On an initial install these may not exist yet.
This isn't used by ACS as such but we use it for deploying cross-realm accounts.
As before, these objects are not created by the cluster manager but by an admin. Part of the problem here is we are using the same object to represent the cluster as a whole and the Git repo controlling it; the classification here is of the repo. If they were separate objects then the cluster manager could own the repo object; but then we would need to be able to record the association to the cluster.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This has not been fully tested and is not ready for merge. Also it should be rebased onto main.
Currently the Manage objects ConfigDB permission covers a large range of actions. This is because the object model has been made richer but the permission structure has not been expanded to cover new actions.
Divide the Manage objects permission into finer-grained permissions. Ensure appropriate permissions are granted to appropriate services. This requires use of ownership and Mine permissions, so a fixup step is needed to reset ownership on some existing objects.