Authorization for apiserver #6

YaoZengzeng · 2025-10-28T02:53:44Z

No description provided.

gemini-code-assist · 2025-10-28T02:54:10Z

Summary of Changes

Hello @YaoZengzeng, I'm Gemini Code Assist¹! 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 introduces a significant architectural change to the pico-apiserver's authorization system. It transitions from a rudimentary JWT-based authentication to a robust, Kubernetes-native Service Account TokenReview mechanism. This shift ensures that all API operations, particularly those involving Kubernetes resource management like sandbox lifecycles, are performed with the explicit permissions of the requesting user's Service Account and within their specific namespace. This change not only strengthens the security posture but also lays the groundwork for more sophisticated multi-tenant environments.

Highlights

Kubernetes Native Authorization: The pico-apiserver now uses Kubernetes Service Account TokenReview for authenticating API requests, replacing the previous placeholder JWT validation.
Caller-Permission-Based Operations: Sandbox creation and deletion operations are now executed using the authenticated caller's Kubernetes Service Account permissions within their designated namespace, enhancing security and multi-tenancy.
Updated Integration Test Client: The SSH key-based authentication integration test client has been relocated to the 'example' directory and updated to demonstrate the new token-based authorization model, including detailed RBAC configuration guidance.
RBAC Permissions for API Server: The pico-apiserver's Kubernetes Role has been updated to include 'create' permissions for 'tokenreviews', enabling it to validate incoming Service Account tokens.

Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

gemini-code-assist

Code Review

This pull request is a significant improvement, transitioning the apiserver from a simple JWT secret to a robust Kubernetes-native authentication model using Service Account tokens and the TokenReview API. The shift to a caller-permission-based model, where actions are performed with the user's identity in their own namespace, is a great security enhancement. The code is well-structured, and the new example client with its detailed README is very helpful. My review includes a few suggestions to enhance the robustness of token parsing and context value extraction to make the new implementation even more solid.

gemini-code-assist · 2025-10-28T02:55:32Z

example/client.go

+	if authToken == "" {
+		log.Println("⚠️  WARNING: API_TOKEN environment variable not set")
+		log.Println("   Attempting to proceed without authentication token")
+		log.Println("   Set API_TOKEN to authenticate with the API server")
+		log.Println()
+	} else {
+		log.Println("✅ API authentication token loaded from environment")
+		log.Println()
+	}


Since the server now mandates an authentication token, the client will fail with a 401 Unauthorized error if API_TOKEN is not set. Instead of printing a warning and proceeding, it would provide clearer and more immediate feedback to the user if the client exits with a fatal error. This makes the example client's behavior consistent with the new server requirements.

if authToken == "" { log.Fatalf("❌ ERROR: API_TOKEN environment variable not set. Please set it to a valid service account token.") } log.Println("✅ API authentication token loaded from environment") log.Println()

pkg/apiserver/auth.go

gemini-code-assist · 2025-10-28T02:55:33Z

pkg/apiserver/handlers.go

+	userToken, _ := r.Context().Value(contextKeyUserToken).(string)
+	userNamespace, _ := r.Context().Value(contextKeyNamespace).(string)
+	serviceAccount, _ := r.Context().Value(contextKeyServiceAccount).(string)
+
+	if userToken == "" || userNamespace == "" {
+		respondError(w, http.StatusUnauthorized, "UNAUTHORIZED", "Unable to extract user credentials")
+		return
+	}


It's more idiomatic in Go to check the ok boolean returned from a type assertion when retrieving values from a context. While the current code works because a failed assertion for a string results in an empty string, explicitly checking ok makes the intent clearer and the code more robust.

Suggested change

userToken, _ := r.Context().Value(contextKeyUserToken).(string)

userNamespace, _ := r.Context().Value(contextKeyNamespace).(string)

serviceAccount, _ := r.Context().Value(contextKeyServiceAccount).(string)

if userToken == "" || userNamespace == "" {

respondError(w, http.StatusUnauthorized, "UNAUTHORIZED", "Unable to extract user credentials")

return

}

