LCORE-261: Add RH identity header auth#814
Conversation
WalkthroughAdds Red Hat Identity authentication: a FastAPI dependency that decodes and validates x-rh-identity headers (User/System types) with optional entitlement checks, plus configuration models, example configs, unit and integration tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant App as FastAPI App
participant Dep as RHIdentityAuthDependency
participant Data as RHIdentityData
Client->>App: Request with x-rh-identity header
App->>Dep: __call__(request)
alt Header missing
Dep-->>App: HTTPException(401)
App-->>Client: 401 Unauthorized
else
Dep->>Dep: Base64 decode header
alt Invalid base64
Dep-->>App: HTTPException(400)
App-->>Client: 400 Bad Request
else
Dep->>Dep: Parse JSON
alt Invalid JSON
Dep-->>App: HTTPException(400)
App-->>Client: 400 Bad Request
else
Dep->>Data: RHIdentityData(decoded_json, required_entitlements)
Data->>Data: _validate_structure()
alt Structure invalid
Data-->>Dep: HTTPException(400)
Dep-->>App: 400 Bad Request
else
opt required_entitlements present
Data->>Data: validate_entitlements()
alt Missing entitlements
Data-->>Dep: HTTPException(403)
Dep-->>App: 403 Forbidden
end
end
Dep->>Data: get_user_id(), get_username()
Dep-->>App: (user_id, username, skip_userid_check, NO_USER_TOKEN)
App-->>Client: proceed to handler (200/404)
end
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
|
Hi @major. Thanks for your PR. I'm waiting for a github.com member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/ok-to-test |
tisnik
left a comment
tisnik
left a comment
There was a problem hiding this comment.
LGTM in overall, nice work
There are some tests issues, please look at CI tasks logs etc.
|
@major: Cannot trigger testing until a trusted user reviews the PR and leaves an DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/ok-to-test |
Add support for receiving a base64-encoded JSON blob passed in by a load balancer in Red Hat Insights deployments. Handle authentication for users and systems with the ability to check that certain entitlements are in place for various products. Signed-off-by: Major Hayden <major@redhat.com>
major
force-pushed
the
rh-identity-auth
branch
from
2aebf9e to
4818576
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
tests/unit/models/config/test_dump_configuration.py (1)
145-152: Snapshot updates forrh_identity_configlook correct; consider future-proofing expectationsIncluding
"rh_identity_config": Nonein both expectedauthenticationsnapshots matches the newAuthenticationConfigurationdefault and guards that the field is present in dumps even when using the"noop"module. The trade-off is that any future field change inAuthenticationConfigurationwill require touching these large literals; if that becomes painful, you could factor a small helper/fixture that builds the expectedauthenticationdict (and reuse it across tests) to centralize shape changes.Also applies to: 444-451
src/models/config.py (1)
412-417: RH Identity config model is sound; consider tighteningrequired_entitlementsvalidationThe new
RHIdentityConfigurationplusrh_identity_configfield, the extra check incheck_authentication_model, and therh_identity_configurationaccessor follow the same pattern as the existing JWK support and give you a clear, validated path to the RH Identity settings. That’s a solid design.One refinement you might want to add: validate that
required_entitlements, when present, is a non-empty list of non-empty strings. Right now an explicit empty list or a list with empty/whitespace values would be accepted and likely behave the same as omitting entitlements, which can be a surprising misconfiguration for operators.For example:
class RHIdentityConfiguration(ConfigurationBase): """Red Hat Identity authentication configuration.""" - required_entitlements: Optional[list[str]] = None + required_entitlements: Optional[list[str]] = None + + @model_validator(mode="after") + def check_required_entitlements(self) -> Self: + """Validate RH Identity entitlement configuration.""" + if self.required_entitlements is not None: + if not self.required_entitlements: + raise ValueError( + "required_entitlements must be omitted or a non-empty list" + ) + if not all(isinstance(e, str) and e.strip() for e in self.required_entitlements): + raise ValueError( + "Each entitlement must be a non-empty string" + ) + return selfThis keeps the current semantics (omit the field → no entitlement check; non-empty list → enforce ALL) while failing fast on ambiguous configs.
Also applies to: 426-427, 444-450, 464-473
src/authentication/rh_identity.py (1)
177-191:virtual_pathis currently unused; either wire it or drop it
RHIdentityAuthDependency.__init__accepts and storesvirtual_path, but nothing in this class uses it. That’s a potential source of confusion for callers and reviewers (it’s also not exercised in tests).If there is no near-term plan to use it, consider removing the parameter/field for now; otherwise, add a short docstring note or TODO describing the intended future use (e.g. for downstream authz) so it’s clear why it exists.
tests/unit/authentication/test_rh_identity.py (1)
225-347: Auth dependency tests are realistic and well-structuredThe
TestRHIdentityAuthDependencytests nicely cover:
- Happy paths for User and System identities.
- Missing header, invalid base64, and invalid JSON.
- Entitlement gating (both allowed and denied).
- Behavior when
entitlementsis empty.Using
Mock(spec=Request)+ a simpleheadersdict keeps the tests fast while still emulating FastAPI’s access pattern.Once the small robustness tweaks in the implementation are made, these tests should still pass unchanged.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
examples/lightspeed-stack-rh-identity.yaml(1 hunks)src/authentication/__init__.py(2 hunks)src/authentication/rh_identity.py(1 hunks)src/constants.py(1 hunks)src/models/config.py(4 hunks)tests/configuration/rh-identity-config.yaml(1 hunks)tests/integration/test_rh_identity_integration.py(1 hunks)tests/unit/authentication/test_rh_identity.py(1 hunks)tests/unit/models/config/test_dump_configuration.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/configuration/rh-identity-config.yaml
- tests/integration/test_rh_identity_integration.py
🧰 Additional context used
🧬 Code graph analysis (3)
src/authentication/rh_identity.py (1)
src/authentication/interface.py (1)
AuthInterface(34-39)
tests/unit/authentication/test_rh_identity.py (1)
src/authentication/rh_identity.py (7)
RHIdentityAuthDependency(170-250)RHIdentityData(20-167)get_user_id(104-114)get_username(116-126)has_entitlement(128-139)has_entitlements(141-150)validate_entitlements(152-167)
src/authentication/__init__.py (3)
src/configuration.py (2)
configuration(73-77)authentication_configuration(108-113)src/models/config.py (1)
rh_identity_configuration(465-473)src/authentication/rh_identity.py (1)
RHIdentityAuthDependency(170-250)
⏰ 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). (2)
- GitHub Check: e2e_tests (azure)
- GitHub Check: e2e_tests (ci)
🔇 Additional comments (5)
examples/lightspeed-stack-rh-identity.yaml (1)
1-23: Example config correctly reflects RH Identity wiringThe example uses
module: "rh-identity"andrh_identity_config.required_entitlementsin a way that matches the newAuthenticationConfiguration/RHIdentityConfigurationmodel and the"rh-identity"constant, and the three options are documented clearly for operators.src/constants.py (1)
93-113: RH Identity auth module constant is wired correctlyDefining
AUTH_MOD_RH_IDENTITY = "rh-identity"and adding it toSUPPORTED_AUTHENTICATION_MODULEScleanly integrates the new module into existing auth validation and matches the example configuration’smodule: "rh-identity"value.src/authentication/__init__.py (1)
7-8: RH Identity auth dependency is correctly integratedImporting
rh_identityand adding acase constants.AUTH_MOD_RH_IDENTITYbranch that pullsconfiguration.authentication_configuration.rh_identity_configurationand passesrequired_entitlementsintoRHIdentityAuthDependencymatches the new config model and keeps module-specific validation inAuthenticationConfiguration. This should behave consistently with the existing JWK and K8s branches.Also applies to: 49-56
src/authentication/rh_identity.py (1)
192-250: Main auth flow looks solid and aligns with testsThe
__call__implementation:
- Cleanly separates header extraction, base64 decoding, JSON parsing, and structural validation.
- Uses appropriate status codes (401 for missing header, 400 for format issues, 403 for entitlement failures).
- Delegates ID/username extraction and entitlement logic to
RHIdentityData, keeping this dependency focused.Given the above hardening in
_validate_structure/entitlement handling, this flow looks ready to integrate with FastAPI endpoints.tests/unit/authentication/test_rh_identity.py (1)
17-213: Good coverage of identity parsing and validation edge casesThe
TestRHIdentityDatasuite thoroughly exercises:
- User vs System extraction (
get_user_id/get_username).- Entitlement presence (single and multiple services).
- Entitlement validation behavior for various required sets.
- Structural validation for a wide range of missing fields and an unsupported
type.This gives strong confidence that changes in
RHIdentityDatawill be caught by tests. No issues from a correctness standpoint.
| def _validate_structure(self) -> None: | ||
| """Validate the identity data structure. | ||
|
|
||
| Raises: | ||
| HTTPException: 400 if required fields are missing | ||
| """ | ||
| if ( |
There was a problem hiding this comment.
Harden structure/entitlement shape validation to avoid 500s on malformed payloads
_validate_structure and the entitlement helpers assume certain subfields are dicts. For some malformed but valid JSON payloads, this can raise TypeError instead of the intended 4xx:
identitypresent but not a mapping (e.g.{"identity": 1}or{"identity": true}) →"type" not in identitymay raise at runtime.- Similarly, if
identity["user"]oridentity["system"]is present but not a dict,"user_id" not in user/"cn" not in systemcan raise. - If
entitlementsis present but not a dict,entitlements.get(...)inhas_entitlementwill raise.
That turns bad input into a 500 rather than a clean 400/403, and it bypasses your otherwise precise error messages.
A minimal hardening that preserves current behavior would be:
def _validate_structure(self) -> None:
@@
- identity = self.identity_data["identity"]
+ identity = self.identity_data["identity"]
+ if not isinstance(identity, dict):
+ raise HTTPException(
+ status_code=400,
+ detail="Invalid 'identity' field format",
+ )
@@
- if identity_type == "User":
- if "user" not in identity:
+ if identity_type == "User":
+ user = identity.get("user")
+ if not isinstance(user, dict):
raise HTTPException(
- status_code=400, detail="Missing 'user' field for User type"
+ status_code=400,
+ detail="Missing 'user' field for User type",
)
- user = identity["user"]
@@
- elif identity_type == "System":
- if "system" not in identity:
+ elif identity_type == "System":
+ system = identity.get("system")
+ if not isinstance(system, dict):
raise HTTPException(
- status_code=400, detail="Missing 'system' field for System type"
+ status_code=400,
+ detail="Missing 'system' field for System type",
)
- system = identity["system"]
@@
- entitlements = self.identity_data.get("entitlements", {})
- service_entitlement = entitlements.get(service, {})
+ entitlements = self.identity_data.get("entitlements") or {}
+ if not isinstance(entitlements, dict):
+ return False
+ service_entitlement = entitlements.get(service, {})This keeps your current contract (still 400/403 for bad input) but avoids accidental 500s on weird-but-possible payloads.
Also applies to: 58-95, 128-167
|
/ok-to-test |
2 participants
Description
Add support for receiving a base64-encoded JSON blob passed in by a load balancer in Red Hat Insights deployments. Handle authentication for users and systems with the ability to check that certain entitlements are in place for various products.
Type of change
Related Tickets & Documents
Checklist before requesting a review
Testing
Testing this in production requires putting the deployment behind a load balancer in the Red Hat Insights environment, but you can also test it by providing a base64-encoded JSON string in the
x-rh-identityheader.Summary by CodeRabbit
New Features
Examples
Tests