diff --git a/docs/getting-started/components/authz_manager.md b/docs/getting-started/components/authz_manager.md index eae3fece50b..b24c7abd4c7 100644 --- a/docs/getting-started/components/authz_manager.md +++ b/docs/getting-started/components/authz_manager.md @@ -44,10 +44,10 @@ The server, in turn, uses the same OIDC server to validate the token and extract Some assumptions are made in the OIDC server configuration: * The OIDC token refers to a client with roles matching the RBAC roles of the configured `Permission`s (*) -* The roles are exposed in the access token under `resource_access..roles` +* The roles are exposed in the access token under `resource_access..roles` (Keycloak) or in the top-level `roles` claim (Entra ID app roles). Roles found in both are merged. * The JWT token is expected to have a verified signature and not be expired. The Feast OIDC token parser logic validates for `verify_signature` and `verify_exp` so make sure that the given OIDC provider is configured to meet these requirements. -* The `preferred_username` should be part of the JWT token claim. -* For `GroupBasedPolicy` support, the `groups` claim should be present in the access token (requires a "Group Membership" protocol mapper in Keycloak). +* The username is read from the first of `preferred_username`, `upn`, `azp`, `appid`, `sub` present in the token. Entra ID client-credentials (app-only) tokens carry no user claim, so they authenticate as the calling application. +* For `GroupBasedPolicy` support, the `groups` claim should be present in the access token (requires a "Group Membership" protocol mapper in Keycloak). Entra ID emits group object IDs (GUIDs) in this claim rather than names, and omits it once a principal exceeds Entra's group overage limit, so on Entra a `GroupBasedPolicy` must reference those IDs and cannot rely on the claim for very large group memberships. (*) Please note that **the role match is case-sensitive**, e.g. the name of the role in the OIDC server and in the `Permission` configuration must be exactly the same. @@ -69,6 +69,16 @@ For example, the access token for a client `app` of a user with `reader` role an } ``` +A Microsoft Entra ID (Azure AD) client-credentials (app-only) token has no user claim; the application authenticates as itself, and its app roles arrive in the top-level `roles` claim: +```json +{ + "azp": "11111111-2222-3333-4444-555555555555", + "roles": [ + "reader" + ] +} +``` + #### Server-Side Configuration The server requires `auth_discovery_url` and `client_id` to validate incoming JWT tokens via JWKS: diff --git a/sdk/python/feast/permissions/auth/oidc_token_parser.py b/sdk/python/feast/permissions/auth/oidc_token_parser.py index 7940c9d7525..2bbe8f69942 100644 --- a/sdk/python/feast/permissions/auth/oidc_token_parser.py +++ b/sdk/python/feast/permissions/auth/oidc_token_parser.py @@ -61,17 +61,26 @@ async def _validate_token(self, access_token: str): @staticmethod def _extract_username_or_raise_error(data: dict) -> str: - """Extract the username from the decoded JWT. Raises if missing — identity is mandatory. - - Checks ``preferred_username`` first (Keycloak default), then falls back - to ``upn`` (Azure AD / Entra ID). + """Extract the username from the decoded JWT, or raise if the token + carries none of the recognised identity claims. + + Human identity claims take precedence: ``preferred_username`` (Keycloak + default), then ``upn`` (Azure AD / Entra ID). Client-credentials + (app-only) tokens carry neither, so the calling application's own + identity is used instead: ``azp`` (Entra ID v2 tokens), ``appid`` + (Entra ID v1 tokens), and finally ``sub``, which every issuer sets. + A claim present with a null or non-string value is skipped rather than + returned, so it never shadows a usable later claim. Because ``sub`` is a + mandatory string JWT claim, the raise below fires only for a malformed + token that provides none of the five as a string. """ - if "preferred_username" in data: - return data["preferred_username"] - if "upn" in data: - return data["upn"] + for claim in ("preferred_username", "upn", "azp", "appid", "sub"): + value = data.get(claim) + if isinstance(value, str): + return value raise AuthenticationError( - "Missing preferred_username or upn field in access token." + "Missing username claim in access token: expected one of " + "preferred_username, upn, azp, appid or sub." ) @staticmethod @@ -194,6 +203,10 @@ async def user_details_from_access_token(self, access_token: str) -> User: if self._auth_config.client_id else [] ) + # Entra ID emits app roles in the top-level `roles` claim instead of + # Keycloak's nested `resource_access..roles`. Merge both + # shapes for every issuer, preserving order and dropping duplicates. + roles = list(dict.fromkeys(roles + self._extract_claim(data, "roles"))) groups = self._extract_claim(data, "groups") logger.info( diff --git a/sdk/python/tests/unit/permissions/auth/conftest.py b/sdk/python/tests/unit/permissions/auth/conftest.py index 1ad5a30fe7d..1fd2f34bbfd 100644 --- a/sdk/python/tests/unit/permissions/auth/conftest.py +++ b/sdk/python/tests/unit/permissions/auth/conftest.py @@ -1,3 +1,5 @@ +from unittest.mock import MagicMock + import pytest from kubernetes import client @@ -62,6 +64,22 @@ def oidc_config() -> OidcAuthConfig: ) +@pytest.fixture +def discovery_data() -> dict: + return { + "authorization_endpoint": "https://localhost:8080/realms/master/protocol/openid-connect/auth", + "token_endpoint": "https://localhost:8080/realms/master/protocol/openid-connect/token", + "jwks_uri": "https://localhost:8080/realms/master/protocol/openid-connect/certs", + } + + +@pytest.fixture +def signing_key() -> MagicMock: + key = MagicMock() + key.key = "a-key" + return key + + @pytest.fixture( scope="module", params=[ 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 fdec71e109a..41f201672dd 100644 --- a/sdk/python/tests/unit/permissions/auth/test_token_parser.py +++ b/sdk/python/tests/unit/permissions/auth/test_token_parser.py @@ -23,17 +23,16 @@ @patch("feast.permissions.auth.oidc_token_parser.jwt.decode") @patch("feast.permissions.oidc_service.OIDCDiscoveryService._fetch_discovery_data") def test_oidc_token_validation_success( - mock_discovery_data, mock_jwt, mock_signing_key, mock_oauth2, oidc_config + mock_discovery_data, + mock_jwt, + mock_signing_key, + mock_oauth2, + oidc_config, + discovery_data, + signing_key, ): - signing_key = MagicMock() - signing_key.key = "a-key" mock_signing_key.return_value = signing_key - - mock_discovery_data.return_value = { - "authorization_endpoint": "https://localhost:8080/realms/master/protocol/openid-connect/auth", - "token_endpoint": "https://localhost:8080/realms/master/protocol/openid-connect/token", - "jwks_uri": "https://localhost:8080/realms/master/protocol/openid-connect/certs", - } + mock_discovery_data.return_value = discovery_data user_data = { "preferred_username": "my-name", @@ -67,17 +66,16 @@ def test_oidc_token_validation_success( @patch("feast.permissions.auth.oidc_token_parser.jwt.decode") @patch("feast.permissions.oidc_service.OIDCDiscoveryService._fetch_discovery_data") def test_oidc_token_missing_roles_key_returns_empty( - mock_discovery_data, mock_jwt, mock_signing_key, mock_oauth2, oidc_config + mock_discovery_data, + mock_jwt, + mock_signing_key, + mock_oauth2, + oidc_config, + discovery_data, + signing_key, ): - signing_key = MagicMock() - signing_key.key = "a-key" mock_signing_key.return_value = signing_key - - mock_discovery_data.return_value = { - "authorization_endpoint": "https://localhost:8080/realms/master/protocol/openid-connect/auth", - "token_endpoint": "https://localhost:8080/realms/master/protocol/openid-connect/token", - "jwks_uri": "https://localhost:8080/realms/master/protocol/openid-connect/certs", - } + mock_discovery_data.return_value = discovery_data user_data = { "preferred_username": "my-name", @@ -104,17 +102,16 @@ def test_oidc_token_missing_roles_key_returns_empty( @patch("feast.permissions.auth.oidc_token_parser.jwt.decode") @patch("feast.permissions.oidc_service.OIDCDiscoveryService._fetch_discovery_data") def test_oidc_token_extracts_groups( - mock_discovery_data, mock_jwt, mock_signing_key, mock_oauth2, oidc_config + mock_discovery_data, + mock_jwt, + mock_signing_key, + mock_oauth2, + oidc_config, + discovery_data, + signing_key, ): - signing_key = MagicMock() - signing_key.key = "a-key" mock_signing_key.return_value = signing_key - - mock_discovery_data.return_value = { - "authorization_endpoint": "https://localhost:8080/realms/master/protocol/openid-connect/auth", - "token_endpoint": "https://localhost:8080/realms/master/protocol/openid-connect/token", - "jwks_uri": "https://localhost:8080/realms/master/protocol/openid-connect/certs", - } + mock_discovery_data.return_value = discovery_data user_data = { "preferred_username": "my-name", @@ -146,17 +143,16 @@ def test_oidc_token_extracts_groups( @patch("feast.permissions.auth.oidc_token_parser.jwt.decode") @patch("feast.permissions.oidc_service.OIDCDiscoveryService._fetch_discovery_data") def test_oidc_token_extracts_groups_and_roles( - mock_discovery_data, mock_jwt, mock_signing_key, mock_oauth2, oidc_config + mock_discovery_data, + mock_jwt, + mock_signing_key, + mock_oauth2, + oidc_config, + discovery_data, + signing_key, ): - signing_key = MagicMock() - signing_key.key = "a-key" mock_signing_key.return_value = signing_key - - mock_discovery_data.return_value = { - "authorization_endpoint": "https://localhost:8080/realms/master/protocol/openid-connect/auth", - "token_endpoint": "https://localhost:8080/realms/master/protocol/openid-connect/token", - "jwks_uri": "https://localhost:8080/realms/master/protocol/openid-connect/certs", - } + mock_discovery_data.return_value = discovery_data user_data = { "preferred_username": "my-name", @@ -184,6 +180,212 @@ def test_oidc_token_extracts_groups_and_roles( assertpy.assert_that(user.has_matching_group(["banking-admin"])).is_true() +@pytest.mark.parametrize( + "token_claims, expected_username", + [ + # Human identity claims keep precedence over the application claims. + ( + { + "preferred_username": "my-name", + "upn": "my-name@example.com", + "azp": "my-client-app", + }, + "my-name", + ), + ( + {"upn": "my-name@example.com", "azp": "my-client-app"}, + "my-name@example.com", + ), + # A human claim still wins when accompanied only by application claims. + ( + { + "preferred_username": "my-name", + "appid": "my-v1-client-app", + "sub": "my-subject", + }, + "my-name", + ), + ({"upn": "my-name@example.com", "sub": "my-subject"}, "my-name@example.com"), + # Entra ID client-credentials (app-only) tokens carry no human claim. + ( + {"azp": "my-client-app", "appid": "my-v1-client-app", "sub": "my-subject"}, + "my-client-app", + ), + ({"appid": "my-v1-client-app", "sub": "my-subject"}, "my-v1-client-app"), + ({"sub": "my-subject"}, "my-subject"), + ], +) +@patch( + "feast.permissions.auth.oidc_token_parser.OAuth2AuthorizationCodeBearer.__call__" +) +@patch("feast.permissions.auth.oidc_token_parser.PyJWKClient.get_signing_key_from_jwt") +@patch("feast.permissions.auth.oidc_token_parser.jwt.decode") +@patch("feast.permissions.oidc_service.OIDCDiscoveryService._fetch_discovery_data") +def test_oidc_token_username_claim_precedence( + mock_discovery_data, + mock_jwt, + mock_signing_key, + mock_oauth2, + token_claims, + expected_username, + oidc_config, + discovery_data, + signing_key, +): + mock_signing_key.return_value = signing_key + mock_discovery_data.return_value = discovery_data + + mock_jwt.return_value = token_claims + + access_token = "aaa-bbb-ccc" + token_parser = OidcTokenParser(auth_config=oidc_config) + user = asyncio.run( + token_parser.user_details_from_access_token(access_token=access_token) + ) + + assertpy.assert_that(user).is_type_of(User) + if isinstance(user, User): + assertpy.assert_that(user.username).is_equal_to(expected_username) + + +@pytest.mark.parametrize( + "token_claims", + [ + # No identity claim at all. + {"resource_access": {_CLIENT_ID: {"roles": ["reader"]}}}, + # Identity claims present but null are skipped, not accepted. + {"preferred_username": None, "upn": None, "azp": None, "sub": None}, + # A non-string identity claim is skipped, not accepted. + {"sub": 12345}, + ], +) +@patch( + "feast.permissions.auth.oidc_token_parser.OAuth2AuthorizationCodeBearer.__call__" +) +@patch("feast.permissions.auth.oidc_token_parser.PyJWKClient.get_signing_key_from_jwt") +@patch("feast.permissions.auth.oidc_token_parser.jwt.decode") +@patch("feast.permissions.oidc_service.OIDCDiscoveryService._fetch_discovery_data") +def test_oidc_token_without_usable_username_claim_fails( + mock_discovery_data, + mock_jwt, + mock_signing_key, + mock_oauth2, + token_claims, + oidc_config, + discovery_data, + signing_key, +): + mock_signing_key.return_value = signing_key + mock_discovery_data.return_value = discovery_data + + mock_jwt.return_value = token_claims + + access_token = "aaa-bbb-ccc" + token_parser = OidcTokenParser(auth_config=oidc_config) + with pytest.raises(AuthenticationError): + asyncio.run( + token_parser.user_details_from_access_token(access_token=access_token) + ) + + +@pytest.mark.parametrize( + "token_claims, expected_roles", + [ + # Entra ID app roles arrive in the top-level `roles` claim. + ({"roles": ["reader", "writer"]}, ["reader", "writer"]), + # Duplicates within the top-level claim are collapsed. + ({"roles": ["reader", "reader", "writer"]}, ["reader", "writer"]), + # Keycloak's nested claim keeps working on its own. + ({"resource_access": {_CLIENT_ID: {"roles": ["reader"]}}}, ["reader"]), + # Both shapes present: merged, in order, without duplicates. + ( + { + "resource_access": {_CLIENT_ID: {"roles": ["reader", "writer"]}}, + "roles": ["writer", "admin"], + }, + ["reader", "writer", "admin"], + ), + ], +) +@patch( + "feast.permissions.auth.oidc_token_parser.OAuth2AuthorizationCodeBearer.__call__" +) +@patch("feast.permissions.auth.oidc_token_parser.PyJWKClient.get_signing_key_from_jwt") +@patch("feast.permissions.auth.oidc_token_parser.jwt.decode") +@patch("feast.permissions.oidc_service.OIDCDiscoveryService._fetch_discovery_data") +def test_oidc_token_merges_top_level_and_resource_access_roles( + mock_discovery_data, + mock_jwt, + mock_signing_key, + mock_oauth2, + token_claims, + expected_roles, + oidc_config, + discovery_data, + signing_key, +): + mock_signing_key.return_value = signing_key + mock_discovery_data.return_value = discovery_data + + mock_jwt.return_value = {"preferred_username": "my-name", **token_claims} + + access_token = "aaa-bbb-ccc" + token_parser = OidcTokenParser(auth_config=oidc_config) + user = asyncio.run( + token_parser.user_details_from_access_token(access_token=access_token) + ) + + assertpy.assert_that(user).is_type_of(User) + if isinstance(user, User): + assertpy.assert_that(user.roles).is_equal_to(expected_roles) + + +@patch( + "feast.permissions.auth.oidc_token_parser.OAuth2AuthorizationCodeBearer.__call__" +) +@patch("feast.permissions.auth.oidc_token_parser.PyJWKClient.get_signing_key_from_jwt") +@patch("feast.permissions.auth.oidc_token_parser.jwt.decode") +@patch("feast.permissions.oidc_service.OIDCDiscoveryService._fetch_discovery_data") +def test_oidc_identity_and_roles_come_from_verified_decode( + mock_discovery_data, + mock_jwt, + mock_signing_key, + mock_oauth2, + oidc_config, + discovery_data, + signing_key, +): + """Identity and roles must come from the signature-verified decode, not the + unverified decode used only for routing.""" + mock_signing_key.return_value = signing_key + mock_discovery_data.return_value = discovery_data + + def decode(token, key=None, *args, **kwargs): + if kwargs.get("options", {}).get("verify_signature") is False: + # Unverified decode, used only for routing; must not drive identity. + return { + "preferred_username": "spoofed-user", + "resource_access": {_CLIENT_ID: {"roles": ["spoofed-role"]}}, + } + return { + "preferred_username": "verified-user", + "resource_access": {_CLIENT_ID: {"roles": ["reader"]}}, + } + + mock_jwt.side_effect = decode + + access_token = "aaa-bbb-ccc" + token_parser = OidcTokenParser(auth_config=oidc_config) + user = asyncio.run( + token_parser.user_details_from_access_token(access_token=access_token) + ) + + assertpy.assert_that(user).is_type_of(User) + if isinstance(user, User): + assertpy.assert_that(user.username).is_equal_to("verified-user") + assertpy.assert_that(user.roles).is_equal_to(["reader"]) + + @patch( "feast.permissions.auth.oidc_token_parser.OAuth2AuthorizationCodeBearer.__call__" ) @@ -401,14 +603,10 @@ def test_k8s_inter_server_comm( @patch("feast.permissions.auth.oidc_token_parser.jwt.decode") @patch("feast.permissions.oidc_service.OIDCDiscoveryService._fetch_discovery_data") def test_oidc_parser_handles_sa_token_via_token_review( - mock_discovery_data, mock_jwt_decode, oidc_config + mock_discovery_data, mock_jwt_decode, oidc_config, discovery_data ): """When a token contains kubernetes.io claim, _handle_sa_token is called (not the OIDC JWKS path).""" - mock_discovery_data.return_value = { - "authorization_endpoint": "https://localhost:8080/auth", - "token_endpoint": "https://localhost:8080/token", - "jwks_uri": "https://localhost:8080/certs", - } + mock_discovery_data.return_value = discovery_data mock_jwt_decode.return_value = { "kubernetes.io": {"namespace": "feast"}, @@ -447,18 +645,17 @@ def test_oidc_parser_handles_sa_token_via_token_review( @patch("feast.permissions.auth.oidc_token_parser.jwt.decode") @patch("feast.permissions.oidc_service.OIDCDiscoveryService._fetch_discovery_data") def test_oidc_parser_routes_keycloak_token_normally( - mock_discovery_data, mock_jwt_decode, mock_signing_key, mock_oauth2, oidc_config + mock_discovery_data, + mock_jwt_decode, + mock_signing_key, + mock_oauth2, + oidc_config, + discovery_data, + signing_key, ): """When a token does NOT contain kubernetes.io claim, it should follow the OIDC path.""" - signing_key = MagicMock() - signing_key.key = "a-key" mock_signing_key.return_value = signing_key - - mock_discovery_data.return_value = { - "authorization_endpoint": "https://localhost:8080/auth", - "token_endpoint": "https://localhost:8080/token", - "jwks_uri": "https://localhost:8080/certs", - } + mock_discovery_data.return_value = discovery_data keycloak_payload = { "preferred_username": "testuser",