From 867f00ec2c08aaf99b70b7b78b84cebe8ef9a285 Mon Sep 17 00:00:00 2001 From: Oleksandr Piskun Date: Thu, 22 Oct 2026 12:10:04 +0000 Subject: [PATCH] fix: user-info lookups leave out client headers that break them Signed-off-by: Oleksandr Piskun --- .github/workflows/tests-deploy-docker.yml | 5 +++ haproxy_agent.py | 27 +++++++++++- tests/unit/test_requests.py | 50 +++++++++++++++++++++-- 3 files changed, 77 insertions(+), 5 deletions(-) diff --git a/.github/workflows/tests-deploy-docker.yml b/.github/workflows/tests-deploy-docker.yml index 76affc7..2147edf 100644 --- a/.github/workflows/tests-deploy-docker.yml +++ b/.github/workflows/tests-deploy-docker.yml @@ -549,6 +549,11 @@ jobs: test_req admin "-u usr:password" 403 test_req admin "-u admin:admin" 200 + # A chunked request with credentials, sent to HaRP directly because the nginx in front buffers bodies + code=$(printf 'body' | curl -sS -m 10 -o /dev/null -w "%{http_code}" -u usr:password -X GET \ + -H "Transfer-Encoding: chunked" --data-binary @- http://127.0.0.1:8780/exapps/app-skeleton-python/user) + test "$code" = 200 + # clients get blacklisted for 5 bad requests for 5 minutes sleep 301 diff --git a/haproxy_agent.py b/haproxy_agent.py index 2b05210..7d4626d 100644 --- a/haproxy_agent.py +++ b/haproxy_agent.py @@ -98,7 +98,24 @@ def _in_cluster_api_server() -> str: NC_REQ_URL = NC_INSTANCE_URL.removesuffix("/").removesuffix("/index.php") EX_APP_URL = f"{NC_REQ_URL}/index.php/apps/app_api/harp/exapp-meta" USER_INFO_URL = f"{NC_REQ_URL}/index.php/apps/app_api/harp/user-info" -EXCLUDE_HEADERS_USER_INFO = {"host", "content-length"} +# Client headers the user-info lookup leaves out, as it does every content-* header +EXCLUDE_HEADERS_USER_INFO = { + "accept-encoding", + "connection", + "expect", + "host", + "keep-alive", + "proxy-authenticate", + "proxy-authorization", + "proxy-connection", + "range", + "te", + "trailer", + "transfer-encoding", + "upgrade", +} +# aiohttp refuses to send a header that contains one of these +UNSENDABLE_HEADER_CHARS = re.compile(r"[\x00-\x08\x0a-\x1f\x7f]") # Keep total below the SPOE `timeout processing` (30s); aiohttp's default is 5 minutes. NC_HTTP_TIMEOUT = aiohttp.ClientTimeout(total=25.0, connect=5.0) @@ -1033,7 +1050,13 @@ async def _get_or_fetch_exapp(exapp_id: str) -> ExApp | None: async def nc_get_user(app_id: str, all_headers: dict[str, str]) -> NcUser | None: - ext_headers = {k: v for k, v in all_headers.items() if k.lower() not in EXCLUDE_HEADERS_USER_INFO} + ext_headers = { + k: v + for k, v in all_headers.items() + if k.lower() not in EXCLUDE_HEADERS_USER_INFO + and not k.lower().startswith("content-") + and not UNSENDABLE_HEADER_CHARS.search(k + v) + } log_headers("Requesting user info for ExApp '%s' with headers %s", ext_headers, app_id) async with _get_nc_session().get( USER_INFO_URL, diff --git a/tests/unit/test_requests.py b/tests/unit/test_requests.py index f6fb2b9..b87430a 100644 --- a/tests/unit/test_requests.py +++ b/tests/unit/test_requests.py @@ -339,7 +339,9 @@ def test_k8s_create_refuses_an_invalid_role_before_contacting_the_cluster(no_doc assert text.startswith("Payload validation error: role_suffix: ") -def test_user_info_requests_keep_the_credentials_but_the_log_does_not(monkeypatch, caplog): +@pytest.fixture +def user_info_sent(monkeypatch) -> dict: + """Answer user-info lookups with the admin user; the returned dict gets the headers the lookup sent.""" sent = {} class Response: @@ -360,17 +362,59 @@ def get(self, url, headers, params): return Response() monkeypatch.setattr(agent, "_get_nc_session", Session) + return sent + + +def test_user_info_requests_keep_the_credentials_but_the_log_does_not(user_info_sent, caplog): headers = {"authorization": "Basic YWRtaW46YWRtaW4=", "cookie": "oc_sessionPassphrase=passphrase-value"} with caplog.at_level(logging.DEBUG, logger=agent.LOGGER.name): user = asyncio.run(agent.nc_get_user("app", headers)) assert user.user_id == "admin" - assert sent["authorization"] == "Basic YWRtaW46YWRtaW4=" # Nextcloud still gets them - assert sent["cookie"] == "oc_sessionPassphrase=passphrase-value" + assert user_info_sent["authorization"] == "Basic YWRtaW46YWRtaW4=" # Nextcloud still gets them + assert user_info_sent["cookie"] == "oc_sessionPassphrase=passphrase-value" assert "Requesting user info for ExApp 'app'" in caplog.text assert "YWRtaW46YWRtaW4=" not in caplog.text assert "passphrase-value" not in caplog.text +def test_user_info_requests_leave_out_the_client_headers_that_break_them(user_info_sent): + headers = agent.parse_headers( + "\n".join( + [ + "Host: cloud.example.com", + "Authorization: Basic YWRtaW46YWRtaW4=", + "Cookie: oc_sessionPassphrase=passphrase-value", + "User-Agent: curl/8.22.0", + "X-Forwarded-For: 198.51.100.7", + "Transfer-Encoding: chunked", + "Expect: 100-continue", + "Content-Type: application/octet-stream", + "Connection: keep-alive, Upgrade", + "Keep-Alive: timeout=5", + "Upgrade: websocket", + "TE: trailers", + "Accept-Encoding: gzip, deflate, br, zstd", + "Proxy-Authorization: Basic cHJveHk6c2VjcmV0", + "Range: bytes=1000-", + ] + ) + ) + assert asyncio.run(agent.nc_get_user("app", headers)).user_id == "admin" + assert user_info_sent == { + "authorization": "Basic YWRtaW46YWRtaW4=", + "cookie": "oc_sessionPassphrase=passphrase-value", + "user-agent": "curl/8.22.0", + "x-forwarded-for": "198.51.100.7", + "harp-shared-key": KEY, + } + + +def test_user_info_requests_leave_out_a_header_with_a_control_character(user_info_sent): + headers = {"authorization": "Basic YWRtaW46YWRtaW4=", "x-custom": "a\x01b"} + assert asyncio.run(agent.nc_get_user("app", headers)).user_id == "admin" + assert user_info_sent == {"authorization": "Basic YWRtaW46YWRtaW4=", "harp-shared-key": KEY} + + def test_create_refuses_a_read_write_system_mount_once_enforced(no_docker_engine, monkeypatch): # By default the read-write entries only warn (see test_bind_mounts.py); HP_EXAPP_BIND_MOUNTS_ENFORCE_ALL # empties the warn-only set.