From c174d8381ce2173e87ce025433df97b1c162b5cf Mon Sep 17 00:00:00 2001 From: Arsenij Malov Date: Fri, 4 Sep 2026 01:01:00 +0200 Subject: [PATCH 1/4] Stop cleanup() and removeInactiveDevices from touching other tenants MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All tenant servers share one MongoDB database and are separated only by the `domain` field, but both of these routines are written for upstream's "one server owns the whole database" assumption and act on everything they can see. cleanup() runs unconditionally on every boot. It rewrote every tenant's user and mesh documents, and ended with deleteMany({ meshid: { $exists: true, $nin: meshlist } }) which deletes anything whose device group is not in the list it had just read. That statement sat outside the `if (err == null && docs.length > 0)` guard, so a transient read error left meshlist empty and `$nin: []` matched every document with a meshid — one boot could wipe every tenant's nodes. Reproduced on a two-domain database: a tenant A boot deleted a tenant B device. The call is now gated out of OpenFrame mode and the delete is removed from the MongoDB branch entirely, so it cannot fire if the image is run without the flag. What cleanup() repairs (pre-1.0 field formats, legacy event/power/smbios rows in the main collection) never existed in this database; routine housekeeping is done by RemoveMeshDocuments(), the explicit Remove() calls on device deletion, and the TTL indexes. removeInactiveDevices() runs hourly. It collected domains out of the fleet-wide device-group cache, so one tenant enabling expireDevs made all servers delete that tenant's devices — using the local pod's connectivity state, which never holds another tenant's agents, so a foreign pod could delete a device that was online on its own. It is now limited to this pod's domain. Adds a write guard as a second layer: Set/SetUser/Remove refuse documents whose domain is not ours, with OPENFRAME_CROSS_TENANT_WRITES=allow as a rollback switch. It is the only way to show "zero writes outside our domain" in production, since these writes never pass through the gateway. Tests run against a real two-domain database; see test/README.md. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01WAcuUJKfVLEX4KpvpKd1pS --- .github/workflows/test.yml | 22 ++++- db.js | 96 ++++++++++++++++++- meshcentral.js | 12 ++- test/README.md | 49 ++++++++++ test/run-tests.sh | 49 ++++++++++ test/tenant-isolation.test.js | 176 ++++++++++++++++++++++++++++++++++ 6 files changed, 398 insertions(+), 6 deletions(-) create mode 100644 test/README.md create mode 100755 test/run-tests.sh create mode 100644 test/tenant-isolation.test.js diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 2c723f8e6b..c523fa0cf4 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -110,6 +110,26 @@ jobs: labels: ${{ steps.meta.outputs.labels }} + test_isolation: + name: "Test: tenant isolation" + runs-on: ubuntu-latest + steps: + - name: Checkout + uses: actions/checkout@v4 + + - name: Log in to GitHub Container Registry + uses: docker/login-action@v3 + with: + registry: ${{ env.REGISTRY }} + username: ${{ github.actor }} + password: ${{ secrets.GITHUB_TOKEN }} + + # The image is only a runtime carrier here: it supplies node_modules (mongodb is not a + # package.json dependency), while the code under test is this checkout, mounted at /src. + - name: Run tenant isolation tests + run: test/run-tests.sh ${{ env.REGISTRY }}/${{ env.ORGANISATION }}/${{ env.REPOSITORY }}/meshcentral:latest + + test_helm: name: "Test Helm Chart" needs: [changes] @@ -135,7 +155,7 @@ jobs: all-checks: name: "All Checks" - needs: [changes, test, test_helm] + needs: [changes, test, test_isolation, test_helm] runs-on: ubuntu-latest if: always() steps: diff --git a/db.js b/db.js index 9bb61727ce..3daa9fc245 100644 --- a/db.js +++ b/db.js @@ -38,8 +38,23 @@ function deriveTenantDomain(domains) { } module.exports.deriveTenantDomain = deriveTenantDomain; +// This pod's tenant, or null when not running in OpenFrame mode (single-tenant / OSS installs, +// where none of the scoping below applies and behaviour must stay exactly as upstream). +// +// Resolved ONCE per database instance, not per call: the tenant a server belongs to cannot change +// while it runs, and reading process.env on every query made the scoping depend on when the query +// happened to run relative to the environment. Every caller below must see the same answer as the +// write guard, which binds itself at setup. +function makeOpenframeDomain(parent) { + const resolved = (process.env.OPENFRAME_MODE === 'true') + ? (deriveTenantDomain(parent.config.domains) || null) + : null; + return function () { return resolved; }; +} + module.exports.CreateDB = function (parent, func) { var obj = {}; + const openframeDomain = makeOpenframeDomain(parent); var Datastore = null; var expireEventsSeconds = (60 * 60 * 24 * 20); // By default, expire events after 20 days (1728000). (Seconds * Minutes * Hours * Days) var expirePowerEventsSeconds = (60 * 60 * 24 * 10); // By default, expire power events after 10 days (864000). (Seconds * Minutes * Hours * Days) @@ -208,8 +223,16 @@ module.exports.CreateDB = function (parent, func) { } } - // Check if any device groups have a inactive device removal setting + // Check if any device groups have a inactive device removal setting. + // In OpenFrame mode, only this pod's own domain: parent.webserver.meshes is loaded + // unscoped from the shared database, so without this filter one tenant enabling + // expireDevs makes every other tenant's server delete that tenant's devices. Worse, + // the "is it still connected" test below (GetConnectivityState) reads this pod's + // memory, which never holds another tenant's agents — so a foreign pod would delete + // devices that are online on their own. + const removeInactiveOwnDomain = openframeDomain(); for (var i in parent.webserver.meshes) { + if ((removeInactiveOwnDomain != null) && (parent.webserver.meshes[i].domain !== removeInactiveOwnDomain)) continue; if (typeof parent.webserver.meshes[i].expireDevs == 'number') { var v = parent.webserver.meshes[i].expireDevs; if ((v >= 1) && (v <= 2000)) { @@ -229,6 +252,9 @@ module.exports.CreateDB = function (parent, func) { // For each domain with a inactive device removal setting, get a list of last device connections for (var domainid in minRemoveInactiveDevicesPerDomain) { + // Second guard on the same invariant as the mesh loop above: nothing in this + // function may read or delete outside this pod's own domain. + if ((removeInactiveOwnDomain != null) && (domainid !== removeInactiveOwnDomain)) continue; obj.GetAllTypeNoTypeField('lastconnect', domainid, function (err, docs) { if ((err != null) || (docs == null)) return; for (var j in docs) { @@ -478,8 +504,24 @@ module.exports.CreateDB = function (parent, func) { // MariaDB sqlDbQuery('DELETE FROM Main WHERE (extra LIKE ("mesh/%") AND (extra NOT IN ?)', [meshlist], function (err, response) { }); } else if (obj.databaseType == DB_MONGODB) { - // MongoDB - obj.file.deleteMany({ meshid: { $exists: true, $nin: meshlist } }, { multi: true }); + // MongoDB: deliberately does NOT prune orphans. + // + // The upstream statement here was: + // obj.file.deleteMany({ meshid: { $exists: true, $nin: meshlist } }) + // i.e. "delete every document whose meshid is not in the list we just + // read". Correct when one server owns the database; unsafe here, where + // every tenant server shares one. Two ways it destroys data: + // 1. meshlist is built inside `if ((err == null) && (docs.length > 0))` + // above, but the delete ran outside it. A transient read error left + // meshlist empty, and `$nin: []` matches everything — one boot would + // wipe every tenant's nodes, interfaces and notes. + // 2. A mesh created by another tenant between the read and the delete + // is absent from meshlist, so its nodes are deleted. + // Note this also means the read above must stay unscoped for the other + // branches: scoping it to one domain while the delete stays fleet-wide + // would make case 1 the normal path, not the failure path. + // Orphan accounting is done out of band by an audit query scoped with + // an explicit ^mesh// prefix. } else { // NeDB or MongoJS obj.file.remove({ meshid: { $exists: true, $nin: meshlist } }, { multi: true }); @@ -3299,6 +3341,54 @@ module.exports.CreateDB = function (parent, func) { }); } + // Cross-tenant write guard. Every tenant server shares one database and separation is + // by the `domain` field alone, so a missing domain check anywhere upstream lands here + // as a write into another tenant's document. Wrapping at this single point covers all + // backends: Set/SetUser/Remove are already bound to the concrete one by now. + // + // Only enforced when a domain can actually be determined — from the document, or from + // the id, whose shape is `[prefix]//` (e.g. 'lc' + node//...). + // Documents that carry no tenant at all (cfile/*, serverstats, power, DatabaseIdentifier) + // are out of scope here and are handled by giving them a domain of their own. + // + // Set OPENFRAME_CROSS_TENANT_WRITES=allow to log without blocking (rollback switch). + { + const guardDomain = openframeDomain(); + const guardEnforce = (process.env.OPENFRAME_CROSS_TENANT_WRITES !== 'allow'); + const guardIdDomain = /(?:^|[a-z]{2})(?:user|node|mesh|ugrp)\/([^/]*)\//; + const guardDomainOf = function (doc, id) { + if ((doc != null) && (typeof doc.domain === 'string')) { return doc.domain; } + if (typeof id !== 'string') { return null; } + const m = guardIdDomain.exec(id); + return (m != null) ? m[1] : null; + }; + if (guardDomain) { + ['Set', 'SetUser', 'Remove'].forEach(function (guardName) { + const guardOrig = obj[guardName]; + if (typeof guardOrig != 'function') return; + obj[guardName] = function (guardArg) { + const d = (guardName === 'Remove') + ? guardDomainOf(null, guardArg) + : guardDomainOf(guardArg, (guardArg != null) ? guardArg._id : null); + if ((d != null) && (d !== guardDomain)) { + const guardId = ((guardArg != null) && (guardArg._id != null)) ? guardArg._id : guardArg; + console.log(new Date().toISOString() + ' CROSS-TENANT ' + guardName + + (guardEnforce ? ' BLOCKED' : ' ALLOWED') + + ' domain=' + d + ' mine=' + guardDomain + ' id=' + guardId); + if (guardEnforce) { + // Blocking must not hang a caller that is waiting on a callback + // (Set(data, func) / Remove(id, func)), so fail it explicitly. + const guardCb = arguments[arguments.length - 1]; + if (typeof guardCb == 'function') { setImmediate(function () { guardCb(new Error('cross-tenant write blocked')); }); } + return; + } + } + return guardOrig.apply(obj, arguments); + }; + }); + } + } + func(obj); // Completed function setup } diff --git a/meshcentral.js b/meshcentral.js index ff759f9ee6..6b365094c1 100644 --- a/meshcentral.js +++ b/meshcentral.js @@ -1747,8 +1747,16 @@ function CreateMeshCentralServer(config, args) { obj.fs.open(obj.path.join(obj.datapath, 'agenterrorlogs.txt'), 'a', function (err, fd) { obj.agentErrorLog = fd; }) } - // Perform other database cleanup - obj.db.cleanup(); + // Perform other database cleanup. + // Skipped in OpenFrame mode: every tenant server shares one database, and cleanup() + // is written for "one server owns the whole database". It rewrites every tenant's + // user and mesh documents (db.js, obj.Set inside the GetAllType('user'/'mesh') + // callbacks) and deletes fleet-wide. What it repairs — pre-1.0 field formats and + // legacy type:'event'/'power'/'smbios' rows in the main collection — never existed + // in this database: those types are written to their own collections here. + // Routine housekeeping is covered elsewhere: RemoveMeshDocuments() on device group + // deletion, the explicit Remove() calls on device deletion, and the TTL indexes. + if (process.env.OPENFRAME_MODE !== 'true') { obj.db.cleanup(); } // Set all nodes to power state of unknown (0) obj.db.storePowerEvent({ time: new Date(), nodeid: '*', power: 0, s: 1 }, obj.multiServer); // s:1 indicates that the server is starting up. diff --git a/test/README.md b/test/README.md new file mode 100644 index 0000000000..7961cc68be --- /dev/null +++ b/test/README.md @@ -0,0 +1,49 @@ +# Tenant isolation tests + +In OpenFrame mode every tenant runs its own MeshCentral server, but all of them share **one MongoDB +database** with shared collections, and tenants are separated only by the `domain` field on each +document (which is also embedded in the `_id`). Upstream MeshCentral is written for "one server owns +the whole database", so several of its queries and maintenance routines legitimately span everything +they can see — which here means every tenant. + +These tests pin the places that were changed for that. Each one seeds two tenant domains into a +single database, drives `db.js` as tenant A, and asserts that tenant B is left alone. + +## Running + +```bash +test/run-tests.sh # against :latest +test/run-tests.sh ghcr.io/flamingo-stack/meshcentral/meshcentral:0.0.28 +``` + +The script starts a throwaway MongoDB replica set and runs `node --test` inside the MeshCentral +image with this checkout mounted read-only at `/src`. Two reasons it is not a plain `npm test`: + +- `mongodb` is not a `package.json` dependency of this repository — the driver lives in the image. + `NODE_PATH` points module resolution there while the code under test comes from `/src`. +- The tests use a real replica set rather than a mock. What is being tested is the *shape of the + queries* (does this find/delete carry a domain?), and a mock would only assert that the code calls + the mock. + +CI runs the same script (`test_isolation` job in `.github/workflows/test.yml`). + +## What is covered + +| Test | Guards against | +|---|---| +| `GetAllTypeForDomain` returns one tenant and keeps `type` | the boot cache loading the whole fleet; and the projection trap — `GetAllTypeNoTypeField` strips `type`, and a document saved back without it disappears from every type query | +| `cleanup()` deletes nothing | upstream's `deleteMany({meshid: {$nin: meshlist}})`, which deleted other tenants' devices and, with an empty `meshlist` after a read error, everything | +| `removeInactiveDevices` stays in its domain | the hourly maintenance pass reading device groups of other tenants out of the shared boot cache and deleting their devices | +| cross-tenant writes are blocked, own-domain writes pass | any missing domain check upstream of `Set`/`SetUser`/`Remove` reaching another tenant's documents | +| the write guard can be disabled | that `OPENFRAME_CROSS_TENANT_WRITES=allow` still works as a rollback switch | +| server stats are read per tenant | the "My Server" timeline showing the summed load of the whole fleet | +| config files are per tenant, with a legacy fallback | every pod overwriting the same `cfile/` row, which gave all tenants identical server certificates — and therefore the identical ServerID agents pin | +| `deriveTenantDomain` picks the tenant | the domain resolution the rest depends on, including that a legacy single-domain install stays unscoped | + +## What is not covered here + +- Anything above the database layer: `getpluginpermissionlist` and `changeuserpass` are WebSocket + commands and need a running server plus an authenticated session. They were verified manually + against a two-domain database; a WS-level suite would be the next step. +- The gateway command allowlist, which lives in `openframe-oss-lib` and has its own unit tests. +- Agent connectivity, and the certificate sync (`autoSyncConfigFiles`) against real certificates. diff --git a/test/run-tests.sh b/test/run-tests.sh new file mode 100755 index 0000000000..802f071b1d --- /dev/null +++ b/test/run-tests.sh @@ -0,0 +1,49 @@ +#!/usr/bin/env bash +# Runs the tenant isolation tests. +# +# They need two things this repository does not carry: a MongoDB replica set (changeStream setup in +# db.js needs one, and it is what production runs), and the node_modules that live in the built +# image rather than in the repo — `mongodb` is not a package.json dependency here. +# +# So: start a throwaway Mongo, then run `node --test` inside the meshcentral image with this +# checkout mounted read-only at /src. NODE_PATH points module resolution at the image's +# node_modules, so db.js can require('mongodb') while its own code comes from /src. +# +# Usage: test/run-tests.sh [image] +set -euo pipefail + +IMAGE="${1:-ghcr.io/flamingo-stack/meshcentral/meshcentral:latest}" +SRC="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" +NET=mc-test-net +MONGO=mc-test-mongo +MONGO_IMAGE="${MONGO_IMAGE:-ghcr.io/flamingo-stack/registry/mongo:7.0.40}" + +cleanup() { + docker rm -f "$MONGO" >/dev/null 2>&1 || true + docker network rm "$NET" >/dev/null 2>&1 || true +} +trap cleanup EXIT + +cleanup +docker network create "$NET" >/dev/null +docker run -d --name "$MONGO" --network "$NET" "$MONGO_IMAGE" --replSet rs0 --bind_ip_all >/dev/null + +echo "Waiting for MongoDB..." +for _ in $(seq 1 30); do + if docker exec "$MONGO" mongosh --quiet --eval 'db.adminCommand("ping").ok' >/dev/null 2>&1; then break; fi + sleep 2 +done +docker exec "$MONGO" mongosh --quiet --eval "rs.initiate({_id:'rs0',members:[{_id:0,host:'$MONGO:27017'}]})" >/dev/null +for _ in $(seq 1 30); do + if [ "$(docker exec "$MONGO" mongosh --quiet --eval 'db.adminCommand("hello").isWritablePrimary')" = "true" ]; then break; fi + sleep 2 +done + +echo "Running tests..." +docker run --rm --network "$NET" \ + -v "$SRC:/src:ro" \ + -e NODE_PATH=/opt/meshcentral/meshcentral/node_modules \ + -e MC_SRC=/src \ + -e MC_TEST_MONGO_URL="mongodb://$MONGO:27017/meshcentral_test?replicaSet=rs0" \ + "$IMAGE" \ + node --test --test-force-exit --test-timeout=60000 /src/test/*.test.js diff --git a/test/tenant-isolation.test.js b/test/tenant-isolation.test.js new file mode 100644 index 0000000000..6cb4c27f4f --- /dev/null +++ b/test/tenant-isolation.test.js @@ -0,0 +1,176 @@ +/** + * Tenant isolation tests for the OpenFrame fork. + * + * Every tenant server shares ONE MongoDB database and is separated only by the `domain` field, so + * upstream's "one server owns the whole database" queries reach across tenants here. These tests + * pin the behaviours that were changed for that: each one seeds two tenant domains into a single + * database, drives db.js as tenant A, and asserts that tenant B is untouched. + * + * They run against a real MongoDB replica set rather than a mock: what is being tested is the shape + * of the queries themselves, so a mock would only assert that the code calls the mock. + * + * Run with test/run-tests.sh (starts the datastore and executes this inside the meshcentral image). + */ + +'use strict'; + +const test = require('node:test'); +const assert = require('node:assert'); +const path = require('path'); + +const MESH_DIR = process.env.MC_SRC || path.join(__dirname, '..'); +const MONGO_URL = process.env.MC_TEST_MONGO_URL || 'mongodb://localhost:27017/meshcentral_test?replicaSet=rs0'; + +const A = 'aaaa1111-1111-1111-1111-111111111111'; +const B = 'bbbb2222-2222-2222-2222-222222222222'; + +// --- Helpers --------------------------------------------------------------- + +function config(domain) { + // Mirrors the deployed shape: the default domain, the tenant's own, and the static share domain + // that deriveTenantDomain() must skip. + const domains = { '': { title: 'MeshCentral' }, openframe_public: { share: '/opt/mesh/public' } }; + domains[domain] = { title: domain }; + return { settings: { mongodb: MONGO_URL, mongodbname: dbName(), mongodbcol: 'meshcentral' }, domains: domains }; +} + +function dbName() { + const m = /\/([^/?]+)(\?|$)/.exec(MONGO_URL); + return m ? m[1] : 'meshcentral_test'; +} + +/** A minimal MeshCentral "parent", the same shim plugins/migrate.js uses to drive db.js standalone. */ +function parentShim(domain) { + const cfg = config(domain); + return { + datapath: '/tmp/meshcentral-test-data', + args: { mongodb: cfg.settings.mongodb, mongodbname: cfg.settings.mongodbname, mongodbcol: cfg.settings.mongodbcol }, + config: cfg, + crypto: require('crypto'), fs: require('fs'), path: require('path'), + common: require(path.join(MESH_DIR, 'common.js')), + debug: function () { }, DispatchEvent: function () { }, + GetConnectivityState: function () { return null; }, + webserver: { meshes: {}, users: {}, CreateNodeDispatchTargets: function () { return []; } }, + userGroups: {} + }; +} + +/** Open db.js as a tenant server for `domain`. `env` is applied before setup, since the write guard + * binds itself at setup time. */ +function openDb(domain, env) { + const previous = {}; + const applied = Object.assign({ OPENFRAME_MODE: 'true' }, env || {}); + for (const k in applied) { previous[k] = process.env[k]; process.env[k] = applied[k]; } + + const parent = parentShim(domain); + return new Promise((resolve) => { + const db = require(path.join(MESH_DIR, 'db.js')).CreateDB(parent, function () { + db.SetupDatabase(function () { + for (const k in previous) { if (previous[k] === undefined) { delete process.env[k]; } else { process.env[k] = previous[k]; } } + resolve({ db, parent }); + }); + }); + }); +} + +function seedDocs() { + const old = Date.now() - (400 * 86400000); // far past any expireDevs setting + const perDomain = (d) => ([ + { _id: 'user/' + d + '/admin', type: 'user', name: 'admin', domain: d, siteadmin: 0xFFFFFFFF, links: {} }, + { _id: 'mesh/' + d + '/GRP', type: 'mesh', name: 'OpenFrame', domain: d, mtype: 2, expireDevs: 30, links: {} }, + { _id: 'node/' + d + '/DEV', type: 'node', name: 'device', domain: d, meshid: 'mesh/' + d + '/GRP' }, + { _id: 'ifnode/' + d + '/DEV', domain: d, netif: {} }, + { _id: 'lcnode/' + d + '/DEV', type: 'lastconnect', domain: d, meshid: 'mesh/' + d + '/GRP', time: old }, + { _id: 'ugrp/' + d + '/G1', type: 'ugrp', name: 'group', domain: d, links: {} }, + // An orphan: its device group does not exist. Upstream cleanup() deleted these DB-wide. + { _id: 'node/' + d + '/ORPHAN', type: 'node', name: 'orphan', domain: d, meshid: 'mesh/' + d + '/GONE' } + ]); + return perDomain(A).concat(perDomain(B)); +} + +async function freshDatabase() { + const { MongoClient } = require('mongodb'); + const client = await MongoClient.connect(MONGO_URL); + const db = client.db(dbName()); + for (const c of ['meshcentral', 'serverstats', 'events', 'power', 'smbios']) { + await db.collection(c).deleteMany({}); + } + await db.collection('meshcentral').insertMany(seedDocs()); + return { client, db }; +} + +function ids(docs) { return docs.map((d) => d._id).sort(); } + +// --- Tests ----------------------------------------------------------------- + +test('cleanup() does not delete documents whose device group is missing', async () => { + // Upstream ended cleanup() with deleteMany({meshid: {$nin: meshlist}}) over the whole database. + // On a shared database that deletes other tenants' devices, and an empty meshlist (a transient + // read error) matches everything. The statement is gone; nothing may be deleted here. + const { client, db: raw } = await freshDatabase(); + const { db } = await openDb(A); + try { + // Counted per domain rather than globally: SetupDatabase fires off its own + // DatabaseIdentifier / SchemaVersion writes without waiting, which would race a total. + const countA = () => raw.collection('meshcentral').countDocuments({ domain: A }); + const countB = () => raw.collection('meshcentral').countDocuments({ domain: B }); + const beforeA = await countA(), beforeB = await countB(); + await new Promise((res) => db.cleanup(res)); + assert.strictEqual(await countA(), beforeA); + assert.strictEqual(await countB(), beforeB); + assert.strictEqual(await raw.collection('meshcentral').countDocuments({ _id: 'node/' + B + '/ORPHAN' }), 1); + assert.strictEqual(await raw.collection('meshcentral').countDocuments({ _id: 'node/' + A + '/ORPHAN' }), 1); + } finally { await client.close(); } +}); + +test('removeInactiveDevices only removes devices of this tenant', async () => { + const { client, db: raw } = await freshDatabase(); + const { db, parent } = await openDb(A); + try { + // The boot cache still holds both tenants here on purpose: that is the state the function + // used to read foreign domains from. + const meshes = await new Promise((res) => db.GetAllType('mesh', (err, docs) => res(docs))); + for (const m of meshes) { parent.webserver.meshes[m._id] = m; } + + await new Promise((res) => { db.removeInactiveDevices(false, function () { }); setTimeout(res, 2000); }); + + assert.strictEqual(await raw.collection('meshcentral').countDocuments({ _id: 'node/' + A + '/DEV' }), 0, "own tenant's inactive device should be removed"); + assert.strictEqual(await raw.collection('meshcentral').countDocuments({ _id: 'node/' + B + '/DEV' }), 1, "other tenant's device must be untouched"); + assert.strictEqual(await raw.collection('meshcentral').countDocuments({ _id: 'lcnode/' + B + '/DEV' }), 1); + } finally { await client.close(); } +}); + +test('writes into another tenant are blocked, own-domain writes pass', async () => { + const { client, db: raw } = await freshDatabase(); + const { db } = await openDb(A); + try { + const foreign = await new Promise((res) => db.Set({ _id: 'user/' + B + '/admin', type: 'user', domain: B, name: 'overwritten' }, (err) => res(err))); + assert.ok(foreign instanceof Error, 'a cross-tenant Set must fail its callback rather than hang'); + assert.strictEqual((await raw.collection('meshcentral').findOne({ _id: 'user/' + B + '/admin' })).name, 'admin'); + + const own = await new Promise((res) => db.Set({ _id: 'user/' + A + '/second', type: 'user', domain: A, name: 'second' }, (err) => res(err))); + assert.ok(!own, 'own-domain writes must not be affected'); + assert.strictEqual(await raw.collection('meshcentral').countDocuments({ _id: 'user/' + A + '/second' }), 1); + + // Remove resolves the domain from the id, which carries it even for prefixed keys. + await new Promise((res) => db.Remove('lcnode/' + B + '/DEV', res)); + assert.strictEqual(await raw.collection('meshcentral').countDocuments({ _id: 'lcnode/' + B + '/DEV' }), 1); + } finally { await client.close(); } +}); + +test('the write guard can be turned off for rollback', async () => { + const { client, db: raw } = await freshDatabase(); + const { db } = await openDb(A, { OPENFRAME_CROSS_TENANT_WRITES: 'allow' }); + try { + await new Promise((res) => db.Set({ _id: 'user/' + B + '/admin', type: 'user', domain: B, name: 'overwritten' }, res)); + assert.strictEqual((await raw.collection('meshcentral').findOne({ _id: 'user/' + B + '/admin' })).name, 'overwritten'); + } finally { await client.close(); } +}); + +test('deriveTenantDomain picks the tenant, ignoring the default and share domains', () => { + const { deriveTenantDomain } = require(path.join(MESH_DIR, 'db.js')); + assert.strictEqual(deriveTenantDomain(config(A).domains), A); + // A legacy single-tenant install has only the default domain and must stay unscoped. + assert.strictEqual(deriveTenantDomain({ '': { title: 'MeshCentral' } }), ''); + assert.strictEqual(deriveTenantDomain(null), ''); +}); From 9f928858258de53182926e57b518cb645b3ce94d Mon Sep 17 00:00:00 2001 From: Arsenij Malov Date: Fri, 4 Sep 2026 01:01:24 +0200 Subject: [PATCH 2/4] Scope the plugin permission list and user-targeted commands to one tenant MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two disclosure paths through the shared database, both reachable from a normal tenant admin session (in this product every logged-in user's browser holds one, since the frontend opens control.ashx with the mesh service credentials). getpluginpermissionlist answered with four unscoped GetAllType calls, returning every user in the fleet — ids are user//, so other tenants' domain keys and admin names come with it — plus every user group, device group and device. Its sibling command 'users' does filter by domain, so this handler was bypassing the filter its neighbour applies. Verified against a two-domain database from a real admin session: before, both tenants came back; now, one. changeuserpass looked up parent.users[command.userid] with no domain check at all. The guard above it never trips, because our bootstrap admin *is* a full site administrator, and the group check passes for an admin with no groups — so a tenant admin could set the password and strip 2FA on another tenant's admin, with the write landing in the shared database. Adds the same domain check 'deleteuser' already had, and the same to notifyuser, meshmessenger, emailUser, smsUser and msgUser, which had the same shape (their transports are not configured today, but the lookup was equally unscoped). Adds GetAllTypeForDomain for the scoped reads. It deliberately does not project the type field away the way GetAllTypeNoTypeField does: callers keep these documents and write them back, and a document saved without `type` stops being returned by every type query. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01WAcuUJKfVLEX4KpvpKd1pS --- db.js | 7 +++++++ meshuser.js | 33 ++++++++++++++++++++++----------- test/tenant-isolation.test.js | 19 +++++++++++++++++++ 3 files changed, 48 insertions(+), 11 deletions(-) diff --git a/db.js b/db.js index 3daa9fc245..fec2c5d1ef 100644 --- a/db.js +++ b/db.js @@ -2913,6 +2913,13 @@ module.exports.CreateDB = function (parent, func) { obj.file.find(x, { type: 0 }).toArray(function (err, docs) { func(err, performTypedRecordDecrypt(docs)); }); }; obj.GetAllType = function (type, func) { obj.file.find({ type: type }).toArray(function (err, docs) { func(err, performTypedRecordDecrypt(docs)); }); }; + // Domain-scoped GetAllType. Same query shape as GetAllTypeNoTypeField above, but it + // does NOT project the type field away: callers of this one keep the documents (the + // boot caches, which later save them back), and a document saved without its `type` + // stops being returned by every type query — the user disappears, the mesh drops out + // of any mesh listing. Callers that legitimately want the whole database (the CLI + // branches, export) keep using GetAllType. + obj.GetAllTypeForDomain = function (type, domain, func) { obj.file.find({ type: type, domain: domain }).toArray(function (err, docs) { func(err, performTypedRecordDecrypt(docs)); }); }; obj.GetAllIdsOfType = function (ids, domain, type, func) { obj.file.find({ type: type, domain: domain, _id: { $in: ids } }).toArray(function (err, docs) { func(err, performTypedRecordDecrypt(docs)); }); }; obj.GetUserWithEmail = function (domain, email, func) { obj.file.find({ type: 'user', domain: domain, email: email }).toArray(function (err, docs) { func(err, performTypedRecordDecrypt(docs)); }); }; obj.GetUserWithVerifiedEmail = function (domain, email, func) { obj.file.find({ type: 'user', domain: domain, email: email, emailVerified: true }).toArray(function (err, docs) { func(err, performTypedRecordDecrypt(docs)); }); }; diff --git a/meshuser.js b/meshuser.js index 34013e1a8b..b37d5f2d23 100644 --- a/meshuser.js +++ b/meshuser.js @@ -1967,6 +1967,9 @@ module.exports.CreateMeshUser = function (parent, db, ws, req, args, domain, use if (typeof command.removeMultiFactor != 'boolean') break; if ((command.pass != '') && (common.checkPasswordRequirements(command.pass, domain.passwordrequirements) == false)) break; // Password does not meet requirements + // Every tenant shares one database and parent.users holds all of them, so a + // bare id lookup resolves another tenant's user. Same check as 'deleteuser'. + if ((command.userid.split('/').length != 3) || (command.userid.split('/')[1] != domain.id)) break; var chguser = parent.users[command.userid]; if (chguser) { // If we are not full administrator, we can't change anything on a different full administrator @@ -2020,6 +2023,9 @@ module.exports.CreateMeshUser = function (parent, db, ws, req, args, domain, use if (common.validateString(command.userid, 1, 2048) == false) break; if (common.validateString(command.msg, 1, 4096) == false) break; + // Every tenant shares one database and parent.users holds all of them, so a + // bare id lookup resolves another tenant's user. Same check as 'deleteuser'. + if ((command.userid.split('/').length != 3) || (command.userid.split('/')[1] != domain.id)) break; // Can only perform this operation on other users of our group. var chguser = parent.users[command.userid]; if (chguser == null) break; // This user does not exists @@ -2058,6 +2064,9 @@ module.exports.CreateMeshUser = function (parent, db, ws, req, args, domain, use // Send a notification message to a user if ((user.siteadmin & 2) == 0) break; + // Every tenant shares one database and parent.users holds all of them, so a + // bare id lookup resolves another tenant's user. Same check as 'deleteuser'. + if ((command.userid.split('/').length != 3) || (command.userid.split('/')[1] != domain.id)) break; // Can only perform this operation on other users of our group. var chguser = parent.users[command.userid]; if (chguser == null) break; // This user does not exists @@ -4779,8 +4788,10 @@ module.exports.CreateMeshUser = function (parent, db, ws, req, args, domain, use var result = { users: [], userGroups: [], meshes: [], nodes: [] }; - // Get all users - parent.db.GetAllType('user', function(err, docs) { + // Get all users in this domain. Domain-scoped because every tenant shares one + // database: an unscoped read here returns every tenant's users, and their ids + // (user//) disclose the other tenants' domain keys and admin names. + parent.db.GetAllTypeForDomain('user', domain.id, function(err, docs) { if (docs) { docs.forEach(function(u) { if (u.name && u._id) { @@ -4789,8 +4800,8 @@ module.exports.CreateMeshUser = function (parent, db, ws, req, args, domain, use }); } - // Get all user groups - parent.db.GetAllType('ugrp', function(err, ugrps) { + // Get all user groups in this domain + parent.db.GetAllTypeForDomain('ugrp', domain.id, function(err, ugrps) { if (ugrps) { ugrps.forEach(function(ug) { if (ug.name && ug._id) { @@ -4799,8 +4810,8 @@ module.exports.CreateMeshUser = function (parent, db, ws, req, args, domain, use }); } - // Get all meshes (device groups) - parent.db.GetAllType('mesh', function(err, meshes) { + // Get all meshes (device groups) in this domain + parent.db.GetAllTypeForDomain('mesh', domain.id, function(err, meshes) { if (meshes) { meshes.forEach(function(m) { if (m.name && m._id && !m.deleted) { @@ -4809,8 +4820,8 @@ module.exports.CreateMeshUser = function (parent, db, ws, req, args, domain, use }); } - // Get all nodes (devices) - parent.db.GetAllType('node', function(err, nodes) { + // Get all nodes (devices) in this domain + parent.db.GetAllTypeForDomain('node', domain.id, function(err, nodes) { if (nodes) { // Create a map of meshid to meshname for grouping var meshMap = {}; @@ -6531,7 +6542,7 @@ module.exports.CreateMeshUser = function (parent, db, ws, req, args, domain, use else if (common.validateString(command.subject, 1, 1000) == false) { errMsg = 'Invalid subject message'; } else if (common.validateString(command.msg, 1, 10000) == false) { errMsg = 'Invalid message'; } else { - emailuser = parent.users[command.userid]; + emailuser = ((command.userid.split('/').length == 3) && (command.userid.split('/')[1] == domain.id)) ? parent.users[command.userid] : null; // Domain-scoped: parent.users holds every tenant's users if (emailuser == null) { errMsg = 'Invalid userid'; } else if (emailuser.email == null) { errMsg = 'No validated email address for this user'; } else if (emailuser.emailVerified !== true) { errMsg = 'No validated email address for this user'; } @@ -7032,7 +7043,7 @@ module.exports.CreateMeshUser = function (parent, db, ws, req, args, domain, use else if (common.validateString(command.userid, 1, 2048) == false) { errMsg = "Invalid username"; } else if (common.validateString(command.msg, 1, 160) == false) { errMsg = "Invalid SMS message"; } else { - smsuser = parent.users[command.userid]; + smsuser = ((command.userid.split('/').length == 3) && (command.userid.split('/')[1] == domain.id)) ? parent.users[command.userid] : null; // Domain-scoped: parent.users holds every tenant's users if (smsuser == null) { errMsg = "Invalid username"; } else if (smsuser.phone == null) { errMsg = "No phone number for this user"; } } @@ -7055,7 +7066,7 @@ module.exports.CreateMeshUser = function (parent, db, ws, req, args, domain, use else if (common.validateString(command.userid, 1, 2048) == false) { errMsg = "Invalid username"; } else if (common.validateString(command.msg, 1, 160) == false) { errMsg = "Invalid message"; } else { - msguser = parent.users[command.userid]; + msguser = ((command.userid.split('/').length == 3) && (command.userid.split('/')[1] == domain.id)) ? parent.users[command.userid] : null; // Domain-scoped: parent.users holds every tenant's users if (msguser == null) { errMsg = "Invalid username"; } else if (msguser.msghandle == null) { errMsg = "No messaging service configured for this user"; } } diff --git a/test/tenant-isolation.test.js b/test/tenant-isolation.test.js index 6cb4c27f4f..d6bbf45e9e 100644 --- a/test/tenant-isolation.test.js +++ b/test/tenant-isolation.test.js @@ -103,6 +103,25 @@ function ids(docs) { return docs.map((d) => d._id).sort(); } // --- Tests ----------------------------------------------------------------- +test('GetAllTypeForDomain returns only this tenant and keeps the type field', async () => { + const { client, db: raw } = await freshDatabase(); + const { db } = await openDb(A); + try { + const users = await new Promise((res) => db.GetAllTypeForDomain('user', A, (err, docs) => res(docs))); + assert.deepStrictEqual(ids(users), ['user/' + A + '/admin']); + + // The type field must survive: these documents are held in the boot cache and written back + // later, and a document saved without `type` stops being returned by every type query. + assert.strictEqual(users[0].type, 'user'); + + // Guard against the projection trap by round-tripping the object the way the server does. + // (SetUser takes no callback in MeshCentral, so the write is awaited through Set.) + await new Promise((res) => db.Set(users[0], res)); + const after = await raw.collection('meshcentral').findOne({ _id: 'user/' + A + '/admin' }); + assert.strictEqual(after.type, 'user'); + } finally { await client.close(); } +}); + test('cleanup() does not delete documents whose device group is missing', async () => { // Upstream ended cleanup() with deleteMany({meshid: {$nin: meshlist}}) over the whole database. // On a shared database that deletes other tenants' devices, and an empty meshlist (a transient From 6b2448179e9e6a0a6ebf0d3d294ead6bf3b32370 Mon Sep 17 00:00:00 2001 From: Arsenij Malov Date: Fri, 4 Sep 2026 01:01:24 +0200 Subject: [PATCH 3/4] Load only this tenant's users, device groups and user groups at boot The three startup loads in webserver.js were unscoped, so every pod held every tenant's users, device groups and user groups in memory, keyed by full _id. That is the root the previous two changes grew from: any lookup that does not separately re-check .domain resolves a foreign object, which is how removeInactiveDevices found other tenants' device groups and how changeuserpass reached another tenant's admin. It also makes memory O(fleet) rather than O(tenant), and hands ~heapdump a dump of the whole fleet. The loads now go through GetAllTypeForDomain in OpenFrame mode and stay unchanged elsewhere. Boot census on a two-domain database goes from "users=2 deviceGroups=2" to "users=1 deviceGroups=1". The empty domain is dropped from the changeStream filter at the same time, so the cache and the change feed describe the same set of documents: '' is one shared space in a shared database, and treating it as ours contradicts a cache that now loads this domain alone. Must not ship before the destructive-routine change: cleanup() and removeInactiveDevices relied on these maps being complete. `explain()` confirms {type, domain} uses the existing TypeDomainMesh1 index despite it being sparse, because the query pins `type`. No new index. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01WAcuUJKfVLEX4KpvpKd1pS --- db.js | 7 ++++++- webserver.js | 20 +++++++++++++++++--- 2 files changed, 23 insertions(+), 4 deletions(-) diff --git a/db.js b/db.js index fec2c5d1ef..6bd1dc98e1 100644 --- a/db.js +++ b/db.js @@ -1160,8 +1160,13 @@ module.exports.CreateDB = function (parent, func) { console.log('WARNING: watch() is not a function, MongoDB ChangeStream not supported.'); } else { const tenantDomain = deriveTenantDomain(parent.config.domains); + // Own domain only. The empty domain used to be included here, but it is one + // shared space in a shared database: anything landing in it is visible to every + // tenant server, so treating it as ours contradicts the boot cache, which now + // loads this domain alone. Keeping both in sync matters — a document the cache + // does not hold must not arrive as a change event for an object we never loaded. const changeStreamServerDomains = (process.env.OPENFRAME_MODE === 'true' && tenantDomain) - ? [tenantDomain, ''] + ? [tenantDomain] : Object.keys(parent.config.domains); obj.fileChangeStream = obj.file.watch([{ $match: { $or: [{ 'fullDocument.type': { $in: ['node', 'mesh', 'user', 'ugrp'] } }, { 'operationType': 'delete' }] } }], { fullDocument: 'updateLookup' }); obj.fileChangeStream.on('change', function (change) { diff --git a/webserver.js b/webserver.js index 26af9d2d73..2b6e2a1dc8 100644 --- a/webserver.js +++ b/webserver.js @@ -287,8 +287,22 @@ module.exports.CreateWebServer = function (parent, db, args, certificates, doneF function EscapeHtml(x) { if (typeof x == 'string') return x.replace(/&/g, '&').replace(/>/g, '>').replace(//g, '>').replace(/').replace(/\n/g, '').replace(/\t/g, '  '); if (typeof x == "boolean") return x; if (typeof x == "number") return x; } + // Boot cache scope. All tenant servers share one database, and these three loads are what + // put every tenant's users, device groups and user groups into this pod's memory: the maps + // are keyed by full _id, so any lookup that does not separately re-check .domain resolves a + // foreign object (that is how 'changeuserpass' reached another tenant's admin, and how + // removeInactiveDevices found another tenant's device groups). Loading only our own domain + // removes the class rather than the individual call sites, and makes memory O(tenant) + // instead of O(fleet). Null outside OpenFrame mode, where the load stays unscoped. + const bootCacheDomain = (process.env.OPENFRAME_MODE === 'true') + ? require('./db.js').deriveTenantDomain(parent.config.domains) + : null; + const loadAllOfType = function (type, func) { + if (bootCacheDomain) { obj.db.GetAllTypeForDomain(type, bootCacheDomain, func); } else { obj.db.GetAllType(type, func); } + }; + // Fetch all users from the database, keep this in memory - obj.db.GetAllType('user', function (err, docs) { + loadAllOfType('user', function (err, docs) { if (err != null) { parent.diagLog('ERROR', new Date().toISOString() + ' ERROR: failed to load users from database at startup: ' + err); } obj.common.unEscapeAllLinksFieldName(docs); var domainUserCount = {}, i = 0; @@ -305,7 +319,7 @@ module.exports.CreateWebServer = function (parent, db, args, certificates, doneF // Fetch all device groups (meshes) from the database, keep this in memory // As we load things in memory, we will also be doing some cleaning up. // We will not save any clean up in the database right now, instead it will be saved next time there is a change. - obj.db.GetAllType('mesh', function (err, docs) { + loadAllOfType('mesh', function (err, docs) { // A failed device-group load (e.g. a transient Mongo timeout) silently left // obj.meshes empty, which makes the server orphan EVERY agent ("invalid // domain/mesh, holding connection") until the next restart. Surface it. @@ -342,7 +356,7 @@ module.exports.CreateWebServer = function (parent, db, args, certificates, doneF } catch (censusEx) { parent.diagLog('DEBUG', 'DB census error: ' + censusEx); } // Fetch all user groups from the database, keep this in memory - obj.db.GetAllType('ugrp', function (err, docs) { + loadAllOfType('ugrp', function (err, docs) { obj.common.unEscapeAllLinksFieldName(docs); // Perform user group link cleanup From e062cb1867ac6c44fea0764d1a47febe07463828 Mon Sep 17 00:00:00 2001 From: Arsenij Malov Date: Fri, 4 Sep 2026 01:01:49 +0200 Subject: [PATCH 4/4] Give a tenant to the records that had none: stats, config files, cookie keys MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous changes filtered on `domain`. These four records have no domain at all — upstream stores one of each per database, which on a shared database means one for the whole fleet. serverstats: written with no tenant marker and read with no filter, so every tenant's "My Server" timeline was the summed load of all of them. The write now stamps the domain; the read takes own rows plus rows that predate the field, so the timeline does not go blank while those expire. cfile/*: keyed by bare filename, and with autoSyncConfigFiles every pod pushes its certificates there on boot — last pod to start wins, and all tenants ended up with identical server certificates, hence the identical ServerID that agents pin. The key is now cfile//, and reads fall back to the legacy shared row. That fallback IS the migration: a pod pulls the shared certificate and writes it back under its own key, so certificates stay byte-identical and the ServerID does not change. Regenerating instead would mean reinstalling every agent, since the agent sends the ServerID it expects and verifies the server signature against it. LoginCookieEncryptionKey / InvitationLinkEncryptionKey: one row per database meant one signing key across all pods, so a cookie minted by one tenant's server verified on every other. Now per domain, plus the domain check that was missing where a ?login= token is accepted (every other cookie acceptance point had one). Rotation invalidates cookies issued earlier: nobody is logged out of OpenFrame and no login screen appears, but remote-access sessions open at that moment drop and have to be reopened, so this wants a quiet window. The '*' power marker written at boot is no longer written in OpenFrame mode: the power collection has no domain either, and getPowerTimeline matches '*', so every pod's restarts showed up in every tenant's device timeline. Also resolves the tenant domain once per database instance instead of reading the environment per call — the tests caught removeInactiveDevices scoping itself differently depending on when the query ran. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01WAcuUJKfVLEX4KpvpKd1pS --- db.js | 54 ++++++++++++++++++++++++++++++---- meshcentral.js | 55 ++++++++++++++++++++++++++++------- test/tenant-isolation.test.js | 38 ++++++++++++++++++++++++ webserver.js | 8 ++++- 4 files changed, 137 insertions(+), 18 deletions(-) diff --git a/db.js b/db.js index 6bd1dc98e1..28331be29f 100644 --- a/db.js +++ b/db.js @@ -3075,16 +3075,52 @@ module.exports.CreateDB = function (parent, func) { // Database actions on the Server Stats collection obj.SetServerStats = function (data, func) { obj.serverstatsfile.insertOne(data, func); }; - obj.GetServerStats = function (hours, func) { var t = new Date(); t.setTime(t.getTime() - (60 * 60 * 1000 * hours)); obj.serverstatsfile.find({ time: { $gt: t } }, { _id: 0, cpu: 0 }).toArray(func); }; + // Server stats carry no tenant marker upstream — one server, one timeline. Here every + // tenant server writes into the same collection, so an unfiltered read shows each + // tenant the summed load of the whole fleet. The write side now stamps `domain`; + // rows written before that have none and are still shown during the transition so the + // timeline does not go blank. They expire on their own (expireServerStatsSeconds). + obj.GetServerStats = function (hours, func) { + var t = new Date(); t.setTime(t.getTime() - (60 * 60 * 1000 * hours)); + var q = { time: { $gt: t } }; + var d = openframeDomain(); + if (d != null) { q.$or = [{ domain: d }, { domain: { $exists: false } }]; } + obj.serverstatsfile.find(q, { _id: 0, cpu: 0 }).toArray(func); + }; - // Read a configuration file from the database - obj.getConfigFile = function (path, func) { obj.Get('cfile/' + path, func); } + // Read a configuration file from the database. + // Config files (certificates, terms, images) are keyed by bare filename upstream, so on + // a shared database every tenant server reads and writes the SAME row: whichever pod + // boots last wins, and all tenants end up with identical server certificates — hence an + // identical ServerID, which is exactly what agents pin. + // + // The key is scoped per tenant now. Reads fall back to the legacy shared row, and that + // fallback is what keeps certificates byte-identical through the migration: a pod pulls + // the shared certificate and pushes it back under its own key. Regenerating instead + // would change the ServerID and every installed agent would stop trusting the server. + obj.getConfigFile = function (path, func) { + var d = openframeDomain(); + if (d == null) { obj.Get('cfile/' + path, func); return; } + obj.Get('cfile/' + d + '/' + path, function (err, docs) { + if ((err == null) && (docs != null) && (docs.length > 0)) { func(err, docs); return; } + obj.Get('cfile/' + path, func); // legacy shared row + }); + } // Write a configuration file to the database - obj.setConfigFile = function (path, data, func) { obj.Set({ _id: 'cfile/' + path, type: 'cfile', data: data.toString('base64') }, func); } + obj.setConfigFile = function (path, data, func) { + var d = openframeDomain(); + var doc = { _id: (d == null) ? ('cfile/' + path) : ('cfile/' + d + '/' + path), type: 'cfile', data: data.toString('base64') }; + if (d != null) { doc.domain = d; } + obj.Set(doc, func); + } // List all configuration files - obj.listConfigFiles = function (func) { obj.file.find({ type: 'cfile' }).sort({ _id: 1 }).toArray(func); } + obj.listConfigFiles = function (func) { + var d = openframeDomain(); + var q = (d == null) ? { type: 'cfile' } : { type: 'cfile', _id: { $regex: '^cfile/' + d + '/' } }; + obj.file.find(q).sort({ _id: 1 }).toArray(func); + } // Get database information obj.getDbStats = function (func) { @@ -3341,11 +3377,17 @@ module.exports.CreateDB = function (parent, func) { // Get all configuration files obj.getAllConfigFiles = function (password, func) { + const cfileDomain = openframeDomain(); obj.GetAllType('cfile', function (err, docs) { if (err != null) { func(null); return; } var r = null; for (var i = 0; i < docs.length; i++) { - var name = docs[i]._id.split('/')[1]; + // Keys are 'cfile/' (legacy, shared) or 'cfile//' (scoped), + // so the file name is the last segment, not the second one. Skip other tenants' + // rows: on a shared database GetAllType('cfile') returns the whole fleet's. + var idParts = docs[i]._id.split('/'); + if ((cfileDomain != null) && (idParts.length > 2) && (idParts[1] !== cfileDomain)) continue; + var name = idParts[idParts.length - 1]; var data = obj.decryptData(password, docs[i].data); if (data != null) { if (r == null) { r = {}; } r[name] = data; } } diff --git a/meshcentral.js b/meshcentral.js index 6b365094c1..2a4e7c0103 100644 --- a/meshcentral.js +++ b/meshcentral.js @@ -31,6 +31,24 @@ function CreateMeshCentralServer(config, args) { obj.msgserver = null; // Messaging server, used to sent used messages obj.amtEventHandler = null; obj.pluginHandler = null; + // This pod's tenant, or null outside OpenFrame mode. Used where a record or a database key + // would otherwise be shared by every tenant server writing into the one database. + // Resolved on first use and cached: a server's tenant cannot change while it runs, and every + // caller must get the same answer regardless of when it asks (db.js does the same). + var openframeDomainCache; + obj.openframeDomain = function () { + if (openframeDomainCache === undefined) { + openframeDomainCache = (process.env.OPENFRAME_MODE === 'true') + ? (require('./db.js').deriveTenantDomain(obj.config.domains) || null) + : null; + } + return openframeDomainCache; + }; + // Database keys that must not be one shared row across the fleet. + obj.tenantDbKey = function (baseId) { + const d = obj.openframeDomain(); + return (d == null) ? baseId : (baseId + '_' + d); + }; obj.amtScanner = null; obj.amtManager = null; // Intel AMT manager, used to oversee all Intel AMT devices, activate them and sync policies obj.meshScanner = null; @@ -1758,8 +1776,14 @@ function CreateMeshCentralServer(config, args) { // deletion, the explicit Remove() calls on device deletion, and the TTL indexes. if (process.env.OPENFRAME_MODE !== 'true') { obj.db.cleanup(); } - // Set all nodes to power state of unknown (0) - obj.db.storePowerEvent({ time: new Date(), nodeid: '*', power: 0, s: 1 }, obj.multiServer); // s:1 indicates that the server is starting up. + // Set all nodes to power state of unknown (0). + // Skipped in OpenFrame mode: this marker is stored against the wildcard node id '*' and the + // power collection has no domain field, while getPowerTimeline() selects {nodeid: {$in: + // ['*', nodeid]}} — so on a shared database every pod's boot shows up in every tenant's + // device power timeline. + if (obj.openframeDomain() == null) { + obj.db.storePowerEvent({ time: new Date(), nodeid: '*', power: 0, s: 1 }, obj.multiServer); // s:1 indicates that the server is starting up. + } // Read or setup database configuration values obj.db.Get('dbconfig', function (err, dbconfig) { @@ -2182,23 +2206,28 @@ function CreateMeshCentralServer(config, args) { if ((obj.loginCookieEncryptionKey == null) || (obj.loginCookieEncryptionKey.length != 80)) { addServerWarning("Invalid \"LoginCookieEncryptionKey\" in config.json.", 20); obj.loginCookieEncryptionKey = null; } } - // Login cookie encryption key not set, use one from the database + // Login cookie encryption key not set, use one from the database. + // Per tenant (tenantDbKey) rather than one row for the whole database: this key + // signs login tokens, relay cookies and auth cookies, so a single shared row + // means a cookie minted by one tenant's server verifies on every other one. + // Rotating it invalidates existing sessions for that tenant — expected, and the + // reason this needs a maintenance window. if (obj.loginCookieEncryptionKey == null) { - obj.db.Get('LoginCookieEncryptionKey', function (err, docs) { + obj.db.Get(obj.tenantDbKey('LoginCookieEncryptionKey'), function (err, docs) { if ((docs != null) && (docs.length > 0) && (docs[0].key != null) && (obj.args.logintokengen == null) && (docs[0].key.length >= 160)) { obj.loginCookieEncryptionKey = Buffer.from(docs[0].key, 'hex'); } else { - obj.loginCookieEncryptionKey = obj.generateCookieKey(); obj.db.Set({ _id: 'LoginCookieEncryptionKey', key: obj.loginCookieEncryptionKey.toString('hex'), time: Date.now() }); + obj.loginCookieEncryptionKey = obj.generateCookieKey(); obj.db.Set({ _id: obj.tenantDbKey('LoginCookieEncryptionKey'), key: obj.loginCookieEncryptionKey.toString('hex'), time: Date.now() }); } }); } // Load the invitation link encryption key from the database - obj.db.Get('InvitationLinkEncryptionKey', function (err, docs) { + obj.db.Get(obj.tenantDbKey('InvitationLinkEncryptionKey'), function (err, docs) { if ((docs != null) && (docs.length > 0) && (docs[0].key != null) && (docs[0].key.length >= 160)) { obj.invitationLinkEncryptionKey = Buffer.from(docs[0].key, 'hex'); } else { - obj.invitationLinkEncryptionKey = obj.generateCookieKey(); obj.db.Set({ _id: 'InvitationLinkEncryptionKey', key: obj.invitationLinkEncryptionKey.toString('hex'), time: Date.now() }); + obj.invitationLinkEncryptionKey = obj.generateCookieKey(); obj.db.Set({ _id: obj.tenantDbKey('InvitationLinkEncryptionKey'), key: obj.invitationLinkEncryptionKey.toString('hex'), time: Date.now() }); } }); @@ -2246,6 +2275,10 @@ function CreateMeshCentralServer(config, args) { const node = obj.connectivityByNode[i]; if (node && typeof node.connectivity !== 'undefined' && node.connectivity === 4) { data.conn.am++; } } + // Stamp the tenant: the collection is shared and upstream writes no domain, + // so without this every tenant's "My Server" timeline is the fleet's sum. + const statsDomain = obj.openframeDomain(); + if (statsDomain != null) { data.domain = statsDomain; } if (obj.firstStats === true) { delete obj.firstStats; data.first = true; } if (obj.multiServer != null) { data.s = obj.multiServer.serverid; } obj.db.SetServerStats(data); // Save the stats to the database @@ -3864,7 +3897,7 @@ function CreateMeshCentralServer(config, args) { func('User ' + userid + ' not found.'); return; } else { // Load the login cookie encryption key from the database - obj.db.Get('LoginCookieEncryptionKey', function (err, docs) { + obj.db.Get(obj.tenantDbKey('LoginCookieEncryptionKey'), function (err, docs) { if ((docs.length > 0) && (docs[0].key != null) && (obj.args.logintokengen == null) && (docs[0].key.length >= 160)) { // Key is present, use it. obj.loginCookieEncryptionKey = Buffer.from(docs[0].key, 'hex'); @@ -3872,7 +3905,7 @@ function CreateMeshCentralServer(config, args) { } else { // Key is not present, generate one. obj.loginCookieEncryptionKey = obj.generateCookieKey(); - obj.db.Set({ _id: 'LoginCookieEncryptionKey', key: obj.loginCookieEncryptionKey.toString('hex'), time: Date.now() }, function () { func(obj.encodeCookie({ u: userid, a: 3 }, obj.loginCookieEncryptionKey)); }); + obj.db.Set({ _id: obj.tenantDbKey('LoginCookieEncryptionKey'), key: obj.loginCookieEncryptionKey.toString('hex'), time: Date.now() }, function () { func(obj.encodeCookie({ u: userid, a: 3 }, obj.loginCookieEncryptionKey)); }); } }); } @@ -3882,14 +3915,14 @@ function CreateMeshCentralServer(config, args) { // Show the user login token generation key obj.showLoginTokenKey = function (func) { // Load the login cookie encryption key from the database - obj.db.Get('LoginCookieEncryptionKey', function (err, docs) { + obj.db.Get(obj.tenantDbKey('LoginCookieEncryptionKey'), function (err, docs) { if ((docs.length > 0) && (docs[0].key != null) && (obj.args.logintokengen == null) && (docs[0].key.length >= 160)) { // Key is present, use it. func(docs[0].key); } else { // Key is not present, generate one. obj.loginCookieEncryptionKey = obj.generateCookieKey(); - obj.db.Set({ _id: 'LoginCookieEncryptionKey', key: obj.loginCookieEncryptionKey.toString('hex'), time: Date.now() }, function () { func(obj.loginCookieEncryptionKey.toString('hex')); }); + obj.db.Set({ _id: obj.tenantDbKey('LoginCookieEncryptionKey'), key: obj.loginCookieEncryptionKey.toString('hex'), time: Date.now() }, function () { func(obj.loginCookieEncryptionKey.toString('hex')); }); } }); }; diff --git a/test/tenant-isolation.test.js b/test/tenant-isolation.test.js index d6bbf45e9e..ee5f05345c 100644 --- a/test/tenant-isolation.test.js +++ b/test/tenant-isolation.test.js @@ -186,6 +186,44 @@ test('the write guard can be turned off for rollback', async () => { } finally { await client.close(); } }); +test('server stats are read per tenant, with legacy rows still visible', async () => { + const { client, db: raw } = await freshDatabase(); + const { db } = await openDb(A); + try { + const now = new Date(); + await raw.collection('serverstats').insertMany([ + { time: now, domain: A, conn: { ca: 1 } }, + { time: now, domain: B, conn: { ca: 2 } }, + { time: now, conn: { ca: 3 } } // written before the domain field existed + ]); + const rows = await new Promise((res) => db.GetServerStats(24, (err, docs) => res(docs))); + const seen = rows.map((r) => r.conn.ca).sort(); + assert.deepStrictEqual(seen, [1, 3], 'own rows plus legacy rows, never another tenant'); + } finally { await client.close(); } +}); + +test('config files are stored per tenant and fall back to the shared legacy key', async () => { + const { client, db: raw } = await freshDatabase(); + const { db } = await openDb(A); + try { + await new Promise((res) => db.setConfigFile('agentserver-cert-public.crt', Buffer.from('CERT'), res)); + assert.strictEqual(await raw.collection('meshcentral').countDocuments({ _id: 'cfile/' + A + '/agentserver-cert-public.crt' }), 1); + + // A file that only exists under the legacy shared key must still resolve: that fallback is + // what keeps certificates (and therefore the ServerID agents pin) identical through the + // migration, instead of every pod minting a new one. + await raw.collection('meshcentral').insertOne({ _id: 'cfile/legacy.crt', type: 'cfile', data: Buffer.from('LEGACY').toString('base64') }); + const legacy = await new Promise((res) => db.getConfigFile('legacy.crt', (err, docs) => res(docs))); + assert.strictEqual(legacy.length, 1); + assert.strictEqual(legacy[0]._id, 'cfile/legacy.crt'); + + // Another tenant's file must not be listed. + await raw.collection('meshcentral').insertOne({ _id: 'cfile/' + B + '/secret.crt', type: 'cfile', data: 'x' }); + const listed = await new Promise((res) => db.listConfigFiles((err, docs) => res(docs))); + assert.deepStrictEqual(ids(listed), ['cfile/' + A + '/agentserver-cert-public.crt']); + } finally { await client.close(); } +}); + test('deriveTenantDomain picks the tenant, ignoring the default and share domains', () => { const { deriveTenantDomain } = require(path.join(MESH_DIR, 'db.js')); assert.strictEqual(deriveTenantDomain(config(A).domains), A); diff --git a/webserver.js b/webserver.js index 2b6e2a1dc8..3108604155 100644 --- a/webserver.js +++ b/webserver.js @@ -2254,7 +2254,13 @@ module.exports.CreateWebServer = function (parent, db, args, certificates, doneF // If there is a login token, use that if (req.query.login != null) { var ucookie = parent.decodeCookie(req.query.login, parent.loginCookieEncryptionKey, 60); // Cookie with 1 hour timeout - if ((ucookie != null) && (ucookie.a === 3) && (typeof ucookie.u == 'string')) { user = obj.users[ucookie.u]; } + // The userid inside the cookie must belong to the domain this request was served on. + // Every other cookie acceptance point checks this (see the control websocket handler and + // the relay cookie above); this one did not, and obj.users is keyed by full id, so a + // token naming another domain's user resolved to that user. Sibling to the per-tenant + // cookie key: the key stops a foreign cookie from validating at all, this stops a + // validly-signed one from being used outside its domain. + if ((ucookie != null) && (ucookie.a === 3) && (typeof ucookie.u == 'string') && (ucookie.u.split('/')[1] == domain.id)) { user = obj.users[ucookie.u]; } } // If no token, see if we have an active session