Przeglądaj źródła

fix(relations): T7 review - CheckRelation must not be an existence oracle

CheckRelation resolved the relation (to learn its child collection to
gate on) before checking access, so a nonexistent relation and an
existing-but-unauthorized one returned distinguishable responses -
OK/"does not exist" vs PERMISSION_DENIED. A non-admin caller could map
which relation names exist by probing, exactly what gate()'s comment
warns against and inconsistent with every sibling relation RPC.

Fix: gate with requireAnyAdmin() before any lookup, matching
CreateRelation/DropRelation/ListRelations/GetRelationInfo. Simplest
option - gating on `child` still needs the relation resolved first to
know what `child` is, so it doesn't actually fix the ordering.

Also, from the same review:
- Corrected a wrong claim in CreateRelation's failure comment: a fully
  failed bootstrap scan leaves the index sub-db NOT CREATED (one write
  txn, aborts uncommitted), so `relations check` cannot find what it
  missed - only the next restart's self-heal recovers. The code was
  already right; only the comment overstated its own coverage.
- Noted explicitly (not changed) that boot-time self-heal still has no
  dedicated test; Task 9 has the DatabaseService fixture for it.

New test: tests/load_test/test_policy_enforcement.cpp Phase 10 - a
stranger with no policy gets byte-identical denial text for an existing
relation and a nonexistent one; admin still tells them apart. Also fixed
that script's hardcoded ROOT path so it runs against this worktree.

test_policy_enforcement.sh: 26/26 (was ~21; +7 new assertions).
test_relation_enforcement 45/45, test_relation_index 43/43,
test_subdb_identity 236/236 - unchanged, as expected (fix is confined to
gRPC-handler gate ordering, which none of the three exercise).
fszontagh 1 miesiąc temu
rodzic
commit
01c53ea2a9

+ 8 - 0
client/include/smartbotic/database/client.hpp

@@ -893,6 +893,14 @@ public:
      * not `childCollection`'s row count, but that is still a full index
      * walk. Treat this as an operator/migration tool, not something to call
      * on a hot path or in a request-serving loop.
+     *
+     * Admin-only, like the other relation-management calls (createRelation,
+     * dropRelation, listRelations, getRelationInfo) - NOT a per-collection
+     * read on the child, even though it changes nothing. Answering requires
+     * looking the relation up first to learn its child collection, and
+     * gating after that lookup would make "relation does not exist" and
+     * "relation exists but you can't read it" distinguishable responses -
+     * a probe-able existence oracle.
      */
     [[nodiscard]] CheckRelationResult checkRelation(const std::string& name);
 

+ 7 - 3
proto/database.proto

