Skip to content

Change: progress information #1832

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Change: progress information #1832

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ec2d5e4
Doc: Add progress information to HTTP API
jjnicola Jan 31, 2025
c510808
Change: scan status information returns the total scripts and finishe…
jjnicola Feb 3, 2025
03ebe0e
Doc: update faq regarding progress calculation
jjnicola Feb 3, 2025
cc684f2
fix typo and apply suggestions
jjnicola Feb 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 42 additions & 13 deletions rust/doc/openapi.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -895,10 +895,7 @@ components:
type: "integer"
format: "int32"
scanning:
description: "The IP Addresses of the currently scanned hosts."
type: "array"
items:
type: "string"
$ref: "#/components/schemas/Scanning"

required:
- all
Expand All @@ -908,6 +905,30 @@ components:
- queued
- finished

SingleHostProgress: {
type: "object",
description: "Additional information about the scanned host",
properties: {
finished_tests: {
description: "The number of vulnerability test alredy run for the host",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
description: "The number of vulnerability test alredy run for the host",
description: "The number of vulnerability tests already run for the host",

type: "integer",
format: "int32"
},
total_tests: {
description: "The total amount of vulnerability test to be run for the host",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
description: "The total amount of vulnerability test to be run for the host",
description: "The total amount of vulnerability tests to be run for the host",

type: "integer",
format: "int32"
}
}
}

Scanning:
description: "The IP Addresses of the currently scanned hosts."
type: "object"
additionalProperties: {
$ref: "#/components/schemas/SingleHostProgress"
}

ScanAction:
description: "An action to perform on a scan"
type: "object"
Expand Down Expand Up @@ -1228,16 +1249,24 @@ components:
{
"start_time": 1679649183,
"status": "running",
"host_info":
{
"all": 14,
"excluded": 0,
"dead": 4,
"alive": 6,
"queued": 1,
"finished": 1,
"scanning": ["127.0.0.1", "10.0.5.1", "10.0.5.2", "10.0.5.3"],
"host_info": {
"all": 14,
"excluded": 0,
"dead": 4,
"alive": 6,
"queued": 1,
"finished": 1,
"scanning": {
"192.168.0.1": {
"finished_tests": 456,
"total_tests": 1000
},
"192.168.0.2": {
"finished_tests": 456,
"total_tests": 1000
}
},
},
}

scan_status_success:
Expand Down
43 changes: 37 additions & 6 deletions rust/src/models/host_info.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ pub struct HostInfoBuilder {
pub alive: u64,
pub queued: u64,
pub finished: u64,
pub scanning: Option<HashMap<String, i32>>,
pub scanning: Option<HashMap<String, SingleHostScanInfo>>,
}

impl HostInfoBuilder {
Expand All @@ -32,6 +32,33 @@ impl HostInfoBuilder {
}
}

#[derive(Default, Debug, Clone, Eq, PartialEq)]
#[cfg_attr(
feature = "serde_support",
derive(serde::Serialize, serde::Deserialize)
)]
pub struct SingleHostScanInfo {
finished_tests: i32,
total_tests: i32,
}

impl SingleHostScanInfo {
pub fn new(finished_tests: i32, total_tests: i32) -> Self {
Self {
finished_tests,
total_tests,
}
}

pub fn finished_tests(&self) -> i32 {
self.finished_tests
}

pub fn total_tests(&self) -> i32 {
self.total_tests
}
}

/// Information about hosts of a running scan
#[derive(Debug, Clone, Default, PartialEq, Eq)]
#[cfg_attr(
Expand All @@ -51,9 +78,8 @@ pub struct HostInfo {
feature = "serde_support",
serde(skip_serializing_if = "Option::is_none")
)]
scanning: Option<HashMap<String, i32>>,
// Hosts that are currently being scanned. The second entry is the number of
// remaining VTs for this host.
scanning: Option<HashMap<String, SingleHostScanInfo>>,
#[cfg_attr(feature = "serde_support", serde(skip))]
remaining_vts_per_host: HashMap<String, usize>,
}

Expand Down Expand Up @@ -92,6 +118,9 @@ impl HostInfo {
}

