Removed hardcoded secrets#121
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR externalizes deployment secrets across Compose, Helm, CI/CD, and docs, adds shared Kubernetes security-context hardening, and changes py-genai-helper database initialization to retry when tables are not yet available. ChangesSecret externalization across compose, Helm, CI/CD, and docs
Kubernetes pod security hardening
py-genai-helper database initialization retry
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
infra/ansible/playbook.yml (1)
86-93: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winSuppress Ansible logging for the secret copy task.
Add
no_log: trueso compose secrets cannot appear in verbose or diff output.Proposed change
- name: Write compose .env file copy: src: "{{ compose_env_file }}" dest: "{{ app_dir }}/infra/.env" mode: "0600" owner: "{{ ansible_user }}" group: "{{ ansible_user }}" + no_log: true🤖 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 `@infra/ansible/playbook.yml` around lines 86 - 93, The Write compose .env file task in the playbook should suppress sensitive output so secrets do not leak in Ansible logs. Update the copy task for the compose env file to use no_log: true, keeping the existing behavior in place while preventing verbose or diff output from exposing secrets.
🤖 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 @.github/workflows/cd.yml:
- Around line 33-64: The compose .env writer in the cd workflow is missing
required secrets, so values in infra/docker-compose.yml can fall back to
defaults. Update the “Write compose .env file” step to include
GRAFANA_OAUTH_CLIENT_SECRET and FORWARD_AUTH_CLIENT_SECRET alongside the
existing secret exports, using the same secret-to-env pattern as the other
variables. Also add a simple validation in that step to fail fast if any
required compose secret is empty before writing /tmp/compose.env.
- Around line 289-300: The Helm deploy step is using --set for secret-backed
password inputs, which can still be type-inferred or mangled before templating;
update the affected entries in the deploy command to use --set-string for the
database and keycloak password flags referenced in the workflow snippet. If any
of these secrets may contain commas or other special characters, prefer passing
them through a temporary values file instead of inline CLI flags.
In `@infra/.env.example`:
- Around line 9-21: Replace the hardcoded sample credentials in
infra/.env.example with non-reusable placeholders, especially
FORWARD_AUTH_COOKIE_SECRET and the password entries. Update the examples to use
blanks or <generate-random> style values and add a brief note that each
developer/environment must supply its own secrets locally. Keep the changes
within the existing environment variable list so the file remains an example
only.
In `@infra/docker-compose.yml`:
- Line 300: The Grafana and Traefik OAuth secret fallbacks are still hardcoded
in the docker-compose configuration, so remove the default values from the
GF_AUTH_GENERIC_OAUTH_CLIENT_SECRET and Traefik forward auth secret references.
Update the compose setup to use required environment interpolation for both
secrets and make sure the workflow writes them into compose.env from GitHub
Secrets, using the existing Grafana and Traefik secret variables as the points
to update.
- Around line 431-433: The realm-config render step in the docker-compose
Keycloak import command is doing raw sed substitution, which can break on
passwords containing /, &, quotes, or backslashes. Update the Keycloak realm
rendering flow to use a JSON-aware templating/rendering approach, or explicitly
escape the secret values before substitution, and make sure the replacement
logic in the existing sed-based import command no longer writes invalid JSON.
In `@infra/helm/team-devoops/templates/keycloak-realm-configmap.yaml`:
- Around line 10-11: The Keycloak realm payload is being rendered in the
ConfigMap via the keycloak-realm-configmap template, which exposes admin/user
passwords and can break JSON when values contain special characters. Move the
rendered realm JSON generation out of the ConfigMap into a Secret, and update
the Keycloak mount/reference to read from that Secret instead. Use the existing
keycloak-realm-config template and the realm-config.json rendering path as the
main locations to adjust, keeping the ConfigMap free of secret material.
In `@infra/helm/team-devoops/values.yaml`:
- Around line 251-252: The forward-auth OIDC client secret still has a hardcoded
default, so it needs to be treated like the cookie secret and injected at deploy
time instead. Update the forward-auth configuration in values.yaml to remove the
default for forwardAuth.clientSecret and wire it from a GitHub Secret alongside
the existing deploy-time secret handling, using the forwardAuth.* settings to
locate the affected config.
---
Nitpick comments:
In `@infra/ansible/playbook.yml`:
- Around line 86-93: The Write compose .env file task in the playbook should
suppress sensitive output so secrets do not leak in Ansible logs. Update the
copy task for the compose env file to use no_log: true, keeping the existing
behavior in place while preventing verbose or diff output from exposing secrets.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3cdf96bb-d78a-40c5-b7c4-173ec900e679
📒 Files selected for processing (10)
.github/workflows/cd.ymlREADME.mdinfra/.env.exampleinfra/ansible/playbook.ymlinfra/docker-compose.override.ymlinfra/docker-compose.ymlinfra/helm/team-devoops/files/realm-config.jsoninfra/helm/team-devoops/templates/keycloak-realm-configmap.yamlinfra/helm/team-devoops/values.yamlinfra/keycloak/realm-config.json
| - name: Write compose .env file | ||
| env: | ||
| POSTGRES_PASSWORD: ${{ secrets.DB_ADMIN_PASSWORD }} | ||
| ORGANIZATION_DB_PASSWORD: ${{ secrets.DB_ORGANIZATION_PASSWORD }} | ||
| MEMBER_DB_PASSWORD: ${{ secrets.DB_MEMBER_PASSWORD }} | ||
| EVENT_DB_PASSWORD: ${{ secrets.DB_EVENT_PASSWORD }} | ||
| FEEDBACK_DB_PASSWORD: ${{ secrets.DB_FEEDBACK_PASSWORD }} | ||
| FINANCE_DB_PASSWORD: ${{ secrets.DB_FINANCE_PASSWORD }} | ||
| LETTER_DB_PASSWORD: ${{ secrets.DB_LETTER_PASSWORD }} | ||
| REPORTS_DB_PASSWORD: ${{ secrets.DB_REPORTS_PASSWORD }} | ||
| KEYCLOAK_ADMIN_PASSWORD: ${{ secrets.KEYCLOAK_ADMIN_PASSWORD }} | ||
| KEYCLOAK_DB_PASSWORD: ${{ secrets.KEYCLOAK_DB_PASSWORD }} | ||
| KEYCLOAK_REALM_ADMIN_PASSWORD: ${{ secrets.KEYCLOAK_REALM_ADMIN_PASSWORD }} | ||
| KEYCLOAK_REALM_USER_PASSWORD: ${{ secrets.KEYCLOAK_REALM_USER_PASSWORD }} | ||
| FORWARD_AUTH_COOKIE_SECRET: ${{ secrets.FORWARD_AUTH_COOKIE_SECRET }} | ||
| run: | | ||
| cat > /tmp/compose.env << ENVFILE | ||
| POSTGRES_PASSWORD=$POSTGRES_PASSWORD | ||
| ORGANIZATION_DB_PASSWORD=$ORGANIZATION_DB_PASSWORD | ||
| MEMBER_DB_PASSWORD=$MEMBER_DB_PASSWORD | ||
| EVENT_DB_PASSWORD=$EVENT_DB_PASSWORD | ||
| FEEDBACK_DB_PASSWORD=$FEEDBACK_DB_PASSWORD | ||
| FINANCE_DB_PASSWORD=$FINANCE_DB_PASSWORD | ||
| LETTER_DB_PASSWORD=$LETTER_DB_PASSWORD | ||
| REPORTS_DB_PASSWORD=$REPORTS_DB_PASSWORD | ||
| KEYCLOAK_ADMIN_PASSWORD=$KEYCLOAK_ADMIN_PASSWORD | ||
| KEYCLOAK_DB_PASSWORD=$KEYCLOAK_DB_PASSWORD | ||
| KEYCLOAK_REALM_ADMIN_PASSWORD=$KEYCLOAK_REALM_ADMIN_PASSWORD | ||
| KEYCLOAK_REALM_USER_PASSWORD=$KEYCLOAK_REALM_USER_PASSWORD | ||
| FORWARD_AUTH_COOKIE_SECRET=$FORWARD_AUTH_COOKIE_SECRET | ||
| ENVFILE | ||
|
|
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Write and validate all Compose secrets.
This file omits GRAFANA_OAUTH_CLIENT_SECRET and FORWARD_AUTH_CLIENT_SECRET, so Compose falls back to the literal defaults in infra/docker-compose.yml. Add those secrets and fail fast if any required value is empty.
🤖 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 @.github/workflows/cd.yml around lines 33 - 64, The compose .env writer in
the cd workflow is missing required secrets, so values in
infra/docker-compose.yml can fall back to defaults. Update the “Write compose
.env file” step to include GRAFANA_OAUTH_CLIENT_SECRET and
FORWARD_AUTH_CLIENT_SECRET alongside the existing secret exports, using the same
secret-to-env pattern as the other variables. Also add a simple validation in
that step to fail fast if any required compose secret is empty before writing
/tmp/compose.env.
| --set database.password="${{ secrets.DB_ADMIN_PASSWORD }}" \ | ||
| --set database.users.organization.password="${{ secrets.DB_ORGANIZATION_PASSWORD }}" \ | ||
| --set database.users.member.password="${{ secrets.DB_MEMBER_PASSWORD }}" \ | ||
| --set database.users.event.password="${{ secrets.DB_EVENT_PASSWORD }}" \ | ||
| --set database.users.feedback.password="${{ secrets.DB_FEEDBACK_PASSWORD }}" \ | ||
| --set database.users.finance.password="${{ secrets.DB_FINANCE_PASSWORD }}" \ | ||
| --set database.users.letter.password="${{ secrets.DB_LETTER_PASSWORD }}" \ | ||
| --set database.users.reports.password="${{ secrets.DB_REPORTS_PASSWORD }}" \ | ||
| --set keycloak.adminPassword="${{ secrets.KEYCLOAK_ADMIN_PASSWORD }}" \ | ||
| --set keycloak.db.password="${{ secrets.KEYCLOAK_DB_PASSWORD }}" \ | ||
| --set keycloak.users.admin.password="${{ secrets.KEYCLOAK_REALM_ADMIN_PASSWORD }}" \ | ||
| --set keycloak.users.user.password="${{ secrets.KEYCLOAK_REALM_USER_PASSWORD }}" \ |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect Helm secret injection sites and template consumers before switching to --set-string or a values file.
rg -n -C 2 -- '--set .*secrets\.|database\.users\..*password|keycloak\..*password|forwardAuth\..*Secret' .github/workflows infra/helm/team-devoopsRepository: AET-DevOps26/team-devoops
Length of output: 7434
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the exact deploy command and the corresponding Helm value consumers.
sed -n '270,305p' .github/workflows/cd.yml
printf '\n---\n'
sed -n '1,120p' infra/helm/team-devoops/templates/secret-db.yaml
printf '\n---\n'
sed -n '1,80p' infra/helm/team-devoops/templates/keycloak-secret.yaml
printf '\n---\n'
sed -n '1,80p' infra/helm/team-devoops/templates/keycloak-realm-configmap.yaml
printf '\n---\n'
sed -n '1,90p' infra/helm/team-devoops/templates/forward-auth.yamlRepository: AET-DevOps26/team-devoops
Length of output: 7746
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check whether these values are documented as plain strings or can be interpreted as numbers/bools.
sed -n '1,40p' infra/helm/team-devoops/values.yaml
printf '\n---\n'
sed -n '176,220p' infra/helm/team-devoops/values.yamlRepository: AET-DevOps26/team-devoops
Length of output: 3444
🌐 Web query:
Helm --set type coercion --set-string passwords documentation
💡 Result:
In Helm, the --set and --set-string flags serve different purposes regarding type coercion [1][2][3]. --set The --set flag attempts to automatically detect and coerce values into appropriate types (such as integers, floats, booleans, or strings) [4][5][6]. Because of this, it can sometimes behave unexpectedly when a value looks like a number or boolean but should be treated as a string (e.g., a password containing only digits or a Git commit hash) [5][6][7]. --set-string The --set-string flag is designed to force values to be treated as strings, bypassing the automatic type coercion performed by the --set parser [5][6]. It is the recommended approach for values like passwords, image tags, or any other data where you want to ensure the input is interpreted strictly as a string [6]. Important Notes: 1. Syntax Compatibility: Despite its name, --set-string still supports Helm's complex value syntax, allowing you to define arrays and dictionaries (e.g., --set-string list={1,2,3}) [5]. It simply forces the terminal leaf values to be strings rather than native types [5]. 2. Not a Literal Flag: It is important to note that --set-string is not a "literal" flag [5]. It does not prevent all interpretation; for example, it may still be subject to shell expansion or Helm's internal parsing of complex structures [4][5]. If you require a value to be treated absolutely literally, or if you encounter issues where --set-string does not produce the desired result (such as with certain boolean-looking strings), you may need to quote the value or ensure your Helm chart templates handle the input correctly [4][5]. 3. Precedence: When using these flags, the last (right-most) value specified takes precedence if the same key is set multiple times [1]. For security-sensitive data like passwords, using --set-string is a standard practice to prevent Helm from misinterpreting numeric strings as integers or floating-point numbers [6].
Citations:
- 1: https://github.com/helm/helm-www/blob/main/docs/helm/helm_install.md
- 2: https://v3-1-0.helm.sh/docs/helm/helm_install/
- 3: https://v2.helm.sh/docs/helm/
- 4: --set-string doesn't seem to handle booleans properly helm/helm#4006
- 5: Provide an option or alternative for --set and/or --set-string to take a value literally helm/helm#4030
- 6: --set parses number only values as float64 helm/helm#1707
- 7: Force string values with --set helm/helm#3155
Use --set-string for the secret-backed Helm inputs here. Helm still type-infers these values before the templates quote them, so a numeric-looking password can change shape; if any secret may contain commas or other special characters, switch to a temporary values file instead. [.github/workflows/cd.yml:289-300]
🤖 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 @.github/workflows/cd.yml around lines 289 - 300, The Helm deploy step is
using --set for secret-backed password inputs, which can still be type-inferred
or mangled before templating; update the affected entries in the deploy command
to use --set-string for the database and keycloak password flags referenced in
the workflow snippet. If any of these secrets may contain commas or other
special characters, prefer passing them through a temporary values file instead
of inline CLI flags.
| POSTGRES_PASSWORD=app_admin_password | ||
| ORGANIZATION_DB_PASSWORD=organization_password | ||
| MEMBER_DB_PASSWORD=member_password | ||
| EVENT_DB_PASSWORD=event_password | ||
| FEEDBACK_DB_PASSWORD=feedback_password | ||
| FINANCE_DB_PASSWORD=finance_password | ||
| LETTER_DB_PASSWORD=letter_password | ||
| REPORTS_DB_PASSWORD=reports_password | ||
| KEYCLOAK_ADMIN_PASSWORD=admin | ||
| KEYCLOAK_DB_PASSWORD=keycloak_password | ||
| KEYCLOAK_REALM_ADMIN_PASSWORD=admin123 | ||
| KEYCLOAK_REALM_USER_PASSWORD=user123 | ||
| FORWARD_AUTH_COOKIE_SECRET=VdpPltrnS+cc+1hswmya727K69iG7MpMZH/LOLlmox0= |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Replace the example credentials with placeholders.
These are still real-looking secrets, not examples. In particular, the FORWARD_AUTH_COOKIE_SECRET value is a shared session-signing key, so copying this file verbatim would give every environment the same cookie secret. Use blanks or <generate-random> placeholders instead and document how to populate them locally. As per PR objective "Removed hardcoded secrets", .env.example should not ship reusable credentials.
🧰 Tools
🪛 Betterleaks (1.6.0)
[high] 21-21: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 dotenv-linter (4.0.0)
[warning] 10-10: [UnorderedKey] The ORGANIZATION_DB_PASSWORD key should go before the POSTGRES_PASSWORD key
(UnorderedKey)
[warning] 11-11: [UnorderedKey] The MEMBER_DB_PASSWORD key should go before the ORGANIZATION_DB_PASSWORD key
(UnorderedKey)
[warning] 12-12: [UnorderedKey] The EVENT_DB_PASSWORD key should go before the MEMBER_DB_PASSWORD key
(UnorderedKey)
[warning] 13-13: [UnorderedKey] The FEEDBACK_DB_PASSWORD key should go before the MEMBER_DB_PASSWORD key
(UnorderedKey)
[warning] 14-14: [UnorderedKey] The FINANCE_DB_PASSWORD key should go before the MEMBER_DB_PASSWORD key
(UnorderedKey)
[warning] 15-15: [UnorderedKey] The LETTER_DB_PASSWORD key should go before the MEMBER_DB_PASSWORD key
(UnorderedKey)
[warning] 17-17: [UnorderedKey] The KEYCLOAK_ADMIN_PASSWORD key should go before the LETTER_DB_PASSWORD key
(UnorderedKey)
[warning] 18-18: [UnorderedKey] The KEYCLOAK_DB_PASSWORD key should go before the LETTER_DB_PASSWORD key
(UnorderedKey)
[warning] 19-19: [UnorderedKey] The KEYCLOAK_REALM_ADMIN_PASSWORD key should go before the LETTER_DB_PASSWORD key
(UnorderedKey)
[warning] 20-20: [UnorderedKey] The KEYCLOAK_REALM_USER_PASSWORD key should go before the LETTER_DB_PASSWORD key
(UnorderedKey)
[warning] 21-21: [UnorderedKey] The FORWARD_AUTH_COOKIE_SECRET key should go before the KEYCLOAK_ADMIN_PASSWORD key
(UnorderedKey)
🤖 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 `@infra/.env.example` around lines 9 - 21, Replace the hardcoded sample
credentials in infra/.env.example with non-reusable placeholders, especially
FORWARD_AUTH_COOKIE_SECRET and the password entries. Update the examples to use
blanks or <generate-random> style values and add a brief note that each
developer/environment must supply its own secrets locally. Keep the changes
within the existing environment variable list so the file remains an example
only.
Source: Linters/SAST tools
| {{- $rendered := .Files.Get "files/realm-config.json" | replace "__KEYCLOAK_REALM_ADMIN_PASSWORD__" .Values.keycloak.users.admin.password | replace "__KEYCLOAK_REALM_USER_PASSWORD__" .Values.keycloak.users.user.password }} | ||
| {{ $rendered | nindent 4 | trim }} |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify all consumers of keycloak-realm-config so the volume source can be changed consistently.
rg -n -C 3 'keycloak-realm-config|realm-config\.json|configMap:|secret:' infra/helm/team-devoopsRepository: AET-DevOps26/team-devoops
Length of output: 12356
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== keycloak-realm-configmap.yaml ==\n'
sed -n '1,80p' infra/helm/team-devoops/templates/keycloak-realm-configmap.yaml
printf '\n== keycloak-deployment.yaml (realm-config volume) ==\n'
sed -n '100,125p' infra/helm/team-devoops/templates/keycloak-deployment.yaml
printf '\n== search for Secret-backed realm config ==\n'
rg -n 'keycloak-realm-config|realm-config|kind: Secret|secret:' infra/helm/team-devoops/templates infra/helm/team-devoops/values.yamlRepository: AET-DevOps26/team-devoops
Length of output: 3692
Move the realm config payload out of the ConfigMap
This template renders the realm user passwords directly into keycloak-realm-config, exposing secret material in a ConfigMap. Store the rendered JSON in a Secret instead and update the Keycloak volume mount; the direct replacements also risk invalid JSON for passwords containing quotes, backslashes, or newlines.
🤖 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 `@infra/helm/team-devoops/templates/keycloak-realm-configmap.yaml` around lines
10 - 11, The Keycloak realm payload is being rendered in the ConfigMap via the
keycloak-realm-configmap template, which exposes admin/user passwords and can
break JSON when values contain special characters. Move the rendered realm JSON
generation out of the ConfigMap into a Secret, and update the Keycloak
mount/reference to read from that Secret instead. Use the existing
keycloak-realm-config template and the realm-config.json rendering path as the
main locations to adjust, keeping the ConfigMap free of secret material.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
infra/helm/team-devoops/values.yaml (1)
302-302: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
KEYCLOAK_ADMIN_CLIENT_SECRETis still hardcoded.Line 302 defaults
KEYCLOAK_ADMIN_CLIENT_SECRETto"org-role-sync-secret". The comment on line 298 says to override via--setin CI/prod, but the default value is a real secret string, not aCHANGE_MEplaceholder like the other externalized secrets in this PR. If the chart is deployed without the override, this secret ships to production.🔒 Proposed fix
KEYCLOAK_ADMIN_CLIENT_ID: "org-role-sync" - KEYCLOAK_ADMIN_CLIENT_SECRET: "org-role-sync-secret" + KEYCLOAK_ADMIN_CLIENT_SECRET: "CHANGE_ME_KEYCLOAK_ADMIN_CLIENT_SECRET" KEYCLOAK_ROLES_CLIENT: "devops-client"🤖 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 `@infra/helm/team-devoops/values.yaml` at line 302, The default for KEYCLOAK_ADMIN_CLIENT_SECRET in values.yaml is still a real secret string, so replace it with a non-sensitive placeholder like the other externalized secrets and keep the actual value supplied only through override mechanisms. Update the chart values entry near the KEYCLOAK_ADMIN_CLIENT_SECRET setting to use a CHANGE_ME-style default, and ensure any consuming templates or docs still point to the override path for CI/prod. Reference the KEYCLOAK_ADMIN_CLIENT_SECRET value in the team-devoops Helm values so it is easy to locate.
🧹 Nitpick comments (1)
infra/helm/team-devoops/templates/_helpers.tpl (1)
19-39: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winDrop all capabilities by default in
team-devoops.containerSecurityContext
infra/helm/team-devoops/templates/_helpers.tpl:19-39—drop: ["NET_RAW"]still leaves other Linux capabilities available. Switch todrop: ["ALL"]and add back only the capabilities each workload needs.🤖 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 `@infra/helm/team-devoops/templates/_helpers.tpl` around lines 19 - 39, The default container security context in team-devoops.containerSecurityContext only drops NET_RAW, which still leaves other Linux capabilities enabled. Update the capabilities block to drop all capabilities by default, then explicitly add back only the minimum required ones for any workload that needs them, keeping the existing allowPrivilegeEscalation, runAsNonRoot, and runAsUser behavior intact.
🤖 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 `@infra/helm/team-devoops/values.yaml`:
- Line 249: forwardAuth.clientSecret is still hardcoded in the chart values and
should be externalized like cookieSecret. Update the forwardAuth section in
values.yaml to replace the literal traefik-forward-auth-secret with a
CHANGE_ME-style placeholder or configurable value, keeping the existing
cookieSecret pattern as the reference and ensuring no secret remains embedded in
the default values.
---
Outside diff comments:
In `@infra/helm/team-devoops/values.yaml`:
- Line 302: The default for KEYCLOAK_ADMIN_CLIENT_SECRET in values.yaml is still
a real secret string, so replace it with a non-sensitive placeholder like the
other externalized secrets and keep the actual value supplied only through
override mechanisms. Update the chart values entry near the
KEYCLOAK_ADMIN_CLIENT_SECRET setting to use a CHANGE_ME-style default, and
ensure any consuming templates or docs still point to the override path for
CI/prod. Reference the KEYCLOAK_ADMIN_CLIENT_SECRET value in the team-devoops
Helm values so it is easy to locate.
---
Nitpick comments:
In `@infra/helm/team-devoops/templates/_helpers.tpl`:
- Around line 19-39: The default container security context in
team-devoops.containerSecurityContext only drops NET_RAW, which still leaves
other Linux capabilities enabled. Update the capabilities block to drop all
capabilities by default, then explicitly add back only the minimum required ones
for any workload that needs them, keeping the existing allowPrivilegeEscalation,
runAsNonRoot, and runAsUser behavior intact.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 77539e18-0753-4862-b619-32acff2c8656
📒 Files selected for processing (13)
infra/helm/team-devoops/templates/_helpers.tplinfra/helm/team-devoops/templates/alloy-deployment.yamlinfra/helm/team-devoops/templates/deployment.yamlinfra/helm/team-devoops/templates/forward-auth.yamlinfra/helm/team-devoops/templates/grafana-deployment.yamlinfra/helm/team-devoops/templates/keycloak-deployment.yamlinfra/helm/team-devoops/templates/keycloak-postgres-statefulset.yamlinfra/helm/team-devoops/templates/loki-deployment.yamlinfra/helm/team-devoops/templates/ollama-deployment.yamlinfra/helm/team-devoops/templates/postgres-statefulset.yamlinfra/helm/team-devoops/templates/prometheus-deployment.yamlinfra/helm/team-devoops/values.yamlservices/spring-letter/.env.example
f-s-h
added
enhancement
openssl-rand-base64 secrets routinely contain `/`, which breaks a `/`-delimited sed s/// (sed has no way to escape the delimiter). With `set -e` in the entrypoint, that crash-loops the keycloak container before it ever starts. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
org-role-sync-secret, traefik-forward-auth-secret, and grafana-oauth-secret
were hardcoded in realm-config.json, values.yaml, docker-compose.yml
fallback defaults, and directly in spring-organization's
application.properties -- never actually connected to a GitHub secret in
either deploy target, despite values.yaml comments claiming they would be.
org-role-sync in particular carries Keycloak manage-users, making it the
most sensitive credential left unaddressed.
- realm-config.json (both copies): hardcoded client secrets -> placeholders,
substituted the same way the two realm passwords already are (Compose sed,
Helm configmap replace)
- values.yaml: CHANGE_ME placeholders for forwardAuth.clientSecret,
monitoring.grafana.oauthClientSecret, and the org-role-sync secret in
organization-service's env; added the same to member-service, which
previously had no override path at all and silently ran on its Spring
property's hardcoded fallback in every deploy target
- spring-organization: client-secret now reads from
KEYCLOAK_ADMIN_CLIENT_SECRET env (was a bare literal, no override
possible without a rebuild)
- docker-compose.yml/.override.yml: dropped the ${VAR:-hardcoded} fallbacks
for the two OAuth client secrets so a missing .env value fails loudly
instead of silently deploying the public default
- cd.yml: 3 new GH secrets threaded through both the VM .env and the Helm
--set list (KEYCLOAK_ADMIN_CLIENT_SECRET feeds two --set targets, since
organization-service and member-service both authenticate as the same
org-role-sync client)
- .env.example: local-dev defaults for the 3 new vars (matching values,
not real secrets)
- README: documented the 3 new secrets and the org-role-sync client
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…code-secrets # Conflicts: # infra/helm/team-devoops/values.yaml
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
infra/helm/team-devoops/values.yaml (1)
24-54: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftReject
CHANGE_ME_*defaults at render time. These values are written straight intoSecrets and Keycloak config; if a deploy misses--set, the chart will roll out known credentials. Addrequired/failguards or a schema that blocks placeholder and empty secret values.🤖 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 `@infra/helm/team-devoops/values.yaml` around lines 24 - 54, The placeholder credentials in the database values are currently allowed through and can be rendered into Secrets and Keycloak config. Add render-time validation in the Helm chart, using the values handling around the database secret fields in values.yaml plus the template/helpers that consume them, so deployment fails if any password is empty or still set to a CHANGE_ME_* placeholder. Prefer required/fail checks or a values schema guard that covers the top-level password and each entry under users..github/workflows/cd.yml (1)
295-311: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAvoid inlining secrets in the Helm command In the Helm upgrade step,
${{ secrets.* }}is substituted into the script before Bash runs, so shell metacharacters in a secret can be reinterpreted.helm --setalso treats commas and some special characters as syntax. Pass these values viaenv:and a temp values file, or--set-file, instead.🤖 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 @.github/workflows/cd.yml around lines 295 - 311, The Helm upgrade step is inlining multiple secret values directly into the command, which can break on shell metacharacters and Helm --set parsing. Update the CD workflow command to stop referencing `${{ secrets.* }}` inline and instead pass these values through `env:` plus a temporary values file or `--set-file`. Focus on the Helm invocation in the deployment step and keep the existing secret keys and service settings mapped through safer inputs.
🤖 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 `@infra/docker-compose.yml`:
- Line 40: The Compose secret references are using plain variable expansion,
which can silently default to empty values when env vars are missing. Update the
secret-related entries in the docker-compose file to use required-variable
syntax like ${VAR:?VAR is required}, focusing on the existing
SPRING_DATASOURCE_PASSWORD reference and any similar secret refs in this compose
config.
In `@services/spring-member/src/main/resources/application.properties`:
- Line 22: The Keycloak service-account secret is still configured with a dummy
fallback, so make it a required setting instead of defaulting to
CHANGE_ME_KEYCLOAK_ADMIN_CLIENT_SECRET. Update the property in
application.properties to remove the placeholder default and ensure the related
Helm defaults use a real required value path as well, so missing configuration
fails fast before any Keycloak calls in the service-account setup.
---
Outside diff comments:
In @.github/workflows/cd.yml:
- Around line 295-311: The Helm upgrade step is inlining multiple secret values
directly into the command, which can break on shell metacharacters and Helm
--set parsing. Update the CD workflow command to stop referencing `${{ secrets.*
}}` inline and instead pass these values through `env:` plus a temporary values
file or `--set-file`. Focus on the Helm invocation in the deployment step and
keep the existing secret keys and service settings mapped through safer inputs.
In `@infra/helm/team-devoops/values.yaml`:
- Around line 24-54: The placeholder credentials in the database values are
currently allowed through and can be rendered into Secrets and Keycloak config.
Add render-time validation in the Helm chart, using the values handling around
the database secret fields in values.yaml plus the template/helpers that consume
them, so deployment fails if any password is empty or still set to a CHANGE_ME_*
placeholder. Prefer required/fail checks or a values schema guard that covers
the top-level password and each entry under users.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5ab32a59-e3c8-46d9-ac9a-f50c9aecbdbb
📒 Files selected for processing (12)
.github/workflows/cd.ymlREADME.mdinfra/.env.exampleinfra/docker-compose.override.ymlinfra/docker-compose.ymlinfra/helm/team-devoops/files/realm-config.jsoninfra/helm/team-devoops/templates/deployment.yamlinfra/helm/team-devoops/templates/keycloak-realm-configmap.yamlinfra/helm/team-devoops/values.yamlinfra/keycloak/realm-config.jsonservices/spring-member/src/main/resources/application.propertiesservices/spring-organization/src/main/resources/application.properties
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- infra/docker-compose.override.yml
- infra/helm/team-devoops/templates/deployment.yaml
| - SPRING_DATASOURCE_URL=jdbc:postgresql://app-database:5432/app_db | ||
| - SPRING_DATASOURCE_USERNAME=reports_user | ||
| - SPRING_DATASOURCE_PASSWORD=reports_password | ||
| - SPRING_DATASOURCE_PASSWORD=${REPORTS_DB_PASSWORD} |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files infra/docker-compose.yml
wc -l infra/docker-compose.yml
sed -n '1,260p' infra/docker-compose.yml
printf '\n--- search ---\n'
rg -n '\$\{[A-Z0-9_]+(:[^}]*)?\}' infra/docker-compose.ymlRepository: AET-DevOps26/team-devoops
Length of output: 12798
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,240p' infra/docker-compose.yml | nl -ba | sed -n '1,240p'
printf '\n--- env refs ---\n'
rg -n '\$\{REPORTS_DB_PASSWORD|\$\{[^}]+\}' infra/docker-compose.ymlRepository: AET-DevOps26/team-devoops
Length of output: 203
Make secret Compose vars required Plain ${VAR} falls back to empty when a deploy env is missing one, which can start services with empty DB credentials or client secrets. Use ${VAR:?VAR is required} for the secret refs in this compose file.
🤖 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 `@infra/docker-compose.yml` at line 40, The Compose secret references are using
plain variable expansion, which can silently default to empty values when env
vars are missing. Update the secret-related entries in the docker-compose file
to use required-variable syntax like ${VAR:?VAR is required}, focusing on the
existing SPRING_DATASOURCE_PASSWORD reference and any similar secret refs in
this compose config.
| keycloak.realm=devops | ||
| keycloak.service-account.client-id=${KEYCLOAK_SERVICE_ACCOUNT_CLIENT_ID:org-role-sync} | ||
| keycloak.service-account.client-secret=${KEYCLOAK_SERVICE_ACCOUNT_CLIENT_SECRET:org-role-sync-secret} | ||
| keycloak.service-account.client-secret=${KEYCLOAK_SERVICE_ACCOUNT_CLIENT_SECRET:CHANGE_ME_KEYCLOAK_ADMIN_CLIENT_SECRET} |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target file and nearby configuration references
git ls-files services/spring-member/src/main/resources/application.properties
echo '---'
cat -n services/spring-member/src/main/resources/application.properties | sed -n '1,120p'
echo '--- SEARCH keycloak.service-account / CHANGE_ME in repo ---'
rg -n "keycloak\.service-account\.client-secret|CHANGE_ME_KEYCLOAK_ADMIN_CLIENT_SECRET|KEYCLOAK_SERVICE_ACCOUNT_CLIENT_SECRET" .Repository: AET-DevOps26/team-devoops
Length of output: 2940
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- organization application.properties ---'
cat -n services/spring-organization/src/main/resources/application.properties | sed -n '1,120p'
echo '--- member KeycloakService ---'
cat -n services/spring-member/src/main/java/tum/devoops/memberservice/service/KeycloakService.java | sed -n '1,140p'
echo '--- helm values around relevant env vars ---'
sed -n '300,360p' infra/helm/team-devoops/values.yaml
echo '--- README excerpt around secret docs ---'
sed -n '190,220p' README.mdRepository: AET-DevOps26/team-devoops
Length of output: 15807
Remove the placeholder default for the service-account secret.
application.properties still falls back to CHANGE_ME_KEYCLOAK_ADMIN_CLIENT_SECRET, and the Helm defaults use the same placeholder. Make this a required value so missing config fails fast instead of letting Keycloak calls run with a dummy secret.
🤖 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 `@services/spring-member/src/main/resources/application.properties` at line 22,
The Keycloak service-account secret is still configured with a dummy fallback,
so make it a required setting instead of defaulting to
CHANGE_ME_KEYCLOAK_ADMIN_CLIENT_SECRET. Update the property in
application.properties to remove the placeholder default and ensure the related
Helm defaults use a real required value path as well, so missing configuration
fails fast before any Keycloak calls in the service-account setup.
The namespace's ResourceQuota was at 6040Mi/6144Mi (98%) and 3560m/4000m
(89%) even at rest -- no room for the old+new pod overlap of a
multi-service rolling deploy, which is what caused ollama's replacement
pod to be flatly rejected ("exceeded quota") during the last cd run,
failing the whole release.
organization/member/event/feedback/finance/letter-service were the one
category in this chart still on the generic untuned default (384Mi/200m)
-- everywhere else has already been trimmed to just above observed peak.
Live `kubectl top` shows ~271-286Mi steady-state per service; their JVM
heap is already capped at 300Mi via JAVA_TOOL_OPTIONS, so 350Mi leaves a
reasonable non-heap margin without idle-vs-peak risk. CPU cut harder
(200m->150m) since it's compressible, not OOM-prone, and live usage is
~2-3m. app-database gets a smaller trim (256Mi->224Mi) given its 146Mi of
observed headroom despite serving 7 services' connection pools.
Frees ~236Mi/330m: quota headroom goes from 104Mi/440m to 340Mi/770m.
Reduces but does not guarantee against the quota-exceeded failure mode
under a full simultaneous rollout -- verified via `helm template` that
totals now sum to 5804Mi/3230m against the 6144Mi/4000m hard limits.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
organization/member/event/feedback/finance/letter-service, py-genai-helper (image USER "user") and prometheus (image USER "nobody") were all stuck in CreateContainerConfigError -- "container has runAsNonRoot and image has non-numeric user, cannot verify user is non-root". The kubelet refuses to resolve a named USER against the image's /etc/passwd; it only accepts a numeric UID (or an explicit runAsUser in the pod spec). Same tradeoff already applied to postgres/ollama/alloy elsewhere in this chart: the images already run as non-root by default, we just can't assert it to the kubelet without either this or pinning a numeric runAsUser. allowPrivilegeEscalation:false and the NET_RAW capability drop are unaffected. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
On a fresh database, init_db() races against member-service and organization-service creating member.members / organization.teams, which its own tables have foreign keys into. Losing that race raised immediately, crashing the process -- recovery then depended on the container's restart backoff, which is exponential and can push the next attempt out far enough to exceed a deploy's overall timeout even though the underlying dependency clears within a couple of minutes. Retry the table creation internally instead (2 minute budget, within the startup probe's ~5.3 minute allowance) so losing the race costs a slower first boot rather than a container restart. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
services/py-genai-helper/db.py (1)
77-100: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a retry log in
init_db()
The loop can stay silent for up to 2 minutes on cold starts. Add an info log beforetime.sleep(2)so operators can see the retry cadence and remaining time; this file doesn’t importloggingyet, so add that too.🤖 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 `@services/py-genai-helper/db.py` around lines 77 - 100, `init_db()` retries silently for up to 2 minutes, so add observability to the retry loop by logging each `ProgrammingError` retry before `time.sleep(2)`, including the retry cadence and remaining time. Update `db.py` to import and use `logging` (or the module’s logger) and keep the log inside the `except ProgrammingError as e` branch, while preserving the existing `UndefinedTable`/deadline behavior around `_create_report_tables()`.infra/helm/team-devoops/values.yaml (1)
331-337: 🔒 Security & Privacy | 🔵 Trivial | ⚖️ Poor tradeoff
runAsNonRoot: falseremoves the non-root guardrail; prefer pinning a numericrunAsUser.The comment correctly diagnoses why the kubelet can't verify a named image user, but disabling the assertion means the workload would silently start even if a future image change made it run as root, and it breaks Pod Security "restricted" compliance. The documented alternative — an explicit numeric
runAsUser(matching each image'sadduserUID, e.g. 1000 as already used for py-genai-helper'sfsGroup) — keeps the guarantee without the name-resolution failure. Consider applying that instead across these services.Also applies to: 364-364, 389-389, 406-406, 423-423, 440-440, 458-458, 534-534
🤖 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 `@infra/helm/team-devoops/values.yaml` around lines 331 - 337, The securityContext settings are disabling the non-root check with runAsNonRoot: false, which removes the guardrail; instead, update the affected service entries in values.yaml to use an explicit numeric runAsUser that matches each image’s adduser UID (as already done for py-genai-helper’s fsGroup, e.g. 1000) while keeping runAsNonRoot enabled. Use the repeated service blocks in the values file to apply the same numeric UID consistently across all listed services so kubelet verification and Pod Security restricted compliance are preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@infra/helm/team-devoops/values.yaml`:
- Around line 331-337: The securityContext settings are disabling the non-root
check with runAsNonRoot: false, which removes the guardrail; instead, update the
affected service entries in values.yaml to use an explicit numeric runAsUser
that matches each image’s adduser UID (as already done for py-genai-helper’s
fsGroup, e.g. 1000) while keeping runAsNonRoot enabled. Use the repeated service
blocks in the values file to apply the same numeric UID consistently across all
listed services so kubelet verification and Pod Security restricted compliance
are preserved.
In `@services/py-genai-helper/db.py`:
- Around line 77-100: `init_db()` retries silently for up to 2 minutes, so add
observability to the retry loop by logging each `ProgrammingError` retry before
`time.sleep(2)`, including the retry cadence and remaining time. Update `db.py`
to import and use `logging` (or the module’s logger) and keep the log inside the
`except ProgrammingError as e` branch, while preserving the existing
`UndefinedTable`/deadline behavior around `_create_report_tables()`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3110a106-c242-4bb8-bf94-6d5f4aacfde1
📒 Files selected for processing (3)
infra/helm/team-devoops/templates/prometheus-deployment.yamlinfra/helm/team-devoops/values.yamlservices/py-genai-helper/db.py
raphael-frank
left a comment
raphael-frank
left a comment
There was a problem hiding this comment.
I moved some remaining secrets into GH secrets and fixed deployment.
3 participants
Removed hardcoded secrets and created a .env file for local development and github secrets for deployment.
Closes #120
Summary by CodeRabbit