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

fix: Replaced ClusterRoles with local RoleBindings #4625

Merged
merged 2 commits into from
Oct 15, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion docs/getting-started/components/authz_manager.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
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.
16 changes: 9 additions & 7 deletions examples/rbac-remote/server/k8s/server_resources.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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
dmartinol marked this conversation as resolved.
Show resolved Hide resolved
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
dmartinol marked this conversation as resolved.
Show resolved Hide resolved
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
50 changes: 30 additions & 20 deletions sdk/python/feast/permissions/auth/kubernetes_token_parser.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down Expand Up @@ -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:
Expand All @@ -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)

Expand Down
34 changes: 8 additions & 26 deletions sdk/python/tests/unit/permissions/auth/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -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",
)
],
Expand Down
14 changes: 7 additions & 7 deletions sdk/python/tests/unit/permissions/auth/server/mock_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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,
)
47 changes: 20 additions & 27 deletions sdk/python/tests/unit/permissions/auth/test_token_parser.py
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand All @@ -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()


Expand Down Expand Up @@ -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",
Expand All @@ -248,7 +244,6 @@ def test_k8s_inter_server_comm(
)

roles = rolebindings["roles"]
croles = clusterrolebindings["roles"]

access_token = "aaa-bbb-ccc"
token_parser = KubernetesTokenParser()
Expand All @@ -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()
Loading