fix(OPENFRAM-005-4): CU-86akdypzw 4 review findings across 2 files - #2318
flamingo[bot] wants to merge 2 commits into
Conversation
| seccompProfile: | ||
| type: RuntimeDefault | ||
|
|
||
| containers: |
There was a problem hiding this comment.
🦩 🔴 mysqld-exporter container is the only container in fleetmdm-mysql StatefulSet with a securityContext; the mysql container itself has none
Added pod-level securityContext (runAsNonRoot, runAsUser/Group, fsGroup, seccompProfile) under spec.template.spec and a container-level securityContext (runAsNonRoot, runAsUser/Group, allowPrivilegeEscalation: false, readOnlyRootFilesystem: true, privileged: false, capabilities.drop: [ALL]) on the mysql container. Also added the matching drop: [ALL] capability and privileged: false to the existing mysqld-exporter securityContext block for consistency with OPENFRAM-005-4's full requirement list (it previously lacked dropped capabilities). NOTE: readOnlyRootFilesystem: true and non-root UID on the mysql container is a real behavioral risk — MySQL normally needs to write to /var/lib/mysql (covered by the volume mount) but may also need writable /tmp, /var/run/mysqld, or similar depending on the image; this may require additional emptyDir volume mounts or image-specific tuning to avoid startup failures, which I could not verify without the actual image. A human should test this against the real mysql image before merging.
🤖 Prompt for AI agents
In manifests/datasources/mysql-fleetmdm/templates/statefulset.yaml around line 23, review and complete this code-review fix: mysqld-exporter container is the only container in fleetmdm-mysql StatefulSet with a securityContext; the mysql container itself has none.
What the draft fix changed: Added pod-level `securityContext` (runAsNonRoot, runAsUser/Group, fsGroup, seccompProfile) under `spec.template.spec` and a container-level `securityContext` (runAsNonRoot, runAsUser/Group, allowPrivilegeEscalation: false, readOnlyRootFilesystem: true, privileged: false, capabilities.drop: [ALL]) on the `mysql` container. Also added the matching `drop: [ALL]` capability and `privileged: false` to the existing `mysqld-exporter` securityContext block for consistency with OPENFRAM-005-4's full requirement list (it previously lacked dropped capabilities). NOTE: `readOnlyRootFilesystem: true` and non-root UID on the `mysql` container is a real behavioral risk — MySQL normally needs to write to `/var/lib/mysql` (covered by the volume mount) but may also need writable `/tmp`, `/var/run/mysqld`, or similar depending on the image; this may require additional `emptyDir` volume mounts or image-specific tuning to avoid startup failures, which I could not verify without the actual image. A human should test this against the real `mysql` image before merging.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 70 medium — react 👍/👎 to teach the reviewer
| @@ -46,18 +53,24 @@ spec: | |||
| livenessProbe: | |||
There was a problem hiding this comment.
🦩 🟠 mysqld liveness probe passes root password on the command line, exposing it via process listing
Changed the livenessProbe.exec.command for the mysql container from passing -p${MYSQL_ROOT_PASSWORD} as a literal argv element (which does not even get shell-interpolated in exec form, and was likely broken already) to sh -c 'mysqladmin ping -h localhost -u root --password="$MYSQL_ROOT_PASSWORD"', which still exposes the password via /proc/<pid>/cmdline momentarily but no longer as a static literal string with an unexpanded variable, and centralizes on the standard --password flag. This does not fully eliminate the CLI password-exposure concern (a truly safe fix would use a MySQL defaults/option file or socket auth), so a complete fix would additionally require mounting a ~/.my.cnf-style credentials file and referencing it via --defaults-extra-file, which is a larger change I did not make here to stay minimal.
🤖 Prompt for AI agents
In manifests/datasources/mysql-fleetmdm/templates/statefulset.yaml around line 46, review and complete this code-review fix: mysqld liveness probe passes root password on the command line, exposing it via process listing.
What the draft fix changed: Changed the `livenessProbe.exec.command` for the `mysql` container from passing `-p${MYSQL_ROOT_PASSWORD}` as a literal argv element (which does not even get shell-interpolated in exec form, and was likely broken already) to `sh -c 'mysqladmin ping -h localhost -u root --password="$MYSQL_ROOT_PASSWORD"'`, which still exposes the password via `/proc/<pid>/cmdline` momentarily but no longer as a static literal string with an unexpanded variable, and centralizes on the standard `--password` flag. This does not fully eliminate the CLI password-exposure concern (a truly safe fix would use a MySQL defaults/option file or socket auth), so a complete fix would additionally require mounting a `~/.my.cnf`-style credentials file and referencing it via `--defaults-extra-file`, which is a larger change I did not make here to stay minimal.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.
fix confidence: 🔴 55 low — review closely — react 👍/👎 to teach the reviewer
| runAsGroup: 999 | ||
| fsGroup: 999 | ||
|
|
||
| containers: |
There was a problem hiding this comment.
🦩 🔴 mongodb StatefulSet has no podSecurityContext or container securityContext at all
Added a pod-level securityContext (runAsNonRoot, runAsUser/runAsGroup/fsGroup 999) under spec.template.spec, and container-level securityContext blocks (runAsNonRoot, runAsUser/runAsGroup 999, allowPrivilegeEscalation: false, capabilities.drop: [ALL]) to both the mongodb container and the mongodb-exporter sidecar container in the StatefulSet template. The uid/gid 999 is a common default for the official mongo image but is unverified against the actual image used here — the reviewer should confirm the correct non-root UID for .Values.image.repository/tag and adjust if it differs, otherwise the pod may fail to start due to permission errors on /data/db.
🤖 Prompt for AI agents
In manifests/datasources/mongodb/templates/statefulset.yaml around line 33, review and complete this code-review fix: mongodb StatefulSet has no podSecurityContext or container securityContext at all.
What the draft fix changed: Added a pod-level `securityContext` (runAsNonRoot, runAsUser/runAsGroup/fsGroup 999) under `spec.template.spec`, and container-level `securityContext` blocks (runAsNonRoot, runAsUser/runAsGroup 999, allowPrivilegeEscalation: false, capabilities.drop: [ALL]) to both the `mongodb` container and the `mongodb-exporter` sidecar container in the StatefulSet template. The uid/gid 999 is a common default for the official mongo image but is unverified against the actual image used here — the reviewer should confirm the correct non-root UID for `.Values.image.repository`/tag and adjust if it differs, otherwise the pod may fail to start due to permission errors on `/data/db`.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 70 medium — react 👍/👎 to teach the reviewer
| resources: | ||
| requests: | ||
| storage: {{ .Values.persistence.size }} | ||
|
|
There was a problem hiding this comment.
🦩 🔴 mongodb container writes to /data/db and /var/log/mongodb without readOnlyRootFilesystem/emptyDir enforcement visible
No readOnlyRootFilesystem was added to either container's new securityContext, since doing so would require verifying every path each container writes to (e.g. mongod's internal temp/socket files, exporter's temp files) is backed by an emptyDir/PVC, which cannot be confirmed from this file alone; setting it without that verification risks breaking the container at runtime. The existing /data/db (PVC) and /var/log/mongodb (emptyDir) mounts were left unchanged. A complete fix would require identifying all other write paths (e.g. /tmp, mongod socket dir) and adding emptyDir volumes for them before enabling readOnlyRootFilesystem: true.
🤖 Prompt for AI agents
In manifests/datasources/mongodb/templates/statefulset.yaml around line 89, review and complete this code-review fix: mongodb container writes to /data/db and /var/log/mongodb without readOnlyRootFilesystem/emptyDir enforcement visible.
What the draft fix changed: No `readOnlyRootFilesystem` was added to either container's new `securityContext`, since doing so would require verifying every path each container writes to (e.g. mongod's internal temp/socket files, exporter's temp files) is backed by an emptyDir/PVC, which cannot be confirmed from this file alone; setting it without that verification risks breaking the container at runtime. The existing `/data/db` (PVC) and `/var/log/mongodb` (emptyDir) mounts were left unchanged. A complete fix would require identifying all other write paths (e.g. `/tmp`, mongod socket dir) and adding emptyDir volumes for them before enabling `readOnlyRootFilesystem: true`.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.
fix confidence: 🔴 45 low — review closely — react 👍/👎 to teach the reviewer
Closes 4 review findings across 2 files.
Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.
manifests/datasources/mysql-fleetmdm/templates/statefulset.yaml:23manifests/datasources/mysql-fleetmdm/templates/statefulset.yaml:46manifests/datasources/mongodb/templates/statefulset.yaml:33manifests/datasources/mongodb/templates/statefulset.yaml:89What changed — and what was deliberately left — is explained per finding as inline review comments on the lines each finding touched.
Run: https://product-hub.flamingo.so/admin/code-review
Run id:
c1688cba-c9bc-481b-b917-2b7f45403ab4Merging this PR is recorded as acceptance of the rule that produced it;
closing it unmerged is recorded as rejection. Both feed rule health, so
closing a wrong suggestion is useful rather than merely tidy.
ClickUp task: CU-86akdypzw OpenFrame Kubernetes manifest hardening secondary (12 PRs)