revise: enhance RBAC settings #122
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #122 +/- ##
===========================================
+ Coverage 34.46% 52.33% +17.87%
===========================================
Files 7 7
Lines 560 684 +124
===========================================
+ Hits 193 358 +165
+ Misses 353 289 -64
- Partials 14 37 +23 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
navidshariaty
added
enhancement
Member
navidshariaty
left a comment
navidshariaty
left a comment
There was a problem hiding this comment.
please add tests to make sure these roles are added inside appproj
Member
|
Can you update the README too? In the new design we can define accesses and permissions into the AppProject instead of CSV? |
1995parham
requested changes
Nov 18, 2025
Contributor
Author
|
Sure, I'll update the mentioned issues, and add tests, then request for review again. |
debMan
force-pushed
the
revise/fix-RBAC-settings
branch
from
a7104d7 to
067d76a
Compare
Member
|
@debMan I think tests are failing. |
Contributor
Author
Yes, still working on this @1995parham jan. I'll inform you when completed |
debMan
force-pushed
the
revise/fix-RBAC-settings
branch
from
587a231 to
ea9f4a0
Compare
Contributor
Author
ArgoCD Complementary Operator - Code Review ReportBranch: Executive SummaryThis comprehensive code review analyzes the RBAC refactoring work on the Key Metrics
VerdictTable of Contents
Change SummaryModified FilesKey Improvements✅ Added finalizer pattern to ArgocdUser controller Critical Security Issues🔴 CRITICAL-1: Password Stored in Plain Text in CRD SpecCWE: CWE-256 (Plaintext Storage of a Password) Current Codetype ArgocdCIAdmin struct {
CIPass string `json:"ciPass,omitempty"` // ❌ Plain text password
Users []string `json:"users,omitempty"`
}
type ArgocdCIView struct {
CIPass string `json:"ciPass,omitempty"` // ❌ Plain text password
Users []string `json:"users,omitempty"`
}Security Impact
Recommended FixOption 1: Reference Kubernetes Secret (Recommended) type ArgocdCIAdmin struct {
CIPassSecretRef SecretReference `json:"ciPassSecretRef"` // ✅ Secret reference
Users []string `json:"users,omitempty"`
}
type SecretReference struct {
Name string `json:"name"`
Namespace string `json:"namespace,omitempty"` // Optional, default to ArgoCD namespace
Key string `json:"key"`
}Controller Update: func (r *ArgocdUserReconciler) getPasswordFromSecret(ctx context.Context, secretRef SecretReference) (string, error) {
secret := &corev1.Secret{}
namespace := secretRef.Namespace
if namespace == "" {
namespace = userArgocdNS
}
if err := r.Get(ctx, types.NamespacedName{
Name: secretRef.Name,
Namespace: namespace,
}, secret); err != nil {
return "", fmt.Errorf("failed to get secret: %w", err)
}
password, ok := secret.Data[secretRef.Key]
if !ok {
return "", fmt.Errorf("key %s not found in secret %s", secretRef.Key, secretRef.Name)
}
return string(password), nil
}Option 2: External Secrets Operator Integration type ArgocdCIAdmin struct {
CIPassExternalSecretRef ExternalSecretReference `json:"ciPassExternalSecretRef"`
Users []string `json:"users,omitempty"`
}Option 3: Webhook Admission Controller
🔴 CRITICAL-2: Error Swallowing in Password HashingCWE: CWE-391 (Unchecked Error Condition) Current Codehash, _ := HashPassword(ciPass) // ❌ ignore error for the sake of simplicity
encodedPass := b64.StdEncoding.EncodeToString([]byte(hash))Security Impact
Attack Scenario
Recommended Fix// ✅ Properly handle errors
hash, err := HashPassword(ciPass)
if err != nil {
log.Error(err, "Failed to hash password", "user", argocduser.Name, "role", roleName)
return fmt.Errorf("password hashing failed: %w", err)
}
// Validate hash is not empty
if hash == "" {
return fmt.Errorf("password hash is empty for user %s role %s", argocduser.Name, roleName)
}
encodedPass := b64.StdEncoding.EncodeToString([]byte(hash))
// Also validate password constraints
func HashPassword(password string) (string, error) {
if len(password) > 72 {
return "", fmt.Errorf("password exceeds bcrypt maximum length of 72 bytes")
}
if len(password) < 8 {
return "", fmt.Errorf("password must be at least 8 characters")
}
bytes, err := bcrypt.GenerateFromPassword([]byte(password), 14)
if err != nil {
return "", fmt.Errorf("bcrypt failed: %w", err)
}
return string(bytes), nil
}🔴 CRITICAL-3: Overly Permissive RBAC WildcardCWE: CWE-269 (Improper Privilege Management) Current Code- apiGroups:
- user.openshift.io
resources:
- '*' # ❌ Wildcard grants access to ALL OpenShift user resources
verbs:
- create
- delete
- get
- list
- patch
- update
- watchSecurity Impact
Resources Affected by Wildcard
Recommended Fix# ✅ Explicit resource type
- apiGroups:
- user.openshift.io
resources:
- groups # Only grant access to Groups
verbs:
- create
- delete
- get
- list
- patch
- update
- watchUpdate kubebuilder marker in controller: //+kubebuilder:rbac:groups=user.openshift.io,resources=groups,verbs=get;list;watch;create;update;patch;delete🔴 CRITICAL-4: String Concatenation in RBAC Policies (Injection Risk)CWE: CWE-74 (Improper Neutralization) Current CodePolicies: []string{
"p, proj:" + teamName + ":" + teamName + "-admin, applications, *, " + teamName + "/*, allow",
"p, proj:" + teamName + ":" + teamName + "-admin, repositories, *, " + teamName + "/*, allow",
"p, proj:" + teamName + ":" + teamName + "-admin, exec, create, " + teamName + "/*, allow",
// ❌ No validation of teamName - could contain policy injection characters
}Security Impact
Attack ScenarioapiVersion: argocd.snappcloud.io/v1alpha1
kind: ArgocdUser
metadata:
name: "evil,p,role:admin,*,*,*,allow" # ❌ Injects admin policy
spec:
admin:
ciPass: "password"
users: []Resulting policy: Recommended Fix// Add validation at CRD level
//+kubebuilder:validation:Pattern=`^[a-z0-9]([-a-z0-9]*[a-z0-9])?$`
//+kubebuilder:validation:MaxLength=63
type ArgocdUser struct {
metav1.TypeMeta `json:",inline"`
metav1.ObjectMeta `json:"metadata,omitempty"`
Spec ArgocdUserSpec `json:"spec,omitempty"`
}
// Add runtime validation
func validateTeamName(teamName string) error {
// DNS-1123 subdomain validation
if matched, _ := regexp.MatchString(`^[a-z0-9]([-a-z0-9]*[a-z0-9])?$`, teamName); !matched {
return fmt.Errorf("team name must be DNS-1123 compliant lowercase alphanumeric")
}
if len(teamName) > 63 {
return fmt.Errorf("team name exceeds 63 characters")
}
// Explicitly check for injection characters
if strings.ContainsAny(teamName, ",\n\r:") {
return fmt.Errorf("team name contains invalid characters")
}
return nil
}
// Use in reconciler
func (r *ArgocdUserReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
// ... get argocduser ...
if err := validateTeamName(argocduser.Name); err != nil {
log.Error(err, "Invalid team name", "name", argocduser.Name)
return ctrl.Result{}, err
}
// ... continue reconciliation ...
}
// Use template with validation
func buildPolicy(teamName, role, resource, action string) (string, error) {
if err := validateTeamName(teamName); err != nil {
return "", err
}
return fmt.Sprintf("p, proj:%s:%s-%s, %s, %s, %s/*, allow",
teamName, teamName, role, resource, action, teamName), nil
}🔴 CRITICAL-5: Missing OwnerReference ValidationCWE: CWE-476 (NULL Pointer Dereference potential) Current Codegroup = &userv1.Group{
ObjectMeta: metav1.ObjectMeta{
Name: groupName,
OwnerReferences: []metav1.OwnerReference{
{
APIVersion: "argocd.snappcloud.io/v1alpha1",
Kind: "ArgocdUser",
Name: argocduser.Name,
UID: argocduser.UID, // ❌ Could be empty if just created
Controller: ptr.To(true),
BlockOwnerDeletion: ptr.To(true),
},
},
},
Users: argoUsers,
}Security Impact
Recommended Fix// Validate OwnerReference before creating Group
if argocduser.UID == "" {
return fmt.Errorf("ArgocdUser UID is empty, resource may not be persisted yet")
}
// Use controllerutil for safety
group = &userv1.Group{
ObjectMeta: metav1.ObjectMeta{
Name: groupName,
},
Users: argoUsers,
}
// SetControllerReference handles validation
if err := controllerutil.SetControllerReference(argocduser, group, r.Scheme); err != nil {
log.Error(err, "Failed to set controller reference on Group")
return fmt.Errorf("failed to set controller reference: %w", err)
}
err = r.Create(ctx, group)🔴 CRITICAL-6: Missing AppProject Delete PermissionLocation: Current Code- apiGroups:
- argoproj.io
resources:
- appprojects
verbs:
- create
- get
- list
- patch
- update
- watch
# ❌ Missing 'delete' verbImpact
Recommended Fix- apiGroups:
- argoproj.io
resources:
- appprojects
verbs:
- create
- delete # ✅ Add delete permission
- get
- list
- patch
- update
- watchUpdate kubebuilder marker: //+kubebuilder:rbac:groups=argoproj.io,resources=appprojects,verbs=get;list;watch;create;update;patch;delete🔴 CRITICAL-7: Namespace List Permission MissingLocation: Current Code- apiGroups:
- ""
resources:
- namespaces
verbs:
- create
- delete
- get
- list # ✅ Present
- patch
- update
- watchAnalysisThis is actually correctly configured ✅. The controller has However, there's a security concern: Issue
Recommended Fix- apiGroups:
- ""
resources:
- namespaces
verbs:
- get # ✅ Needed for namespace lookups
- list # ✅ Needed for hasActiveNamespaces
- watch # ✅ Needed for namespace controller
# Remove: create, delete, patch, update (not used by controller)High Priority Issues🟡 HIGH-1: Context Mismatch in Password UpdateLocation: Issuefunc (r *ArgocdUserReconciler) UpdateUserArgocdConfig(ctx context.Context, ...) error {
// ... uses ctx correctly ...
err = r.Patch(context.Background(), &corev1.Secret{...}) // ❌ Wrong context
// ^^^^^^^^^^^^^^^^^^ Should be ctx
}Impact
Fixerr = r.Patch(ctx, &corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Namespace: userArgocdNS,
Name: userArgocdSecret,
},
}, client.RawPatch(types.StrategicMergePatchType, staticPassByte))🟡 HIGH-2: Test Isolation FailureLocation: Test OutputImpact
Root CauseTests use Recommended FixOption 1: Use unique names per test var _ = Describe("ArgocdUser controller", func() {
var testCounter int
Context("When creating an ArgocdUser resource", func() {
It("Should generate correct RBAC policies", func() {
testCounter++
teamName := fmt.Sprintf("test-team-%d", testCounter)
argocdUser := &argocduserv1alpha1.ArgocdUser{
ObjectMeta: metav1.ObjectMeta{
Name: teamName, // ✅ Unique per test
},
// ...
}
})
})
})Option 2: Proper cleanup in BeforeEach/AfterEach var _ = Describe("ArgocdUser controller", func() {
BeforeEach(func() {
// Clean up any existing test resources
testTeamUser := &argocduserv1alpha1.ArgocdUser{}
if err := k8sClient.Get(ctx, types.NamespacedName{Name: "test-team"}, testTeamUser); err == nil {
Expect(k8sClient.Delete(ctx, testTeamUser)).Should(Succeed())
// Wait for deletion to complete
Eventually(func() bool {
err := k8sClient.Get(ctx, types.NamespacedName{Name: "test-team"}, testTeamUser)
return errors.IsNotFound(err)
}, timeout, interval).Should(BeTrue())
}
})
})Option 3: Remove var _ = Describe("ArgocdUser controller", func() { // Remove Ordered
Context("When creating an ArgocdUser resource", func() {
It("Should generate correct RBAC policies", func() {
// Each test creates and cleans up its own resources
})
})
})🟡 HIGH-3: No Input Validation on User ListsLocation: Issuefunc (r *ArgocdUserReconciler) AddArgoUsersToGroup(ctx context.Context, argocduser *argocduserv1alpha1.ArgocdUser, roleName string, argoUsers []string) error {
// ❌ No validation of argoUsers contents
group.Users = argoUsers
}Security Impact
Recommended Fix// Add validation function
func validateUsernames(users []string) error {
for _, user := range users {
// OpenShift username validation (simplified)
if matched, _ := regexp.MatchString(`^[a-zA-Z0-9._@-]+$`, user); !matched {
return fmt.Errorf("invalid username: %s", user)
}
if len(user) > 255 {
return fmt.Errorf("username too long: %s", user)
}
}
return nil
}
func (r *ArgocdUserReconciler) AddArgoUsersToGroup(..., argoUsers []string) error {
// Validate input
if err := validateUsernames(argoUsers); err != nil {
return fmt.Errorf("invalid user list: %w", err)
}
// ... rest of function ...
}
// Also add to CRD
//+kubebuilder:validation:Pattern=`^[a-zA-Z0-9._@-]+$`
//+kubebuilder:validation:MaxLength=255
type ArgocdCIAdmin struct {
CIPass string `json:"ciPass,omitempty"`
Users []string `json:"users,omitempty"`
}🟡 HIGH-4: Race Condition in User MergingLocation: Issue// Merge users: keep existing users and add new ones
existingUsers := make(map[string]bool)
for _, user := range group.Users {
existingUsers[user] = true
}
for _, user := range argoUsers {
if !existingUsers[user] {
group.Users = append(group.Users, user) // ❌ No mutex
}
}
err = r.Update(ctx, group) // ❌ Potential race if concurrent reconcilesSecurity Impact
Scenario
Recommended FixOption 1: Use deterministic set operations // Merge users with set semantics and deterministic ordering
func mergeUsers(existing, new []string) []string {
userSet := make(map[string]struct{})
// Add existing users
for _, user := range existing {
userSet[user] = struct{}{}
}
// Add new users
for _, user := range new {
userSet[user] = struct{}{}
}
// Convert back to sorted slice for deterministic results
users := make([]string, 0, len(userSet))
for user := range userSet {
users = append(users, user)
}
sort.Strings(users) // ✅ Deterministic ordering
return users
}
// Use in controller
group.Users = mergeUsers(group.Users, argoUsers)
err = r.Update(ctx, group)Option 2: Use optimistic locking with retry err := retry.RetryOnConflict(retry.DefaultRetry, func() error {
// Get latest version
if err := r.Get(ctx, types.NamespacedName{Name: groupName}, group); err != nil {
return err
}
// Merge users
group.Users = mergeUsers(group.Users, argoUsers)
// Update with conflict detection
return r.Update(ctx, group)
})🟡 HIGH-5: Missing Idempotency in RBAC Policy AdditionLocation: Issueis_changed := false
for _, policy := range policies {
duplicatePolicy := false
for _, line := range strings.Split(found.Data["policy.csv"], "\n") {
if policy == line { // ❌ O(n*m) complexity
duplicatePolicy = true
}
}
if !duplicatePolicy {
found.Data["policy.csv"] = found.Data["policy.csv"] + "\n" + policy
is_changed = true
}
}Performance Impact
Scenario
Recommended Fix// ✅ Use map for O(1) lookups
func (r *ArgocdUserReconciler) AddArgocdRBACPolicy(ctx context.Context, argocduser *argocduserv1alpha1.ArgocdUser) error {
log := log.FromContext(ctx)
found := &corev1.ConfigMap{}
err := r.Get(ctx, types.NamespacedName{Name: userArgocdRbacPolicyCM, Namespace: userArgocdNS}, found)
if err != nil {
return err
}
policies := []string{
"p, role:common, clusters, get, *, allow",
"g, " + argocduser.Name + "-admin-ci, role:common",
"g, " + argocduser.Name + "-view-ci, role:common",
"g, " + argocduser.Name + "-admin, role:common",
"g, " + argocduser.Name + "-view, role:common",
}
// Parse existing policies into map - O(n)
existingLines := strings.Split(found.Data["policy.csv"], "\n")
policySet := make(map[string]struct{}, len(existingLines))
for _, line := range existingLines {
trimmed := strings.TrimSpace(line)
if trimmed != "" {
policySet[trimmed] = struct{}{}
}
}
// Add new policies - O(m)
newPolicies := []string{}
for _, policy := range policies {
if _, exists := policySet[policy]; !exists {
newPolicies = append(newPolicies, policy)
policySet[policy] = struct{}{} // Update set
}
}
// Only update if there are new policies
if len(newPolicies) > 0 {
// Rebuild policy.csv with all policies
allPolicies := make([]string, 0, len(policySet))
for policy := range policySet {
allPolicies = append(allPolicies, policy)
}
sort.Strings(allPolicies) // ✅ Deterministic ordering
found.Data["policy.csv"] = strings.Join(allPolicies, "\n")
if err := r.Update(ctx, found); err != nil {
log.Error(err, "error updating argocd-rbac-cm")
return err
}
log.Info("Added RBAC policies", "count", len(newPolicies))
} else {
log.Info("All RBAC policies already exist, skipping update")
}
return nil
}🟡 HIGH-6: Incomplete Cleanup LogicLocation: Issuefunc (r *ArgocdUserReconciler) cleanupArgocdUserResources(ctx context.Context, argocduser *argocduserv1alpha1.ArgocdUser) error {
// ...
for _, line := range lines {
// Keep lines that don't reference this team
if !strings.Contains(line, teamName+"-admin-ci") &&
!strings.Contains(line, teamName+"-view-ci") &&
!strings.Contains(line, teamName+"-admin") &&
!strings.Contains(line, teamName+"-view") {
newLines = append(newLines, line)
}
}
// ❌ What if teamName is substring of another team?
// e.g., "team" matches "my-team-admin"
}Security Impact
Attack Scenario# Attacker creates team with substring name
apiVersion: argocd.snappcloud.io/v1alpha1
kind: ArgocdUser
metadata:
name: "prod" # Substring of "production"
---
# When "prod" is deleted, it removes "production" team's policies too!Recommended Fixfunc (r *ArgocdUserReconciler) cleanupArgocdUserResources(ctx context.Context, argocduser *argocduserv1alpha1.ArgocdUser) error {
log := log.FromContext(ctx)
teamName := argocduser.Name
// ✅ Use regex with word boundaries for exact matching
teamPatterns := []*regexp.Regexp{
regexp.MustCompile(`\b` + regexp.QuoteMeta(teamName) + `-admin-ci\b`),
regexp.MustCompile(`\b` + regexp.QuoteMeta(teamName) + `-view-ci\b`),
regexp.MustCompile(`\b` + regexp.QuoteMeta(teamName) + `-admin\b`),
regexp.MustCompile(`\b` + regexp.QuoteMeta(teamName) + `-view\b`),
}
// Remove RBAC policies
rbacConfigMap := &corev1.ConfigMap{}
if err := r.Get(ctx, types.NamespacedName{Name: userArgocdRbacPolicyCM, Namespace: userArgocdNS}, rbacConfigMap); err != nil {
if !errors.IsNotFound(err) {
return err
}
} else {
policyCsv := rbacConfigMap.Data["policy.csv"]
lines := strings.Split(policyCsv, "\n")
var newLines []string
for _, line := range lines {
shouldRemove := false
for _, pattern := range teamPatterns {
if pattern.MatchString(line) {
shouldRemove = true
log.Info("Removing policy line", "line", line)
break
}
}
if !shouldRemove && strings.TrimSpace(line) != "" {
newLines = append(newLines, line)
}
}
rbacConfigMap.Data["policy.csv"] = strings.Join(newLines, "\n")
if err := r.Update(ctx, rbacConfigMap); err != nil {
return fmt.Errorf("failed to update argocd-rbac-cm: %w", err)
}
}
// ... rest of cleanup ...
}Medium Priority Issues🟢 MEDIUM-1: TODO Comment for 4+ MonthsLocation: // TODO: Enhance this, for example adding view roles to the admin not works!IssueThis TODO has been in the codebase and indicates a known issue with role aggregation that hasn't been addressed. Current BehaviorThe comment suggests that admin users might not be inheriting view permissions properly, which could be a bug. AnalysisLooking at the current implementation (lines 319-326), the role aggregation IS implemented: {
// View role includes both admin and view groups for role aggregation
Groups: []string{teamName + "-admin", teamName + "-admin-ci", teamName + "-view", teamName + "-view-ci"},
Name: teamName + "-view",
// ...
}Recommendation
🟢 MEDIUM-2: No Metrics or ObservabilityIssue: No Prometheus metrics for monitoring reconciliation health Recommended Metricsimport (
"github.com/prometheus/client_golang/prometheus"
"sigs.k8s.io/controller-runtime/pkg/metrics"
)
var (
reconcileTotal = prometheus.NewCounterVec(
prometheus.CounterOpts{
Name: "argocduser_reconcile_total",
Help: "Total number of reconciliations",
},
[]string{"controller", "result"},
)
reconcileDuration = prometheus.NewHistogramVec(
prometheus.HistogramOpts{
Name: "argocduser_reconcile_duration_seconds",
Help: "Reconciliation duration in seconds",
},
[]string{"controller"},
)
argocdUserTotal = prometheus.NewGauge(
prometheus.GaugeOpts{
Name: "argocduser_total",
Help: "Total number of ArgocdUser resources",
},
)
)
func init() {
metrics.Registry.MustRegister(reconcileTotal, reconcileDuration, argocdUserTotal)
}
// Use in reconciler
func (r *ArgocdUserReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
start := time.Now()
defer func() {
reconcileDuration.WithLabelValues("argocduser").Observe(time.Since(start).Seconds())
}()
// ... reconciliation logic ...
reconcileTotal.WithLabelValues("argocduser", "success").Inc()
return ctrl.Result{}, nil
}🟢 MEDIUM-3: No Logging of Security EventsIssue: Security-relevant events (password changes, group modifications) not logged Recommended Fix// Add audit logging
func (r *ArgocdUserReconciler) UpdateUserArgocdConfig(...) error {
// Log security event
log.Info("SECURITY: Updating static user password",
"team", argocduser.Name,
"role", roleName,
"timestamp", time.Now().UTC(),
)
// ... existing code ...
}
func (r *ArgocdUserReconciler) AddArgoUsersToGroup(...) error {
// Log group membership changes
log.Info("SECURITY: Modifying group membership",
"group", groupName,
"team", argocduser.Name,
"users", argoUsers,
"timestamp", time.Now().UTC(),
)
// ... existing code ...
}🟢 MEDIUM-4: JSON Marshaling Error IgnoredLocation: staticPassByte, _ := json.Marshal(staticPassword)
// ❌ Error ignoredFixstaticPassByte, err := json.Marshal(staticPassword)
if err != nil {
log.Error(err, "Failed to marshal static password data")
return fmt.Errorf("JSON marshaling failed: %w", err)
}🟢 MEDIUM-5: No Rate Limiting on ReconciliationIssue: Rapid updates to ArgocdUser could cause reconciliation storm Recommended Fixfunc (r *ArgocdUserReconciler) SetupWithManager(mgr ctrl.Manager) error {
return ctrl.NewControllerManagedBy(mgr).
For(&argocduserv1alpha1.ArgocdUser{}).
WithOptions(controller.Options{
MaxConcurrentReconciles: 3, // ✅ Limit concurrent reconciles
RateLimiter: workqueue.NewItemExponentialFailureRateLimiter(
time.Second, // Base delay
time.Minute * 5, // Max delay
),
}).
Complete(r)
}🟢 MEDIUM-6: ConfigMap Data Not ValidatedLocation: if configMap.Data == nil {
configMap.Data = make(map[string]string)
}
// ❌ But what if Data is not nil but policy.csv is missing?Fixif configMap.Data == nil {
configMap.Data = make(map[string]string)
}
if _, exists := configMap.Data["policy.csv"]; !exists {
configMap.Data["policy.csv"] = "" // ✅ Initialize if missing
}🟢 MEDIUM-7: No Webhook ValidationIssue: CRD validation happens only at API level, not business logic Recommended Implementation// Create validating webhook
//+kubebuilder:webhook:path=/validate-argocd-snappcloud-io-v1alpha1-argocduser,mutating=false,failurePolicy=fail,groups=argocd.snappcloud.io,resources=argocdusers,verbs=create;update,versions=v1alpha1,name=vargocduser.kb.io,sideEffects=None,admissionReviewVersions=v1
func (r *ArgocdUser) ValidateCreate() error {
// Validate team name
if err := validateTeamName(r.Name); err != nil {
return err
}
// Warn about plain text passwords (or reject)
if r.Spec.Admin.CIPass != "" {
return fmt.Errorf("plain text passwords not allowed, use ciPassSecretRef")
}
// Validate users
allUsers := append(r.Spec.Admin.Users, r.Spec.View.Users...)
if err := validateUsernames(allUsers); err != nil {
return err
}
return nil
}🟢 MEDIUM-8: No Status Subresource UsedIssue: ArgocdUser has status subresource but it's never populated Recommendationtype ArgocdUserStatus struct {
Conditions []metav1.Condition `json:"conditions,omitempty"`
Phase string `json:"phase,omitempty"`
LastReconcileTime metav1.Time `json:"lastReconcileTime,omitempty"`
}
// Update in reconciler
func (r *ArgocdUserReconciler) Reconcile(...) (ctrl.Result, error) {
// ... reconciliation ...
// Update status
argocduser.Status.Phase = "Ready"
argocduser.Status.LastReconcileTime = metav1.Now()
argocduser.Status.Conditions = []metav1.Condition{
{
Type: "Ready",
Status: metav1.ConditionTrue,
Reason: "ReconcileSuccess",
Message: "ArgocdUser reconciled successfully",
},
}
if err := r.Status().Update(ctx, argocduser); err != nil {
log.Error(err, "Failed to update status")
}
return ctrl.Result{}, nil
}Architectural Improvements✅ Excellent: Separation of ConcernsThe refactoring properly separates responsibilities: ArgocdUserReconciler (RBAC Management):
NamespaceReconciler (Namespace Management):
This is excellent architectural design ✅ following the Single Responsibility Principle. ✅ Excellent: Finalizer PatternThe implementation of finalizers with dependency checking is exemplary: // Check if there are namespaces still referencing this ArgocdUser
hasActive, err := r.hasActiveNamespaces(ctx, argocduser.Name)
if hasActive {
log.Info("Cannot delete ArgocdUser: namespaces still reference it")
return ctrl.Result{Requeue: true}, nil
}This prevents orphaned namespaces and cascading failures. Well done! ✅ ✅ Excellent: Role AggregationThe role aggregation pattern where admin users inherit view permissions is well-implemented: {
// View role includes both admin and view groups for role aggregation
Groups: []string{teamName + "-admin", teamName + "-admin-ci", teamName + "-view", teamName + "-view-ci"},
Name: teamName + "-view",
Policies: []string{
"p, proj:" + teamName + ":" + teamName + "-view, applications, get, " + teamName + "/*, allow",
"p, proj:" + teamName + ":" + teamName + "-view, repositories, get, " + teamName + "/*, allow",
"p, proj:" + teamName + ":" + teamName + "-view, logs, get, " + teamName + "/*, allow",
},
}This follows Kubernetes RBAC best practices ✅
|
| Component | Estimated Coverage | Priority |
|---|---|---|
argocduser_controller.go |
~65% | 🔴 Need error paths |
namespace_controller.go |
~70% | 🟡 Add multi-team tests |
| Cleanup functions | ~0% | 🔴 Critical gap |
| Error handlers | ~10% | 🔴 Critical gap |
Test Quality Assessment
Strengths ✅
- 943 lines of new tests for ArgocdUser controller
- 476 lines of new tests for Namespace controller
- Tests cover RBAC policy generation
- Tests verify AppProject creation
- Tests validate group membership
- Tests check multi-team scenarios
Weaknesses ❌
- No tests for cleanup (
cleanupArgocdUserResourceshas 0% coverage) - No tests for error paths (what happens when bcrypt fails?)
- No tests for finalizer edge cases (deletion with active namespaces)
- No tests for concurrent reconciliation
- No tests for invalid input (malicious team names, injection attempts)
- No integration tests with real ArgoCD instance
Recommended Additional Tests
// Test error handling
var _ = Describe("ArgocdUser error handling", func() {
It("Should handle bcrypt failure gracefully", func() {
// Test with password > 72 bytes
argocdUser := &argocduserv1alpha1.ArgocdUser{
Spec: argocduserv1alpha1.ArgocdUserSpec{
Admin: argocduserv1alpha1.ArgocdCIAdmin{
CIPass: strings.Repeat("a", 100), // ❌ Too long
},
},
}
// Expect reconciliation to fail with clear error
})
It("Should validate team name for injection", func() {
argocdUser := &argocduserv1alpha1.ArgocdUser{
ObjectMeta: metav1.ObjectMeta{
Name: "evil,admin", // ❌ Contains comma
},
}
Expect(k8sClient.Create(ctx, argocdUser)).Should(Not(Succeed()))
})
})
// Test cleanup
var _ = Describe("ArgocdUser cleanup", func() {
It("Should remove all RBAC policies on deletion", func() {
// Create ArgocdUser
// Verify policies exist
// Delete ArgocdUser
// Verify policies removed
})
It("Should prevent deletion with active namespaces", func() {
// Create ArgocdUser
// Create namespace with label
// Try to delete ArgocdUser
// Expect deletion to be blocked by finalizer
})
})
// Test concurrent reconciliation
var _ = Describe("ArgocdUser concurrent operations", func() {
It("Should handle concurrent group updates", func() {
// Create multiple ArgocdUsers updating same group
// Verify no race conditions or duplicates
})
})Recommendations
Immediate Actions (Before Merge) 🔴
-
FIX CRITICAL-1: Migrate passwords from plain text to Secret references
- Add
ciPassSecretReffield - Deprecate
ciPassfield - Add migration guide
- Add
-
FIX CRITICAL-2: Handle bcrypt errors properly
- Remove
_, _error ignoring - Add proper error handling and logging
- Validate password constraints
- Remove
-
FIX CRITICAL-3: Replace RBAC wildcard with explicit
groupsresource- Update
config/rbac/role.yaml - Update kubebuilder markers
- Run
make manifests
- Update
-
FIX CRITICAL-4: Validate team names for injection
- Add regex validation
- Add kubebuilder validation markers
- Add runtime validation
-
FIX CRITICAL-6: Add
deleteverb for appprojects- Update RBAC manifest
- Test garbage collection works
-
FIX HIGH-2: Fix test isolation issue
- Use unique names or proper cleanup
- Ensure tests can run in any order
Short Term (Next Sprint) 🟡
- Add input validation for usernames and team names
- Fix context.Background() usage - use ctx parameter
- Add comprehensive error path testing
- Implement race condition protection for group updates
- Add Prometheus metrics for observability
- Improve cleanup logic to use regex with word boundaries
- Add security event logging for audit trail
- Implement rate limiting on reconciliation
Long Term (Technical Debt) 🟢
-
Migrate to External Secrets Operator
- Full integration with Vault, AWS Secrets Manager, etc.
- Remove plain text password support entirely
-
Implement Webhook Admission Controller
- Validate ArgocdUser at admission time
- Reject invalid configurations early
- Add mutating webhook for defaults
-
Add Integration Tests
- Test with real ArgoCD instance
- Verify RBAC enforcement works
- Test end-to-end user flows
-
Performance Optimization
- Cache namespace lookups
- Batch policy updates
- Optimize string operations
-
Add OpenTelemetry Tracing
- Distributed tracing for debugging
- Correlate logs across reconciliations
- Performance profiling
-
Security Hardening
- Add OPA/Kyverno policies
- Implement pod security standards
- Regular security scanning (Snyk, Trivy)
Compliance & Standards
✅ Follows:
- ✅ Kubernetes Operator best practices
- ✅ Controller-runtime patterns
- ✅ Ginkgo/Gomega testing conventions
- ✅ Kubebuilder markers and scaffolding
- ✅ Go coding standards (mostly)
- ✅ Semantic versioning
⚠️ Partially Follows:
⚠️ OWASP Top 10 (violations in A01, A02, A03)⚠️ CIS Kubernetes Benchmark⚠️ Error handling best practices
❌ Missing:
- ❌ Security Policy as Code (OPA/Kyverno policies)
- ❌ SBOM (Software Bill of Materials)
- ❌ Container image scanning in CI/CD
- ❌ Secret detection in CI/CD (GitGuardian, TruffleHog)
- ❌ SAST tools (Semgrep, CodeQL)
- ❌ Dependency vulnerability scanning
OWASP Top 10 2025 Compliance
| Category | Status | Issues |
|---|---|---|
| A01: Broken Access Control | 🔴 FAIL | Wildcard RBAC, missing validation |
| A02: Cryptographic Failures | 🔴 FAIL | Plain text passwords |
| A03: Injection | 🔴 FAIL | Policy injection via team name |
| A04: Insecure Design | 🟡 PARTIAL | Missing webhook validation |
| A05: Security Misconfiguration | 🟡 PARTIAL | Overly permissive defaults |
| A06: Vulnerable Components | 🟢 PASS | Dependencies seem up to date |
| A07: Authentication Failures | 🟡 PARTIAL | Password validation weak |
| A08: Data Integrity Failures | 🟢 PASS | Using bcrypt correctly |
| A09: Logging Failures | 🟡 PARTIAL | Missing security event logs |
| A10: SSRF | 🟢 PASS | Not applicable |
Summary & Verdict
Overall Assessment: ⚠️ DO NOT MERGE YET
The RBAC refactoring shows excellent architectural improvements with proper separation of concerns, finalizer patterns, and comprehensive test coverage. However, 7 critical security vulnerabilities must be addressed before merging to production.
Strengths 💪
- ✅ Excellent separation of concerns between controllers
- ✅ Comprehensive test suite (943 + 476 new test lines)
- ✅ Proper finalizer implementation with dependency checking
- ✅ Role aggregation following Kubernetes best practices
- ✅ Multi-team support with thoughtful design
- ✅ Code builds successfully
Critical Concerns 🚨
- 🔴 Plain text passwords in CRD (CRITICAL)
- 🔴 Error swallowing in cryptography (CRITICAL)
- 🔴 RBAC wildcard permissions (CRITICAL)
- 🔴 Policy injection vulnerability (CRITICAL)
- 🔴 Missing AppProject delete permission
- 🔴 Test isolation failures
- 🔴 Race conditions in concurrent updates
Risk Assessment
| Risk Category | Level | Impact |
|---|---|---|
| Security | 🔴 HIGH | Multiple critical vulnerabilities |
| Reliability | 🟡 MEDIUM | Test failures, race conditions |
| Performance | 🟢 LOW | Some O(n²) algorithms |
| Maintainability | 🟢 LOW | Generally clean code |
Next Steps
For Code Author
- Review and address all CRITICAL issues
- Fix test isolation problem
- Add error path test coverage
- Update RBAC manifests
- Run
make manifeststo regenerate CRDs
For Code Reviewers
- Verify all CRITICAL issues are resolved
- Check test coverage improvements
- Validate RBAC changes
- Approve only after all issues addressed
For DevOps/Platform Team
- Plan password migration strategy
- Set up secret management infrastructure
- Enable security scanning in CI/CD
- Review cluster RBAC configuration
Appendix A: Testing Commands
# Run tests with coverage
KUBEBUILDER_ASSETS="./bin/k8s/1.29.0-darwin-arm64" \
go test -v -coverprofile=coverage.out ./internal/controller/...
# View coverage report
go tool cover -html=coverage.out
# Run specific test
KUBEBUILDER_ASSETS="./bin/k8s/1.29.0-darwin-arm64" \
go test -v ./internal/controller/ -run TestArgocdUser
# Run tests in parallel
KUBEBUILDER_ASSETS="./bin/k8s/1.29.0-darwin-arm64" \
go test -v -parallel=4 ./...
# Generate RBAC manifests
make manifests
# Build operator
make build
# Run linters
golangci-lint run
# Security scanning (if available)
govulncheck ./...Appendix B: References
Security Resources
Kubernetes Resources
Testing Resources
Review Completed: November 26, 2025
Reviewed By: AI Code Review Specialist (Claude 4.5 Sonnet)
Next Review: After addressing critical issues
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR implements comprehensive lifecycle management for the ArgoCD Complementary Operator, including automatic garbage collection, deletion prevention, and multi-team namespace support. The implementation follows TDD practices with extensive test coverage.
🎯 Features Added
1. OwnerReferences for Automatic Garbage Collection
controller=true,blockOwnerDeletion=true)2. Finalizers for Safe Deletion
argocd.snappcloud.io/finalizer) added to all ArgocdUsersargocd-rbac-cmargocd-cmargocd-secret3. Multi-Team Namespace Support with Deletion Prevention
team-a.team-b)🧪 Test Coverage
New Tests Added (11 tests total)
OwnerReference Tests (4 tests):
Finalizer & Deletion Tests (5 tests):
Multi-Team Validation Tests (2 tests):
Test Results:
🧹 Code Quality Improvements
Removed Dead Code
createAppProjmethod (75 lines) from NamespaceReconcilerisTeamClusterAdminhelper functionLinting Fixes
k8s.io/utils/pointerwithk8s.io/utils/ptrTest Isolation
rbac-gen-testname instead of conflictingtest-team📚 Documentation
README Updates
Added two new comprehensive sections:
Deletion and Lifecycle Management:
Architecture:
🔧 Architecture Changes
ArgocdUserReconciler Responsibilities
NamespaceReconciler Responsibilities
🎨 Code Changes Summary
✅ Verification
🔄 Migration Notes
No action required for existing deployments. The changes are backward compatible: