feat: add X-Trusted-Proxy header validation for event streams#1604
Open
mkanoor wants to merge 2 commits into
Open
feat: add X-Trusted-Proxy header validation for event streams#1604mkanoor wants to merge 2 commits into
mkanoor wants to merge 2 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds trusted-proxy validation for external event stream POSTs, introduces a new default setting and Docker Compose environment values, changes mTLS failure messages to a generic string, and updates unit and integration tests. ChangesTrusted proxy and authentication changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/integration/api/conftest.py (1)
21-23: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse exact module matching for fixture bypass
Line 21 uses substring matching on
request.node.nodeid, which is brittle and can accidentally bypass the override for unrelated tests. Match the test file path explicitly.Suggested change
- if "test_event_stream_trusted_proxy" in request.node.nodeid: + test_file = request.node.nodeid.split("::", 1)[0] + if test_file.endswith("test_event_stream_trusted_proxy.py"): yield return🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/api/conftest.py` around lines 21 - 23, The fixture bypass at lines 21-23 uses substring matching with the `in` operator on `request.node.nodeid`, which is too broad and can accidentally match unrelated tests. Replace the substring matching logic with an explicit check against the full test file path or module name to ensure only the intended test (test_event_stream_trusted_proxy) bypasses the fixture override. Use exact matching on the nodeid by comparing against the complete test path rather than checking if the test name appears anywhere in the nodeid string.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/integration/api/conftest.py`:
- Around line 21-23: The fixture bypass at lines 21-23 uses substring matching
with the `in` operator on `request.node.nodeid`, which is too broad and can
accidentally match unrelated tests. Replace the substring matching logic with an
explicit check against the full test file path or module name to ensure only the
intended test (test_event_stream_trusted_proxy) bypasses the fixture override.
Use exact matching on the nodeid by comparing against the complete test path
rather than checking if the test name appears anywhere in the nodeid string.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 2fd1b541-1a43-4437-9828-26750137e2b7
📒 Files selected for processing (8)
src/aap_eda/api/event_stream_authentication.pysrc/aap_eda/api/views/external_event_stream.pysrc/aap_eda/settings/defaults.pytests/integration/api/conftest.pytests/integration/api/test_event_stream_trusted_proxy.pytests/unit/test_mtls_authentication.pytools/docker/docker-compose-mac-pg-mtls.ymltools/docker/docker-compose-mac.yml
mkanoor
force-pushed
the
x_trusted_proxy
branch
3 times, most recently
from
4e628a2 to
3211aaf
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #1604 +/- ##
==========================================
+ Coverage 92.48% 92.49% +0.01%
==========================================
Files 244 244
Lines 11248 11264 +16
==========================================
+ Hits 10403 10419 +16
Misses 845 845
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tools/docker/docker-compose-stage.yaml`:
- Line 38: The stage Compose default for EDA_EVENT_STREAM_REQUIRE_TRUSTED_PROXY
is disabling trusted-proxy validation by default, which conflicts with the
secure default used by EVENT_STREAM_REQUIRE_TRUSTED_PROXY in the settings and
the X-Trusted-Proxy check in external_event_stream. Update the default in
docker-compose-stage.yaml to keep enforcement enabled unless explicitly
overridden, so the stage stack preserves the anti-spoofing behavior while still
allowing local development to opt out.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: ee2ab950-836f-47cb-8bbe-048d9534d8c5
📒 Files selected for processing (10)
src/aap_eda/api/event_stream_authentication.pysrc/aap_eda/api/views/external_event_stream.pysrc/aap_eda/settings/defaults.pytests/integration/api/conftest.pytests/integration/api/test_event_stream_trusted_proxy.pytests/unit/test_mtls_authentication.pytools/docker/docker-compose-dev.yamltools/docker/docker-compose-mac-pg-mtls.ymltools/docker/docker-compose-mac.ymltools/docker/docker-compose-stage.yaml
✅ Files skipped from review due to trivial changes (1)
- tools/docker/docker-compose-mac-pg-mtls.yml
🚧 Files skipped from review as they are similar to previous changes (7)
- tests/integration/api/conftest.py
- src/aap_eda/settings/defaults.py
- src/aap_eda/api/event_stream_authentication.py
- tools/docker/docker-compose-mac.yml
- src/aap_eda/api/views/external_event_stream.py
- tests/unit/test_mtls_authentication.py
- tests/integration/api/test_event_stream_trusted_proxy.py
Contributor
|
/run-atf-tests |
wfealdel
reviewed
Jun 24, 2026
| # Default: True (secure by default - requires Gateway/Envoy Proxy) | ||
| # Set to False for local development without proxy: | ||
| # export EDA_EVENT_STREAM_REQUIRE_TRUSTED_PROXY=False | ||
| EVENT_STREAM_REQUIRE_TRUSTED_PROXY: bool = True |
Contributor
There was a problem hiding this comment.
Question: Are the following documented elsewhere?
- The upstream
trusted_header_timeout(default 1000ms) — in cross-region deployments this window could cause legitimate request rejections. Deployers would benefit from knowing the tuning knob exists. - Key rotation procedures for
ANSIBLE_BASE_JWT_KEY— during rotation there's a window where Gateway and EDA may have mismatched keys, rejecting all event stream requests.
These seem out of scope for this PR, just want to confirm they're covered.
Contributor
Author
There was a problem hiding this comment.
@wfealdel This is definitely an issue but it might have to be in a different PR, how the ANSIBLE_BASE_JWT_KEY is rotated or they might go with different headers for establishing trust between services.
✅ Test Results - PASSEDSummary
Pass Rate: 75.4% |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/integration/api/test_event_stream_trusted_proxy.py (1)
66-89: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared event-stream setup into a fixture/helper.
The same credential + event stream + payload setup is duplicated in every test, which makes future contract updates error-prone. Please centralize this in a fixture/helper and keep each test focused on the header variant only.
As per path instructions, this is a maintainability-focused issue (“Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.”).Also applies to: 118-140, 167-190, 215-238, 264-287, 330-353, 384-407
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/api/test_event_stream_trusted_proxy.py` around lines 66 - 89, Centralize the repeated event-stream setup used across the trusted proxy tests by moving the shared credential, event stream, and payload creation logic into a fixture or helper, rather than duplicating the same create_event_stream_credential, create_event_stream, get_default_test_org, and payload preparation steps in each test. Keep the individual test cases focused only on the header-specific behavior they validate, and have them consume the shared setup object/data from the new fixture/helper so future contract changes only need to be updated in one place.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/integration/api/test_event_stream_trusted_proxy.py`:
- Line 295: The expiry check in the trusted proxy integration test relies on a
real wall-clock delay via time.sleep, which makes the suite slow and flaky.
Update the test around the expiry validation logic in
test_event_stream_trusted_proxy to use a deterministic mechanism instead, such
as overriding the timeout/expiry source or mocking time progression, so the
assertion can run immediately and reliably without sleeping.
---
Nitpick comments:
In `@tests/integration/api/test_event_stream_trusted_proxy.py`:
- Around line 66-89: Centralize the repeated event-stream setup used across the
trusted proxy tests by moving the shared credential, event stream, and payload
creation logic into a fixture or helper, rather than duplicating the same
create_event_stream_credential, create_event_stream, get_default_test_org, and
payload preparation steps in each test. Keep the individual test cases focused
only on the header-specific behavior they validate, and have them consume the
shared setup object/data from the new fixture/helper so future contract changes
only need to be updated in one place.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 3b61678d-1e2f-4dc0-953e-2ed3f4e1bb08
📒 Files selected for processing (10)
src/aap_eda/api/event_stream_authentication.pysrc/aap_eda/api/views/external_event_stream.pysrc/aap_eda/settings/defaults.pytests/integration/api/conftest.pytests/integration/api/test_event_stream_trusted_proxy.pytests/unit/test_mtls_authentication.pytools/docker/docker-compose-dev.yamltools/docker/docker-compose-mac-pg-mtls.ymltools/docker/docker-compose-mac.ymltools/docker/docker-compose-stage.yaml
🚧 Files skipped from review as they are similar to previous changes (9)
- tools/docker/docker-compose-dev.yaml
- src/aap_eda/api/event_stream_authentication.py
- tests/integration/api/conftest.py
- tests/unit/test_mtls_authentication.py
- tools/docker/docker-compose-stage.yaml
- tools/docker/docker-compose-mac-pg-mtls.yml
- tools/docker/docker-compose-mac.yml
- src/aap_eda/settings/defaults.py
- src/aap_eda/api/views/external_event_stream.py
mkanoor
force-pushed
the
x_trusted_proxy
branch
2 times, most recently
from
4b66a12 to
d808720
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pyproject.toml (1)
101-101: 📐 Maintainability & Code Quality | 🔵 TrivialRefine test dependency version constraint for reproducibility.
At Line 101,
freezegun = "*"removes version constraints from the dependency manifest. Althoughpoetry.lockcurrently pins the version to 1.5.5, the manifest's wildcard resolution relies on the lockfile remaining in sync. Ifpoetry.lockis regenerated, the setup may arbitrarily upgrade to an incompatible upstream version, risking CI instability. Update to a specific version or compatible range (e.g.,freezegun = "^1.5.5") to ensure deterministic resolution across all environments.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pyproject.toml` at line 101, The test dependency declaration for freezegun is currently unconstrained, which can lead to non-deterministic installs if the lockfile changes. Update the dependency entry in the manifest to a fixed version or a compatible range such as the one already pinned by the lockfile, and keep it aligned with poetry.lock so resolution stays reproducible across environments.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pyproject.toml`:
- Line 101: The test dependency declaration for freezegun is currently
unconstrained, which can lead to non-deterministic installs if the lockfile
changes. Update the dependency entry in the manifest to a fixed version or a
compatible range such as the one already pinned by the lockfile, and keep it
aligned with poetry.lock so resolution stays reproducible across environments.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 9a2f8842-b1f0-4875-97e6-18ba9778ec68
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
pyproject.tomlsrc/aap_eda/api/event_stream_authentication.pysrc/aap_eda/api/views/external_event_stream.pysrc/aap_eda/settings/defaults.pytests/integration/api/conftest.pytests/integration/api/test_event_stream_trusted_proxy.pytests/unit/test_mtls_authentication.pytools/docker/docker-compose-dev.yamltools/docker/docker-compose-mac-pg-mtls.ymltools/docker/docker-compose-mac.ymltools/docker/docker-compose-stage.yaml
✅ Files skipped from review due to trivial changes (2)
- tools/docker/docker-compose-mac.yml
- tools/docker/docker-compose-mac-pg-mtls.yml
🚧 Files skipped from review as they are similar to previous changes (8)
- tests/unit/test_mtls_authentication.py
- tools/docker/docker-compose-dev.yaml
- tests/integration/api/conftest.py
- src/aap_eda/api/event_stream_authentication.py
- tests/integration/api/test_event_stream_trusted_proxy.py
- src/aap_eda/settings/defaults.py
- tools/docker/docker-compose-stage.yaml
- src/aap_eda/api/views/external_event_stream.py
wfealdel
approved these changes
Jun 25, 2026
Add cryptographic validation of X-Trusted-Proxy header to ensure external event stream requests originate from trusted Gateway/Envoy Proxy. This prevents spoofing of forwarded headers and strengthens request authentication. Key Changes: - Add EVENT_STREAM_REQUIRE_TRUSTED_PROXY setting (default: True, secure by default) - Implement _validate_trusted_proxy_header() method using django-ansible-base - Validate header signature and timestamp (1000ms window) - Add comprehensive test suite (6 tests) for validation scenarios - Add autouse fixture to disable validation for non-proxy tests - Configure docker-compose files for local development (default: False) Security Improvements: - Change mTLS authentication error to generic "Authentication failed" - Log detailed Subject mismatch info internally without leaking in response - Prevent information disclosure about authentication methods and credentials Testing: - 70 event stream integration tests passing - 20 mTLS authentication unit tests passing - All tests pass black/isort/flake8 checks Local Development: - Set EDA_EVENT_STREAM_REQUIRE_TRUSTED_PROXY=False for direct testing - Production deployments use default True (validation enabled) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
ptoscano
approved these changes
Jun 26, 2026
|
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
5 participants
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.
Summary
Add cryptographic validation of X-Trusted-Proxy header to ensure external event stream requests originate from trusted Gateway/Envoy Proxy. This prevents spoofing of forwarded headers and strengthens request authentication.
Key Changes
Security Improvements
Testing
Local Development
Developers can disable validation for local testing:
export EDA_EVENT_STREAM_REQUIRE_TRUSTED_PROXY=FalseOr use docker-compose which sets this to False by default.
Production deployments use the default value of
True(validation enabled).Implementation Details
The X-Trusted-Proxy header format is:
{timestamp_ns}-{hex_signature}🤖 Generated with Claude Code
Summary by CodeRabbit
EVENT_STREAM_REQUIRE_TRUSTED_PROXY(on by default), returningInvalid requestwhen the header is missing/invalid."Authentication failed"message instead of exposing subject-mismatch details.freezegunfor time-based checks.