From a611bf8d71b4550ee6e5a7669a499e33e81a88af Mon Sep 17 00:00:00 2001 From: Martin Anker Have Date: Thu, 12 Mar 2026 08:15:35 +0100 Subject: [PATCH 1/2] test: reproduce cross-namespace role revocation bug When the same PostgreSQL user has PostgreSQLUser resources in multiple namespaces pointing to the same host, each reconciliation revokes roles granted by other namespaces because it only computes expected roles from its own namespace's databases. Co-Authored-By: Claude Opus 4.6 --- pkg/grants/cross_namespace_revoke_test.go | 245 ++++++++++++++++++++ pkg/postgres/cross_namespace_revoke_test.go | 120 ++++++++++ 2 files changed, 365 insertions(+) create mode 100644 pkg/grants/cross_namespace_revoke_test.go create mode 100644 pkg/postgres/cross_namespace_revoke_test.go diff --git a/pkg/grants/cross_namespace_revoke_test.go b/pkg/grants/cross_namespace_revoke_test.go new file mode 100644 index 0000000..964967e --- /dev/null +++ b/pkg/grants/cross_namespace_revoke_test.go @@ -0,0 +1,245 @@ +package grants + +import ( + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + lunarwayv1alpha1 "go.lunarway.com/postgresql-controller/api/v1alpha1" + "go.lunarway.com/postgresql-controller/pkg/postgres" + "go.lunarway.com/postgresql-controller/test" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +// TestCrossNamespaceRevocation_KrviScenario reproduces the real-world issue +// from krvi.yaml where the same user (krvi) has PostgreSQLUser resources in +// multiple namespaces, and two of them (auth and openbanking) point to the same +// PostgreSQL host (auth-rds.hydra.svc.cluster.local.). +// +// Each namespace's reconciliation independently computes desired roles from +// only its own databases. When rolesDiff runs, it revokes any existing roles +// not in the expected list — including roles granted by the other namespace. +func TestCrossNamespaceRevocation_KrviScenario(t *testing.T) { + sharedHost := "auth-rds.hydra.svc.cluster.local." + + // Databases known in the "auth" namespace on the shared host. + authNamespaceDatabases := []lunarwayv1alpha1.PostgreSQLDatabase{ + runningDatabase(sharedHost, "hydra"), + runningDatabase(sharedHost, "consent"), + } + + // Databases known in the "openbanking" namespace on the shared host. + openbankingNamespaceDatabases := []lunarwayv1alpha1.PostgreSQLDatabase{ + runningDatabase(sharedHost, "obie_connect"), + runningDatabase(sharedHost, "obie_payment"), + } + + // --- Step 1: groupAccesses for auth namespace --- + authGranter := newTestGranter(authNamespaceDatabases) + authAccesses, err := authGranter.groupAccesses( + test.NewLogger(t), "auth", + []lunarwayv1alpha1.AccessSpec{ + { + Host: lunarwayv1alpha1.ResourceVar{Value: sharedHost}, + AllDatabases: &trueValue, + Reason: "I am a developer in squad Void who maintains Hydra", + }, + }, + nil, + ) + require.NoError(t, err) + + authSchemas := databaseSchemas(authAccesses[sharedHost]) + authRoleNames := schemaRoleNames(authSchemas) + assert.ElementsMatch(t, []string{"hydra_read", "consent_read"}, authRoleNames) + + // --- Step 2: groupAccesses for openbanking namespace --- + obGranter := newTestGranter(openbankingNamespaceDatabases) + obAccesses, err := obGranter.groupAccesses( + test.NewLogger(t), "openbanking", + []lunarwayv1alpha1.AccessSpec{ + { + Host: lunarwayv1alpha1.ResourceVar{Value: sharedHost}, + AllDatabases: &trueValue, + Reason: "I am a developer in squad Void who maintains Hydra", + }, + }, + nil, + ) + require.NoError(t, err) + + obSchemas := databaseSchemas(obAccesses[sharedHost]) + obRoleNames := schemaRoleNames(obSchemas) + assert.ElementsMatch(t, []string{"obie_connect_read", "obie_payment_read"}, obRoleNames) + + // --- Step 3: Demonstrate the conflict --- + // The two namespaces produce completely disjoint role sets for the same + // host and the same user. When reconciled independently, each namespace's + // rolesDiff call will revoke the other's roles because they are not in its + // expected list. + // + // This is the root cause: SyncUser processes one PostgreSQLUser at a time + // and has no awareness of other PostgreSQLUser resources for the same + // username on the same host in different namespaces. + for _, authRole := range authRoleNames { + assert.NotContains(t, obRoleNames, authRole, + "BUG CONFIRMED: auth role %q is not in openbanking's expected list and WILL be revoked when openbanking reconciles", authRole) + } + for _, obRole := range obRoleNames { + assert.NotContains(t, authRoleNames, obRole, + "BUG CONFIRMED: openbanking role %q is not in auth's expected list and WILL be revoked when auth reconciles", obRole) + } +} + +// TestCrossNamespaceRevocation_SameNamespaceSafe verifies that within a single +// namespace, multiple entries on the same host do NOT cause revocations because +// all entries are grouped together into one databaseSchemas slice. +func TestCrossNamespaceRevocation_SameNamespaceSafe(t *testing.T) { + host := "db.host.example.com" + + databases := []lunarwayv1alpha1.PostgreSQLDatabase{ + runningDatabase(host, "authentication"), + runningDatabase(host, "mitid"), + runningDatabase(host, "fortnox"), + } + + // User has allDatabases AND specific database entries on the same host. + // This mirrors the prod namespace from krvi.yaml. + reads := []lunarwayv1alpha1.AccessSpec{ + { + Host: lunarwayv1alpha1.ResourceVar{Value: host}, + AllDatabases: &trueValue, + Reason: "I am a developer in squad Void", + }, + { + Host: lunarwayv1alpha1.ResourceVar{Value: host}, + Database: lunarwayv1alpha1.ResourceVar{Value: "authentication"}, + Schema: lunarwayv1alpha1.ResourceVar{Value: "authentication"}, + Reason: "I am a developer in squad Void", + }, + } + + granter := newTestGranter(databases) + accesses, err := granter.groupAccesses(test.NewLogger(t), "prod", reads, nil) + require.NoError(t, err) + + schemas := databaseSchemas(accesses[host]) + roleNames := schemaRoleNames(schemas) + + // All databases should be present. The duplicate from the specific entry + // is harmless — rolesDiff deduplicates via contains(). + assert.Contains(t, roleNames, "authentication_read") + assert.Contains(t, roleNames, "mitid_read") + assert.Contains(t, roleNames, "fortnox_read") +} + +// TestCrossNamespaceRevocation_ExpiredWriteOnlyRevokesWrite verifies that when +// a time-bound write entry expires, only the write role is removed — read +// access from allDatabases is unaffected. This is correct single-namespace +// behavior. +func TestCrossNamespaceRevocation_ExpiredWriteOnlyRevokesWrite(t *testing.T) { + host := "db.host.example.com" + now := time.Date(2025, 10, 26, 0, 0, 0, 0, time.UTC) + + databases := []lunarwayv1alpha1.PostgreSQLDatabase{ + runningDatabase(host, "fortnox"), + runningDatabase(host, "other_db"), + } + + reads := []lunarwayv1alpha1.AccessSpec{ + { + Host: lunarwayv1alpha1.ResourceVar{Value: host}, + AllDatabases: &trueValue, + Reason: "I am a developer in squad Void", + }, + } + + // This write entry has expired. + startTime := metav1.NewTime(time.Date(2025, 10, 25, 10, 0, 0, 0, time.UTC)) + stopTime := metav1.NewTime(time.Date(2025, 10, 25, 15, 0, 0, 0, time.UTC)) + writes := []lunarwayv1alpha1.WriteAccessSpec{ + { + AccessSpec: lunarwayv1alpha1.AccessSpec{ + Host: lunarwayv1alpha1.ResourceVar{Value: host}, + Database: lunarwayv1alpha1.ResourceVar{Value: "fortnox"}, + Schema: lunarwayv1alpha1.ResourceVar{Value: "fortnox"}, + Reason: "...", + Start: &startTime, + Stop: &stopTime, + }, + }, + } + + granter := Granter{ + Now: func() time.Time { return now }, + AllDatabasesReadEnabled: true, + AllDatabases: func(namespace string) ([]lunarwayv1alpha1.PostgreSQLDatabase, error) { + return databases, nil + }, + ResourceResolver: func(r lunarwayv1alpha1.ResourceVar, ns string) (string, error) { + return r.Value, nil + }, + } + + accesses, err := granter.groupAccesses(test.NewLogger(t), "prod", reads, writes) + require.NoError(t, err) + + schemas := databaseSchemas(accesses[host]) + roleNames := schemaRoleNames(schemas) + + // The expired write should NOT be included. + assert.NotContains(t, roleNames, "fortnox_readwrite", + "expired write should not be in expected roles") + + // Read access should still be present for all databases. + assert.Contains(t, roleNames, "fortnox_read") + assert.Contains(t, roleNames, "other_db_read") +} + +// --- Helpers --- + +func runningDatabase(host, name string) lunarwayv1alpha1.PostgreSQLDatabase { + return lunarwayv1alpha1.PostgreSQLDatabase{ + Spec: lunarwayv1alpha1.PostgreSQLDatabaseSpec{ + Name: name, + Host: lunarwayv1alpha1.ResourceVar{Value: host}, + User: lunarwayv1alpha1.ResourceVar{}, // empty → fallback to database name as schema + }, + Status: lunarwayv1alpha1.PostgreSQLDatabaseStatus{ + Phase: lunarwayv1alpha1.PostgreSQLDatabasePhaseRunning, + }, + } +} + +func newTestGranter(databases []lunarwayv1alpha1.PostgreSQLDatabase) Granter { + return Granter{ + Now: time.Now, + AllDatabasesReadEnabled: true, + AllDatabases: func(namespace string) ([]lunarwayv1alpha1.PostgreSQLDatabase, error) { + return databases, nil + }, + ResourceResolver: func(r lunarwayv1alpha1.ResourceVar, ns string) (string, error) { + return r.Value, nil + }, + } +} + +func schemaRoleNames(schemas []postgres.DatabaseSchema) []string { + var names []string + for _, s := range schemas { + suffix := "read" + switch s.Privileges { + case postgres.PrivilegeWrite: + suffix = "readwrite" + case postgres.PrivilegeOwningWrite: + suffix = "readowningwrite" + } + schema := s.Schema + if schema == "public" { + schema = s.Name + } + names = append(names, schema+"_"+suffix) + } + return names +} diff --git a/pkg/postgres/cross_namespace_revoke_test.go b/pkg/postgres/cross_namespace_revoke_test.go new file mode 100644 index 0000000..64fd625 --- /dev/null +++ b/pkg/postgres/cross_namespace_revoke_test.go @@ -0,0 +1,120 @@ +package postgres + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "go.lunarway.com/postgresql-controller/test" +) + +// TestRolesDiff_CrossNamespaceRevocation demonstrates the bug where two +// PostgreSQLUser resources in different namespaces pointing to the same +// PostgreSQL host cause each reconciliation to revoke roles granted by the +// other namespace. +// +// Real-world scenario from krvi.yaml: +// - krvi in "auth" namespace: allDatabases on auth-rds.hydra.svc.cluster.local. +// - krvi in "openbanking" namespace: allDatabases on auth-rds.hydra.svc.cluster.local. +// +// When "auth" reconciles, it only knows about auth-namespace databases and +// revokes roles from openbanking-namespace databases. When "openbanking" +// reconciles next, it revokes auth-namespace roles. The user sees intermittent +// access loss. +func TestRolesDiff_CrossNamespaceRevocation(t *testing.T) { + logger := test.NewLogger(t) + + // Simulate initial state: both namespaces have been reconciled once. + // The PostgreSQL user has roles from both auth and openbanking namespaces. + existingRoles := []string{ + "rds_iam", + "hydra_read", // from auth namespace + "consent_read", // from auth namespace + "obie_connect_read", // from openbanking namespace + "obie_payment_read", // from openbanking namespace + } + + // --- Reconciliation from "auth" namespace --- + // The auth namespace only knows about its own databases: hydra, consent. + authDatabases := []DatabaseSchema{ + {Name: "hydra", Schema: "hydra", Privileges: PrivilegeRead}, + {Name: "consent", Schema: "consent", Privileges: PrivilegeRead}, + } + staticRoles := []string{"rds_iam"} + + addable, removeable := rolesDiff(logger, existingRoles, staticRoles, authDatabases) + + // BUG: auth reconciliation revokes openbanking roles because they are not + // in the auth namespace's expected list. + assert.Nil(t, addable, "no roles should be added") + assert.Equal(t, []string{"obie_connect_read", "obie_payment_read"}, removeable, + "BUG REPRODUCED: auth namespace reconciliation revokes openbanking namespace roles") + + // --- After auth reconciliation, the user only has auth roles --- + rolesAfterAuthReconcile := []string{ + "rds_iam", + "hydra_read", + "consent_read", + } + + // --- Reconciliation from "openbanking" namespace --- + // The openbanking namespace only knows about its own databases: obie_connect, obie_payment. + openbankingDatabases := []DatabaseSchema{ + {Name: "obie_connect", Schema: "obie_connect", Privileges: PrivilegeRead}, + {Name: "obie_payment", Schema: "obie_payment", Privileges: PrivilegeRead}, + } + + addable2, removeable2 := rolesDiff(logger, rolesAfterAuthReconcile, staticRoles, openbankingDatabases) + + // BUG: openbanking reconciliation now revokes auth roles! + assert.Equal(t, []string{"obie_connect_read", "obie_payment_read"}, addable2, + "openbanking roles should be re-added") + assert.Equal(t, []string{"hydra_read", "consent_read"}, removeable2, + "BUG REPRODUCED: openbanking namespace reconciliation revokes auth namespace roles") +} + +// TestRolesDiff_CrossNamespaceRevocationCycle demonstrates a full +// grant/revoke cycle showing how roles oscillate between namespaces across +// reconciliation loops. +func TestRolesDiff_CrossNamespaceRevocationCycle(t *testing.T) { + logger := test.NewLogger(t) + staticRoles := []string{"rds_iam"} + + authDatabases := []DatabaseSchema{ + {Name: "hydra", Schema: "hydra", Privileges: PrivilegeRead}, + } + openbankingDatabases := []DatabaseSchema{ + {Name: "obie_connect", Schema: "obie_connect", Privileges: PrivilegeRead}, + } + + // Start: user has no roles. + existingRoles := []string{} + + // Cycle 1: auth reconciles first. + addable, removeable := rolesDiff(logger, existingRoles, staticRoles, authDatabases) + assert.Equal(t, []string{"rds_iam", "hydra_read"}, addable) + assert.Nil(t, removeable) + + // Apply: user now has rds_iam + hydra_read. + existingRoles = []string{"rds_iam", "hydra_read"} + + // Cycle 1: openbanking reconciles second. + addable, removeable = rolesDiff(logger, existingRoles, staticRoles, openbankingDatabases) + assert.Equal(t, []string{"obie_connect_read"}, addable) + // BUG: hydra_read is revoked! + assert.Equal(t, []string{"hydra_read"}, removeable, + "BUG: openbanking revokes hydra_read granted by auth namespace") + + // Apply: user now has rds_iam + obie_connect_read (hydra_read LOST). + existingRoles = []string{"rds_iam", "obie_connect_read"} + + // Cycle 2: auth reconciles again. + addable, removeable = rolesDiff(logger, existingRoles, staticRoles, authDatabases) + assert.Equal(t, []string{"hydra_read"}, addable) + // BUG: obie_connect_read is revoked! + assert.Equal(t, []string{"obie_connect_read"}, removeable, + "BUG: auth revokes obie_connect_read granted by openbanking namespace") + + // The user NEVER has both hydra_read AND obie_connect_read at the same + // time after the initial reconciliation. Access oscillates depending on + // which namespace reconciles last. +} From ca2ce220ce1bf6b47cc571d2202cae0ec602dacb Mon Sep 17 00:00:00 2001 From: Martin Anker Have Date: Thu, 12 Mar 2026 08:54:57 +0100 Subject: [PATCH 2/2] feat(grants): experimental fix for cross-namespace role revocation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Merge sibling-namespace accesses before diffing so rolesDiff sees the full expected role set across all namespaces sharing the same PostgreSQL username and host. Without this, each namespace independently revokes roles granted by the others. Changes: - kube.PostgreSQLUsers: cluster-wide user listing - Granter.AllUsers: drop namespace param (always cluster-wide) - SyncUser: call mergeSiblingAccesses after groupAccesses - mergeSiblingAccesses: expand accesses with sibling namespaces, but only for hosts the current namespace already connects to - Wire AllUsers in cmd/main.go - Anonymise test fixtures (no real hostnames, usernames, or db names) - Add TestCrossNamespaceRevocation_MergeFixPreventsRevocation WIP: experimental — needs discussion before merging. Co-Authored-By: Claude Sonnet 4.6 --- cmd/main.go | 3 + pkg/grants/cross_namespace_revoke_test.go | 201 ++++++++++++++------ pkg/grants/grants.go | 2 +- pkg/grants/sync.go | 36 ++++ pkg/kube/kube.go | 10 + pkg/postgres/cross_namespace_revoke_test.go | 143 +++++++------- 6 files changed, 265 insertions(+), 130 deletions(-) diff --git a/cmd/main.go b/cmd/main.go index ae31b33..ae0402f 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -155,6 +155,9 @@ func main() { AllDatabases: func(namespace string) ([]postgresqlv1alpha1.PostgreSQLDatabase, error) { return kube.PostgreSQLDatabases(mgr.GetClient(), namespace) }, + AllUsers: func() ([]postgresqlv1alpha1.PostgreSQLUser, error) { + return kube.PostgreSQLUsers(mgr.GetClient()) + }, ResourceResolver: func(resource postgresqlv1alpha1.ResourceVar, namespace string) (string, error) { return kube.ResourceValue(mgr.GetClient(), resource, namespace) }, diff --git a/pkg/grants/cross_namespace_revoke_test.go b/pkg/grants/cross_namespace_revoke_test.go index 964967e..58a914a 100644 --- a/pkg/grants/cross_namespace_revoke_test.go +++ b/pkg/grants/cross_namespace_revoke_test.go @@ -13,65 +13,65 @@ import ( ) // TestCrossNamespaceRevocation_KrviScenario reproduces the real-world issue -// from krvi.yaml where the same user (krvi) has PostgreSQLUser resources in -// multiple namespaces, and two of them (auth and openbanking) point to the same -// PostgreSQL host (auth-rds.hydra.svc.cluster.local.). +// where the same user has PostgreSQLUser resources in multiple namespaces, and +// two of them (team-a and team-b) point to the same PostgreSQL host +// (shared-db.example.local.). // // Each namespace's reconciliation independently computes desired roles from // only its own databases. When rolesDiff runs, it revokes any existing roles // not in the expected list — including roles granted by the other namespace. func TestCrossNamespaceRevocation_KrviScenario(t *testing.T) { - sharedHost := "auth-rds.hydra.svc.cluster.local." + sharedHost := "shared-db.example.local." - // Databases known in the "auth" namespace on the shared host. - authNamespaceDatabases := []lunarwayv1alpha1.PostgreSQLDatabase{ - runningDatabase(sharedHost, "hydra"), - runningDatabase(sharedHost, "consent"), + // Databases known in the "team-a" namespace on the shared host. + teamADatabases := []lunarwayv1alpha1.PostgreSQLDatabase{ + runningDatabase(sharedHost, "orders"), + runningDatabase(sharedHost, "invoices"), } - // Databases known in the "openbanking" namespace on the shared host. - openbankingNamespaceDatabases := []lunarwayv1alpha1.PostgreSQLDatabase{ - runningDatabase(sharedHost, "obie_connect"), - runningDatabase(sharedHost, "obie_payment"), + // Databases known in the "team-b" namespace on the shared host. + teamBDatabases := []lunarwayv1alpha1.PostgreSQLDatabase{ + runningDatabase(sharedHost, "reports"), + runningDatabase(sharedHost, "billing"), } - // --- Step 1: groupAccesses for auth namespace --- - authGranter := newTestGranter(authNamespaceDatabases) - authAccesses, err := authGranter.groupAccesses( - test.NewLogger(t), "auth", + // --- Step 1: groupAccesses for team-a namespace --- + teamAGranter := newTestGranter(teamADatabases) + teamAAccesses, err := teamAGranter.groupAccesses( + test.NewLogger(t), "team-a", []lunarwayv1alpha1.AccessSpec{ { Host: lunarwayv1alpha1.ResourceVar{Value: sharedHost}, AllDatabases: &trueValue, - Reason: "I am a developer in squad Void who maintains Hydra", + Reason: "team-a service account needs read access", }, }, nil, ) require.NoError(t, err) - authSchemas := databaseSchemas(authAccesses[sharedHost]) - authRoleNames := schemaRoleNames(authSchemas) - assert.ElementsMatch(t, []string{"hydra_read", "consent_read"}, authRoleNames) + teamASchemas := databaseSchemas(teamAAccesses[sharedHost]) + teamARoleNames := schemaRoleNames(teamASchemas) + assert.ElementsMatch(t, []string{"orders_read", "invoices_read"}, teamARoleNames) - // --- Step 2: groupAccesses for openbanking namespace --- - obGranter := newTestGranter(openbankingNamespaceDatabases) - obAccesses, err := obGranter.groupAccesses( - test.NewLogger(t), "openbanking", + // --- Step 2: groupAccesses for team-b namespace --- + teamBGranter := newTestGranter(teamBDatabases) + teamBAccesses, err := teamBGranter.groupAccesses( + test.NewLogger(t), "team-b", []lunarwayv1alpha1.AccessSpec{ { Host: lunarwayv1alpha1.ResourceVar{Value: sharedHost}, AllDatabases: &trueValue, - Reason: "I am a developer in squad Void who maintains Hydra", + Reason: "team-b service account needs read access", }, }, nil, ) require.NoError(t, err) - obSchemas := databaseSchemas(obAccesses[sharedHost]) - obRoleNames := schemaRoleNames(obSchemas) - assert.ElementsMatch(t, []string{"obie_connect_read", "obie_payment_read"}, obRoleNames) + teamBSchemas := databaseSchemas(teamBAccesses[sharedHost]) + teamBRoleNames := schemaRoleNames(teamBSchemas) + assert.ElementsMatch(t, []string{"reports_read", "billing_read"}, teamBRoleNames) // --- Step 3: Demonstrate the conflict --- // The two namespaces produce completely disjoint role sets for the same @@ -82,13 +82,13 @@ func TestCrossNamespaceRevocation_KrviScenario(t *testing.T) { // This is the root cause: SyncUser processes one PostgreSQLUser at a time // and has no awareness of other PostgreSQLUser resources for the same // username on the same host in different namespaces. - for _, authRole := range authRoleNames { - assert.NotContains(t, obRoleNames, authRole, - "BUG CONFIRMED: auth role %q is not in openbanking's expected list and WILL be revoked when openbanking reconciles", authRole) + for _, teamARole := range teamARoleNames { + assert.NotContains(t, teamBRoleNames, teamARole, + "BUG CONFIRMED: team-a role %q is not in team-b's expected list and WILL be revoked when team-b reconciles", teamARole) } - for _, obRole := range obRoleNames { - assert.NotContains(t, authRoleNames, obRole, - "BUG CONFIRMED: openbanking role %q is not in auth's expected list and WILL be revoked when auth reconciles", obRole) + for _, teamBRole := range teamBRoleNames { + assert.NotContains(t, teamARoleNames, teamBRole, + "BUG CONFIRMED: team-b role %q is not in team-a's expected list and WILL be revoked when team-a reconciles", teamBRole) } } @@ -99,29 +99,28 @@ func TestCrossNamespaceRevocation_SameNamespaceSafe(t *testing.T) { host := "db.host.example.com" databases := []lunarwayv1alpha1.PostgreSQLDatabase{ - runningDatabase(host, "authentication"), - runningDatabase(host, "mitid"), - runningDatabase(host, "fortnox"), + runningDatabase(host, "users"), + runningDatabase(host, "products"), + runningDatabase(host, "analytics"), } - // User has allDatabases AND specific database entries on the same host. - // This mirrors the prod namespace from krvi.yaml. + // User has allDatabases AND a specific database entry on the same host. reads := []lunarwayv1alpha1.AccessSpec{ { Host: lunarwayv1alpha1.ResourceVar{Value: host}, AllDatabases: &trueValue, - Reason: "I am a developer in squad Void", + Reason: "service account needs read access to all databases", }, { Host: lunarwayv1alpha1.ResourceVar{Value: host}, - Database: lunarwayv1alpha1.ResourceVar{Value: "authentication"}, - Schema: lunarwayv1alpha1.ResourceVar{Value: "authentication"}, - Reason: "I am a developer in squad Void", + Database: lunarwayv1alpha1.ResourceVar{Value: "users"}, + Schema: lunarwayv1alpha1.ResourceVar{Value: "users"}, + Reason: "explicit entry for users database", }, } granter := newTestGranter(databases) - accesses, err := granter.groupAccesses(test.NewLogger(t), "prod", reads, nil) + accesses, err := granter.groupAccesses(test.NewLogger(t), "team-a", reads, nil) require.NoError(t, err) schemas := databaseSchemas(accesses[host]) @@ -129,9 +128,9 @@ func TestCrossNamespaceRevocation_SameNamespaceSafe(t *testing.T) { // All databases should be present. The duplicate from the specific entry // is harmless — rolesDiff deduplicates via contains(). - assert.Contains(t, roleNames, "authentication_read") - assert.Contains(t, roleNames, "mitid_read") - assert.Contains(t, roleNames, "fortnox_read") + assert.Contains(t, roleNames, "users_read") + assert.Contains(t, roleNames, "products_read") + assert.Contains(t, roleNames, "analytics_read") } // TestCrossNamespaceRevocation_ExpiredWriteOnlyRevokesWrite verifies that when @@ -143,15 +142,15 @@ func TestCrossNamespaceRevocation_ExpiredWriteOnlyRevokesWrite(t *testing.T) { now := time.Date(2025, 10, 26, 0, 0, 0, 0, time.UTC) databases := []lunarwayv1alpha1.PostgreSQLDatabase{ - runningDatabase(host, "fortnox"), - runningDatabase(host, "other_db"), + runningDatabase(host, "orders"), + runningDatabase(host, "products"), } reads := []lunarwayv1alpha1.AccessSpec{ { Host: lunarwayv1alpha1.ResourceVar{Value: host}, AllDatabases: &trueValue, - Reason: "I am a developer in squad Void", + Reason: "service account needs read access", }, } @@ -162,9 +161,9 @@ func TestCrossNamespaceRevocation_ExpiredWriteOnlyRevokesWrite(t *testing.T) { { AccessSpec: lunarwayv1alpha1.AccessSpec{ Host: lunarwayv1alpha1.ResourceVar{Value: host}, - Database: lunarwayv1alpha1.ResourceVar{Value: "fortnox"}, - Schema: lunarwayv1alpha1.ResourceVar{Value: "fortnox"}, - Reason: "...", + Database: lunarwayv1alpha1.ResourceVar{Value: "orders"}, + Schema: lunarwayv1alpha1.ResourceVar{Value: "orders"}, + Reason: "temporary write access", Start: &startTime, Stop: &stopTime, }, @@ -182,19 +181,107 @@ func TestCrossNamespaceRevocation_ExpiredWriteOnlyRevokesWrite(t *testing.T) { }, } - accesses, err := granter.groupAccesses(test.NewLogger(t), "prod", reads, writes) + accesses, err := granter.groupAccesses(test.NewLogger(t), "team-a", reads, writes) require.NoError(t, err) schemas := databaseSchemas(accesses[host]) roleNames := schemaRoleNames(schemas) // The expired write should NOT be included. - assert.NotContains(t, roleNames, "fortnox_readwrite", + assert.NotContains(t, roleNames, "orders_readwrite", "expired write should not be in expected roles") // Read access should still be present for all databases. - assert.Contains(t, roleNames, "fortnox_read") - assert.Contains(t, roleNames, "other_db_read") + assert.Contains(t, roleNames, "orders_read") + assert.Contains(t, roleNames, "products_read") +} + +// TestCrossNamespaceRevocation_MergeFixPreventsRevocation verifies that +// mergeSiblingAccesses expands the expected role set to include roles from +// sibling namespaces, preventing cross-namespace revocations. +func TestCrossNamespaceRevocation_MergeFixPreventsRevocation(t *testing.T) { + sharedHost := "shared-db.example.local." + + teamADatabases := []lunarwayv1alpha1.PostgreSQLDatabase{ + runningDatabase(sharedHost, "orders"), + runningDatabase(sharedHost, "invoices"), + } + teamBDatabases := []lunarwayv1alpha1.PostgreSQLDatabase{ + runningDatabase(sharedHost, "reports"), + runningDatabase(sharedHost, "billing"), + } + + teamAUser := lunarwayv1alpha1.PostgreSQLUser{ + ObjectMeta: metav1.ObjectMeta{Name: "alice", Namespace: "team-a"}, + Spec: lunarwayv1alpha1.PostgreSQLUserSpec{ + Name: "alice", + Read: &[]lunarwayv1alpha1.AccessSpec{ + { + Host: lunarwayv1alpha1.ResourceVar{Value: sharedHost}, + AllDatabases: &trueValue, + Reason: "team-a service account needs read access", + }, + }, + }, + } + teamBUser := lunarwayv1alpha1.PostgreSQLUser{ + ObjectMeta: metav1.ObjectMeta{Name: "alice", Namespace: "team-b"}, + Spec: lunarwayv1alpha1.PostgreSQLUserSpec{ + Name: "alice", + Read: &[]lunarwayv1alpha1.AccessSpec{ + { + Host: lunarwayv1alpha1.ResourceVar{Value: sharedHost}, + AllDatabases: &trueValue, + Reason: "team-b service account needs read access", + }, + }, + }, + } + + allUsers := []lunarwayv1alpha1.PostgreSQLUser{teamAUser, teamBUser} + + // Build a granter that knows about all databases across namespaces. + granter := Granter{ + Now: time.Now, + AllDatabasesReadEnabled: true, + AllDatabases: func(namespace string) ([]lunarwayv1alpha1.PostgreSQLDatabase, error) { + switch namespace { + case "team-a": + return teamADatabases, nil + case "team-b": + return teamBDatabases, nil + default: + return append(teamADatabases, teamBDatabases...), nil + } + }, + AllUsers: func() ([]lunarwayv1alpha1.PostgreSQLUser, error) { + return allUsers, nil + }, + ResourceResolver: func(r lunarwayv1alpha1.ResourceVar, ns string) (string, error) { + return r.Value, nil + }, + } + + // Compute accesses for the team-a namespace. + teamAAccesses, err := granter.groupAccesses( + test.NewLogger(t), "team-a", + *teamAUser.Spec.Read, + nil, + ) + require.NoError(t, err) + + // Apply the merge fix. + granter.mergeSiblingAccesses(test.NewLogger(t), "team-a", "alice", teamAAccesses) + + mergedSchemas := databaseSchemas(teamAAccesses[sharedHost]) + mergedRoleNames := schemaRoleNames(mergedSchemas) + + // After merging, all 4 databases must be present so rolesDiff won't revoke any of them. + assert.ElementsMatch(t, + []string{"orders_read", "invoices_read", "reports_read", "billing_read"}, + mergedRoleNames, + "merged accesses must contain roles from both namespaces to prevent revocation", + ) } // --- Helpers --- diff --git a/pkg/grants/grants.go b/pkg/grants/grants.go index 2b77b5b..f36214f 100644 --- a/pkg/grants/grants.go +++ b/pkg/grants/grants.go @@ -17,7 +17,7 @@ type Granter struct { AllDatabasesWriteEnabled bool ExtendedWritesEnabled bool AllDatabases func(namespace string) ([]lunarwayv1alpha1.PostgreSQLDatabase, error) - AllUsers func(namespace string) ([]lunarwayv1alpha1.PostgreSQLUser, error) + AllUsers func() ([]lunarwayv1alpha1.PostgreSQLUser, error) ResourceResolver func(resource lunarwayv1alpha1.ResourceVar, namespace string) (string, error) StaticRoles []string diff --git a/pkg/grants/sync.go b/pkg/grants/sync.go index a7ed9b8..29195e9 100644 --- a/pkg/grants/sync.go +++ b/pkg/grants/sync.go @@ -34,6 +34,9 @@ func (g *Granter) SyncUser(log logr.Logger, namespace, rolePrefix string, user l } log.Error(err, "Some access requests could not be resolved. Continuating with the resolved ones") } + if g.AllUsers != nil { + g.mergeSiblingAccesses(log, namespace, user.Spec.Name, accesses) + } log.Info(fmt.Sprintf("Found access requests for %d hosts", len(accesses))) hosts, err := g.connectToHosts(log, accesses) @@ -113,6 +116,39 @@ func (g *Granter) setRolesOnHosts(log logr.Logger, name string, accesses HostAcc return nil } +func (g *Granter) mergeSiblingAccesses(log logr.Logger, namespace, username string, accesses HostAccess) { + allUsers, err := g.AllUsers() + if err != nil { + log.Error(err, "Failed to list all users for cross-namespace merge; proceeding without") + return + } + for _, sibling := range allUsers { + if sibling.Spec.Name != username || sibling.Namespace == namespace { + continue + } + siblingRead := []lunarwayv1alpha1.AccessSpec{} + if sibling.Spec.Read != nil { + siblingRead = *sibling.Spec.Read + } + siblingWrite := []lunarwayv1alpha1.WriteAccessSpec{} + if sibling.Spec.Write != nil { + siblingWrite = *sibling.Spec.Write + } + siblingAccesses, err := g.groupAccesses(log, sibling.Namespace, siblingRead, siblingWrite) + if err != nil { + log.Error(err, "Failed to compute sibling accesses", "siblingNamespace", sibling.Namespace) + if len(siblingAccesses) == 0 { + continue + } + } + for host, siblingList := range siblingAccesses { + if _, ok := accesses[host]; ok { + accesses[host] = append(accesses[host], siblingList...) + } + } + } +} + func databaseSchemas(accesses []ReadWriteAccess) []postgres.DatabaseSchema { var ds []postgres.DatabaseSchema for _, access := range accesses { diff --git a/pkg/kube/kube.go b/pkg/kube/kube.go index 717847c..ca258b6 100644 --- a/pkg/kube/kube.go +++ b/pkg/kube/kube.go @@ -95,3 +95,13 @@ func PostgreSQLDatabases(c client.Client, namespace string) ([]lunarwayv1alpha1. } return databases.Items, nil } + +// PostgreSQLUsers returns all PostgreSQLUser resources cluster-wide. +func PostgreSQLUsers(c client.Client) ([]lunarwayv1alpha1.PostgreSQLUser, error) { + var users lunarwayv1alpha1.PostgreSQLUserList + err := c.List(context.TODO(), &users) + if err != nil { + return nil, fmt.Errorf("get users: %w", err) + } + return users.Items, nil +} diff --git a/pkg/postgres/cross_namespace_revoke_test.go b/pkg/postgres/cross_namespace_revoke_test.go index 64fd625..0780bc7 100644 --- a/pkg/postgres/cross_namespace_revoke_test.go +++ b/pkg/postgres/cross_namespace_revoke_test.go @@ -12,64 +12,63 @@ import ( // PostgreSQL host cause each reconciliation to revoke roles granted by the // other namespace. // -// Real-world scenario from krvi.yaml: -// - krvi in "auth" namespace: allDatabases on auth-rds.hydra.svc.cluster.local. -// - krvi in "openbanking" namespace: allDatabases on auth-rds.hydra.svc.cluster.local. +// Scenario: +// - alice in "team-a" namespace: allDatabases on shared-db.example.local. +// - alice in "team-b" namespace: allDatabases on shared-db.example.local. // -// When "auth" reconciles, it only knows about auth-namespace databases and -// revokes roles from openbanking-namespace databases. When "openbanking" -// reconciles next, it revokes auth-namespace roles. The user sees intermittent -// access loss. +// When "team-a" reconciles, it only knows about team-a databases and revokes +// roles from team-b databases. When "team-b" reconciles next, it revokes +// team-a roles. The user sees intermittent access loss. func TestRolesDiff_CrossNamespaceRevocation(t *testing.T) { logger := test.NewLogger(t) // Simulate initial state: both namespaces have been reconciled once. - // The PostgreSQL user has roles from both auth and openbanking namespaces. + // The PostgreSQL user has roles from both team-a and team-b namespaces. existingRoles := []string{ - "rds_iam", - "hydra_read", // from auth namespace - "consent_read", // from auth namespace - "obie_connect_read", // from openbanking namespace - "obie_payment_read", // from openbanking namespace + "app_role", + "orders_read", // from team-a namespace + "invoices_read", // from team-a namespace + "reports_read", // from team-b namespace + "billing_read", // from team-b namespace } - // --- Reconciliation from "auth" namespace --- - // The auth namespace only knows about its own databases: hydra, consent. - authDatabases := []DatabaseSchema{ - {Name: "hydra", Schema: "hydra", Privileges: PrivilegeRead}, - {Name: "consent", Schema: "consent", Privileges: PrivilegeRead}, + // --- Reconciliation from "team-a" namespace --- + // team-a only knows about its own databases: orders, invoices. + teamADatabases := []DatabaseSchema{ + {Name: "orders", Schema: "orders", Privileges: PrivilegeRead}, + {Name: "invoices", Schema: "invoices", Privileges: PrivilegeRead}, } - staticRoles := []string{"rds_iam"} + staticRoles := []string{"app_role"} - addable, removeable := rolesDiff(logger, existingRoles, staticRoles, authDatabases) + addable, removeable := rolesDiff(logger, existingRoles, staticRoles, teamADatabases) - // BUG: auth reconciliation revokes openbanking roles because they are not - // in the auth namespace's expected list. + // BUG: team-a reconciliation revokes team-b roles because they are not + // in team-a's expected list. assert.Nil(t, addable, "no roles should be added") - assert.Equal(t, []string{"obie_connect_read", "obie_payment_read"}, removeable, - "BUG REPRODUCED: auth namespace reconciliation revokes openbanking namespace roles") - - // --- After auth reconciliation, the user only has auth roles --- - rolesAfterAuthReconcile := []string{ - "rds_iam", - "hydra_read", - "consent_read", + assert.Equal(t, []string{"reports_read", "billing_read"}, removeable, + "BUG REPRODUCED: team-a namespace reconciliation revokes team-b namespace roles") + + // --- After team-a reconciliation, the user only has team-a roles --- + rolesAfterTeamAReconcile := []string{ + "app_role", + "orders_read", + "invoices_read", } - // --- Reconciliation from "openbanking" namespace --- - // The openbanking namespace only knows about its own databases: obie_connect, obie_payment. - openbankingDatabases := []DatabaseSchema{ - {Name: "obie_connect", Schema: "obie_connect", Privileges: PrivilegeRead}, - {Name: "obie_payment", Schema: "obie_payment", Privileges: PrivilegeRead}, + // --- Reconciliation from "team-b" namespace --- + // team-b only knows about its own databases: reports, billing. + teamBDatabases := []DatabaseSchema{ + {Name: "reports", Schema: "reports", Privileges: PrivilegeRead}, + {Name: "billing", Schema: "billing", Privileges: PrivilegeRead}, } - addable2, removeable2 := rolesDiff(logger, rolesAfterAuthReconcile, staticRoles, openbankingDatabases) + addable2, removeable2 := rolesDiff(logger, rolesAfterTeamAReconcile, staticRoles, teamBDatabases) - // BUG: openbanking reconciliation now revokes auth roles! - assert.Equal(t, []string{"obie_connect_read", "obie_payment_read"}, addable2, - "openbanking roles should be re-added") - assert.Equal(t, []string{"hydra_read", "consent_read"}, removeable2, - "BUG REPRODUCED: openbanking namespace reconciliation revokes auth namespace roles") + // BUG: team-b reconciliation now revokes team-a roles! + assert.Equal(t, []string{"reports_read", "billing_read"}, addable2, + "team-b roles should be re-added") + assert.Equal(t, []string{"orders_read", "invoices_read"}, removeable2, + "BUG REPRODUCED: team-b namespace reconciliation revokes team-a namespace roles") } // TestRolesDiff_CrossNamespaceRevocationCycle demonstrates a full @@ -77,44 +76,44 @@ func TestRolesDiff_CrossNamespaceRevocation(t *testing.T) { // reconciliation loops. func TestRolesDiff_CrossNamespaceRevocationCycle(t *testing.T) { logger := test.NewLogger(t) - staticRoles := []string{"rds_iam"} + staticRoles := []string{"app_role"} - authDatabases := []DatabaseSchema{ - {Name: "hydra", Schema: "hydra", Privileges: PrivilegeRead}, + teamADatabases := []DatabaseSchema{ + {Name: "orders", Schema: "orders", Privileges: PrivilegeRead}, } - openbankingDatabases := []DatabaseSchema{ - {Name: "obie_connect", Schema: "obie_connect", Privileges: PrivilegeRead}, + teamBDatabases := []DatabaseSchema{ + {Name: "reports", Schema: "reports", Privileges: PrivilegeRead}, } // Start: user has no roles. existingRoles := []string{} - // Cycle 1: auth reconciles first. - addable, removeable := rolesDiff(logger, existingRoles, staticRoles, authDatabases) - assert.Equal(t, []string{"rds_iam", "hydra_read"}, addable) + // Cycle 1: team-a reconciles first. + addable, removeable := rolesDiff(logger, existingRoles, staticRoles, teamADatabases) + assert.Equal(t, []string{"app_role", "orders_read"}, addable) assert.Nil(t, removeable) - // Apply: user now has rds_iam + hydra_read. - existingRoles = []string{"rds_iam", "hydra_read"} - - // Cycle 1: openbanking reconciles second. - addable, removeable = rolesDiff(logger, existingRoles, staticRoles, openbankingDatabases) - assert.Equal(t, []string{"obie_connect_read"}, addable) - // BUG: hydra_read is revoked! - assert.Equal(t, []string{"hydra_read"}, removeable, - "BUG: openbanking revokes hydra_read granted by auth namespace") - - // Apply: user now has rds_iam + obie_connect_read (hydra_read LOST). - existingRoles = []string{"rds_iam", "obie_connect_read"} - - // Cycle 2: auth reconciles again. - addable, removeable = rolesDiff(logger, existingRoles, staticRoles, authDatabases) - assert.Equal(t, []string{"hydra_read"}, addable) - // BUG: obie_connect_read is revoked! - assert.Equal(t, []string{"obie_connect_read"}, removeable, - "BUG: auth revokes obie_connect_read granted by openbanking namespace") - - // The user NEVER has both hydra_read AND obie_connect_read at the same - // time after the initial reconciliation. Access oscillates depending on - // which namespace reconciles last. + // Apply: user now has app_role + orders_read. + existingRoles = []string{"app_role", "orders_read"} + + // Cycle 1: team-b reconciles second. + addable, removeable = rolesDiff(logger, existingRoles, staticRoles, teamBDatabases) + assert.Equal(t, []string{"reports_read"}, addable) + // BUG: orders_read is revoked! + assert.Equal(t, []string{"orders_read"}, removeable, + "BUG: team-b revokes orders_read granted by team-a namespace") + + // Apply: user now has app_role + reports_read (orders_read LOST). + existingRoles = []string{"app_role", "reports_read"} + + // Cycle 2: team-a reconciles again. + addable, removeable = rolesDiff(logger, existingRoles, staticRoles, teamADatabases) + assert.Equal(t, []string{"orders_read"}, addable) + // BUG: reports_read is revoked! + assert.Equal(t, []string{"reports_read"}, removeable, + "BUG: team-a revokes reports_read granted by team-b namespace") + + // The user NEVER has both orders_read AND reports_read at the same time + // after the initial reconciliation. Access oscillates depending on which + // namespace reconciles last. }