NM-341-v6: AMB Multi-tenancy Management - #4101
Conversation
…n from ValidateUser (already validated in UpdateUser);
…tration functions;
|
Review complete. 13 potential issues to review. Files Reviewed: 221 By Severity:
This PR introduces a multi-tenant architecture with critical authorization bypasses, cross-tenant data leakage, and broken multi-tenancy isolation. The org-owner creation endpoint is unauthenticated, the license enforcement can be silently disabled, and several routes bypass tenant-scoped queries — any of these could allow tenants to access or delete other tenants' data. Files Reviewed (221 files) |
There was a problem hiding this comment.
Review complete. 13 potential issues to review.
Critical Security Gaps in Multi-Tenant Implementation
This PR (221 files, ~9.5k insertions, ~5k deletions) introduces a multi-tenant rearchitecture across Netmaker. The review uncovered 6 critical/high-severity security issues that undermine tenant isolation:
Authentication & Authorization Bypasses
- Org owner creation is completely unauthenticated in
controllers/org.go:28— any unauthenticated request can create a super-admin across tenants, enabling full multi-tenant takeover. - User enumeration is possible via the unauthenticated
/api/v1/auth/endpoint incontrollers/user.go:44, leaking tenant information.
Cross-Tenant Data Leakage
- Network cleanup in
controllers/network.go:254usescontext.TODO()inside a goroutine, losing all tenant/scope context and risking cross-tenant data deletion. - License enforcement silently disabled in
pro/license/utils.go:35when the cache is unavailable due to a swallowed error fromgetCachedResp. - TOCTOU race condition in
pro/license/utils.go:45allows concurrent requests to exceed licensed resource counts. - MetricsRecord upsert in
schema/metrics_record.go:54does not include tenant_id in theOnConflictupdate clause, rendering records invisible to scoped queries.
Code Correctness
- Compilation failures exist:
logic/device_test.go:219uses old function signatures withoutctxparameters, andpro/logic/security_test.go:36references a removedFullAccessstruct field.
Remediation Priority
- Add authentication to the org-owner creation endpoint immediately
- Ensure all goroutines propagate tenant context (not
context.TODO()) - Fix the license enforcement error swallowing path
- Complete the MetricsRecord upsert to update tenant_id on conflict
- Fix compilation errors in test files
There was a problem hiding this comment.
Review complete. 39 potential issues to review.
Critical Issues (9 findings)
GORM Chain Ordering Bug (finding-001 through finding-007): In 8 Get() methods across schema/ and models/, the pattern db.First(&record).Where("id = ?", id) is used. GORM executes First() immediately, returning the first row in the table before the Where() filter is applied. This means the id parameter is silently ignored and the wrong record is returned from every affected table. Affected files: schema/egress.go, schema/dns.go, schema/pending_hosts.go, schema/posture_check.go, schema/user_access_token.go, schema/event.go, schema/tenant_settings.go, schema/sso_state_record.go, schema/cache_record.go, and models/accessToken.go.
EMQX ACL (finding-008): mq/emqx_on_prem.go:260 creates a default ACL rule granting unrestricted pubsub access on # (all topics) to all authenticated users. This allows any authenticated MQTT client to publish/subscribe to any tenant's topics, bypassing multi-tenant isolation.
Bcrypt Cost Factor (finding-017): logic/auth.go:180 uses bcrypt cost=5, which modern hardware can crack at millions of attempts per second. Industry minimum is 10-12.
Shared OAuth Secret (finding-028): logic/auth.go:68 uses a single shared secret for all SSO/OAuth users, enabling an authenticated user to forge tokens for any other user.
Encryption Chunk Delimiter (finding-012): netclient/ncutils/encryption.go:84 embeds chunk delimiter bytes in the encrypted data stream. If these bytes appear naturally in ciphertext, the decrypted output will be truncated or corrupted.
HTTP Response Handling (finding-011): controllers/enrollmentkeys.go:621 handleHostRegister returns without writing an HTTP response after EMQX user creation failure, causing a client hang.
High Severity Issues (10 findings)
CLI Argument Swaps (finding-013): cli/cmd/gateway/create.go:40 swaps NETWORK NAME and NODE ID in the gateway create command arguments.
Index Panic (finding-014): cli/cmd/host/refresh_keys.go:15 panics with index-out-of-range when called without arguments.
Migration Issues (findings-021, 039): Multi-tenant enrollment key collision in migration 007, and non-atomic check-then-create in migration init.
Performance (findings-022, 026): N+1 queries in migration v1_7 and peer host lookups.
Other high issues: Race in timer hook replacement (finding-015), IPv6 validation gap (finding-016), license cache storing nil (finding-024), orphaned database node (finding-025), missing context.Context in test (finding-018).
Medium Severity Issues (17 findings)
Config YAML decode errors silently ignored, HTTP response body leaks in multiple places, API error responses leaking internal provider data, defense-risk defaulting unrecognized scores to "no risk", and world-readable config directory permissions (0777).
Advisory
This is the initial repository commit with no prior review baseline. The GORM chain-ordering bug alone (8 files) indicates systemic data integrity risk. The EMQX ACL and shared OAuth secret are security-critical architectural flaws. Recommend addressing all critical and high findings before production deployment.
Describe your changes
Provide Issue ticket number if applicable/not in title
Provide testing steps
Checklist before requesting a review