Add implementation of PicoD#65
Conversation
This commit introduces PicoD, a lightweight agent daemon designed for remote execution and file management within the agentcube runtime environment. Key features included: - Secure initialization and authentication using RSA keys and JWT. - Remote command execution with timeout and environment support. - File system operations: upload, download, and listing. - Workspace isolation for file access safety. - Initial end-to-end tests. Signed-off-by: Zhou Zihang <z@mcac.cc>
Summary of ChangesHello @acsoto, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request establishes the foundational implementation for a new PicoD server, designed to offer secure remote file management and command execution capabilities. The primary goal is to provide a tightly controlled and authenticated interface for interacting with a remote system's filesystem and shell, ensuring operations are confined to a secure workspace and validated through cryptographic means. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 introduces a new standalone PicoD server, which is a significant addition. The implementation provides REST endpoints for file operations and command execution, secured by JWT authentication. The code is well-structured and includes a comprehensive set of end-to-end tests.
My review has identified several issues, with a focus on security and correctness. Most critically, the command execution endpoint is vulnerable to command injection and path traversal, which could allow an authenticated user to gain full control over the server. I have also found a bug in the command timeout logic and a minor race condition in file uploads. My comments provide specific suggestions to address these vulnerabilities and bugs to improve the overall security and robustness of the PicoD server.
There was a problem hiding this comment.
Pull request overview
This PR introduces a new PicoD server implementation that provides a REST API for secure remote command execution and file operations. The server uses JWT-based authentication with RSA public key cryptography, implementing a bootstrap-then-session key model for initialization and subsequent operations. All file operations are scoped to a configured workspace directory for security.
Key changes:
- Added standalone PicoD server with JWT authentication and workspace-scoped file operations
- Implemented REST endpoints for command execution, file upload/download, and directory listing
- Added comprehensive end-to-end test coverage with security validation scenarios
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 17 comments.
Show a summary per file
| File | Description |
|---|---|
| cmd/picod/main.go | Main entry point for PicoD server with CLI flag parsing for port, bootstrap key, and workspace configuration |
| pkg/picod/server.go | Server initialization, routing setup, and health check endpoint |
| pkg/picod/auth.go | JWT-based authentication with RSA keys, implementing bootstrap initialization and session authentication middleware |
| pkg/picod/execute.go | Command execution handler with timeout support and environment variable configuration |
| pkg/picod/files.go | File upload (multipart and base64), download, and listing handlers with path sanitization for workspace security |
| pkg/picod/picod_test.go | Comprehensive end-to-end tests covering authentication flow, file operations, command execution, and security scenarios |
| go.mod | Added golang-jwt/jwt/v5 dependency and promoted k8s.io/utils from indirect to direct dependency |
| go.sum | Updated checksums for new jwt dependency |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Zhou Zihang <z@mcac.cc>
|
/ok-to-test |
pkg/picod/auth.go
Outdated
|
|
||
| // Verify body hash if present in claims | ||
| // We mandate body_sha256 for requests with body to ensure integrity | ||
| if claimHash, ok := claims["body_sha256"].(string); ok { |
There was a problem hiding this comment.
Do we really need to verify the body? This means we need to sign different token each request
Signed-off-by: Zhou Zihang <z@mcac.cc>
Signed-off-by: Zhou Zihang <z@mcac.cc>
…iddleware Signed-off-by: Zhou Zihang <z@mcac.cc>
…ror handling Signed-off-by: Zhou Zihang <z@mcac.cc>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated 21 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…and update related tests Signed-off-by: Zhou Zihang <z@mcac.cc>
Signed-off-by: Zhou Zihang <z@mcac.cc>
… safety Signed-off-by: Zhou Zihang <z@mcac.cc>
…eliminating the race window Signed-off-by: Zhou Zihang <z@mcac.cc>
…unctions Signed-off-by: Zhou Zihang <z@mcac.cc>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated 16 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Zhou Zihang <z@mcac.cc>
|
I've addressed all comments. Please take another look. |
Signed-off-by: Zhou Zihang <z@mcac.cc>
| return am.savePublicKeyLocked(publicKeyStr) | ||
| } | ||
|
|
||
| func (am *AuthManager) savePublicKeyLocked(publicKeyStr string) error { |
There was a problem hiding this comment.
can you explain why do need to save it into a file?
There was a problem hiding this comment.
To save the publick key and to check whether the sandbox is registered.
pkg/picod/auth.go
Outdated
|
|
||
| // Verify body hash if present in claims | ||
| // We mandate body_sha256 for requests with body to ensure integrity | ||
| if claimHash, ok := claims["body_sha256"].(string); ok { |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated 21 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if !am.IsInitialized() { | ||
| c.JSON(http.StatusForbidden, gin.H{ | ||
| "error": "Server not initialized", | ||
| "code": http.StatusForbidden, | ||
| "detail": fmt.Sprintf("Please initialize this Picod instance first via /init. Request path is %s", c.Request.URL.Path), | ||
| }) | ||
| c.Abort() | ||
| return | ||
| } |
There was a problem hiding this comment.
The initialized flag is checked without holding the mutex in the AuthMiddleware. While am.IsInitialized() properly uses RLock, the subsequent read of am.publicKey at lines 315-317 acquires the lock again. Between these two operations, the state could theoretically change. Although in practice initialization happens once and the publicKey is never modified after being set, this creates a subtle race condition window. Consider holding a single read lock for the entire check-and-use operation, or document why this is safe due to write-once semantics.
| defer dst.Close() | ||
|
|
||
| // Copy content | ||
| if _, err := io.Copy(dst, src); err != nil { | ||
| c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to save file content", "code": http.StatusInternalServerError}) | ||
| return | ||
| } |
There was a problem hiding this comment.
File handle may be writable as a result of data flow from a call to OpenFile and closing it may result in data loss upon failure, which is not handled explicitly.
| defer dst.Close() | |
| // Copy content | |
| if _, err := io.Copy(dst, src); err != nil { | |
| c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to save file content", "code": http.StatusInternalServerError}) | |
| return | |
| } | |
| // Defer is not used so we can check for errors on close | |
| // Copy content | |
| if _, err := io.Copy(dst, src); err != nil { | |
| c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to save file content", "code": http.StatusInternalServerError}) | |
| dst.Close() // attempt to close, ignore error since copy already failed | |
| return | |
| } | |
| // Check for errors on close | |
| if err := dst.Close(); err != nil { | |
| c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to finalize file write", "code": http.StatusInternalServerError}) | |
| return | |
| } |
Signed-off-by: Zhou Zihang <z@mcac.cc>
|
[APPROVALNOTIFIER] This PR is APPROVED Approval requirements bypassed by manually added approval. This pull-request has been approved by: hzxuzhonghu The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
This pull request introduces a new standalone PicoD server with REST endpoints for secure file operations and command execution. It adds a main entry point, implements handlers for file upload, download, and listing, and enables command execution with timeout and environment support. The implementation includes workspace scoping for security and updates dependencies.
New PicoD server implementation:
cmd/picod/main.goas the main entry point for the PicoD server, supporting configuration via flags and secure workspace scoping.File operations REST API:
UploadFileHandler,DownloadFileHandler, andListFilesHandlerinpkg/picod/files.gofor file upload (multipart and base64 JSON), download, and directory listing, with robust path sanitization and permission handling to prevent directory traversal.Command execution REST API:
ExecuteHandlerinpkg/picod/execute.goto execute shell commands with configurable timeout, working directory, and environment variables, returning detailed execution results.Workspace security:
pkg/picod/files.goto restrict all file operations to a configured root directory, preventing unauthorized access outside the workspace.Dependency updates:
go.modto addgithub.com/golang-jwt/jwt/v5andk8s.io/utils, and removed duplicate indirect dependency fork8s.io/utils. [1] [2] [3]