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

[RFC] Access control for cross-namespace source refs #2092

Closed
wants to merge 4 commits into from

Conversation

stefanprodan
Copy link
Member

@stefanprodan stefanprodan commented Nov 16, 2021

This RFC proposes a way to share sources between namespaces when running Flux on multi-tenant clusters.

Supersedes #582

@stefanprodan stefanprodan added the area/rfc Feature request proposals in the RFC format label Nov 16, 2021
@vterdunov
Copy link
Contributor

vterdunov commented Nov 21, 2021

@stefanprodan I'd like to say the problem that I see in the ACL proposal and the user story 3. As a tenant somehow I need to know each GitRepository name+namespace allowed to use by me. It's a bit implicit for me. However ACL can solve my problem, when I have a tenant wich 10+ namespaces. I would place a GitRepository with ACLs to one of the tenant namespace. E.g.: tenant-name-system.
On the other hand, the SourceReflection looks more explicitly, however, I don't know how to properly prevent creating SourceReflections resources by one tenant in another tenant namespace.

Another implicit behavior for me is an implicit reference from ImageUpdateAutomation to ImagePolicy.

rfcs/0002-source-acl/README.md Show resolved Hide resolved
rfcs/0002-source-acl/README.md Show resolved Hide resolved
Yes another alternative is to introduce a new API kind `SourceReflection` as described in
[fluxcd/flux2#582-821027543](https://github.com/fluxcd/flux2/pull/582#issuecomment-821027543).

And finally, this is more of an improvement of the current proposal, is for source-controller to compile the ACLs
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current implementation of ACLs is not described in detail in this current doc, so it is a bit unclear to me what the current gap is between improving the current implementation and the implementation described in this paragraph.

I understand that the discussion might be listed elsewhere, but it would be good in one of these RFCs to explicitly lay out what the controller does and what service account and permissions it runs under when attempting to check the ACL

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The RFC-0001 may fill that gap as it defines the current authentication model.

@jonathan-innis
Copy link
Contributor

I think the primary issue I see with ACLs is understanding what is available to you in terms of sources. As much as I like the idea of having some sort of CLI support for grabbing the available sources, I cannot think of a good way to show the available sources in a secure way to the tenant user

> As a cluster admin, I want to let tenants configure image automation in their namespaces by
> referring to a Git repository managed by the platform team.

If the owner of a Flux `GitRepository` wants to grant write access to `ImageUpdateAutomations` in a different namespace,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be good to lay out the list of possible access... If as a user I grant access to another namespace, depending on the running controllers on that NS I might re-consider.

Copy link
Member Author

@stefanprodan stefanprodan Nov 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The level of access to the underlying Git repository depends on how you've configured the SSH key or token, if it has write access to just read. ImageUpdateAutomations need write access to patch image tags, while a HelmRelease or Kustomization needs only read access.

@stefanprodan
Copy link
Member Author

stefanprodan commented Nov 23, 2021

I think the primary issue I see with ACLs is understanding what is available to you in terms of sources. As much as I like the idea of having some sort of CLI support for grabbing the available sources, I cannot think of a good way to show the available sources in a secure way to the tenant user

ACLs are very much like network polices. As far as I know, there is no way for a tenant to query Kubernetes API and get a list of endpoints that allow ingress traffic from its namespace.

Copy link
Contributor

@jonathan-innis jonathan-innis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 from Microsoft 🚀

@pjbgf
Copy link
Member

pjbgf commented Dec 21, 2021

I think the primary issue I see with ACLs is understanding what is available to you in terms of sources. As much as I like the idea of having some sort of CLI support for grabbing the available sources, I cannot think of a good way to show the available sources in a secure way to the tenant user

A way to achieve this could be by a platform admin leveraging the built-in RBAC, having a pre-agreed namespace (say flux-shared) and provide list permissions to all sources that are available to a given tenant. Effectively allowing them to query them via kubectl. But it would be the platform admin's responsibility to ensure both RBAC and ACL permissions are in-sync.

Copy link
Member

@pjbgf pjbgf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A potential alternative approach to rely solely on the built-in RBAC instead of creating a new authorisation mechanism, is to leverage the use verb. Below is a simple example that would provide a service account (to be used by Helm/Kustomization objects) the ability to use all sources from namespace flux-shared. It also gives specific-tenant-user the ability to query (list/get) all such resources:

apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRole
metadata:
  name: flux-use-sources
rules:
- apiGroups: ['source.toolkit.fluxcd.io']
  resources:
  - bucket
  - gitrepository
  - helmchart
  - helmrepository
  verbs:
  - use
  - list
  - get
---
apiVersion: rbac.authorization.k8s.io/v1
kind: RoleBinding
metadata:
  name: flux-shared-resources
  namespace: flux-shared
roleRef:
  apiGroup: rbac.authorization.k8s.io
  kind: ClusterRole
  name: flux-use-sources
subjects:
- kind: ServiceAccount
  name: flux
  namespace: specific-tenant
- apiGroup: rbac.authorization.k8s.io
  kind: User
  name: specific-tenant-user

It is not as flexible as the proposed solution, but it requires less effort to implement and delays the need to change flux's API. The main impact being that users won't be able to use selectors, but instead will have to create RoleBinding and ClusterRoleBinding objects to enable the use of flux sources.

The main benefit being that the built-in RBAC would still be the single point of truth for authorisation of flux objects.

Yes another alternative is to introduce a new API kind `SourceReflection` as described in
[fluxcd/flux2#582-821027543](https://github.com/fluxcd/flux2/pull/582#issuecomment-821027543).

And finally, this is more of an improvement of the current proposal, is for source-controller to compile the ACLs
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The RFC-0001 may fill that gap as it defines the current authentication model.

Comment on lines +118 to +117
accessFrom:
namespaceSelectors:
- matchLabels:
kubernetes.io/metadata.name: dev-team1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pattern is very flexible for resource discovery and I can easily understand the benefits of using it in this use case. The main challenge I can think of using this as part of flux's authorisation model in addition to the built-in RBAC is that a cluster admin who is unaware of flux's inner workings may be blindsided by this behaviour, leading to less secure flux deployments.

Given that RBAC will stop being the single source of truth from an authorisation perspective, it may be useful to also provide a sub-command on flux to allow admins to inspect effective permissions across a cluster.

Copy link
Contributor

@darkowlzz darkowlzz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

rfcs/0002-source-acl/README.md Outdated Show resolved Hide resolved
@stefanprodan stefanprodan changed the title [RFC-0002] Access control for cross-namespace source refs [RFC] Access control for cross-namespace source refs Apr 12, 2022
@stefanprodan stefanprodan marked this pull request as draft April 12, 2022 12:00
@pjbgf pjbgf mentioned this pull request Apr 20, 2022
4 tasks
@stealthybox
Copy link
Member

I think the primary issue I see with ACLs is understanding what is available to you in terms of sources. As much as I like the idea of having some sort of CLI support for grabbing the available sources, I cannot think of a good way to show the available sources in a secure way to the tenant user

ACLs are very much like network polices. As far as I know, there is no way for a tenant to query Kubernetes API and get a list of endpoints that allow ingress traffic from its namespace.

@jonathan-innis @stefanprodan

Should we move forward with ACL's, implementing the ACL's via intermediary RoleBindings and relying on User Impersonation to enforce the policy will have the benefit of making the list of allowed Sources available via the kubectl/flux CLI as long as the CLI user also has access to impersonate the reconcile User.

This is another great reason to not build our own authorization mechanism.
Since the policy is recorded and enforced by the kube-apiserver, all clients that request the resource can be constrained to the proper rules.

example:

kubectl auth can-i get gitrepository \
  --all-namespaces --as system:serviceaccount:flux-system:reconciler

The apiserver is happy to tell you via RBAC whether or not a particular User can access resources, either generally or by name in whichever namespace.

@stefanprodan
Copy link
Member Author

@stealthybox we currently offer a way to disable cross-ns refs and enforce a default SA on multi-tenant environments https://fluxcd.io/docs/installation/#multi-tenancy-lockdown

We’ve reached out to TAG Security for guidance on ACLs. IMO ACLs made like network polices are way easier to setup than RBAC.

@phillebaba
Copy link
Member

phillebaba commented May 22, 2022

@stefanprodan I think that an important feature for this should be an additional CLI command to to make it clear which namespaces can access what resources. Generally the most difficult part of network policies is debugging. Other than that I think this looks good.

@stefanprodan
Copy link
Member Author

stefanprodan commented Mar 8, 2023

We should investigate if ReferenceGrant is a better alternative to the ACL.

@stefanprodan
Copy link
Member Author

Closing this with the resolution: Adopt ReferenceGrant once SIG Auth API Group promotes the API to GA.

@Sturgelose
Copy link

@stefanprodan, sorry to write in this closed PR, but, I'm understanding that until ReferenceGrant doesn't go GA by the SIG Auth API Group, FluxCD community won't do any movement in order to support cross-namespace support?

I'm asking because there is very little movement in the KEP, and seems this could go for long. I do not know if FluxCD community could provide at least some sort of feature to allow people to get the cross-namespace functionality (and its huge value) right now, with a big disclaimer that once it is supported by K8s core, it will be migrated to it.

From what I see, the API the SIG will implement seems pretty clear and in use in the gateway-api SIG, from where they want to export it from, so even Flux could already work knowing which will how the final implementation might look like.

Of course I understand that making work to have it deprecated soon(ish) has little value, but seeing how things are moving might make sense to invest the time, specially when there are some PRs around already with lots of dicussion or close to have the feature/value implemented.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/rfc Feature request proposals in the RFC format
Projects
None yet
Development

Successfully merging this pull request may close these issues.