Skip to content

feat: improve c2d docker image security#1302

Open
ndrpp wants to merge 10 commits intomainfrom
feat/update-c2d-docker-permissions
Open

feat: improve c2d docker image security#1302
ndrpp wants to merge 10 commits intomainfrom
feat/update-c2d-docker-permissions

Conversation

@ndrpp
Copy link
Copy Markdown
Member

@ndrpp ndrpp commented Mar 27, 2026

Fixes # .

Changes proposed in this PR:

  • limit number of child process ids the job container can spawn to avoid process flooding
  • use user:group 1000:1000 for job containers instead of root
  • add `no-new-privileges' to security options to avoid escalation
  • make sure non-root user has permissions in job specific directories

@ndrpp
Copy link
Copy Markdown
Member Author

ndrpp commented Mar 30, 2026

/run-security-scan

Copy link
Copy Markdown
Member

@alexcos20 alexcos20 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI automated code review (Gemini 3).

Overall risk: medium

Summary:
This pull request introduces significant security enhancements for C2D Docker containers by running them as a non-root user (UID/GID 1000), limiting PIDs, and enforcing no-new-privileges. It also includes minor refactorings. However, two critical await keywords were removed from asynchronous function calls, which could lead to bugs.

Comments:
• [ERROR][bug] The getKeyManager().getEthAddress() method is asynchronous and returns a Promise. Removing the await keyword here means the nodeAddress variable will be assigned a Promise object instead of the resolved address string. This could lead to unexpected runtime errors or incorrect behavior later in the cleanUpUnknownLocks function. Please re-add await.
• [WARNING][security] Setting directory permissions to 0o777 (read, write, execute for all users) is very permissive on the host system. While this might be necessary to allow the non-root container user to write to the volume, it introduces a potential security risk if the host machine has other processes or users that shouldn't have full access to these job-specific directories.

Consider if a more restricted permission set (e.g., 0o770 or 0o750 if the UID/GID matches a specific group on the host, or ensuring the directory ownership aligns with the container user's UID/GID 1000 and then using less permissive chmod) could be used without impacting container functionality. Please add a comment explaining the rationale for 0o777 if it's strictly required.
• [ERROR][bug] The deleteOutputFolder(job) method is asynchronous. Removing the await keyword here means the cleanup operation might not complete before the function returns or other operations proceed, potentially leading to race conditions or incomplete cleanup. Please re-add await.
• [INFO][security] Great addition of PidsLimit. This is a good practice to prevent fork bombs or excessive resource consumption within containers, enhancing system stability.
• [INFO][security] Excellent work on explicitly setting the User to a non-root UID/GID (1000:1000) and enforcing no-new-privileges (L1693). These are significant security improvements that limit the potential impact of vulnerabilities within the container. This aligns well with best practices for container security.

@ndrpp
Copy link
Copy Markdown
Member Author

ndrpp commented Mar 30, 2026

the getKeyManager().getEthAddress() and deleteOutputFolder(job) are synchronous, so no issues here

@ndrpp ndrpp marked this pull request as ready for review March 30, 2026 10:27
@ndrpp ndrpp linked an issue Mar 30, 2026 that may be closed by this pull request
@ndrpp ndrpp self-assigned this Mar 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Change docker user

2 participants