mirror of
https://github.com/ModernRelay/omnigraph.git
synced 2026-06-12 01:45:14 +02:00
mr-668: server-scoped actions always require explicit policy (green)
`server_graphs_list` returned 200 in Open mode (`--unauthenticated`, no tokens, no policy) because `authorize_request`'s no-policy fallback only denied when `actor.is_some()` AND action != Read. In Open mode `actor: None`, so the denial branch never fired and the call returned `Ok(())` — leaking the registry (graph IDs + URIs that may contain S3 bucket paths or internal hostnames) to any unauthenticated caller. The docstring on `server_graphs_list` claimed it was "Cedar-gated" and that the server should "not leak the registry until the operator explicitly authorizes it" — docstring intent and code disagreed. Symptomatic fix: special-case GraphList. Breaks the moment another server-scoped action (`graph_create`, `graph_delete`) is added. Correct-by-design fix: tie authorization to the action's `resource_kind()`. Server-scoped actions (`PolicyResourceKind::Server`) always require explicit policy authorization — there is no runtime state where they're served by default. Per-graph actions keep the existing default-deny logic (DefaultDeny denies non-Read for authenticated actors; Open mode allows everything per the operator's `--unauthenticated` opt-in for graph DATA, but not for server topology). The fix uses the existing `PolicyResourceKind` enum that #119 already added — no new abstraction. Future server-scoped actions (runtime `graph_create`/`graph_delete` when the cluster catalog ships) automatically pick up the same enforcement without any per-action handler change. Changes: * `crates/omnigraph-server/src/lib.rs:51` — re-export `PolicyResourceKind` (the kind discriminator was already public on the omnigraph-policy crate; needed in scope here). * `crates/omnigraph-server/src/lib.rs:1457` — `authorize_request`'s no-policy fallback gains a server-scoped-action check that fires before the actor-based default-deny logic. Error message names the failure mode and points at `server.policy.file`. * `crates/omnigraph-server/tests/server.rs:5037` — `get_graphs_with_server_policy_authorizes_per_cedar` extended to register two graphs in non-alphabetical order and assert the admin-200 response is sorted alphabetically. Restores the sort-order coverage that lived in `get_graphs_lists_registered_graphs_in_multi_mode` before the red commit renamed it to assert denial. Also bundles a small adjacent cleanup that the bot-review flagged: * `crates/omnigraph-server/src/graph_id.rs:124` — drop the unreachable `"openapi.json"` entry from `is_reserved`. The regex `^[a-zA-Z0-9-]{1,64}$` rejects every dot-containing name before `is_reserved` can run, so dotted entries in this list were dead code that misled readers into thinking the list needed to cover them. Comment now names the structural exclusion. The `rejects_reserved_route_names` test loses its `openapi.json` row (covered by `rejects_dots` via the regex). Closes the "server-scoped management actions silently leak in Open mode" class. Red test from the previous commit (`get_graphs_denied_in_open_mode_without_server_policy`) turns green; all 78 server integration tests + 76 lib tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
c5a6a7125a
commit
65e53933fb
3 changed files with 83 additions and 38 deletions
|
|
@ -119,13 +119,16 @@ fn regex() -> &'static Regex {
|
|||
|
||||
/// Reserved `graph_id` values that the regex alone wouldn't catch.
|
||||
/// The leading-underscore rule already excludes every engine-managed
|
||||
/// filename pattern (`_schema.pg`, `__manifest`, etc.); this list covers
|
||||
/// route-prefix collisions and standard endpoint names.
|
||||
/// filename pattern (`_schema.pg`, `__manifest`, etc.); the regex
|
||||
/// `^[a-zA-Z0-9-]{1,64}$` (see `regex()`) additionally rejects every
|
||||
/// dot-containing name structurally — `openapi.json` and friends
|
||||
/// never reach this check.
|
||||
///
|
||||
/// This list only needs to cover route-prefix collisions and
|
||||
/// top-level endpoint names whose spellings DO satisfy the regex
|
||||
/// (no dots, no underscores).
|
||||
fn is_reserved(value: &str) -> bool {
|
||||
matches!(
|
||||
value,
|
||||
"policies" | "healthz" | "openapi" | "openapi.json" | "graphs"
|
||||
)
|
||||
matches!(value, "policies" | "healthz" | "openapi" | "graphs")
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
|
|
@ -205,7 +208,11 @@ mod tests {
|
|||
|
||||
#[test]
|
||||
fn rejects_reserved_route_names() {
|
||||
for bad in ["policies", "healthz", "openapi", "openapi.json", "graphs"] {
|
||||
// Names that satisfy the regex but are still reserved because
|
||||
// they'd collide with top-level route prefixes / endpoint names.
|
||||
// Dot-containing names (e.g. `openapi.json`) are rejected by the
|
||||
// regex, not this list — `rejects_dots` above covers them.
|
||||
for bad in ["policies", "healthz", "openapi", "graphs"] {
|
||||
assert!(
|
||||
GraphId::try_from(bad).is_err(),
|
||||
"expected reject (reserved): {bad}"
|
||||
|
|
|
|||
|
|
@ -50,7 +50,7 @@ use omnigraph_compiler::query::parser::parse_query;
|
|||
use omnigraph_compiler::{JsonParamMode, ParamMap};
|
||||
pub use policy::{
|
||||
PolicyAction, PolicyCompiler, PolicyConfig, PolicyDecision, PolicyEngine, PolicyExpectation,
|
||||
PolicyRequest, PolicyTestConfig,
|
||||
PolicyRequest, PolicyResourceKind, PolicyTestConfig,
|
||||
};
|
||||
use serde::Deserialize;
|
||||
use serde_json::Value;
|
||||
|
|
@ -1455,20 +1455,38 @@ fn authorize_request(
|
|||
request: PolicyRequest,
|
||||
) -> std::result::Result<(), ApiError> {
|
||||
let Some(engine) = policy else {
|
||||
// MR-723 default-deny path. We're here when no PolicyEngine is
|
||||
// installed. Two startup-validated shapes can reach this:
|
||||
// No PolicyEngine installed. Three runtime states can reach this:
|
||||
//
|
||||
// * **Open mode** (`--unauthenticated`): no tokens, no policy.
|
||||
// `require_bearer_auth` short-circuits before this is called,
|
||||
// but defense in depth — if a future change makes the
|
||||
// middleware call here for an unauthenticated request, we
|
||||
// want every action to remain Ok rather than 403. The
|
||||
// operator opted in.
|
||||
// Per-graph operations are open by operator opt-in (they
|
||||
// accepted "trust the network" for graph data).
|
||||
// * **DefaultDeny mode**: tokens configured but no policy. The
|
||||
// request went through bearer auth, so `actor` is Some and
|
||||
// identifies a known actor. Only `Read` is permitted; every
|
||||
// other action returns 403. This closes the "configured auth
|
||||
// but forgot the policy file" trap from MR-723.
|
||||
// request went through bearer auth, so `actor` is Some. Only
|
||||
// per-graph `Read` is permitted; other per-graph actions
|
||||
// return 403. Closes the "configured auth but forgot the
|
||||
// policy file" trap from MR-723.
|
||||
// * Either of the above with a **server-scoped** action
|
||||
// (`graph_list`, future `graph_create`/`graph_delete`).
|
||||
//
|
||||
// Server-scoped actions are always denied here, regardless of
|
||||
// mode or actor presence. The management surface leaks server
|
||||
// topology (graph IDs + URIs that may contain S3 bucket paths
|
||||
// or internal hostnames) — operators who opted into Open mode
|
||||
// accepted exposure of graph DATA, not exposure of server
|
||||
// topology. Closing the management surface by default in every
|
||||
// runtime state means the docstring contract on
|
||||
// `server_graphs_list` ("don't leak the registry until the
|
||||
// operator explicitly authorizes it") holds uniformly; the
|
||||
// operator's only path to enabling it is configuring an
|
||||
// explicit `server.policy.file` in omnigraph.yaml.
|
||||
if request.action.resource_kind() == PolicyResourceKind::Server {
|
||||
return Err(ApiError::forbidden(
|
||||
"server-scoped actions require an explicit `server.policy.file` \
|
||||
configured in omnigraph.yaml — the management surface is closed \
|
||||
by default in every runtime state, including --unauthenticated, \
|
||||
so that server topology is never exposed without operator opt-in.",
|
||||
));
|
||||
}
|
||||
if actor.is_some() && request.action != PolicyAction::Read {
|
||||
return Err(ApiError::forbidden(
|
||||
"server runs in default-deny mode (bearer tokens configured but no \
|
||||
|
|
|
|||
|
|
@ -5034,23 +5034,39 @@ graphs:
|
|||
assert_eq!(resp_authed.status(), StatusCode::FORBIDDEN);
|
||||
}
|
||||
|
||||
/// `GET /graphs` with a server policy that allows `graph_list` → 200.
|
||||
/// `GET /graphs` with a server policy that does NOT allow `graph_list` → 403.
|
||||
/// `GET /graphs` with a server policy that allows `graph_list` → 200
|
||||
/// and returns the registry sorted alphabetically by `graph_id`.
|
||||
/// `GET /graphs` with a server policy that does NOT allow
|
||||
/// `graph_list` (viewer group) → 403.
|
||||
///
|
||||
/// This test owns the alphabetical-sort coverage that previously
|
||||
/// lived in `get_graphs_lists_registered_graphs_in_multi_mode`.
|
||||
/// That test now asserts denial in Open mode (server-scoped actions
|
||||
/// require explicit policy in every runtime state), so the positive
|
||||
/// body-shape assertions need a home where the response is
|
||||
/// operator-authorized — here.
|
||||
#[tokio::test(flavor = "multi_thread")]
|
||||
async fn get_graphs_with_server_policy_authorizes_per_cedar() {
|
||||
use omnigraph_policy::PolicyEngine;
|
||||
use omnigraph_server::{GraphHandle, GraphId, GraphKey};
|
||||
|
||||
let dir = tempfile::tempdir().unwrap();
|
||||
let graph_uri = dir.path().join("alpha").to_str().unwrap().to_string();
|
||||
|
||||
// Two graphs deliberately registered in non-alphabetical order
|
||||
// so the test would fail if the handler relied on insertion
|
||||
// order instead of server-side sorting.
|
||||
let schema = fs::read_to_string(fixture("test.pg")).unwrap();
|
||||
let engine = Omnigraph::init(&graph_uri, &schema).await.unwrap();
|
||||
let handle = Arc::new(GraphHandle {
|
||||
key: GraphKey::cluster(GraphId::try_from("alpha").unwrap()),
|
||||
uri: graph_uri,
|
||||
engine: Arc::new(engine),
|
||||
policy: None,
|
||||
});
|
||||
let mut handles = Vec::new();
|
||||
for id in ["beta", "alpha"] {
|
||||
let graph_uri = dir.path().join(id).to_str().unwrap().to_string();
|
||||
let engine = Omnigraph::init(&graph_uri, &schema).await.unwrap();
|
||||
handles.push(Arc::new(GraphHandle {
|
||||
key: GraphKey::cluster(GraphId::try_from(id).unwrap()),
|
||||
uri: graph_uri,
|
||||
engine: Arc::new(engine),
|
||||
policy: None,
|
||||
}));
|
||||
}
|
||||
|
||||
// Server policy: admins can graph_list, viewers cannot.
|
||||
let policy_path = dir.path().join("server-policy.yaml");
|
||||
|
|
@ -5076,17 +5092,11 @@ rules:
|
|||
("act-bruno".to_string(), "bruno-token".to_string()),
|
||||
];
|
||||
let workload = omnigraph_server::workload::WorkloadController::from_env();
|
||||
let state = AppState::new_multi(
|
||||
vec![handle],
|
||||
tokens,
|
||||
Some(server_policy),
|
||||
workload,
|
||||
None,
|
||||
)
|
||||
.unwrap();
|
||||
let state = AppState::new_multi(handles, tokens, Some(server_policy), workload, None)
|
||||
.unwrap();
|
||||
let app = build_app(state);
|
||||
|
||||
// Admin → 200
|
||||
// Admin → 200, body returns both graphs alphabetically sorted.
|
||||
let resp_admin = app
|
||||
.clone()
|
||||
.oneshot(
|
||||
|
|
@ -5104,6 +5114,16 @@ rules:
|
|||
StatusCode::OK,
|
||||
"admin must be allowed graph_list"
|
||||
);
|
||||
let body = to_bytes(resp_admin.into_body(), usize::MAX).await.unwrap();
|
||||
let json: Value = serde_json::from_slice(&body).unwrap();
|
||||
let graphs = json["graphs"].as_array().unwrap();
|
||||
assert_eq!(graphs.len(), 2, "response must list both registered graphs");
|
||||
assert_eq!(
|
||||
graphs[0]["graph_id"].as_str().unwrap(),
|
||||
"alpha",
|
||||
"server must sort graphs alphabetically by graph_id (insertion order was 'beta', 'alpha')"
|
||||
);
|
||||
assert_eq!(graphs[1]["graph_id"].as_str().unwrap(), "beta");
|
||||
|
||||
// Viewer → 403
|
||||
let resp_viewer = app
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue