CWFHEALTH-4934: add trusted-proxy authentication module#1887
Conversation
|
Warning Review limit reached
More reviews will be available in 13 minutes and 19 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (10)
WalkthroughThis PR introduces a new ChangesTrusted-Proxy Authentication Module
🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers:
🚥 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)
✨ Simplify code
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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/authentication/trusted_proxy.py`:
- Around line 80-82: Validate the Kubernetes TokenReview response before blind
casting: check that user_info.user is not None and is an instance of
kubernetes.client.V1UserInfo, and that user.username exists and is a non-empty
str before assigning sa_username; if any check fails, handle it (return an
unauthorized error/raise a clear exception or log and abort) instead of
proceeding with the cast. Use the existing variables user_info, user and
sa_username to locate the spot to add these guards and ensure the subsequent
logic only runs when the validated types/values are present.
- Around line 63-69: Restructure the Authorization-bypass logic so
health/metrics bypasses are checked explicitly before attempting to extract a
token: check request.url.path against the health paths
("/readiness","/liveness") and the metrics path ("/metrics") when
configuration.authentication_configuration.skip_for_health_probes or
skip_for_metrics are enabled and return NO_AUTH_TUPLE for those matches; if the
Authorization header is missing and no bypass matched, return an explicit
auth-failure (instead of falling through to extract_user_token); keep references
to request.headers.get("Authorization"),
configuration.authentication_configuration.skip_for_health_probes,
configuration.authentication_configuration.skip_for_metrics, request.url.path,
extract_user_token and NO_AUTH_TUPLE while harmonizing the metrics check to use
the single string "/metrics".
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e620ce8a-64f5-463e-ba2e-d9bf9084b0f4
📒 Files selected for processing (9)
src/authentication/__init__.pysrc/authentication/trusted_proxy.pysrc/authorization/middleware.pysrc/constants.pysrc/models/config.pytests/unit/authentication/test_auth.pytests/unit/authentication/test_trusted_proxy.pytests/unit/authorization/test_middleware.pytests/unit/models/config/test_dump_configuration.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: E2E: server mode / ci / group 1
- GitHub Check: E2E: library mode / ci / group 1
- GitHub Check: E2E: library mode / ci / group 3
- GitHub Check: E2E: server mode / ci / group 3
- GitHub Check: E2E Tests for Lightspeed Evaluation job
- GitHub Check: E2E: server mode / ci / group 2
- GitHub Check: E2E: library mode / ci / group 2
🧰 Additional context used
📓 Path-based instructions (5)
src/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.py: Use absolute imports for internal modules:from authentication import get_auth_dependency
Llama Stack imports: Usefrom llama_stack_client import AsyncLlamaStackClient
Checkconstants.pyfor shared constants before defining new ones
All modules must start with descriptive docstrings explaining purpose
Uselogger = get_logger(__name__)fromlog.pyfor module logging
All functions must have complete type annotations for parameters and return types, use modern syntax (str | int), and include descriptive docstrings
Use snake_case with descriptive, action-oriented names for functions (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead of modifying function parameters
Useasync deffor I/O operations and external API calls
Use standard log levels with clear purposes:debug()for diagnostic info,info()for program execution,warning()for unexpected events,error()for serious problems
All classes must have descriptive docstrings explaining purpose and use PascalCase with standard suffixes:Configuration,Error/Exception,Resolver,Interface
Abstract classes must use ABC with@abstractmethoddecorators
Follow Google Python docstring conventions with required sections: Parameters, Returns, Raises, and Attributes for classes
Files:
src/authentication/trusted_proxy.pysrc/constants.pysrc/authorization/middleware.pysrc/models/config.pysrc/authentication/__init__.py
src/constants.py
📄 CodeRabbit inference engine (AGENTS.md)
Use
constants.pyfor shared constants with descriptive comments and type hints usingFinal[type]
Files:
src/constants.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.py: Use pytest for all unit and integration tests; do not use unittest
Usepytest.mark.asynciomarker for async tests
Files:
tests/unit/authentication/test_auth.pytests/unit/authorization/test_middleware.pytests/unit/models/config/test_dump_configuration.pytests/unit/authentication/test_trusted_proxy.py
src/models/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Pydantic models must use
@model_validatorand@field_validatorfor validation and complete type annotations for all attributes, avoidingAnytype
Files:
src/models/config.py
src/**/__init__.py
📄 CodeRabbit inference engine (AGENTS.md)
Package
__init__.pyfiles must contain brief package descriptions
Files:
src/authentication/__init__.py
🧠 Learnings (2)
📚 Learning: 2026-01-12T10:58:40.230Z
Learnt from: blublinsky
Repo: lightspeed-core/lightspeed-stack PR: 972
File: src/models/config.py:459-513
Timestamp: 2026-01-12T10:58:40.230Z
Learning: In lightspeed-core/lightspeed-stack, for Python files under src/models, when a user claims a fix is done but the issue persists, verify the current code state before accepting the fix. Steps: review the diff, fetch the latest changes, run relevant tests, reproduce the issue, search the codebase for lingering references to the original problem, confirm the fix is applied and not undone by subsequent commits, and validate with local checks to ensure the issue is resolved.
Applied to files:
src/models/config.py
📚 Learning: 2026-02-25T07:46:33.545Z
Learnt from: asimurka
Repo: lightspeed-core/lightspeed-stack PR: 1211
File: src/models/responses.py:8-16
Timestamp: 2026-02-25T07:46:33.545Z
Learning: In the Python codebase, requests.py should use OpenAIResponseInputTool as Tool while responses.py uses OpenAIResponseTool as Tool. This difference is intentional due to differing schemas for input vs output tools in llama-stack-api. Apply this distinction consistently to other models under src/models (e.g., ensure request-related tools use the InputTool variant and response-related tools use the ResponseTool variant). If adding new tools, choose the corresponding InputTool or Tool class based on whether the tool represents input or output, and document the rationale in code comments.
Applied to files:
src/models/config.py
🔇 Additional comments (12)
src/constants.py (1)
122-122: LGTM!Also applies to: 132-132
src/models/config.py (4)
1268-1297: LGTM!
1321-1321: LGTM!
1370-1376: LGTM!
1430-1448: LGTM!tests/unit/authentication/test_trusted_proxy.py (1)
1-424: LGTM!tests/unit/models/config/test_dump_configuration.py (1)
157-157: LGTM!Also applies to: 367-367, 721-721, 974-974, 1207-1207, 1430-1430, 1803-1803, 2022-2022, 2241-2241, 2467-2467
src/authorization/middleware.py (1)
82-91: LGTM!tests/unit/authorization/test_middleware.py (2)
102-105: LGTM!
188-207: LGTM!src/authentication/__init__.py (1)
86-90: Update: None access is already prevented for trusted-proxy auth.
AuthenticationConfigurationhas a@model_validator(mode="after")(check_authentication_model) that raisesValueErrorwhenmodule == constants.AUTH_MOD_TRUSTED_PROXYandtrusted_proxy_configisNone; additionally, thetrusted_proxy_configurationproperty raises iftrusted_proxy_configisNone. Thetrusted_proxy_configurationaccess in theAUTH_MOD_TRUSTED_PROXYbranch is therefore guarded.tests/unit/authentication/test_auth.py (1)
44-52: Reassess attribute-name mismatch:trusted_proxy_configurationis a property alias fortrusted_proxy_config.
AuthenticationConfigurationdefines the underlying fieldtrusted_proxy_config, and itstrusted_proxy_configurationproperty returnsself.trusted_proxy_config(and errors only if that field isNone).get_auth_dependency()usestrusted_proxy_configuration, while the test setstrusted_proxy_config, so the attribute naming is consistent and noAttributeErrorshould occur.> Likely an incorrect or invalid review comment.
17375d7 to
7f2a7df
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/unit/authorization/test_middleware.py (1)
93-123:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClear resolver cache per parameterized iteration.
get_authorization_resolvers()is cached, so this parameterized test can reuse the first computed resolvers and skip exercising later auth-module branches (including trusted-proxy). Add a cache clear at test start.Suggested fix
def test_noop_auth_modules( self, mocker: MockerFixture, mock_configuration: MockType, auth_module: str, expected_types: tuple[AccessResolver, AccessResolver], ) -> None: """Test resolver selection for noop-style authentication modules.""" + get_authorization_resolvers.cache_clear() mock_configuration.authentication_configuration.module = auth_module mocker.patch("authorization.middleware.configuration", mock_configuration)🤖 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/unit/authorization/test_middleware.py` around lines 93 - 123, The parameterized test reuses cached results from get_authorization_resolvers(), so add an explicit cache clear at the start of test_noop_auth_modules to force recomputation per iteration: call get_authorization_resolvers.cache_clear() (or the appropriate cache-clear method if a different caching decorator is used) right after setting up mock_configuration and before invoking get_authorization_resolvers() in the test to ensure each auth_module branch (including trusted-proxy) is exercised.
🤖 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 `@docs/openapi.json`:
- Around line 11617-11626: The OpenAPI artifact is stale; regenerate and commit
the updated schema so docs/openapi.json matches the code. Run the provided
generation script (uv run scripts/generate_openapi_schema.py docs/openapi.json)
to regenerate the file, verify that the "trusted_proxy_config" schema and
surrounding ranges (around lines 19770-19820) are updated, and commit the
regenerated docs/openapi.json to the PR.
In `@src/authentication/trusted_proxy.py`:
- Around line 109-114: The header value read into forwarded_user can be
only-whitespace and bypass the falsy check; update the handler that reads
request.headers.get(self.config.user_header) to strip() the value, re-check
emptiness, and raise the same UnauthorizedResponse/HTTPException if the stripped
value is empty, then use the stripped value when creating the AuthTuple (or
wherever forwarded_user is used as user id/username) so downstream services
never receive a whitespace-only principal.
- Around line 116-120: The debug log currently writes the forwarded end-user
identifier (forwarded_user) in logger.debug inside trusted_proxy.py; change it
to avoid persisting PII by logging only the proxy service account (sa_username)
and whether a forwarded header was present (e.g., boolean or redacted/hash), or
replace forwarded_user with a non-identifying placeholder or hashed value;
update the logger.debug call that references sa_username and forwarded_user to
remove the raw forwarded_user and instead log sa_username and a redacted/hashed
indicator or header_present flag.
- Around line 73-74: In trusted_proxy.py inside the async __call__ flow, avoid
calling the synchronous get_user_info(token) directly; offload the TokenReview
work by running get_user_info in a threadpool (e.g., use run_in_threadpool or an
equivalent executor) so the AuthenticationV1Api.create_token_review call inside
src/authentication/k8s.get_user_info does not block the event loop. Also tighten
the forwarded_user empty check by treating whitespace-only strings as missing
(use .strip() semantics) and stop logging the raw forwarded end-user identifier
in debug logs—either omit it or log a masked/hashed version instead when
referencing forwarded_user. Ensure you reference
extract_user_token(request.headers) for token extraction and replace direct
get_user_info(token) calls with the threadpool-wrapped call.
In `@src/models/config.py`:
- Around line 1291-1296: The allowed_service_accounts field currently treats an
explicit empty list the same as None, so add a Pydantic validation to reject []
(or convert it to an explicit deny-all behavior) by implementing a validator on
allowed_service_accounts in the config model using `@field_validator` or a
`@model_validator`; specifically, in the class that defines
allowed_service_accounts, create a validator that raises a ValueError when the
value is an empty list (or alternately maps [] to a distinct sentinel that
TrustedProxyAuthDependency recognizes as deny-all), and ensure
TrustedProxyAuthDependency then enforces the allowlist only when the value is a
non-empty list (or treats the deny-all sentinel as denying all) so explicit []
does not silently open access.
In `@tests/unit/authentication/test_trusted_proxy.py`:
- Around line 124-127: The test creates a token-review mock via
mock_get_user_info and sets status.user = None but doesn't set
status.authenticated; update the fixture setup used in the test (the mocked
return value from authentication.trusted_proxy.get_user_info, i.e., the status
mock) to explicitly set status.authenticated = True so downstream code that
checks "is True" sees an authenticated token while still having user=None;
modify the mock_get_user_info.return_value (status) to include
authenticated=True alongside user=None.
---
Outside diff comments:
In `@tests/unit/authorization/test_middleware.py`:
- Around line 93-123: The parameterized test reuses cached results from
get_authorization_resolvers(), so add an explicit cache clear at the start of
test_noop_auth_modules to force recomputation per iteration: call
get_authorization_resolvers.cache_clear() (or the appropriate cache-clear method
if a different caching decorator is used) right after setting up
mock_configuration and before invoking get_authorization_resolvers() in the test
to ensure each auth_module branch (including trusted-proxy) is exercised.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d68942d4-23f4-4cde-9db9-1c7d7e176ade
📒 Files selected for processing (10)
docs/openapi.jsonsrc/authentication/__init__.pysrc/authentication/trusted_proxy.pysrc/authorization/middleware.pysrc/constants.pysrc/models/config.pytests/unit/authentication/test_auth.pytests/unit/authentication/test_trusted_proxy.pytests/unit/authorization/test_middleware.pytests/unit/models/config/test_dump_configuration.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Pylinter
- GitHub Check: unit_tests (3.12)
- GitHub Check: unit_tests (3.13)
- GitHub Check: build-pr
- GitHub Check: E2E: server mode / ci / group 2
- GitHub Check: E2E: server mode / ci / group 3
- GitHub Check: E2E: server mode / ci / group 1
- GitHub Check: E2E: library mode / ci / group 3
- GitHub Check: E2E: library mode / ci / group 1
- GitHub Check: E2E: library mode / ci / group 2
- GitHub Check: E2E Tests for Lightspeed Evaluation job
🧰 Additional context used
📓 Path-based instructions (5)
src/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.py: Use absolute imports for internal modules:from authentication import get_auth_dependency
Llama Stack imports: Usefrom llama_stack_client import AsyncLlamaStackClient
Checkconstants.pyfor shared constants before defining new ones
All modules must start with descriptive docstrings explaining purpose
Uselogger = get_logger(__name__)fromlog.pyfor module logging
All functions must have complete type annotations for parameters and return types, use modern syntax (str | int), and include descriptive docstrings
Use snake_case with descriptive, action-oriented names for functions (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead of modifying function parameters
Useasync deffor I/O operations and external API calls
Use standard log levels with clear purposes:debug()for diagnostic info,info()for program execution,warning()for unexpected events,error()for serious problems
All classes must have descriptive docstrings explaining purpose and use PascalCase with standard suffixes:Configuration,Error/Exception,Resolver,Interface
Abstract classes must use ABC with@abstractmethoddecorators
Follow Google Python docstring conventions with required sections: Parameters, Returns, Raises, and Attributes for classes
Files:
src/authorization/middleware.pysrc/constants.pysrc/authentication/__init__.pysrc/authentication/trusted_proxy.pysrc/models/config.py
src/constants.py
📄 CodeRabbit inference engine (AGENTS.md)
Use
constants.pyfor shared constants with descriptive comments and type hints usingFinal[type]
Files:
src/constants.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.py: Use pytest for all unit and integration tests; do not use unittest
Usepytest.mark.asynciomarker for async tests
Files:
tests/unit/authentication/test_auth.pytests/unit/models/config/test_dump_configuration.pytests/unit/authorization/test_middleware.pytests/unit/authentication/test_trusted_proxy.py
src/**/__init__.py
📄 CodeRabbit inference engine (AGENTS.md)
Package
__init__.pyfiles must contain brief package descriptions
Files:
src/authentication/__init__.py
src/models/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Pydantic models must use
@model_validatorand@field_validatorfor validation and complete type annotations for all attributes, avoidingAnytype
Files:
src/models/config.py
🧠 Learnings (2)
📚 Learning: 2026-01-12T10:58:40.230Z
Learnt from: blublinsky
Repo: lightspeed-core/lightspeed-stack PR: 972
File: src/models/config.py:459-513
Timestamp: 2026-01-12T10:58:40.230Z
Learning: In lightspeed-core/lightspeed-stack, for Python files under src/models, when a user claims a fix is done but the issue persists, verify the current code state before accepting the fix. Steps: review the diff, fetch the latest changes, run relevant tests, reproduce the issue, search the codebase for lingering references to the original problem, confirm the fix is applied and not undone by subsequent commits, and validate with local checks to ensure the issue is resolved.
Applied to files:
src/models/config.py
📚 Learning: 2026-02-25T07:46:33.545Z
Learnt from: asimurka
Repo: lightspeed-core/lightspeed-stack PR: 1211
File: src/models/responses.py:8-16
Timestamp: 2026-02-25T07:46:33.545Z
Learning: In the Python codebase, requests.py should use OpenAIResponseInputTool as Tool while responses.py uses OpenAIResponseTool as Tool. This difference is intentional due to differing schemas for input vs output tools in llama-stack-api. Apply this distinction consistently to other models under src/models (e.g., ensure request-related tools use the InputTool variant and response-related tools use the ResponseTool variant). If adding new tools, choose the corresponding InputTool or Tool class based on whether the tool represents input or output, and document the rationale in code comments.
Applied to files:
src/models/config.py
🪛 GitHub Actions: OpenAPI (Spectral) / 0_spectral.txt
docs/openapi.json
[error] 1-1: OpenAPI schema mismatch. Step failed because 'docs/openapi.json is out of date' (diff -u between docs/openapi.json and regenerated schema). Regenerate with: 'uv run scripts/generate_openapi_schema.py docs/openapi.json'.
🪛 GitHub Actions: OpenAPI (Spectral) / spectral
docs/openapi.json
[error] 1-1: CI check failed: docs/openapi.json is out of date versus the generated schema. Regenerate with: uv run scripts/generate_openapi_schema.py docs/openapi.json
🔇 Additional comments (6)
tests/unit/authentication/test_trusted_proxy.py (1)
1-123: LGTM!Also applies to: 129-479
src/authentication/__init__.py (1)
13-13: LGTM!Also applies to: 22-23, 86-90
src/authorization/middleware.py (1)
82-91: LGTM!tests/unit/authentication/test_auth.py (1)
3-9: LGTM!Also applies to: 11-17, 44-52
tests/unit/authorization/test_middleware.py (1)
188-208: LGTM!tests/unit/models/config/test_dump_configuration.py (1)
157-157: LGTM!Also applies to: 367-367, 721-721, 974-974, 1207-1207, 1430-1430, 1803-1803, 2022-2022, 2241-2241, 2467-2467
7f2a7df to
2dbaf0d
Compare
Add a new auth module that validates requests forwarded by a trusted Kubernetes proxy. The proxy authenticates itself via a ServiceAccount token (validated through TokenReview), then the end user's identity is extracted from a configurable HTTP header (default: X-Forwarded-User). Key behaviors: - Optional SA allowlist restricts which proxies may forward requests - Health probe and metrics endpoints can skip auth when configured - Authorization resolvers use the same pattern as rh-identity (NoopRoles + GenericAccess when access rules are defined) Signed-off-by: Willian Rampazzo <willianr@redhat.com>
2dbaf0d to
091a76d
Compare
Implements the trusted-proxy authentication module for lightspeed-stack, enabling the Konflux UI proxy to authenticate requests on behalf of end users without forwarding their JWT tokens between services.
Description
In the Konflux architecture, the UI proxy sits between the user's browser and lightspeed-stack. The original approach would have required passing the user's JWT token from the proxy to lightspeed-stack, which is undesirable as it exposes user credentials to internal services and couples lightspeed-stack to the UI's identity provider. This was raised during the architecture design review of konflux-lightspeed (section 4.2 - Authentication Flow).
How it works
The trusted-proxy module separates proxy authentication from user identification:
Configuration
Type of change
Tools used to create PR
Identify any AI code assistants used in this PR (for transparency and review context)
Related Tickets & Documents
Checklist before requesting a review
Testing
Create a test-proxy ServiceAccount and grant tokenreview-creator ClusterRole to the lightspeed-stack SA.
Create a test pod using the test-proxy SA and run the following tests against the lightspeed-stack pod:
Health probe without auth:
curl http://<pod-ip>:8080/readiness= 200Liveness probe without auth:
curl http://<pod-ip>:8080/liveness= 200Valid proxy token + user header on readiness:
curl -H "Authorization: Bearer $TOKEN" -H "X-Forwarded-User: user@test" http://<pod-ip>:8080/readiness= 200Valid proxy token + user header on query:
curl -X POST -H "Authorization: Bearer $TOKEN" -H "X-Forwarded-User: user@test" -H "Content-Type: application/json" -d '{"query":"hello"}' http://<pod-ip>:8080/v1/query= 200No auth header on query:
curl -X POST -H "Content-Type: application/json" -d '{"query":"hello"}' http://<pod-ip>:8080/v1/query= 401Valid token, missing user header:
curl -X POST -H "Authorization: Bearer $TOKEN" -H "Content-Type: application/json" -d '{"query":"hello"}' http://<pod-ip>:8080/v1/query= 401Invalid token:
curl -X POST -H "Authorization: Bearer faketoken" -H "X-Forwarded-User: user@test" -H "Content-Type: application/json" -d '{"query":"hello"}' http://<pod-ip>:8080/v1/query= 401SA not in allowlist: Use a token from a different SA (e.g., lightspeed-stack) = 403
How were the fix/results from this change verified? Please provide relevant screenshots or results.
Summary by CodeRabbit