Skip to content
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

Prevent unintended usage of "replace" #14161

Open
mattenklicker opened this issue Jun 21, 2023 · 18 comments · May be fixed by #20720
Open

Prevent unintended usage of "replace" #14161

mattenklicker opened this issue Jun 21, 2023 · 18 comments · May be fixed by #20720
Labels
enhancement New feature or request

Comments

@mattenklicker
Copy link

Summary

Make it possible to prevent the unintended usage of "replace" in UI sync options.

Motivation

To update resources by "kubectl replace" is dangerous under certain conditions. It already has a warning sign, but some might want to disable the usage completely. See #9767 also. It is very present in UI sync options and unfortunately it is likely impossible to block "replace" through kubernetes RBAC or other measures like OPA.

Proposal

  • Create config flag to make the "replace" box grey out or disappear in UI sync options
  • Alternatively: Make it a RBAC control.
@mattenklicker mattenklicker added the enhancement New feature or request label Jun 21, 2023
@fhopfensperger
Copy link
Contributor

fhopfensperger commented Jul 14, 2023

I am also looking for something like this. It would be nice to be able to customize the sync options per application.
image

@pranchals
Copy link

pranchals commented Feb 26, 2024

Implementing RBAC for Sync Options would be good addition and will enforce stricter access controls and define precise permissions with regards synchronisation actions within an Argo CD Application.

Is there any update on this being implemented ?

@conman2305
Copy link

I'll +1 this - I want to give users the ability to sync their apps, but doing so currently also means they can totally wipe it out. It'd be nice to implement fine-grained controls for the sync permission like there is for update and delete: https://argo-cd.readthedocs.io/en/stable/operator-manual/rbac/#fine-grained-permissions-for-updatedelete-action

@jeremyrajan
Copy link

Is there a way around it? Can I give sync permissions but don't allow replace?

@todaywasawesome
Copy link
Contributor

@jeremyrajan not today

@andrii-korotkov-verkada
Copy link
Contributor

I'm planning to work on this an implement an option in the config which allows to hide the button. UI would query that data from the backend and not render a button at all if configured so.

@andrii-korotkov-verkada
Copy link
Contributor

Extra UI friction may not be as helpful, since people may ignore it anyways as they can ignore the warning icon near the checkbox.

@andrii-korotkov-verkada
Copy link
Contributor

@todaywasawesome, which configmap is best for putting the option in? Is it argocd-cm or some other?

andrii-korotkov-verkada added a commit to andrii-korotkov-verkada/argo-cd that referenced this issue Nov 1, 2024
Tell more about possible practical implications of sync with replace such as pods scaling down to min replicas.

Signed-off-by: Andrii Korotkov <[email protected]>
andrii-korotkov-verkada added a commit to andrii-korotkov-verkada/argo-cd that referenced this issue Nov 1, 2024
Helps with argoproj#14161

Tell more about possible practical implications of sync with replace such as pods scaling down to min replicas.

Signed-off-by: Andrii Korotkov <[email protected]>
andrii-korotkov-verkada added a commit to andrii-korotkov-verkada/argo-cd that referenced this issue Nov 1, 2024
Helps with argoproj#14161

Tell more about possible practical implications of sync with replace such as pods scaling down to min replicas.

Signed-off-by: Andrii Korotkov <[email protected]>
@andrii-korotkov-verkada
Copy link
Contributor

Seems like RBAC already has a support for rules about resource deletion https://argo-cd.readthedocs.io/en/stable/operator-manual/rbac/#fine-grained-permissions-for-updatedelete-action. Somebody brought this up during the contributors meeting, that checking for resource deletion permission can be done. Given possibility to customize like above, I think it will solve the problem.

@andrii-korotkov-verkada
Copy link
Contributor

@todaywasawesome, @crenshaw-dev, @nitishfy, @agaudreault, do you know where's the code which checks for sync permissions, i.e. if the rbac allows to sync an application? I've tried to check in SyncAppState, but only found checks for group/kind and destination.

@andrii-korotkov-verkada
Copy link
Contributor

Btw, does sync with prune checks for deletion permissions? If not, should it be added as well?

keithchong pushed a commit that referenced this issue Nov 7, 2024
Helps with #14161

Tell more about possible practical implications of sync with replace such as pods scaling down to min replicas.

Signed-off-by: Andrii Korotkov <[email protected]>
andrii-korotkov-verkada added a commit to andrii-korotkov-verkada/argo-cd that referenced this issue Nov 8, 2024
…proj#14161)

