feat: limit controller access to control plane secrets #2522
Conversation
✅ Deploy Preview for docs-kargo-akuity-io ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
@fykaa, this looks like a good start, but as you know, we've encountered some scope creep that is described in detail here: #2448 (comment) |
fykaa
force-pushed
the
limit-controller-access
branch
from
c19fb35
to
2e2590b
Compare
| ctx context.Context, | ||
| project *kargoapi.Project, | ||
| ) error { | ||
| const roleBindingName = "kargo-controller-secrets-readonly" |
There was a problem hiding this comment.
Interesting. In my head, each Project was going to have a RoleBinding per controller SA.
You're actually creating or updating a single RoleBinding and making sure it has all the controller SAs listed as its subjects.
On some level, I quite like that, but I have a worry this will be more complicated than anticipated once you get into part two of this PR...
Remember that we also need to deal with creating/removing bindings as new controller SAs come and go. Once we start doing that, you start to run into possibilities like a new Project and a new SA being reconciled concurrently and racing to update the same binding.
One tempting solution to that problem is to patch the RoleBinding instead of updating it, but patching lists can be quite difficult.
In the end, it may be more straightforward if every Project has one RoleBinding per controller SA. I feel that would lead to fewer conflicts and race conditions.
But I also want @hiddeco's opinion on this matter.
There was a problem hiding this comment.
Note that apart from this, this PR is off to a very good start.
There was a problem hiding this comment.
Thank you for your detailed feedback! You raised some excellent points that I hadn’t fully considered, particularly around the potential for race conditions and the implications of managing rolebinding for multiple SAs.
As it stands, the implementation creates or updates a single RoleBinding that grants read-only access to Secrets for all controller ServiceAccounts associated with a project. This approach was straightforward for me initially, but I now see that it could lead to complications, especially when new controller SAs are added or existing ones are removed (I am yet to add the SA reconciler package)
I need to explore the approach you mentioned regarding patching the RoleBinding (instead of updating it).
As of now, one RoleBinding per controller ServiceAccount may simplify the management of permissions and reduce the risk of conflicts. I guess I need to understand the architecture better, that would enable me to propose a more robust solution that aligns with our overall design.
There was a problem hiding this comment.
Let's wait to hear from @hiddeco before making any changes. He usually has the best insight into avoiding races in controllers.
There was a problem hiding this comment.
As the list of subjects is a list of primitives, there is no support for merging them in Kubernetes. Besides this, even if there would be support for it, we would not be in control over defining this behavior as it is part of the CustomResourceDefinition (of which we are not the author) using patchMergeKey.
Given this, there are only a few options to avoid any concurrency issues:
- You put one reconciler in control of the reconciliation of the RoleBinding, while making it react to both Project and ServiceAccount changes (using carefully crafted watches). This way, any race condition is eliminated, as any change of interest will trigger a new observation taking the whole state of the world into account.
- You issue a patch in combination with an optimistic lock. When this results in a conflict (due to A winning from B), you retry it by re-fetching the resources and all the bits you are interested in, to then attempt to make the change again.
The main issue with option 2 is that both reconcilers only update the bits they care about, causing a potential blind spot. This means each reconciler lacks full visibility into changes made by the other, potentially still leading to unintended overwrites or inconsistent states in the RoleBinding. This is why a single reconciler with a holistic view (as in option 1) would be the more reliable option.
There was a problem hiding this comment.
You put one reconciler in control of the reconciliation of the RoleBinding
ikd if this is a viable option. It's not the RoleBindings that need to be reconciled.
RoleBindings need to be updated or created/removed in response to both Projects and controller SAs being added/removed.
There was a problem hiding this comment.
You can create a reconciler around an arbitrary resource type since controller-runtime v0.19.0. See:
There was a problem hiding this comment.
To confirm:
Are we aiming to create a reconciler specifically for RoleBindings that would react to changes in Projects and ServiceAccounts (the option 1)?
There was a problem hiding this comment.
To avoid race conditions: we'll go with creating one RoleBinding per Service Account. This way, each SA gets its own RoleBinding in the Project namespace.
(also solve the problem: merging a list of primitives/ subjects in k8s).
This is simple, with more objects to manage, but less complexity and potential races.
fykaa
force-pushed
the
limit-controller-access
branch
from
108545e
to
260b09c
Compare
Signed-off-by: Faeka Ansari <[email protected]>
…cess to secrets in project ns Signed-off-by: Faeka Ansari <[email protected]>
fykaa
force-pushed
the
limit-controller-access
branch
from
260b09c
to
cfc3135
Compare
✅ Deploy Preview for docs-kargo-io canceled.
|
Signed-off-by: Faeka Ansari <[email protected]>
Signed-off-by: Faeka Ansari <[email protected]>
|
This PR is yet to hit the ground running with some tests and refactoring logic. RoleBinding Creation logic is completed. |
Signed-off-by: Faeka Ansari <[email protected]>
Signed-off-by: Faeka Ansari <[email protected]>
Signed-off-by: Faeka Ansari <[email protected]>
internal/controller/management/serviceaccounts/serviceaccounts_test.go
Outdated
Show resolved
Hide resolved
Signed-off-by: Faeka Ansari <[email protected]>
fykaa
force-pushed
the
limit-controller-access
branch
from
14a2ebe
to
2ecd090
Compare
internal/controller/management/serviceaccounts/serviceaccounts.go
Outdated
Show resolved
Hide resolved
internal/controller/management/serviceaccounts/serviceaccounts.go
Outdated
Show resolved
Hide resolved
internal/controller/management/serviceaccounts/serviceaccounts.go
Outdated
Show resolved
Hide resolved
internal/controller/management/serviceaccounts/serviceaccounts.go
Outdated
Show resolved
Hide resolved
internal/controller/management/serviceaccounts/serviceaccounts.go
Outdated
Show resolved
Hide resolved
Signed-off-by: Faeka Ansari <[email protected]> fix nits Signed-off-by: Faeka Ansari <[email protected]>
fykaa
force-pushed
the
limit-controller-access
branch
from
d417495
to
f57983a
Compare
|
Closing in favor of #2761 |
Issue Reference:
Closes: #2448
This PR limits the
kargo-controller's access to project-specific secrets by creating aClusterRole(kargo-controller-secrets-readonly) with read-only permissions. Role bindings are dynamically created per project and removed upon project deletion and this ensures least-privileged access for controllers.Impact:
Tighter scoping of controller access to secrets, more security for sharded controllers.