userToken, ok1 := r.Context().Value(contextKeyUserToken).(string)

userNamespace, ok2 := r.Context().Value(contextKeyNamespace).(string)

serviceAccount, _ := r.Context().Value(contextKeyServiceAccount).(string)

if !ok1 || !ok2 || userToken == "" || userNamespace == "" {

respondError(w, http.StatusUnauthorized, "UNAUTHORIZED", "Unable to extract user credentials")

return

}

gemini-code-assist · 2025-10-28T02:55:33Z

pkg/apiserver/handlers.go

+	userToken, _ := r.Context().Value(contextKeyUserToken).(string)
+	userNamespace, _ := r.Context().Value(contextKeyNamespace).(string)
+	serviceAccount, _ := r.Context().Value(contextKeyServiceAccount).(string)
+
+	if userToken == "" || userNamespace == "" {
+		respondError(w, http.StatusUnauthorized, "UNAUTHORIZED", "Unable to extract user credentials")
+		return
+	}


Similar to the handleCreateSession handler, it's more idiomatic and robust to check the ok boolean from the type assertion when extracting values from the request context. This improves code clarity and ensures that the absence of a value is handled explicitly.

Suggested change

userToken, _ := r.Context().Value(contextKeyUserToken).(string)

userNamespace, _ := r.Context().Value(contextKeyNamespace).(string)

serviceAccount, _ := r.Context().Value(contextKeyServiceAccount).(string)

if userToken == "" || userNamespace == "" {

respondError(w, http.StatusUnauthorized, "UNAUTHORIZED", "Unable to extract user credentials")

return

}

userToken, ok1 := r.Context().Value(contextKeyUserToken).(string)

userNamespace, ok2 := r.Context().Value(contextKeyNamespace).(string)

serviceAccount, _ := r.Context().Value(contextKeyServiceAccount).(string)

if !ok1 || !ok2 || userToken == "" || userNamespace == "" {

respondError(w, http.StatusUnauthorized, "UNAUTHORIZED", "Unable to extract user credentials")

return

}

hzxuzhonghu · 2025-11-04T07:34:16Z

example/README.md

+
+## Permission Model
+
+**Important:** pico-apiserver now uses a caller-permission-based operation model. This means:


check all pico-apiserver and update to agentcube-apiserver

good catch, maybe the rebase issue.

hzxuzhonghu · 2025-11-04T07:36:35Z

example/README.md

+rules:
+- apiGroups: ["agents.x-k8s.io"]
+  resources: ["sandboxes"]
+  verbs: ["create", "delete", "get", "list", "watch"]


we may not need watch

Copilot

Pull Request Overview

This PR refactors the authentication and authorization model to implement caller-permission-based operations using Kubernetes Service Account tokens. The key change is moving from a centralized permission model to one where operations are executed using the caller's own Service Account credentials.

Replaces JWT-based authentication with Kubernetes TokenReview API validation
Introduces per-user Kubernetes clients that execute operations with caller's permissions
Updates sandbox operations to use user namespaces and credentials

Reviewed Changes

Copilot reviewed 9 out of 11 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
pkg/apiserver/k8s_client.go	Adds UserK8sClient type and baseConfig storage for creating per-user Kubernetes clients
pkg/apiserver/handlers.go	Updates create/delete handlers to extract user credentials and use UserK8sClient for operations
pkg/apiserver/auth.go	Replaces JWT validation with Kubernetes TokenReview API and extracts service account details
pkg/apiserver/config.go	Removes deprecated JWTSecret field
cmd/apiserver/main.go	Removes jwt-secret command line flag
k8s/agentcube-apiserver.yaml	Adds RBAC permissions for TokenReview API access
example/client.go	Updates to use API_TOKEN environment variable with improved messaging
example/README.md	Documents new permission model and RBAC configuration requirements
.gitignore	Updates test binary path
go.mod, go.sum	Adds indirect dependencies (kr/fs, pkg/sftp)