Closes argoproj#14161

Sync with replace can be quite dangerous, but we currently allow it if the sync is allowed. Since sync with replace deletes resources and recreates them, it's reasonable to check deletion permissions for application and/or specific resources.

Signed-off-by: Andrii Korotkov <[email protected]>
andrii-korotkov-verkada added a commit to andrii-korotkov-verkada/argo-cd that referenced this issue Nov 8, 2024
…proj#14161)

Closes argoproj#14161

Sync with replace can be quite dangerous, but we currently allow it if the sync is allowed. Since sync with replace deletes resources and recreates them, it's reasonable to check deletion permissions for application and/or specific resources.

Let's cherry-pick this to 2.13.

Signed-off-by: Andrii Korotkov <[email protected]>
andrii-korotkov-verkada added a commit to andrii-korotkov-verkada/argo-cd that referenced this issue Nov 8, 2024
…proj#14161)

Closes argoproj#14161

Sync with replace can be quite dangerous, but we currently allow it if the sync is allowed. Since sync with replace deletes resources and recreates them, it's reasonable to check deletion permissions for application and/or specific resources.

Let's cherry-pick this to 2.13.

Signed-off-by: Andrii Korotkov <[email protected]>
@crenshaw-dev
Copy link
Member

I think the proposal needs to be more clearly/specifically defined. For example, I think an RBAC-based system checking for delete permission has some problems we'd need to overcome: #20720 (review)

andrii-korotkov-verkada added a commit to andrii-korotkov-verkada/argo-cd that referenced this issue Nov 8, 2024
…proj#14161)

Closes argoproj#14161

Sync with replace can be quite dangerous, but we currently allow it if the sync is allowed. Since sync with replace deletes resources and recreates them, it's reasonable to check deletion permissions for application and/or specific resources.

Let's cherry-pick this to 2.13.

Signed-off-by: Andrii Korotkov <[email protected]>
andrii-korotkov-verkada added a commit to andrii-korotkov-verkada/argo-cd that referenced this issue Nov 8, 2024
…proj#14161)

Closes argoproj#14161

Sync with replace can be quite dangerous, but we currently allow it if the sync is allowed. Since sync with replace deletes resources and recreates them, it's reasonable to check deletion permissions for application and/or specific resources.

Let's cherry-pick this to 2.13.

Signed-off-by: Andrii Korotkov <[email protected]>
andrii-korotkov-verkada added a commit to andrii-korotkov-verkada/argo-cd that referenced this issue Nov 8, 2024
…proj#14161)

Closes argoproj#14161

Sync with replace can be quite dangerous, but we currently allow it if the sync is allowed. Since sync with replace deletes resources and recreates them, it's reasonable to check deletion permissions for application and/or specific resources.

Let's cherry-pick this to 2.13.

Signed-off-by: Andrii Korotkov <[email protected]>
andrii-korotkov-verkada added a commit to andrii-korotkov-verkada/argo-cd that referenced this issue Nov 8, 2024
…proj#14161)

Closes argoproj#14161

Sync with replace can be quite dangerous, but we currently allow it if the sync is allowed. Since sync with replace deletes resources and recreates them, it's reasonable to check deletion permissions for application and/or specific resources.

Let's cherry-pick this to 2.13.

Signed-off-by: Andrii Korotkov <[email protected]>
andrii-korotkov-verkada added a commit to andrii-korotkov-verkada/argo-cd that referenced this issue Nov 8, 2024
…proj#14161)

Closes argoproj#14161

Sync with replace can be quite dangerous, but we currently allow it if the sync is allowed. Since sync with replace deletes resources and recreates them, it's reasonable to check deletion permissions for application and/or specific resources.

Let's cherry-pick this to 2.13.

Signed-off-by: Andrii Korotkov <[email protected]>
andrii-korotkov-verkada added a commit to andrii-korotkov-verkada/argo-cd that referenced this issue Nov 8, 2024
…proj#14161)

Closes argoproj#14161

Sync with replace can be quite dangerous, but we currently allow it if the sync is allowed. Since sync with replace deletes resources and recreates them, it's reasonable to check deletion permissions for application and/or specific resources.

Let's cherry-pick this to 2.13.

Signed-off-by: Andrii Korotkov <[email protected]>
andrii-korotkov-verkada added a commit to andrii-korotkov-verkada/argo-cd that referenced this issue Nov 8, 2024
…proj#14161)

