feat: integrating OIDC Authentication with maas API AuthPolicy#598
feat: integrating OIDC Authentication with maas API AuthPolicy#598dmytro-zaharnytskyi wants to merge 2 commits intomainfrom
Conversation
Copy of this PR without keycloak - #578 JIRA - https://redhat.atlassian.net/browse/RHOAIENG-47977 --------- Signed-off-by: Dmytro Zaharnytskyi <zdmytro@redhat.com>
Add Keycloak deployment scripts and configuration examples for external OIDC support ## Description This PR adds Keycloak deployment automation and configuration examples to support external OIDC authentication for MaaS ([RHAISTRAT-1120](https://redhat.atlassian.net/browse/RHAISTRAT-1120)). ### Changes Included **Deployment Scripts:** - `scripts/setup-keycloak.sh` - Automated Keycloak deployment - Auto-installs Keycloak operator from community-operators catalog - Creates `keycloak-system` namespace - Auto-detects cluster domain for hostname configuration - Deploys POC-grade Keycloak instance (1 replica, HTTP enabled) - Creates HTTPRoute for external access via existing MaaS Gateway API - Displays commands to securely retrieve admin credentials (no passwords in output) - Idempotent - safe to run multiple times - `scripts/cleanup-keycloak.sh` - Proper cleanup script - Deletes Keycloak instance (allows operator to clean up properly) - Deletes `keycloak-system` namespace - Optional CRD deletion with `--delete-crds` flag - Confirmation prompts (skip with `--force`) **Configuration Examples:** - `examples/keycloak/README.md` - Comprehensive configuration guide - Admin console access instructions - Step-by-step realm creation via web UI - OIDC client configuration for MaaS (client ID: `maas`) - Group mapper configuration for JWT tokens - Token verification examples - Troubleshooting guide - `examples/keycloak/test-realms/` - Multi-tenant test realms for development - `tenant-a-realm.json` - Test realm with Engineering, Site-Reliability, Project-Alpha groups - `tenant-b-realm.json` - Test realm with Product-Security, Project-Omega groups - `apply-test-realms.sh` - One-command deployment of test realms - Clear warnings about test-only use (hardcoded passwords, wildcard redirects) ### Design Decisions 1. **Opt-in deployment** - Keycloak is not required for basic MaaS, suitable for `--enable-keycloak` flag in future deploy.sh integration 2. **No realms by default** - Admins configure realms post-deployment via Admin Console (easiest method) 3. **Separate namespace** - Uses `keycloak-system` to isolate from MaaS components 4. **POC-grade configuration** - Suitable for development/testing; production deployments should use external database and TLS 5. **Test realms as examples** - Optional multi-tenant demo configuration for quick testing ## How Has This Been Tested? ### Manual Testing 1. **Deployed Keycloak** ```bash ./scripts/setup-keycloak.sh - Verified operator installation and CRD creation - Verified Keycloak pod startup and readiness - Verified HTTPRoute creation and hostname auto-detection - Retrieved admin credentials from secret 2. Applied test realms ./examples/keycloak/test-realms/apply-test-realms.sh - Verified ConfigMap creation - Verified Keycloak restart and realm import - Confirmed realms visible in Admin Console 3. Tested OIDC token generation 3. Following the instructions in examples/keycloak/test-realms/README.md: CLUSTER_DOMAIN=$(kubectl get ingresses.config.openshift.io cluster -o jsonpath='{.spec.domain}') KEYCLOAK_HOST="keycloak.${CLUSTER_DOMAIN}" curl -k -X POST \ "https://${KEYCLOAK_HOST}/realms/tenant-a/protocol/openid-connect/token" \ -d "grant_type=password" \ -d "client_id=test-client" \ -d "username=alice_lead" \ -d "password=letmein" | jq . 3. Verified groups claim in decoded token: $ echo $TOKEN | cut -d'.' -f2 | base64 -d 2>/dev/null | jq . { "exp": 1774014245, "iat": 1774013945, "jti": "onrtro:c72d50c7-9ccd-e06b-9bd1-cb1e5d51efb8", "iss": "https://keycloak.apps.rosa.j8bqr-55hr9-zr9.dv8i.p3.openshiftapps.com/realms/tenant-a", "sub": "b9ac9148-252a-4919-8830-b3d9f6af3d20", "typ": "Bearer", "azp": "test-client", "sid": "MztLpDc3-0cNcaCNekpuH10q", "acr": "1", "allowed-origins": [ "*" ], "scope": "profile email", "email_verified": true, "groups": [ "Engineering", "Project-Alpha" ], "preferred_username": "alice_lead" } 3. ✅ Groups claim correctly populated - Token contains ["Engineering", "Project-Alpha"] for user alice_lead 4. Tested cleanup ./scripts/cleanup-keycloak.sh - Verified Keycloak instance deletion - Verified namespace deletion - Verified proper cleanup order (no orphaned resources) Testing Environment - Platform: OpenShift (ROSA) - Cluster version: 4.x - Gateway: Existing MaaS Gateway API setup - Keycloak Operator: community-operators (fast channel) Merge criteria: - [ ] The commits are squashed in a cohesive manner and have meaningful messages. - [x] Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious). - [x] The developer has manually tested the changes and verified that the changes work <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added Keycloak deployment for external OIDC authentication in MaaS. * Added `--enable-keycloak` flag to enable Keycloak during deployment. * Included pre-configured test realms with sample users and groups for development. * **Documentation** * Added comprehensive guides for Keycloak configuration, setup, and verification. * Included troubleshooting steps and token validation instructions. * **Chores** * Added cleanup utilities for removing Keycloak deployments. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dmytro-zaharnytskyi The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
📝 WalkthroughWalkthroughThis PR integrates external OIDC identity provider support into the MaaS platform. It introduces Keycloak deployment automation via new setup/cleanup scripts, extends the deployment workflow to patch AuthPolicy resources with OIDC configuration, adds Kuadrant AuthPolicy specifications for JWT token validation, and provides comprehensive documentation and E2E test coverage for OIDC-to-API-key token minting flows. Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Security-first findingsCWE-295: Insufficient Certificate Validation – OIDC Issuer URLLocation: The OIDC issuer URL is injected into the AuthPolicy without validation. The deployment script reads
The Recommendation: Add CWE-798: Use of Hard-Coded CredentialsLocation: Test realm users are seeded with hardcoded password
Recommendation: Add a deployment-time assertion in CWE-200: Exposure of Sensitive Information – JWT Token HandlingLocation: The test module lacks credential redaction. If OIDC credentials ( Recommendation: Wrap any test output that references credentials with a sanitization function that redacts tokens and passwords before printing. Example: def _redact_token(token_str):
if len(token_str) > 20:
return token_str[:10] + "...[redacted]"
return "[redacted]"CWE-862: Missing Authorization Check – AuthPolicy Patch ApplicationLocation: The script patches the live
Recommendation: Add explicit RBAC checks before patching: kubectl auth can-i patch authpolicies.kuadrant.io -n "${MAAS_NAMESPACE}" || \
die "RBAC: cannot patch AuthPolicy in ${MAAS_NAMESPACE}"Make patch failure fatal by using CWE-434: Unrestricted Upload of File with Dangerous Type – Keycloak Realm ImportLocation: Both scripts deploy ConfigMaps containing Keycloak realm JSON. The realm files are read from the repository directly without validation:
Recommendation: Add JSON validation using jq empty "${realm_file}" || die "Invalid JSON in ${realm_file}"Validate critical fields exist: CWE-377: Insecure Temporary FileLocation: Scripts generate temporary files (e.g., patched YAML) without explicit temporary directories or cleanup guarantees. No evidence of Recommendation: Add trap handlers: temp_dir=$(mktemp -d) || die "Cannot create temp dir"
trap "rm -rf ${temp_dir}" EXITCWE-400: Uncontrolled Resource Consumption – Keycloak PollingLocation: The script polls for Keycloak pod readiness with a 60-second timeout and 1-second sleep intervals (60 iterations). If the cluster is under load or autoscaling, the timeout silently completes with a warning but continues deployment. No exponential backoff; fixed 1-second polling could hammer the API server on large clusters. Recommendation: Implement exponential backoff and increase timeout. Use kubectl wait --for=condition=Ready pod \
-l app.kubernetes.io/name=keycloak \
-n keycloak-system \
--timeout=5m 2>/dev/null || warn "Keycloak pods not ready; continuing anyway"CWE-347: Improper Verification of Cryptographic Signature – OIDC Token ValidationLocation: The AuthPolicy references
If the OIDC provider is compromised or returns misconfigured JWKS, Kuadrant could accept invalid tokens. The policy does not require standard claims beyond what's implicitly validated. Recommendation: Explicitly document in
CWE-269: Improper Access Control – Namespace Deletion Without VerificationLocation: The cleanup script deletes the
Recommendation: Before namespace deletion, list all resources: echo "Resources to be deleted:"
kubectl get all,configmap,secret -n keycloak-system
read -p "Confirm deletion? [y/N]: " confirmAdditional actionable issuesMissing error context in E2E testsLocation: Test assertions lack descriptive error messages. Example: assert response.status_code == 200Should be: assert response.status_code == 200, f"OIDC token endpoint returned {response.status_code}: {response.text}"Inconsistent environment variable namingLocation: OIDC configuration uses three different name formats:
This inconsistency could cause environment lookups to silently fall back to defaults. Standardize on Missing cleanup for failed deploymentsLocation: If the script exits early (e.g., operator not ready), the partially-created Recommendation: Add: trap 'warn "Setup failed. Run ./scripts/cleanup-keycloak.sh to remove partial deployment."' EXIT🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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.
Actionable comments posted: 14
🧹 Nitpick comments (2)
scripts/cleanup-keycloak.sh (2)
95-98: Quote$REPLYto prevent word splitting (CWE-78).- if [[ ! $REPLY =~ ^[Yy]$ ]]; then + if [[ ! "$REPLY" =~ ^[Yy]$ ]]; thenAs per coding guidelines: "Quote all variables to prevent injection".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/cleanup-keycloak.sh` around lines 95 - 98, The conditional uses an unquoted shell variable REPLY which can cause word-splitting or injection; update the test in the interactive prompt handling so the REPLY variable is quoted inside the [[ ... ]] pattern match (i.e., reference "$REPLY" in the if condition that currently reads if [[ ! $REPLY =~ ^[Yy]$ ]]; then) so the script safely evaluates the user input.
112-119: Keycloak CR deletion lacks timeout handling.If
kubectl delete keycloakhangs (e.g., webhook issues), the script blocks indefinitely. The subsequentsleep 5is also arbitrary and may be insufficient for operator reconciliation.Proposed fix
if kubectl get keycloak "$KEYCLOAK_NAME" -n "$NAMESPACE" &>/dev/null; then - kubectl delete keycloak "$KEYCLOAK_NAME" -n "$NAMESPACE" + kubectl delete keycloak "$KEYCLOAK_NAME" -n "$NAMESPACE" --timeout=60s echo " Waiting for operator to clean up resources..." - sleep 5 + kubectl wait --for=delete keycloak/"$KEYCLOAK_NAME" -n "$NAMESPACE" --timeout=30s 2>/dev/null || sleep 5 echo "✓ Keycloak instance deleted"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/cleanup-keycloak.sh` around lines 112 - 119, The Keycloak deletion block can hang and currently uses an arbitrary sleep; replace the simple delete+sleep pattern with a deletion that enforces a maximum wait: call kubectl delete keycloak "$KEYCLOAK_NAME" -n "$NAMESPACE" and then use a bounded wait (e.g. kubectl wait --for=delete keycloak/"$KEYCLOAK_NAME" -n "$NAMESPACE" --timeout=<reasonable-duration>) or implement a loop that polls kubectl get keycloak "$KEYCLOAK_NAME" -n "$NAMESPACE" with a counter and sleep interval, aborting after a max retry count and emitting an error/exit code if the CR is not removed; remove the arbitrary sleep 5 and ensure you surface timeout/failure using the script exit status.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/hack/cleanup-odh.sh:
- Around line 129-136: The assignment to SCRIPT_DIR can result in an empty
string if the subshell cd operations fail, which makes the subsequent [[ -f
"${SCRIPT_DIR}/scripts/cleanup-keycloak.sh" ]] test unreliable; update the
SCRIPT_DIR computation and guard to ensure it succeeded: compute SCRIPT_DIR in a
way that fails fast (e.g., use a subshell that exits non-zero on cd failure or
test each cd), then check that SCRIPT_DIR is non-empty and points to an existing
directory before running the -f test and invoking scripts/cleanup-keycloak.sh;
reference the SCRIPT_DIR variable, the conditional test [[ -f
"${SCRIPT_DIR}/scripts/cleanup-keycloak.sh" ]], and the INCLUDE_CRDS branch when
adding the validation and early exit or error handling.
In `@deployment/overlays/odh/maas-api-auth-policy-oidc-patch.yaml`:
- Around line 10-15: The oidc-identities block currently only sets jwt.issuerUrl
(issuerUrl) so add an authorization rule under spec.rules.authorization that
enforces audience/authorized-party checks: create a rule (e.g.,
jwt-audience-validation) using patternMatching with selector auth.identity.aud
operator eq value "maas-api" and also add an alternative check for
auth.identity.azp when present; apply the same change pattern to the
corresponding scripts/data/maas-api-authpolicy-external-oidc-patch.yaml to keep
external patch in sync.
In `@docs/samples/install/keycloak/README.md`:
- Around line 164-179: Remove the insecure curl option by deleting "-k" from the
token request (the curl command that posts to
"https://${KEYCLOAK_HOST}/realms/${REALM_NAME}/protocol/openid-connect/token")
and rely on system CA validation or supply a CA bundle (e.g., --cacert) instead;
for JWT decoding, change the payload decode pipeline that uses TOKEN and "echo
$TOKEN | cut -d'.' -f2 | base64 -d" to perform base64url decoding with URL-safe
character translation and proper padding (replace '-'/'_' to '+'/'/' and add '='
padding to a length multiple of 4) before calling base64 --decode (or an
equivalent base64url decode) so the JWT payload is decoded correctly.
- Around line 74-75: Replace the wildcard redirect URI and permissive Web
origins in the README instructions: change the "Valid redirect URIs:
`https://*.{your-cluster-domain}/*`" guidance to require explicit, exact
callback URLs (e.g., `https://app.example.com/oauth/callback`) and update "Web
origins: `+`" to list explicit origins (e.g., `https://app.example.com`) only;
ensure the text references that Keycloak/OpenID Connect requires exact string
matching of redirect_uri and that CORS must be restricted to the named origins
rather than using `+` or wildcards.
In `@docs/samples/install/keycloak/test-realms/apply-test-realms.sh`:
- Around line 68-109: The current kubectl patch uses JSON merge semantics which
will overwrite arrays under spec.unsupported.podTemplate (containers, volumes);
instead either refuse to run if spec.unsupported.podTemplate is already
populated or perform a safe merge: fetch the existing Keycloak CR (use
EXISTING_ARGS/kubectl get keycloak "$KEYCLOAK_NAME" -n "$NAMESPACE" -o json),
inspect spec.unsupported.podTemplate, and if it exists exit with a clear
message; otherwise build a merged object that appends/merges the new container
entry (name "keycloak", args including "--import-realm" but avoid hardcoding the
full args list) and the test-realms volume into the existing arrays, then apply
the merged CR via kubectl apply -f - (or use kubectl patch --type=json with
explicit add operations targeting specific array positions) to avoid clobbering
containers or volumes; ensure references to CONFIGMAP_NAME, KEYCLOAK_NAME,
spec.unsupported.podTemplate, containers, and volumes are used to locate and
merge the correct fields.
In `@docs/samples/install/keycloak/test-realms/README.md`:
- Around line 173-189: Update the README to prevent leaving test realms and
credentials active: either (A) add explicit commands to delete the persisted
realms (e.g., show how to run kcadm.sh inside the Keycloak pod or call the
Keycloak REST API to delete realms "tenant-a" and "tenant-b" and remove users
"alice_lead", "bob_sre", "charlie_sec_lead", "grace_dev"), or (B) state that
./scripts/cleanup-keycloak.sh is the only supported cleanup and remove the
partial ConfigMap+restart instructions; if keeping the partial approach, add a
clear warning that deleting the ConfigMap (keycloak-test-realms) and restarting
the StatefulSet (maas-keycloak) does NOT remove persisted realms and leaves the
hardcoded password "letmein" active. Ensure the README references the ConfigMap
name (keycloak-test-realms), the StatefulSet name (maas-keycloak), the realm
names (tenant-a, tenant-b), user names, and the cleanup script name so
maintainers know which resources to target.
In `@scripts/cleanup-keycloak.sh`:
- Around line 66-70: The current conditional ([[ "$DELETE_CRDS" == true ]] || [[
"$FORCE" == true ]]) lets --force trigger CRD deletion; change it so CRDs are
deleted only when DELETE_CRDS is true (remove FORCE from this OR check).
Specifically, update the if condition to check only DELETE_CRDS (e.g., if [[
"$DELETE_CRDS" == true ]]; then ...) and keep FORCE used only to skip any
interactive confirmation step prior to deletion (use FORCE to bypass
read/confirm logic, not to enable deletion itself). Ensure references to
DELETE_CRDS and FORCE remain intact so the delete block only runs when
DELETE_CRDS is true and FORCE merely suppresses prompts.
In `@scripts/data/maas-api-authpolicy-external-oidc-patch.yaml`:
- Around line 15-20: The patch is adding plain.expression to X-MaaS-Username-OC
while an earlier patch leaves plain.selector present, causing an
invalid/ambiguous header definition; update the X-MaaS-Username-OC entry so that
the plain block does not retain selector when you add expression — specifically
remove or unset plain.selector and ensure plain contains only expression
(has(auth.identity.preferred_username) ? ...) so the merge from the earlier
patch doesn't leave both plain.selector and plain.expression present; target the
X-MaaS-Username-OC plain block in this YAML to replace/remove the selector
before adding expression.
In `@scripts/deploy.sh`:
- Around line 1486-1489: The script currently treats failures from
patch_authpolicy_from_template as non-fatal (calls log_warn then returns 0),
which lets deployment succeed despite AuthPolicy not being applied; change both
occurrences (the block using patch_authpolicy_from_template "$authpolicy_name"
"$api_keys_patch" "$NAMESPACE" and the similar block at the other location) to
treat failure as fatal by returning a non-zero status or exiting non-zero (e.g.,
replace the current log_warn + return 0 with log_error + return 1 or an exit 1)
so the deployment aborts on AuthPolicy patch failure.
- Around line 1480-1483: This patch is mutating the generated
kuadrant.io/AuthPolicy (using authpolicy_name and
opendatahub.io/managed="false") which will be reverted by the maas controller;
instead update the owning MaaS resource/template that produces the AuthPolicy
(see maas-controller/pkg/controller/maas/maasauthpolicy_controller.go) so
ownership and annotations persist. Replace the kubectl annotate calls that touch
authpolicy_name (and the audience patch sections) with logic that patches the
MaaS CR or its template (the resource that creates maas-api-auth-policy) to
include opendatahub.io/managed="false" and any audience modifications, ensuring
the controller will reconcile the generated AuthPolicy with those desired values
rather than overwriting them.
In `@scripts/setup-keycloak.sh`:
- Around line 26-30: The usage banner currently points to the hardcoded secret
name "keycloak-initial-admin" which is inconsistent with the rest of the script
and docs; change the banner to reference the dynamic secret name using the
KEYCLOAK_NAME variable (i.e., replace "keycloak-initial-admin" with
"${KEYCLOAK_NAME}-initial-admin" so it matches occurrences like
${KEYCLOAK_NAME}-initial-admin printed later and in
docs/samples/install/keycloak/README.md).
- Around line 91-95: Replace the unpinned Subscription channel value to pin a
tested Keycloak Operator release: set a specific startingCSV in the Subscription
(instead of relying solely on channel: fast) and change installPlanApproval to
Manual (or provide startingCSV with Automatic if you intentionally pin a
version), and make the operator source and channel configurable inputs (e.g.,
variables used when rendering the Subscription) with a known-good default;
update the subscription block that currently contains the fields channel, name,
source, sourceNamespace, installPlanApproval to accept configurable inputs and
include startingCSV to fix the non-reproducible upgrade behavior.
In `@test/e2e/tests/test_external_oidc.py`:
- Around line 65-68: The test test_oidc_token_can_create_api_key prints a
truncated API key prefix which leaks credential material; update the
print/logging in that test (the print in test_oidc_token_can_create_api_key that
uses data.get('key')) to stop printing any part of the key and only output the
API key id (data.get('id')) or remove the print entirely—locate the print
statement in test_oidc_token_can_create_api_key and replace it so only the id is
logged (or no secret-derived values are included).
- Around line 25-40: _request_oidc_token currently reuses global TLS_VERIFY
causing tests to disable TLS for external OIDC token exchange; introduce a
separate OIDC_TLS_VERIFY env flag (default true) and use it for the
requests.post verify parameter in _request_oidc_token instead of TLS_VERIFY;
parse OIDC_TLS_VERIFY into a boolean (e.g., via existing env helper or a new
_bool_env/_optional_env) and document default behavior so password-grant
requests to OIDC_TOKEN_URL always verify TLS unless explicitly overridden.
---
Nitpick comments:
In `@scripts/cleanup-keycloak.sh`:
- Around line 95-98: The conditional uses an unquoted shell variable REPLY which
can cause word-splitting or injection; update the test in the interactive prompt
handling so the REPLY variable is quoted inside the [[ ... ]] pattern match
(i.e., reference "$REPLY" in the if condition that currently reads if [[ !
$REPLY =~ ^[Yy]$ ]]; then) so the script safely evaluates the user input.
- Around line 112-119: The Keycloak deletion block can hang and currently uses
an arbitrary sleep; replace the simple delete+sleep pattern with a deletion that
enforces a maximum wait: call kubectl delete keycloak "$KEYCLOAK_NAME" -n
"$NAMESPACE" and then use a bounded wait (e.g. kubectl wait --for=delete
keycloak/"$KEYCLOAK_NAME" -n "$NAMESPACE" --timeout=<reasonable-duration>) or
implement a loop that polls kubectl get keycloak "$KEYCLOAK_NAME" -n
"$NAMESPACE" with a counter and sleep interval, aborting after a max retry count
and emitting an error/exit code if the CR is not removed; remove the arbitrary
sleep 5 and ensure you surface timeout/failure using the script exit status.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 2ef35d54-5825-421c-99e6-c5ef70a7b15f
📒 Files selected for processing (20)
.github/hack/cleanup-odh.shdeployment/overlays/odh/kustomization.yamldeployment/overlays/odh/maas-api-auth-policy-oidc-patch.yamldeployment/overlays/odh/params.envdocs/content/architecture.mddocs/content/configuration-and-management/token-management.mddocs/samples/install/keycloak/README.mddocs/samples/install/keycloak/test-realms/README.mddocs/samples/install/keycloak/test-realms/apply-test-realms.shdocs/samples/install/keycloak/test-realms/tenant-a-realm.jsondocs/samples/install/keycloak/test-realms/tenant-b-realm.jsonscripts/README.mdscripts/cleanup-keycloak.shscripts/data/maas-api-authpolicy-api-keys-patch.yamlscripts/data/maas-api-authpolicy-external-oidc-patch.yamlscripts/deploy.shscripts/setup-keycloak.shtest/e2e/README.mdtest/e2e/scripts/prow_run_smoke_test.shtest/e2e/tests/test_external_oidc.py
| SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && cd ../.. && pwd)" | ||
| if [[ -f "${SCRIPT_DIR}/scripts/cleanup-keycloak.sh" ]]; then | ||
| # Pass --delete-crds if --include-crds was specified for this script | ||
| if $INCLUDE_CRDS; then | ||
| "${SCRIPT_DIR}/scripts/cleanup-keycloak.sh" --force --delete-crds 2>/dev/null || true | ||
| else | ||
| "${SCRIPT_DIR}/scripts/cleanup-keycloak.sh" --force 2>/dev/null || true | ||
| fi |
There was a problem hiding this comment.
SCRIPT_DIR may be empty if directory traversal fails.
If cd "$(dirname "${BASH_SOURCE[0]}")" or cd ../.. fails, SCRIPT_DIR becomes empty and the -f test passes vacuously on an empty path. With set -e, the subshell exits but the assignment still succeeds with empty string.
Proposed fix
-SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && cd ../.. && pwd)"
+SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")/../.." && pwd)" || SCRIPT_DIR=""
if [[ -f "${SCRIPT_DIR}/scripts/cleanup-keycloak.sh" ]]; then
+if [[ -n "$SCRIPT_DIR" && -f "${SCRIPT_DIR}/scripts/cleanup-keycloak.sh" ]]; then🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/hack/cleanup-odh.sh around lines 129 - 136, The assignment to
SCRIPT_DIR can result in an empty string if the subshell cd operations fail,
which makes the subsequent [[ -f "${SCRIPT_DIR}/scripts/cleanup-keycloak.sh" ]]
test unreliable; update the SCRIPT_DIR computation and guard to ensure it
succeeded: compute SCRIPT_DIR in a way that fails fast (e.g., use a subshell
that exits non-zero on cd failure or test each cd), then check that SCRIPT_DIR
is non-empty and points to an existing directory before running the -f test and
invoking scripts/cleanup-keycloak.sh; reference the SCRIPT_DIR variable, the
conditional test [[ -f "${SCRIPT_DIR}/scripts/cleanup-keycloak.sh" ]], and the
INCLUDE_CRDS branch when adding the validation and early exit or error handling.
| oidc-identities: | ||
| when: | ||
| - predicate: '!request.headers.authorization.startsWith("Bearer sk-oai-") && request.headers.authorization.matches("^Bearer [^.]+\\.[^.]+\\.[^.]+$")' | ||
| jwt: | ||
| issuerUrl: https://oidc.example.invalid/realms/maas | ||
| priority: 1 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Locate and examine the main YAML file
find . -name "maas-api-auth-policy-oidc-patch.yaml" -type fRepository: opendatahub-io/models-as-a-service
Length of output: 140
🏁 Script executed:
# Find the mirror file mentioned in the review
find . -name "*authpolicy*external*oidc*" -type fRepository: opendatahub-io/models-as-a-service
Length of output: 137
🏁 Script executed:
# Get a broader view of the auth policy files
find . -path "*/maas-api*" -name "*.yaml" | head -20Repository: opendatahub-io/models-as-a-service
Length of output: 1181
🏁 Script executed:
cat -n ./deployment/overlays/odh/maas-api-auth-policy-oidc-patch.yamlRepository: opendatahub-io/models-as-a-service
Length of output: 1788
🏁 Script executed:
cat -n ./scripts/data/maas-api-authpolicy-external-oidc-patch.yamlRepository: opendatahub-io/models-as-a-service
Length of output: 1320
🏁 Script executed:
cat -n ./deployment/base/maas-api/policies/auth-policy.yamlRepository: opendatahub-io/models-as-a-service
Length of output: 3869
🌐 Web query:
Authorino JWT validation audience claim supported fields kuadrant
💡 Result:
Authorino, part of the Kuadrant project, supports JWT validation primarily through OpenID Connect (OIDC) Discovery. For OIDC JWTs, Authorino verifies the JSON Web Signature (JWS), checks time validity (exp, nbf, iat claims), ensures the 'iss' claim matches the configured issuerUrl, and verifies the 'kid' header matches a cached JWK from the JWKS. There is no explicit configuration field in the jwt spec (issuerUrl, ttl, jwksUrl, etc.) for specifying expected audience values ('aud' claim). The standard JWT 'aud' claim validation is not directly supported or configurable in Authorino's core JWT authentication. Instead, users can access the validated JWT payload (including 'aud') in the authorization JSON via selectors like auth.identity.aud and enforce audience checks using authorization policies, such as pattern-matching rules or OPA Rego policies. For comparison, Authorino does explicitly support audience validation in kubernetesTokenReview via an 'audiences' array field, where token audiences must include specified values or the requested host/port. To enforce JWT audience: - Use patternMatching authorization: selector: auth.identity.aud, operator: in, value: ["expected-aud"]. - Or OPA policy checking input.auth.identity.aud. This is confirmed across official Kuadrant docs, GitHub features.md, CRD schema snippets, and user guides showing JWT configs without 'aud' fields but payload access for policies.
Citations:
- 1: https://docs.kuadrant.io/0.11.0/authorino/docs/features/
- 2: https://docs.kuadrant.io/1.0.x/authorino/docs/user-guides/oidc-jwt-authentication/
- 3: https://github.com/Kuadrant/authorino/blob/main/docs/features.md
- 4: https://github.com/Kuadrant/authorino/blob/main/docs/user-guides/oidc-jwt-authentication.md
- 5: https://github.com/Kuadrant/authorino/blob/main/docs/user-guides/envoy-jwt-authn-and-authorino.md
- 6: https://pkg.go.dev/github.com/kuadrant/authorino
- 7: https://www.none
- 8: https://raw.githubusercontent.com/Kuadrant/authorino-operator/main/config/deploy/manifests.yaml
🌐 Web query:
Kuadrant AuthPolicy JWT validation client binding issuerUrl only security
💡 Result:
In Kuadrant AuthPolicy, JWT validation is configured under spec.rules.authentication..jwt, with issuerUrl specifying the OpenID Connect issuer endpoint (e.g., http://keycloak.keycloak.svc.cluster.local:8080/realms/kuadrant). Authorino (underlying Kuadrant auth) uses OpenID Connect Discovery to fetch JWKS and configuration from the issuerUrl, verifies the JWT signature (JWS), kid claim match, and time validity. The decoded JWT payload becomes auth.identity for further use in authorization or rate limiting. No explicit "client binding" field like azp (authorized party, client_id) or audience validation is shown in examples or references; validation relies on standard OIDC checks from discovery (likely including aud if present in token and config). For security-focused client binding, use authorization rules (e.g., patternMatching on auth.identity.azp or auth.identity.aud) after JWT validation, such as: authorization: client-check: patternMatching: patterns: - selector: auth.identity.azp operator: eq value: "expected-client-id" This ensures only JWTs from authorized clients are accepted post-validation. issuerUrl must match the token's iss claim exactly for validation to succeed.
Citations:
- 1: https://docs.kuadrant.io/1.2.x/kuadrant-operator/doc/user-guides/ratelimiting/authenticated-rl-with-jwt-and-k8s-authnz/
- 2: https://docs.kuadrant.io/1.3.x/authorino/docs/features/
- 3: https://docs.kuadrant.io/0.11.0/kuadrant-operator/doc/user-guides/authenticated-rl-with-jwt-and-k8s-authnz/
- 4: https://docs.kuadrant.io/1.3.x/kuadrant-operator/doc/reference/authpolicy/
- 5: https://docs.kuadrant.io/1.3.x/kuadrant-operator/doc/overviews/auth/
- 6: https://docs.kuadrant.io/0.11.0/authorino/docs/features/
🏁 Script executed:
# Check if there are any other JWT validation examples in the repo
rg -i "audience|authorized.*party" deployment/ --type yaml | head -20Repository: opendatahub-io/models-as-a-service
Length of output: 341
🏁 Script executed:
# Search for any JWT validation documentation or comments in the repo
rg -i "jwt.*audience|aud.*claim" . --type yaml --type md | head -20Repository: opendatahub-io/models-as-a-service
Length of output: 60
Add audience/authorized-party constraint via authorization rule.
Severity: major (CWE-287). Authorino's JWT authentication validates the issuer URL, JWS signature, and token timing, but does not support direct audience or client-binding validation in the jwt spec. With issuerUrl alone, any JWT minted by that realm—including tokens issued for different clients—will pass JWT validation and allow token replay against maas-api. Add an authorization rule in spec.rules.authorization to validate the audience claim:
authorization:
jwt-audience-validation:
patternMatching:
patterns:
- selector: auth.identity.aud
operator: eq
value: "maas-api"
Or validate authorized party (azp) if present. Mirror the same constraint in scripts/data/maas-api-authpolicy-external-oidc-patch.yaml.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@deployment/overlays/odh/maas-api-auth-policy-oidc-patch.yaml` around lines 10
- 15, The oidc-identities block currently only sets jwt.issuerUrl (issuerUrl) so
add an authorization rule under spec.rules.authorization that enforces
audience/authorized-party checks: create a rule (e.g., jwt-audience-validation)
using patternMatching with selector auth.identity.aud operator eq value
"maas-api" and also add an alternative check for auth.identity.azp when present;
apply the same change pattern to the corresponding
scripts/data/maas-api-authpolicy-external-oidc-patch.yaml to keep external patch
in sync.
| - **Valid redirect URIs:** `https://*.{your-cluster-domain}/*` | ||
| - **Web origins:** `+` (allows CORS from redirect URIs) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Keycloak/OpenID Connect, are exact redirect URIs and explicit web origins recommended over wildcard redirect URIs or Web origins: + for confidential clients?
💡 Result:
Yes, in Keycloak/OpenID Connect, exact redirect URIs and explicit web origins are recommended over wildcard redirect URIs or Web origins: + for confidential clients.
Citations:
- 1: https://docs.redhat.com/en/documentation/red_hat_build_of_keycloak/22.0/html/server_administration_guide/assembly-managing-clients_server_administration_guide
- 2: https://www.keycloak.org/securing-apps/oidc-layers
- 3: https://docs.redhat.com/en/documentation/red_hat_build_of_keycloak/24.0/html/server_administration_guide/assembly-managing-clients_server_administration_guide
- 4: https://wjw465150.gitbooks.io/keycloak-documentation/content/server_admin/topics/clients/client-oidc.html
- 5: https://www.keycloak.org/docs/25.0.6/securing_apps/index.html
- 6: Clients policies for wildcards in redirect-uris keycloak/keycloak#9278
- 7: https://securityblog.omegapoint.se/en/writeup-keycloak-cve-2023-6927/
- 8: https://wjw465150.gitbooks.io/keycloak-documentation/content/server_admin/topics/threat/open-redirect.html
🏁 Script executed:
# First, find the README file and check its size
fd "keycloak.*README" --type fRepository: opendatahub-io/models-as-a-service
Length of output: 60
🌐 Web query:
CWE-346 Origin Validation Error and CWE-942 Permissive Cross-domain Policy - are these applicable to OIDC redirect URI and web origins security?
💡 Result:
Yes, both CWE-346 (Origin Validation Error) and CWE-942 (Permissive Cross-domain Policy with Untrusted Domains) are applicable to OIDC redirect URI and web origins security. CWE-346 applies to OIDC redirect URI validation. The OpenID Connect Core specification requires exact matching of the redirect_uri against pre-registered values using simple string comparison. Insufficient validation of the redirect_uri's origin (scheme, host) can lead to open redirects, SSRF, or open redirection attacks (related to CWE-601), where untrusted origins are accepted. Examples include failures to restrict dangerous schemes like javascript: or data:, or improper host/path validation, as seen in vulnerabilities like CVE-2020-10776 in Keycloak and analyses showing path confusion/parameter pollution risks. CWE-942 applies to web origins security in OIDC, particularly CORS configurations on OIDC endpoints (e.g., /.well-known/openid-configuration, /userinfo, /token). Permissive CORS policies (e.g., Access-Control-Allow-Origin: *) on these endpoints allow untrusted origins to make cross-origin requests, potentially leaking user info or tokens. Official docs recommend restricting to trusted origins, and implementations like Ory Hydra configure per-client allowed_cors_origins to avoid this.
Citations:
- 1: Validate JWKS URI against allowlist in GitHub OIDC flow apache/tooling-trusted-releases#671
- 2: https://cwe.mitre.org/data/definitions/346.html
- 3: https://cvefeed.io/cwe/detail/cwe-346-origin-validation-error
- 4: https://cwe.mitre.org/data/definitions/942.html
- 5: https://security.lauritz-holtmann.de/post/sso-security-redirect-uri/
- 6: https://dl.acm.org/doi/fullHtml/10.1145/3627106.3627140
- 7: https://security.lauritz-holtmann.de/post/sso-security-redirect-uri-ii/
- 8: Investigate missing
redirect_uricheck when performing OIDC flows ory/fosite#685 - 9: https://openid.net/specs/openid-connect-core-1_0-errata2.html
- 10: https://ory.sh/docs/hydra/guides/cors
🏁 Script executed:
# Search for the exact file
find . -path "*/docs/samples/install/keycloak/README.md" -type fRepository: opendatahub-io/models-as-a-service
Length of output: 119
🏁 Script executed:
# If found, read the file to verify the content at lines 74-75 and 164-179
cat -n docs/samples/install/keycloak/README.md | sed -n '70,80p'
cat -n docs/samples/install/keycloak/README.md | sed -n '160,180p'Repository: opendatahub-io/models-as-a-service
Length of output: 1316
Restrict Keycloak client redirect URIs and web origins to explicit values.
Severity: major. Lines 74–75 recommend wildcard redirect URIs and Web origins: +, which violate OIDC security controls. On a shared cluster domain, this allows any route under the wildcard to receive authorization responses (CWE-346: Origin Validation Error) and permits unrestricted CORS access to OIDC endpoints (CWE-942: Permissive Cross-domain Policy). OpenID Connect requires exact string matching of redirect_uri values. Use explicit, non-wildcard callback URLs and origins.
Suggested fix
- - **Valid redirect URIs:** `https://*.{your-cluster-domain}/*`
- - **Web origins:** `+` (allows CORS from redirect URIs)
+ - **Valid redirect URIs:** `https://<exact-maas-host>/<exact-callback-path>`
+ - **Web origins:** `https://<exact-maas-host>`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/samples/install/keycloak/README.md` around lines 74 - 75, Replace the
wildcard redirect URI and permissive Web origins in the README instructions:
change the "Valid redirect URIs: `https://*.{your-cluster-domain}/*`" guidance
to require explicit, exact callback URLs (e.g.,
`https://app.example.com/oauth/callback`) and update "Web origins: `+`" to list
explicit origins (e.g., `https://app.example.com`) only; ensure the text
references that Keycloak/OpenID Connect requires exact string matching of
redirect_uri and that CORS must be restricted to the named origins rather than
using `+` or wildcards.
| curl -k -X POST \ | ||
| "https://${KEYCLOAK_HOST}/realms/${REALM_NAME}/protocol/openid-connect/token" \ | ||
| -H "Content-Type: application/x-www-form-urlencoded" \ | ||
| -d "grant_type=password" \ | ||
| -d "client_id=${CLIENT_ID}" \ | ||
| -d "client_secret=${CLIENT_SECRET}" \ | ||
| -d "username=${USERNAME}" \ | ||
| -d "password=${PASSWORD}" \ | ||
| | jq . | ||
|
|
||
| # Decode the access token to verify groups claim | ||
| # Copy the access_token from above output | ||
| TOKEN="eyJhbGci..." | ||
|
|
||
| # Decode (requires jq and base64) | ||
| echo $TOKEN | cut -d'.' -f2 | base64 -d 2>/dev/null | jq . |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does curl's -k/--insecure option disable TLS certificate verification, and are JWT segments base64url encoded rather than standard base64?
💡 Result:
Yes, curl's -k/--insecure option disables TLS certificate verification. Yes, JWT segments are base64url encoded rather than standard base64.
Citations:
- 1: https://curl.se/docs/manpage.html
- 2: https://manpages.ubuntu.com/manpages/lunar/en/man1/curl.1.html
- 3: https://linuxman7.org/linux/man-pages/man1/curl.1.html
- 4: http://www.manpagez.com/man/1/curl/
- 5: https://www.rfc-editor.org/rfc/rfc7519
Remove -k from curl commands and fix JWT payload decoding.
Sending client secrets and passwords over HTTPS with curl -k (which disables certificate validation) enables MITM credential capture—CWE-295. The base64 decoding is also incorrect; JWT payloads are base64url-encoded, not standard base64, and require proper padding handling.
Suggested changes
- curl -k -X POST \
+ curl --fail --silent --show-error --cacert /path/to/router-ca.crt -X POST \- echo $TOKEN | cut -d'.' -f2 | base64 -d 2>/dev/null | jq .
+ TOKEN="$TOKEN" python - <<'PY'
+ import os, base64, json
+ payload = os.environ["TOKEN"].split(".")[1]
+ payload += "=" * (-len(payload) % 4)
+ print(json.dumps(json.loads(base64.urlsafe_b64decode(payload)), indent=2))
+ PY🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/samples/install/keycloak/README.md` around lines 164 - 179, Remove the
insecure curl option by deleting "-k" from the token request (the curl command
that posts to
"https://${KEYCLOAK_HOST}/realms/${REALM_NAME}/protocol/openid-connect/token")
and rely on system CA validation or supply a CA bundle (e.g., --cacert) instead;
for JWT decoding, change the payload decode pipeline that uses TOKEN and "echo
$TOKEN | cut -d'.' -f2 | base64 -d" to perform base64url decoding with URL-safe
character translation and proper padding (replace '-'/'_' to '+'/'/' and add '='
padding to a length multiple of 4) before calling base64 --decode (or an
equivalent base64url decode) so the JWT payload is decoded correctly.
| EXISTING_ARGS=$(kubectl get keycloak "$KEYCLOAK_NAME" -n "$NAMESPACE" \ | ||
| -o jsonpath='{.spec.unsupported.podTemplate.spec.containers[0].args}' 2>/dev/null || echo "[]") | ||
|
|
||
| if echo "$EXISTING_ARGS" | grep -q "import-realm"; then | ||
| echo " Keycloak already configured for realm import" | ||
| else | ||
| # Patch Keycloak to enable realm import | ||
| kubectl patch keycloak "$KEYCLOAK_NAME" -n "$NAMESPACE" --type=merge -p ' | ||
| { | ||
| "spec": { | ||
| "unsupported": { | ||
| "podTemplate": { | ||
| "spec": { | ||
| "containers": [ | ||
| { | ||
| "name": "keycloak", | ||
| "args": [ | ||
| "--verbose", | ||
| "start", | ||
| "--import-realm" | ||
| ], | ||
| "volumeMounts": [ | ||
| { | ||
| "name": "test-realms", | ||
| "mountPath": "/opt/keycloak/data/import" | ||
| } | ||
| ] | ||
| } | ||
| ], | ||
| "volumes": [ | ||
| { | ||
| "name": "test-realms", | ||
| "configMap": { | ||
| "name": "'${CONFIGMAP_NAME}'" | ||
| } | ||
| } | ||
| ] | ||
| } | ||
| } | ||
| } | ||
| } | ||
| }' |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For kubectl patch --type=mergeon a Kubernetes CustomResource, are list fields replaced wholesale, and does the Keycloak Operator documentspec.unsupported.podTemplate as an unsupported escape hatch?
💡 Result:
For kubectl patch --type=merge on a Kubernetes CustomResource, list fields are replaced wholesale. Strategic merge patch, which intelligently merges lists based on patchStrategy and merge keys defined in built-in types, is not supported for custom resources. With --type=merge, it falls back to JSON merge patch semantics where providing a list replaces the entire existing list. The Keycloak Operator documents spec.unsupported.podTemplate as an unsupported escape hatch. Official documentation explicitly describes it as a "temporary workaround" for cases where no supported field exists, containing "highly experimental configuration options that are not completely tested and are Tech Preview," with "no guarantee exists that the Deployment will work as expected."
Citations:
- 1: https://kubernetes.io/docs/tasks/manage-kubernetes-objects/update-api-object-kubectl-patch/
- 2: https://v1-33.docs.kubernetes.io/docs/tasks/manage-kubernetes-objects/update-api-object-kubectl-patch
- 3: https://kubernetes.io/docs/reference/kubectl/generated/kubectl_patch/
- 4: https://stackoverflow.com/questions/57480205/error-while-applying-json-patch-to-kubernetes-custom-resource
- 5: https://www.keycloak.org/operator/advanced-configuration
- 6: https://docs.redhat.com/en/documentation/red_hat_build_of_keycloak/26.0/html/operator_guide/advanced-configuration-
- 7: https://keycloak.org/operator/advanced-configuration
Avoid merge-patching the unsupported pod template on CustomResources.
kubectl patch --type=merge on CustomResources uses JSON merge patch semantics (not strategic merge patch), which replaces array fields wholesale. This patch will clobber any existing spec.unsupported.podTemplate.spec.containers or volumes entries. The Keycloak Operator explicitly documents spec.unsupported.podTemplate as a "temporary workaround" and unsupported escape hatch with "no guarantee [the] Deployment will work as expected."
Additionally, hardcoding the full Keycloak args list tightly couples this import flow to operator and container internals.
Refuse to run when spec.unsupported.podTemplate is already populated, or refetch and merge the current object instead of replacing it. Minimum guard:
Guard to prevent clobbering existing unsupported pod settings
if echo "$EXISTING_ARGS" | grep -q "import-realm"; then
echo " Keycloak already configured for realm import"
+elif kubectl get keycloak "$KEYCLOAK_NAME" -n "$NAMESPACE" \
+ -o jsonpath='{.spec.unsupported.podTemplate}' 2>/dev/null | grep -q .; then
+ echo "❌ ERROR: existing spec.unsupported.podTemplate detected; refusing to overwrite it" >&2
+ exit 1
else📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| EXISTING_ARGS=$(kubectl get keycloak "$KEYCLOAK_NAME" -n "$NAMESPACE" \ | |
| -o jsonpath='{.spec.unsupported.podTemplate.spec.containers[0].args}' 2>/dev/null || echo "[]") | |
| if echo "$EXISTING_ARGS" | grep -q "import-realm"; then | |
| echo " Keycloak already configured for realm import" | |
| else | |
| # Patch Keycloak to enable realm import | |
| kubectl patch keycloak "$KEYCLOAK_NAME" -n "$NAMESPACE" --type=merge -p ' | |
| { | |
| "spec": { | |
| "unsupported": { | |
| "podTemplate": { | |
| "spec": { | |
| "containers": [ | |
| { | |
| "name": "keycloak", | |
| "args": [ | |
| "--verbose", | |
| "start", | |
| "--import-realm" | |
| ], | |
| "volumeMounts": [ | |
| { | |
| "name": "test-realms", | |
| "mountPath": "/opt/keycloak/data/import" | |
| } | |
| ] | |
| } | |
| ], | |
| "volumes": [ | |
| { | |
| "name": "test-realms", | |
| "configMap": { | |
| "name": "'${CONFIGMAP_NAME}'" | |
| } | |
| } | |
| ] | |
| } | |
| } | |
| } | |
| } | |
| }' | |
| EXISTING_ARGS=$(kubectl get keycloak "$KEYCLOAK_NAME" -n "$NAMESPACE" \ | |
| -o jsonpath='{.spec.unsupported.podTemplate.spec.containers[0].args}' 2>/dev/null || echo "[]") | |
| if echo "$EXISTING_ARGS" | grep -q "import-realm"; then | |
| echo " Keycloak already configured for realm import" | |
| elif kubectl get keycloak "$KEYCLOAK_NAME" -n "$NAMESPACE" \ | |
| -o jsonpath='{.spec.unsupported.podTemplate}' 2>/dev/null | grep -q .; then | |
| echo "❌ ERROR: existing spec.unsupported.podTemplate detected; refusing to overwrite it" >&2 | |
| exit 1 | |
| else | |
| # Patch Keycloak to enable realm import | |
| kubectl patch keycloak "$KEYCLOAK_NAME" -n "$NAMESPACE" --type=merge -p ' | |
| { | |
| "spec": { | |
| "unsupported": { | |
| "podTemplate": { | |
| "spec": { | |
| "containers": [ | |
| { | |
| "name": "keycloak", | |
| "args": [ | |
| "--verbose", | |
| "start", | |
| "--import-realm" | |
| ], | |
| "volumeMounts": [ | |
| { | |
| "name": "test-realms", | |
| "mountPath": "/opt/keycloak/data/import" | |
| } | |
| ] | |
| } | |
| ], | |
| "volumes": [ | |
| { | |
| "name": "test-realms", | |
| "configMap": { | |
| "name": "'${CONFIGMAP_NAME}'" | |
| } | |
| } | |
| ] | |
| } | |
| } | |
| } | |
| } | |
| }' |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/samples/install/keycloak/test-realms/apply-test-realms.sh` around lines
68 - 109, The current kubectl patch uses JSON merge semantics which will
overwrite arrays under spec.unsupported.podTemplate (containers, volumes);
instead either refuse to run if spec.unsupported.podTemplate is already
populated or perform a safe merge: fetch the existing Keycloak CR (use
EXISTING_ARGS/kubectl get keycloak "$KEYCLOAK_NAME" -n "$NAMESPACE" -o json),
inspect spec.unsupported.podTemplate, and if it exists exit with a clear
message; otherwise build a merged object that appends/merges the new container
entry (name "keycloak", args including "--import-realm" but avoid hardcoding the
full args list) and the test-realms volume into the existing arrays, then apply
the merged CR via kubectl apply -f - (or use kubectl patch --type=json with
explicit add operations targeting specific array positions) to avoid clobbering
containers or volumes; ensure references to CONFIGMAP_NAME, KEYCLOAK_NAME,
spec.unsupported.podTemplate, containers, and volumes are used to locate and
merge the correct fields.
| if ! patch_authpolicy_from_template "$authpolicy_name" "$api_keys_patch" "$NAMESPACE"; then | ||
| log_warn " Failed to patch AuthPolicy with API key configuration" | ||
| return 0 | ||
| fi |
There was a problem hiding this comment.
AuthPolicy patch failures should abort the deployment.
Both branches warn and return success, so --external-oidc can finish green while the API-key path or JWT validation was never installed. For auth configuration, fail closed.
Suggested fix
log_info " Patching AuthPolicy to ensure API key support..."
if ! patch_authpolicy_from_template "$authpolicy_name" "$api_keys_patch" "$NAMESPACE"; then
- log_warn " Failed to patch AuthPolicy with API key configuration"
- return 0
+ log_error " Failed to patch AuthPolicy with API key configuration"
+ return 1
fi
@@
log_info " Enabling OIDC JWT validation with issuer: $oidc_issuer_url"
if ! patch_authpolicy_from_template "$authpolicy_name" "$oidc_patch" "$NAMESPACE" "$oidc_issuer_url"; then
- log_warn " Failed to patch AuthPolicy with external OIDC configuration"
- return 0
+ log_error " Failed to patch AuthPolicy with external OIDC configuration"
+ return 1
fiAlso applies to: 1504-1507
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/deploy.sh` around lines 1486 - 1489, The script currently treats
failures from patch_authpolicy_from_template as non-fatal (calls log_warn then
returns 0), which lets deployment succeed despite AuthPolicy not being applied;
change both occurrences (the block using patch_authpolicy_from_template
"$authpolicy_name" "$api_keys_patch" "$NAMESPACE" and the similar block at the
other location) to treat failure as fatal by returning a non-zero status or
exiting non-zero (e.g., replace the current log_warn + return 0 with log_error +
return 1 or an exit 1) so the deployment aborts on AuthPolicy patch failure.
| # Access Keycloak Admin Console: | ||
| # 1. Get admin password: | ||
| # kubectl get secret -n keycloak-system keycloak-initial-admin \ | ||
| # -o jsonpath='{.data.password}' | base64 -d | ||
| # 2. Navigate to: https://keycloak.{cluster-domain} |
There was a problem hiding this comment.
Fix the bootstrap secret name in the usage banner.
Lines 28-29 point readers at keycloak-initial-admin, but this file later prints ${KEYCLOAK_NAME}-initial-admin at Lines 294-299 and docs/samples/install/keycloak/README.md:19-24 uses maas-keycloak-initial-admin. Following the banner as written returns NotFound.
Suggested fix
-# kubectl get secret -n keycloak-system keycloak-initial-admin \
+# kubectl get secret -n keycloak-system maas-keycloak-initial-admin \🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/setup-keycloak.sh` around lines 26 - 30, The usage banner currently
points to the hardcoded secret name "keycloak-initial-admin" which is
inconsistent with the rest of the script and docs; change the banner to
reference the dynamic secret name using the KEYCLOAK_NAME variable (i.e.,
replace "keycloak-initial-admin" with "${KEYCLOAK_NAME}-initial-admin" so it
matches occurrences like ${KEYCLOAK_NAME}-initial-admin printed later and in
docs/samples/install/keycloak/README.md).
| channel: fast | ||
| name: keycloak-operator | ||
| source: community-operators | ||
| sourceNamespace: openshift-marketplace | ||
| installPlanApproval: Automatic |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What stability guarantees does the Keycloak Operator fast channel provide, and is pinning a tested operator release recommended for reproducible installs?
💡 Result:
The Keycloak Operator's fast channel provides supported, production-quality releases that are monitored for stability before potential promotion to a stable channel, per standard OLM channel conventions. It delivers new minor and patch versions as soon as they are generally available, offering full support but intended for early adopters or pre-production testing to assess quality. No explicit stability guarantees like "no breaking changes" are stated in official Keycloak docs beyond general OLM practices. Pinning a specific tested operator release (via startingCSV in the Subscription or manual approval) is strongly recommended for reproducible installs. Official docs warn that automatic OLM upgrades can cause issues like database migration problems, CR incompatibilities, and lack of downgrade options, advising manual approval and testing before upgrades.
Citations:
- 1: https://www.keycloak.org/operator/installation
- 2: https://olm.operatorframework.io/docs/best-practices/channel-naming/
- 3: https://docs.redhat.com/en/documentation/red_hat_build_of_keycloak/26.4/html/operator_guide/installation-
- 4: https://keycloak.org/operator/rolling-updates
- 5: https://artifacthub.io/packages/olm/community-operators/keycloak-operator/26.4.2
Pin the Keycloak Operator to a tested release instead of subscribing to fast.
The unpinned fast channel subscription causes non-reproducible deployments: subsequent runs can pull different operator versions without any code change, while docs/samples/install/keycloak/test-realms/apply-test-realms.sh:68-109 depends on unsupported pod-template mutation. Official Keycloak documentation warns that automatic OLM upgrades can trigger database migration problems and CR incompatibilities. Pin a specific tested operator release via startingCSV in the Subscription or use manual approval, and expose the operator source/channel as configurable inputs with a known-good default.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/setup-keycloak.sh` around lines 91 - 95, Replace the unpinned
Subscription channel value to pin a tested Keycloak Operator release: set a
specific startingCSV in the Subscription (instead of relying solely on channel:
fast) and change installPlanApproval to Manual (or provide startingCSV with
Automatic if you intentionally pin a version), and make the operator source and
channel configurable inputs (e.g., variables used when rendering the
Subscription) with a known-good default; update the subscription block that
currently contains the fields channel, name, source, sourceNamespace,
installPlanApproval to accept configurable inputs and include startingCSV to fix
the non-reproducible upgrade behavior.
| def _request_oidc_token() -> str: | ||
| token_url = _required_env("OIDC_TOKEN_URL") | ||
| client_id = _required_env("OIDC_CLIENT_ID") | ||
| username = _required_env("OIDC_USERNAME") | ||
| password = _required_env("OIDC_PASSWORD") | ||
|
|
||
| response = requests.post( | ||
| token_url, | ||
| data={ | ||
| "grant_type": "password", | ||
| "client_id": client_id, | ||
| "username": username, | ||
| "password": password, | ||
| }, | ||
| timeout=30, | ||
| verify=TLS_VERIFY, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd test/e2e && find . -name "test_external_oidc.py" -o -name "prow_run_smoke_test.sh" -o -name "conftest.py" | head -20Repository: opendatahub-io/models-as-a-service
Length of output: 160
🏁 Script executed:
cat -n test/e2e/tests/test_external_oidc.py | head -100Repository: opendatahub-io/models-as-a-service
Length of output: 4415
🏁 Script executed:
cat test/e2e/scripts/prow_run_smoke_test.shRepository: opendatahub-io/models-as-a-service
Length of output: 33603
🏁 Script executed:
cat -n test/e2e/tests/conftest.py 2>/dev/null | head -100Repository: opendatahub-io/models-as-a-service
Length of output: 4181
Split TLS verification for external OIDC token exchange.
Severity: major (CWE-295). _request_oidc_token() uses verify=TLS_VERIFY, which is disabled when E2E_SKIP_TLS_VERIFY=true (set globally in prow_run_smoke_test.sh for self-signed cluster certificates). This applies TLS verification bypass to the external IdP password-grant exchange against OIDC_TOKEN_URL, exposing real credentials (username, password, client_id) to MITM attacks. Introduce a dedicated OIDC_TLS_VERIFY setting that defaults to enabled, independent of cluster TLS configuration.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/e2e/tests/test_external_oidc.py` around lines 25 - 40,
_request_oidc_token currently reuses global TLS_VERIFY causing tests to disable
TLS for external OIDC token exchange; introduce a separate OIDC_TLS_VERIFY env
flag (default true) and use it for the requests.post verify parameter in
_request_oidc_token instead of TLS_VERIFY; parse OIDC_TLS_VERIFY into a boolean
(e.g., via existing env helper or a new _bool_env/_optional_env) and document
default behavior so password-grant requests to OIDC_TOKEN_URL always verify TLS
unless explicitly overridden.
| def test_oidc_token_can_create_api_key(self, maas_api_base_url: str): | ||
| token = _request_oidc_token() | ||
| data = _create_oidc_api_key(maas_api_base_url, token) | ||
| print(f"[oidc] created api key id={data.get('id')} prefix={data.get('key', '')[:18]}...") |
There was a problem hiding this comment.
Stop printing API-key prefixes.
Severity: minor (CWE-532). Even a truncated sk-oai-* value ends up in retained CI artifacts and is still credential material. Log the key id only.
Proposed fix
- print(f"[oidc] created api key id={data.get('id')} prefix={data.get('key', '')[:18]}...")
+ print(f"[oidc] created api key id={data.get('id')}")As per coding guidelines, "Security vulnerabilities (provide severity, exploit scenario, and remediation code)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/e2e/tests/test_external_oidc.py` around lines 65 - 68, The test
test_oidc_token_can_create_api_key prints a truncated API key prefix which leaks
credential material; update the print/logging in that test (the print in
test_oidc_token_can_create_api_key that uses data.get('key')) to stop printing
any part of the key and only output the API key id (data.get('id')) or remove
the print entirely—locate the print statement in
test_oidc_token_can_create_api_key and replace it so only the id is logged (or
no secret-derived values are included).
2 participants
JIRA - https://redhat.atlassian.net/browse/RHOAIENG-47977
Adding optional external OIDC authentication to the maas-api AuthPolicy so maas can accept Keycloak-issued JWTs in addition to the existing flow. + sample of Keycloak installation.
Not ready for review (awaits changes to how we modify AuthPolicy, coming change with a new CRD introduction)
Summary by CodeRabbit
Release Notes
New Features
Documentation
Tests