From 8178eb2df995223e3c7bce4afd90c2827fa13f76 Mon Sep 17 00:00:00 2001 From: Zeev Manilovich Date: Wed, 7 Oct 2026 23:52:36 +0300 Subject: [PATCH 01/21] Forward every value of a repeated query parameter (PER-16922) The facts proxy built the forwarded query with {**request.query_params, ...} and the /cloud and /sdk proxies with dict(request.query_params). Both keep only the last value of a key, so GET /facts/role_assignments?user=a&user=b asked the backend about user b alone. Reported in #299. Forward request.query_params.multi_items() instead, so each key keeps all of its values in order. In the facts client an explicit query_params override (return_deleted=True on unassign) still replaces every value the caller sent for that key. Co-authored-by: omribz156 <260634988+omribz156@users.noreply.github.com> Co-Authored-By: Claude Opus 5.5 --- horizon/facts/client.py | 11 +++- horizon/proxy/api.py | 2 +- horizon/tests/test_facts_client.py | 80 ++++++++++++++++++++++++++++-- horizon/tests/test_facts_router.py | 34 +++++++++++++ horizon/tests/test_proxy_api.py | 57 +++++++++++++++++++++ 5 files changed, 178 insertions(+), 6 deletions(-) create mode 100644 horizon/tests/test_proxy_api.py diff --git a/horizon/facts/client.py b/horizon/facts/client.py index 7ac1799f..7bf7e1e2 100644 --- a/horizon/facts/client.py +++ b/horizon/facts/client.py @@ -48,6 +48,9 @@ async def build_forward_request( Build an HTTPX request from a FastAPI request to forward to the facts service. :param request: FastAPI request :param path: Backend facts service path to forward to + :param query_params: query parameters to set on the forwarded request. Each replaces every value + the request has for the same key; every other request query parameter is forwarded with all + of its values, in order. :param is_consistent_update: if True, marks the request as a consistent update so the backend skips the control-plane delta update (the PDP handles propagation locally). :return: HTTPX request @@ -67,11 +70,15 @@ async def build_forward_request( ) full_path = urljoin(f"/v2/facts/{project_id}/{environment_id}/", path.removeprefix("/")) - _query_params = {**request.query_params, **(query_params or {})} + overrides = query_params or {} + forward_params: list[tuple[str, Any]] = [ + (key, value) for key, value in request.query_params.multi_items() if key not in overrides + ] + forward_params.extend(overrides.items()) return self.client.build_request( method=request.method, url=full_path, - params=_query_params, + params=forward_params, headers=forward_headers, content=request.stream(), ) diff --git a/horizon/proxy/api.py b/horizon/proxy/api.py index fd0adb68..3b4636c9 100644 --- a/horizon/proxy/api.py +++ b/horizon/proxy/api.py @@ -182,7 +182,7 @@ async def proxy_request_to_cloud_service( headers={"WWW-Authenticate": "Bearer"}, ) path = f"{cloud_service_url}/{path}" - params = dict(request.query_params) or {} + params = request.query_params.multi_items() original_headers = {k.lower(): v for k, v in iter(dict(request.headers).items())} headers = additional_headers diff --git a/horizon/tests/test_facts_client.py b/horizon/tests/test_facts_client.py index d1e2a370..ab125657 100644 --- a/horizon/tests/test_facts_client.py +++ b/horizon/tests/test_facts_client.py @@ -1,5 +1,5 @@ from collections.abc import Iterator -from typing import TYPE_CHECKING +from typing import TYPE_CHECKING, Any from unittest.mock import AsyncMock, MagicMock, patch import pytest @@ -14,13 +14,13 @@ from loguru import Record -def _make_request(headers: dict[str, str] | None = None) -> FastApiRequest: +def _make_request(headers: dict[str, str] | None = None, query_string: bytes = b"") -> FastApiRequest: scope = { "type": "http", "method": "POST", "path": "/facts/users", "raw_path": b"/facts/users", - "query_string": b"", + "query_string": query_string, "headers": [(k.lower().encode(), v.encode()) for k, v in (headers or {}).items()], } @@ -90,6 +90,80 @@ async def test_send_forward_request_propagates_consistent_update_kwarg(): assert sent_request.headers.get("X-Permit-Consistent-Update") == "true" +@pytest.fixture +def facts_environment() -> Iterator[None]: + """The remote config and API key build_forward_request reads, as the PDP has them once started.""" + remote_config = MagicMock() + remote_config.context = {"project_id": "proj1", "env_id": "env1"} + with ( + patch("horizon.facts.client.get_remote_config", return_value=remote_config), + patch("horizon.facts.client.get_env_api_key", return_value="test_api_key"), + ): + yield + + +def _values_by_key(items: list[tuple[str, str]]) -> dict[str, list[str]]: + values: dict[str, list[str]] = {} + for key, value in items: + values.setdefault(key, []).append(value) + return values + + +async def _forwarded_query(query_string: bytes, query_params: dict[str, Any] | None = None) -> dict[str, list[str]]: + """Each query parameter of the request the PDP forwards for one with ``query_string``, with its decoded + values in order.""" + request = _make_request(query_string=query_string) + forward_request = await FactsClient().build_forward_request(request, "/role_assignments", query_params=query_params) + return _values_by_key(forward_request.url.params.multi_items()) + + +@pytest.mark.usefixtures("facts_environment") +@pytest.mark.parametrize( + "query_string", + [ + b"", + b"user=u1", + b"user=u1&user=u2", + b"user=u2&role=r1&user=u1&tenant=t1&role=r2&search=b&tenant=t2&search=a", + b"user=u1&user=u1", + b"search=a%20b&search=c%26d&search=e%3Df&search=&search=%C3%A9", + ], + ids=["none", "single", "repeated", "interleaved-unsorted", "repeated-same-value", "encoded-values"], +) +@pytest.mark.asyncio +async def test_build_forward_request_forwards_every_query_parameter_value_in_order(query_string: bytes): + incoming = _make_request(query_string=query_string).query_params.multi_items() + + assert await _forwarded_query(query_string) == _values_by_key(incoming) + + +@pytest.mark.usefixtures("facts_environment") +@pytest.mark.asyncio +async def test_build_forward_request_keeps_each_repeated_value_and_its_order(): + """The generic test above compares against the request's own parsing; pin the decoded values once.""" + forwarded = await _forwarded_query(b"user=u2&role=r1&user=u1&search=c%26d&search=&search=%C3%A9") + + assert forwarded == {"user": ["u2", "u1"], "role": ["r1"], "search": ["c&d", "", "\u00e9"]} + + +@pytest.mark.usefixtures("facts_environment") +@pytest.mark.parametrize( + ("query_string", "expected"), + [ + (b"", {"return_deleted": ["true"]}), + (b"user=u1&user=u2", {"user": ["u1", "u2"], "return_deleted": ["true"]}), + (b"return_deleted=false&user=u1&return_deleted=0&user=u2", {"user": ["u1", "u2"], "return_deleted": ["true"]}), + ], + ids=["no-request-params", "request-without-the-key", "request-sets-the-key-twice"], +) +@pytest.mark.asyncio +async def test_build_forward_request_query_params_replace_every_request_value_for_their_key( + query_string: bytes, expected: dict[str, list[str]] +): + """The router's return_deleted=True must win over whatever the caller sent for return_deleted.""" + assert await _forwarded_query(query_string, query_params={"return_deleted": True}) == expected + + @pytest.fixture def logged_errors() -> Iterator[list["Record"]]: """Every loguru record at ERROR or above emitted during the test.""" diff --git a/horizon/tests/test_facts_router.py b/horizon/tests/test_facts_router.py index 430313c6..4737b8c4 100644 --- a/horizon/tests/test_facts_router.py +++ b/horizon/tests/test_facts_router.py @@ -219,6 +219,40 @@ def test_facts_write_with_an_invalid_wait_header_is_400_before_forwarding(facts, facts.subscriber.publish_and_wait.assert_not_awaited() +def test_facts_read_forwards_every_value_of_a_repeated_query_parameter(facts): + """GET /facts/role_assignments?user=a&user=b lists both users' assignments: permitio/PDP#299.""" + response = facts.client.get("/facts/role_assignments?user=u2&tenant=t1&user=u1&role=r1&role=r2", headers=AUTH) + + assert response.status_code == status.HTTP_200_OK + (backend_call,) = facts.backend_calls + assert backend_call.url.path == "/v2/facts/proj/env/role_assignments" + params = backend_call.url.params + assert sorted(params.keys()) == ["role", "tenant", "user"] + assert params.get_list("user") == ["u2", "u1"] + assert params.get_list("tenant") == ["t1"] + assert params.get_list("role") == ["r1", "r2"] + + +@pytest.mark.parametrize( + ("method", "path"), + [("DELETE", "/facts/role_assignments"), ("DELETE", "/facts/users/user-1/roles")], + ids=["role-assignments", "user-roles"], +) +def test_facts_unassign_sends_return_deleted_true_whatever_the_caller_sent(facts, method, path): + facts.client.request( + method, + f"{path}?return_deleted=false&tenant=t1&tenant=t2&return_deleted=0", + headers=AUTH, + json={"user": "user-1", "role": "viewer", "tenant": "t1"}, + ) + + (backend_call,) = facts.backend_calls + params = backend_call.url.params + assert sorted(params.keys()) == ["return_deleted", "tenant"] + assert params.get_list("return_deleted") == ["true"] + assert params.get_list("tenant") == ["t1", "t2"] + + @pytest.fixture def logged_warnings() -> Iterator[list["Record"]]: """Every loguru record at WARNING or above emitted during the test.""" diff --git a/horizon/tests/test_proxy_api.py b/horizon/tests/test_proxy_api.py new file mode 100644 index 00000000..f9c10be5 --- /dev/null +++ b/horizon/tests/test_proxy_api.py @@ -0,0 +1,57 @@ +"""Query parameters the /cloud and /sdk proxy routes forward to the backend.""" + +import re + +import pytest +from aioresponses import aioresponses +from fastapi import FastAPI +from fastapi.testclient import TestClient +from starlette import status +from yarl import URL + +from horizon.config import sidecar_config +from horizon.proxy.api import router + +CLOUD_BACKEND = "http://backend.test/cloud-api" +LEGACY_BACKEND = "http://backend.test/legacy-api" +AUTH = {"Authorization": "Bearer proxy-test-token"} + + +@pytest.fixture +def proxy_client(monkeypatch) -> TestClient: + """The proxy routes alone, pointed at a backend that aioresponses answers.""" + monkeypatch.setattr(sidecar_config, "BACKEND_SERVICE_URL", CLOUD_BACKEND) + monkeypatch.setattr(sidecar_config, "BACKEND_LEGACY_URL", LEGACY_BACKEND) + app = FastAPI() + app.include_router(router) + return TestClient(app) + + +@pytest.mark.parametrize(("route", "backend"), [("/cloud", CLOUD_BACKEND), ("/sdk", LEGACY_BACKEND)]) +@pytest.mark.parametrize( + ("query_string", "expected"), + [ + ("", []), + ("?user=u1", [("user", "u1")]), + ( + "?user=u2&tenant=t1&user=u1&search=b%26c&search=a", + [("user", "u2"), ("tenant", "t1"), ("user", "u1"), ("search", "b&c"), ("search", "a")], + ), + ], + ids=["none", "single", "repeated"], +) +def test_proxy_forwards_every_query_parameter_value_in_order( + proxy_client: TestClient, route: str, backend: str, query_string: str, expected: list[tuple[str, str]] +): + with aioresponses() as backend_mock: + backend_mock.get(re.compile(rf"^{re.escape(backend)}/v2/role_assignments"), payload=[]) + + response = proxy_client.get(f"{route}/v2/role_assignments{query_string}", headers=AUTH) + + assert response.status_code == status.HTTP_200_OK + ((_, recorded_url), calls) = next(iter(backend_mock.requests.items())) + # aioresponses records the URL with its query sorted, so it shows that no value was dropped. + # aiohttp builds the query it sends from the params it was given, which show the order. + assert sorted(recorded_url.query.items()) == sorted(expected) + sent_query = URL(backend).with_query(calls[0].kwargs["params"]).query + assert list(sent_query.items()) == expected From 4008d0ffae34c166899179e21c783513abadc65a Mon Sep 17 00:00:00 2001 From: Zeev Manilovich Date: Wed, 7 Oct 2026 23:56:14 +0300 Subject: [PATCH 02/21] Handle repeated query parameters in /allowed_url rules (PER-16927) Mapping-rule matching and query attribute extraction read the last value of a repeated query parameter of the requested URL: ?id=2&id=1 matched a rule on id=1, ?id=1&id=2 did not, and a rule with ?id={id} checked the last id. The app behind the URL may read the first value instead, and then act on a value other than the one checked. When a parameter a rule reads has more than one distinct value, the URL now gets no mapping rule at all and /allowed_url answers allow=false with the reason in debug. Skipping only that rule is not enough: a lower-priority rule that ignores the parameter would then apply whatever value the app reads. A rule that no value of the URL could satisfy still simply does not match, and a parameter repeated with one value counts as given once. Co-Authored-By: Claude Opus 5.5 --- horizon/enforcer/api.py | 26 +++-- horizon/enforcer/utils/mapping_rules_utils.py | 54 +++++++-- horizon/tests/test_enforcer_api.py | 60 ++++++++++ horizon/tests/test_mapping_rules_utils.py | 107 +++++++++++++++++- 4 files changed, 232 insertions(+), 15 deletions(-) diff --git a/horizon/enforcer/api.py b/horizon/enforcer/api.py index fed6894c..7380296b 100644 --- a/horizon/enforcer/api.py +++ b/horizon/enforcer/api.py @@ -42,7 +42,7 @@ KongWrappedAuthorizationQuery, ) from horizon.enforcer.schemas_v1 import AuthorizationQueryV1 -from horizon.enforcer.utils.mapping_rules_utils import MappingRulesUtils +from horizon.enforcer.utils.mapping_rules_utils import ConflictingQueryParameterError, MappingRulesUtils from horizon.enforcer.utils.statistics_utils import StatisticsManager from horizon.state import PersistentStateHandler @@ -77,6 +77,12 @@ def extract_pdp_api_key(request: Request) -> str: return token +def _url_not_allowed(query: UrlAuthorizationQuery, error: ConflictingQueryParameterError) -> dict: + """The /allowed_url answer for a URL whose mapping rule cannot be applied unambiguously.""" + logger.debug("allowed_url: {reason}; not allowed", reason=str(error), url=query.url) + return {"allow": False, "result": False, "query": query.dict(), "debug": {"reason": str(error)}} + + def transform_headers(request: Request) -> dict: token = extract_pdp_api_key(request) return { @@ -341,9 +347,12 @@ async def is_allowed_url( data_result = json.loads(data.body).get("result") or {} mapping_rules_json = data_result.get("all") or [] mapping_rules = [parse_obj_as(MappingRuleData, mapping_rule) for mapping_rule in mapping_rules_json] - matched_mapping_rule = MappingRulesUtils.extract_mapping_rule_by_request( - mapping_rules, query.http_method, query.url - ) + try: + matched_mapping_rule = MappingRulesUtils.extract_mapping_rule_by_request( + mapping_rules, query.http_method, query.url + ) + except ConflictingQueryParameterError as e: + return _url_not_allowed(query, e) if matched_mapping_rule is None: return { "allow": False, @@ -365,9 +374,12 @@ async def is_allowed_url( path_attributes = MappingRulesUtils.extract_attributes_from_url(matched_mapping_rule.url, query.url) # Query params handling remains the same for both types - query_params_attributes = MappingRulesUtils.extract_attributes_from_query_params( - matched_mapping_rule.url, query.url - ) + try: + query_params_attributes = MappingRulesUtils.extract_attributes_from_query_params( + matched_mapping_rule.url, query.url + ) + except ConflictingQueryParameterError as e: + return _url_not_allowed(query, e) attributes = {**path_attributes, **query_params_attributes} allowed_query = AuthorizationQuery( user=query.user, diff --git a/horizon/enforcer/utils/mapping_rules_utils.py b/horizon/enforcer/utils/mapping_rules_utils.py index bdab75d6..e0fc7886 100644 --- a/horizon/enforcer/utils/mapping_rules_utils.py +++ b/horizon/enforcer/utils/mapping_rules_utils.py @@ -9,6 +9,18 @@ from horizon.enforcer.schemas import MappingRuleData, UrlTypes +class ConflictingQueryParameterError(ValueError): + """A query parameter a mapping rule reads has more than one distinct value in the requested URL. + + Which value the protected app reads for such a parameter is up to the app, so the PDP cannot + tell which rule applies or which attribute value to check. + """ + + def __init__(self, key: str): + super().__init__(f"Query parameter '{key}' has more than one distinct value in the requested URL") + self.key = key + + class MappingRulesUtils: @staticmethod def _compare_httpurls(mapping_rule_url: str, request_url: str) -> bool: @@ -40,20 +52,32 @@ def _compare_url_path(mapping_rule_url: str | None, request_url: str | None) -> @staticmethod def _compare_query_params(mapping_rule_query_string: str, request_url_query_string: str) -> bool: + """Whether the request's query satisfies every parameter of the mapping rule's query. + + A parameter repeated with one value counts as given once. + + Raises: + ConflictingQueryParameterError: the request could satisfy the rule, but a parameter the + rule reads has more than one distinct value, so whether it does depends on the value read. + """ mapping_rule_query_params = QueryParams(mapping_rule_query_string) request_query_params = QueryParams(request_url_query_string) + conflicting_key = None for key in mapping_rule_query_params: - if key not in request_query_params: + request_values = set(request_query_params.getlist(key)) + if not request_values: return False - if mapping_rule_query_params[key].startswith("{") and mapping_rule_query_params[key].endswith("}"): - # if the value is an attribute - # we just need to make sure the attribute is in the request query params - continue - if mapping_rule_query_params[key] != request_query_params[key]: - # if the value is not an attribute, verify that the values are the same + rule_value = mapping_rule_query_params[key] + is_attribute = rule_value.startswith("{") and rule_value.endswith("}") + if not is_attribute and rule_value not in request_values: return False + if len(request_values) > 1: + conflicting_key = key + + if conflicting_key is not None: + raise ConflictingQueryParameterError(conflicting_key) return True @staticmethod @@ -70,6 +94,13 @@ def extract_attributes_from_url(rule_url: str, request_url: str) -> dict: @staticmethod def extract_attributes_from_query_params(rule_url: str, request_url: str) -> dict: + """The attributes a mapping rule's query reads from the request URL, e.g. ``{"id": "7"}`` for + a rule with ``?id={id}`` and a request with ``?id=7``. + + Raises: + ConflictingQueryParameterError: a parameter the rule reads an attribute from has more than + one distinct value in the request URL. + """ if "?" not in rule_url or "?" not in request_url: return {} rule_query_params = QueryParams(rule_url.split("?")[1]) @@ -77,6 +108,8 @@ def extract_attributes_from_query_params(rule_url: str, request_url: str) -> dic attributes = {} for key in rule_query_params: if rule_query_params[key].startswith("{") and rule_query_params[key].endswith("}"): + if len(set(request_query_params.getlist(key))) > 1: + raise ConflictingQueryParameterError(key) attributes[rule_query_params[key][1:-1]] = request_query_params[key] return attributes @@ -105,6 +138,13 @@ def extract_mapping_rule_by_request( http_method: str, url: AnyHttpUrl, ) -> MappingRuleData | None: + """The highest-priority mapping rule for the request's method and URL, or None if none matches. + + Raises: + ConflictingQueryParameterError: a rule for the method and path could match, but a query + parameter it reads has more than one distinct value. The request then gets no rule at + all, rather than a lower-priority one that skips the parameter. + """ matched_mapping_rules = [] http_method = http_method.lower() # Convert once instead of in each iteration diff --git a/horizon/tests/test_enforcer_api.py b/horizon/tests/test_enforcer_api.py index b90167a8..c6a113ae 100644 --- a/horizon/tests/test_enforcer_api.py +++ b/horizon/tests/test_enforcer_api.py @@ -6,6 +6,7 @@ from typing import TYPE_CHECKING import aiohttp +import httpx import pytest from aioresponses import aioresponses from fastapi import FastAPI, Response @@ -366,6 +367,65 @@ def test_nginx_allowed_without_a_required_header_is_422_and_never_asks_opa(monke assert [error["loc"] for error in response.json()["detail"]] == [["header", missing]] +DOCUMENTS_URL = "https://api.example.com/documents" +DOC_ID_RULE = {"url": DOCUMENTS_URL + "?id={doc_id}", "http_method": "get", "action": "read", "resource": "document"} +DOC_7_RULE_OVER_CATCH_ALL = [ + {"url": DOCUMENTS_URL + "?id=7", "http_method": "get", "action": "edit", "resource": "document", "priority": 10}, + {"url": DOCUMENTS_URL, "http_method": "get", "action": "read", "resource": "document", "priority": 1}, +] + + +def _post_allowed_url(monkeypatch, url: str, mapping_rules: list[dict]) -> tuple[httpx.Response, list[dict]]: + """POST /allowed_url for ``url`` with OPA holding ``mapping_rules`` and allowing every check. + + Returns the response and the input of each check the PDP asked OPA to decide. + """ + monkeypatch.setattr(stats_manager, "_messages", asyncio.Queue()) + opa_data_url = f"{opal_client_config.POLICY_STORE_URL}/v1/data" + query = UrlAuthorizationQuery(user=User(key="user1"), http_method="GET", url=url, tenant="default") + with aioresponses() as m: + m.post(f"{opa_data_url}/mapping_rules", payload={"result": {"all": mapping_rules}}) + m.post(f"{opa_data_url}/permit/root", payload={"result": {"allow": True}}, repeat=True) + response = TestClient(sidecar._app).post( + "/allowed_url", headers={"authorization": f"Bearer {sidecar_config.API_KEY}"}, json=query.dict() + ) + checks = [ + json.loads(call.kwargs["data"])["input"] + for (_, requested_url), calls in m.requests.items() + if requested_url.path.endswith("/permit/root") + for call in calls + ] + return response, checks + + +@pytest.mark.parametrize("query_string", ["?id=7", "?id=7&id=7"], ids=["single", "repeated-same-value"]) +def test_allowed_url_checks_the_value_of_the_query_parameter_the_rule_reads(monkeypatch, query_string: str): + response, checks = _post_allowed_url(monkeypatch, DOCUMENTS_URL + query_string, [DOC_ID_RULE]) + + assert response.status_code == status.HTTP_200_OK + assert response.json()["allow"] is True + (check,) = checks + assert check["resource"]["attributes"] == {"doc_id": "7"} + + +@pytest.mark.parametrize("query_string", ["?id=7&id=8", "?id=8&id=7"], ids=["rule-value-first", "rule-value-last"]) +@pytest.mark.parametrize( + "mapping_rules", [[DOC_ID_RULE], DOC_7_RULE_OVER_CATCH_ALL], ids=["attribute-rule", "literal-rule-over-catch-all"] +) +def test_allowed_url_with_conflicting_values_for_a_rule_query_parameter_is_not_allowed( + monkeypatch, query_string: str, mapping_rules: list[dict] +): + """The app behind the URL may read either value, so no single check covers the request.""" + response, checks = _post_allowed_url(monkeypatch, DOCUMENTS_URL + query_string, mapping_rules) + + assert response.status_code == status.HTTP_200_OK + body = response.json() + assert body["allow"] is False + assert body["result"] is False + assert body["debug"] == {"reason": "Query parameter 'id' has more than one distinct value in the requested URL"} + assert checks == [] + + ALLOWED_ENDPOINTS = [ ( "/allowed", diff --git a/horizon/tests/test_mapping_rules_utils.py b/horizon/tests/test_mapping_rules_utils.py index 85e3f810..3d51f2db 100644 --- a/horizon/tests/test_mapping_rules_utils.py +++ b/horizon/tests/test_mapping_rules_utils.py @@ -4,7 +4,7 @@ from pydantic import AnyHttpUrl, parse_obj_as from horizon.enforcer.schemas import MappingRuleData, UrlTypes -from horizon.enforcer.utils.mapping_rules_utils import MappingRulesUtils +from horizon.enforcer.utils.mapping_rules_utils import ConflictingQueryParameterError, MappingRulesUtils BASE = "https://api.example.com/documents" @@ -66,3 +66,108 @@ def test_a_request_matches_a_rule_whose_query_it_satisfies(rule_query: str, requ ) def test_a_request_does_not_match_a_rule_whose_query_it_lacks(rule_query: str, request_query: str): assert not _matches(BASE + rule_query, BASE + request_query) + + +@pytest.mark.parametrize( + ("rule_query", "request_query"), + [ + ("?id=1", "?id=1&id=1"), + ("?id=1", "?id=%31&id=1"), + ("?id={id}", "?id=7&id=7"), + ("?id=1", "?id=1&page=2&page=3"), + ("", "?id=1&id=2"), + ], + ids=[ + "literal-repeated-same-value", + "literal-same-value-once-decoded", + "attribute-repeated-same-value", + "repeated-param-the-rule-does-not-read", + "rule-reads-no-query", + ], +) +def test_a_query_parameter_repeated_with_one_value_matches_as_if_given_once(rule_query: str, request_query: str): + assert _matches(BASE + rule_query, BASE + request_query) + + +@pytest.mark.parametrize( + ("rule_query", "request_query"), + [ + ("?id=1", "?id=2&id=1"), + ("?id=1", "?id=1&id=2"), + ("?id=1", "?id=1&id="), + ("?id={id}", "?id=1&id=2"), + ("?team=a&id=1", "?team=a&id=1&id=2"), + ], + ids=["last-value-is-the-rules", "first-value-is-the-rules", "other-value-blank", "attribute", "with-other-params"], +) +def test_a_rule_reading_a_query_parameter_with_conflicting_values_raises(rule_query: str, request_query: str): + """Before PER-16927 the last value decided: ?id=2&id=1 matched a rule on id=1 and ?id=1&id=2 did not. + An app reading the first value would then act on a different id than the one the PDP checked.""" + with pytest.raises(ConflictingQueryParameterError) as raised: + _matches(BASE + rule_query, BASE + request_query) + assert raised.value.key == "id" + + +@pytest.mark.parametrize( + ("rule_query", "request_query"), + [ + ("?id=3", "?id=1&id=2"), + ("?team=a&id=1", "?team=b&id=1&id=2"), + ("?team=a&id={id}", "?id=1&id=2"), + ], + ids=["no-value-is-the-rules", "another-param-differs", "another-param-missing"], +) +def test_a_rule_no_value_could_satisfy_does_not_match_even_with_conflicting_values(rule_query: str, request_query: str): + assert not _matches(BASE + rule_query, BASE + request_query) + + +def _rule(url: str, priority: int | None = None) -> MappingRuleData: + return MappingRuleData(url=url, http_method="get", resource="document", action="read", priority=priority) + + +@pytest.mark.parametrize("request_query", ["?admin=true&admin=false", "?admin=false&admin=true"]) +def test_conflicting_values_do_not_fall_through_to_a_rule_that_ignores_the_parameter(request_query: str): + """Skipping only the rule on ?admin=true would hand the request to the catch-all rule, whatever + value the app goes on to read.""" + rules = [_rule(BASE + "?admin=true", priority=10), _rule(BASE, priority=1)] + url = parse_obj_as(AnyHttpUrl, BASE + request_query) + + with pytest.raises(ConflictingQueryParameterError): + MappingRulesUtils.extract_mapping_rule_by_request(rules, "GET", url) + + +def test_conflicting_values_matter_only_for_rules_on_the_requested_path(): + rules = [_rule("https://api.example.com/other?id=1"), _rule(BASE)] + url = parse_obj_as(AnyHttpUrl, BASE + "?id=1&id=2") + + assert MappingRulesUtils.extract_mapping_rule_by_request(rules, "GET", url) is rules[1] + + +@pytest.mark.parametrize( + ("rule_url", "request_url", "expected"), + [ + (BASE + "?id={doc_id}", BASE + "?id=7", {"doc_id": "7"}), + (BASE + "?id={doc_id}", BASE + "?id=7&id=7", {"doc_id": "7"}), + (BASE + "?id={doc_id}", BASE + "?id=7&page=1&page=2", {"doc_id": "7"}), + (BASE + "?id=7", BASE + "?id=7&id=8", {}), + (BASE, BASE + "?id=7&id=8", {}), + (BASE + "?id={doc_id}", BASE, {}), + ], + ids=[ + "single", + "repeated-same-value", + "repeated-param-the-rule-does-not-read", + "rule-reads-no-attribute", + "rule-without-query", + "request-without-query", + ], +) +def test_query_attributes_come_from_the_parameters_value(rule_url: str, request_url: str, expected: dict): + assert MappingRulesUtils.extract_attributes_from_query_params(rule_url, request_url) == expected + + +@pytest.mark.parametrize("request_query", ["?id=7&id=8", "?id=8&id=7", "?id=7&id="]) +def test_query_attributes_raise_for_a_parameter_with_conflicting_values(request_query: str): + with pytest.raises(ConflictingQueryParameterError) as raised: + MappingRulesUtils.extract_attributes_from_query_params(BASE + "?id={doc_id}", BASE + request_query) + assert raised.value.key == "id" From 6af41b19d49521e359c6741a67ac12ae8451d538 Mon Sep 17 00:00:00 2001 From: Zeev Manilovich Date: Thu, 8 Oct 2026 00:01:09 +0300 Subject: [PATCH 03/21] Tidy pdp-server auth middleware (PER-16926) The WARN line for an Authorization header without the Bearer scheme carried the whole header value. It now says only that the scheme is missing. The presented API key is now compared with subtle's ConstantTimeEq instead of `!=`, the same kind of comparison horizon's _token_matches makes with hmac.compare_digest. subtle 2.6.1 is already in Cargo.lock through rustls, so this adds a direct dependency edge but no new crate to the build; it is pinned exactly to the locked version. The test logger now also collects the log lines of the current test thread on request (LogCapture), so a test can check what the middleware logged. New tests cover a lowercase scheme, the right key under another scheme, keys that differ only in length, and the absence of the presented header from every log line on each rejection path. Co-Authored-By: Claude Opus 5.5 --- Cargo.lock | 1 + pdp-server/Cargo.toml | 1 + pdp-server/src/api/authn_middleware.rs | 98 ++++++++++++++++++++++++-- pdp-server/src/test_utils.rs | 70 ++++++++++++++++-- 4 files changed, 159 insertions(+), 11 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 853316ac..eff89b9f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1453,6 +1453,7 @@ dependencies = [ "serde_json", "serde_yaml", "sha2", + "subtle", "tempfile", "thiserror", "tokio", diff --git a/pdp-server/Cargo.toml b/pdp-server/Cargo.toml index 9bf1017e..12ad65ec 100644 --- a/pdp-server/Cargo.toml +++ b/pdp-server/Cargo.toml @@ -22,6 +22,7 @@ serde = { version = "1.0.219", features = ["derive"] } serde_json = "1.0.140" serde_yaml = "0.9" sha2 = "0.11.0" +subtle = "=2.6.1" thiserror = "2.0.12" tokio = { version = "1.44.1", features = ["full"] } url = "2.5.4" diff --git a/pdp-server/src/api/authn_middleware.rs b/pdp-server/src/api/authn_middleware.rs index 83cbf7b8..1d7f7b16 100644 --- a/pdp-server/src/api/authn_middleware.rs +++ b/pdp-server/src/api/authn_middleware.rs @@ -7,6 +7,7 @@ use axum::{ response::Response, }; use log::warn; +use subtle::ConstantTimeEq; pub(super) async fn authentication_middleware( State(state): State, @@ -32,8 +33,8 @@ pub(super) async fn authentication_middleware( // Remove the "Bearer " prefix header_str[7..].to_string() } - Ok(header_str) => { - warn!("Invalid Authorization header format, missing 'Bearer ' prefix: {header_str}"); + Ok(_) => { + warn!("Invalid Authorization header format, missing 'Bearer ' prefix"); return Response::builder() .status(StatusCode::FORBIDDEN) .body( @@ -55,7 +56,11 @@ pub(super) async fn authentication_middleware( }; // Verify the API key - if api_key != state.config.api_key { + let key_matches: bool = api_key + .as_bytes() + .ct_eq(state.config.api_key.as_bytes()) + .into(); + if !key_matches { warn!("Authentication failed: Invalid API key"); return Response::builder() .status(StatusCode::FORBIDDEN) @@ -70,9 +75,10 @@ pub(super) async fn authentication_middleware( #[cfg(test)] mod tests { use super::*; - use crate::test_utils::TestFixture; + use crate::test_utils::{LogCapture, TestFixture}; use axum::routing::get; use axum::Router; + use http::HeaderValue; use http_body_util::BodyExt; use tower::ServiceExt; @@ -97,6 +103,15 @@ mod tests { /// Helper function to build a request with optional authorization header async fn send_request(app: &Router, auth_header: Option<&str>) -> (StatusCode, String) { + let auth_header = + auth_header.map(|auth| HeaderValue::from_str(auth).expect("Invalid header value")); + send_request_with_header(app, auth_header).await + } + + async fn send_request_with_header( + app: &Router, + auth_header: Option, + ) -> (StatusCode, String) { let mut request_builder = Request::builder().uri(TEST_ROUTE); if let Some(auth) = auth_header { @@ -168,4 +183,79 @@ mod tests { "You are not authorized to access this resource, please check your API key." ); } + + #[tokio::test] + async fn test_lowercase_bearer_scheme_is_accepted() { + let app = setup_authn_mock_app("test_api_key").await; + let (status, body) = send_request(&app, Some("bearer test_api_key")).await; + + assert_eq!(status, StatusCode::OK); + assert_eq!(body, "Authenticated"); + } + + #[tokio::test] + async fn test_right_key_with_another_scheme_is_rejected() { + let app = setup_authn_mock_app("test_api_key").await; + let (status, _) = send_request(&app, Some("Basic test_api_key")).await; + + assert_eq!(status, StatusCode::FORBIDDEN); + } + + #[tokio::test] + async fn test_key_that_differs_only_in_length_is_rejected() { + let app = setup_authn_mock_app("test_api_key").await; + + for header in [ + "Bearer ", + "Bearer test_api_ke", + "Bearer test_api_key2", + "Bearer test_api_key ", + ] { + let (status, _) = send_request(&app, Some(header)).await; + assert_eq!(status, StatusCode::FORBIDDEN, "header {header:?}"); + } + } + + /// The Authorization header the caller sent appears in no log line, at any level, whatever + /// the reason the middleware rejects it for. + #[tokio::test] + async fn test_rejected_authorization_header_is_not_logged() { + const PRESENTED: &str = "presented_key_7f3a9c"; + let app = setup_authn_mock_app("test_api_key").await; + let cases = [ + ( + HeaderValue::from_str(&format!("Basic {PRESENTED}")).expect("Invalid header value"), + "Invalid Authorization header format", + ), + ( + HeaderValue::from_str(&format!("Bearer {PRESENTED}")) + .expect("Invalid header value"), + "Authentication failed", + ), + ( + HeaderValue::from_bytes(format!("Bearer {PRESENTED}\u{e9}").as_bytes()) + .expect("Invalid header value"), + "Failed to parse Authorization header", + ), + ]; + + for (header, expected_warning) in cases { + let logs = LogCapture::start(); + + let (status, _) = send_request_with_header(&app, Some(header)).await; + + assert_eq!(status, StatusCode::FORBIDDEN); + let lines = logs.lines(); + assert!( + lines + .iter() + .any(|line| line.starts_with("WARN") && line.contains(expected_warning)), + "expected a WARN line containing {expected_warning:?}, got {lines:?}" + ); + assert!( + lines.iter().all(|line| !line.contains(PRESENTED)), + "a log line repeats the Authorization header: {lines:?}" + ); + } + } } diff --git a/pdp-server/src/test_utils.rs b/pdp-server/src/test_utils.rs index 5bc60e4e..12cd3f9b 100644 --- a/pdp-server/src/test_utils.rs +++ b/pdp-server/src/test_utils.rs @@ -7,6 +7,7 @@ use http::{Method, Request, StatusCode}; use http_body_util::BodyExt; use log::LevelFilter; use serde::{de::DeserializeOwned, Serialize}; +use std::cell::RefCell; use tower::ServiceExt; use wiremock::matchers; use wiremock::Mock; @@ -82,11 +83,7 @@ impl TestFixture { /// } /// ``` pub async fn new() -> Self { - // Initialize test logger - let _ = env_logger::builder() - .filter_level(LevelFilter::Debug) - .is_test(true) - .try_init(); + Self::setup_logger(LevelFilter::Debug); // Create mock servers let opa_mock = MockServer::start().await; @@ -239,10 +236,14 @@ impl TestFixture { /// } /// ``` pub fn setup_logger(level: LevelFilter) { - let _ = env_logger::builder() + let inner = env_logger::builder() .filter_level(level) .is_test(true) - .try_init(); + .build(); + let max_level = inner.filter(); + if log::set_boxed_logger(Box::new(TestLogger { inner })).is_ok() { + log::set_max_level(max_level); + } } /// Creates a request builder with pre-configured headers. @@ -707,3 +708,58 @@ impl TestResponse { String::from_utf8_lossy(&self.body).to_string() } } + +thread_local! { + static CAPTURED_LOG_LINES: RefCell>> = const { RefCell::new(None) }; +} + +/// The logger [`TestFixture::setup_logger`] installs: env_logger, which prints through the test +/// harness's output capture, plus the per-thread collection behind [`LogCapture`]. +struct TestLogger { + inner: env_logger::Logger, +} + +impl log::Log for TestLogger { + fn enabled(&self, metadata: &log::Metadata) -> bool { + self.inner.enabled(metadata) + } + + fn log(&self, record: &log::Record) { + CAPTURED_LOG_LINES.with_borrow_mut(|lines| { + if let Some(lines) = lines { + lines.push(format!("{} {}", record.level(), record.args())); + } + }); + self.inner.log(record); + } + + fn flush(&self) { + self.inner.flush(); + } +} + +/// Collects the log lines emitted on the current thread, from [`LogCapture::start`] until it is +/// dropped. +/// +/// `#[tokio::test]` runs on a single-threaded runtime, so a request a test sends through a router +/// with `oneshot` is handled on the test's thread and its log lines are the test's alone. +pub struct LogCapture; + +impl LogCapture { + pub fn start() -> Self { + TestFixture::setup_logger(LevelFilter::Debug); + CAPTURED_LOG_LINES.set(Some(Vec::new())); + Self + } + + /// Each line collected so far, as "LEVEL message". + pub fn lines(&self) -> Vec { + CAPTURED_LOG_LINES.with_borrow(|lines| lines.clone().unwrap_or_default()) + } +} + +impl Drop for LogCapture { + fn drop(&mut self) { + CAPTURED_LOG_LINES.set(None); + } +} From ee1f3ba451d20a8674cc484b9fd3e3e4e2964436 Mon Sep 17 00:00:00 2001 From: Zeev Manilovich Date: Thu, 8 Oct 2026 00:01:18 +0300 Subject: [PATCH 04/21] Drop the Authorization value from a 401 detail (PER-16926) extract_pdp_api_key answered a header that does not split into a scheme and a token with 401 "bad authz header: ". The detail is now just "bad authz header"; the caller already knows what it sent. The route-level 401 tests for malformed headers now also check that the response does not repeat the header. Co-Authored-By: Claude Opus 5.5 --- horizon/enforcer/api.py | 5 +---- horizon/tests/test_enforcer_api.py | 20 ++++++++++++++++++-- 2 files changed, 19 insertions(+), 6 deletions(-) diff --git a/horizon/enforcer/api.py b/horizon/enforcer/api.py index 7380296b..0fa6c761 100644 --- a/horizon/enforcer/api.py +++ b/horizon/enforcer/api.py @@ -67,10 +67,7 @@ def extract_pdp_api_key(request: Request) -> str: authorization: str = request.headers.get(AUTHZ_HEADER, "") parts = authorization.split(" ") if len(parts) != AUTHZ_HEADER_PARTS: - raise HTTPException( - status.HTTP_401_UNAUTHORIZED, - detail=f"bad authz header: {authorization}", - ) + raise HTTPException(status.HTTP_401_UNAUTHORIZED, detail="bad authz header") schema, token = parts if schema.strip().lower() != "bearer": raise HTTPException(status.HTTP_401_UNAUTHORIZED, detail="Invalid PDP token") diff --git a/horizon/tests/test_enforcer_api.py b/horizon/tests/test_enforcer_api.py index c6a113ae..3d065c3a 100644 --- a/horizon/tests/test_enforcer_api.py +++ b/horizon/tests/test_enforcer_api.py @@ -9,7 +9,7 @@ import httpx import pytest from aioresponses import aioresponses -from fastapi import FastAPI, Response +from fastapi import FastAPI, HTTPException, Request, Response from fastapi.testclient import TestClient from loguru import logger from opal_client.client import OpalClient @@ -17,7 +17,7 @@ from starlette import status from horizon.config import sidecar_config -from horizon.enforcer.api import log_query_result, log_query_result_kong, stats_manager +from horizon.enforcer.api import extract_pdp_api_key, log_query_result, log_query_result_kong, stats_manager from horizon.enforcer.schemas import ( AuthorizationQuery, Resource, @@ -134,6 +134,22 @@ def test_enforcer_endpoint_malformed_header_is_401_not_500(endpoint, value): client = TestClient(sidecar._app) response = client.post(endpoint, headers={"authorization": value}, json={}) assert response.status_code == status.HTTP_401_UNAUTHORIZED + assert value.strip() not in response.text + + +@pytest.mark.parametrize( + "authorization", + [f"Bearer {sidecar_config.API_KEY} trailing-part", sidecar_config.API_KEY], + ids=["three-parts", "no-scheme"], +) +def test_extract_pdp_api_key_401_does_not_repeat_the_header(authorization: str): + request = Request({"type": "http", "headers": [(b"authorization", authorization.encode())]}) + + with pytest.raises(HTTPException) as raised: + extract_pdp_api_key(request) + + assert raised.value.status_code == status.HTTP_401_UNAUTHORIZED + assert raised.value.detail == "bad authz header" def test_health_endpoint_is_public(monkeypatch): From 35f16e520a23700ecb656a9b953f523b74c52d4a Mon Sep 17 00:00:00 2001 From: Zeev Manilovich Date: Thu, 8 Oct 2026 00:19:54 +0300 Subject: [PATCH 05/21] Let a higher-priority mapping rule decide over a conflict (PER-16927) A query parameter with more than one distinct value stopped the rule search as soon as any candidate rule read it, so /allowed_url denied requests that a higher-priority rule matches cleanly. For example, with a priority-10 rule on ?id={id} and a priority-1 rule on ?tag=x, the request ?id=1&tag=x&tag=y was denied, although the id rule comes first whichever tag value the app reads. Each candidate rule is now kept with its conflict, if any. The rules are sorted by priority as before (the sort is stable, so equal priorities keep their order), and only a conflict in the rule that comes first makes /allowed_url answer not allowed. Also reword a test docstring to describe the current behaviour. Co-Authored-By: Claude Opus 5.5 --- horizon/enforcer/utils/mapping_rules_utils.py | 37 +++++++++----- horizon/tests/test_enforcer_api.py | 19 +++++++ horizon/tests/test_mapping_rules_utils.py | 51 ++++++++++++++++++- 3 files changed, 91 insertions(+), 16 deletions(-) diff --git a/horizon/enforcer/utils/mapping_rules_utils.py b/horizon/enforcer/utils/mapping_rules_utils.py index e0fc7886..145dfeb0 100644 --- a/horizon/enforcer/utils/mapping_rules_utils.py +++ b/horizon/enforcer/utils/mapping_rules_utils.py @@ -140,12 +140,16 @@ def extract_mapping_rule_by_request( ) -> MappingRuleData | None: """The highest-priority mapping rule for the request's method and URL, or None if none matches. + Rules of equal priority keep their order in ``mapping_rules``. + Raises: - ConflictingQueryParameterError: a rule for the method and path could match, but a query + ConflictingQueryParameterError: the rule that would come first could match, but a query parameter it reads has more than one distinct value. The request then gets no rule at - all, rather than a lower-priority one that skips the parameter. + all, rather than a lower-priority one that skips the parameter. A conflict in a rule + that a matching rule outranks does not matter: that rule comes first whatever value + is read. """ - matched_mapping_rules = [] + candidates: list[tuple[MappingRuleData, ConflictingQueryParameterError | None]] = [] http_method = http_method.lower() # Convert once instead of in each iteration for mapping_rule in mapping_rules: @@ -166,14 +170,19 @@ def extract_mapping_rule_by_request( # if the method is not the same, we don't need to check the url continue - if not cls._compare_urls(mapping_rule.url, url, is_regex=is_regex): - continue - - matched_mapping_rules.append(mapping_rule) - - # most priority first - matched_mapping_rules.sort(key=lambda rule: rule.priority or 0, reverse=True) - if len(matched_mapping_rules) > 0: - return matched_mapping_rules[0] - - return None + try: + if not cls._compare_urls(mapping_rule.url, url, is_regex=is_regex): + continue + except ConflictingQueryParameterError as conflict: + candidates.append((mapping_rule, conflict)) + else: + candidates.append((mapping_rule, None)) + + if not candidates: + return None + # most priority first; the sort is stable, so equal priorities keep the rules' order + candidates.sort(key=lambda candidate: candidate[0].priority or 0, reverse=True) + first_rule, conflict = candidates[0] + if conflict is not None: + raise conflict + return first_rule diff --git a/horizon/tests/test_enforcer_api.py b/horizon/tests/test_enforcer_api.py index 3d065c3a..2874363f 100644 --- a/horizon/tests/test_enforcer_api.py +++ b/horizon/tests/test_enforcer_api.py @@ -442,6 +442,25 @@ def test_allowed_url_with_conflicting_values_for_a_rule_query_parameter_is_not_a assert checks == [] +@pytest.mark.parametrize("tag_rule_first", [True, False], ids=["tag-rule-listed-first", "tag-rule-listed-last"]) +def test_allowed_url_checks_a_higher_priority_rule_that_reads_no_conflicting_parameter( + monkeypatch, *, tag_rule_first: bool +): + """tag=x&tag=y could select the lower-priority tag rule, but the id rule comes first either way.""" + tag_rule = {"url": DOCUMENTS_URL + "?tag=x", "http_method": "get", "action": "tag", "resource": "document"} + mapping_rules = [{**DOC_ID_RULE, "priority": 10}, {**tag_rule, "priority": 1}] + if tag_rule_first: + mapping_rules.reverse() + + response, checks = _post_allowed_url(monkeypatch, DOCUMENTS_URL + "?id=7&tag=x&tag=y", mapping_rules) + + assert response.status_code == status.HTTP_200_OK + assert response.json()["allow"] is True + (check,) = checks + assert check["action"] == "read" + assert check["resource"]["attributes"] == {"doc_id": "7"} + + ALLOWED_ENDPOINTS = [ ( "/allowed", diff --git a/horizon/tests/test_mapping_rules_utils.py b/horizon/tests/test_mapping_rules_utils.py index 3d51f2db..ac49134f 100644 --- a/horizon/tests/test_mapping_rules_utils.py +++ b/horizon/tests/test_mapping_rules_utils.py @@ -101,8 +101,8 @@ def test_a_query_parameter_repeated_with_one_value_matches_as_if_given_once(rule ids=["last-value-is-the-rules", "first-value-is-the-rules", "other-value-blank", "attribute", "with-other-params"], ) def test_a_rule_reading_a_query_parameter_with_conflicting_values_raises(rule_query: str, request_query: str): - """Before PER-16927 the last value decided: ?id=2&id=1 matched a rule on id=1 and ?id=1&id=2 did not. - An app reading the first value would then act on a different id than the one the PDP checked.""" + """The answer is the same whichever order the values come in: the app behind the URL may read + either one, so neither reading decides whether the rule applies.""" with pytest.raises(ConflictingQueryParameterError) as raised: _matches(BASE + rule_query, BASE + request_query) assert raised.value.key == "id" @@ -136,6 +136,53 @@ def test_conflicting_values_do_not_fall_through_to_a_rule_that_ignores_the_param MappingRulesUtils.extract_mapping_rule_by_request(rules, "GET", url) +TAG_X_RULE = _rule(BASE + "?tag=x") +ID_RULE = _rule(BASE + "?id={id}") +TAGGED_REQUEST = BASE + "?id=1&tag=x&tag=y" + + +@pytest.mark.parametrize( + ("rules", "expected_rule"), + [ + ([_rule(ID_RULE.url, priority=10), _rule(TAG_X_RULE.url, priority=1)], 0), + ([_rule(TAG_X_RULE.url, priority=1), _rule(ID_RULE.url, priority=10)], 1), + ([ID_RULE, TAG_X_RULE], 0), + ], + ids=[ + "clean-rule-ranks-higher-listed-first", + "clean-rule-ranks-higher-listed-last", + "equal-priority-clean-listed-first", + ], +) +def test_a_rule_without_conflicts_that_comes_first_decides_whatever_value_a_lower_rule_reads( + rules: list[MappingRuleData], expected_rule: int +): + """tag=x&tag=y could select the tag rule, but the id rule comes first under either reading.""" + url = parse_obj_as(AnyHttpUrl, TAGGED_REQUEST) + + assert MappingRulesUtils.extract_mapping_rule_by_request(rules, "GET", url) is rules[expected_rule] + + +@pytest.mark.parametrize( + "rules", + [ + [_rule(TAG_X_RULE.url, priority=10), _rule(ID_RULE.url, priority=1)], + [_rule(ID_RULE.url, priority=1), _rule(TAG_X_RULE.url, priority=10)], + [TAG_X_RULE, ID_RULE], + ], + ids=["conflicting-rule-ranks-higher-listed-first", "conflicting-rule-ranks-higher-listed-last", "equal-priority"], +) +def test_a_rule_with_conflicting_values_that_comes_first_raises_over_a_rule_without_conflicts( + rules: list[MappingRuleData], +): + """Read as tag=x the tag rule wins, read as tag=y the id rule does: the reading decides.""" + url = parse_obj_as(AnyHttpUrl, TAGGED_REQUEST) + + with pytest.raises(ConflictingQueryParameterError) as raised: + MappingRulesUtils.extract_mapping_rule_by_request(rules, "GET", url) + assert raised.value.key == "tag" + + def test_conflicting_values_matter_only_for_rules_on_the_requested_path(): rules = [_rule("https://api.example.com/other?id=1"), _rule(BASE)] url = parse_obj_as(AnyHttpUrl, BASE + "?id=1&id=2") From 3d617322ec489f63baefe8ac357df25ef7fc34da Mon Sep 17 00:00:00 2001 From: Zeev Manilovich Date: Thu, 8 Oct 2026 00:20:51 +0300 Subject: [PATCH 06/21] Read query attributes from the query the rule matched (PER-16927) Rule matching takes everything after the first "?" as the query, but attribute extraction cut the URL at every "?". A "?" inside a query value made the two read different queries: ?z=1?id=5&id=7 matched a ?id={id} rule and then failed extraction with a KeyError (a 500 from /allowed_url), and ?id=1?id=2 matched with id "1?id=2" but was checked as id "1". Split once in extraction too, so it reads exactly the query the matcher read. Co-Authored-By: Claude Opus 5.5 --- horizon/enforcer/utils/mapping_rules_utils.py | 4 ++-- horizon/tests/test_enforcer_api.py | 6 +++++- horizon/tests/test_mapping_rules_utils.py | 13 +++++++++++++ 3 files changed, 20 insertions(+), 3 deletions(-) diff --git a/horizon/enforcer/utils/mapping_rules_utils.py b/horizon/enforcer/utils/mapping_rules_utils.py index 145dfeb0..cfbf45ca 100644 --- a/horizon/enforcer/utils/mapping_rules_utils.py +++ b/horizon/enforcer/utils/mapping_rules_utils.py @@ -103,8 +103,8 @@ def extract_attributes_from_query_params(rule_url: str, request_url: str) -> dic """ if "?" not in rule_url or "?" not in request_url: return {} - rule_query_params = QueryParams(rule_url.split("?")[1]) - request_query_params = QueryParams(request_url.split("?")[1]) + rule_query_params = QueryParams(rule_url.split("?", 1)[1]) + request_query_params = QueryParams(request_url.split("?", 1)[1]) attributes = {} for key in rule_query_params: if rule_query_params[key].startswith("{") and rule_query_params[key].endswith("}"): diff --git a/horizon/tests/test_enforcer_api.py b/horizon/tests/test_enforcer_api.py index 2874363f..9879738c 100644 --- a/horizon/tests/test_enforcer_api.py +++ b/horizon/tests/test_enforcer_api.py @@ -414,7 +414,11 @@ def _post_allowed_url(monkeypatch, url: str, mapping_rules: list[dict]) -> tuple return response, checks -@pytest.mark.parametrize("query_string", ["?id=7", "?id=7&id=7"], ids=["single", "repeated-same-value"]) +@pytest.mark.parametrize( + "query_string", + ["?id=7", "?id=7&id=7", "?z=1?id=5&id=7"], + ids=["single", "repeated-same-value", "question-mark-in-another-value"], +) def test_allowed_url_checks_the_value_of_the_query_parameter_the_rule_reads(monkeypatch, query_string: str): response, checks = _post_allowed_url(monkeypatch, DOCUMENTS_URL + query_string, [DOC_ID_RULE]) diff --git a/horizon/tests/test_mapping_rules_utils.py b/horizon/tests/test_mapping_rules_utils.py index ac49134f..e2b1af5d 100644 --- a/horizon/tests/test_mapping_rules_utils.py +++ b/horizon/tests/test_mapping_rules_utils.py @@ -213,6 +213,19 @@ def test_query_attributes_come_from_the_parameters_value(rule_url: str, request_ assert MappingRulesUtils.extract_attributes_from_query_params(rule_url, request_url) == expected +@pytest.mark.parametrize( + ("request_query", "expected"), + [("?z=1?id=5&id=7", {"doc_id": "7"}), ("?id=1?id=2", {"doc_id": "1?id=2"})], + ids=["question-mark-in-another-value", "question-mark-in-the-value"], +) +def test_query_attributes_come_from_the_query_the_rule_matched(request_query: str, expected: dict): + """Everything after the first "?" is the query, both for matching a rule and for reading its attributes.""" + rule_url = BASE + "?id={doc_id}" + + assert _matches(rule_url, BASE + request_query) + assert MappingRulesUtils.extract_attributes_from_query_params(rule_url, BASE + request_query) == expected + + @pytest.mark.parametrize("request_query", ["?id=7&id=8", "?id=8&id=7", "?id=7&id="]) def test_query_attributes_raise_for_a_parameter_with_conflicting_values(request_query: str): with pytest.raises(ConflictingQueryParameterError) as raised: From 135b31ce51e85afdd402e35c18cd807df8e0e74d Mon Sep 17 00:00:00 2001 From: Zeev Manilovich Date: Thu, 8 Oct 2026 00:24:26 +0300 Subject: [PATCH 07/21] Apply the repeated-parameter rule to regex mapping rules (PER-16927) A regex mapping rule is matched against the raw request URL, so for a repeated query parameter it matched or captured whichever occurrence the pattern reached: a rule on \?id=1 matched ?id=1&id=2 but not ?id=2&id=1, and a named group on id captured the first value. Which parameters a pattern reads cannot be known, so when a parameter has more than one distinct value the rule is also matched against the URL as an app reading the first value, and as one reading the last value, of each such parameter. If the match or a named group differs, the rule is a conflict and goes through the same priority handling as default rules: /allowed_url answers not allowed only when that rule comes first. A pattern that gives the same answer for every reading, such as a catch-all on the path, matches as before. The extra work is two more matches per regex rule on URLs no longer than the original, and only for URLs with such a parameter. Co-Authored-By: Claude Opus 5.5 --- horizon/enforcer/utils/mapping_rules_utils.py | 67 ++++++++++++++-- horizon/tests/test_enforcer_api.py | 31 ++++++++ horizon/tests/test_mapping_rules_utils.py | 78 +++++++++++++++++++ 3 files changed, 170 insertions(+), 6 deletions(-) diff --git a/horizon/enforcer/utils/mapping_rules_utils.py b/horizon/enforcer/utils/mapping_rules_utils.py index cfbf45ca..5b237aff 100644 --- a/horizon/enforcer/utils/mapping_rules_utils.py +++ b/horizon/enforcer/utils/mapping_rules_utils.py @@ -1,6 +1,7 @@ # TODO: change to use re2 in the future, currently not supported in alpine due to c++ library issues # import re2 as re # use re2 instead of re for regex matching because it's simiplier and safer for user inputted regexes # noqa: ERA001,E501 import re +from urllib.parse import parse_qsl from loguru import logger from pydantic import AnyHttpUrl @@ -13,7 +14,8 @@ class ConflictingQueryParameterError(ValueError): """A query parameter a mapping rule reads has more than one distinct value in the requested URL. Which value the protected app reads for such a parameter is up to the app, so the PDP cannot - tell which rule applies or which attribute value to check. + tell which rule applies or which attribute value to check. For a regex rule, the parameter is one + whose value can change whether the rule matches or what its named groups capture. """ def __init__(self, key: str): @@ -115,8 +117,16 @@ def extract_attributes_from_query_params(rule_url: str, request_url: str) -> dic @classmethod def _compare_urls(cls, mapping_rule_url: str, request_url: str, *, is_regex: bool = False) -> bool: - """ - Compare a mapping rule URL against a request URL. + """Whether the request URL matches the mapping rule URL. + + A regex rule is matched against the raw URL, so which query parameters it reads is unknown. When + a parameter has more than one distinct value, the rule is also matched against the URL as an app + reading the first value and as one reading the last value of each such parameter. + + Raises: + ConflictingQueryParameterError: the rule could match, but whether it does, or what it reads, + depends on which value of a repeated query parameter is read. For a regex rule the error + names the first parameter with more than one distinct value. """ # If the mapping rule is a regex pattern if is_regex: @@ -125,12 +135,57 @@ def _compare_urls(cls, mapping_rule_url: str, request_url: str, *, is_regex: boo except re.error as e: logger.warning("regex pattern compilation failed", pattern=mapping_rule_url, error=str(e)) return False - match_result = bool(pattern.match(request_url)) - logger.debug("regex url comparison", pattern=mapping_rule_url, url=request_url, matched=match_result) - return match_result + groups = cls._regex_match_groups(pattern, request_url) + logger.debug("regex url comparison", pattern=mapping_rule_url, url=request_url, matched=groups is not None) + readings = cls._first_and_last_value_readings(request_url) + if readings is not None: + key, reading_urls = readings + if any(cls._regex_match_groups(pattern, reading_url) != groups for reading_url in reading_urls): + raise ConflictingQueryParameterError(key) + return groups is not None return cls._compare_httpurls(mapping_rule_url, request_url) + @staticmethod + def _regex_match_groups(pattern: re.Pattern[str], url: str) -> dict[str, str | None] | None: + """The named groups of the pattern's match at the start of the URL, or None if it does not match.""" + match = pattern.match(url) + return match.groupdict() if match else None + + @staticmethod + def _first_and_last_value_readings(request_url: str) -> tuple[str, tuple[str, str]] | None: + """The request URL as an app reading the first, and as one reading the last, value of each query + parameter that has more than one distinct value. + + Each reading drops the occurrences of such a parameter that carry another value and keeps the + rest of the URL as it was, so a pattern sees the same characters as in the original. + + Returns: + The first parameter with more than one distinct value and the two readings, or None if no + parameter has more than one distinct value. + """ + path, separator, query = request_url.partition("?") + if not separator: + return None + segments = [(segment, parse_qsl(segment, keep_blank_values=True)) for segment in query.split("&")] + values_by_key: dict[str, list[str]] = {} + for _, pairs in segments: + for key, value in pairs: + values_by_key.setdefault(key, []).append(value) + kept_values = {key: (values[0], values[-1]) for key, values in values_by_key.items() if len(set(values)) > 1} + if not kept_values: + return None + + def reading(position: int) -> str: + kept_segments = [ + segment + for segment, pairs in segments + if all(key not in kept_values or value == kept_values[key][position] for key, value in pairs) + ] + return f"{path}?{'&'.join(kept_segments)}" + + return next(iter(kept_values)), (reading(0), reading(1)) + @classmethod def extract_mapping_rule_by_request( cls, diff --git a/horizon/tests/test_enforcer_api.py b/horizon/tests/test_enforcer_api.py index 9879738c..4c012693 100644 --- a/horizon/tests/test_enforcer_api.py +++ b/horizon/tests/test_enforcer_api.py @@ -465,6 +465,37 @@ def test_allowed_url_checks_a_higher_priority_rule_that_reads_no_conflicting_par assert check["resource"]["attributes"] == {"doc_id": "7"} +DOC_ID_REGEX_RULE = { + "url": r"^https://api\.example\.com/documents\?id=(?P\d+)", + "url_type": "regex", + "http_method": "get", + "action": "read", + "resource": "document", +} + + +def test_allowed_url_checks_the_value_a_regex_rule_captures(monkeypatch): + response, checks = _post_allowed_url(monkeypatch, DOCUMENTS_URL + "?id=7&tag=a&tag=b", [DOC_ID_REGEX_RULE]) + + assert response.status_code == status.HTTP_200_OK + assert response.json()["allow"] is True + (check,) = checks + assert check["resource"]["attributes"] == {"doc_id": "7"} + + +@pytest.mark.parametrize( + "query_string", ["?id=7&id=8", "?id=8&id=7"], ids=["captured-value-first", "captured-value-last"] +) +def test_allowed_url_with_conflicting_values_for_a_regex_rule_is_not_allowed(monkeypatch, query_string: str): + response, checks = _post_allowed_url(monkeypatch, DOCUMENTS_URL + query_string, [DOC_ID_REGEX_RULE]) + + assert response.status_code == status.HTTP_200_OK + body = response.json() + assert body["allow"] is False + assert body["debug"] == {"reason": "Query parameter 'id' has more than one distinct value in the requested URL"} + assert checks == [] + + ALLOWED_ENDPOINTS = [ ( "/allowed", diff --git a/horizon/tests/test_mapping_rules_utils.py b/horizon/tests/test_mapping_rules_utils.py index e2b1af5d..b86b9770 100644 --- a/horizon/tests/test_mapping_rules_utils.py +++ b/horizon/tests/test_mapping_rules_utils.py @@ -190,6 +190,84 @@ def test_conflicting_values_matter_only_for_rules_on_the_requested_path(): assert MappingRulesUtils.extract_mapping_rule_by_request(rules, "GET", url) is rules[1] +REGEX_BASE = r"^https://api\.example\.com/documents" + + +@pytest.mark.parametrize( + ("rule_regex", "request_query", "key"), + [ + (REGEX_BASE + r"\?id=1", "?id=1&id=2", "id"), + (REGEX_BASE + r"\?id=1", "?id=2&id=1", "id"), + (REGEX_BASE + r"\?id=(?P\d+)", "?id=1&id=2", "id"), + (REGEX_BASE + r".*[?&]admin=true", "?admin=false&admin=true", "admin"), + (REGEX_BASE + r".*[?&]admin=true", "?admin=true&page=1&admin=false", "admin"), + (REGEX_BASE + r".*[?&]id=(?P\d+)", "?tag=a&tag=b&id=1&id=2", "tag"), + ], + ids=[ + "literal-value-first", + "literal-value-last", + "named-group", + "anywhere-value-last", + "anywhere-value-first", + "names-the-first-repeated-param", + ], +) +def test_a_regex_rule_whose_answer_depends_on_the_value_read_raises(rule_regex: str, request_query: str, key: str): + """A pattern is matched against the raw URL, so the PDP cannot tell which parameters it reads. It + tries the readings of an app taking the first and one taking the last value of each repeated + parameter, and a difference in the match or in a named group is a conflict.""" + with pytest.raises(ConflictingQueryParameterError) as raised: + _matches(rule_regex, BASE + request_query, UrlTypes.REGEX) + assert raised.value.key == key + + +@pytest.mark.parametrize( + ("rule_regex", "request_query", "expected"), + [ + (REGEX_BASE, "?tag=a&tag=b", True), + (REGEX_BASE + r"\?id=1", "?id=1&id=1", True), + (REGEX_BASE + r"\?id=(?P\d+)", "?id=1&tag=a&tag=b", True), + (REGEX_BASE + r"\?id=1", "?id=1&id=2&id=1", True), + (REGEX_BASE + "$", "?tag=a&tag=b", False), + (r"^https://api\.example\.com/other", "?id=1&id=2", False), + ], + ids=[ + "catch-all", + "repeated-same-value", + "repeated-param-the-pattern-does-not-read", + "first-and-last-values-agree", + "pattern-ends-before-the-query", + "another-path", + ], +) +def test_a_regex_rule_whose_answer_is_the_same_for_every_value_read_matches_as_before( + rule_regex: str, request_query: str, *, expected: bool +): + assert _matches(rule_regex, BASE + request_query, UrlTypes.REGEX) is expected + + +@pytest.mark.parametrize(("regex_priority", "conflict"), [(1, False), (20, True)], ids=["ranks-lower", "ranks-higher"]) +def test_a_regex_rule_with_conflicting_values_matters_only_when_it_comes_first(regex_priority: int, *, conflict: bool): + rules = [ + MappingRuleData( + url=REGEX_BASE + r".*[?&]tag=x", + http_method="get", + resource="document", + action="tag", + url_type=UrlTypes.REGEX, + priority=regex_priority, + ), + _rule(ID_RULE.url, priority=10), + ] + url = parse_obj_as(AnyHttpUrl, TAGGED_REQUEST) + + if conflict: + with pytest.raises(ConflictingQueryParameterError): + MappingRulesUtils.extract_mapping_rule_by_request(rules, "GET", url) + else: + assert MappingRulesUtils.extract_mapping_rule_by_request(rules, "GET", url) is rules[1] + + @pytest.mark.parametrize( ("rule_url", "request_url", "expected"), [ From bbfe2a6cb6d9cb6a1cf7d116cc1dc3e3af72d3c1 Mon Sep 17 00:00:00 2001 From: Zeev Manilovich Date: Thu, 8 Oct 2026 00:27:09 +0300 Subject: [PATCH 08/21] Capture every log level in the pdp-server test log capture LogCapture installs the test logger at Debug, but the logger can be installed only once per test process. When a test that asks for Info runs first, as the health handler tests do, log::max_level() stays at Info and the log macros drop debug and trace records before the capture sees them. A test asserting that no line, at any level, repeats a value would then miss a debug line. LogCapture::start now raises the global maximum to Trace. env_logger keeps filtering what it prints with the level it was built with, so the printed output of other tests does not change. Co-Authored-By: Claude Opus 5.5 --- pdp-server/src/test_utils.rs | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/pdp-server/src/test_utils.rs b/pdp-server/src/test_utils.rs index 12cd3f9b..6e66bac7 100644 --- a/pdp-server/src/test_utils.rs +++ b/pdp-server/src/test_utils.rs @@ -746,8 +746,11 @@ impl log::Log for TestLogger { pub struct LogCapture; impl LogCapture { + /// Starts collecting at every level, whatever level the logger was installed with. That level + /// still filters what env_logger prints, so other tests' output does not change. pub fn start() -> Self { TestFixture::setup_logger(LevelFilter::Debug); + log::set_max_level(LevelFilter::Trace); CAPTURED_LOG_LINES.set(Some(Vec::new())); Self } @@ -763,3 +766,22 @@ impl Drop for LogCapture { CAPTURED_LOG_LINES.set(None); } } + +#[cfg(test)] +mod tests { + use super::*; + + /// Another test may have installed the logger first with a higher level, as the health + /// handler tests do with `Info`; a capture still sees every level. + #[test] + fn test_log_capture_sees_every_level_whatever_logger_came_first() { + TestFixture::setup_logger(LevelFilter::Info); + log::set_max_level(LevelFilter::Info); + + let logs = LogCapture::start(); + log::debug!("debug line"); + log::trace!("trace line"); + + assert_eq!(logs.lines(), ["DEBUG debug line", "TRACE trace line"]); + } +} From 30200a2899d538f7df8b13567173a12c693c5985 Mon Sep 17 00:00:00 2001 From: Zeev Manilovich Date: Thu, 8 Oct 2026 00:27:09 +0300 Subject: [PATCH 09/21] Use a caret requirement for subtle like the other dependencies subtle was already in Cargo.lock through rustls, and the image builds with --locked, so the lock already fixes the version. An exact "=2.6.1" requirement would also stop cargo from resolving a future rustls or reqwest release that needs a newer subtle 2.x, since one lock holds one semver-compatible version of a crate. Cargo.lock does not change. Co-Authored-By: Claude Opus 5.5 --- pdp-server/Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pdp-server/Cargo.toml b/pdp-server/Cargo.toml index 12ad65ec..7dcc1f35 100644 --- a/pdp-server/Cargo.toml +++ b/pdp-server/Cargo.toml @@ -22,7 +22,7 @@ serde = { version = "1.0.219", features = ["derive"] } serde_json = "1.0.140" serde_yaml = "0.9" sha2 = "0.11.0" -subtle = "=2.6.1" +subtle = "2.6.1" thiserror = "2.0.12" tokio = { version = "1.44.1", features = ["full"] } url = "2.5.4" From c68a9694c55ae3715bd71e83e044c954327ac024 Mon Sep 17 00:00:00 2001 From: Zeev Manilovich Date: Thu, 8 Oct 2026 00:57:41 +0300 Subject: [PATCH 10/21] Pin permit-opa with the derivation fix (PER-16921) /user-permissions dropped ReBAC-derived resource instances after any update to a resource type that other types derive roles from (#341): the permit_rebac.all_roles index removed the child types' derivation rules when the parent type changed, and kept them out until a full data reload. /authorized_users had a related gap when a type gained its first derived role. Both are fixed in permit-opa. The same permit-opa change also builds the updated derivation index as a copy and swaps it in, instead of editing the map a running /user-permissions call may be reading. This moves the pin to that commit. The new commit sits on permit-opa main, so the pin also brings in permitio/permit-opa#53-#57 (query_engine builtins and tests): no version changes; one new test-only indirect module. The commit is not merged yet; re-pin to the merge commit once it is. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/release.yml | 2 +- .github/workflows/tests.yml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 88442af5..735982c3 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -82,7 +82,7 @@ jobs: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: repository: permitio/permit-opa - ref: ff2356d560e3c7152bcb6c6ab8ebd5d4eb691306 # permit-opa 0.0.23 (#52; go 1.26, x/crypto v0.57.0) + ref: 2f411da9b486226a0bfe4c1e39344eaa885553a1 # permit-opa main (permitio/permit-opa#53-#57) + ReBAC derivation fixes (PER-16921); re-pin to the merge commit path: './permit-opa' token: ${{ steps.permit-opa-token.outputs.token }} # Nothing after the clone needs the token; don't leave it in .git/config. diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 8c1326ec..b5a3ca38 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -163,7 +163,7 @@ jobs: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: repository: permitio/permit-opa - ref: ff2356d560e3c7152bcb6c6ab8ebd5d4eb691306 # permit-opa 0.0.23 (#52; go 1.26, x/crypto v0.57.0) + ref: 2f411da9b486226a0bfe4c1e39344eaa885553a1 # permit-opa main (permitio/permit-opa#53-#57) + ReBAC derivation fixes (PER-16921); re-pin to the merge commit path: './permit-opa' token: ${{ steps.permit-opa-token.outputs.token }} # Nothing after the clone needs the token; don't leave it in .git/config. From 6cfc64736057bf5c5d2162ca2c93a2a52878ca5d Mon Sep 17 00:00:00 2001 From: Zeev Manilovich Date: Thu, 8 Oct 2026 00:58:15 +0300 Subject: [PATCH 11/21] Write OPA's config file when plugins are set (PER-12470) With PDP_OPA_DECISION_LOG_ENABLED=false the PDP never wrote OPA's config file, and that file is the only place PDP_OPA_PLUGINS reach OPA. The permit_graph plugin then never loaded, and ReBAC user permissions came back empty. Console decision logs (PDP_OPA_DECISION_LOG_CONSOLE, which the Helm chart's logs_forwarder turns on) are set in the same file and were dropped the same way. - The config file is written, and OPA pointed at it through the inline OPA config, when decision log uploads, console decision logs or any OPA plugin is on. - The permit_io service, with the API key it authenticates with, is written only when decision log uploads are on: OPA uses it to upload them and for nothing else. It does not depend on PDP_OPA_BEARER_TOKEN_REQUIRED, which is about callers of OPA. Console decision logs no longer need uploads on. - A different config_file already set in OPAL_INLINE_OPA_CONFIG is still replaced, as it was with decision logs on, and now a warning names both files. - With none of these on and no bearer token required, the inline OPA config is left as it is. Re-implements #274 by omer9564, which tied the service credentials to PDP_OPA_BEARER_TOKEN_REQUIRED instead. Co-authored-by: Omer Zuarets <42326891+omer9564@users.noreply.github.com> Co-Authored-By: Claude Opus 5.5 --- horizon/config.py | 4 +- horizon/enforcer/opa/config_maker.py | 16 +- horizon/pdp.py | 34 ++- horizon/static/templates/config.yaml.template | 8 +- horizon/tests/test_opa_config.py | 220 ++++++++++++++++++ 5 files changed, 268 insertions(+), 14 deletions(-) create mode 100644 horizon/tests/test_opa_config.py diff --git a/horizon/config.py b/horizon/config.py index 3e86f271..b5db6323 100644 --- a/horizon/config.py +++ b/horizon/config.py @@ -250,8 +250,8 @@ def __new__(cls, *, prefix=None, is_model=True): # noqa: ARG004 OPA_DECISION_LOG_CONSOLE = confi.bool( "OPA_DECISION_LOG_CONSOLE", False, - description="if true, OPA decision logs will also be printed to console " - "(only relevant if `OPA_DECISION_LOG_ENABLED` is true)", + description="if true, OPA decision logs will be printed to console, " + "whether or not `OPA_DECISION_LOG_ENABLED` uploads them", ) OPA_DECISION_LOG_INGRESS_ROUTE = confi.str( "OPA_DECISION_LOG_INGRESS_ROUTE", diff --git a/horizon/enforcer/opa/config_maker.py b/horizon/enforcer/opa/config_maker.py index 76052a0a..478bac21 100644 --- a/horizon/enforcer/opa/config_maker.py +++ b/horizon/enforcer/opa/config_maker.py @@ -31,23 +31,31 @@ def get_opa_config_file_path( puts the rendered contents in a file and returns the path to that file. - NOTE: Not all features of the config are implemented - only decision logs for now. + NOTE: Not all features of the config are implemented - only decision logs (upload and + console) and plugins. The permit_io service and the API key OPA sends to it are written only + when decision logs are enabled: uploading them is the service's only use. Console decision + logs do not depend on the upload. """ env = get_jinja_environment() target_path = sidecar_config.OPA_CONFIG_FILE_PATH + decision_logs_enabled = sidecar_config.OPA_DECISION_LOG_ENABLED decision_logs_backend_tier = ( sidecar_config.OPA_DECISION_LOG_INGRESS_BACKEND_TIER_URL or sidecar_config.CONTROL_PLANE ) logger.info( - "Uploading decision logs to backend tier: {tier}", - tier=decision_logs_backend_tier, + "Writing the OPA config file: decision log upload {upload}; console decision logs {console}; " + "plugins: {plugins}", + upload=f"to {decision_logs_backend_tier}" if decision_logs_enabled else "disabled", + console="enabled" if sidecar_config.OPA_DECISION_LOG_CONSOLE else "disabled", + plugins=", ".join(sidecar_config.OPA_PLUGINS) or "none", ) try: template = env.get_template(template_path) contents = template.render( + decision_logs_enabled=decision_logs_enabled, cloud_service_url=decision_logs_backend_tier, - bearer_token=get_env_api_key(), + bearer_token=get_env_api_key() if decision_logs_enabled else None, log_ingress_endpoint=sidecar_config.OPA_DECISION_LOG_INGRESS_ROUTE, min_delay_seconds=sidecar_config.OPA_DECISION_LOG_MIN_DELAY, max_delay_seconds=sidecar_config.OPA_DECISION_LOG_MAX_DELAY, diff --git a/horizon/pdp.py b/horizon/pdp.py index 6e3dea99..543e4746 100644 --- a/horizon/pdp.py +++ b/horizon/pdp.py @@ -299,9 +299,7 @@ def __init__(self): self._log_environment(remote_config.context) - if sidecar_config.OPA_BEARER_TOKEN_REQUIRED or sidecar_config.OPA_DECISION_LOG_ENABLED: - # we need to pass to OPAL a custom inline OPA config to enable these features - self._configure_inline_opa_config() + self._configure_inline_opa_config() self._configure_opal_data_updater() self._configure_opal_offline_mode() @@ -419,16 +417,40 @@ def _configure_cloud_logging(self, remote_context: dict | None = None): catch=True, # if sink throws exceptions, swallow them as not critical ) - def _configure_inline_opa_config(self): + @staticmethod + def _configure_inline_opa_config(): + """Pass OPAL the inline OPA config that decision logs, plugins and bearer auth need. + + OPA reads decision log (upload or console) and plugin settings only from its config file, + so the file is written whenever any of them is on. A different ``config_file`` already in + the inline config is replaced, with a warning. With none of them on and no bearer token + required, the inline config is left as it is. + """ + config_file_needed = ( + sidecar_config.OPA_DECISION_LOG_ENABLED + or sidecar_config.OPA_DECISION_LOG_CONSOLE + or bool(sidecar_config.OPA_PLUGINS) + ) + if not (config_file_needed or sidecar_config.OPA_BEARER_TOKEN_REQUIRED): + return + # Start from the existing config inline_opa_config = opal_client_config.INLINE_OPA_CONFIG.dict() logger.debug(f"existing OPAL_INLINE_OPA_CONFIG={inline_opa_config}") - if sidecar_config.OPA_DECISION_LOG_ENABLED: - # decision logs needs to be configured via the config file + if config_file_needed: config_file_path = get_opa_config_file_path(sidecar_config) + existing_config_file = inline_opa_config.get("config_file") + if existing_config_file and Path(existing_config_file).expanduser() != Path(config_file_path): + logger.warning( + "OPAL_INLINE_OPA_CONFIG sets config_file={existing}; OPA uses the PDP's config file " + "{path} instead, which holds its decision log and plugin settings.", + existing=existing_config_file, + path=config_file_path, + ) + # append the config file to inline OPA config inline_opa_config.update({"config_file": config_file_path}) diff --git a/horizon/static/templates/config.yaml.template b/horizon/static/templates/config.yaml.template index 51a901f1..c57f3d2e 100644 --- a/horizon/static/templates/config.yaml.template +++ b/horizon/static/templates/config.yaml.template @@ -1,22 +1,26 @@ +{% if decision_logs_enabled %} services: permit_io: url: {{ cloud_service_url }} - {% if bearer_token is defined %} credentials: bearer: token: "{{ bearer_token }}" - {% endif %} +{% endif %} +{% if decision_logs_enabled or log_to_console %} decision_logs: {% if log_to_console %} console: true {% endif %} + {% if decision_logs_enabled %} service: permit_io resource: {{ log_ingress_endpoint }} reporting: min_delay_seconds: {{ min_delay_seconds }} max_delay_seconds: {{ max_delay_seconds }} upload_size_limit_bytes: {{ upload_size_limit_bytes }} + {% endif %} +{% endif %} {% if plugins %} plugins: diff --git a/horizon/tests/test_opa_config.py b/horizon/tests/test_opa_config.py new file mode 100644 index 00000000..4d06ce51 --- /dev/null +++ b/horizon/tests/test_opa_config.py @@ -0,0 +1,220 @@ +"""What the PDP hands OPA: the OPA config file, and the inline OPA config OPAL starts OPA with. + +OPA reads decision log and plugin settings only from its config file. The file used to be written +only with decision logs on, so a PDP with them off never loaded its plugins (permit_graph among +them) and its ReBAC user permissions came back empty. +""" + +import itertools +from collections.abc import Iterator +from pathlib import Path +from types import SimpleNamespace +from typing import NamedTuple + +import pytest +import yaml +from loguru import logger +from opal_client.config import EngineLogFormat, opal_client_config, opal_common_config +from opal_client.engine.options import OpaServerOptions + +from horizon.config import sidecar_config +from horizon.enforcer.opa import config_maker +from horizon.enforcer.opa.config_maker import get_opa_config_file_path +from horizon.pdp import OPA_LOGGER_MODULE, PermitPDP + +API_KEY = "test-pdp-api-key" +BACKEND_TIER = "https://logs.example.test" +PLUGINS: dict[str, dict[str, int | bool | str]] = { + "permit_graph": {}, + "envoy_ext_authz_grpc": {"addr": ":9191", "path": "permit/root"}, +} + + +class OpaSettings(NamedTuple): + decision_logs: bool + plugins: dict[str, dict[str, int | bool | str]] + bearer_required: bool + console: bool = False + + @property + def id(self) -> str: + return "-".join( + [ + "decision-logs" if self.decision_logs else "no-decision-logs", + "plugins" if self.plugins else "no-plugins", + "bearer-required" if self.bearer_required else "bearer-optional", + "console" if self.console else "no-console", + ] + ) + + +ALL_SETTINGS = [ + OpaSettings(decision_logs, plugins, bearer_required, console) + for decision_logs, plugins, bearer_required, console in itertools.product( + [True, False], [PLUGINS, {}], [True, False], [True, False] + ) +] +CONFIG_FILE_SETTINGS = [ + settings for settings in ALL_SETTINGS if settings.decision_logs or settings.plugins or settings.console +] + + +@pytest.fixture +def opa_files(monkeypatch, tmp_path) -> SimpleNamespace: + """Write the OPA files under tmp_path, and answer the API key lookup without a control plane.""" + files = SimpleNamespace(config=tmp_path / "opa" / "config.yaml", authz=tmp_path / "opa" / "basic-authz.rego") + monkeypatch.setattr(sidecar_config, "OPA_CONFIG_FILE_PATH", str(files.config)) + monkeypatch.setattr(sidecar_config, "OPA_AUTH_POLICY_FILE_PATH", str(files.authz)) + monkeypatch.setattr(sidecar_config, "OPA_DECISION_LOG_INGRESS_BACKEND_TIER_URL", BACKEND_TIER) + monkeypatch.setattr(config_maker, "get_env_api_key", lambda: API_KEY) + monkeypatch.setattr("horizon.pdp.get_env_api_key", lambda: API_KEY) + return files + + +@pytest.fixture +def inline_opa_config(monkeypatch) -> OpaServerOptions: + """The image's OPAL_INLINE_OPA_CONFIG. The OPAL settings the PDP may change are restored after.""" + shipped = OpaServerOptions(v0_compatible=True) + monkeypatch.setattr(opal_client_config, "INLINE_OPA_CONFIG", shipped) + monkeypatch.setattr(opal_client_config, "POLICY_STORE_AUTH_TOKEN", opal_client_config.POLICY_STORE_AUTH_TOKEN) + monkeypatch.setattr(opal_client_config, "POLICY_STORE_AUTH_TYPE", opal_client_config.POLICY_STORE_AUTH_TYPE) + monkeypatch.setattr(opal_client_config, "INLINE_OPA_LOG_FORMAT", EngineLogFormat.NONE) + monkeypatch.setattr(opal_common_config, "LOG_MODULE_EXCLUDE_LIST", ["uvicorn", OPA_LOGGER_MODULE]) + return shipped + + +@pytest.fixture +def logged_warnings() -> Iterator[list[str]]: + """Every loguru message at WARNING or above emitted during the test.""" + messages: list[str] = [] + sink_id = logger.add(lambda message: messages.append(message.record["message"]), level="WARNING") + yield messages + logger.remove(sink_id) + + +def _configure(monkeypatch, settings: OpaSettings) -> None: + monkeypatch.setattr(sidecar_config, "OPA_DECISION_LOG_ENABLED", settings.decision_logs) + monkeypatch.setattr(sidecar_config, "OPA_PLUGINS", settings.plugins) + monkeypatch.setattr(sidecar_config, "OPA_BEARER_TOKEN_REQUIRED", settings.bearer_required) + monkeypatch.setattr(sidecar_config, "OPA_DECISION_LOG_CONSOLE", settings.console) + + +def _expected_config(settings: OpaSettings) -> dict: + expected: dict = {} + if settings.decision_logs: + expected["services"] = { + "permit_io": {"url": BACKEND_TIER, "credentials": {"bearer": {"token": API_KEY}}}, + } + if settings.decision_logs or settings.console: + expected["decision_logs"] = {} + if settings.console: + expected["decision_logs"]["console"] = True + if settings.decision_logs: + expected["decision_logs"] |= { + "service": "permit_io", + "resource": sidecar_config.OPA_DECISION_LOG_INGRESS_ROUTE, + "reporting": { + "min_delay_seconds": sidecar_config.OPA_DECISION_LOG_MIN_DELAY, + "max_delay_seconds": sidecar_config.OPA_DECISION_LOG_MAX_DELAY, + "upload_size_limit_bytes": sidecar_config.OPA_DECISION_LOG_UPLOAD_SIZE_LIMIT, + }, + } + if settings.plugins: + # A plugin with an empty config is written as `permit_graph:`, which YAML reads as null. + expected["plugins"] = { + plugin_id: plugin_config or None for plugin_id, plugin_config in settings.plugins.items() + } + return expected + + +def _configure_inline_opa_config() -> OpaServerOptions: + PermitPDP._configure_inline_opa_config() + return opal_client_config.INLINE_OPA_CONFIG + + +@pytest.mark.parametrize("settings", ALL_SETTINGS, ids=lambda settings: settings.id) +def test_the_config_file_holds_what_is_enabled(monkeypatch, opa_files, settings: OpaSettings): + # Whether callers of OPA need a bearer token does not change the file: the permit_io + # credentials are what OPA uploads decision logs with. + _configure(monkeypatch, settings) + + path = get_opa_config_file_path(sidecar_config) + + assert path == str(opa_files.config) + contents = Path(path).read_text() + assert (yaml.safe_load(contents) or {}) == _expected_config(settings) + assert (API_KEY in contents) == settings.decision_logs + + +@pytest.mark.usefixtures("inline_opa_config") +@pytest.mark.parametrize("settings", CONFIG_FILE_SETTINGS, ids=lambda settings: settings.id) +def test_opa_gets_the_config_file_when_decision_logs_console_or_plugins_are_on( + monkeypatch, opa_files, settings: OpaSettings +): + _configure(monkeypatch, settings) + + configured = _configure_inline_opa_config() + + assert configured.config_file == str(opa_files.config) + assert yaml.safe_load(opa_files.config.read_text()) == _expected_config(settings) + assert configured.authentication == ("token" if settings.bearer_required else "off") + assert configured.v0_compatible is True # the image's own setting is kept + # Console decision logs reach the PDP's output only with OPA's log lines shown. + shown = opal_client_config.INLINE_OPA_LOG_FORMAT == EngineLogFormat.FULL + assert shown == settings.console + assert (OPA_LOGGER_MODULE not in opal_common_config.LOG_MODULE_EXCLUDE_LIST) == settings.console + + +@pytest.mark.usefixtures("inline_opa_config") +def test_a_required_bearer_token_alone_writes_no_config_file(monkeypatch, opa_files): + _configure(monkeypatch, OpaSettings(decision_logs=False, plugins={}, bearer_required=True)) + + configured = _configure_inline_opa_config() + + assert configured.config_file is None + assert not opa_files.config.exists() + assert configured.authentication == "token" + assert configured.files == [str(opa_files.authz)] + assert configured.v0_compatible is True # the image's own setting is kept + + +def test_with_nothing_to_add_the_inline_config_is_left_as_it_is(monkeypatch, opa_files, inline_opa_config): + _configure(monkeypatch, OpaSettings(decision_logs=False, plugins={}, bearer_required=False)) + + configured = _configure_inline_opa_config() + + assert configured is inline_opa_config + assert not opa_files.config.exists() + + +@pytest.mark.usefixtures("inline_opa_config") +def test_a_different_config_file_in_the_inline_config_is_replaced_with_a_warning( + monkeypatch, opa_files, logged_warnings +): + user_config_file = "/etc/opa/user-config.yaml" + monkeypatch.setattr( + opal_client_config, "INLINE_OPA_CONFIG", OpaServerOptions(v0_compatible=True, config_file=user_config_file) + ) + _configure(monkeypatch, OpaSettings(decision_logs=False, plugins=PLUGINS, bearer_required=False)) + + configured = _configure_inline_opa_config() + + assert configured.config_file == str(opa_files.config) + assert len(logged_warnings) == 1 + assert user_config_file in logged_warnings[0] + assert str(opa_files.config) in logged_warnings[0] + + +@pytest.mark.usefixtures("inline_opa_config") +def test_the_pdp_config_file_already_in_the_inline_config_is_kept_without_a_warning( + monkeypatch, opa_files, logged_warnings +): + monkeypatch.setattr( + opal_client_config, "INLINE_OPA_CONFIG", OpaServerOptions(v0_compatible=True, config_file=str(opa_files.config)) + ) + _configure(monkeypatch, OpaSettings(decision_logs=False, plugins=PLUGINS, bearer_required=False)) + + configured = _configure_inline_opa_config() + + assert configured.config_file == str(opa_files.config) + assert logged_warnings == [] From 0b269a6f00bfe4c9ba7307d9611ab667be82e36c Mon Sep 17 00:00:00 2001 From: Zeev Manilovich Date: Thu, 8 Oct 2026 00:58:21 +0300 Subject: [PATCH 12/21] Let an explicit local decision-log setting win (PER-16928) The control plane's config overrides set OPA_DECISION_LOG_ENABLED=true for every PDP, and the PDP applied them over its own settings, so a PDP started with PDP_OPA_DECISION_LOG_ENABLED=false still uploaded decision logs. The control plane has sent that key since August 2025; before then a local false was kept. When PDP_OPA_DECISION_LOG_ENABLED is set to a boolean (true, false, 1 or 0, in any case) in the PDP's environment, its value is now kept and that one override is skipped. Without it, the control plane's value is used as before. An empty or other value, which the PDP's settings already read as the default, is named in a warning and the control plane's value is used. The PDP logs which value it uses, and the setting's description says that a value set for the PDP takes precedence. The other overrides are applied unchanged. With uploads off the PDP still writes OPA's config file for its plugins and console decision logs (PER-12470, the previous commit), so turning uploads off does not unload permit_graph or stop the Helm chart's logs_forwarder output. Co-Authored-By: Claude Opus 5.5 --- horizon/config.py | 4 +- horizon/pdp.py | 52 +++++++++++- horizon/tests/test_decision_log_setting.py | 97 ++++++++++++++++++++++ 3 files changed, 150 insertions(+), 3 deletions(-) create mode 100644 horizon/tests/test_decision_log_setting.py diff --git a/horizon/config.py b/horizon/config.py index b5db6323..addcdffd 100644 --- a/horizon/config.py +++ b/horizon/config.py @@ -245,7 +245,9 @@ def __new__(cls, *, prefix=None, is_model=True): # noqa: ARG004 OPA_DECISION_LOG_ENABLED = confi.bool( "OPA_DECISION_LOG_ENABLED", True, - description="if true, OPA decision logs will be uploaded to the Permit.io cloud console", + description="if true, OPA decision logs will be uploaded to the Permit.io cloud console. " + "The control plane also sends this setting; a value set in the PDP's environment takes " + "precedence over it", ) OPA_DECISION_LOG_CONSOLE = confi.bool( "OPA_DECISION_LOG_CONSOLE", diff --git a/horizon/pdp.py b/horizon/pdp.py index 543e4746..b64117c6 100644 --- a/horizon/pdp.py +++ b/horizon/pdp.py @@ -21,7 +21,7 @@ opal_common_config, ) from opal_client.engine.options import OpaServerOptions -from opal_common.confi import Confi +from opal_common.confi import Confi, UndefinedValueError, cast_boolean from opal_common.fetcher.providers.http_fetch_provider import ( HttpFetcherConfig, HttpMethods, @@ -164,6 +164,54 @@ def apply_config(overrides_dict: dict, config_object: Confi): logger.warning(f"Ignored non-existing config key: {prefixed_key}") +DECISION_LOG_ENABLED_KEY = "OPA_DECISION_LOG_ENABLED" + + +def apply_pdp_overrides(overrides_dict: dict) -> None: + """Apply the control plane's PDP config overrides to ``sidecar_config``. + + The control plane sends the same ``OPA_DECISION_LOG_ENABLED`` to every PDP. When + ``PDP_OPA_DECISION_LOG_ENABLED`` is set to a boolean in this PDP's environment, the local + value is kept and that one override is skipped. An empty or non-boolean value is logged as a + warning and the control plane's value is used. Which of the two values is used is logged. + + Args: + overrides_dict: the ``pdp`` section of the remote config. + """ + overrides = dict(overrides_dict) + if DECISION_LOG_ENABLED_KEY in overrides: + env_name = sidecar_config._prefix_key(DECISION_LOG_ENABLED_KEY) + remote_value = overrides[DECISION_LOG_ENABLED_KEY] + local_text = os.environ.get(env_name) + if local_text is None: + logger.info( + "{env_name} is not set for this PDP; using the control plane's value ({remote}).", + env_name=env_name, + remote=remote_value, + ) + else: + try: + local_value = cast_boolean(local_text) + except UndefinedValueError: + logger.warning( + "{env_name} is set to {local!r}, which is not true, false, 1 or 0; " + "using the control plane's value ({remote}).", + env_name=env_name, + local=local_text, + remote=remote_value, + ) + else: + del overrides[DECISION_LOG_ENABLED_KEY] + logger.info( + "{env_name} is set for this PDP ({local}); " + "using it instead of the control plane's value ({remote}).", + env_name=env_name, + local=local_value, + remote=remote_value, + ) + apply_config(overrides, sidecar_config) + + # Declared as a ``response_model`` (rather than left as a bare dict) because the trigger routes' # customer-facing OpenAPI description tells integrators to branch on ``triggered``: without one, # FastAPI publishes an empty 200 schema, so the prose would reference a field the machine-readable @@ -295,7 +343,7 @@ def __init__(self): apply_config(remote_config.opal_common or {}, opal_common_config) apply_config(remote_config.opal_client or {}, opal_client_config) - apply_config(remote_config.pdp or {}, sidecar_config) + apply_pdp_overrides(remote_config.pdp or {}) self._log_environment(remote_config.context) diff --git a/horizon/tests/test_decision_log_setting.py b/horizon/tests/test_decision_log_setting.py new file mode 100644 index 00000000..5c010939 --- /dev/null +++ b/horizon/tests/test_decision_log_setting.py @@ -0,0 +1,97 @@ +"""Which PDP_OPA_DECISION_LOG_ENABLED a PDP ends up with, its own or the control plane's. + +The control plane sends OPA_DECISION_LOG_ENABLED to every PDP with its config overrides. A boolean +set in the PDP's environment is kept; without one, or with an empty or non-boolean value, the +control plane's value is used. + +Each case runs in a new interpreter, because horizon.config reads the environment once, at import. +""" + +import json +import os +import subprocess +import sys +from pathlib import Path + +import pytest + +REPO_ROOT = Path(__file__).resolve().parents[2] +ENV_NAME = "PDP_OPA_DECISION_LOG_ENABLED" +ROUTE = "/v1/decision_logs/custom-ingress" + + +def _apply_overrides(env_value: str | None, overrides: dict) -> tuple[dict, str]: + """Start with ENV_NAME set to env_value (unset for None), apply the overrides as the PDP does. + + Returns the resulting decision log settings and everything the interpreter logged. + """ + env = {name: value for name, value in os.environ.items() if name != ENV_NAME} + if env_value is not None: + env[ENV_NAME] = env_value + code = ( + "import json, sys\n" + "from horizon.config import sidecar_config\n" + "from horizon.pdp import apply_pdp_overrides\n" + "apply_pdp_overrides(json.loads(sys.argv[1]))\n" + "print(json.dumps({'enabled': sidecar_config.OPA_DECISION_LOG_ENABLED," + " 'route': sidecar_config.OPA_DECISION_LOG_INGRESS_ROUTE}))\n" + ) + result = subprocess.run( + [sys.executable, "-c", code, json.dumps(overrides)], + cwd=REPO_ROOT, + env=env, + capture_output=True, + text=True, + check=False, + timeout=120, + ) + assert result.returncode == 0, result.stderr + return json.loads(result.stdout.splitlines()[-1]), result.stderr + + +def test_false_set_for_the_pdp_is_kept_over_the_control_plane_true(): + settings, logged = _apply_overrides( + "false", {"OPA_DECISION_LOG_ENABLED": True, "OPA_DECISION_LOG_INGRESS_ROUTE": ROUTE} + ) + + assert settings == {"enabled": False, "route": ROUTE} + assert f"{ENV_NAME} is set for this PDP (False); using it instead of the control plane's value (True)." in logged + + +def test_true_set_for_the_pdp_is_kept_over_the_control_plane_false(): + settings, logged = _apply_overrides( + "true", {"OPA_DECISION_LOG_ENABLED": False, "OPA_DECISION_LOG_INGRESS_ROUTE": ROUTE} + ) + + assert settings == {"enabled": True, "route": ROUTE} + assert f"{ENV_NAME} is set for this PDP (True); using it instead of the control plane's value (False)." in logged + + +def test_without_a_value_set_for_the_pdp_the_control_plane_value_is_used(): + settings, logged = _apply_overrides( + None, {"OPA_DECISION_LOG_ENABLED": False, "OPA_DECISION_LOG_INGRESS_ROUTE": ROUTE} + ) + + assert settings == {"enabled": False, "route": ROUTE} + assert f"{ENV_NAME} is not set for this PDP; using the control plane's value (False)." in logged + + +@pytest.mark.parametrize("env_value", ["", "maybe"], ids=["empty", "not-a-boolean"]) +def test_a_pdp_value_that_is_not_a_boolean_is_ignored_with_a_warning(env_value: str): + settings, logged = _apply_overrides( + env_value, {"OPA_DECISION_LOG_ENABLED": False, "OPA_DECISION_LOG_INGRESS_ROUTE": ROUTE} + ) + + assert settings == {"enabled": False, "route": ROUTE} + message = ( + f"{ENV_NAME} is set to {env_value!r}, which is not true, false, 1 or 0; " + "using the control plane's value (False)." + ) + assert [line for line in logged.splitlines() if message in line and "WARNING" in line] + + +def test_a_control_plane_that_sends_no_value_leaves_the_pdp_value_alone(): + settings, logged = _apply_overrides("false", {"OPA_DECISION_LOG_INGRESS_ROUTE": ROUTE}) + + assert settings == {"enabled": False, "route": ROUTE} + assert ENV_NAME not in logged From 3d067210ede4014c424bc1956644453196bc5a08 Mon Sep 17 00:00:00 2001 From: Zeev Manilovich Date: Wed, 7 Oct 2026 23:52:30 +0300 Subject: [PATCH 13/21] Add nodeSelector, tolerations and affinity to the pdp chart (PER-16923) The chart had no way to place the PDP pods, so clusters that taint their node pools or keep workloads on one pool could not schedule it there (#310). The pod spec now takes pdp.nodeSelector, pdp.tolerations and pdp.affinity as given. All three default to empty and render nothing, so existing releases render the same manifests. horizon/tests/test_helm_chart.py renders the chart with helm template and checks that each value lands in the pod spec, on its own and next to the optional logs-forwarder and OpenShift volumes. The pytests job installs a pinned Helm for it; without Helm the tests skip locally and fail in CI. Based on #311. Co-authored-by: cooksec <231643840+cooksec@users.noreply.github.com> Co-Authored-By: Claude Opus 5.5 --- .github/workflows/tests.yml | 8 +++ charts/pdp/templates/deployment.yaml | 12 ++++ charts/pdp/values.yaml | 22 ++++++ horizon/tests/test_helm_chart.py | 102 +++++++++++++++++++++++++++ pyproject.toml | 4 +- 5 files changed, 146 insertions(+), 2 deletions(-) create mode 100644 horizon/tests/test_helm_chart.py diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index b5a3ca38..8a2f11e3 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -104,6 +104,14 @@ jobs: python -m pip install --dry-run --quiet --disable-pip-version-check \ --require-hashes --no-deps --requirement "$RUNNER_TEMP/lock.txt" + # For horizon/tests/test_helm_chart.py, which renders and lints charts/pdp. Pinned so a + # Helm release cannot change the result on its own; the tests fail, not skip, when CI is set + # and helm is missing. + - name: Install Helm + uses: azure/setup-helm@9bc31f4ebc9c6b171d7bfbaa5d006ae7abdb4310 # v5.0.1 + with: + version: v4.3.0 + - name: Run Pytests run: uv run --frozen pytest -s --cache-clear horizon/tests/ diff --git a/charts/pdp/templates/deployment.yaml b/charts/pdp/templates/deployment.yaml index 214cea11..ce9293ab 100644 --- a/charts/pdp/templates/deployment.yaml +++ b/charts/pdp/templates/deployment.yaml @@ -155,3 +155,15 @@ spec: - name: opa-volume emptyDir: {} {{- end }} + {{- with .Values.pdp.nodeSelector }} + nodeSelector: + {{- toYaml . | nindent 8 }} + {{- end }} + {{- with .Values.pdp.affinity }} + affinity: + {{- toYaml . | nindent 8 }} + {{- end }} + {{- with .Values.pdp.tolerations }} + tolerations: + {{- toYaml . | nindent 8 }} + {{- end }} diff --git a/charts/pdp/values.yaml b/charts/pdp/values.yaml index 49aebdf7..6e60d706 100644 --- a/charts/pdp/values.yaml +++ b/charts/pdp/values.yaml @@ -43,6 +43,28 @@ pdp: index: "" debug_mode: false + # Pod scheduling, copied into the PDP pod spec as given. Empty values render nothing. + # Example - run only on a dedicated, tainted node pool: + # nodeSelector: + # pool: pdp + # tolerations: + # - key: "dedicated" + # operator: "Equal" + # value: "pdp" + # effect: "NoSchedule" + # affinity: + # podAntiAffinity: + # preferredDuringSchedulingIgnoredDuringExecution: + # - weight: 100 + # podAffinityTerm: + # topologyKey: kubernetes.io/hostname + # labelSelector: + # matchLabels: + # app: permitio-pdp + nodeSelector: {} + tolerations: [] + affinity: {} + podDisruptionBudget: # Automatically enabled when replicas > 1 # Set minAvailable OR maxUnavailable (not both) diff --git a/horizon/tests/test_helm_chart.py b/horizon/tests/test_helm_chart.py new file mode 100644 index 00000000..4ffc6fc1 --- /dev/null +++ b/horizon/tests/test_helm_chart.py @@ -0,0 +1,102 @@ +"""Render tests for the Helm chart in charts/pdp. + +They live here because the required `pytests` job runs exactly +`pytest -s --cache-clear horizon/tests/`, and that job installs Helm for them. Without Helm on +PATH they skip locally and fail in CI, so a runner that lost Helm cannot pass them silently. +""" + +import os +import shutil +import subprocess +from pathlib import Path +from typing import Any + +import pytest +import yaml + +CHART = Path(__file__).resolve().parents[2] / "charts" / "pdp" +SCHEDULING_KEYS = {"nodeSelector", "tolerations", "affinity"} +SCHEDULING = { + "nodeSelector": {"pool": "pdp"}, + "tolerations": [{"key": "dedicated", "operator": "Equal", "value": "pdp", "effect": "NoSchedule"}], + "affinity": { + "nodeAffinity": { + "requiredDuringSchedulingIgnoredDuringExecution": { + "nodeSelectorTerms": [{"matchExpressions": [{"key": "pool", "operator": "In", "values": ["pdp"]}]}] + } + } + }, +} + + +def _helm() -> str: + helm = shutil.which("helm") + if helm is None: + message = "helm is not on PATH: install Helm to run the chart tests" + if os.environ.get("CI"): + pytest.fail(message) + pytest.skip(message) + return helm + + +def _helm_run(tmp_path: Path, command: list[str], values: dict[str, Any] | None) -> subprocess.CompletedProcess[str]: + args = [_helm(), *command] + if values is not None: + values_file = tmp_path / "values.yaml" + values_file.write_text(yaml.safe_dump(values), encoding="utf-8") + args += ["--values", str(values_file)] + return subprocess.run(args, capture_output=True, text=True, check=False) + + +def _render(tmp_path: Path, values: dict[str, Any] | None = None) -> list[dict[str, Any]]: + result = _helm_run(tmp_path, ["template", "pdp", str(CHART), "--namespace", "pdp"], values) + assert result.returncode == 0, result.stderr + return [doc for doc in yaml.safe_load_all(result.stdout) if doc] + + +def _pod_spec(docs: list[dict[str, Any]]) -> dict[str, Any]: + [deployment] = [doc for doc in docs if doc["kind"] == "Deployment"] + return deployment["spec"]["template"]["spec"] + + +@pytest.mark.parametrize( + "values", + [ + pytest.param(None, id="defaults"), + pytest.param({"pdp": SCHEDULING}, id="scheduling"), + ], +) +def test_chart_lints_clean(tmp_path, values): + result = _helm_run(tmp_path, ["lint", "--strict", str(CHART)], values) + assert result.returncode == 0, result.stdout + result.stderr + + +def test_default_render_sets_no_scheduling_constraints(tmp_path): + assert not SCHEDULING_KEYS & _pod_spec(_render(tmp_path)).keys() + + +def test_scheduling_values_reach_the_pod_spec(tmp_path): + pod = _pod_spec(_render(tmp_path, {"pdp": SCHEDULING})) + assert {key: pod[key] for key in SCHEDULING_KEYS} == SCHEDULING + + +@pytest.mark.parametrize("key", sorted(SCHEDULING_KEYS)) +def test_each_scheduling_value_renders_on_its_own(tmp_path, key): + pod = _pod_spec(_render(tmp_path, {"pdp": {key: SCHEDULING[key]}})) + assert pod[key] == SCHEDULING[key] + assert not (SCHEDULING_KEYS - {key}) & pod.keys() + + +@pytest.mark.parametrize( + ("values", "volumes"), + [ + pytest.param( + {"pdp": {"logs_forwarder": {"enabled": True}}}, ["fluent-bit-config", "logs"], id="logs-forwarder" + ), + pytest.param({"openshift": {"enabled": True}}, ["tmp-volume", "opa-volume"], id="openshift"), + ], +) +def test_scheduling_values_sit_beside_the_optional_volumes(tmp_path, values, volumes): + pod = _pod_spec(_render(tmp_path, {**values, "pdp": {**values.get("pdp", {}), **SCHEDULING}})) + assert {key: pod[key] for key in SCHEDULING_KEYS} == SCHEDULING + assert [volume["name"] for volume in pod["volumes"]] == volumes diff --git a/pyproject.toml b/pyproject.toml index e7ab4c7d..d178e098 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -81,8 +81,8 @@ dev = [ "pytest-timeout", "aioresponses", # Imported by .github/scripts/check_waiver_parity.py, which horizon/tests/test_ci_scripts.py - # loads at collection time, so a missing PyYAML fails the whole pytests job. A test and - # CI-tooling dependency only; nothing under horizon/ imports it. + # loads at collection time, and by horizon/tests/test_helm_chart.py, so a missing PyYAML fails + # the whole pytests job. A test and CI-tooling dependency only; no runtime code imports it. "PyYAML", # Here, unlike ruff, because it types third-party imports from the project's .venv; the # `ty-check` hook runs it with `uv run`. Pinned exactly: every ty release adds checks, so a From 88b401b8b5897b27121e8333a12d72a889ca9f32 Mon Sep 17 00:00:00 2001 From: Zeev Manilovich Date: Wed, 7 Oct 2026 23:54:43 +0300 Subject: [PATCH 14/21] Let the pdp chart take the API key from the pod env (PER-16924) The chart always set PDP_API_KEY from a secretKeyRef, to the Secret it creates or to existingApiKeySecret, so the key could not come from something that adds env to the pod, such as a bank-vaults mutating webhook resolving a vault: reference (#300). pdp.userProvidedSecret: true makes the chart create no Secret and set no PDP_API_KEY; PDP_API_KEY comes from pdpEnvs or from the webhook. values.yaml says that pdpEnvs values are stored in the Deployment as plain text, so they should hold a reference the webhook resolves, and a literal key belongs in ApiKey or existingApiKeySecret. Two settings fail the render with a message naming the value: both userProvidedSecret and existingApiKeySecret, since one says to read a Secret and the other to read none, and a userProvidedSecret that is not a boolean, since a quoted "false" is truthy in a template and would drop PDP_API_KEY. The default render is unchanged. The chart tests cover the default, existingApiKeySecret, userProvidedSecret with and without pdpEnvs, the quoted and null values, and both settings together. Based on #315. Co-authored-by: omribz156 <260634988+omribz156@users.noreply.github.com> Co-Authored-By: Claude Opus 5.5 --- charts/pdp/templates/_helpers.tpl | 13 +++++ charts/pdp/templates/deployment.yaml | 3 ++ charts/pdp/templates/secret.yaml | 2 +- charts/pdp/values.yaml | 14 +++++ horizon/tests/test_helm_chart.py | 76 +++++++++++++++++++++++++++- 5 files changed, 105 insertions(+), 3 deletions(-) diff --git a/charts/pdp/templates/_helpers.tpl b/charts/pdp/templates/_helpers.tpl index b65ae0ce..437cca0f 100644 --- a/charts/pdp/templates/_helpers.tpl +++ b/charts/pdp/templates/_helpers.tpl @@ -15,6 +15,19 @@ Common labels {{- end }} {{- end }} +{{/* +Fail on API key settings that contradict each other or that a template would misread +*/}} +{{- define "pdp.validateApiKeySource" -}} +{{- $userProvidedSecret := .Values.pdp.userProvidedSecret | default false -}} +{{- if not (kindIs "bool" $userProvidedSecret) -}} +{{- fail (printf "pdp.userProvidedSecret must be true or false, got the %s %q. A quoted \"false\" counts as set and would drop PDP_API_KEY." (kindOf $userProvidedSecret) (toString $userProvidedSecret)) -}} +{{- end -}} +{{- if and $userProvidedSecret .Values.pdp.existingApiKeySecret -}} +{{- fail "pdp.userProvidedSecret and pdp.existingApiKeySecret cannot both be set: existingApiKeySecret makes the chart read PDP_API_KEY from that Secret, userProvidedSecret makes it read no Secret at all. Unset one of them." -}} +{{- end -}} +{{- end }} + {{/* Get the secret name for the API key */}} diff --git a/charts/pdp/templates/deployment.yaml b/charts/pdp/templates/deployment.yaml index ce9293ab..4dfb8226 100644 --- a/charts/pdp/templates/deployment.yaml +++ b/charts/pdp/templates/deployment.yaml @@ -1,3 +1,4 @@ +{{- include "pdp.validateApiKeySource" . }} apiVersion: apps/v1 kind: Deployment metadata: @@ -50,11 +51,13 @@ spec: containerPort: {{ .targetPort }} {{- end }} env: + {{- if not .Values.pdp.userProvidedSecret }} - name: PDP_API_KEY valueFrom: secretKeyRef: name: {{ include "pdp.secretName" . }} key: {{ include "pdp.secretKey" . }} + {{- end }} {{- if .Values.pdp.pdpEnvs }} {{- range .Values.pdp.pdpEnvs }} - name: {{ .name }} diff --git a/charts/pdp/templates/secret.yaml b/charts/pdp/templates/secret.yaml index b3b1f650..69c8f705 100644 --- a/charts/pdp/templates/secret.yaml +++ b/charts/pdp/templates/secret.yaml @@ -1,4 +1,4 @@ -{{- if not .Values.pdp.existingApiKeySecret }} +{{- if not (or .Values.pdp.existingApiKeySecret .Values.pdp.userProvidedSecret) }} apiVersion: v1 kind: Secret metadata: diff --git a/charts/pdp/values.yaml b/charts/pdp/values.yaml index 6e60d706..c4588662 100644 --- a/charts/pdp/values.yaml +++ b/charts/pdp/values.yaml @@ -10,6 +10,10 @@ pdp: # Example - enable Envoy gRPC ext_authz on port 9191 (requires PDP >= 0.9.10): # - name: PDP_OPA_PLUGINS # value: '{"permit_graph":{},"envoy_ext_authz_grpc":{"addr":":9191","path":"permit/root"}}' + # The PDP's API key. By default the chart stores ApiKey in a Secret it creates + # (permitio-pdp-secret) and passes it to the PDP as PDP_API_KEY. existingApiKeySecret and + # userProvidedSecret below change that. Set at most one of them: with both set, the chart + # fails to render. ApiKey: "" # Use an existing secret for the API key instead of creating one @@ -17,6 +21,16 @@ pdp: # existingApiKeySecret: # name: "my-existing-secret" # key: "api-key" + + # Set to true when something other than the chart supplies PDP_API_KEY, for example a + # mutating webhook such as bank-vaults. The chart then creates no Secret, sets no PDP_API_KEY + # and ignores ApiKey. Set PDP_API_KEY in pdpEnvs above, or have the webhook add it to the pod. + # pdpEnvs values are stored in the Deployment as plain text, so put a reference there that + # the webhook resolves when the container starts (for example a bank-vaults `vault:` path), + # and keep a literal key in ApiKey or existingApiKeySecret. Must be a boolean: a quoted + # "false" fails the render. Without PDP_API_KEY the PDP does not start: its health checks + # fail and Kubernetes keeps restarting the container. + userProvidedSecret: false port: 7766 # Example - expose Envoy gRPC ext_authz port (requires PDP_OPA_PLUGINS env var above): # additionalPorts: diff --git a/horizon/tests/test_helm_chart.py b/horizon/tests/test_helm_chart.py index 4ffc6fc1..36dcea98 100644 --- a/horizon/tests/test_helm_chart.py +++ b/horizon/tests/test_helm_chart.py @@ -15,6 +15,12 @@ import yaml CHART = Path(__file__).resolve().parents[2] / "charts" / "pdp" +TEMPLATE = ["template", "pdp", str(CHART), "--namespace", "pdp"] +EXISTING_SECRET = {"existingApiKeySecret": {"name": "pdp-api-key", "key": "api-key"}} +USER_PROVIDED_SECRET = { + "userProvidedSecret": True, + "pdpEnvs": [{"name": "PDP_API_KEY", "value": "vault:secret/data/pdp#api-key"}], +} SCHEDULING_KEYS = {"nodeSelector", "tolerations", "affinity"} SCHEDULING = { "nodeSelector": {"pool": "pdp"}, @@ -49,21 +55,34 @@ def _helm_run(tmp_path: Path, command: list[str], values: dict[str, Any] | None) def _render(tmp_path: Path, values: dict[str, Any] | None = None) -> list[dict[str, Any]]: - result = _helm_run(tmp_path, ["template", "pdp", str(CHART), "--namespace", "pdp"], values) + result = _helm_run(tmp_path, TEMPLATE, values) assert result.returncode == 0, result.stderr return [doc for doc in yaml.safe_load_all(result.stdout) if doc] +def _of_kind(docs: list[dict[str, Any]], kind: str) -> list[dict[str, Any]]: + return [doc for doc in docs if doc["kind"] == kind] + + def _pod_spec(docs: list[dict[str, Any]]) -> dict[str, Any]: - [deployment] = [doc for doc in docs if doc["kind"] == "Deployment"] + [deployment] = _of_kind(docs, "Deployment") return deployment["spec"]["template"]["spec"] +def _pdp_env(docs: list[dict[str, Any]], *names: str) -> list[dict[str, Any]]: + """The PDP container's env entries with the given names, in render order.""" + [container] = [c for c in _pod_spec(docs)["containers"] if c["name"] == "permitio-pdp"] + return [entry for entry in container.get("env") or [] if entry["name"] in names] + + @pytest.mark.parametrize( "values", [ pytest.param(None, id="defaults"), pytest.param({"pdp": SCHEDULING}, id="scheduling"), + pytest.param({"pdp": EXISTING_SECRET}, id="existing-secret"), + pytest.param({"pdp": USER_PROVIDED_SECRET}, id="user-provided-secret"), + pytest.param({"pdp": {"userProvidedSecret": True}}, id="user-provided-secret-without-envs"), ], ) def test_chart_lints_clean(tmp_path, values): @@ -100,3 +119,56 @@ def test_scheduling_values_sit_beside_the_optional_volumes(tmp_path, values, vol pod = _pod_spec(_render(tmp_path, {**values, "pdp": {**values.get("pdp", {}), **SCHEDULING}})) assert {key: pod[key] for key in SCHEDULING_KEYS} == SCHEDULING assert [volume["name"] for volume in pod["volumes"]] == volumes + + +@pytest.mark.parametrize( + "values", + [ + pytest.param(None, id="defaults"), + pytest.param({"pdp": {"userProvidedSecret": False}}, id="user-provided-secret-off"), + # A null in a values file removes the key, which must read as false. + pytest.param({"pdp": {"userProvidedSecret": None}}, id="user-provided-secret-null"), + ], +) +def test_api_key_comes_from_the_chart_secret_by_default(tmp_path, values): + docs = _render(tmp_path, values) + [secret] = _of_kind(docs, "Secret") + assert secret["metadata"]["name"] == "permitio-pdp-secret" + assert set(secret["data"]) == {"ApiKey"} + assert _pdp_env(docs, "PDP_API_KEY") == [ + {"name": "PDP_API_KEY", "valueFrom": {"secretKeyRef": {"name": "permitio-pdp-secret", "key": "ApiKey"}}} + ] + + +def test_existing_secret_replaces_the_chart_secret(tmp_path): + docs = _render(tmp_path, {"pdp": EXISTING_SECRET}) + assert not _of_kind(docs, "Secret") + assert _pdp_env(docs, "PDP_API_KEY") == [ + {"name": "PDP_API_KEY", "valueFrom": {"secretKeyRef": {"name": "pdp-api-key", "key": "api-key"}}} + ] + + +def test_user_provided_secret_leaves_the_api_key_to_pdp_envs(tmp_path): + docs = _render(tmp_path, {"pdp": USER_PROVIDED_SECRET}) + assert not _of_kind(docs, "Secret") + assert _pdp_env(docs, "PDP_API_KEY") == [{"name": "PDP_API_KEY", "value": "vault:secret/data/pdp#api-key"}] + + +def test_user_provided_secret_without_pdp_envs_sets_no_api_key(tmp_path): + docs = _render(tmp_path, {"pdp": {"userProvidedSecret": True}}) + assert not _of_kind(docs, "Secret") + assert not _pdp_env(docs, "PDP_API_KEY") + + +def test_user_provided_secret_and_existing_secret_together_fail_to_render(tmp_path): + result = _helm_run(tmp_path, TEMPLATE, {"pdp": {**EXISTING_SECRET, **USER_PROVIDED_SECRET}}) + assert result.returncode != 0 + assert "pdp.userProvidedSecret and pdp.existingApiKeySecret cannot both be set" in result.stderr + + +@pytest.mark.parametrize("value", ["false", "true"]) +def test_user_provided_secret_given_as_a_string_fails_to_render(tmp_path, value): + # A quoted "false" is truthy in a template, so without this check it would drop PDP_API_KEY. + result = _helm_run(tmp_path, TEMPLATE, {"pdp": {"userProvidedSecret": value}}) + assert result.returncode != 0 + assert f'pdp.userProvidedSecret must be true or false, got the string "{value}"' in result.stderr From 55c09f64693c148e49cb6ce2f5dbbaf13a6bd6ce Mon Sep 17 00:00:00 2001 From: Zeev Manilovich Date: Wed, 7 Oct 2026 23:54:55 +0300 Subject: [PATCH 15/21] Bump the pdp chart to 0.0.7 Publishes pdp.nodeSelector, pdp.tolerations and pdp.affinity (PER-16923) and pdp.userProvidedSecret (PER-16924) together. helm_release.yml only releases when Chart.yaml changes, so both share this one bump. Co-Authored-By: Claude Opus 5.5 --- charts/pdp/Chart.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/charts/pdp/Chart.yaml b/charts/pdp/Chart.yaml index ac9815e2..63a8ae85 100644 --- a/charts/pdp/Chart.yaml +++ b/charts/pdp/Chart.yaml @@ -1,7 +1,7 @@ apiVersion: v2 name: pdp description: An official Helm chart for Permit.io PDP (Policy Decision Point) with OpenShift support and configurable ports -version: 0.0.6 +version: 0.0.7 keywords: - policy - authorization From 91b5fdfad48a0dec29a3b5eb05b5a1fd5eee4125 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Wed, 7 Oct 2026 15:47:26 +0000 Subject: [PATCH 16/21] Bump docker/scout-action to 1.25.0 (PER-16931) Folds in Dependabot's #387. All five pins (tests.yml, release.yml, scheduled-security-scan.yml) move from 7c6b6c3f (v1.24.0) to 221e7f4860634eeb1579e3bd7ca232e577bd1864. #387 labelled that commit v1.26.0, and the v1.26.0 tag does point at it, but so does v1.25.0: it is the v1.25.0 release commit, and its index.js downloads Scout CLI v1.25.0. The v1.26.0 release commit (02c6c79b) has no tag. The comments say v1.25.0, the version that runs. Dependabot reads the current version from the tags on the SHA, not from the comment, and rewrites the comment on the next bump. 1.25.0 makes Scout look inside Debian and Alpine package archives by default, so the docker-scout gate can report packages 1.24.0 did not. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/release.yml | 2 +- .github/workflows/scheduled-security-scan.yml | 4 ++-- .github/workflows/tests.yml | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 735982c3..ff96e96d 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -347,7 +347,7 @@ jobs: - name: Docker Scout scan the release image id: scout if: ${{ !cancelled() }} - uses: docker/scout-action@7c6b6c3f7844478ace1ffd4e7aef649053d1f87d # v1.24.0 + uses: docker/scout-action@221e7f4860634eeb1579e3bd7ca232e577bd1864 # v1.25.0 with: command: cves image: local://permitio/pdp-v2:release-scan diff --git a/.github/workflows/scheduled-security-scan.yml b/.github/workflows/scheduled-security-scan.yml index 4b381da9..21b7ed7b 100644 --- a/.github/workflows/scheduled-security-scan.yml +++ b/.github/workflows/scheduled-security-scan.yml @@ -269,7 +269,7 @@ jobs: # `if:` is skipped once anything earlier in the job has failed, and a skipped report # leaves code scanning showing yesterday's answer for this image. if: ${{ !cancelled() }} - uses: docker/scout-action@7c6b6c3f7844478ace1ffd4e7aef649053d1f87d # v1.24.0 + uses: docker/scout-action@221e7f4860634eeb1579e3bd7ca232e577bd1864 # v1.25.0 with: command: cves # `registry://`, NOT the `local://` that tests.yml uses. There is no local image @@ -292,7 +292,7 @@ jobs: # Load-bearing, not defensive. Without it a failed report step skips this one, and # the report job would have nothing from Scout to read. if: ${{ !cancelled() }} - uses: docker/scout-action@7c6b6c3f7844478ace1ffd4e7aef649053d1f87d # v1.24.0 + uses: docker/scout-action@221e7f4860634eeb1579e3bd7ca232e577bd1864 # v1.25.0 with: command: cves image: registry://permitio/pdp-v2:latest diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 8a2f11e3..0bd8a619 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -455,7 +455,7 @@ jobs: username: ${{ vars.DOCKERHUB_ORGANIZATION }} - name: Docker Scout CVE report (all severities) - uses: docker/scout-action@7c6b6c3f7844478ace1ffd4e7aef649053d1f87d # v1.24.0 + uses: docker/scout-action@221e7f4860634eeb1579e3bd7ca232e577bd1864 # v1.25.0 with: command: cves image: local://permitio/pdp-v2:next @@ -481,7 +481,7 @@ jobs: - name: Docker Scout CVE gate (high and critical) id: scout-gate - uses: docker/scout-action@7c6b6c3f7844478ace1ffd4e7aef649053d1f87d # v1.24.0 + uses: docker/scout-action@221e7f4860634eeb1579e3bd7ca232e577bd1864 # v1.25.0 with: command: cves image: local://permitio/pdp-v2:next From 8ecb566db2e30610df3630cc16c02af14c5d023a Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Wed, 7 Oct 2026 15:37:38 +0000 Subject: [PATCH 17/21] Bump pre-commit/pre-commit-hooks to v6.0.0 (PER-16931) Folds in Dependabot's #384. The rev moves from cef0300f (v5.0.0) to 3e8a8703264a2f4a69428a0aa4dcb512790b2c8c, the commit the v6.0.0 tag points to. v6.0.0 needs Python 3.9+ and removes check-byte-order-marker and fix-encoding-pragma; this config uses neither, and every hook it does use passes on the whole tree. Co-Authored-By: Claude Opus 5.5 --- .pre-commit-config.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index cc2854a6..5624f757 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -1,6 +1,6 @@ repos: - repo: https://github.com/pre-commit/pre-commit-hooks - rev: cef0300fd0fc4d2a87a85fa2093c6b283ea36f4b # frozen: v5.0.0 + rev: 3e8a8703264a2f4a69428a0aa4dcb512790b2c8c # frozen: v6.0.0 hooks: - id: trailing-whitespace - id: end-of-file-fixer From 2d606aab0aabb23852e4f0544147cf3c43d979a5 Mon Sep 17 00:00:00 2001 From: Zeev Manilovich Date: Wed, 7 Oct 2026 23:56:07 +0300 Subject: [PATCH 18/21] Stop Dependabot proposing OPAL 0.9.x past 0.9.6 (PER-16931) opal-common and opal-client 0.9.9 declare Requires-Python <3.13, so they cannot run on the 3.13 image; pyproject.toml holds them at 0.9.6 for that reason. uv ignores Requires-Python upper bounds when it locks, so Dependabot would still open a bump that only the pytests job's pip check on 3.13 rejects. Ignore >=0.9.7, <0.10 for both. A 0.10 release still arrives as a PR. Co-Authored-By: Claude Opus 5.5 --- .github/dependabot.yml | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/.github/dependabot.yml b/.github/dependabot.yml index ecef8efb..7a89fab1 100644 --- a/.github/dependabot.yml +++ b/.github/dependabot.yml @@ -83,6 +83,16 @@ updates: # exercises end to end. Majors stay manual; minors and patches are fine. - dependency-name: "websockets" update-types: ["version-update:semver-major"] + # opal-common and opal-client 0.9.9, the first final release after the 0.9.6 pinned in + # pyproject.toml, declare Requires-Python <3.13 and cannot run on the 3.13 image (see the + # note at that pin). uv ignores Requires-Python upper bounds when it locks, so Dependabot + # would still open the PR and only the pytests job's pip check would catch it. 0.10 stays + # in, so a release that lifts the bound still arrives as a PR. Drop these two once an + # OPAL 0.9.x release supports Python 3.13. + - dependency-name: "opal-common" + versions: [">=0.9.7, <0.10"] + - dependency-name: "opal-client" + versions: [">=0.9.7, <0.10"] commit-message: prefix: "deps" prefix-development: "deps-dev" From 2f48dd8ce1dffdd5318f7d8ea15560c40218f394 Mon Sep 17 00:00:00 2001 From: Zeev Manilovich Date: Wed, 7 Oct 2026 23:58:00 +0300 Subject: [PATCH 19/21] Run pdp-tester one at a time against staging (PER-16929) Every pdp-tester run writes to the same staging environment, and the job had no concurrency group. Overlapping runs failed each other's sync cases: Dependabot PRs 384-386 ran together and each failed a different case, and 387 passed alone right after. The job now joins one repository-wide group with `queue: max`. Runs from PRs, main and releases (which call this workflow) wait their turn in order, up to 100. The default queue would have kept only one waiting run and cancelled it when the next arrived, leaving another PR's required check cancelled. GitHub rejects queue: max with cancel-in-progress, so a started run always finishes. actionlint 1.7.12 does not know the `queue` key (rhysd/actionlint#657), so .github/actionlint.yaml suppresses that one message for tests.yml, with a removal gate. A misspelled concurrency key still fails. Co-Authored-By: Claude Opus 5.5 --- .github/actionlint.yaml | 19 +++++++++++++++---- .github/workflows/tests.yml | 9 +++++++++ 2 files changed, 24 insertions(+), 4 deletions(-) diff --git a/.github/actionlint.yaml b/.github/actionlint.yaml index fd1378bc..860f45a5 100644 --- a/.github/actionlint.yaml +++ b/.github/actionlint.yaml @@ -1,23 +1,34 @@ # actionlint configuration. # -# One suppression, and it is a gap in the linter rather than in the workflow. +# Two suppressions, and both are gaps in the linter rather than in the workflows. # -# `vulnerability-alerts: read` is the ONLY GITHUB_TOKEN permission that grants +# 1. `vulnerability-alerts: read` is the ONLY GITHUB_TOKEN permission that grants # GET /repos/{owner}/{repo}/dependabot/alerts. GitHub documents it under the # `permissions` key in the workflow syntax reference ("For Dependabot alerts, use the # vulnerability-alerts permission"), but actionlint 1.7.12 still validates permission # scopes against a hard-coded list that predates it, so it reports the real, working key # as `unknown permission scope`. # -# Removal gate: drop this block once actionlint's `AllPermissionScopes` includes +# Removal gate: drop this entry once actionlint's `AllPermissionScopes` includes # `vulnerability-alerts` — check with # actionlint .github/workflows/scheduled-security-scan.yml -# after an actionlint upgrade. If it passes, delete this file. +# after an actionlint upgrade. # # Deliberately scoped to the one file and the one message. A blanket `permissions` ignore # would also hide a genuinely misspelled scope, which fails OPEN: an unrecognised scope is # not granted, the API call 403s, and the watcher reports zero alerts. +# +# 2. `concurrency.queue` (GitHub changelog, 2026-05-07) is what keeps pdp-tester runs waiting +# instead of cancelling each other in tests.yml. actionlint 1.7.12 only knows `group` and +# `cancel-in-progress` (rhysd/actionlint#657), so it reports the documented key as a syntax +# error. Scoped to that file and that message, so a misspelled concurrency key still fails. +# +# Removal gate: drop this entry once `actionlint .github/workflows/tests.yml` passes without +# it after an actionlint upgrade. With both entries gone, delete this file. paths: .github/workflows/scheduled-security-scan.yml: ignore: - 'unknown permission scope "vulnerability-alerts"' + .github/workflows/tests.yml: + ignore: + - 'unexpected key "queue" for "concurrency" section' diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 0bd8a619..3a6d0fdd 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -271,6 +271,15 @@ jobs: pdp-tester: runs-on: ubuntu-24.04 needs: build-pdp-image + # One run at a time across the repository - PRs, pushes and releases, which reach this + # job through release.yml - because every run writes to the same staging environment and + # overlapping runs failed each other's sync cases (PER-16929). `queue: max` lets up to 100 + # runs wait their turn. The default queue holds one, and a newer run cancels the one already + # waiting, which would leave another PR with a cancelled required check. GitHub rejects + # `queue: max` together with cancel-in-progress, so a running test always finishes. + concurrency: + group: pdp-tester-staging + queue: max # Cap the run so a hung tester fails fast instead of holding the runner # until GitHub's 360-minute default. The old k3d path was bounded by # `kubectl wait --timeout=600s`; the tester's own max_running_time only From f50b276a0acb6cc23d0fbe7122a1b6681274ed76 Mon Sep 17 00:00:00 2001 From: Zeev Manilovich Date: Thu, 8 Oct 2026 00:00:32 +0300 Subject: [PATCH 20/21] Rename the Python project from horizon to permit-pdp (PER-16930) Since the move to uv, GitHub's dependency graph reads the project's own name from pyproject.toml and uv.lock, and matched "horizon" to the unrelated PyPI project of that name, raising that project's Dependabot alerts on this repository. Only the distribution name changes. The import package is still horizon/, and nothing reads the name: the project is virtual (package = false), and the Dockerfile and the pytests job export the lock with --no-emit-project. uv lock only renames the root entry; the exported requirements are identical before and after (79 pins), and the pip check on Python 3.13 still passes. Co-Authored-By: Claude Opus 5.5 --- pyproject.toml | 5 +- uv.lock | 154 ++++++++++++++++++++++++------------------------- 2 files changed, 81 insertions(+), 78 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index d178e098..9530a211 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,5 +1,8 @@ [project] -name = "horizon" +# The distribution name; the import package is still `horizon` (horizon/). Not "horizon": GitHub's +# dependency graph matched that name to an unrelated PyPI project and raised its alerts here +# (PER-16930). Nothing reads this name: the image and CI export the lock with --no-emit-project. +name = "permit-pdp" version = "0.2.0" description = "The Python side of the Permit.io PDP: OPAL client, policy-store sync and the PDP API." # uv.lock is resolved for the interpreter the image ships (Dockerfile: python:3.13-alpine3.23) diff --git a/uv.lock b/uv.lock index 02610e4b..d56f3071 100644 --- a/uv.lock +++ b/uv.lock @@ -559,83 +559,6 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/04/4b/29cac41a4d98d144bf5f6d33995617b185d14b22401f75ca86f384e87ff1/h11-0.16.0-py3-none-any.whl", hash = "sha256:63cf8bbe7522de3bf65932fda1d9c2772064ffb3dae62d55932da54b31cb6c86", size = 37515, upload-time = "2025-04-24T03:35:24.344Z" }, ] -[[package]] -name = "horizon" -version = "0.2.0" -source = { virtual = "." } -dependencies = [ - { name = "aiohttp" }, - { name = "cryptography" }, - { name = "ddtrace" }, - { name = "fastapi" }, - { name = "fastapi-websocket-pubsub" }, - { name = "fastapi-websocket-rpc" }, - { name = "gitpython" }, - { name = "gunicorn" }, - { name = "httpx" }, - { name = "jinja2" }, - { name = "logzio-python-handler" }, - { name = "opal-client" }, - { name = "opal-common" }, - { name = "protobuf" }, - { name = "pydantic", extra = ["email"] }, - { name = "requests" }, - { name = "scalar-fastapi" }, - { name = "starlette" }, - { name = "tenacity" }, - { name = "typer" }, - { name = "urllib3" }, - { name = "uvicorn", extra = ["standard"] }, - { name = "websockets" }, -] - -[package.dev-dependencies] -dev = [ - { name = "aioresponses" }, - { name = "pytest" }, - { name = "pytest-asyncio" }, - { name = "pytest-timeout" }, - { name = "pyyaml" }, - { name = "ty" }, -] - -[package.metadata] -requires-dist = [ - { name = "aiohttp", specifier = ">=3.14.3,<4" }, - { name = "cryptography", specifier = ">=50.0.0,<51" }, - { name = "ddtrace", specifier = "==3.19.8" }, - { name = "fastapi", specifier = ">=0.124.0,<1" }, - { name = "fastapi-websocket-pubsub", specifier = "==1.0.1" }, - { name = "fastapi-websocket-rpc", specifier = "==0.1.29" }, - { name = "gitpython", specifier = ">=3.1.59,<4" }, - { name = "gunicorn", specifier = ">=23.0.0,<24" }, - { name = "httpx", specifier = ">=0.27.0,<1" }, - { name = "jinja2", specifier = ">=3.1.2,<4" }, - { name = "logzio-python-handler" }, - { name = "opal-client", specifier = "==0.9.6" }, - { name = "opal-common", specifier = "==0.9.6" }, - { name = "protobuf", specifier = ">=7.36.2" }, - { name = "pydantic", extras = ["email"], specifier = ">=1.9.1,<2" }, - { name = "requests", specifier = ">=2.32.4,<3" }, - { name = "scalar-fastapi", specifier = "==1.8.2" }, - { name = "starlette", specifier = "==0.50.0" }, - { name = "tenacity", specifier = ">=8.0.1,<9" }, - { name = "typer", specifier = ">=0.27.2,<1" }, - { name = "urllib3", specifier = ">=2.8.0,<3" }, - { name = "uvicorn", extras = ["standard"], specifier = ">=0.54.0,<1" }, - { name = "websockets", specifier = "==17.0" }, -] - -[package.metadata.requires-dev] -dev = [ - { name = "aioresponses" }, - { name = "pytest" }, - { name = "pytest-asyncio" }, - { name = "pytest-timeout" }, - { name = "pyyaml" }, - { name = "ty", specifier = "==0.0.84" }, -] - [[package]] name = "httpcore" version = "1.0.9" @@ -947,6 +870,83 @@ redis = [ { name = "asyncio-redis" }, ] +[[package]] +name = "permit-pdp" +version = "0.2.0" +source = { virtual = "." } +dependencies = [ + { name = "aiohttp" }, + { name = "cryptography" }, + { name = "ddtrace" }, + { name = "fastapi" }, + { name = "fastapi-websocket-pubsub" }, + { name = "fastapi-websocket-rpc" }, + { name = "gitpython" }, + { name = "gunicorn" }, + { name = "httpx" }, + { name = "jinja2" }, + { name = "logzio-python-handler" }, + { name = "opal-client" }, + { name = "opal-common" }, + { name = "protobuf" }, + { name = "pydantic", extra = ["email"] }, + { name = "requests" }, + { name = "scalar-fastapi" }, + { name = "starlette" }, + { name = "tenacity" }, + { name = "typer" }, + { name = "urllib3" }, + { name = "uvicorn", extra = ["standard"] }, + { name = "websockets" }, +] + +[package.dev-dependencies] +dev = [ + { name = "aioresponses" }, + { name = "pytest" }, + { name = "pytest-asyncio" }, + { name = "pytest-timeout" }, + { name = "pyyaml" }, + { name = "ty" }, +] + +[package.metadata] +requires-dist = [ + { name = "aiohttp", specifier = ">=3.14.3,<4" }, + { name = "cryptography", specifier = ">=50.0.0,<51" }, + { name = "ddtrace", specifier = "==3.19.8" }, + { name = "fastapi", specifier = ">=0.124.0,<1" }, + { name = "fastapi-websocket-pubsub", specifier = "==1.0.1" }, + { name = "fastapi-websocket-rpc", specifier = "==0.1.29" }, + { name = "gitpython", specifier = ">=3.1.59,<4" }, + { name = "gunicorn", specifier = ">=23.0.0,<24" }, + { name = "httpx", specifier = ">=0.27.0,<1" }, + { name = "jinja2", specifier = ">=3.1.2,<4" }, + { name = "logzio-python-handler" }, + { name = "opal-client", specifier = "==0.9.6" }, + { name = "opal-common", specifier = "==0.9.6" }, + { name = "protobuf", specifier = ">=7.36.2" }, + { name = "pydantic", extras = ["email"], specifier = ">=1.9.1,<2" }, + { name = "requests", specifier = ">=2.32.4,<3" }, + { name = "scalar-fastapi", specifier = "==1.8.2" }, + { name = "starlette", specifier = "==0.50.0" }, + { name = "tenacity", specifier = ">=8.0.1,<9" }, + { name = "typer", specifier = ">=0.27.2,<1" }, + { name = "urllib3", specifier = ">=2.8.0,<3" }, + { name = "uvicorn", extras = ["standard"], specifier = ">=0.54.0,<1" }, + { name = "websockets", specifier = "==17.0" }, +] + +[package.metadata.requires-dev] +dev = [ + { name = "aioresponses" }, + { name = "pytest" }, + { name = "pytest-asyncio" }, + { name = "pytest-timeout" }, + { name = "pyyaml" }, + { name = "ty", specifier = "==0.0.84" }, +] + [[package]] name = "pluggy" version = "1.6.0" From 85e5658735a6a2d608014f63083d2db69adeb87f Mon Sep 17 00:00:00 2001 From: Zeev Manilovich Date: Thu, 8 Oct 2026 00:00:59 +0300 Subject: [PATCH 21/21] Document building the PDP image with upstream OPA (PER-16934) The README's build section did not say that the default build clones the private permit-opa repository, or that PDP_VANILLA=true builds with upstream OPA instead (the Makefile maps it to OPA_BUILD=vanilla). Say both, and that an upstream-OPA image cannot evaluate Permit-generated policies, which call builtins only Permit's OPA build has: those need the published permitio/pdp-v2 image. Co-Authored-By: Claude Opus 5.5 --- README.md | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/README.md b/README.md index de3e9ddd..52321956 100644 --- a/README.md +++ b/README.md @@ -55,6 +55,9 @@ PDP_CONTROL_PLANE=https://api.permit.io PDP_API_KEY= uv run uvicor ``` ## Building a Custom PDP Docker image +The build compiles Permit's OPA build from the private `permitio/permit-opa` repository, which +`build_opal_bundle.sh` clones over SSH into `../permit-opa`. + For ARM architecture: ``` VERSION= make build-arm64 @@ -64,6 +67,16 @@ For AMD64 architecture: VERSION= make build-amd64 ``` +### Building without access to permit-opa +`PDP_VANILLA=true` builds the image with upstream OPA instead (`OPA_BUILD=vanilla`), for +development without access to `permitio/permit-opa`. It works with every build target: +``` +PDP_VANILLA=true VERSION= make build +``` +Permit-generated policies call builtins that exist only in Permit's OPA build, so an image built +this way cannot evaluate them. To evaluate Permit policies, use the published `permitio/pdp-v2` +image. + ### Running the image in development mode ``` VERSION= API_KEY= make run