From a275306a153e172bc2c8944be9e99e7e3191936d Mon Sep 17 00:00:00 2001 From: Andrew Altshuler Date: Mon, 18 May 2026 04:06:21 +0300 Subject: [PATCH] =?UTF-8?q?policy:=20CLI=20policy=20injection=20=E2=80=94?= =?UTF-8?q?=20local=20writes=20go=20through=20engine=20enforce=20(MR-722)?= =?UTF-8?q?=20(#104)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the CLI side of the policy chassis fan-out. Before this commit, CLI direct-engine writes bypassed Cedar entirely because the CLI never called `Omnigraph::with_policy(...)` for non-`policy validate|test|explain` subcommands. After this commit, every CLI direct-engine writer (change, load, ingest, branch create/delete/merge, schema apply) opens the engine via a new `open_local_db_with_policy(uri, &config)` helper that installs the configured `PolicyEngine` when `policy.file` is set, and threads the resolved actor through to the `_as` writer methods. Actor identity resolution: - New top-level `--as ` global flag on the CLI overrides config. - New `cli.actor` field in `omnigraph.yaml` provides a default actor. - Precedence: `--as` > `cli.actor` > None. - When policy is configured and neither is set, the engine-layer footgun guard fires and the write is denied — silent bypass via "I forgot the actor" is exactly what the guard prevents. - Remote HTTP writes ignore both — bearer-token-resolved server-side. Helpers added in main.rs: - `open_local_db_with_policy(uri, &config) -> Result` — opens the DB and installs the PolicyEngine when configured. Without policy this is identical to a bare `Omnigraph::open`. - `resolve_cli_actor(cli_as, &config) -> Option<&str>` — implements the flag > config > None precedence. Engine: added `load_file_as` to the loader as the actor-aware mirror of `load_file`, so CLI file-path loads flow through the same enforce gate as in-memory `load_as` calls. Test rewrite: `local_cli_policy_tooling_is_end_to_end_while_local_writes_stay_unenforced` was the explicit assertion of the pre-chassis hole. Renamed and split: - `local_cli_policy_tooling_is_end_to_end` — sanity for the read-only policy CLI surfaces (validate/test/explain), unchanged behavior. - `local_cli_change_enforces_engine_layer_policy` — the new assertion: policy installed + no actor → footgun-guard denial; `--as act-bruno` on protected main → Cedar denial; `--as act-ragnor` (admins-write rule) on main → permit, write committed. POLICY_E2E_YAML gains an `admins-write` rule so the permit case has a non-trivial actor to exercise. docs/user/policy.md updated with `cli.actor` + `--as ` usage. Co-authored-by: Claude Opus 4.7 --- Cargo.lock | 1 + crates/omnigraph-cli/Cargo.toml | 1 + crates/omnigraph-cli/src/main.rs | 99 +++++++++++++++++----- crates/omnigraph-cli/tests/system_local.rs | 92 ++++++++++++++++++-- crates/omnigraph-server/src/config.rs | 6 ++ crates/omnigraph/src/loader/mod.rs | 15 +++- docs/user/policy.md | 13 +++ 7 files changed, 199 insertions(+), 28 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index e2005f3..0e3aac4 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4604,6 +4604,7 @@ dependencies = [ "lance-index", "omnigraph-compiler", "omnigraph-engine", + "omnigraph-policy", "omnigraph-server", "predicates", "reqwest", diff --git a/crates/omnigraph-cli/Cargo.toml b/crates/omnigraph-cli/Cargo.toml index 2da4384..6441bd9 100644 --- a/crates/omnigraph-cli/Cargo.toml +++ b/crates/omnigraph-cli/Cargo.toml @@ -15,6 +15,7 @@ path = "src/main.rs" [dependencies] omnigraph = { package = "omnigraph-engine", path = "../omnigraph", version = "0.4.2" } omnigraph-compiler = { path = "../omnigraph-compiler", version = "0.4.2" } +omnigraph-policy = { path = "../omnigraph-policy", version = "0.4.2" } omnigraph-server = { path = "../omnigraph-server", version = "0.4.2" } clap = { workspace = true } color-eyre = { workspace = true } diff --git a/crates/omnigraph-cli/src/main.rs b/crates/omnigraph-cli/src/main.rs index 09fd53f..95114e8 100644 --- a/crates/omnigraph-cli/src/main.rs +++ b/crates/omnigraph-cli/src/main.rs @@ -2,6 +2,7 @@ use std::fs; use std::io::{self, Write}; use std::path::Path; use std::path::PathBuf; +use std::sync::Arc; use clap::{Arg, ArgAction, Args, CommandFactory, FromArgMatches, Parser, Subcommand, ValueEnum}; use color_eyre::eyre::{Result, bail}; @@ -44,6 +45,17 @@ const DEFAULT_BEARER_TOKEN_ENV: &str = "OMNIGRAPH_BEARER_TOKEN"; #[command(about = "Omnigraph graph database CLI")] #[command(version = env!("CARGO_PKG_VERSION"), disable_version_flag = true)] struct Cli { + /// Actor identity for direct-engine writes (MR-722). Overrides + /// `cli.actor` from `omnigraph.yaml`. When the configured policy + /// is in effect, Cedar evaluates this actor against the requested + /// action and scope; with policy configured but neither this flag + /// nor `cli.actor` set, the engine-layer footgun guard fires and + /// the write is denied (no silent bypass). Has no effect on remote + /// HTTP writes — those resolve their actor server-side from the + /// bearer token. + #[arg(long = "as", global = true, value_name = "ACTOR")] + as_actor: Option, + #[command(subcommand)] command: Command, } @@ -685,6 +697,37 @@ fn resolve_policy_engine(config: &OmnigraphConfig) -> Result { PolicyEngine::load(&policy_file, &policy_repo_id(config)) } +/// Open a local-URI repo and, when `policy.file` is configured in +/// `omnigraph.yaml`, install the resolved `PolicyEngine` on the engine +/// handle so every direct-engine write goes through +/// `Omnigraph::enforce(...)` (MR-722). Without a configured policy this +/// is identical to a bare `Omnigraph::open`. +/// +/// Returns owned `Omnigraph`; chained on top of `Omnigraph::open(...)`'s +/// existing future to keep call sites narrow. +async fn open_local_db_with_policy(uri: &str, config: &OmnigraphConfig) -> Result { + let db = Omnigraph::open(uri).await?; + if config.resolve_policy_file().is_some() { + let engine = Arc::new(resolve_policy_engine(config)?); + Ok(db.with_policy(engine as Arc)) + } else { + Ok(db) + } +} + +/// Resolve the CLI's effective actor identity for engine-layer policy +/// (MR-722). Precedence: `--as ` (top-level flag) overrides +/// `cli.actor` from `omnigraph.yaml`; both unset returns `None`. When +/// policy is configured and this returns `None`, the engine-layer +/// footgun guard intentionally denies — silent bypass via "I forgot the +/// actor" is what the guard prevents. +fn resolve_cli_actor<'a>( + cli_as: Option<&'a str>, + config: &'a OmnigraphConfig, +) -> Option<&'a str> { + cli_as.or(config.cli.actor.as_deref()) +} + fn resolve_policy_tests_path(config: &OmnigraphConfig) -> Result { config.resolve_policy_tests_file().ok_or_else(|| { color_eyre::eyre::eyre!( @@ -1553,19 +1596,22 @@ async fn execute_change( query_name: Option<&str>, branch: &str, params_json: Option<&Value>, + config: &OmnigraphConfig, + cli_as_actor: Option<&str>, ) -> Result { let (selected_name, query_params) = select_named_query(query_source, query_name)?; let params = query_params_from_json(&query_params, params_json)?; - let mut db = Omnigraph::open(uri).await?; + let db = open_local_db_with_policy(uri, config).await?; + let actor = resolve_cli_actor(cli_as_actor, config); let result = db - .mutate(branch, query_source, &selected_name, ¶ms) + .mutate_as(branch, query_source, &selected_name, ¶ms, actor) .await?; Ok(ChangeOutput { branch: branch.to_string(), query_name: selected_name, affected_nodes: result.affected_nodes, affected_edges: result.affected_edges, - actor_id: None, + actor_id: actor.map(String::from), }) } @@ -1689,9 +1735,10 @@ async fn main() -> Result<()> { let config = load_cli_config(config.as_ref())?; let uri = resolve_local_uri(&config, uri, target.as_deref(), "load")?; let branch = resolve_branch(&config, branch, None, "main"); - let mut db = Omnigraph::open(&uri).await?; + let db = open_local_db_with_policy(&uri, &config).await?; + let actor = resolve_cli_actor(cli.as_actor.as_deref(), &config); let result = db - .load_file(&branch, &data.to_string_lossy(), mode.into()) + .load_file_as(&branch, &data.to_string_lossy(), mode.into(), actor) .await?; let payload = LoadOutput { uri: &uri, @@ -1748,9 +1795,16 @@ async fn main() -> Result<()> { ) .await? } else { - let mut db = Omnigraph::open(&uri).await?; + let db = open_local_db_with_policy(&uri, &config).await?; + let actor = resolve_cli_actor(cli.as_actor.as_deref(), &config); let result = db - .ingest_file(&branch, Some(&from), &data.to_string_lossy(), mode.into()) + .ingest_file_as( + &branch, + Some(&from), + &data.to_string_lossy(), + mode.into(), + actor, + ) .await?; ingest_output(&uri, &result, None) }; @@ -1787,14 +1841,15 @@ async fn main() -> Result<()> { ) .await? } else { - let mut db = Omnigraph::open(&uri).await?; - db.branch_create_from(ReadTarget::branch(&from), &name) + let db = open_local_db_with_policy(&uri, &config).await?; + let actor = resolve_cli_actor(cli.as_actor.as_deref(), &config); + db.branch_create_from_as(ReadTarget::branch(&from), &name, actor) .await?; BranchCreateOutput { uri: uri.clone(), from: from.clone(), name: name.clone(), - actor_id: None, + actor_id: actor.map(String::from), } }; if json { @@ -1857,12 +1912,13 @@ async fn main() -> Result<()> { ) .await? } else { - let mut db = Omnigraph::open(&uri).await?; - db.branch_delete(&name).await?; + let db = open_local_db_with_policy(&uri, &config).await?; + let actor = resolve_cli_actor(cli.as_actor.as_deref(), &config); + db.branch_delete_as(&name, actor).await?; BranchDeleteOutput { uri: uri.clone(), name: name.clone(), - actor_id: None, + actor_id: actor.map(String::from), } }; if json { @@ -1897,13 +1953,14 @@ async fn main() -> Result<()> { ) .await? } else { - let mut db = Omnigraph::open(&uri).await?; - let outcome = db.branch_merge(&source, &into).await?; + let db = open_local_db_with_policy(&uri, &config).await?; + let actor = resolve_cli_actor(cli.as_actor.as_deref(), &config); + let outcome = db.branch_merge_as(&source, &into, actor).await?; BranchMergeOutput { source: source.clone(), target: into.clone(), outcome: outcome.into(), - actor_id: None, + actor_id: actor.map(String::from), } }; if json { @@ -2051,11 +2108,13 @@ async fn main() -> Result<()> { ) .await? } else { - let mut db = Omnigraph::open(&uri).await?; + let db = open_local_db_with_policy(&uri, &config).await?; + let actor = resolve_cli_actor(cli.as_actor.as_deref(), &config); let result = db - .apply_schema_with_options( + .apply_schema_as( &schema_source, omnigraph::db::SchemaApplyOptions { allow_data_loss }, + actor, ) .await?; schema_apply_output(&uri, result) @@ -2340,6 +2399,8 @@ async fn main() -> Result<()> { query_name.as_deref(), &branch, params_json.as_ref(), + &config, + cli.as_actor.as_deref(), ) .await? }; @@ -2397,7 +2458,7 @@ async fn main() -> Result<()> { } => { let config = load_cli_config(config.as_ref())?; let uri = resolve_uri(&config, uri, target.as_deref())?; - let mut db = Omnigraph::open(&uri).await?; + let db = Omnigraph::open(&uri).await?; let stats = db.optimize().await?; if json { let value = serde_json::json!({ diff --git a/crates/omnigraph-cli/tests/system_local.rs b/crates/omnigraph-cli/tests/system_local.rs index d890603..fd9441d 100644 --- a/crates/omnigraph-cli/tests/system_local.rs +++ b/crates/omnigraph-cli/tests/system_local.rs @@ -30,6 +30,11 @@ rules: actors: { group: admins } actions: [branch_merge] target_branch_scope: protected + - id: admins-write + allow: + actors: { group: admins } + actions: [change] + branch_scope: any "#; const POLICY_E2E_TESTS_YAML: &str = r#" @@ -949,12 +954,14 @@ query vector_search($q: String) { // surface is the same engine path the unit test already covers. #[test] -fn local_cli_policy_tooling_is_end_to_end_while_local_writes_stay_unenforced() { +fn local_cli_policy_tooling_is_end_to_end() { + // Sanity check for the read-only policy CLI surfaces. These don't + // mutate the graph — they just parse and evaluate the policy file — + // so they don't depend on PR #4's engine-side enforcement. let repo = SystemRepo::loaded(); let config = repo.write_config("omnigraph-policy.yaml", &local_policy_config(&repo)); repo.write_config("policy.yaml", POLICY_E2E_YAML); repo.write_config("policy.tests.yaml", POLICY_E2E_TESTS_YAML); - let mutation_file = insert_person_query(&repo, "system-local-policy-change.gq"); let validate = output_success( cli() @@ -984,8 +991,34 @@ fn local_cli_policy_tooling_is_end_to_end_while_local_writes_stay_unenforced() { let explain_stdout = stdout_string(&explain); assert!(explain_stdout.contains("decision: deny")); assert!(explain_stdout.contains("branch: main")); +} - let local_change = parse_stdout_json(&output_success( +#[test] +fn local_cli_change_enforces_engine_layer_policy() { + // Asserts MR-722 PR #4: when `policy.file` is configured in + // `omnigraph.yaml`, the CLI loads PolicyEngine into Omnigraph and + // every direct-engine write hits `enforce(action, scope, actor)` — + // identical to what the HTTP server gets, regardless of transport. + // + // Three cases, each discriminating: + // + // 1. Policy installed, no actor source (no `cli.actor` in config, + // no `--as` flag) → engine-layer footgun guard fires; CLI exits + // non-zero with a "no actor" message. Silent bypass is the bug + // PR #4 prevents. + // 2. Policy installed, `--as act-bruno`, change on main → Cedar + // denies (bruno can change unprotected branches; main is + // protected). CLI exits non-zero with a "denied" message. + // 3. Policy installed, `--as act-ragnor`, change on main → + // Cedar permits (admins-write rule). Write succeeds and the + // inserted row is readable. + let repo = SystemRepo::loaded(); + let config = repo.write_config("omnigraph-policy.yaml", &local_policy_config(&repo)); + repo.write_config("policy.yaml", POLICY_E2E_YAML); + let mutation_file = insert_person_query(&repo, "system-local-policy-change.gq"); + + // Case 1: policy configured, no actor threaded → footgun guard. + let no_actor = output_failure( cli() .arg("change") .arg("--config") @@ -993,12 +1026,55 @@ fn local_cli_policy_tooling_is_end_to_end_while_local_writes_stay_unenforced() { .arg("--query") .arg(&mutation_file) .arg("--params") - .arg(r#"{"name":"PolicyLocal","age":44}"#) + .arg(r#"{"name":"NoActorPerson","age":1}"#) + .arg("--json"), + ); + let no_actor_stderr = String::from_utf8_lossy(&no_actor.stderr); + assert!( + no_actor_stderr.contains("no actor"), + "expected 'no actor' footgun message, got stderr: {no_actor_stderr}" + ); + + // Case 2: `--as act-bruno` against protected main → denied. + let denied = output_failure( + cli() + .arg("--as") + .arg("act-bruno") + .arg("change") + .arg("--config") + .arg(&config) + .arg("--query") + .arg(&mutation_file) + .arg("--params") + .arg(r#"{"name":"BrunoOnMain","age":2}"#) + .arg("--json"), + ); + let denied_stderr = String::from_utf8_lossy(&denied.stderr); + assert!( + denied_stderr.contains("denied"), + "expected 'denied' message for bruno/main, got stderr: {denied_stderr}" + ); + + // Case 3: `--as act-ragnor` against main → permitted by admins-write. + let allowed = parse_stdout_json(&output_success( + cli() + .arg("--as") + .arg("act-ragnor") + .arg("change") + .arg("--config") + .arg(&config) + .arg("--query") + .arg(&mutation_file) + .arg("--params") + .arg(r#"{"name":"RagnorOnMain","age":3}"#) .arg("--json"), )); - assert_eq!(local_change["branch"], "main"); - assert_eq!(local_change["affected_nodes"], 1); + assert_eq!(allowed["branch"], "main"); + assert_eq!(allowed["affected_nodes"], 1); + assert_eq!(allowed["actor_id"], "act-ragnor"); + // Verify the row landed — proves the write actually committed, not + // just that enforce returned Ok and silently dropped the work. let verify = parse_stdout_json(&output_success( cli() .arg("read") @@ -1008,9 +1084,9 @@ fn local_cli_policy_tooling_is_end_to_end_while_local_writes_stay_unenforced() { .arg("--name") .arg("get_person") .arg("--params") - .arg(r#"{"name":"PolicyLocal"}"#) + .arg(r#"{"name":"RagnorOnMain"}"#) .arg("--json"), )); assert_eq!(verify["row_count"], 1); - assert_eq!(verify["rows"][0]["p.name"], "PolicyLocal"); + assert_eq!(verify["rows"][0]["p.name"], "RagnorOnMain"); } diff --git a/crates/omnigraph-server/src/config.rs b/crates/omnigraph-server/src/config.rs index 035ad74..7145ff2 100644 --- a/crates/omnigraph-server/src/config.rs +++ b/crates/omnigraph-server/src/config.rs @@ -46,6 +46,12 @@ pub struct CliDefaults { pub output_format: Option, pub table_max_column_width: Option, pub table_cell_layout: Option, + /// Default actor identity for CLI direct-engine writes (MR-722). + /// Used when `policy.file` is configured and the operator hasn't + /// passed `--as ` on the command line. With policy configured + /// and neither this nor `--as` set, the engine-layer footgun guard + /// fires (no silent bypass). + pub actor: Option, } #[derive(Debug, Clone, Default, Serialize, Deserialize)] diff --git a/crates/omnigraph/src/loader/mod.rs b/crates/omnigraph/src/loader/mod.rs index 50abfe3..878dcfe 100644 --- a/crates/omnigraph/src/loader/mod.rs +++ b/crates/omnigraph/src/loader/mod.rs @@ -217,9 +217,22 @@ impl Omnigraph { branch: &str, path: &str, mode: LoadMode, + ) -> Result { + self.load_file_as(branch, path, mode, None).await + } + + /// Read a file into memory and delegate to `load_as`. Used by the + /// CLI's `omnigraph load` so file-path-based writes flow through + /// the same engine-layer policy gate as in-memory `load_as` calls. + pub async fn load_file_as( + &self, + branch: &str, + path: &str, + mode: LoadMode, + actor_id: Option<&str>, ) -> Result { let data = std::fs::read_to_string(path).map_err(|e| OmniError::Io(e))?; - self.load(branch, &data, mode).await + self.load_as(branch, &data, mode, actor_id).await } async fn load_direct_on_branch( diff --git a/docs/user/policy.md b/docs/user/policy.md index 974323c..8550fb8 100644 --- a/docs/user/policy.md +++ b/docs/user/policy.md @@ -27,15 +27,28 @@ OmniGraph integrates AWS Cedar (`cedar-policy = 4.9`) for ABAC. policy: file: ./policy.yaml # Cedar rules + groups tests: ./policy.tests.yaml # declarative test cases + +cli: + actor: act-andrew # default actor for CLI direct-engine writes ``` Each rule must use exactly one of `branch_scope` or `target_branch_scope`. +`cli.actor` is the default actor identity for CLI direct-engine writes +when `policy.file` is configured. Override per-invocation with `--as +` (top-level flag) — `--as` wins, otherwise `cli.actor` is used, +otherwise no actor. With policy configured and neither set, the +engine-layer footgun guard intentionally denies the write (silent bypass +via "I forgot the actor" is exactly what the guard prevents). Remote +HTTP writes ignore both — they resolve their actor server-side from the +bearer token. + ## CLI - `omnigraph policy validate` — parse + count actors, exit 1 on parse error. - `omnigraph policy test` — run cases in `policy.tests.yaml`, exit 1 on any expectation mismatch. - `omnigraph policy explain --actor … --action … [--branch …] [--target-branch …]` — show decision and matched rule. +- `omnigraph --as ` — set the actor for the duration of one invocation. Effective for `change`, `load`, `ingest`, `branch create|delete|merge`, and `schema apply` against local URIs. No-op against remote HTTP URIs (actor is bearer-token-resolved server-side). ## Enforcement