Hardening (Security improvements)#302
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enhances security across the GeoNode Helm chart by configuring non-root execution, dropping capabilities, and setting seccomp profiles for various components. It also migrates from a single shared persistent volume to four distinct PVCs, updates Nginx to run unprivileged on port 8080 with a read-only root filesystem, and configures Celery to log to stdout/stderr. Feedback highlights a potential permission conflict on the shared statics volume due to mismatched fsGroup values, resource waste from using a single hardcoded storage size for all split PVCs, the need to conditionally render storageClassName, and cleanup of unused favicon values.
|
@jwkaltz I appreciate your PR. But we will need an upgrade path, as you changes to the PVC setup seems like a breaking change to me on first view. |
Good point, we will at least need some migration notes (and a new release). |
|
I would like to do a major version for this changes along with the introduction of gateway api. |
|
/gemini review I have addressed the previous review comments. Please review the latest revision of this pull request. |
There was a problem hiding this comment.
Code Review
This pull request introduces significant security and storage enhancements to the GeoNode Helm chart. Key changes include splitting the single shared persistent volume into separate dedicated PVCs for statics, geoserver data, backup/restore, and data directories, as well as applying strict security contexts (non-root execution, dropped capabilities, and read-only root filesystems) across all components. Additionally, Nginx is migrated to an unprivileged image running on port 8080 with temporary writable scratch directories. Review feedback highlights critical improvements: wrapping octal file modes (e.g., 0555) in double quotes to prevent YAML 1.2 parsing errors, defaulting Nginx's runAsUser to 101 to avoid template rendering issues, and adding fsGroupChangePolicy: OnRootMismatch to the GeoNode deployment to prevent slow pod startup times when mounting large volumes.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
/gemini review I have addressed the previous review comments, and integrated your suggestions, except changing defaultMode to a string, which does not work. Please review the latest revision of this pull request. |
There was a problem hiding this comment.
Code Review
This pull request introduces security context configurations across various GeoNode components to support running containers as non-root users, updates Nginx to an unprivileged image listening on port 8080, and splits the single persistent volume claim into separate PVCs for statics, GeoServer data, backup/restore, and general data. Feedback on these changes focuses on preventing potential Helm rendering errors by safely defaulting the newly introduced component-specific securityContext values to empty dictionaries before accessing nested fields. Additionally, it is recommended to use the -D flag with the install command in the statics job to ensure target directories are created automatically.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
/gemini review I have added refactoring in order to be able to run the standard geonode container as non-root, and also introduced a temporary fix in order to be able to run celery as non-root. Please review the latest revision of this pull request. |
There was a problem hiding this comment.
Code Review
This pull request introduces significant security and architectural improvements to the GeoNode Helm chart, including running containers as non-root, configuring Nginx as unprivileged on port 8080, splitting the single PVC into dedicated PVCs for different storage needs, and implementing a Python settings wrapper to handle configuration overrides. Feedback on these changes highlights a copy-paste typo in the Celery temporary fix script, potential nil pointer evaluation errors across multiple templates if nested keys under persistant or component securityContext are omitted in custom values, and a recommendation to pass globals() to exec() in the settings wrapper.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
/gemini review I have addressed the previous review comments. Please review the latest revision of this pull request. |
There was a problem hiding this comment.
Code Review
This pull request enhances security by configuring containers to run as non-root, updates Nginx to run unprivileged on port 8080, splits the single persistent volume claim into dedicated PVCs, and introduces a settings wrapper. Feedback focuses on preventing Helm rendering errors by safely defaulting nested security context and persistence values. Other recommendations include correctly formatting the multi-line favicon ConfigMap, quoting arguments in bash scripts, safely handling empty PID files, using standard Celery stdout logging, and replacing exec() with dynamic imports in Python.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
/gemini review I have addressed the previous review comments. Please review the latest revision of this pull request. |
There was a problem hiding this comment.
Code Review
This pull request enhances the security posture of the GeoNode Helm chart by implementing non-root execution, dropping capabilities, and configuring seccomp profiles across various components. It also splits the monolithic persistent volume claim into dedicated PVCs for statics, GeoServer data, backup/restore, and general data, and transitions Nginx to an unprivileged container running on port 8080. The review feedback highlights several critical robustness improvements, including quoting shell variables in entrypoint scripts, safely handling potentially nil Helm values using the default dict helper in cleanup jobs, replacing fragile ast.literal_eval parsing for boolean environment variables, and ensuring process IDs are non-empty before signaling them in the Celery startup script.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
/gemini review I have addressed the previous review comments. Please review the latest revision of this pull request. |
There was a problem hiding this comment.
Code Review
This pull request introduces significant security hardening and configuration improvements to the GeoNode Helm chart, including running containers as non-root, updating Nginx to an unprivileged image listening on port 8080, splitting the persistent volume claim into dedicated PVCs, and refactoring settings overrides. Feedback on these changes highlights a typo in the Celery temporary fix script where CELERY__HARVESTER_MAX_MEMORY_PER_CHILD incorrectly defaults to the standard worker's memory limit. Additionally, reviewers recommend making runAsNonRoot configurable for GeoServer to prevent deployment failures with standard images, and omitting the -f flag in Celery when logging to /dev/stdout to avoid permission issues in non-root environments.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
jwkaltz
changed the title
|
@mwallschlaeger Note that, unfortunately, one custom image is still needed, I have documented that and will track progress upstream so we can get rid of that one day. I will do some more integration and functional tests in the next few weeks, if I find more issues or improvements ideas, I will add them. |
|
@jwkaltz I haven't checked your latest changes. That said first i will give this version a try in the upcoming week. Also we will need default As we are building our geonode ourself, too. I will evaluate your recommendations on this, too. Further more we need an update path for updating from chart version 1.3.3 to version 2.0.0 (incl. hardening). Furthermore we will add support for gateway API to this repository as a last feature of 1.x chart version next week. |
I changed the default now so that the default geonode/geoserver image can be used. (The default geonode/geonode image can already be used with hardened settings, because we have an included patch for the celery-cmd script, this is the only issue I have found so far).
I have added a note regarding update path, it is not much but I am not sure what further information would be useful. |
|
@jwkaltz I managed to do my first tests on your PR. What I did:
Could deploy the chart without problems. GeoNode was working properly on first glance.
Image was not starting properly as it has missing permission. This change was applied to existing deployment.
Geoserver still has missing permission as the files were created with root user permission before.
In general thinks look quite ready to me, but are missing more documentation regarding updates. I will to do some more tests on:
|
Zalfsten
left a comment
Zalfsten
left a comment
There was a problem hiding this comment.
At first glance, everything looks fine
Only I for myself came to the conclusion that UID/GID 1000 is not the best idea.
…uld use the default
…rent standard image)
|
@jwkaltz i will take myself some time this week to merge this PR. I think we are close. Only documentation on the migration path from 1.3.x to hardening coming with release 2.0.0 is missing. |
3 participants
Description
Hardening (security improvement) as described in #301.
Current status:
Type of Change
Please select the relevant option:
Related Issue
closes #301