Repository navigation
fix: apply trusted-host, body-size and optional auth checks to the privacy proxy - #4196
Open
pardeep-singh wants to merge 1 commit into
Open
pardeep-singh wants to merge 1 commit into
pardeep-singh wants to merge 1 commit into
Conversation
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes #3898.
openmed.service.privacy_proxy.create_appbuilt a FastAPI app with no inbound checks. There was no trusted-host allowlist, no body-size bound and no authentication. With a synthetic transport that counts calls, master did this:Host: rebound.attacker.example.test200, transport called400 bad_request, no call200, transport called413 request_size_exceeded, no call200, transport called413 privacy_proxy_limit_exceeded, no callSo the documented loopback deployment (
uvicorn --host 127.0.0.1) could be reached through DNS rebinding and could spend the operator's upstream model or credentials.The proxy now reuses the REST service controls instead of adding its own:
ErrorEnvelopeTrustedHostMiddlewarewith the loopback default.ServiceAuth, for opt-in API-key or JWT checks.BoundedRequestBodyMiddlewarewithOperationalLimits.Message count, total text characters and streaming duration are now bounded too.
Type of Change
Changes Made
openmed/service/privacy_proxy/app.py:create_appadds the shared middlewares in this order: trusted host, then auth (only when enabled), then the body bound. The two header checks reject a request before any body bytes are read.trusted_hosts,operational_limits,limitsandauth_config. Each one left asNoneis read from the environment with a safe default.PrivacyProxy.preparechecks the message count and the total characters of every text value it would redact. This runs before the extractor and the transport. The count walk is iterative, so deep nesting cannot exhaust the stack.PrivacyProxyLimitError(413 privacy_proxy_limit_exceeded, reasonsmessage_count_exceededandtext_length_exceeded). NewPrivacyProxyTimeoutError(privacy_proxy_timeout, reasonstream_duration_exceeded), sent as an SSE error event with no[DONE]marker.app.state, so a deployment check can read them without sending a request.openmed/service/privacy_proxy/settings.py: thePrivacyProxyLimitsdataclass, theOPENMED_PRIVACY_PROXY_*settings, and the proxy route scopes. Both completion routes needpii:write, the same scope asPOST /privacy-gateway/complete.openmed/service/privacy_proxy/__init__.py: exportsPrivacyProxyLimits.openmed/service/security_headers.py:parse_trusted_hoststakes an optionalenv_varso its error names the right setting. The default behavior is unchanged.openmed/service/request_limits.py:request_limits_from_envtakes an optionalenv_var, for the same reason. The default behavior is unchanged.openmed/service/api_compatibility.py: registers the two new stable error codes.docs/service/privacy-proxy.md: inbound protections, a settings table, auth setup, streaming timeout behavior and an error-code table.CHANGELOG.md: Unreleased / Security entry.Testing
New
tests/unit/service/privacy_proxy/test_app_inbound_protections.pyhas 38 tests. Each one uses a synthetic extractor and a transport that records calls:127.0.0.1.<attacker>andlocalhost.<attacker>names, are rejected before parsing and before any extractor or transport call. The body is invalid JSON, so a400 bad_request(notinvalid_json) shows which check ran first.127.0.0.1,localhost,[::1], with and without a port) still work. Env and argument overrides work.*and an empty allowlist are refused./healthstays open. The sharedOPENMED_SERVICE_AUTH_*settings turn it on. Proxy routes still need credentials with deny-by-default off. Auth is off by default.privacy_proxy_limit_exceededwith no transport call. A request exactly at the limit passes.[DONE].OPENMED_PHIandmappingappear in neither the response nor the captured logs (caplogat DEBUG).Before the fix, the new module had 33 failed and 5 passed. The 5 that passed were the loopback and auth-off controls. After the fix, all 38 pass, and the existing proxy tests are unchanged.
Documentation
Code Quality
Dependencies
Notes
OPENMED_SERVICE_AUTH_*, so one credential contract covers both listeners. It stays off unlessOPENMED_SERVICE_AUTH_ENABLED=true. As a result, a shell that enables REST auth also enables it for the proxy. That fails closed. To use other scopes, pass anauth_configwith your ownroute_scopes.OPENMED_SERVICE_AUTH_ROUTE_SCOPEScannot name proxy routes, because it only accepts REST routes.OPENMED_PRIVACY_PROXY_*, not the REST names. A REST allowlist for a public hostname should not lock the loopback proxy out.redaction_appputs trusted host outside its body bound. The proxy follows the "trusted host, auth, then body" order.testserveris not trusted. The REST service does not trust it either. The existing proxy tests already usehttp://127.0.0.1.200and some chunks may already be sent, so it arrives as an SSE error event. The exception's 504 status is never sent.openmed/service/security_profile.py) is not on master yet. This PR keeps the resolved settings onapp.stateand documents them, so Add a fail-closed production service security-profile self-check #3552 can read them. It does not add a registry of its own.Related Issues
Closes #3898
Related to #3552, #3581, #2294, #658, #3543