refactor(security): implement secure two-tiered RBAC model #39
Conversation
- Add namespace-scoped RBAC creation in controller for provisioner jobs - Update documentation to reflect CRDs as single source of truth and job-based provisioning - Change Helm repository URLs to official Supabase charts - Update API status mappings and add new status constants - Fix CLI test mocks and update troubleshooting guides - Add new documentation guide and update development prerequisites BREAKING CHANGE: RBAC permissions now use a two-tiered model with namespace-scoped provisioner roles. Existing deployments must be updated to use the new ClusterRole and Role structure. See docs/security/ADVISORY-001-provisioner-rbac.md for details.
Summary of ChangesHello @michaelruelas, 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 significantly enhances the security and clarity of the SupaControl platform by introducing a robust two-tiered RBAC model for instance provisioning, ensuring strict isolation and least privilege. Accompanying these core architectural changes are comprehensive documentation updates that clarify the CRD-centric and job-based provisioning patterns, streamline Helm chart references, and refine API status reporting for asynchronous operations. This refactoring improves both the operational security and the overall understanding of the system's behavior. 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 is an excellent and substantial pull request that significantly improves the security and architecture of the project. The move to a two-tiered RBAC model is a critical security enhancement, and the adoption of a job-based provisioning pattern aligns the controller with Kubernetes best practices, improving reliability and observability. The documentation updates across the board are thorough and accurately reflect these major architectural changes. I've included one suggestion to further enhance tenant isolation by running provisioning jobs within the instance's own namespace.
| // createProvisionerRBAC creates the Role and RoleBinding for the provisioner Job | ||
| func (r *SupabaseInstanceReconciler) createProvisionerRBAC(ctx context.Context, instance *supacontrolv1alpha1.SupabaseInstance) error { | ||
| namespace := fmt.Sprintf("supa-%s", instance.Spec.ProjectName) | ||
|
|
||
| // Create namespace-scoped Role | ||
| role := &rbacv1.Role{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "supacontrol-provisioner", | ||
| Namespace: namespace, | ||
| }, | ||
| Rules: []rbacv1.PolicyRule{ | ||
| { | ||
| APIGroups: []string{""}, | ||
| Resources: []string{"secrets", "services", "configmaps", "pods", "serviceaccounts", "persistentvolumeclaims"}, | ||
| Verbs: []string{"create", "delete", "get", "list", "patch", "update", "watch"}, | ||
| }, | ||
| { | ||
| APIGroups: []string{"apps"}, | ||
| Resources: []string{"deployments", "statefulsets", "replicasets"}, | ||
| Verbs: []string{"create", "delete", "get", "list", "patch", "update", "watch"}, | ||
| }, | ||
| { | ||
| APIGroups: []string{"networking.k8s.io"}, | ||
| Resources: []string{"ingresses"}, | ||
| Verbs: []string{"create", "delete", "get", "list", "patch", "update", "watch"}, | ||
| }, | ||
| { | ||
| APIGroups: []string{"batch"}, | ||
| Resources: []string{"jobs"}, | ||
| Verbs: []string{"create", "delete", "get", "list", "watch"}, | ||
| }, | ||
| { | ||
| APIGroups: []string{"rbac.authorization.k8s.io"}, | ||
| Resources: []string{"roles", "rolebindings"}, | ||
| Verbs: []string{"create", "delete", "get", "list", "patch", "update", "watch"}, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| if err := r.Create(ctx, role); err != nil && !apierrors.IsAlreadyExists(err) { | ||
| return err | ||
| } | ||
|
|
||
| // Create RoleBinding | ||
| roleBinding := &rbacv1.RoleBinding{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "supacontrol-provisioner", | ||
| Namespace: namespace, | ||
| }, | ||
| RoleRef: rbacv1.RoleRef{ | ||
| APIGroup: "rbac.authorization.k8s.io", | ||
| Kind: "Role", | ||
| Name: "supacontrol-provisioner", | ||
| }, | ||
| Subjects: []rbacv1.Subject{ | ||
| { | ||
| Kind: "ServiceAccount", | ||
| Name: ServiceAccountName, | ||
| Namespace: ControllerNamespace, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| if err := r.Create(ctx, roleBinding); err != nil && !apierrors.IsAlreadyExists(err) { | ||
| return err | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
The current RBAC model is a significant improvement. However, for even stronger tenant isolation, the provisioning Job itself should be created and run within the instance's namespace (supa-<instance-name>) rather than the controller's namespace. This ensures that the entire provisioning process, including the pod executing the logic, is fully contained within the tenant's namespace, further reducing the potential blast radius of a compromised job.
This change would involve:
- Creating the
ServiceAccountfor the job within the instance's namespace. - Creating the
RoleandRoleBindingwithin the instance's namespace, binding the new in-namespaceServiceAccountto theRole. - Modifying the
createProvisioningJobandcreateCleanupJobfunctions (injob_helpers.go) to create theJobwithin the instance's namespace and specifying the new in-namespaceServiceAccountin its pod spec.
The suggested code refactors createProvisionerRBAC to create all necessary RBAC resources within the instance namespace. You would also need to update the job creation functions accordingly.
// createProvisionerRBAC creates the ServiceAccount, Role and RoleBinding for the provisioner Job
// inside the instance's own namespace for better isolation.
func (r *SupabaseInstanceReconciler) createProvisionerRBAC(ctx context.Context, instance *supacontrolv1alpha1.SupabaseInstance) error {
namespace := fmt.Sprintf("supa-%s", instance.Spec.ProjectName)
// 1. Create ServiceAccount in the instance namespace
sa := &corev1.ServiceAccount{
ObjectMeta: metav1.ObjectMeta{
Name: ServiceAccountName,
Namespace: namespace,
},
}
if err := r.Create(ctx, sa); err != nil && !apierrors.IsAlreadyExists(err) {
return fmt.Errorf("failed to create provisioner service account: %w", err)
}
// 2. Create namespace-scoped Role
role := &rbacv1.Role{
ObjectMeta: metav1.ObjectMeta{
Name: "supacontrol-provisioner",
Namespace: namespace,
},
Rules: []rbacv1.PolicyRule{
{
APIGroups: []string{""},
Resources: []string{"secrets", "services", "configmaps", "pods", "serviceaccounts", "persistentvolumeclaims"},
Verbs: []string{"create", "delete", "get", "list", "patch", "update", "watch"},
},
{
APIGroups: []string{"apps"},
Resources: []string{"deployments", "statefulsets", "replicasets"},
Verbs: []string{"create", "delete", "get", "list", "patch", "update", "watch"},
},
{
APIGroups: []string{"networking.k8s.io"},
Resources: []string{"ingresses"},
Verbs: []string{"create", "delete", "get", "list", "patch", "update", "watch"},
},
{
APIGroups: []string{"batch"},
Resources: []string{"jobs"},
Verbs: []string{"create", "delete", "get", "list", "watch"},
},
{
APIGroups: []string{"rbac.authorization.k8s.io"},
Resources: []string{"roles", "rolebindings"},
Verbs: []string{"create", "delete", "get", "list", "patch", "update", "watch"},
},
},
}
if err := r.Create(ctx, role); err != nil && !apierrors.IsAlreadyExists(err) {
return fmt.Errorf("failed to create provisioner role: %w", err)
}
// 3. Create RoleBinding in the instance namespace
roleBinding := &rbacv1.RoleBinding{
ObjectMeta: metav1.ObjectMeta{
Name: "supacontrol-provisioner",
Namespace: namespace,
},
RoleRef: rbacv1.RoleRef{
APIGroup: "rbac.authorization.k8s.io",
Kind: "Role",
Name: "supacontrol-provisioner",
},
Subjects: []rbacv1.Subject{
{
Kind: "ServiceAccount",
Name: ServiceAccountName,
Namespace: namespace, // Subject is now in the instance namespace
},
},
}
if err := r.Create(ctx, roleBinding); err != nil && !apierrors.IsAlreadyExists(err) {
return fmt.Errorf("failed to create provisioner role binding: %w", err)
}
return nil
}…olation - Update provisioning and cleanup jobs to operate within instance namespace - Create ServiceAccount, Role, and RoleBinding in instance namespace for better security - Modify job status retrieval to use dynamic namespace parameter - Remove namespace creation/deletion from job scripts and handle in controller - Delete instance namespace after successful cleanup job completion
BREAKING CHANGE: RBAC permissions now use a two-tiered model with namespace-scoped provisioner roles. Existing deployments must be updated to use the new ClusterRole and Role structure. See docs/security/ADVISORY-001-provisioner-rbac.md for details.
Pull Request
Description
Type of Change
Related Issues
Changes Made
Testing
Test Coverage
Test Results
Backend Tests:
Frontend Tests:
Manual Testing:
Screenshots (if applicable)
Deployment Notes
Deployment instructions:
# If applicable, provide deployment commandsDocumentation
Performance Impact
Performance notes:
Security Considerations
Security notes:
Breaking Changes
Breaking changes:
Migration guide:
# Provide instructions for users to migrateChecklist
Code Quality
gofmt(for Go code) ornpm run lint(for JavaScript)Testing
Documentation
Commit Standards
feat:,fix:,docs:)Dependencies
go.mod/package.jsonas neededCollaboration
Additional Notes
Review Checklist (for maintainers)
Thank you for contributing to SupaControl! 🎉
We appreciate your time and effort in improving the project. Our maintainers will review your PR as soon as possible.