pub fn update_with(mut self, other: &HostInfo) -> Self {
enum ScanProgress {
DeadHost = -1,
}
// total hosts value is sent once and only once must be updated
if other.all != 0 {
self.all = other.all;
Expand All @@ -112,10 +141,12 @@ impl HostInfo {
// and never completely replaced.
let mut hs = other.scanning.clone().unwrap_or_default();
for (host, progress) in self.scanning.clone().unwrap_or_default().iter() {
if *progress == 100 || *progress == -1 {
if progress.finished_tests() == progress.total_tests()
|| progress.total_tests == ScanProgress::DeadHost as i32
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we already have the SingleHostScanInfo (which I like), I think we could move this logic to some sort of is_finished on the type.

Also I'm not sure I fully understand the ScanProgress enum. Are we expecting further variants to be added to that at some point? Maybe a clearer way to express the intent (at least as I understand it) would be to make total_tests some sort of

enum TotalTests {
    Num(i32),
    DeadHost,
}

and to parse that from the given i32 upon creation?

{
hs.remove(host);
} else {
hs.insert(host.to_string(), *progress);
hs.insert(host.to_string(), progress.clone());
}
}
self.scanning = Some(hs);
Expand Down
32 changes: 21 additions & 11 deletions rust/src/openvas/result_collector.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,12 @@ use std::{
sync::{Arc, Mutex},
};

use crate::openvas::openvas_redis::{KbAccess, VtHelper};
use crate::osp::{OspResultType, OspScanResult};
use crate::storage::redis::RedisStorageResult;
use crate::{
models::SingleHostScanInfo,
openvas::openvas_redis::{KbAccess, VtHelper},
};

/// Structure to hold the results retrieve from redis main kb
#[derive(Default, Debug, Clone)]
Expand All @@ -30,7 +33,7 @@ pub struct Results {
/// during the scan
pub count_dead: i64,
/// Current hosts status
pub host_status: HashMap<String, i32>,
pub host_status: HashMap<String, SingleHostScanInfo>,
/// The scan status
pub scan_status: String,
}
Expand Down Expand Up @@ -165,32 +168,38 @@ where
}
let mut new_dead = 0;
let mut new_alive = 0;
let mut all_hosts: HashMap<String, i32> = HashMap::new();
let mut all_hosts: HashMap<String, SingleHostScanInfo> = HashMap::new();
for res in redis_status {
let mut fields = res.splitn(3, '/');
let current_host = fields.next().expect("Valid status value");
let launched = fields.next().expect("Valid status value");
let total = fields.next().expect("Valid status value");

let host_progress: i32 = match i32::from_str(total) {
let total = match i32::from_str(total) {
// No plugins
Ok(0) => {
continue;
}
// Host Dead
Ok(-1) => ScanProgress::DeadHost as i32,
Ok(n) => ((f32::from_str(launched).expect("Integer") / n as f32) * 100.0) as i32,
Ok(n) => n,
_ => {
continue;
}
};

let launched = i32::from_str(launched).expect("Integer");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that .expect is (imo) the worst-named function in the std lib, since it is so intuitive to write the code exactly as you did, yet this results in an error like

thread 'main' panicked at src/main.rs:2:32:
Integer: ParseIntError { kind: InvalidDigit }

If you unwrap, you get

thread 'main' panicked at src/main.rs:2:32:
called `Result::unwrap()` on an `Err` value: ParseIntError { kind: InvalidDigit }

which is almost more informative ... I think the better thing is to either .unwrap(), or provide the actual error message via .expect("Expected an integer") or something similar.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we even unwrap here? It is returning a RedisStorageResult so it could be i32::from_str(launched).map_err(|x|...)?;.


let host_progress = ((launched as f32 / total as f32) * 100.0) as i32;
if host_progress == -1 {
new_dead += 1;
} else if host_progress == 100 {
new_alive += 1;
}
all_hosts.insert(current_host.to_string(), host_progress);
all_hosts.insert(
current_host.to_string(),
SingleHostScanInfo::new(launched, total),
);

tracing::debug!("Host {} has progress: {}", current_host, host_progress);
}
Expand Down Expand Up @@ -225,6 +234,7 @@ mod tests {

use crate::models::{self, Protocol, Result, ResultType};
use crate::openvas::openvas_redis::FakeRedis;
use crate::openvas::result_collector::SingleHostScanInfo;
use std::collections::HashMap;

use super::ResultHelper;
Expand Down Expand Up @@ -346,11 +356,11 @@ mod tests {
resh.process_status(status).unwrap();

let mut r = HashMap::new();
r.insert("127.0.0.1".to_string(), 12);
r.insert("127.0.0.3".to_string(), 75);
r.insert("127.0.0.4".to_string(), 100);
r.insert("127.0.0.2".to_string(), -1);
r.insert("127.0.0.5".to_string(), -1);
r.insert("127.0.0.1".to_string(), SingleHostScanInfo::new(128, 1000));
r.insert("127.0.0.3".to_string(), SingleHostScanInfo::new(750, 1000));
r.insert("127.0.0.4".to_string(), SingleHostScanInfo::new(1000, 1000));
r.insert("127.0.0.2".to_string(), SingleHostScanInfo::new(0, -1));
r.insert("127.0.0.5".to_string(), SingleHostScanInfo::new(0, -1));

assert_eq!(resh.results.as_ref().lock().unwrap().host_status, r);
assert_eq!(resh.results.as_ref().lock().unwrap().count_alive, 1);
Expand Down
6 changes: 4 additions & 2 deletions rust/src/osp/response.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ use std::{collections::HashMap, fmt};

use serde::{de::Visitor, Deserialize};

use crate::models::SingleHostScanInfo;

use super::commands::Error;

/// StringU32 is a wrapper around u32 to allow deserialization of strings
Expand Down Expand Up @@ -602,10 +604,10 @@ impl From<Scan> for crate::models::Status {
ScanStatus::Interrupted => crate::models::Phase::Failed,
};

let mut scanning: HashMap<String, i32> = HashMap::new();
let mut scanning: HashMap<String, SingleHostScanInfo> = HashMap::new();
if let Some(i) = &value.host_info {
for host in &i.host {
scanning.insert(host.name.clone(), 0);
scanning.insert(host.name.clone(), SingleHostScanInfo::new(0, 0));
}
}
crate::models::Status {
Expand Down
Loading