Skip to content

feat: enforce optional inbound HMAC request signatures on configured routes - #4200

Open
pardeep-singh wants to merge 1 commit into
maziyarpanahi:masterfrom
pardeep-singh:pardeep/issue3924-inbound-hmac
Open

pardeep-singh wants to merge 1 commit into
maziyarpanahi:masterfrom
pardeep-singh:pardeep/issue3924-inbound-hmac

Conversation

@pardeep-singh

Copy link
Copy Markdown
Contributor

Description

Fixes #3924.

On master, openmed/service/signing.py already had verify_request_signature
and NonceCache, and the docs said the scheme was available for inbound
verification. But the service had no setting or middleware to require it.
Integrators had to wrap the ASGI app themselves.

This PR adds opt-in enforcement. Operators list route templates and point to a
secret file. Requests to those routes must carry a valid signature, or they are
rejected with a value-free 401 before the handler runs. Everything else is
unchanged, and nothing is installed when the settings are unset.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Code refactoring
  • Performance improvement
  • Test addition/improvement

Changes Made

  • openmed/service/request_signing.py (new): settings parser, route validation, a verifier and a pure ASGI middleware. It reuses verify_request_signature, NonceCache and the existing header names. It does not reimplement the HMAC.
  • openmed/service/app.py: one install_request_signing(...) block, placed so the check runs inside both body-limit layers and before routing.
  • openmed/service/api_compatibility.py: register signature_missing, signature_invalid, signature_stale and signature_replayed as stable error categories.
  • tests/unit/service/test_request_signing_middleware.py (new): 46 tests with synthetic secrets and an injected clock.
  • docs/serving/request-signing.md: settings, route format, failure codes, middleware order, nonce cache limits and the multi-replica caveat.
  • CHANGELOG.md: Unreleased Security entry.

Testing

  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I have tested this change with different models/inputs

New tests cover:

  • a valid signature passes and the handler gets the identical body, including chunked bodies;
  • missing headers give signature_missing;
  • tampered body, path, method, timestamp, nonce, digest, prefix and secret give signature_invalid, and no nonce is consumed;
  • an injected clock past the skew window gives signature_stale in both directions;
  • a replayed nonce gives signature_replayed;
  • the handler is never called on any failure (spy);
  • path-parameter templates such as /jobs/{job_id}, and HEAD on a signed GET template;
  • unconfigured routes return the same status, body and headers with and without signature headers;
  • signing is off by default and installs no middleware;
  • startup fails closed for a missing, unreadable, empty, short or directory secret file, unknown or unregistered routes, malformed entries and bad numbers, with no path or route in the error;
  • oversized bodies still get the body-limit 413, not a signature error;
  • sentinel secret, signature, nonce, body and path text never appear in responses, caplog or /metrics.

Before the fix the new file gave 1 failed, 45 errors (the module did not
exist). After the fix all 46 pass.

PYTHONPATH=$PWD ../.venv/bin/python -m pytest tests/unit/service/test_request_signing_middleware.py -q
# 46 passed
PYTHONPATH=$PWD ../.venv/bin/python -m pytest tests/unit/service -q
# 979 passed, 5 skipped
../.venv/bin/ruff check .
# All checks passed!
../.venv/bin/ruff format --check .
# 2886 files already formatted
PYTHONPATH=$PWD ../.venv/bin/python scripts/security/check_effect_paths.py --check
# effect_paths_verified

Documentation

  • I have updated the documentation accordingly
  • I have added docstrings to new functions/classes
  • I have updated the CHANGELOG.md

Code Quality

  • I ran make format, make lint, and make format-check
  • For Swift/OpenMedKit changes, I ran make format-swift and make lint-swift
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings

Dependencies

  • I have not added any new dependencies

Notes

  • Settings: OPENMED_SERVICE_REQUEST_SIGNING_ROUTES (for example POST /jobs,GET /jobs/{job_id}), OPENMED_SERVICE_REQUEST_SIGNING_SECRET_FILE, ..._MAX_SKEW_SECONDS (300) and ..._NONCE_CACHE_SIZE (10000). Signing is on only when routes are set.
  • Route entries must be in ROUTE_POLICIES and registered on the live app. Route matching reuses route_key_for_request from auth.py, the same matcher used for route scopes.
  • The secret file is read once at startup and stripped of surrounding whitespace. It must be 32 to 4096 bytes. Errors name only the setting and are raised without chained OSError text.
  • A full nonce cache returns 503 service_busy, not one of the four 401 codes. It is a capacity limit, not a bad signature.
  • A signed GET template also guards HEAD, so a router that answers HEAD with the GET handler cannot skip the check.
  • The check runs before authentication and inside the metrics middleware. So an unsigned, unauthenticated request gets signature_missing, and signature failures are not counted in request metrics or the auth failure limiter. Body-limit 413s already behave this way.
  • The middleware keeps its own body bound (the service request-byte limit) because the outer limit only covers POST, PUT and PATCH.
  • Replay protection is per process. Each replica or worker has its own cache, and the cache is lost on restart. The docs state both.
  • app.py change is one import and one call. fix: run trusted-host, CORS and auth checks before buffering request bodies #4140 reorders the middleware in create_app, so a small rebase conflict there is expected. The fix is to keep this call inside the body-limit layers.

Related Issues

Closes #3924
Related to #3573 (outbound webhook signing and key rotation stay there)

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Enforce optional inbound HMAC request signatures on configured service routes

1 participant