Fix etc permissions #9352
Merged
Fix etc permissions #9352
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fix etc permissions Signed-off-by: liranmauda <[email protected]>
📝 WalkthroughWalkthroughThe setup script modifies permission configurations to replace broad group-owned permissions with more restrictive group-writable permissions, removes setuid practices for rsyslogd and crond, and adds explicit directory creation for runtime components to improve OpenShift compatibility. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
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: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/deploy/NVA_build/setup_platform.sh
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: Neon-White
Repo: noobaa/noobaa-core PR: 9196
File: tools/ibm_runner_helpers/run_containerized_warp_on_cloud_runner.sh:3-3
Timestamp: 2025-09-18T07:11:55.053Z
Learning: In the NooBaa project's IBM Cloud Warp testing infrastructure, `/etc/warp.env` is created by cloud-init during VM provisioning with permissions 0600 (owner read/write only). This file contains environment variables and secrets passed from GitHub Actions. The file creation is part of a controlled internal flow with proper permissions set automatically by cloud-init, so additional permission checks before sourcing are not necessary in this context.
Learnt from: Neon-White
Repo: noobaa/noobaa-core PR: 9196
File: tools/ibm_runner_helpers/run_containerized_warp_on_cloud_runner.sh:3-3
Timestamp: 2025-09-18T07:11:55.053Z
Learning: In the NooBaa project's IBM Cloud Warp testing infrastructure, `/etc/warp.env` is created by cloud-init during VM provisioning with mode 0600 (owner read/write only). This file contains environment variables and secrets passed from GitHub Actions. The file creation is part of a controlled internal flow with proper permissions set automatically, so additional permission checks before sourcing are not necessary in this context.
📚 Learning: 2025-09-18T07:11:55.053Z
Learnt from: Neon-White
Repo: noobaa/noobaa-core PR: 9196
File: tools/ibm_runner_helpers/run_containerized_warp_on_cloud_runner.sh:3-3
Timestamp: 2025-09-18T07:11:55.053Z
Learning: In the NooBaa project's IBM Cloud Warp testing infrastructure, `/etc/warp.env` is created by cloud-init during VM provisioning with permissions 0600 (owner read/write only). This file contains environment variables and secrets passed from GitHub Actions. The file creation is part of a controlled internal flow with proper permissions set automatically by cloud-init, so additional permission checks before sourcing are not necessary in this context.
Applied to files:
src/deploy/NVA_build/setup_platform.sh
📚 Learning: 2025-09-18T07:11:55.053Z
Learnt from: Neon-White
Repo: noobaa/noobaa-core PR: 9196
File: tools/ibm_runner_helpers/run_containerized_warp_on_cloud_runner.sh:3-3
Timestamp: 2025-09-18T07:11:55.053Z
Learning: In the NooBaa project's IBM Cloud Warp testing infrastructure, `/etc/warp.env` is created by cloud-init during VM provisioning with mode 0600 (owner read/write only). This file contains environment variables and secrets passed from GitHub Actions. The file creation is part of a controlled internal flow with proper permissions set automatically, so additional permission checks before sourcing are not necessary in this context.
Applied to files:
src/deploy/NVA_build/setup_platform.sh
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Noobaa Image
- GitHub Check: run-jest-unit-tests
- GitHub Check: run-package-lock-validation
🔇 Additional comments (3)
src/deploy/NVA_build/setup_platform.sh (3)
96-97: LGTM: More explicit permission model.Changing from
g=utog+rwXis clearer and avoids copying special bits (setuid/setgid) to the group. TheXflag correctly adds execute permission only for directories and already-executable files.
99-102: LGTM: Appropriate read-only permissions for binaries.Setting supervisord binaries to read+execute only (
g+rX) is the correct approach for executable files that should not be modified by the group.
104-106: LGTM: Proper runtime directory setup.Creating
/var/log/supervisorbefore setting permissions is correct, and the group-writable permissions (g+rwX) are necessary for supervisor to write sockets, PID files, and logs.
Comment on lines
+108
to
+118
| # node modules (needs write) | ||
| chgrp -R 0 /root/node_modules && chmod -R g+rwX /root/node_modules | ||
|
|
||
| # give access for logrotate | ||
| chgrp -R 0 /var/lib/logrotate && chmod -R g=u /var/lib/logrotate | ||
| # data volumes | ||
| chgrp -R 0 /data /log && chmod -R g+rwX /data /log | ||
|
|
||
| # setuid for rsyslog so it can run as root | ||
| chmod u+s /sbin/rsyslogd | ||
|
|
||
| # setuid for crond so it can run as root | ||
| chmod u+s /sbin/crond | ||
| # /etc — READ ONLY | ||
| chgrp -R 0 /etc && chmod -R g+rX /etc | ||
|
|
||
| # logrotate state (needs write) | ||
| chgrp -R 0 /var/lib/logrotate && chmod -R g+rwX /var/lib/logrotate |
There was a problem hiding this comment.
Ensure directories exist before setting permissions.
Lines 112 and 118 set permissions on /data, /log, and /var/lib/logrotate without first ensuring they exist. If these directories are missing, the chmod commands will fail and the script will exit due to set -e (line 123).
🔎 Proposed fix to create directories before setting permissions
# node modules (needs write)
chgrp -R 0 /root/node_modules && chmod -R g+rwX /root/node_modules
# data volumes
+mkdir -p /data /log
chgrp -R 0 /data /log && chmod -R g+rwX /data /log
# /etc — READ ONLY
chgrp -R 0 /etc && chmod -R g+rX /etc
# logrotate state (needs write)
+mkdir -p /var/lib/logrotate
chgrp -R 0 /var/lib/logrotate && chmod -R g+rwX /var/lib/logrotate📝 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.
Suggested change
| # node modules (needs write) | |
| chgrp -R 0 /root/node_modules && chmod -R g+rwX /root/node_modules | |
| # give access for logrotate | |
| chgrp -R 0 /var/lib/logrotate && chmod -R g=u /var/lib/logrotate | |
| # data volumes | |
| chgrp -R 0 /data /log && chmod -R g+rwX /data /log | |
| # setuid for rsyslog so it can run as root | |
| chmod u+s /sbin/rsyslogd | |
| # setuid for crond so it can run as root | |
| chmod u+s /sbin/crond | |
| # /etc — READ ONLY | |
| chgrp -R 0 /etc && chmod -R g+rX /etc | |
| # logrotate state (needs write) | |
| chgrp -R 0 /var/lib/logrotate && chmod -R g+rwX /var/lib/logrotate | |
| # node modules (needs write) | |
| chgrp -R 0 /root/node_modules && chmod -R g+rwX /root/node_modules | |
| # data volumes | |
| mkdir -p /data /log | |
| chgrp -R 0 /data /log && chmod -R g+rwX /data /log | |
| # /etc — READ ONLY | |
| chgrp -R 0 /etc && chmod -R g+rX /etc | |
| # logrotate state (needs write) | |
| mkdir -p /var/lib/logrotate | |
| chgrp -R 0 /var/lib/logrotate && chmod -R g+rwX /var/lib/logrotate |
🤖 Prompt for AI Agents
In src/deploy/NVA_build/setup_platform.sh around lines 108 to 118, the script
changes group/permissions on /data, /log, and /var/lib/logrotate without
ensuring those directories exist, causing chmod/chgrp to fail under set -e;
before each chgrp/chmod pair (or once before the groups of related changes)
create the directories with mkdir -p (and set correct ownership if needed) so
the subsequent chgrp -R and chmod -R commands succeed even when directories are
absent.
jackyalbo
approved these changes
Dec 31, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Explain the Changes
Issues: Fixed #xxx / Gap #xxx
Summary by CodeRabbit
Release Notes
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.