-
Notifications
You must be signed in to change notification settings - Fork 31
perf: optimize /api/projects RBAC checks - 11s → 500ms #297
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -143,8 +143,8 @@ | |||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| // ListProjects handles GET /projects | ||||||||||||||||||||||||||||
| // Lists Namespaces (both platforms) using backend SA with label selector, | ||||||||||||||||||||||||||||
| // then uses SubjectAccessReview to verify user access to each namespace | ||||||||||||||||||||||||||||
| // Uses reverse query approach: finds user's RoleBindings first, then returns matching managed namespaces | ||||||||||||||||||||||||||||
| // This scales to thousands of namespaces (O(1) API calls instead of O(N)) | ||||||||||||||||||||||||||||
| func ListProjects(c *gin.Context) { | ||||||||||||||||||||||||||||
| reqK8s, _ := GetK8sClientsForRequest(c) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
|
|
@@ -165,6 +165,32 @@ | |||||||||||||||||||||||||||
| ctx, cancel := context.WithTimeout(context.Background(), defaultK8sTimeout) | ||||||||||||||||||||||||||||
| defer cancel() | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| // Get user subject (username or service account) | ||||||||||||||||||||||||||||
| userSubject, err := getUserSubjectFromContext(c) | ||||||||||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||||||||||
| log.Printf("Failed to extract user subject: %v", err) | ||||||||||||||||||||||||||||
| c.JSON(http.StatusUnauthorized, gin.H{"error": "Failed to identify user"}) | ||||||||||||||||||||||||||||
| return | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| // Get user groups (may be empty) | ||||||||||||||||||||||||||||
| userGroups := []string{} | ||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 Code Quality: userGroups Extraction Could Be a Helper This pattern of extracting groups from context with type assertion could be error-prone if repeated elsewhere. Consider creating a helper function. Recommendation: // In helpers.go or middleware.go
func getUserGroupsFromContext(c *gin.Context) []string {
if groups, exists := c.Get("userGroups"); exists {
if groupSlice, ok := groups.([]string); ok {
return groupSlice
}
}
return []string{}
}Then use: userGroups := getUserGroupsFromContext(c)This reduces duplication and makes the code more maintainable. |
||||||||||||||||||||||||||||
| if groups, exists := c.Get("userGroups"); exists { | ||||||||||||||||||||||||||||
| if groupSlice, ok := groups.([]string); ok { | ||||||||||||||||||||||||||||
| userGroups = groupSlice | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| // Find namespaces where user has access via RoleBindings | ||||||||||||||||||||||||||||
| // This is much faster than checking each namespace individually (reverse query) | ||||||||||||||||||||||||||||
| userNamespaces, err := getUserAccessibleNamespaces(ctx, userSubject, userGroups) | ||||||||||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||||||||||
| log.Printf("Failed to get user accessible namespaces: %v", err) | ||||||||||||||||||||||||||||
| c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to list projects"}) | ||||||||||||||||||||||||||||
| return | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| // List managed namespaces | ||||||||||||||||||||||||||||
| nsList, err := K8sClientProjects.CoreV1().Namespaces().List(ctx, v1.ListOptions{ | ||||||||||||||||||||||||||||
| LabelSelector: "ambient-code.io/managed=true", | ||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||
|
|
@@ -174,23 +200,91 @@ | |||||||||||||||||||||||||||
| return | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| // Filter to only namespaces where user has access | ||||||||||||||||||||||||||||
| // Use SubjectAccessReview - checks ALL RBAC sources (any RoleBinding, group, etc.) | ||||||||||||||||||||||||||||
| // Return intersection: managed namespaces that user has access to | ||||||||||||||||||||||||||||
| for _, ns := range nsList.Items { | ||||||||||||||||||||||||||||
| hasAccess, err := checkUserCanAccessNamespace(reqK8s, ns.Name) | ||||||||||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||||||||||
| log.Printf("Failed to check access for namespace %s: %v", ns.Name, err) | ||||||||||||||||||||||||||||
| continue | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| if hasAccess { | ||||||||||||||||||||||||||||
| if userNamespaces[ns.Name] { | ||||||||||||||||||||||||||||
| projects = append(projects, projectFromNamespace(&ns, isOpenShift)) | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| c.JSON(http.StatusOK, gin.H{"items": projects}) | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| // getUserAccessibleNamespaces finds all namespaces where the user has RBAC permissions | ||||||||||||||||||||||||||||
| // Returns a map[namespace]bool for O(1) lookup when filtering managed namespaces | ||||||||||||||||||||||||||||
| // This is much faster than O(N) SubjectAccessReviews - typically 2-3 API calls vs 50-1000+ | ||||||||||||||||||||||||||||
| func getUserAccessibleNamespaces(ctx context.Context, userSubject string, userGroups []string) (map[string]bool, error) { | ||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 Documentation: Function Comment Needs Update The comment states "typically 2-3 API calls" but doesn't account for the cluster-admin fast path optimization. Recommendation:
Suggested change
|
||||||||||||||||||||||||||||
| namespaces := make(map[string]bool) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| // Fast path: check if user is cluster-admin (has access to all namespaces) | ||||||||||||||||||||||||||||
| // ClusterRoleBindings with cluster-admin give access to everything | ||||||||||||||||||||||||||||
| clusterRoleBindings, err := K8sClientProjects.RbacV1().ClusterRoleBindings().List(ctx, v1.ListOptions{}) | ||||||||||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||||||||||
| log.Printf("Failed to list ClusterRoleBindings: %v", err) | ||||||||||||||||||||||||||||
| // Non-fatal - continue with RoleBinding check | ||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||
| for _, crb := range clusterRoleBindings.Items { | ||||||||||||||||||||||||||||
| // Check if this ClusterRoleBinding gives cluster-admin | ||||||||||||||||||||||||||||
| if crb.RoleRef.Name == "cluster-admin" || strings.Contains(crb.RoleRef.Name, "admin") { | ||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 Security Issue: Overly Permissive Admin Detection This check is too broad and could grant unauthorized access: if crb.RoleRef.Name == "cluster-admin" || strings.Contains(crb.RoleRef.Name, "admin") {Problems:
Recommendation:
Suggested change
Only |
||||||||||||||||||||||||||||
| if subjectMatchesUser(crb.Subjects, userSubject, userGroups) { | ||||||||||||||||||||||||||||
| // User is cluster-admin - return all namespaces | ||||||||||||||||||||||||||||
| log.Printf("User %s has cluster-admin via ClusterRoleBinding %s", userSubject, crb.Name) | ||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Performance: Error Handling Could Skip Valid Results When listing all namespaces fails (line 232), the function continues to the normal path but the error is silently ignored. This could result in degraded behavior for cluster-admins without clear indication. Recommendation:
Suggested change
This makes the fallback behavior explicit and logged. |
||||||||||||||||||||||||||||
| allNs, err := K8sClientProjects.CoreV1().Namespaces().List(ctx, v1.ListOptions{}) | ||||||||||||||||||||||||||||
| if err == nil { | ||||||||||||||||||||||||||||
| for _, ns := range allNs.Items { | ||||||||||||||||||||||||||||
| namespaces[ns.Name] = true | ||||||||||||||||||||||||||||
|
Comment on lines
+219
to
+235
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The fast path assumes every Useful? React with 👍 / 👎. |
||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| return namespaces, nil | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| // Normal path: find RoleBindings where user is a subject | ||||||||||||||||||||||||||||
| // This gives us the specific namespaces where user has permissions | ||||||||||||||||||||||||||||
| roleBindings, err := K8sClientProjects.RbacV1().RoleBindings("").List(ctx, v1.ListOptions{}) | ||||||||||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||||||||||
| return nil, fmt.Errorf("failed to list RoleBindings: %w", err) | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| // Extract namespaces from RoleBindings where user or their groups are subjects | ||||||||||||||||||||||||||||
| for _, rb := range roleBindings.Items { | ||||||||||||||||||||||||||||
| if subjectMatchesUser(rb.Subjects, userSubject, userGroups) { | ||||||||||||||||||||||||||||
| namespaces[rb.Namespace] = true | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
Comment on lines
+244
to
+255
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The new Useful? React with 👍 / 👎. |
||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| log.Printf("User %s has access to %d namespaces via RoleBindings", userSubject, len(namespaces)) | ||||||||||||||||||||||||||||
| return namespaces, nil | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| // subjectMatchesUser checks if any subject in the list matches the user or their groups | ||||||||||||||||||||||||||||
| func subjectMatchesUser(subjects []rbacv1.Subject, userSubject string, userGroups []string) bool { | ||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Correctness: Missing Nil Check for Subjects The function doesn't check if Recommendation:
Suggested change
This defensive check prevents potential panics. |
||||||||||||||||||||||||||||
| for _, subject := range subjects { | ||||||||||||||||||||||||||||
| // Check direct user/SA match | ||||||||||||||||||||||||||||
| if subject.Kind == "User" && subject.Name == userSubject { | ||||||||||||||||||||||||||||
| return true | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| if subject.Kind == "ServiceAccount" { | ||||||||||||||||||||||||||||
| // ServiceAccount subject format: system:serviceaccount:namespace:name | ||||||||||||||||||||||||||||
| saSubject := fmt.Sprintf("system:serviceaccount:%s:%s", subject.Namespace, subject.Name) | ||||||||||||||||||||||||||||
| if saSubject == userSubject { | ||||||||||||||||||||||||||||
| return true | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| // Check group membership | ||||||||||||||||||||||||||||
| if subject.Kind == "Group" { | ||||||||||||||||||||||||||||
| for _, userGroup := range userGroups { | ||||||||||||||||||||||||||||
| if subject.Name == userGroup { | ||||||||||||||||||||||||||||
| return true | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| return false | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| // projectFromNamespace converts a Kubernetes Namespace to AmbientProject | ||||||||||||||||||||||||||||
| // On OpenShift, extracts displayName and description from namespace annotations | ||||||||||||||||||||||||||||
| func projectFromNamespace(ns *corev1.Namespace, isOpenShift bool) types.AmbientProject { | ||||||||||||||||||||||||||||
|
|
@@ -740,7 +834,7 @@ | |||||||||||||||||||||||||||
| // This is the proper Kubernetes-native way - lets RBAC engine determine access from ALL sources | ||||||||||||||||||||||||||||
| // (RoleBindings, ClusterRoleBindings, groups, etc.) | ||||||||||||||||||||||||||||
| // Deprecated: Use checkUserCanViewProject or checkUserCanModifyProject instead | ||||||||||||||||||||||||||||
| func checkUserCanAccessNamespace(userClient *kubernetes.Clientset, namespace string) (bool, error) { | ||||||||||||||||||||||||||||
| // For backward compatibility, check if user can list agenticsessions | ||||||||||||||||||||||||||||
| return checkUserCanViewProject(userClient, namespace) | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 Testing Gap: No Unit Tests for Critical RBAC Logic
This new RBAC reverse-query logic is security-critical but lacks unit tests. The new functions
getUserAccessibleNamespaces()andsubjectMatchesUser()should have comprehensive test coverage.Recommendation:
Create
components/backend/handlers/projects_test.gowith tests covering:Example test structure: