Skip to content

Commit 8d2d2da

Browse files
authored
fix(security): name no nodes in authorization denials (#1322)
* fix: name no nodes in refused listings A refused location listing named the nodes the person could not view and read differently for a location that does not exist. Listing refusals now carry one fixed message with the decision ID, identical for unassigned and unknown locations. The audit event keeps the missing nodes. * fix: name no nodes in authorization denials Every refusal from person authorization now shows fixed text per action and its decision ID; the audit event keeps the missing nodes. Unknown jobs and triggers are refused only after the caller is authorized for the target, so an unknown object reads the same as a forbidden one.
1 parent b858c24 commit 8d2d2da

17 files changed

Lines changed: 502 additions & 61 deletions

File tree

‎clients/typescript/tests/client.test.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -134,7 +134,7 @@ test("generated client preserves native scopes, replay, path encoding, SSE and b
134134
const decisionId: string | null | undefined = denied.error?.decision_id;
135135
assert.ok(decisionId);
136136
assert.deepEqual(denied.error, {
137-
error: `missing permission in this scope; all requested nodes denied; decision ${decisionId}`,
137+
error: `missing view permission for this target; decision ${decisionId}`,
138138
decision_id: decisionId,
139139
});
140140
const machine = await browser.GET(healthPath, { headers, params });

‎crates/sqlbuild-orchestration/src/authorization/_helpers/custom_roles.rs‎

Lines changed: 17 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -120,10 +120,18 @@ pub(crate) fn custom_coverage(
120120
(granting, missing)
121121
}
122122

123-
/// A denial names at most twenty uncovered nodes so the audited reason stays bounded.
124-
pub(crate) fn denial_reason(action: Action, missing: &[String]) -> Result<String, ControlError> {
123+
/// The audited reason naming up to twenty missing nodes, and node-free text for the caller.
124+
pub(crate) fn denial_reason(
125+
action: Action,
126+
missing: &[String],
127+
) -> Result<(String, String), ControlError> {
128+
let action = action_name(action)?;
129+
let shown = format!("missing {action} permission for this target");
125130
if missing.is_empty() {
126-
return Ok("missing permission in this scope; all requested nodes denied".into());
131+
return Ok((
132+
"missing permission in this scope; all requested nodes denied".into(),
133+
shown,
134+
));
127135
}
128136
let listed: Vec<&str> = missing.iter().take(20).map(String::as_str).collect();
129137
let more = missing.len() - listed.len();
@@ -132,10 +140,12 @@ pub(crate) fn denial_reason(action: Action, missing: &[String]) -> Result<String
132140
} else {
133141
String::new()
134142
};
135-
Ok(format!(
136-
"missing {} on nodes {}{suffix}; all requested nodes denied",
137-
action_name(action)?,
138-
listed.join(", ")
143+
Ok((
144+
format!(
145+
"missing {action} on nodes {}{suffix}; all requested nodes denied",
146+
listed.join(", ")
147+
),
148+
shown,
139149
))
140150
}
141151

‎crates/sqlbuild-orchestration/src/authorization/main/authorize_person.rs‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,14 @@ pub(crate) async fn authorize_person(
9696
context.granting_role = "default:viewer".into();
9797
return Ok((transaction, context));
9898
}
99-
let reason = denial_reason(scope.action, &missing)?;
100-
deny(transaction, &context, json!(scope), &reason).await
99+
let (audited, shown) = denial_reason(scope.action, &missing)?;
100+
deny(transaction, &context, json!(scope), &audited)
101+
.await
102+
.map_err(|error| match error {
103+
ControlError::Denied { decision_id, .. } => ControlError::Denied {
104+
reason: shown,
105+
decision_id,
106+
},
107+
error => error,
108+
})
101109
}

‎crates/sqlbuild-orchestration/src/authorization/main/mod.rs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,3 +11,4 @@ pub(crate) mod publish_custom_roles;
1111
pub(crate) mod publish_group_roles;
1212
pub(crate) mod read_audit_events;
1313
pub mod read_security_audit;
14+
pub(crate) mod refuse_unknown;
Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
//! An unknown object is refused like a forbidden one unless the person may act on the whole scope it was looked up in.
2+
3+
use std::collections::BTreeSet;
4+
5+
use uuid::Uuid;
6+
7+
use crate::authorization::main::authorize_person::authorize_person;
8+
use crate::credentials::models::{Action, AuditContext, DefaultRole, PermissionScope};
9+
use crate::store::errors::ControlError;
10+
use crate::store::main::query::query;
11+
use crate::store::models::{ControlTransaction, Parameter};
12+
13+
/// `location` is where the lookup happened, or `None` when the object alone named it.
14+
pub(crate) async fn refuse_unknown<T>(
15+
transaction: ControlTransaction,
16+
credentials: (&AuditContext, Uuid),
17+
(action, location): (Action, Option<(Uuid, &str)>),
18+
missing: ControlError,
19+
) -> Result<T, ControlError> {
20+
let (transaction, nodes) = match location {
21+
Some((location_id, target)) => {
22+
let (transaction, rows) = query(transaction,
23+
"SELECT n.node_key FROM sqb_snapshot_nodes n JOIN sqb_current_snapshots c ON c.snapshot_id=n.snapshot_id \
24+
WHERE c.location_id=$1 AND c.target=$2",
25+
vec![Parameter::Id(location_id), Parameter::Text(target.into())]).await?;
26+
let nodes: BTreeSet<String> = rows
27+
.iter()
28+
.filter_map(|row| row["node_key"].as_str().map(str::to_owned))
29+
.collect();
30+
(transaction, nodes)
31+
}
32+
None => (transaction, BTreeSet::new()),
33+
};
34+
let scope = PermissionScope {
35+
action,
36+
location_id: location.map(|(location_id, _)| location_id),
37+
target: location.map(|(_, target)| target.to_owned()),
38+
nodes,
39+
};
40+
authorize_person(transaction, credentials, &scope, DefaultRole::None).await?;
41+
Err(missing)
42+
}

‎crates/sqlbuild-orchestration/src/declarations/_helpers/trigger_scope.rs‎

Lines changed: 17 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
33
use uuid::Uuid;
44

5+
use crate::authorization::main::refuse_unknown::refuse_unknown;
56
use crate::credentials::models::{Action, AuditContext, PermissionScope};
67
use crate::declarations::models::{TriggerChange, TriggerChangeRequest};
78
use crate::runs::main::frozen_job_request::frozen_job_request;
@@ -14,15 +15,27 @@ use crate::store::models::{ControlTransaction, Parameter};
1415
/// A group's scope is its frozen selection's write set, which covers every child's write set.
1516
pub(crate) async fn trigger_scope(
1617
transaction: ControlTransaction,
17-
context: &AuditContext,
18+
(context, credentials): (&AuditContext, Uuid),
1819
request: &TriggerChangeRequest,
1920
) -> Result<(ControlTransaction, PermissionScope), ControlError> {
21+
let action = match request.change {
22+
TriggerChange::Enable | TriggerChange::Pause | TriggerChange::ResetDefault => {
23+
Action::TriggerToggle
24+
}
25+
TriggerChange::SetCursor { .. } | TriggerChange::ResetCursor => Action::SensorResetCursor,
26+
};
2027
let (transaction, rows) = query(transaction,
2128
"SELECT location_id,target,job_key FROM sqb_triggers WHERE trigger_id=$1 AND removed_in_snapshot_id IS NULL",
2229
vec![Parameter::Id(request.trigger_id)]).await?;
23-
let row = rows
24-
.first()
25-
.ok_or_else(|| ControlError::Invalid("current trigger not found".into()))?;
30+
let Some(row) = rows.first() else {
31+
return refuse_unknown(
32+
transaction,
33+
(context, credentials),
34+
(action, None),
35+
ControlError::Invalid("current trigger not found".into()),
36+
)
37+
.await;
38+
};
2639
let job = JobRunRequest {
2740
run_id: Uuid::nil(),
2841
location_id: serde_json::from_value(row["location_id"].clone())
@@ -36,12 +49,6 @@ pub(crate) async fn trigger_scope(
3649
let (transaction, frozen) = frozen_job_request(transaction, context, &job).await?;
3750
let (transaction, nodes) =
3851
job_write_set(transaction, frozen.snapshot_id, &frozen.selection).await?;
39-
let action = match request.change {
40-
TriggerChange::Enable | TriggerChange::Pause | TriggerChange::ResetDefault => {
41-
Action::TriggerToggle
42-
}
43-
TriggerChange::SetCursor { .. } | TriggerChange::ResetCursor => Action::SensorResetCursor,
44-
};
4552
Ok((
4653
transaction,
4754
PermissionScope {

‎crates/sqlbuild-orchestration/src/declarations/main/change_person_trigger.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ pub async fn change_person_trigger(
2020
) -> Result<bool, ControlError> {
2121
let (transaction, scope) = trigger_scope(
2222
begin(store, TransactionMode::Write).await?,
23-
context,
23+
(context, credentials),
2424
request,
2525
)
2626
.await?;

‎crates/sqlbuild-orchestration/src/runs/_helpers/requests/manual_job.rs‎

Lines changed: 29 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ use serde_json::json;
44
use uuid::Uuid;
55

66
use crate::authorization::main::authorize_person::authorize_person;
7+
use crate::authorization::main::refuse_unknown::refuse_unknown;
78
use crate::credentials::main::audit::append_audit::append_audit;
89
use crate::credentials::models::{Action, AuditContext, DefaultRole, PermissionScope};
910
use crate::runs::_helpers::requests::permissions::authorize_execution;
@@ -14,7 +15,8 @@ use crate::runs::constants::MANUAL_JOB_AUDIT_TAG;
1415
use crate::runs::main::frozen_job_request::frozen_job_request;
1516
use crate::runs::models::{JobRunRequest, RunWindow};
1617
use crate::store::errors::ControlError;
17-
use crate::store::models::ControlTransaction;
18+
use crate::store::main::query::query;
19+
use crate::store::models::{ControlTransaction, Parameter};
1820

1921
pub(crate) async fn request_job_run_in(
2022
mut transaction: ControlTransaction,
@@ -61,6 +63,10 @@ pub(crate) async fn request_job_run_in(
6163
if retry {
6264
return Ok((transaction, false));
6365
}
66+
let transaction = match credentials {
67+
Some(credentials) => known_job(transaction, (&context, credentials), request).await?,
68+
None => transaction,
69+
};
6470
let (mut transaction, frozen) = frozen_job_request(transaction, &context, request).await?;
6571
if let Some(credentials) = credentials {
6672
(transaction, context) = authorize_execution(
@@ -78,3 +84,25 @@ pub(crate) async fn request_job_run_in(
7884
json!(null), json!({"request":request,"frozen":frozen}))).await?;
7985
Ok((transaction, true))
8086
}
87+
88+
/// A person's unknown job is refused like a forbidden one unless they may run every node of the target.
89+
async fn known_job(
90+
transaction: ControlTransaction,
91+
credentials: (&AuditContext, Uuid),
92+
request: &JobRunRequest,
93+
) -> Result<ControlTransaction, ControlError> {
94+
let (transaction, jobs) = query(transaction,
95+
"SELECT j.job_key FROM sqb_jobs j JOIN sqb_current_snapshots c ON c.location_id=j.location_id \
96+
AND c.target=j.target AND c.snapshot_id=j.snapshot_id WHERE j.location_id=$1 AND j.target=$2 AND j.job_key=$3",
97+
vec![Parameter::Id(request.location_id), Parameter::Text(request.target.clone()), Parameter::Text(request.job_key.clone())]).await?;
98+
if jobs.is_empty() {
99+
return refuse_unknown(
100+
transaction,
101+
credentials,
102+
(Action::JobRun, Some((request.location_id, &request.target))),
103+
ControlError::Invalid("published job not found in this location and target".into()),
104+
)
105+
.await;
106+
}
107+
Ok(transaction)
108+
}
Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,72 @@
1+
//! Masked denial responses, person requests, and the audit detail behind them.
2+
3+
use reqwest::header;
4+
use serde_json::{Value, json};
5+
use uuid::Uuid;
6+
7+
use crate::store::main::{begin::begin, commit::commit, query::query};
8+
use crate::store::models::{ControlStore, TransactionMode};
9+
10+
/// Status, the headers that could tell cases apart, and the body with its decision ID masked.
11+
pub(crate) async fn masked(response: reqwest::Response) -> (Value, Uuid) {
12+
let status = response.status().as_u16();
13+
let headers: Vec<(String, String)> = response
14+
.headers()
15+
.iter()
16+
.filter(|(name, _)| *name != header::DATE)
17+
.map(|(name, value)| (name.to_string(), value.to_str().unwrap_or("").to_owned()))
18+
.collect();
19+
let text = response.text().await.expect("denial body");
20+
let body: Value = serde_json::from_str(&text).expect("JSON denial");
21+
let decision: Uuid = serde_json::from_value(body["decision_id"].clone()).expect("decision ID");
22+
(
23+
json!({"status": status, "headers": headers,
24+
"body": text.replace(&decision.to_string(), "<decision>")}),
25+
decision,
26+
)
27+
}
28+
29+
pub(crate) async fn audited_scope(store: &ControlStore, decision: Uuid) -> Value {
30+
let transaction = begin(store, TransactionMode::Read).await.expect("audit");
31+
let (transaction, rows) = query(
32+
transaction,
33+
"SELECT scope_json, after_json FROM sqb_security_audit WHERE action='authorization.denied'",
34+
vec![],
35+
)
36+
.await
37+
.expect("denials");
38+
commit(transaction).await.expect("close audit");
39+
rows.into_iter()
40+
.find(|row| row["after_json"]["decision_id"] == json!(decision))
41+
.map(|row| json!([row["scope_json"]["nodes"], row["after_json"]["reason"]]))
42+
.unwrap_or(Value::Null)
43+
}
44+
45+
/// A person's browser request with session cookie, origin and CSRF proof.
46+
pub(crate) async fn person_send(
47+
client: &reqwest::Client,
48+
(url, session, csrf): (&str, &str, &str),
49+
body: Option<Value>,
50+
) -> reqwest::Response {
51+
let request = body.map_or_else(|| client.get(url), |body| client.post(url).json(&body));
52+
request
53+
.header(header::HOST, "control.example.com")
54+
.header(header::ORIGIN, "https://control.example.com")
55+
.header(header::COOKIE, format!("__Host-sqb-session={session}"))
56+
.header("x-sqb-csrf", csrf)
57+
.send()
58+
.await
59+
.expect("person request")
60+
}
61+
62+
/// A job request body for a location and job key.
63+
pub(crate) fn job(location_id: Uuid, job_key: &str) -> Value {
64+
json!({"run_id": Uuid::now_v7(), "location_id": location_id, "target": "development",
65+
"job_key": job_key, "reason": "Refresh orders", "priority": 0})
66+
}
67+
68+
/// A trigger change body of the given kind.
69+
pub(crate) fn trigger(trigger_id: Uuid, change: &str) -> Value {
70+
json!({"event_id": Uuid::now_v7(), "trigger_id": trigger_id, "actor": "",
71+
"reason": "Hold orders", "change": {"kind": change}})
72+
}

‎crates/sqlbuild-orchestration/tests/api/test_contracts.rs‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,8 @@
22
33
#[path = "changes/helpers.rs"]
44
mod changes_helpers;
5+
#[path = "denials/helpers.rs"]
6+
mod denials_helpers;
57
#[path = "fingerprint_reads/helpers.rs"]
68
mod fingerprint_reads_helpers;
79
#[path = "helpers.rs"]
@@ -40,6 +42,8 @@ mod test_cli_login;
4042
mod test_contract;
4143
#[path = "test_custom_roles.rs"]
4244
mod test_custom_roles;
45+
#[path = "test_denials.rs"]
46+
mod test_denials;
4347
#[path = "test_development.rs"]
4448
mod test_development;
4549
#[path = "test_external_sessions.rs"]

0 commit comments

Comments
 (0)