fix: move Keycloak admin credentials from ConfigMap to Secret (rebased #161)#179
Conversation
Signed-off-by: Ryan Cook <rcook@redhat.com>
Update demo-ui.md and demo-manual.md to document the new keycloak-admin-secret Secret that client-registration reads admin credentials from (instead of the environments ConfigMap). Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Mariusz Sabath <mrsabath@gmail.com>
pdettori
left a comment
pdettori
left a comment
There was a problem hiding this comment.
Good security improvement moving Keycloak admin credentials from ConfigMap to a dedicated Secret. Code, RBAC, examples, docs, and tests are all updated consistently.
Areas reviewed: Go, Helm/RBAC, YAML, Docs
Commits: 2, all signed-off ✓
CI: passing ✓
One suggestion on configmaps-webhook.yaml (misplaced comment block) and two minor test robustness nits. Nothing blocking.
| name: keycloak-admin-secret | ||
| namespace: team1 | ||
| type: Opaque | ||
| stringData: |
There was a problem hiding this comment.
suggestion: The --- + Secret header insertion causes the PLATFORM_CLIENT_IDS comment block (the lines after KEYCLOAK_ADMIN_PASSWORD) to land under the Secret's stringData: instead of the ConfigMap's data:. The comments are YAML comments so it's not functionally broken, but it's misleading — a reader would think PLATFORM_CLIENT_IDS belongs in the Secret. Consider moving the PLATFORM_CLIENT_IDS comment block back above the --- separator under the ConfigMap.
| @@ -134,3 +134,45 @@ func TestBuildClientRegistrationContainer_HasPlatformClientIDsEnv(t *testing.T) | |||
| t.Error("client-registration container missing PLATFORM_CLIENT_IDS env var") | |||
| } | |||
| } | |||
There was a problem hiding this comment.
nit: This test silently passes if neither KEYCLOAK_ADMIN_USERNAME nor KEYCLOAK_ADMIN_PASSWORD is found in the env list at all (the inner loop just skips). Consider adding a found bool or t.Errorf if the key is absent, so the test catches accidental removal of these env vars.
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
nit: Same as above — silently passes if KEYCLOAK_URL or KEYCLOAK_REALM are missing from the env list entirely.
| verbs: ["get", "list", "watch"] | ||
| - apiGroups: [""] | ||
| resources: ["secrets"] | ||
| verbs: ["get", "list", "watch"] |
There was a problem hiding this comment.
👍 Good least-privilege: only get, list, watch on secrets — no write access needed.
- Move PLATFORM_CLIENT_IDS comment block from Secret stringData back to the environments ConfigMap data section where it belongs - Add 'found' checks to AdminCredentialsFromSecret test so it fails if KEYCLOAK_ADMIN_USERNAME/PASSWORD env vars are absent entirely - Add 'found' checks to NonSensitiveKeysFromConfigMap test so it fails if KEYCLOAK_URL/KEYCLOAK_REALM env vars are absent entirely Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Mariusz Sabath <mrsabath@gmail.com>
3 participants
Summary
main(resolving conflicts with feat: migrate demo-ui.md from demo realm to kagenti realm #149, feat: add client_credentials fallback for outbound requests without Authorization header #171, feat: add agent audience scope to platform clients during registration #172)KEYCLOAK_ADMIN_USERNAMEandKEYCLOAK_ADMIN_PASSWORDfrom theenvironmentsConfigMap to a dedicatedkeycloak-admin-secretSecretdemo-ui.md,demo-manual.md) with thekeycloak-admin-secretcreation stepPLATFORM_CLIENT_IDSenv var from feat: add agent audience scope to platform clients during registration #172 (the original PR inadvertently removed it due to being based on an older commit)Changes from original #161
kubectl create secret generic keycloak-admin-secretBuildClientRegistrationContainerWithSpireOption(current API)Related
Test plan
secretKeyReffor admin credsSigned-off-by: Mariusz Sabath mrsabath@gmail.com