From f88b58192bde559907e35054f31dc4c8366741ed Mon Sep 17 00:00:00 2001 From: krishicks Date: Thu, 29 Oct 2026 00:37:37 +0000 Subject: [PATCH] fix(ocsf): address follow-ups from the #4288 review (#5020) * fix(ocsf): keep the fallback event context feature-independent With test-support, the fallback EventContext carried a fake sandbox identity, so a binary built with that feature unified in, such as by cargo build --workspace --all-targets, attributed unbound events to a sandbox that does not exist. Producer tests no longer need it: the validator accepts top-level container on its own, and the builder conformance test uses an explicit sandbox context. Signed-off-by: Kris Hicks * test(ocsf): pin builder compile_fail doctests to the missing field The HTTP, Network, Process, and SSH type-state doctests built their EventContext from a struct literal, so a new EventContext field would also make them fail to compile and they would keep passing with the guard removed. Build the context with ctx::ctx() so the missing required field is the only compile error. Signed-off-by: Kris Hicks * refactor(ocsf): share setter macros with type-state builders The HTTP, Network, Process, and SSH builders carry a type-state parameter, so they hand-copied the activity, action, disposition, severity, status, and message setters that the shared macros generate for other builders. Let the macros take an optional state parameter and use them on all four, and route both SSH endpoint setters through one state transition. Signed-off-by: Kris Hicks * docs(ocsf): add type_uid to the Network Activity example OCSF 1.8.0 requires type_uid, so the example that follows the conformance claim failed validation as written. Signed-off-by: Kris Hicks * test(e2e): report the last exec error in the OCSF conformance test The test discarded the result of the exec that generates traffic, so when the exec failed, for example without python3 in the image, the test retried for a minute and then blamed the JSONL copy. Keep the latest exec error and include it in the missing-classes failure. Signed-off-by: Kris Hicks * test(e2e): read the sandbox id label from podman ps JSON Signed-off-by: Kris Hicks --------- Signed-off-by: Kris Hicks --- crates/openshell-ocsf/src/builders/http.rs | 54 ++--------- crates/openshell-ocsf/src/builders/mod.rs | 21 +++-- crates/openshell-ocsf/src/builders/network.rs | 54 ++--------- crates/openshell-ocsf/src/builders/process.rs | 49 ++-------- crates/openshell-ocsf/src/builders/ssh.rs | 91 ++++--------------- crates/openshell-ocsf/src/ctx.rs | 24 ++--- docs/observability/ocsf-json-export.mdx | 2 + e2e/rust/tests/ocsf_jsonl_conformance.rs | 29 +++--- 8 files changed, 75 insertions(+), 249 deletions(-) diff --git a/crates/openshell-ocsf/src/builders/http.rs b/crates/openshell-ocsf/src/builders/http.rs index d5ae30aac..e2f85bdab 100644 --- a/crates/openshell-ocsf/src/builders/http.rs +++ b/crates/openshell-ocsf/src/builders/http.rs @@ -42,19 +42,9 @@ impl<'a> HttpActivityBuilder<'a, MissingHttpContext> { /// be built. /// /// ```compile_fail - /// use openshell_ocsf::{EventContext, EventOrigin, HttpActivityBuilder}; + /// use openshell_ocsf::HttpActivityBuilder; /// - /// let ctx = EventContext { - /// sandbox_id: String::new(), - /// sandbox_name: String::new(), - /// container_image: String::new(), - /// hostname: String::new(), - /// product_version: String::new(), - /// proxy_ip: "127.0.0.1".parse().unwrap(), - /// proxy_port: 3128, - /// origin: EventOrigin::Supervisor, - /// }; - /// HttpActivityBuilder::new(&ctx).build(); + /// HttpActivityBuilder::new(openshell_ocsf::ctx::ctx()).build(); /// ``` #[must_use] pub fn new(ctx: impl Into>) -> Self { @@ -164,24 +154,6 @@ impl<'a, Context> HttpActivityBuilder<'a, Context> { self } - #[must_use] - pub fn activity(mut self, id: ActivityId) -> Self { - self.activity = id; - self - } - - #[must_use] - pub fn action(mut self, id: ActionId) -> Self { - self.action = Some(id); - self - } - - #[must_use] - pub fn disposition(mut self, id: DispositionId) -> Self { - self.disposition = Some(id); - self - } - #[must_use] pub fn actor_process(mut self, process: crate::objects::Process) -> Self { self.actor = Some(Actor::process(process)); @@ -199,26 +171,12 @@ impl<'a, Context> HttpActivityBuilder<'a, Context> { self.firewall_rule = Some(FirewallRule::new(name, rule_type)); self } - - #[must_use] - pub fn severity(mut self, id: SeverityId) -> Self { - self.severity = id; - self - } - - #[must_use] - pub fn status(mut self, id: StatusId) -> Self { - self.status = Some(id); - self - } - - #[must_use] - pub fn message(mut self, msg: impl Into) -> Self { - self.message = Some(msg.into()); - self - } } +impl_activity_setter!(HttpActivityBuilder); +impl_action_disposition_setters!(HttpActivityBuilder); +impl_builder_setters!(HttpActivityBuilder); + impl HttpActivityBuilder<'_, HasHttpContext> { #[must_use] pub fn build(self) -> OcsfEvent { diff --git a/crates/openshell-ocsf/src/builders/mod.rs b/crates/openshell-ocsf/src/builders/mod.rs index 76acaba84..09dcd34e4 100644 --- a/crates/openshell-ocsf/src/builders/mod.rs +++ b/crates/openshell-ocsf/src/builders/mod.rs @@ -13,10 +13,11 @@ /// ```ignore /// impl_builder_setters!(MyBuilder); // severity + status + message /// impl_builder_setters!(MyBuilder, no_status); // severity + message only +/// impl_builder_setters!(MyBuilder); // for a type-state builder /// ``` macro_rules! impl_builder_setters { - ($builder:ident) => { - impl<'a> $builder<'a> { + ($builder:ident $(<$state:ident>)?) => { + impl<'a $(, $state)?> $builder<'a $(, $state)?> { /// Set the event severity. #[must_use] pub fn severity(mut self, id: $crate::enums::SeverityId) -> Self { @@ -37,8 +38,8 @@ macro_rules! impl_builder_setters { } } }; - ($builder:ident, no_status) => { - impl<'a> $builder<'a> { + ($builder:ident $(<$state:ident>)?, no_status) => { + impl<'a $(, $state)?> $builder<'a $(, $state)?> { /// Set the event severity. #[must_use] pub fn severity(mut self, id: $crate::enums::SeverityId) -> Self { @@ -58,8 +59,8 @@ macro_rules! impl_builder_setters { /// Generate the `activity` setter used by every event builder that carries an /// `activity` field. macro_rules! impl_activity_setter { - ($builder:ident) => { - impl<'a> $builder<'a> { + ($builder:ident $(<$state:ident>)?) => { + impl<'a $(, $state)?> $builder<'a $(, $state)?> { /// Set the event activity identifier. #[must_use] pub fn activity(mut self, id: $crate::enums::ActivityId) -> Self { @@ -70,11 +71,11 @@ macro_rules! impl_activity_setter { }; } -/// Generate `action` and `disposition` setters shared by network, HTTP, SSH, -/// process, and detection-finding builders. +/// Generate `action` and `disposition` setters shared by every builder whose +/// event records an action and its disposition. macro_rules! impl_action_disposition_setters { - ($builder:ident) => { - impl<'a> $builder<'a> { + ($builder:ident $(<$state:ident>)?) => { + impl<'a $(, $state)?> $builder<'a $(, $state)?> { /// Set the action taken. #[must_use] pub fn action(mut self, id: $crate::enums::ActionId) -> Self { diff --git a/crates/openshell-ocsf/src/builders/network.rs b/crates/openshell-ocsf/src/builders/network.rs index d164384fd..1e182af67 100644 --- a/crates/openshell-ocsf/src/builders/network.rs +++ b/crates/openshell-ocsf/src/builders/network.rs @@ -44,19 +44,9 @@ impl<'a> NetworkActivityBuilder<'a, MissingNetworkEndpoint> { /// it can be built. /// /// ```compile_fail - /// use openshell_ocsf::{EventContext, EventOrigin, NetworkActivityBuilder}; + /// use openshell_ocsf::NetworkActivityBuilder; /// - /// let ctx = EventContext { - /// sandbox_id: String::new(), - /// sandbox_name: String::new(), - /// container_image: String::new(), - /// hostname: String::new(), - /// product_version: String::new(), - /// proxy_ip: "127.0.0.1".parse().unwrap(), - /// proxy_port: 3128, - /// origin: EventOrigin::Supervisor, - /// }; - /// NetworkActivityBuilder::new(&ctx).build(); + /// NetworkActivityBuilder::new(openshell_ocsf::ctx::ctx()).build(); /// ``` #[must_use] pub fn new(ctx: impl Into>) -> Self { @@ -202,24 +192,6 @@ impl<'a, EndpointState> NetworkActivityBuilder<'a, EndpointState> { self } - #[must_use] - pub fn activity(mut self, id: ActivityId) -> Self { - self.activity = id; - self - } - - #[must_use] - pub fn action(mut self, id: ActionId) -> Self { - self.action = Some(id); - self - } - - #[must_use] - pub fn disposition(mut self, id: DispositionId) -> Self { - self.disposition = Some(id); - self - } - #[must_use] pub fn actor_process(mut self, process: crate::objects::Process) -> Self { self.actor = Some(Actor::process(process)); @@ -231,26 +203,12 @@ impl<'a, EndpointState> NetworkActivityBuilder<'a, EndpointState> { self.firewall_rule = Some(FirewallRule::new(name, rule_type)); self } - - #[must_use] - pub fn severity(mut self, id: SeverityId) -> Self { - self.severity = id; - self - } - - #[must_use] - pub fn status(mut self, id: StatusId) -> Self { - self.status = Some(id); - self - } - - #[must_use] - pub fn message(mut self, msg: impl Into) -> Self { - self.message = Some(msg.into()); - self - } } +impl_activity_setter!(NetworkActivityBuilder); +impl_action_disposition_setters!(NetworkActivityBuilder); +impl_builder_setters!(NetworkActivityBuilder); + impl NetworkActivityBuilder<'_, HasNetworkEndpoint> { /// Finalize and return the `OcsfEvent`. #[must_use] diff --git a/crates/openshell-ocsf/src/builders/process.rs b/crates/openshell-ocsf/src/builders/process.rs index d98a4fb2a..ae6a6dfe1 100644 --- a/crates/openshell-ocsf/src/builders/process.rs +++ b/crates/openshell-ocsf/src/builders/process.rs @@ -37,19 +37,9 @@ impl<'a> ProcessActivityBuilder<'a, MissingActor> { /// A Process Activity must name its acting process before it can be built. /// /// ```compile_fail - /// use openshell_ocsf::{EventContext, EventOrigin, ProcessActivityBuilder}; + /// use openshell_ocsf::ProcessActivityBuilder; /// - /// let ctx = EventContext { - /// sandbox_id: String::new(), - /// sandbox_name: String::new(), - /// container_image: String::new(), - /// hostname: String::new(), - /// product_version: String::new(), - /// proxy_ip: "127.0.0.1".parse().unwrap(), - /// proxy_port: 3128, - /// origin: EventOrigin::Supervisor, - /// }; - /// ProcessActivityBuilder::new(&ctx).build(); + /// ProcessActivityBuilder::new(openshell_ocsf::ctx::ctx()).build(); /// ``` #[must_use] pub fn new(ctx: impl Into>) -> Self { @@ -90,43 +80,12 @@ impl<'a> ProcessActivityBuilder<'a, MissingActor> { } impl ProcessActivityBuilder<'_, ActorState> { - /// Set the event activity identifier. - #[must_use] - pub fn activity(mut self, id: ActivityId) -> Self { - self.activity = id; - self - } /// Set the action taken. - #[must_use] - pub fn action(mut self, id: ActionId) -> Self { - self.action = Some(id); - self - } /// Set the disposition of the action. - #[must_use] - pub fn disposition(mut self, id: DispositionId) -> Self { - self.disposition = Some(id); - self - } /// Set the event severity. - #[must_use] - pub fn severity(mut self, id: SeverityId) -> Self { - self.severity = id; - self - } /// Set the overall event status. - #[must_use] - pub fn status(mut self, id: StatusId) -> Self { - self.status = Some(id); - self - } /// Set a human-readable event message. #[must_use] - pub fn message(mut self, msg: impl Into) -> Self { - self.message = Some(msg.into()); - self - } - #[must_use] pub fn process(mut self, proc: Process) -> Self { self.process = Some(proc); self @@ -143,6 +102,10 @@ impl ProcessActivityBuilder<'_, ActorState> { } } +impl_activity_setter!(ProcessActivityBuilder); +impl_action_disposition_setters!(ProcessActivityBuilder); +impl_builder_setters!(ProcessActivityBuilder); + impl ProcessActivityBuilder<'_, HasActor> { #[must_use] pub fn build(self) -> OcsfEvent { diff --git a/crates/openshell-ocsf/src/builders/ssh.rs b/crates/openshell-ocsf/src/builders/ssh.rs index bd1cea82c..21fdfa015 100644 --- a/crates/openshell-ocsf/src/builders/ssh.rs +++ b/crates/openshell-ocsf/src/builders/ssh.rs @@ -40,19 +40,9 @@ impl<'a> SshActivityBuilder<'a, MissingSshEndpoint> { /// can be built. /// /// ```compile_fail - /// use openshell_ocsf::{EventContext, EventOrigin, SshActivityBuilder}; + /// use openshell_ocsf::SshActivityBuilder; /// - /// let ctx = EventContext { - /// sandbox_id: String::new(), - /// sandbox_name: String::new(), - /// container_image: String::new(), - /// hostname: String::new(), - /// product_version: String::new(), - /// proxy_ip: "127.0.0.1".parse().unwrap(), - /// proxy_port: 3128, - /// origin: EventOrigin::Supervisor, - /// }; - /// SshActivityBuilder::new(&ctx).build(); + /// SshActivityBuilder::new(openshell_ocsf::ctx::ctx()).build(); /// ``` #[must_use] pub fn new(ctx: impl Into>) -> Self { @@ -79,40 +69,31 @@ impl<'a, EndpointState> SshActivityBuilder<'a, EndpointState> { /// Set the source endpoint from an address. #[must_use] pub fn src_endpoint_addr( - self, + mut self, ip: std::net::IpAddr, port: u16, ) -> SshActivityBuilder<'a, HasSshEndpoint> { - SshActivityBuilder { - src_endpoint: Some(Endpoint::from_ip(ip, port)), - dst_endpoint: self.dst_endpoint, - ctx: self.ctx, - activity: self.activity, - action: self.action, - disposition: self.disposition, - severity: self.severity, - status: self.status, - actor: self.actor, - auth_type_id: self.auth_type_id, - auth_type_label: self.auth_type_label, - protocol_ver: self.protocol_ver, - message: self.message, - endpoint_state: std::marker::PhantomData, - } + self.src_endpoint = Some(Endpoint::from_ip(ip, port)); + self.with_endpoint() } /// Set the destination endpoint. #[must_use] - pub fn dst_endpoint(self, endpoint: Endpoint) -> SshActivityBuilder<'a, HasSshEndpoint> { + pub fn dst_endpoint(mut self, endpoint: Endpoint) -> SshActivityBuilder<'a, HasSshEndpoint> { + self.dst_endpoint = Some(endpoint); + self.with_endpoint() + } + + fn with_endpoint(self) -> SshActivityBuilder<'a, HasSshEndpoint> { SshActivityBuilder { - src_endpoint: self.src_endpoint, - dst_endpoint: Some(endpoint), ctx: self.ctx, activity: self.activity, action: self.action, disposition: self.disposition, severity: self.severity, status: self.status, + src_endpoint: self.src_endpoint, + dst_endpoint: self.dst_endpoint, actor: self.actor, auth_type_id: self.auth_type_id, auth_type_label: self.auth_type_label, @@ -136,56 +117,18 @@ impl<'a, EndpointState> SshActivityBuilder<'a, EndpointState> { self } - /// Set the event activity identifier. - #[must_use] - pub fn activity(mut self, id: ActivityId) -> Self { - self.activity = id; - self - } - - /// Set the action taken. - #[must_use] - pub fn action(mut self, id: ActionId) -> Self { - self.action = Some(id); - self - } - - /// Set the disposition of the action. - #[must_use] - pub fn disposition(mut self, id: DispositionId) -> Self { - self.disposition = Some(id); - self - } - /// Set the acting process. #[must_use] pub fn actor_process(mut self, process: crate::objects::Process) -> Self { self.actor = Some(Actor::process(process)); self } - - /// Set the event severity. - #[must_use] - pub fn severity(mut self, id: SeverityId) -> Self { - self.severity = id; - self - } - - /// Set the overall event status. - #[must_use] - pub fn status(mut self, id: StatusId) -> Self { - self.status = Some(id); - self - } - - /// Set a human-readable event message. - #[must_use] - pub fn message(mut self, msg: impl Into) -> Self { - self.message = Some(msg.into()); - self - } } +impl_activity_setter!(SshActivityBuilder); +impl_action_disposition_setters!(SshActivityBuilder); +impl_builder_setters!(SshActivityBuilder); + impl SshActivityBuilder<'_, HasSshEndpoint> { #[must_use] pub fn build(self) -> OcsfEvent { diff --git a/crates/openshell-ocsf/src/ctx.rs b/crates/openshell-ocsf/src/ctx.rs index 1a72fe0bd..25651b406 100644 --- a/crates/openshell-ocsf/src/ctx.rs +++ b/crates/openshell-ocsf/src/ctx.rs @@ -16,18 +16,10 @@ use std::sync::{Arc, LazyLock, OnceLock}; static OCSF_CTX: OnceLock = OnceLock::new(); -/// With `test-support`, producer tests get a production-like sandbox identity -/// so their events carry the same container and device fields production does. -#[cfg(any(test, feature = "test-support"))] -const FALLBACK_SANDBOX: (&str, &str, &str) = - ("sandbox-test", "test-sandbox", "example/sandbox:test"); -#[cfg(not(any(test, feature = "test-support")))] -const FALLBACK_SANDBOX: (&str, &str, &str) = ("", "", ""); - static OCSF_CTX_FALLBACK: LazyLock = LazyLock::new(|| EventContext { - sandbox_id: FALLBACK_SANDBOX.0.to_string(), - sandbox_name: FALLBACK_SANDBOX.1.to_string(), - container_image: FALLBACK_SANDBOX.2.to_string(), + sandbox_id: String::new(), + sandbox_name: String::new(), + container_image: String::new(), hostname: "test".to_string(), product_version: env!("CARGO_PKG_VERSION").to_string(), proxy_ip: std::net::IpAddr::from([127, 0, 0, 1]), @@ -67,3 +59,13 @@ pub fn ctx() -> ContextRef<'static> { } ContextRef::Borrowed(OCSF_CTX.get().unwrap_or(&OCSF_CTX_FALLBACK)) } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn fallback_context_names_no_sandbox() { + assert!(OCSF_CTX_FALLBACK.container().is_none()); + } +} diff --git a/docs/observability/ocsf-json-export.mdx b/docs/observability/ocsf-json-export.mdx index f7ec64ef3..cdc5f9418 100644 --- a/docs/observability/ocsf-json-export.mdx +++ b/docs/observability/ocsf-json-export.mdx @@ -163,6 +163,8 @@ Each JSONL file line, or the payload after the stderr marker, is a complete OCSF "category_name": "Network Activity", "activity_id": 1, "activity_name": "Open", + "type_uid": 400101, + "type_name": "Network Activity: Open", "severity_id": 1, "severity": "Informational", "status_id": 1, diff --git a/e2e/rust/tests/ocsf_jsonl_conformance.rs b/e2e/rust/tests/ocsf_jsonl_conformance.rs index ceae9480f..cda6d5874 100644 --- a/e2e/rust/tests/ocsf_jsonl_conformance.rs +++ b/e2e/rust/tests/ocsf_jsonl_conformance.rs @@ -135,13 +135,7 @@ fn supervisor_records(engine: &ContainerEngine, sandbox: &str) -> Result Result = + serde_json::from_slice(&ps.stdout).map_err(|e| format!("parse ps output: {e}"))?; + let id = containers + .iter() + .find_map(|container| container["Labels"]["openshell.ai/sandbox-id"].as_str()) .ok_or_else(|| format!("no container matches {filter}"))?; let dir = tempfile::tempdir().map_err(|e| format!("temp dir: {e}"))?; @@ -222,8 +218,9 @@ async fn supervisor_jsonl_conforms_to_the_native_schema() { // every required class appears. let mut records = Vec::new(); let mut last_error = None; + let mut last_exec_error = None; for _ in 0..12 { - let _ = guard.exec(&["python3", "-c", &traffic]).await; + last_exec_error = guard.exec(&["python3", "-c", &traffic]).await.err(); tokio::time::sleep(Duration::from_secs(5)).await; match supervisor_records(&engine, &guard.name) { Ok(found) => { @@ -241,9 +238,11 @@ async fn supervisor_jsonl_conforms_to_the_native_schema() { let missing = missing_classes(&records); assert!( missing.is_empty(), - "missing OCSF classes {missing:?} in {} records; last copy error: {}", + "missing OCSF classes {missing:?} in {} records; last copy error: {}; \ + last exec error: {}", records.len(), - last_error.as_deref().unwrap_or("none") + last_error.as_deref().unwrap_or("none"), + last_exec_error.as_deref().unwrap_or("none") ); let mut failures = Vec::new(); for record in &records {