Skip to content

Conversation

@solimander
Copy link
Collaborator

Summary

  • Run TEE operations (kms-client, TLS setup) as root to access the attestation socket
  • Drop privileges to the original user before executing the application
  • Use namespaced env var to avoid conflicts with user-defined variables

@claude
Copy link

claude bot commented Dec 2, 2025

Claude finished @solimander's task —— View job


PR Review: Non-root User Support in Dockerfile

Todo List:

  • Read and analyze changed files
  • Review privilege dropping implementation
  • Check security implications
  • Evaluate environment variable handling
  • Provide comprehensive feedback

Overall Assessment

This PR implements a solid approach to handling non-root users in Docker containers while maintaining security for TEE operations. The implementation correctly runs privileged operations as root first, then drops privileges to the original user.

Code Quality & Best Practices ✅

Strengths:

  • Clean separation of concerns: TEE setup runs as root, application runs as original user
  • Proper privilege dropping using su -s /bin/sh with exec to preserve PID 1
  • Good use of conditional templating in Dockerfile based on OriginalUser presence
  • Clear logging throughout the privilege dropping process

Template Structure (Dockerfile.layered.tmpl:27-42):

  • Proper conditional logic for handling root vs non-root base images
  • Correctly sets file permissions (755 for executables, 644 for keys)
  • Environment variable setup is clean and namespaced

Security Analysis 🔒

Positive Security Aspects:

  1. Minimal Root Time: Root privileges are only used for TEE initialization, then immediately dropped
  2. Proper Privilege Dropping: Uses su with shell execution rather than just changing UID
  3. Namespaced Environment Variable: __EIGENX_ORIGINAL_USER prefix avoids conflicts with user variables
  4. No Privilege Escalation Path: Once privileges are dropped, they cannot be regained

Security Considerations:

  • The approach is secure by design - TEE operations require root access to the attestation socket
  • Environment variable handling is safe with the namespaced prefix
  • File permissions are set appropriately for the security model

Performance Considerations ⚡

Minimal Performance Impact:

  • The su command adds negligible overhead during startup
  • Root operations are front-loaded and brief
  • No ongoing performance implications once application starts

Potential Issues & Improvements 🔧

Minor Issues Found:

  1. Error Handling in Privilege Drop (compute-source-env.sh.tmpl:112-115):

    if [ -n "$__EIGENX_ORIGINAL_USER" ] && [ "$(id -u)" = "0" ]; then
        echo "compute-source-env.sh: Dropping privileges to user: $__EIGENX_ORIGINAL_USER"
        exec su -s /bin/sh "$__EIGENX_ORIGINAL_USER" -c 'exec "$@"' -- sh "$@"
    fi
    • Consider validating that the target user exists before attempting privilege drop
    • Could add error handling if su fails
  2. User Existence Check Missing:

    • The code assumes $__EIGENX_ORIGINAL_USER is a valid user but doesn't verify this
    • A failed su command would cause the container to exit

Suggested Improvements:

# Add user validation before privilege drop
if [ -n "$__EIGENX_ORIGINAL_USER" ] && [ "$(id -u)" = "0" ]; then
    if ! id "$__EIGENX_ORIGINAL_USER" >/dev/null 2>&1; then
        echo "compute-source-env.sh: ERROR - User $__EIGENX_ORIGINAL_USER does not exist"
        exit 1
    fi
    echo "compute-source-env.sh: Dropping privileges to user: $__EIGENX_ORIGINAL_USER"
    exec su -s /bin/sh "$__EIGENX_ORIGINAL_USER" -c 'exec "$@"' -- sh "$@"
fi

Test Coverage Considerations 🧪

Current State:

  • No obvious test coverage for privilege dropping logic
  • Would benefit from integration tests covering both root and non-root scenarios

Testing Recommendations:

  1. Test container behavior with both root and non-root base images
  2. Verify TEE operations work correctly under both privilege models
  3. Test error scenarios (invalid user, missing environment variable)
  4. Verify file permissions are set correctly in both modes

Environment Variable Handling ✅

Well Implemented:

  • __EIGENX_ORIGINAL_USER uses appropriate namespacing to avoid conflicts
  • Only set when needed (when OriginalUser template variable is provided)
  • Properly consumed in the entrypoint script

Architecture Alignment 🏗️

The implementation aligns well with the project's architecture:

  • Follows the layered Docker approach already established
  • Integrates cleanly with existing TEE setup workflow
  • Maintains compatibility with existing deployment patterns

Recommendation: APPROVE with Minor Improvements

This PR successfully solves the non-root user support problem with a secure and well-thought-out approach. The suggested user validation improvement would make it more robust, but the current implementation is functionally correct and secure.

Priority for improvements:

  1. Medium: Add user existence validation before privilege drop
  2. Low: Add integration tests for privilege dropping scenarios

@solimander solimander merged commit 7459d50 into main Dec 2, 2025
18 checks passed
@solimander solimander deleted the soli/user-instruction branch December 2, 2025 18:30
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.

3 participants