diff --git a/crates/detectord/crates/mcp_backend/src/lib.rs b/crates/detectord/crates/mcp_backend/src/lib.rs index efb7d6b6..5fc6c07b 100644 --- a/crates/detectord/crates/mcp_backend/src/lib.rs +++ b/crates/detectord/crates/mcp_backend/src/lib.rs @@ -48,9 +48,13 @@ pub enum Error { pub type Result = std::result::Result; /// The org policy flag governing quarantine. -#[derive(Debug, Clone, Copy, PartialEq, Eq)] +#[derive(Debug, Clone, PartialEq, Eq)] pub struct Policy { pub quarantine: bool, + /// The org the backend attributes this policy to. Lets a caller verify the + /// answer came from the tenant it is enrolled in before acting on it. + /// `None` when the backend is too old to report one. + pub org_id: Option, } /// Lifecycle status of a known server in the caller's org. @@ -343,6 +347,8 @@ pub fn user_part_hash(composite_key: &str) -> String { struct DomainConfigDto { #[serde(default)] auto_quarantine_other_mcp_servers: bool, + #[serde(default)] + org_id: Option, } /// Parse the domain-config body into a [`Policy`]. @@ -350,6 +356,7 @@ pub fn parse_policy(body: &str) -> std::result::Result { let dto: DomainConfigDto = serde_json::from_str(body).map_err(|e| e.to_string())?; Ok(Policy { quarantine: dto.auto_quarantine_other_mcp_servers, + org_id: dto.org_id.filter(|s| !s.is_empty()), }) } @@ -471,6 +478,21 @@ mod tests { assert!(!parse_policy(r#"{}"#).unwrap().quarantine); } + #[test] + fn policy_carries_org_id_when_present() { + assert_eq!( + parse_policy(r#"{"org_id":"org-a","auto_quarantine_other_mcp_servers":true}"#) + .unwrap() + .org_id + .as_deref(), + Some("org-a") + ); + // Absent or blank both mean "backend did not tell us", so callers can + // treat the tenant as unverifiable with one check. + assert!(parse_policy(r#"{}"#).unwrap().org_id.is_none()); + assert!(parse_policy(r#"{"org_id":""}"#).unwrap().org_id.is_none()); + } + #[test] fn policy_rejects_garbage() { assert!(parse_policy("not json").is_err()); diff --git a/crates/detectord/crates/mcp_detector_daemon/src/ops.rs b/crates/detectord/crates/mcp_detector_daemon/src/ops.rs index 235b0a9b..db713d60 100644 --- a/crates/detectord/crates/mcp_detector_daemon/src/ops.rs +++ b/crates/detectord/crates/mcp_detector_daemon/src/ops.rs @@ -382,12 +382,21 @@ pub fn status(user: &str) -> anyhow::Result { } /// Re-fetch the policy (fail-closed) and return the updated status. +/// +/// A failed fetch keeps the cached policy, but it is LOGGED rather than +/// swallowed: the returned `Status` is indistinguishable from a successful +/// refresh, so a silently-dropped error let a dead API key look like a +/// confirmed-fresh policy to whoever asked for the refresh. pub async fn refresh_policy(user: &str) -> anyhow::Result { if let Some(mut e) = Enrollment::load_for(user)? { let client = BackendClient::new(e.api_base_url.clone(), e.api_key.clone()); - if let Ok(p) = client.fetch_policy().await { - e.quarantine = p.quarantine; - e.save_for(user)?; + match client.fetch_policy().await { + // Same tenant check the reconcile loop applies; `apply_policy` + // persists on its own. + Ok(p) => crate::runner::apply_policy(&p, &mut e, user), + Err(err) => { + tracing::warn!(error = %err, "policy refresh failed; keeping last-known-good") + } } } status(user) diff --git a/crates/detectord/crates/mcp_detector_daemon/src/runner.rs b/crates/detectord/crates/mcp_detector_daemon/src/runner.rs index dce364ec..1c8234db 100644 --- a/crates/detectord/crates/mcp_detector_daemon/src/runner.rs +++ b/crates/detectord/crates/mcp_detector_daemon/src/runner.rs @@ -71,7 +71,8 @@ pub async fn worker(user: String, enforce: bool, events: Option) -> any paths::ensure_user_dir(&user)?; let mut enrollment = Enrollment::load_for(&user)? .ok_or_else(|| anyhow::anyhow!("not enrolled; run `enroll` first"))?; - let client = BackendClient::new(enrollment.api_base_url.clone(), enrollment.api_key.clone()); + let mut client = + BackendClient::new(enrollment.api_base_url.clone(), enrollment.api_key.clone()); let mut seen = SeenStore::open(paths::seen_store_path(&user), enrollment.org_id.clone())?; refresh(&client, &mut enrollment, &mut seen, &user).await; @@ -106,6 +107,7 @@ pub async fn worker(user: String, enforce: bool, events: Option) -> any ); let mut last_refresh = Instant::now(); + let mut force_refresh = false; let mut reported = HashSet::new(); loop { // Re-read the enrollment from disk each pass so an onboarding-completion @@ -123,6 +125,20 @@ pub async fn worker(user: String, enforce: bool, events: Option) -> any if fresh.selected_agents != enrollment.selected_agents { tracing::info!(agents = ?fresh.selected_agents, "selected agents changed"); } + // `client` is bound to the URL + key it was built from, so a + // re-enroll that repoints the daemon (a different deploy + // environment, or a rotated key) has to rebuild it. Without this + // the worker keeps polling the OLD backend for the whole life of + // the process: policy and fingerprints come back for the wrong + // tenant, and `refresh` persists that answer over the correct one. + if fresh.api_base_url != enrollment.api_base_url || fresh.api_key != enrollment.api_key + { + tracing::info!(url = %fresh.api_base_url, "backend credentials changed; rebuilding client"); + client = BackendClient::new(fresh.api_base_url.clone(), fresh.api_key.clone()); + // Don't sit on the wrong tenant's cached policy for up to + // REFRESH_INTERVAL; re-fetch on this pass. + force_refresh = true; + } enrollment = fresh; } reconcile_once( @@ -145,9 +161,10 @@ pub async fn worker(user: String, enforce: bool, events: Option) -> any tracing::info!(count = healed, "self-healed sealgate install"); } } - if last_refresh.elapsed() >= REFRESH_INTERVAL { + if force_refresh || last_refresh.elapsed() >= REFRESH_INTERVAL { refresh(&client, &mut enrollment, &mut seen, &user).await; last_refresh = Instant::now(); + force_refresh = false; } // Cheap (one open() of a file that is either readable or not), and this // tick is at most every RESCAN_INTERVAL, so no need to pace it further. @@ -323,6 +340,42 @@ fn chown_new_files(record: &QuarantineRecord, user: &str) { } } +/// Whether a fetched policy may be trusted for the org we are enrolled in. +/// +/// Split out from [`apply_policy`] so the tenant rule is testable without +/// touching the on-disk enrollment. +fn policy_matches_tenant(p: &mcp_backend::Policy, enrolled_org: &str) -> bool { + match p.org_id.as_deref() { + Some(org) => org == enrolled_org, + // Unverifiable, not wrong: an older backend reports no org at all. + None => true, + } +} + +/// Cache a freshly-fetched policy onto `enrollment` and persist it. +/// +/// Defence in depth behind the credential rebuild in [`worker`]: a policy is +/// only trusted when the backend attributes it to the org we are enrolled in, +/// so a client left pointing at another tenant can never write that tenant's +/// answer over ours. An older backend that reports no `org_id` is unverifiable +/// rather than wrong, so it is applied as before instead of bricking +/// quarantine for everyone on it. +pub fn apply_policy(p: &mcp_backend::Policy, enrollment: &mut Enrollment, user: &str) { + if !policy_matches_tenant(p, &enrollment.org_id) { + tracing::warn!( + policy_org = ?p.org_id, + enrolled_org = %enrollment.org_id, + "policy org mismatch; keeping last-known-good (re-enroll to repoint this daemon)" + ); + return; + } + if p.quarantine != enrollment.quarantine { + tracing::info!(quarantine = p.quarantine, "policy updated"); + } + enrollment.quarantine = p.quarantine; + let _ = enrollment.save_for(user); +} + /// Refresh policy + known fingerprints into the enrollment/seen-store. /// Fail-closed: on any error the cached values are kept, never downgraded. pub async fn refresh( @@ -332,13 +385,7 @@ pub async fn refresh( user: &str, ) { match client.fetch_policy().await { - Ok(p) => { - if p.quarantine != enrollment.quarantine { - tracing::info!(quarantine = p.quarantine, "policy updated"); - } - enrollment.quarantine = p.quarantine; - let _ = enrollment.save_for(user); - } + Ok(p) => apply_policy(&p, enrollment, user), Err(e) => tracing::warn!(error = %e, "policy refresh failed; keeping last-known-good"), } @@ -574,3 +621,35 @@ fn log_skips(skips: &[Skip]) { tracing::debug!(server = %s.name, agent = s.agent, reason = s.reason, fingerprint = %s.fingerprint, "will not quarantine"); } } + +#[cfg(test)] +mod tests { + use super::*; + + fn policy(org: Option<&str>) -> mcp_backend::Policy { + mcp_backend::Policy { + quarantine: true, + org_id: org.map(str::to_string), + } + } + + #[test] + fn tenant_check_accepts_the_enrolled_org() { + assert!(policy_matches_tenant(&policy(Some("org-a")), "org-a")); + } + + #[test] + fn tenant_check_rejects_another_org() { + // The demo-backend-after-release-re-enroll case: a perfectly valid 200 + // for the wrong tenant must not overwrite our cached policy. + assert!(!policy_matches_tenant( + &policy(Some("org-demo")), + "org-release" + )); + } + + #[test] + fn tenant_check_allows_a_backend_that_reports_no_org() { + assert!(policy_matches_tenant(&policy(None), "org-a")); + } +}