Closes argoproj#14161

Sync with replace can be quite dangerous, but we currently allow it if the sync is allowed. Since sync with replace deletes resources and recreates them, it's reasonable to check deletion permissions for application and/or specific resources.

Let's cherry-pick this to 2.13.

Signed-off-by: Andrii Korotkov <[email protected]>
andrii-korotkov-verkada added a commit to andrii-korotkov-verkada/argo-cd that referenced this issue Nov 8, 2024
…proj#14161)

Closes argoproj#14161

Sync with replace can be quite dangerous, but we currently allow it if the sync is allowed. Since sync with replace deletes resources and recreates them, it's reasonable to check deletion permissions for application and/or specific resources.

Let's cherry-pick this to 2.13.

Signed-off-by: Andrii Korotkov <[email protected]>
@andrii-korotkov-verkada
Copy link
Contributor

andrii-korotkov-verkada commented Nov 8, 2024

@crenshaw-dev, @agaudreault, continuing the conversation here instead of the PR. To summarize:

The current proposal:

  • Partially use delete permissions RBAC to deny some of syncs with replace if the replace option is specified from UI/CLI.
  • Don't use these if the replace is only configured via application/resource annotations (i.e. leave as it is now).
  • If whole apps are allowed to be deleted, sync with replace is allowed.
  • If whole apps are not allowed to be deleted, resource list for sync is not provided and any resource is allowed to be deleted, sync with replace is allowed.
  • If whole apps are not allowed to be deleted, resource list for sync is provided and the corresponding resources are allowed to be deleted, sync with replace is allowed.
  • Otherwise, sync with replace is denied.
  • Use documentation to talk about this behavior.

Some caveats:

  • The approach doesn't go all the way of configuring sync with replace permissions and has some inconsistency, e.g. when using annotations for replace during sync.
  • Delete is not the same as replace or prune, so permissions for delete are under a higher scrutiny.
  • There can be cases of potential over-permissioning due to a new model.

Feel free to summarize the things I've missed.

@andrii-korotkov-verkada
Copy link
Contributor

Based on some other things discussed, maybe indeed coming up with a new permission action like prune as complementary for delete is good. This would apply to sync with replace and sync with prune, both full and partial.

@agaudreault
Copy link
Member

I think the solution with the least caveats would be to add sync-replace and sync-prune as new actions.

To support partial-sync, these actions could either be expressed with sync or sync/*, in the same fashion as the fine-grained delete/update.

So you would have

  1. Implement feature flag to turn on fine-grained sync. (Following items assume feature flag is on)
  2. When user triggers a basic app sync, permissions are validated on sync
  3. When user triggers a partial sync, permission on each resources are validated against sync/<resource>
  4. When user triggers a basic app sync with replace, permissions are validated on sync-replace
  5. When user triggers a partial sync with replace, permission on each resource is validated against sync-replace/<resource>
  6. When user triggers a basic app sync with prune, permissions are validated on sync-prune
  7. When user triggers a partial sync with prune, permission on each resources are validated against sync-prune/<resource>

This way there's only 1 rbac check done in the server based on the request options. If app or individual resources are configured with annotations, then it is not a user action and does not need to go through rbac.

@crenshaw-dev
Copy link
Member

Yep I think I agree with all the above.

But given the cognitive and maintenance costs of new RBAC verbs, and given that the use case seems primarily for accident avoidance rather than security constraints, I think we should seriously consider non-RBAC mitigations.

@andrii-korotkov-verkada
Copy link
Contributor

I like the idea of separate RBAC permission, though it'd add a bit of configuration and maintenance overhead.

Though people would still probably have to give broad sync with replace permissions, if they wanna be able to click a checkbox in UI for the whole app.

Application controller can also check RBAC when it knows the diff and actual resources to be replaced. But it'd be inconsistent with how server RBAC checks work (give immediate feedback).

If we configure on app or app project level, we need some granularity on resource types, since sometimes people would want replace to update immutable fields.

Where RBAC is making it easier is when you can configure some kind of power user permission allowing to do sync with replace, but make it not granted by default and be requestable through Opal for example (or whatever system is used).

@andrii-korotkov-verkada
Copy link
Contributor

There's a tricky part with the annotations approach. Allowing disabling auto sync via UI requires applications update permission turned on. But this is wider than desired, allowing people to add annotations they want. So it'd add friction, but won't prevent people from doing sync with replace.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
9 participants