From bedbff13493ab6944e40dc7c3566d00a5de75ecc Mon Sep 17 00:00:00 2001 From: Tai Groot Date: Mon, 30 Mar 2026 10:04:19 +0000 Subject: [PATCH] feat(auth): enforce username uniqueness at startup and config reload Add ValidateUsernameUniqueness() to reject farmer configs where two or more pubkeys share the same username. Duplicate usernames would make audit logs ambiguous and prevent reliable identification. Validation runs in LoadPolicy() alongside the existing pubkey uniqueness check, catching duplicates both at farmer startup and on SIGHUP config reload. Empty/missing usernames are ignored (only explicitly set usernames are checked). - Add ErrDuplicateUsername sentinel error - Add ValidateUsernameUniqueness() in internal/rbac/config.go - Wire into auth.LoadPolicy() in internal/auth/auth.go - Add 6 unit tests for the validator (cross-role, same-role, empty, mixed, no config) - Add 2 integration tests for LoadPolicy (reject/accept) --- internal/auth/auth.go | 6 ++ internal/auth/jety_test.go | 44 ++++++++++++++ internal/rbac/config.go | 48 +++++++++++++++ internal/rbac/config_test.go | 110 +++++++++++++++++++++++++++++++++++ 4 files changed, 208 insertions(+) diff --git a/internal/auth/auth.go b/internal/auth/auth.go index 69911e1..21fca49 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -46,6 +46,12 @@ func LoadPolicy() error { return err } + // Validate username uniqueness — reject configs where two pubkeys + // share the same username (ambiguous audit logs). + if err := rbac.ValidateUsernameUniqueness(); err != nil { + return err + } + var err error roleStore, err = rbac.LoadRolesFromConfig() if err != nil { diff --git a/internal/auth/jety_test.go b/internal/auth/jety_test.go index fd8068b..c9eadff 100644 --- a/internal/auth/jety_test.go +++ b/internal/auth/jety_test.go @@ -3,6 +3,7 @@ package auth import ( "os" "path/filepath" + "strings" "testing" "github.com/nats-io/nkeys" @@ -590,6 +591,49 @@ func TestLoadPolicyWithCohorts(t *testing.T) { } } +func TestLoadPolicyRejectsDuplicateUsernames(t *testing.T) { + setupJetyForTest(t) + defer clearJetyKeys(t) + defer SetPolicy(nil, nil, nil) + + jety.Set("users", map[string]interface{}{ + "admin": []interface{}{ + map[string]interface{}{"pubkey": "KEY_A", "username": "alice"}, + }, + "viewer": []interface{}{ + map[string]interface{}{"pubkey": "KEY_B", "username": "alice"}, + }, + }) + + err := LoadPolicy() + if err == nil { + t.Fatal("expected LoadPolicy to reject config with duplicate usernames") + } + if !strings.Contains(err.Error(), "duplicate username") { + t.Errorf("error should mention duplicate username, got: %v", err) + } +} + +func TestLoadPolicyAcceptsUniqueUsernames(t *testing.T) { + setupJetyForTest(t) + defer clearJetyKeys(t) + defer SetPolicy(nil, nil, nil) + + jety.Set("users", map[string]interface{}{ + "admin": []interface{}{ + map[string]interface{}{"pubkey": "KEY_A", "username": "alice"}, + }, + "viewer": []interface{}{ + map[string]interface{}{"pubkey": "KEY_B", "username": "bob"}, + }, + }) + + err := LoadPolicy() + if err != nil { + t.Fatalf("LoadPolicy should accept unique usernames, got: %v", err) + } +} + // --- DangerouslyAllowRoot bypass with jety --- func TestDangerouslyAllowRootEnabled(t *testing.T) { diff --git a/internal/rbac/config.go b/internal/rbac/config.go index c3e2893..cfe780c 100644 --- a/internal/rbac/config.go +++ b/internal/rbac/config.go @@ -13,6 +13,10 @@ import ( // multiple roles in the users/pubkeys config sections. var ErrDuplicatePubkey = errors.New("duplicate pubkey assignment") +// ErrDuplicateUsername is returned when two or more pubkeys share the +// same username in the users config section. +var ErrDuplicateUsername = errors.New("duplicate username") + // RoleStore holds all custom role definitions loaded from config. type RoleStore struct { roles map[string]*Role @@ -385,6 +389,50 @@ func ValidateUserUniqueness() error { ErrDuplicatePubkey, strings.Join(dupes, "\n")) } +// ValidateUsernameUniqueness checks the "users" config section for +// entries where two or more pubkeys share the same username. Duplicate +// usernames would make audit logs ambiguous and prevent reliable +// identification. Returns an error listing every conflicting username and +// the pubkeys that claim it. Empty/missing usernames are ignored. +func ValidateUsernameUniqueness() error { + seen := make(map[string][]string) // username → list of pubkeys + + usersMap := jety.GetStringMap("users") + for _, v := range usersMap { + entries := parseUserEntries(v) + for _, e := range entries { + if e.Username == "" { + continue + } + seen[e.Username] = append(seen[e.Username], e.Pubkey) + } + } + + var dupes []string + for username, pubkeys := range seen { + if len(pubkeys) > 1 { + sort.Strings(pubkeys) + // Truncate pubkeys for readability. + truncated := make([]string, len(pubkeys)) + for i, pk := range pubkeys { + if len(pk) > 16 { + truncated[i] = pk[:16] + "..." + } else { + truncated[i] = pk + } + } + dupes = append(dupes, fmt.Sprintf(" %q → [%s]", username, strings.Join(truncated, ", "))) + } + } + if len(dupes) == 0 { + return nil + } + + sort.Strings(dupes) + return fmt.Errorf("%w: the following usernames are assigned to multiple pubkeys:\n%s", + ErrDuplicateUsername, strings.Join(dupes, "\n")) +} + // LoadCohortsFromConfig reads the "cohorts" section from the farmer config // (via jety) and returns a populated Registry. It does not fail on an // empty or missing cohorts section — it simply returns an empty registry. diff --git a/internal/rbac/config_test.go b/internal/rbac/config_test.go index e3c6a92..0e62625 100644 --- a/internal/rbac/config_test.go +++ b/internal/rbac/config_test.go @@ -1,8 +1,10 @@ package rbac import ( + "errors" "os" "path/filepath" + "strings" "testing" "github.com/taigrr/jety" @@ -678,6 +680,114 @@ func TestValidateUserUniqueness_CrossSectionJety(t *testing.T) { } } +func TestValidateUsernameUniqueness_NoDuplicates(t *testing.T) { + setupJetyForRBACTest(t) + defer clearJetyRBACKeys(t) + + jety.Set("users", map[string]any{ + "admin": []any{ + map[string]any{"pubkey": "KEY_A", "username": "alice"}, + map[string]any{"pubkey": "KEY_B", "username": "bob"}, + }, + }) + + err := ValidateUsernameUniqueness() + if err != nil { + t.Errorf("expected no error for unique usernames, got: %v", err) + } +} + +func TestValidateUsernameUniqueness_DetectsDuplicate(t *testing.T) { + setupJetyForRBACTest(t) + defer clearJetyRBACKeys(t) + + jety.Set("users", map[string]any{ + "admin": []any{ + map[string]any{"pubkey": "KEY_A", "username": "alice"}, + }, + "viewer": []any{ + map[string]any{"pubkey": "KEY_B", "username": "alice"}, + }, + }) + + err := ValidateUsernameUniqueness() + if err == nil { + t.Fatal("expected error for duplicate username 'alice' across roles") + } + if !errors.Is(err, ErrDuplicateUsername) { + t.Errorf("expected ErrDuplicateUsername, got: %v", err) + } + if !strings.Contains(err.Error(), "alice") { + t.Errorf("error should mention 'alice', got: %v", err) + } +} + +func TestValidateUsernameUniqueness_SameRoleDuplicate(t *testing.T) { + setupJetyForRBACTest(t) + defer clearJetyRBACKeys(t) + + jety.Set("users", map[string]any{ + "admin": []any{ + map[string]any{"pubkey": "KEY_A", "username": "bob"}, + map[string]any{"pubkey": "KEY_C", "username": "bob"}, + }, + }) + + err := ValidateUsernameUniqueness() + if err == nil { + t.Fatal("expected error for duplicate username within same role") + } + if !errors.Is(err, ErrDuplicateUsername) { + t.Errorf("expected ErrDuplicateUsername, got: %v", err) + } +} + +func TestValidateUsernameUniqueness_EmptyUsernamesIgnored(t *testing.T) { + setupJetyForRBACTest(t) + defer clearJetyRBACKeys(t) + + // Two pubkeys without usernames should not conflict. + jety.Set("users", map[string]any{ + "admin": []any{"KEY_A"}, + "viewer": []any{"KEY_B"}, + }) + + err := ValidateUsernameUniqueness() + if err != nil { + t.Errorf("expected no error when usernames are empty, got: %v", err) + } +} + +func TestValidateUsernameUniqueness_MixedWithAndWithoutUsernames(t *testing.T) { + setupJetyForRBACTest(t) + defer clearJetyRBACKeys(t) + + jety.Set("users", map[string]any{ + "admin": []any{ + map[string]any{"pubkey": "KEY_A", "username": "alice"}, + "KEY_B", // No username — should not conflict. + }, + "viewer": []any{ + map[string]any{"pubkey": "KEY_C", "username": "bob"}, + }, + }) + + err := ValidateUsernameUniqueness() + if err != nil { + t.Errorf("expected no error for unique usernames with some empty, got: %v", err) + } +} + +func TestValidateUsernameUniqueness_EmptyConfig(t *testing.T) { + setupJetyForRBACTest(t) + defer clearJetyRBACKeys(t) + + err := ValidateUsernameUniqueness() + if err != nil { + t.Errorf("expected no error for empty config, got: %v", err) + } +} + func TestLoadCohortsFromConfig_Empty(t *testing.T) { setupJetyForRBACTest(t) defer clearJetyRBACKeys(t)