From 5201a7cd0bdd2fb2e956f8a084b576dd62879955 Mon Sep 17 00:00:00 2001 From: Kraemii Date: Thu, 15 Feb 2024 11:20:39 +0100 Subject: [PATCH] Change: set Scan ID of Scan Model to non-optional As the scan ID was set by the server and not by the client, it was set to Optional. This lead to many unnecessary checks, even though it was not possible, that this value is None. The new implementation automatically fills in an UUID for the scan ID, when parsing a Scan Config. --- rust/Cargo.lock | 1 + rust/models/Cargo.toml | 6 ++++-- rust/models/src/scan.rs | 8 ++++++-- rust/openvasctl/src/ctl.rs | 8 ++------ rust/openvasd/src/controller/entry.rs | 10 ++-------- rust/openvasd/src/controller/mod.rs | 2 +- rust/openvasd/src/storage/file.rs | 10 +++++----- rust/openvasd/src/storage/inmemory.rs | 20 +++++++++----------- rust/osp/src/commands.rs | 27 ++++++++++----------------- 9 files changed, 40 insertions(+), 52 deletions(-) diff --git a/rust/Cargo.lock b/rust/Cargo.lock index a7854f5dc..44052eb25 100644 --- a/rust/Cargo.lock +++ b/rust/Cargo.lock @@ -1636,6 +1636,7 @@ dependencies = [ "bincode 2.0.0-rc.3", "serde", "serde_json", + "uuid", ] [[package]] diff --git a/rust/models/Cargo.toml b/rust/models/Cargo.toml index ca8d8908e..f02d6db14 100644 --- a/rust/models/Cargo.toml +++ b/rust/models/Cargo.toml @@ -7,8 +7,10 @@ license = "GPL-2.0-or-later" # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html [dependencies] -serde = {version = "1", features = ["derive"], optional = true} -bincode = {version = "2.0.0-rc.3", optional = true } +serde = { version = "1", features = ["derive"], optional = true } +bincode = { version = "2.0.0-rc.3", optional = true } +uuid = { version = "1", features = ["v4"] } + [features] default = ["serde_support", "bincode_support"] diff --git a/rust/models/src/scan.rs b/rust/models/src/scan.rs index aac965aa6..638b00a4a 100644 --- a/rust/models/src/scan.rs +++ b/rust/models/src/scan.rs @@ -13,9 +13,9 @@ use super::{scanner_preference::ScannerPreference, target::Target, vt::VT}; )] #[cfg_attr(feature = "bincode_support", derive(bincode::Encode, bincode::Decode))] pub struct Scan { - #[cfg_attr(feature = "serde_support", serde(default))] + #[cfg_attr(feature = "serde_support", serde(skip_deserializing, default = "uuid"))] /// Unique ID of a scan - pub scan_id: Option, + pub scan_id: String, /// Information about the target to scan pub target: Target, #[cfg_attr(feature = "serde_support", serde(default))] @@ -24,3 +24,7 @@ pub struct Scan { /// List of VTs to execute for the target pub vts: Vec, } + +fn uuid() -> String { + uuid::Uuid::new_v4().to_string() +} diff --git a/rust/openvasctl/src/ctl.rs b/rust/openvasctl/src/ctl.rs index 869621c9c..2c9ee0b19 100644 --- a/rust/openvasctl/src/ctl.rs +++ b/rust/openvasctl/src/ctl.rs @@ -91,11 +91,7 @@ impl OpenvasController { /// Prepare scan and start it pub fn start_scan(&mut self, scan: models::Scan) -> Result<(), OpenvasError> { - let scan_id = match scan.scan_id { - Some(id) => id, - None => return Err(OpenvasError::MissingID), - }; - self.add_init(scan_id.clone()); + self.add_init(scan.scan_id.clone()); // TODO: Create new DB for Scan // TODO: Add Scan ID to DB @@ -107,7 +103,7 @@ impl OpenvasController { // TODO: Prepare reverse lookup option (maybe part of target) // TODO: Prepare alive test option (maybe part of target) - if !self.add_running(scan_id)? { + if !self.add_running(scan.scan_id)? { return Ok(()); } diff --git a/rust/openvasd/src/controller/entry.rs b/rust/openvasd/src/controller/entry.rs index 20b74191a..98bb5168a 100644 --- a/rust/openvasd/src/controller/entry.rs +++ b/rust/openvasd/src/controller/entry.rs @@ -256,15 +256,9 @@ where (&Method::POST, Scans(None)) => { match crate::request::json_request::(&ctx.response, req).await { - Ok(mut scan) => { - if scan.scan_id.is_some() { - return Ok(ctx - .response - .bad_request("field scan_id is not allowed to be set.")); - } - let id = uuid::Uuid::new_v4().to_string(); + Ok(scan) => { + let id = scan.scan_id.clone(); let resp = ctx.response.created(&id); - scan.scan_id = Some(id.clone()); ctx.db.insert_scan(scan).await?; ctx.db.add_scan_client_id(id.clone(), cid).await?; tracing::debug!("Scan with ID {} created", &id); diff --git a/rust/openvasd/src/controller/mod.rs b/rust/openvasd/src/controller/mod.rs index 87992cf22..82ec252f6 100644 --- a/rust/openvasd/src/controller/mod.rs +++ b/rust/openvasd/src/controller/mod.rs @@ -361,7 +361,7 @@ mod tests { #[tokio::test] async fn add_scan_with_id_fails() { let scan: models::Scan = models::Scan { - scan_id: Some(String::new()), + scan_id: String::new(), ..Default::default() }; let ctx = Arc::new(Context::default()); diff --git a/rust/openvasd/src/storage/file.rs b/rust/openvasd/src/storage/file.rs index 6e3f1c7fa..d16759e06 100644 --- a/rust/openvasd/src/storage/file.rs +++ b/rust/openvasd/src/storage/file.rs @@ -212,7 +212,7 @@ where S: infisto::base::IndexedByteStorage + std::marker::Sync + std::marker::Send + Clone + 'static, { async fn insert_scan(&self, scan: models::Scan) -> Result<(), Error> { - let id = scan.scan_id.clone().unwrap_or_default(); + let id = scan.scan_id.clone(); let key = format!("scan_{id}"); let status_key = format!("status_{id}"); let storage = Arc::clone(&self.storage); @@ -562,7 +562,7 @@ mod tests { } "#; let mut scan: Scan = serde_json::from_str(jraw).unwrap(); - scan.scan_id = Some("aha".to_string()); + scan.scan_id = "aha".to_string(); let storage = infisto::base::CachedIndexFileStorer::init("/tmp/openvasd/credential").unwrap(); let nfp = "../../examples/feed/nasl"; @@ -615,7 +615,7 @@ mod tests { let mut scans = Vec::with_capacity(100); for i in 0..100 { let scan = Scan { - scan_id: Some(i.to_string()), + scan_id: i.to_string(), ..Default::default() }; scans.push(scan); @@ -631,7 +631,7 @@ mod tests { } for s in scans.clone().into_iter() { - storage.get_scan(&s.scan_id.unwrap()).await.unwrap(); + storage.get_scan(&s.scan_id).await.unwrap(); } storage.remove_scan("5").await.unwrap(); storage.insert_scan(scans[5].clone()).await.unwrap(); @@ -665,7 +665,7 @@ mod tests { .collect(); assert_eq!(2, range.len()); for s in scans { - let _ = storage.remove_scan(&s.scan_id.unwrap_or_default()).await; + let _ = storage.remove_scan(&s.scan_id).await; } let ids = storage.get_scan_ids().await.unwrap(); diff --git a/rust/openvasd/src/storage/inmemory.rs b/rust/openvasd/src/storage/inmemory.rs index cfa5b3798..9f9439131 100644 --- a/rust/openvasd/src/storage/inmemory.rs +++ b/rust/openvasd/src/storage/inmemory.rs @@ -184,7 +184,7 @@ where E: crate::crypt::Crypt + Send + Sync + 'static, { async fn insert_scan(&self, sp: models::Scan) -> Result<(), Error> { - let id = sp.scan_id.clone().unwrap_or_default(); + let id = sp.scan_id.clone(); let mut scans = self.scans.write().await; if let Some(prgs) = scans.get_mut(&id) { prgs.scan = sp; @@ -243,9 +243,7 @@ where let scans = self.scans.read().await; let mut result = Vec::with_capacity(scans.len()); for (_, progress) in scans.iter() { - if let Some(id) = progress.scan.scan_id.as_ref() { - result.push(id.clone()); - } + result.push(progress.scan.scan_id.clone()); } Ok(result) } @@ -441,10 +439,10 @@ mod tests { async fn store_delete_scan() { let storage = Storage::default(); let scan = Scan::default(); - let id = scan.scan_id.clone().unwrap_or_default(); + let id = scan.scan_id.clone(); storage.insert_scan(scan).await.unwrap(); let (retrieved, _) = storage.get_scan(&id).await.unwrap(); - assert_eq!(retrieved.scan_id.unwrap_or_default(), id); + assert_eq!(retrieved.scan_id, id); storage.remove_scan(&id).await.unwrap(); } @@ -462,21 +460,21 @@ mod tests { scan.target.credentials = vec![pw]; - let id = scan.scan_id.clone().unwrap_or_default(); + let id = scan.scan_id.clone(); storage.insert_scan(scan).await.unwrap(); let (retrieved, _) = storage.get_scan(&id).await.unwrap(); - assert_eq!(retrieved.scan_id.unwrap_or_default(), id); + assert_eq!(retrieved.scan_id, id); assert_ne!(retrieved.target.credentials[0].password(), "test"); let (retrieved, _) = storage.get_decrypted_scan(&id).await.unwrap(); - assert_eq!(retrieved.scan_id.unwrap_or_default(), id); + assert_eq!(retrieved.scan_id, id); assert_eq!(retrieved.target.credentials[0].password(), "test"); } async fn store_scan(storage: &Storage) -> String { let mut scan = Scan::default(); let id = uuid::Uuid::new_v4().to_string(); - scan.scan_id = Some(id.clone()); + scan.scan_id = id.clone(); storage.insert_scan(scan).await.unwrap(); id } @@ -495,7 +493,7 @@ mod tests { async fn append_results() { let storage = Storage::default(); let scan = Scan::default(); - let id = scan.scan_id.clone().unwrap_or_default(); + let id = scan.scan_id.clone(); storage.insert_scan(scan).await.unwrap(); let fetch_result = (models::Status::default(), vec![models::Result::default()]); storage diff --git a/rust/osp/src/commands.rs b/rust/osp/src/commands.rs index 445be7ee3..e1e147051 100644 --- a/rust/osp/src/commands.rs +++ b/rust/osp/src/commands.rs @@ -28,17 +28,13 @@ type Writer = quick_xml::Writer>>; impl<'a> ScanCommand<'a> { fn as_byte_response( - scan_id: Option<&str>, + scan_id: &str, element_name: &str, additional: &[(&str, &str)], f: &mut dyn FnMut(&mut Writer) -> Result<()>, ) -> Result> { let mut writer = Writer::new(Cursor::new(Vec::new())); - if let Some(scan_id) = &scan_id { - writer.within_id_element(IDAttribute::ScanID(scan_id), additional, element_name, f)?; - } else { - writer.within_element(element_name, f)?; - } + writer.within_id_element(IDAttribute::ScanID(scan_id), additional, element_name, f)?; let result = writer.into_inner().into_inner(); Ok(result) } @@ -46,32 +42,29 @@ impl<'a> ScanCommand<'a> { /// Returns the XML representation of the command. pub fn try_to_xml(&self) -> Result> { match self { - ScanCommand::Start(scan) => ScanCommand::as_byte_response( - scan.scan_id.as_deref(), - "start_scan", - &[], - &mut |writer| { + ScanCommand::Start(scan) => { + ScanCommand::as_byte_response(&scan.scan_id, "start_scan", &[], &mut |writer| { write_vts(scan, writer)?; write_target(scan, writer)?; write_scanner_prefs(scan, writer)?; Ok(()) - }, - ), + }) + } ScanCommand::Delete(scan_id) => { - ScanCommand::as_byte_response(Some(scan_id), "delete_scan", &[], &mut |_| Ok(())) + ScanCommand::as_byte_response(scan_id, "delete_scan", &[], &mut |_| Ok(())) } ScanCommand::Stop(scan_id) => { - ScanCommand::as_byte_response(Some(scan_id), "stop_scan", &[], &mut |_| Ok(())) + ScanCommand::as_byte_response(scan_id, "stop_scan", &[], &mut |_| Ok(())) } ScanCommand::GetDelete(scan_id) => ScanCommand::as_byte_response( - Some(scan_id), + scan_id, "get_scans", // removes results from ospd-openvas &[("pop_results", "1"), ("progress", "1")], &mut |_| Ok(()), ), ScanCommand::Get(scan_id) => { - ScanCommand::as_byte_response(Some(scan_id), "get_scans", &[], &mut |_| Ok(())) + ScanCommand::as_byte_response(scan_id, "get_scans", &[], &mut |_| Ok(())) } } }