From 6c3dc9e267fa4d6127b0edbad4d8ac4d271d3e35 Mon Sep 17 00:00:00 2001 From: Daniele Martinoli <86618610+dmartinol@users.noreply.github.com> Date: Tue, 15 Oct 2024 17:52:58 +0200 Subject: [PATCH] fix: Replaced ClusterRoles with local RoleBindings (#4625) * replaced fetching ClusterRoles with local RoleBindings Signed-off-by: Daniele Martinoli * added missing namespace configurations Signed-off-by: Daniele Martinoli --------- Signed-off-by: Daniele Martinoli --- .../components/authz_manager.md | 12 ++++- .../server/k8s/server_resources.yaml | 16 +++--- .../auth/kubernetes_token_parser.py | 50 +++++++++++-------- .../tests/unit/permissions/auth/conftest.py | 34 +++---------- .../permissions/auth/server/mock_utils.py | 14 +++--- .../permissions/auth/test_token_parser.py | 47 ++++++++--------- 6 files changed, 85 insertions(+), 88 deletions(-) diff --git a/docs/getting-started/components/authz_manager.md b/docs/getting-started/components/authz_manager.md index 20fcdca107..4014c697ef 100644 --- a/docs/getting-started/components/authz_manager.md +++ b/docs/getting-started/components/authz_manager.md @@ -114,4 +114,14 @@ auth: In case the client cannot run on the same cluster as the servers, the client token can be injected using the `LOCAL_K8S_TOKEN` environment variable on the client side. The value must refer to the token of a service account created on the servers cluster -and linked to the desired RBAC roles. \ No newline at end of file +and linked to the desired RBAC roles. + +#### Setting Up Kubernetes RBAC for Feast + +To ensure the Kubernetes RBAC environment aligns with Feast's RBAC configuration, follow these guidelines: +* The roles defined in Feast `Permission` instances must have corresponding Kubernetes RBAC `Role` names. +* The Kubernetes RBAC `Role` must reside in the same namespace as the Feast service. +* The client application can run in a different namespace, using its own dedicated `ServiceAccount`. +* Finally, the `RoleBinding` that links the client `ServiceAccount` to the RBAC `Role` must be defined in the namespace of the Feast service. + +If the above rules are satisfied, the Feast service must be granted permissions to fetch `RoleBinding` instances from the local namespace. \ No newline at end of file diff --git a/examples/rbac-remote/server/k8s/server_resources.yaml b/examples/rbac-remote/server/k8s/server_resources.yaml index 03e35495d6..21ffcaa68a 100644 --- a/examples/rbac-remote/server/k8s/server_resources.yaml +++ b/examples/rbac-remote/server/k8s/server_resources.yaml @@ -5,23 +5,25 @@ metadata: namespace: feast-dev --- apiVersion: rbac.authorization.k8s.io/v1 -kind: ClusterRole +kind: Role metadata: - name: feast-cluster-role + name: feast-role + namespace: feast-dev rules: - apiGroups: ["rbac.authorization.k8s.io"] - resources: ["roles", "rolebindings", "clusterrolebindings"] + resources: ["roles", "rolebindings"] verbs: ["get", "list", "watch"] --- apiVersion: rbac.authorization.k8s.io/v1 -kind: ClusterRoleBinding +kind: RoleBinding metadata: - name: feast-cluster-rolebinding + name: feast-rolebinding + namespace: feast-dev subjects: - kind: ServiceAccount name: feast-sa namespace: feast-dev roleRef: apiGroup: rbac.authorization.k8s.io - kind: ClusterRole - name: feast-cluster-role + kind: Role + name: feast-role diff --git a/sdk/python/feast/permissions/auth/kubernetes_token_parser.py b/sdk/python/feast/permissions/auth/kubernetes_token_parser.py index c34ebf386d..8108703119 100644 --- a/sdk/python/feast/permissions/auth/kubernetes_token_parser.py +++ b/sdk/python/feast/permissions/auth/kubernetes_token_parser.py @@ -11,6 +11,7 @@ from feast.permissions.user import User logger = logging.getLogger(__name__) +_namespace_file_path = "/var/run/secrets/kubernetes.io/serviceaccount/namespace" class KubernetesTokenParser(TokenParser): @@ -48,24 +49,42 @@ async def user_details_from_access_token(self, access_token: str) -> User: if sa_name is not None and sa_name == intra_communication_base64: return User(username=sa_name, roles=[]) else: - roles = self.get_roles(sa_namespace, sa_name) - logger.info(f"Roles for ServiceAccount {sa_name}: {roles}") + current_namespace = self._read_namespace_from_file() + logger.info( + f"Looking for ServiceAccount roles of {sa_namespace}:{sa_name} in {current_namespace}" + ) + roles = self.get_roles( + current_namespace=current_namespace, + service_account_namespace=sa_namespace, + service_account_name=sa_name, + ) + logger.info(f"Roles: {roles}") return User(username=current_user, roles=roles) - def get_roles(self, namespace: str, service_account_name: str) -> list[str]: + def _read_namespace_from_file(self): + try: + with open(_namespace_file_path, "r") as file: + namespace = file.read().strip() + return namespace + except Exception as e: + raise e + + def get_roles( + self, + current_namespace: str, + service_account_namespace: str, + service_account_name: str, + ) -> list[str]: """ - Fetches the Kubernetes `Role`s associated to the given `ServiceAccount` in the given `namespace`. + Fetches the Kubernetes `Role`s associated to the given `ServiceAccount` in `current_namespace` namespace. - The research also includes the `ClusterRole`s, so the running deployment must be granted enough permissions to query - for such instances in all the namespaces. + The running deployment must be granted enough permissions to query for such instances in this namespace. Returns: - list[str]: Name of the `Role`s and `ClusterRole`s associated to the service account. No string manipulation is performed on the role name. + list[str]: Name of the `Role`s associated to the service account. No string manipulation is performed on the role name. """ - role_bindings = self.rbac_v1.list_namespaced_role_binding(namespace) - cluster_role_bindings = self.rbac_v1.list_cluster_role_binding() - + role_bindings = self.rbac_v1.list_namespaced_role_binding(current_namespace) roles: set[str] = set() for binding in role_bindings.items: @@ -74,16 +93,7 @@ def get_roles(self, namespace: str, service_account_name: str) -> list[str]: if ( subject.kind == "ServiceAccount" and subject.name == service_account_name - ): - roles.add(binding.role_ref.name) - - for binding in cluster_role_bindings.items: - if binding.subjects is not None: - for subject in binding.subjects: - if ( - subject.kind == "ServiceAccount" - and subject.name == service_account_name - and subject.namespace == namespace + and subject.namespace == service_account_namespace ): roles.add(binding.role_ref.name) diff --git a/sdk/python/tests/unit/permissions/auth/conftest.py b/sdk/python/tests/unit/permissions/auth/conftest.py index 5a29f8ec78..246a8c12f2 100644 --- a/sdk/python/tests/unit/permissions/auth/conftest.py +++ b/sdk/python/tests/unit/permissions/auth/conftest.py @@ -20,46 +20,28 @@ def sa_name(): @pytest.fixture -def namespace(): +def my_namespace(): return "my-ns" @pytest.fixture -def rolebindings(sa_name, namespace) -> dict: - roles = ["reader", "writer"] - items = [] - for r in roles: - items.append( - client.V1RoleBinding( - metadata=client.V1ObjectMeta(name=r, namespace=namespace), - subjects=[ - client.V1Subject( - kind="ServiceAccount", - name=sa_name, - api_group="rbac.authorization.k8s.io", - ) - ], - role_ref=client.V1RoleRef( - kind="Role", name=r, api_group="rbac.authorization.k8s.io" - ), - ) - ) - return {"items": client.V1RoleBindingList(items=items), "roles": roles} +def sa_namespace(): + return "sa-ns" @pytest.fixture -def clusterrolebindings(sa_name, namespace) -> dict: - roles = ["updater"] +def rolebindings(my_namespace, sa_name, sa_namespace) -> dict: + roles = ["reader", "writer"] items = [] for r in roles: items.append( - client.V1ClusterRoleBinding( - metadata=client.V1ObjectMeta(name=r, namespace=namespace), + client.V1RoleBinding( + metadata=client.V1ObjectMeta(name=r, namespace=my_namespace), subjects=[ client.V1Subject( kind="ServiceAccount", name=sa_name, - namespace=namespace, + namespace=sa_namespace, api_group="rbac.authorization.k8s.io", ) ], diff --git a/sdk/python/tests/unit/permissions/auth/server/mock_utils.py b/sdk/python/tests/unit/permissions/auth/server/mock_utils.py index 12f7785b05..5bde4d4ecb 100644 --- a/sdk/python/tests/unit/permissions/auth/server/mock_utils.py +++ b/sdk/python/tests/unit/permissions/auth/server/mock_utils.py @@ -53,11 +53,11 @@ async def mock_oath2(self, request): def mock_kubernetes(request, monkeypatch): + my_namespace = request.getfixturevalue("my_namespace") sa_name = request.getfixturevalue("sa_name") - namespace = request.getfixturevalue("namespace") - subject = f"system:serviceaccount:{namespace}:{sa_name}" + sa_namespace = request.getfixturevalue("sa_namespace") + subject = f"system:serviceaccount:{sa_namespace}:{sa_name}" rolebindings = request.getfixturevalue("rolebindings") - clusterrolebindings = request.getfixturevalue("clusterrolebindings") monkeypatch.setattr( "feast.permissions.auth.kubernetes_token_parser.config.load_incluster_config", @@ -71,11 +71,11 @@ def mock_kubernetes(request, monkeypatch): "feast.permissions.auth.kubernetes_token_parser.client.RbacAuthorizationV1Api.list_namespaced_role_binding", lambda *args, **kwargs: rolebindings["items"], ) - monkeypatch.setattr( - "feast.permissions.auth.kubernetes_token_parser.client.RbacAuthorizationV1Api.list_cluster_role_binding", - lambda *args, **kwargs: clusterrolebindings["items"], - ) monkeypatch.setattr( "feast.permissions.client.kubernetes_auth_client_manager.KubernetesAuthClientManager.get_token", lambda self: "my-token", ) + monkeypatch.setattr( + "feast.permissions.auth.kubernetes_token_parser.KubernetesTokenParser._read_namespace_from_file", + lambda self: my_namespace, + ) diff --git a/sdk/python/tests/unit/permissions/auth/test_token_parser.py b/sdk/python/tests/unit/permissions/auth/test_token_parser.py index bac2103b4f..8ac0f2b6d5 100644 --- a/sdk/python/tests/unit/permissions/auth/test_token_parser.py +++ b/sdk/python/tests/unit/permissions/auth/test_token_parser.py @@ -145,27 +145,26 @@ async def mock_oath2(self, request): @patch( "feast.permissions.auth.kubernetes_token_parser.client.RbacAuthorizationV1Api.list_namespaced_role_binding" ) -@patch( - "feast.permissions.auth.kubernetes_token_parser.client.RbacAuthorizationV1Api.list_cluster_role_binding" -) def test_k8s_token_validation_success( - mock_crb, mock_rb, mock_jwt, mock_config, rolebindings, - clusterrolebindings, + monkeypatch, + my_namespace, + sa_name, + sa_namespace, ): - sa_name = "my-name" - namespace = "my-ns" - subject = f"system:serviceaccount:{namespace}:{sa_name}" + monkeypatch.setattr( + "feast.permissions.auth.kubernetes_token_parser.KubernetesTokenParser._read_namespace_from_file", + lambda self: my_namespace, + ) + subject = f"system:serviceaccount:{sa_namespace}:{sa_name}" mock_jwt.return_value = {"sub": subject} mock_rb.return_value = rolebindings["items"] - mock_crb.return_value = clusterrolebindings["items"] roles = rolebindings["roles"] - croles = clusterrolebindings["roles"] access_token = "aaa-bbb-ccc" token_parser = KubernetesTokenParser() @@ -175,12 +174,10 @@ def test_k8s_token_validation_success( assertpy.assert_that(user).is_type_of(User) if isinstance(user, User): - assertpy.assert_that(user.username).is_equal_to(f"{namespace}:{sa_name}") - assertpy.assert_that(user.roles.sort()).is_equal_to((roles + croles).sort()) + assertpy.assert_that(user.username).is_equal_to(f"{sa_namespace}:{sa_name}") + assertpy.assert_that(user.roles.sort()).is_equal_to(roles.sort()) for r in roles: assertpy.assert_that(user.has_matching_role([r])).is_true() - for cr in croles: - assertpy.assert_that(user.has_matching_role([cr])).is_true() assertpy.assert_that(user.has_matching_role(["foo"])).is_false() @@ -212,30 +209,29 @@ def test_k8s_inter_server_comm( oidc_config, request, rolebindings, - clusterrolebindings, monkeypatch, ): if is_intra_server: subject = f":::{intra_communication_val}" else: sa_name = request.getfixturevalue("sa_name") - namespace = request.getfixturevalue("namespace") - subject = f"system:serviceaccount:{namespace}:{sa_name}" + sa_namespace = request.getfixturevalue("sa_namespace") + my_namespace = request.getfixturevalue("my_namespace") + subject = f"system:serviceaccount:{sa_namespace}:{sa_name}" rolebindings = request.getfixturevalue("rolebindings") - clusterrolebindings = request.getfixturevalue("clusterrolebindings") monkeypatch.setattr( "feast.permissions.auth.kubernetes_token_parser.client.RbacAuthorizationV1Api.list_namespaced_role_binding", lambda *args, **kwargs: rolebindings["items"], ) - monkeypatch.setattr( - "feast.permissions.auth.kubernetes_token_parser.client.RbacAuthorizationV1Api.list_cluster_role_binding", - lambda *args, **kwargs: clusterrolebindings["items"], - ) monkeypatch.setattr( "feast.permissions.client.kubernetes_auth_client_manager.KubernetesAuthClientManager.get_token", lambda self: "my-token", ) + monkeypatch.setattr( + "feast.permissions.auth.kubernetes_token_parser.KubernetesTokenParser._read_namespace_from_file", + lambda self: my_namespace, + ) monkeypatch.setattr( "feast.permissions.auth.kubernetes_token_parser.config.load_incluster_config", @@ -248,7 +244,6 @@ def test_k8s_inter_server_comm( ) roles = rolebindings["roles"] - croles = clusterrolebindings["roles"] access_token = "aaa-bbb-ccc" token_parser = KubernetesTokenParser() @@ -263,10 +258,8 @@ def test_k8s_inter_server_comm( else: assertpy.assert_that(user).is_type_of(User) if isinstance(user, User): - assertpy.assert_that(user.username).is_equal_to(f"{namespace}:{sa_name}") - assertpy.assert_that(user.roles.sort()).is_equal_to((roles + croles).sort()) + assertpy.assert_that(user.username).is_equal_to(f"{sa_namespace}:{sa_name}") + assertpy.assert_that(user.roles.sort()).is_equal_to(roles.sort()) for r in roles: assertpy.assert_that(user.has_matching_role([r])).is_true() - for cr in croles: - assertpy.assert_that(user.has_matching_role([cr])).is_true() assertpy.assert_that(user.has_matching_role(["foo"])).is_false()