diff --git a/docs/security-report.md b/docs/security-report.md index 2e0d7c5..5c35d83 100644 --- a/docs/security-report.md +++ b/docs/security-report.md @@ -1,9 +1,9 @@ **1. Executive Summary** -This repo’s remaining main security problems are real and concentrated in authorization, cross-tenant relationship integrity, and operational hardening. The highest-risk remaining issue is that most protected routes check only “has a valid JWT,” not “has the right permission.” The next tier is that cross-tenant user-role links are still possible if foreign UUIDs are known, while forwarded-header trust and Redis fail-open behavior still weaken abuse protection and revocation guarantees. +This repo’s remaining main security problems are real and concentrated in authorization and operational hardening. The highest-risk remaining issue is that most protected routes check only “has a valid JWT,” not “has the right permission.” The next tier is that forwarded-header trust, Redis fail-open behavior, and weak JWT secret acceptance still weaken abuse protection and revocation guarantees. -Update (2026-03-27): findings 1, 3, 5, and 6 have been fixed in the repo. The remaining findings below are still outstanding unless explicitly marked otherwise. +Update (2026-03-28): findings 1, 3, 4, 5, and 6 have been fixed in the repo. The remaining findings below are still outstanding unless explicitly marked otherwise. -This report began as a read-only review of the codebase across auth/session logic, authorization/tenant boundaries, and config/deployment. Findings 1, 3, 5, and 6 have since been remediated in the repo. `go test ./...` passed locally; `govulncheck` was not installed, so dependency-CVE verification remains open. +This report began as a read-only review of the codebase across auth/session logic, authorization/tenant boundaries, and config/deployment. Findings 1, 3, 4, 5, and 6 have since been remediated in the repo. `go test ./...` passed locally; `govulncheck` was not installed, so dependency-CVE verification remains open. **2. Architecture and Attack Surface** - Entry points: public auth routes, `/health`, `/ready`, `/swagger/*`, and JWT-protected `/api/v1/*` in [router.go](/Users/osamamuhammed/uniauth/internal/api/router.go#L76). @@ -29,7 +29,9 @@ This report began as a read-only review of the codebase across auth/session logi Status: Fixed. Remediation implemented: handlers now thread caller `org_id` into the user and RBAC service paths, user/role repository lookups and mutations enforce `org_id` in SQL, out-of-org mutations now resolve as `404`, and the related `/users/{id}/roles` and `/roles/{id}/permissions` service paths validate org ownership before mutating. Regression coverage was added in [tenant_scope_test.go](/Users/osamamuhammed/uniauth/internal/repository/postgres/tenant_scope_test.go#L1) and [rbac_scope_test.go](/Users/osamamuhammed/uniauth/internal/service/rbac_scope_test.go#L1). -4. **Cross-tenant user-role links are possible** — High, High confidence. Affected: [rbac service](/Users/osamamuhammed/uniauth/internal/service/rbac.go#L78), [roles repo](/Users/osamamuhammed/uniauth/internal/repository/postgres/roles.go#L113), [migration](/Users/osamamuhammed/uniauth/migrations/000001_init_schema.up.sql#L91). Evidence: `AssignRoleToUser` inserts `(user_id, role_id)` without checking that both belong to the same org, and the `user_roles` table has no same-org enforcement. Risk: cross-tenant privilege contamination or integrity damage if a victim user UUID and a role UUID are known. Preconditions: attacker knows both IDs. Fix: validate same-org in service logic immediately, then add DB-level enforcement via schema redesign or trigger. Verify with cross-org assignment tests. OWASP: A01 Broken Access Control. +4. **Cross-tenant user-role links are possible** — High, High confidence. Affected: [rbac service](/Users/osamamuhammed/uniauth/internal/service/rbac.go#L81), [roles repo](/Users/osamamuhammed/uniauth/internal/repository/postgres/roles.go#L121), [migration](/Users/osamamuhammed/uniauth/migrations/000002_user_roles_org_scope.up.sql#L1). Evidence: `AssignRoleToUser` inserted `(user_id, role_id)` without checking that both belonged to the same org, and the `user_roles` table had no same-org enforcement. Risk: cross-tenant privilege contamination or integrity damage if a victim user UUID and a role UUID are known. Preconditions: attacker knows both IDs. Fix: validate same-org in service logic immediately, then add DB-level enforcement via schema redesign or trigger. Verify with cross-org assignment tests. OWASP: A01 Broken Access Control. + Status: Fixed. + Remediation implemented: the existing org-scoped service prechecks remain in place, `Store.AssignRoleToUser` now persists `org_id` and maps DB-side tenant-integrity violations to `ErrNotFound`, and [000002_user_roles_org_scope.up.sql](/Users/osamamuhammed/uniauth/migrations/000002_user_roles_org_scope.up.sql#L1) adds composite same-org foreign keys while auto-removing any legacy cross-tenant `user_roles` rows during migration. Regression coverage was expanded in [tenant_scope_test.go](/Users/osamamuhammed/uniauth/internal/repository/postgres/tenant_scope_test.go#L1) and the service-layer `ErrNotFound` guard remains covered in [rbac_scope_test.go](/Users/osamamuhammed/uniauth/internal/service/rbac_scope_test.go#L1). 5. **Password-reset tokens are logged in plaintext when SMTP is unset** — High, High confidence. Affected: [email.go](/Users/osamamuhammed/uniauth/internal/service/email.go#L21), [Helm values](/Users/osamamuhammed/uniauth/helm/uniauth/values.yaml#L12), [k8s configmap](/Users/osamamuhammed/uniauth/k8s/configmap.yaml#L9). Evidence: `SendPasswordReset` prints the live reset token to stdout, and production-oriented manifests default to SMTP unset. Risk: anyone with pod/log aggregation access can take over accounts by replaying reset tokens. Preconditions: SMTP is unset in a real environment and attacker can read logs. Fix: never print tokens; fail closed outside development; if dev mode is needed, log only that a reset was requested. Verify by requesting a reset in production mode with no SMTP and confirming no token reaches stdout/logs. OWASP: A02 Cryptographic Failures / A05 Security Misconfiguration. Status: Fixed. @@ -63,7 +65,7 @@ This report began as a read-only review of the codebase across auth/session logi - Short-term hardening 2: replace naive forwarded-header trust with trusted-proxy-aware IP extraction. Complexity: Small-Medium. Regression risk: Medium. Order: 6. - Short-term hardening 3: enforce JWT secret quality, sanitize `/ready`, and define Redis degraded-mode behavior. Complexity: Small. Regression risk: Low-Medium. Order: 7. - Structural improvement 1: introduce reusable authorization middleware/policy mapping instead of ad hoc handler checks. Complexity: High. Regression risk: Medium. -- Structural improvement 2: add DB-level same-org enforcement for role assignments. Complexity: High. Regression risk: Medium. +- Completed structural improvement 2: add DB-level same-org enforcement for role assignments and auto-remove legacy cross-tenant links during migration. Complexity: Medium. Regression risk: Medium. - Structural improvement 3: pin CI actions/artifacts/images and add `govulncheck` to CI. Complexity: Small. Regression risk: Low. **6. Verification Checklist** @@ -71,6 +73,7 @@ This report began as a read-only review of the codebase across auth/session logi - Add tests proving logout, logout-all, password change, and password reset invalidate both access and refresh credentials in practice. - Add negative authz tests for every privileged endpoint using a valid but low-privilege JWT. - Implemented: cross-org repository tests now cover user/role lookup and mutation scoping in [tenant_scope_test.go](/Users/osamamuhammed/uniauth/internal/repository/postgres/tenant_scope_test.go#L1). +- Implemented: user-role integrity tests now cover same-org assignment success, repository rejection of foreign-org pairs, and raw SQL schema-level enforcement of the same-org invariant in [tenant_scope_test.go](/Users/osamamuhammed/uniauth/internal/repository/postgres/tenant_scope_test.go#L1). - Implemented: RBAC service tests now reject foreign-org role assignment and permission assignment in [rbac_scope_test.go](/Users/osamamuhammed/uniauth/internal/service/rbac_scope_test.go#L1). - Add tests that spoofed `X-Forwarded-For` does not create separate limiter buckets unless the request came through a trusted proxy. - Implemented: webhook validation and delivery tests now reject private/link-local/metadata webhook targets and ensure blocked destinations are not dialed in [webhook_test.go](/Users/osamamuhammed/uniauth/internal/service/webhook_test.go#L1) and [webhooks_test.go](/Users/osamamuhammed/uniauth/internal/api/handlers/webhooks_test.go#L1). @@ -85,4 +88,4 @@ This report began as a read-only review of the codebase across auth/session logi - I did not verify pod/network egress policy. Existing egress controls remain useful defense-in-depth on top of the new webhook validation and delivery guards. - I could not verify dependency-level vulns because `govulncheck` is not installed in this workspace. -This report is now a current status snapshot of the repo; the outstanding findings above remain the next recommended remediation targets. +This report is now a current status snapshot of the repo; 6 issues remain open in the report: findings 2, 7, 8, 9, plus the 2 lower-severity hardening items. diff --git a/internal/repository/postgres/roles.go b/internal/repository/postgres/roles.go index 526ed95..32fe896 100644 --- a/internal/repository/postgres/roles.go +++ b/internal/repository/postgres/roles.go @@ -7,6 +7,7 @@ import ( "github.com/google/uuid" "github.com/jackc/pgx/v5" + "github.com/jackc/pgx/v5/pgconn" "github.com/osama1998h/uniauth/internal/domain" ) @@ -117,10 +118,15 @@ func (s *Store) ListPermissionsByRole(ctx context.Context, roleID uuid.UUID) ([] return collectPermissions(rows) } -func (s *Store) AssignRoleToUser(ctx context.Context, userID, roleID uuid.UUID) error { +func (s *Store) AssignRoleToUser(ctx context.Context, orgID, userID, roleID uuid.UUID) error { _, err := s.pool.Exec(ctx, - `INSERT INTO user_roles (user_id, role_id) VALUES ($1, $2) ON CONFLICT DO NOTHING`, userID, roleID, + `INSERT INTO user_roles (org_id, user_id, role_id) VALUES ($1, $2, $3) ON CONFLICT DO NOTHING`, + orgID, userID, roleID, ) + var pgErr *pgconn.PgError + if errors.As(err, &pgErr) && pgErr.Code == "23503" { + return domain.ErrNotFound + } return err } diff --git a/internal/repository/postgres/tenant_scope_test.go b/internal/repository/postgres/tenant_scope_test.go index 2ef5d01..a53d75c 100644 --- a/internal/repository/postgres/tenant_scope_test.go +++ b/internal/repository/postgres/tenant_scope_test.go @@ -7,6 +7,7 @@ import ( "time" "github.com/google/uuid" + "github.com/jackc/pgx/v5/pgconn" "github.com/osama1998h/uniauth/internal/domain" "github.com/osama1998h/uniauth/internal/testutil" @@ -167,3 +168,58 @@ func TestStoreDeleteRoleReturnsNotFoundForMissingOrgMatch(t *testing.T) { t.Fatalf("expected ErrNotFound for missing role, got %v", err) } } + +func TestStoreUserRoleTenantIntegrity(t *testing.T) { + store := testutil.RequireTestStore(t) + ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) + defer cancel() + + orgA := testutil.CreateOrganization(t, store, "user-roles-org-a") + orgB := testutil.CreateOrganization(t, store, "user-roles-org-b") + userA := testutil.CreateUser(t, store, orgA.ID, "user-roles-user-a") + userB := testutil.CreateUser(t, store, orgB.ID, "user-roles-user-b") + roleA := testutil.CreateRole(t, store, orgA.ID, "user-roles-role-a") + roleB := testutil.CreateRole(t, store, orgB.ID, "user-roles-role-b") + + t.Run("AssignRoleToUser inserts same-org links", func(t *testing.T) { + if err := store.AssignRoleToUser(ctx, orgA.ID, userA.ID, roleA.ID); err != nil { + t.Fatalf("assign scoped role: %v", err) + } + + roles, err := store.ListRolesByUser(ctx, userA.ID) + if err != nil { + t.Fatalf("list roles by user: %v", err) + } + if len(roles) != 1 { + t.Fatalf("expected 1 role for user, got %d", len(roles)) + } + if roles[0].ID != roleA.ID { + t.Fatalf("role id = %s, want %s", roles[0].ID, roleA.ID) + } + }) + + t.Run("AssignRoleToUser rejects foreign-org user", func(t *testing.T) { + err := store.AssignRoleToUser(ctx, orgA.ID, userB.ID, roleA.ID) + if !errors.Is(err, domain.ErrNotFound) { + t.Fatalf("expected ErrNotFound for foreign-org user, got %v", err) + } + }) + + t.Run("AssignRoleToUser rejects foreign-org role", func(t *testing.T) { + err := store.AssignRoleToUser(ctx, orgA.ID, userA.ID, roleB.ID) + if !errors.Is(err, domain.ErrNotFound) { + t.Fatalf("expected ErrNotFound for foreign-org role, got %v", err) + } + }) + + t.Run("raw SQL insert rejects mismatched org tuples", func(t *testing.T) { + _, err := store.Pool().Exec(ctx, + `INSERT INTO user_roles (org_id, user_id, role_id) VALUES ($1, $2, $3)`, + orgA.ID, userB.ID, roleA.ID, + ) + var pgErr *pgconn.PgError + if !errors.As(err, &pgErr) || pgErr.Code != "23503" { + t.Fatalf("expected foreign-key violation for mismatched org tuple, got %v", err) + } + }) +} diff --git a/internal/service/rbac.go b/internal/service/rbac.go index 121ee0c..ab820c7 100644 --- a/internal/service/rbac.go +++ b/internal/service/rbac.go @@ -85,7 +85,7 @@ func (s *RBACService) AssignRoleToUser(ctx context.Context, orgID, userID, roleI if _, err := s.store.GetRoleByID(ctx, orgID, roleID); err != nil { return fmt.Errorf("get role: %w", err) } - if err := s.store.AssignRoleToUser(ctx, userID, roleID); err != nil { + if err := s.store.AssignRoleToUser(ctx, orgID, userID, roleID); err != nil { return fmt.Errorf("assign role: %w", err) } s.auditSvc.Log(&domain.AuditLog{ diff --git a/migrations/000002_user_roles_org_scope.down.sql b/migrations/000002_user_roles_org_scope.down.sql new file mode 100644 index 0000000..b568977 --- /dev/null +++ b/migrations/000002_user_roles_org_scope.down.sql @@ -0,0 +1,18 @@ +ALTER TABLE user_roles + DROP CONSTRAINT user_roles_user_org_fkey, + DROP CONSTRAINT user_roles_role_org_fkey; + +ALTER TABLE user_roles + DROP COLUMN org_id; + +ALTER TABLE user_roles + ADD CONSTRAINT user_roles_user_id_fkey + FOREIGN KEY (user_id) REFERENCES users (id) ON DELETE CASCADE, + ADD CONSTRAINT user_roles_role_id_fkey + FOREIGN KEY (role_id) REFERENCES roles (id) ON DELETE CASCADE; + +ALTER TABLE roles + DROP CONSTRAINT roles_id_org_id_key; + +ALTER TABLE users + DROP CONSTRAINT users_id_org_id_key; diff --git a/migrations/000002_user_roles_org_scope.up.sql b/migrations/000002_user_roles_org_scope.up.sql new file mode 100644 index 0000000..b7661c9 --- /dev/null +++ b/migrations/000002_user_roles_org_scope.up.sql @@ -0,0 +1,32 @@ +ALTER TABLE users + ADD CONSTRAINT users_id_org_id_key UNIQUE (id, org_id); + +ALTER TABLE roles + ADD CONSTRAINT roles_id_org_id_key UNIQUE (id, org_id); + +ALTER TABLE user_roles + ADD COLUMN org_id UUID; + +DELETE FROM user_roles ur +USING users u, roles r +WHERE ur.user_id = u.id + AND ur.role_id = r.id + AND u.org_id <> r.org_id; + +UPDATE user_roles ur +SET org_id = u.org_id +FROM users u +WHERE ur.user_id = u.id; + +ALTER TABLE user_roles + ALTER COLUMN org_id SET NOT NULL; + +ALTER TABLE user_roles + DROP CONSTRAINT user_roles_user_id_fkey, + DROP CONSTRAINT user_roles_role_id_fkey; + +ALTER TABLE user_roles + ADD CONSTRAINT user_roles_user_org_fkey + FOREIGN KEY (user_id, org_id) REFERENCES users (id, org_id) ON DELETE CASCADE, + ADD CONSTRAINT user_roles_role_org_fkey + FOREIGN KEY (role_id, org_id) REFERENCES roles (id, org_id) ON DELETE CASCADE;