@@ -92,9 +92,13 @@ service DatabaseService {
     // parents referenced, not collection size, but on a very large child
     // collection that is still a full index walk - treat this as an
     // operator/migration tool, not something to call on a hot path.
-    // Gated as an ordinary per-collection read on the relation's CHILD
-    // collection (same reasoning as DescribeDelete: it changes nothing, but
-    // reports facts about data in that collection).
+    // Admin-only, like the other relation-management RPCs above (NOT gated
+    // as a per-collection read on `child`, despite being read-only and
+    // reporting facts about that collection's data): answering requires
+    // resolving the relation first to learn its `child`, and gating after
+    // that lookup would make a nonexistent-relation response distinguishable
+    // from an existing-but-unauthorized one - a probe-able existence oracle,
+    // which is exactly what gate() exists to prevent elsewhere.
     rpc CheckRelation(CheckRelationRequest) returns (CheckRelationResponse);
 
     // Collection configuration

+ 29 - 13
service/src/database_grpc_impl.cpp

@@ -2772,10 +2772,17 @@ grpc::Status DatabaseGrpcImpl::CreateRelation(
         } catch (const std::exception& e) {
             // The declaration is already persisted and armed for future
             // writes; failing to backfill existing rows must not roll that
-            // back (the relation is still an improvement over nothing, and
-            // `relations check` can find what the backfill missed). Loud,
-            // because a skipped backfill means pre-existing children stay
-            // invisible to enforcement until re-run.
+            // back (the relation is still an improvement over nothing - it
+            // will maintain postings correctly going forward). Loud, because
+            // build_relation_index is one write txn: on a throw it aborts
+            // uncommitted, so the sub-db is left NOT CREATED at all (an
+            // aborted CREATE does not persist), not partially populated.
+            // ⚠ That means `relations check` cannot find what this missed -
+            // check_relation_dangling treats a missing index sub-db as
+            // "nothing to report" (relation_index_exists() == false), not
+            // "everything dangling", by design (see its own comment). The
+            // only path that recovers from a fully-failed backfill today is
+            // the next restart's applyRelationDeclarations() self-heal.
             spdlog::error("v2.11 relations: bootstrap scan failed for '{}': {}",
                           r.name, e.what());
         }
@@ -2948,6 +2955,24 @@ grpc::Status DatabaseGrpcImpl::CheckRelation(
     const pb::CheckRelationRequest* request,
     pb::CheckRelationResponse* response
 ) {
+    // Admin-gated BEFORE the lookup, matching every other relation
+    // RPC (CreateRelation/DropRelation/ListRelations/GetRelationInfo) —
+    // and unlike the first cut of this handler, which resolved the relation
+    // (to learn its `child` collection to gate on) before checking access.
+    // That made CheckRelation a distinguishable existence oracle: a
+    // nonexistent relation returned success=false with an explicit "does
+    // not exist" message, while an existing relation the caller could not
+    // read returned PERMISSION_DENIED — two shapes a caller could tell
+    // apart by probing, exactly what gate()'s own comment warns against
+    // ("would let a caller map the access model by probing"). Gating on
+    // `child` first would need the same resolve-before-gate order, so it
+    // can't fix this without deciding what an UNRESOLVABLE name gates on
+    // instead — admin-first sidesteps that entirely and matches every
+    // sibling relation RPC.
+    if (auto st = requireAnyAdmin(context, "CheckRelation"); !st.ok()) {
+        return st;
+    }
+
     auto r = relation_manager_.getRelation(request->name());
     if (!r) {
         response->set_success(false);
@@ -2955,15 +2980,6 @@ grpc::Status DatabaseGrpcImpl::CheckRelation(
         return grpc::Status::OK;
     }
 
-    // Gated as an ordinary per-collection READ on the CHILD collection, not
-    // admin - same reasoning as DescribeDelete: this changes nothing, but
-    // the dangling parent ids and sample child ids it reports are facts
-    // about data in `child`.
-    smartbotic::database::Decision dec;
-    if (auto st = gate(context, r->child, smartbotic::database::Access::Read, dec); !st.ok()) {
-        return st;
-    }
-
     try {
         const auto rn = smartbotic::database::resolveCollection(r->name);
         const auto rp = smartbotic::database::resolveCollection(r->parent);

+ 34 - 0
tests/load_test/test_policy_enforcement.cpp

@@ -125,6 +125,40 @@ int main(int argc, char** argv) {
     ck(denied([&]{ (void)reader.get("_policies", "secured:reader"); }),
        "a non-admin cannot read _policies - it describes the access model");
 
+    // ---- Phase 10: v2.11.0 T7 review fix — CheckRelation must not become
+    // an existence oracle. The first cut resolved the relation (to find its
+    // child collection to gate on) BEFORE checking access, so a nonexistent
+    // relation and an existing-but-unauthorized one returned distinguishable
+    // responses (OK/success=false/"does not exist" vs. PERMISSION_DENIED).
+    // A non-admin caller could then map which relation names exist by
+    // probing. Fixed by gating admin-only, before any lookup — this proves
+    // it: a stranger (no policy at all) must see byte-identical denials for
+    // a relation that exists and one that does not.
+    ck(ops.createRelation("docs_rel", "docs", "refId", "docs"),
+       "admin can declare a relation to have something real to probe against");
+
+    auto existing = stranger.checkRelation("docs_rel");
+    auto missing = stranger.checkRelation("does_not_exist_rel");
+    ck(!existing.success, "stranger denied on an EXISTING relation");
+    ck(!missing.success, "stranger denied on a NONEXISTENT relation");
+    ck(!existing.error.empty() && existing.error == missing.error,
+       "identical denial text for both - a stranger cannot tell 'exists but "
+       "denied' apart from 'does not exist' by probing");
+    ck(existing.error.find("PERMISSION_DENIED") != std::string::npos ||
+       existing.error.find("access denied") != std::string::npos,
+       "the denial is in fact the admin gate, not some other failure "
+       "(so this test isn't accidentally passing by both sides erroring "
+       "for unrelated reasons)");
+
+    // The admin path still works and still distinguishes the two cases -
+    // the fix must not have made CheckRelation useless for its actual
+    // audience, only opaque to callers who cannot use it at all.
+    auto adminExisting = ops.checkRelation("docs_rel");
+    auto adminMissing = ops.checkRelation("does_not_exist_rel");
+    ck(adminExisting.success, "admin succeeds on the existing relation");
+    ck(!adminMissing.success && adminMissing.error.find("does not exist") != std::string::npos,
+       "admin still gets a real 'does not exist' for a bogus name");
+
     std::cout << "\npassed=" << pass << " failed=" << fail << "\n";
     return fail == 0 ? 0 : 1;
 }

+ 1 - 1
tests/load_test/test_policy_enforcement.sh

@@ -10,7 +10,7 @@
 set -euo pipefail
 cd "$(dirname "$0")"
 
-ROOT=/data/smartbotic-database
+ROOT="$(cd "$(dirname "$0")/../.." && pwd)"
 DIR=/tmp/sbdb-policy-e2e
 PORT=9012