Comments suppressed due to low confidence (3)

go.mod:3

Go version 1.24.4 does not exist. As of January 2025, the latest Go versions are in the 1.23.x series. This appears to be a future version that is not yet released. Please use a valid Go version such as 1.23.x or earlier.

go 1.24.4

go.mod:5

Go toolchain version 1.24.9 does not exist. As of January 2025, the latest Go versions are in the 1.23.x series. This appears to be a future version that is not yet released. Please use a valid Go toolchain version such as 1.23.x or earlier.

toolchain go1.24.9

example/client.go:61

The authToken variable is read twice from the environment (line 61 and line 66). The second assignment on line 66 is redundant and should be removed to avoid confusion.

	authToken = os.Getenv("API_TOKEN")

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

pkg/apiserver/auth.go

k8s/agentcube-apiserver.yaml

pkg/apiserver/auth.go

hzxuzhonghu · 2025-11-04T07:49:05Z

pkg/apiserver/handlers.go

+	}
+
+	// Create K8s client with user's token
+	userClient, err := s.k8sClient.NewUserK8sClient(userToken, userNamespace)


This would create one client per call. I am concerned, It will create too many clients.

hzxuzhonghu

And it seems you donot protect the tunnel handler

YaoZengzeng · 2025-11-07T06:16:45Z

@hzxuzhonghu updated

hzxuzhonghu · 2025-11-07T07:50:01Z

cmd/apiserver/main.go

+		enableTLS                  = flag.Bool("enable-tls", false, "Enable TLS (HTTPS)")
+		tlsCert                    = flag.String("tls-cert", "", "Path to TLS certificate file")
+		tlsKey                     = flag.String("tls-key", "", "Path to TLS key file")
+		agentCubeAPIServiceAccount = flag.String("agentcube-api-service-account", "agentcube-apiserver", "Service account name for agentcube-apiserver (has admin privileges)")


admin privileges seems confusing

BTW, what token it this?

hzxuzhonghu · 2025-11-07T07:51:48Z

cmd/apiserver/main.go

-		jwtSecret        = flag.String("jwt-secret", "", "JWT secret for token validation (optional)")
+		port                       = flag.String("port", "8080", "API server port")
+		namespace                  = flag.String("namespace", "default", "Kubernetes namespace for sandboxes")
+		sshUsername                = flag.String("ssh-username", "sandbox", "Default SSH username for sandbox pods")


nit: i think this name should be passed from client, it is common client can use their own images, which can run as customized users

pkg/apiserver/auth.go

hzxuzhonghu · 2025-11-07T08:11:51Z

pkg/apiserver/auth.go

