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), ''); +});