Skip to content

refactor: new image layout and organization #13

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Jun 20, 2025
Merged

Conversation

tito
Copy link
Member

@tito tito commented Jun 20, 2025

User description

This PR rework how image are build and separated. Most of the initialization are now shared across image.


PR Type

Enhancement


Description

  • Standardized image initialization system

  • Python-based plugin architecture

  • Shared initialization across images

  • Improved build context management


Changes walkthrough 📝

Relevant files
Enhancement
9 files
cli.py
Enhanced image build process with temporary build context
+61/-13 
container.py
Simplified network connection logging                                       
+7/-10   
cubbi_init.py
Added shared Python initialization system                               
+692/-0 
goose_plugin.py
Added Goose-specific plugin for initialization                     
+195/-0 
update-goose-config.py
Removed in favor of plugin architecture                                   
+0/-106 
cubbi-init.sh
Removed in favor of Python initialization                               
+0/-188 
entrypoint.sh
Removed in favor of Python entrypoint                                       
+0/-7     
init-status.sh
Updated status tracking for new initialization                     
+8/-5     
Dockerfile
Simplified with Python-based initialization                           
+15/-27 
Configuration changes
2 files
config.py
Updated image config file naming convention                           
+2/-2     
cubbi_image.yaml
Simplified configuration removing redundant options           
+1/-26   
Formatting
1 files
__init__.py
Removed unnecessary module docstring                                         
+0/-3     
Documentation
4 files
README.md
Updated image configuration file naming                                   
+1/-1     
README.md
Updated documentation for new architecture                             
+36/-14 
1_SPECIFICATIONS.md
Updated file naming convention references                               
+3/-3     
3_IMAGE.md
Added comprehensive image specification documentation       
+327/-0 

Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
  • @pr-agent-monadical
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
    🧪 No relevant tests
    🔒 Security concerns

    Path traversal vulnerability:
    The ConfigManager.create_symlink method in cubbi_init.py (lines 330-355) creates symlinks based on user-configurable paths without proper validation. An attacker could potentially exploit this to create symlinks to sensitive system files or directories outside the container's intended scope, leading to information disclosure or privilege escalation. The code should validate that both source and target paths are within allowed directories before creating symlinks.

    ⚡ Recommended focus areas for review

    Error Handling

    The new build_image function creates a temporary directory and performs file operations but has incomplete error handling. The exception block catches errors during build context preparation but doesn't handle errors that might occur during the docker build command execution.

    # Create temporary build directory
    with tempfile.TemporaryDirectory() as temp_dir:
        temp_path = Path(temp_dir)
        console.print(f"Using temporary build directory: {temp_path}")
    
        try:
            # Copy all files from the image directory to temp directory
            for item in image_path.iterdir():
                if item.is_file():
                    shutil.copy2(item, temp_path / item.name)
                elif item.is_dir():
                    shutil.copytree(item, temp_path / item.name)
    
            # Copy shared cubbi_init.py to temp directory
            shared_init_path = Path(__file__).parent / "images" / "cubbi_init.py"
            if shared_init_path.exists():
                shutil.copy2(shared_init_path, temp_path / "cubbi_init.py")
                console.print("Copied shared cubbi_init.py to build context")
            else:
                console.print(
                    f"[yellow]Warning: Shared cubbi_init.py not found at {shared_init_path}[/yellow]"
                )
    
            # Copy shared init-status.sh to temp directory
            shared_status_path = Path(__file__).parent / "images" / "init-status.sh"
            if shared_status_path.exists():
                shutil.copy2(shared_status_path, temp_path / "init-status.sh")
                console.print("Copied shared init-status.sh to build context")
            else:
                console.print(
                    f"[yellow]Warning: Shared init-status.sh not found at {shared_status_path}[/yellow]"
                )
    
            # Copy image-specific plugin if it exists
            plugin_path = image_path / f"{image_name.lower()}_plugin.py"
            if plugin_path.exists():
                shutil.copy2(plugin_path, temp_path / f"{image_name.lower()}_plugin.py")
                console.print(f"Copied {image_name.lower()}_plugin.py to build context")
    
            # Copy init-status.sh if it exists (for backward compatibility with shell connection)
            init_status_path = image_path / "init-status.sh"
            if init_status_path.exists():
                shutil.copy2(init_status_path, temp_path / "init-status.sh")
                console.print("Copied init-status.sh to build context")
    
            # Build the image from temporary directory
            with console.status(f"Building image {docker_image_name}..."):
                result = os.system(
                    f"cd {temp_path} && docker build -t {docker_image_name} ."
                )
    
        except Exception as e:
            console.print(f"[red]Error preparing build context: {e}[/red]")
            return
    Suppressed Error

    The code now suppresses DockerException errors when connecting to MCP dedicated networks. While there's a comment explaining this is intentional, the error variable 'e' is referenced in the commented code but not defined in the except clause.

    except DockerException:
        # print(
        #     f"Error connecting to MCP dedicated network '{dedicated_network_name}': {e}"
        # )
        # commented out, may be accessible through another attached network, it's
        # not mandatory here.
        pass
    Security Concern

    The initialization script creates symlinks and directories with potentially insecure permissions. The script should validate paths and ensure proper permission boundaries to prevent path traversal or privilege escalation.

    def create_symlink(
        self, source_path: str, target_path: str, user_id: int, group_id: int
    ) -> bool:
        """Create a symlink with proper ownership"""
        try:
            source = Path(source_path)
    
            parent_dir = source.parent
            if not parent_dir.exists():
                self.status.log(f"Creating parent directory: {parent_dir}")
                parent_dir.mkdir(parents=True, exist_ok=True)
                os.chown(parent_dir, user_id, group_id)
    
            self.status.log(f"Creating symlink: {source_path} -> {target_path}")
            if source.is_symlink() or source.exists():
                source.unlink()
    
            source.symlink_to(target_path)
            os.lchown(source_path, user_id, group_id)
    
            return True
        except Exception as e:
            self.status.log(
                f"Failed to create symlink {source_path} -> {target_path}: {e}", "ERROR"
            )
            return False

    Comment on lines +571 to +577
    except DockerException:
    # print(
    # f"Error connecting to MCP dedicated network '{dedicated_network_name}': {e}"
    # )
    # commented out, may be accessible through another attached network, it's
    # not mandatory here.
    pass
    Copy link
    Contributor

    Choose a reason for hiding this comment

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

    Suggestion: The exception handler catches a DockerException but doesn't define the exception variable 'e' that was previously used in the commented code. This could cause issues if the commented code is uncommented in the future. [general, importance: 8]

    Suggested change
    except DockerException:
    # print(
    # f"Error connecting to MCP dedicated network '{dedicated_network_name}': {e}"
    # )
    # commented out, may be accessible through another attached network, it's
    # not mandatory here.
    pass
    except DockerException as e:
    # print(
    # f"Error connecting to MCP dedicated network '{dedicated_network_name}': {e}"
    # )
    # commented out, may be accessible through another attached network, it's
    # not mandatory here.
    pass

    Comment on lines +52 to +53
    # Pre-install the cubbi_init
    RUN /cubbi/cubbi_init.py --help
    Copy link
    Contributor

    Choose a reason for hiding this comment

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

    Suggestion: The pre-installation of cubbi_init using --help is inefficient and could mask potential initialization errors. Instead, install required Python dependencies explicitly. [general, importance: 6]

    Suggested change
    # Pre-install the cubbi_init
    RUN /cubbi/cubbi_init.py --help
    # Install Python dependencies for cubbi_init
    RUN pip install ruamel.yaml


    # Set up environment
    ENV PYTHONUNBUFFERED=1
    ENV PYTHONDONTWRITEBYTECODE=1
    ENV UV_LINK_MODE=copy
    Copy link
    Contributor

    Choose a reason for hiding this comment

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

    Suggestion: The UV_LINK_MODE=copy environment variable is set but the uv package installer is only used during build time, not runtime. This environment variable should be set only during the installation command to avoid unnecessary persistent configuration. [general, importance: 3]

    Suggested change
    ENV UV_LINK_MODE=copy
    # No need for persistent UV_LINK_MODE in the final image
    # Remove this line or use it only during installation

    Comment on lines +39 to +44
    # Copy initialization system
    COPY cubbi_init.py /cubbi/cubbi_init.py
    COPY goose_plugin.py /cubbi/goose_plugin.py
    COPY cubbi_image.yaml /cubbi/cubbi_image.yaml
    COPY init-status.sh /cubbi/init-status.sh
    RUN chmod +x /cubbi/cubbi_init.py /cubbi/init-status.sh
    Copy link
    Contributor

    Choose a reason for hiding this comment

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

    Suggestion: The initialization scripts are copied to /cubbi/ directory but the entrypoint references /cubbi/cubbi_init.py directly. This inconsistency with the documentation (which specifies scripts should be in the root directory) could cause integration issues. [possible issue, importance: 8]

    Suggested change
    # Copy initialization system
    COPY cubbi_init.py /cubbi/cubbi_init.py
    COPY goose_plugin.py /cubbi/goose_plugin.py
    COPY cubbi_image.yaml /cubbi/cubbi_image.yaml
    COPY init-status.sh /cubbi/init-status.sh
    RUN chmod +x /cubbi/cubbi_init.py /cubbi/init-status.sh
    # Copy initialization system according to specifications
    COPY cubbi_init.py /cubbi_init.py
    COPY goose_plugin.py /goose_plugin.py
    COPY cubbi_image.yaml /cubbi/cubbi_image.yaml
    COPY init-status.sh /init-status.sh
    RUN chmod +x /cubbi_init.py /init-status.sh

    COPY cubbi_image.yaml /cubbi/cubbi_image.yaml
    COPY init-status.sh /cubbi/init-status.sh
    RUN chmod +x /cubbi/cubbi_init.py /cubbi/init-status.sh
    RUN echo '[ -x /cubbi/init-status.sh ] && /cubbi/init-status.sh' >> /etc/bash.bashrc
    Copy link
    Contributor

    Choose a reason for hiding this comment

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

    Suggestion: The path to init-status.sh in the bash.bashrc configuration doesn't match the path specified in the documentation. This will prevent the status tracking from working correctly when users log in. [possible issue, importance: 7]

    Suggested change
    RUN echo '[ -x /cubbi/init-status.sh ] && /cubbi/init-status.sh' >> /etc/bash.bashrc
    RUN echo '[ -x /init-status.sh ] && /init-status.sh' >> /etc/bash.bashrc

    @tito tito merged commit e5121dd into main Jun 20, 2025
    6 of 7 checks passed
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    1 participant