+//   - User is the creator of the sandbox
+func (s *Server) checkSandboxAccess(sandbox *Sandbox, serviceAccountName string) bool {
+	// Check if user is agentcube-apiserver (has admin access)
+	if s.config.AgentCubeAPIServiceAccount != "" && serviceAccountName == s.config.AgentCubeAPIServiceAccount {


I am not quite understand why compare with s.config.AgentCubeAPIServiceAccount.

hzxuzhonghu · 2025-11-07T08:22:41Z

pkg/apiserver/client_cache.go

+
+	// Check if entry is expired
+	if time.Since(entry.lastAccess) > c.ttl {
+		return nil


I donot see where you call the cleanup funciton, could do lazy cleanup here

hzxuzhonghu · 2025-11-07T08:23:22Z

pkg/apiserver/k8s_client.go

+
+	// If cache entry exists but token doesn't match, remove it
+	// This handles token rotation scenarios
+	if c.clientCache.Get(cacheKey) != nil {


duplicate with L131?

hzxuzhonghu · 2025-11-07T08:25:19Z

pkg/apiserver/k8s_client.go

+
+// NewUserK8sClient creates a K8s client using the provided user token
+// This method creates a new client without using cache (for direct creation)
+func (c *K8sClient) NewUserK8sClient(userToken, namespace string) (*UserK8sClient, error) {


From my perspective, we can only cache dynamicClient.

Signed-off-by: YaoZengzeng <yaozengzeng@huawei.com>

YaoZengzeng · 2025-11-11T07:09:50Z

@hzxuzhonghu updated

hzxuzhonghu · 2025-11-11T09:09:14Z

/lgtm
/approve

We need to keep track of sdk side in sync with server side update

volcano-sh-bot · 2025-11-11T09:09:32Z

[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

Details

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

YaoZengzeng · 2025-11-11T11:18:15Z

@hzxuzhonghu please merge

hzxuzhonghu · 2025-11-12T01:11:12Z

[needs-rebase] label prevented the bot merging

YaoZengzeng · 2025-11-12T01:29:53Z

[needs-rebase] label prevented the bot merging

I have rebase the PR, but the bot doesn't detect it and not remove the label, weird.

gemini-code-assist bot reviewed Oct 28, 2025

View reviewed changes

volcano-sh-bot added the needs-rebase label Oct 30, 2025

YaoZengzeng force-pushed the token branch 2 times, most recently from b9bf44a to 7c46eb9 Compare November 3, 2025 08:16

volcano-sh-bot removed the needs-rebase label Nov 4, 2025

hzxuzhonghu requested a review from Copilot November 4, 2025 07:32

hzxuzhonghu reviewed Nov 4, 2025

View reviewed changes

Copilot AI reviewed Nov 4, 2025

View reviewed changes

pkg/apiserver/auth.go Show resolved Hide resolved

hzxuzhonghu reviewed Nov 4, 2025

View reviewed changes

k8s/agentcube-apiserver.yaml Show resolved Hide resolved

hzxuzhonghu reviewed Nov 4, 2025

View reviewed changes

pkg/apiserver/auth.go Show resolved Hide resolved

hzxuzhonghu reviewed Nov 4, 2025

View reviewed changes

volcano-sh-bot added the size/XL label Nov 6, 2025

hzxuzhonghu reviewed Nov 7, 2025

View reviewed changes

token authorization

3d1ee07

Signed-off-by: YaoZengzeng <yaozengzeng@huawei.com>

volcano-sh-bot added the needs-rebase label Nov 11, 2025

fix the cache implementation

8dd43c1

Signed-off-by: YaoZengzeng <yaozengzeng@huawei.com>

YaoZengzeng force-pushed the token branch from 8abc894 to 8dd43c1 Compare November 11, 2025 06:55

fix comments

e13f225

Signed-off-by: YaoZengzeng <yaozengzeng@huawei.com>

volcano-sh-bot assigned hzxuzhonghu Nov 11, 2025

volcano-sh-bot added the lgtm label Nov 11, 2025

hzxuzhonghu added the approved label Nov 11, 2025

YaoZengzeng mentioned this pull request Nov 11, 2025

SDK need support authorization #21

Open

volcano-sh-bot removed the needs-rebase label Nov 12, 2025

volcano-sh-bot merged commit 0b1fa62 into volcano-sh:main Nov 12, 2025
2 checks passed


		## Permission Model

		Important: pico-apiserver now uses a caller-permission-based operation model. This means:

Authorization for apiserver #6

Authorization for apiserver #6

Uh oh!

Conversation

YaoZengzeng commented Oct 28, 2025

Uh oh!

gemini-code-assist bot commented Oct 28, 2025

Summary of Changes

Highlights

Footnotes

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Oct 28, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gemini-code-assist bot Oct 28, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Oct 28, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hzxuzhonghu left a comment

Choose a reason for hiding this comment

Uh oh!

YaoZengzeng commented Nov 7, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

YaoZengzeng commented Nov 11, 2025

Uh oh!

hzxuzhonghu commented Nov 11, 2025

Uh oh!

volcano-sh-bot commented Nov 11, 2025

Uh oh!

YaoZengzeng commented Nov 11, 2025

Uh oh!

hzxuzhonghu commented Nov 12, 2025

Uh oh!

YaoZengzeng commented Nov 12, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development