Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Merge pull request #1698 from greenbone/async-interpreter
-- Structure of Scanner Code --
This PR adds the `Scanner` type in the `nasl_interpreter` crate. (It would be nicer to have this in a different crate but I don't want to add a new one. Maybe in the future if we ever merge some of the crates, this can be moved/renamed. ) The `Scanner` is the single instance managing all scans during a run with `Openvasd` scanner type. To do so, it starts a number of `RunningScan`s, each of which is responsible for a single `Scan`.
The `RunningScan` is responsible for computing the execution plan for this particular `Scan` and running it to completion. It also takes care of controlling the status of the scan and stopping it if necessary. Internally, it runs code via the `ScanRunner` (sorry, I ran out of names) which is responsible for performing all VTs in all stages on each target host. It is also responsible for sticking to the scheduling requirements. I suspect that this part of the code is where we should be able to easily improve performance by making the scripts run concurrently.
Finally, for a given VT and a given Host, the VT is then run to completion using the `VTRunner`.
-- Tests --
Since all of our NASL test employed the `Iterator` impl on `CodeInterpreter` which cannot exist anymore (since the `CodeInterpreter` is now async), they all needed to be changed. I took this as an opportunity to move the tests to a more unified structure, so that it will be easier to 1. add new tests and 2. change how the tests are done internally without having to make changes all over the code base.
Here is an example how the tests work now:
```rust
let mut t = TestBuilder::default();
t.run("l = [1,2,3,4,5];");
t.ok("max_index(l);", 5);
```
The `TestBuilder` struct can be used to set up the proper `Context`, `Storage` and function `Loader` and has a number of utility functions to perform common tests.
Lines of code can be added to the `TestBuilder` one-by-one, along with certain requirements on their results (For example, the `t.ok("max_index(l)", 5)` above will check that the result of the `max_index(l)` call is `NaslValue::Number(5)`.). At the end of the test, the `TestBuilder` will automatically check that all of the requirements are fulfilled. Even though lines are added individually, the `TestBuilder` keeps track of the state of the `Context` during execution, so tests that require multiple lines to set up (like the one above) will still work as intended.
To test that the correct `Err` variant is returned in certain lines, the `check_err_matches!` macro can be used as before, by passing it the `TestBuilder` as an argument:
```rust
check_err_matches!(
t,
r#"typeof(23,76);"#,
FunctionErrorKind::TrailingPositionalArguments { .. }
);
```
To quickly check individual lines, `check_ok` still exists.
-- Function executor - -
Previously, every set of NASL functions was added to the standard library by implementing the `NaslFunctionExecutor` trait for a struct (that might be either a marker ZST or a proper stateful struct). This approach was difficult to incorporate with the new `async` interpreter where NASL functions might be both sync or async, since async functions can't really be referred to by a simple type like `fn(&Context, &Register) -> NaslResult` like sync functions can.
Instead, the executor part of the `Context` is now an actual struct: `Executor`. This internally tracks all the registered functions and keeps all the necessary state (such as open ssh connections) around. Moreover, it provides the same functionality that `NaslFunctionExecutor` provided previously (namely executing functions by name).
The `Executor` can store multiple `FunctionSet`s which are collections as functions. A new `FunctionSet` can be created as such:
```rust
pub struct Array;
function_set! {
Array,
sync_stateless,
(
make_array,
make_list,
(nasl_sort, "sort"),
keys,
max_index,
)
}
```
which automatically implements the `IntoFunctionSet` trait on `Array` and adds the given 5 functions. The functions will automatically be entered with their own name unless specified otherwise (as for `nasl_sort` here). This replaces the `lookup` functions we had to write before.
At this point, NASL functions come in four flavors:
1. `async_stateful` (for `async fn(&S, &Register, &Context)`)
2. `sync_stateful` (for `fn(&S, &Register, &Context)`)
3. `async_stateless` (for `async fn(&Register, &Context)`)
4. `sync_stateless` (for `fn(&Register, &Context)`)
At the moment, the user (i.e. the person using the `function_set!` macro) unfortunately needs to specify what flavour of function the set contains. (This is because the rust compiler does not know that something implementing `Fn(A, B) -> Z` will never also implement `Fn(A, B, C) -> Z`, so that implementing a common trait for all four flavours is impossible since the compiler will think that we implemented the trait multiple times for a single object. Unfortunately, specialization is not yet a thing in Rust.).
In the future, I think it should be possible to do something very cool with the `Executor`: Previously, we only got read access to the state (i.e. to the SSH handles), so we had to employ interior mutability in order to work with this state if we ever needed mutable access (in order to add a new SSH handle to our list of handles, for example). Since all the states are now managed by a `StatefulFunctionSet`, and we can tell whether a given function needs mutable access or not by its signature, I think we should be able to use an external RwLock and improve this situation with some careful logic. Concretely, this could mean having `Vec<Arc<Mutex<SSHSession>>>` instead of `Arc<Mutex<Vec<SSHSession>>>` which we previously needed, which would enable tons of concurrency that wasn't previously possible. This is pretty futuristic at this point though.
-- Regressions --
- Removed ability to have a generic reader in `HashSumLoader`. This was done for simplicity and since we currently don't use this ability, however it can be reverted if necessary by adding `Send` bounds on the trait object.
- Removed the ability to have a generic `NaslFunctionExecutor` and replaced it with a `Executor` which is a concrete struct. We still have the ability to only register any subset of functions that we want, so I don't think this is a true regression, but I'm writing it down for completeness' sake.
- Previously, the `NaslFunctionExecutor` trait had a `nasl_fn_cache_clear` method. This wasn't really called properly anywhere other than in `scannerctl` and wasn't handled properly in the `std` library either. I don't think there are currently any instances that needed this functionality, since RAII should take care of it. However, if we ever need this, it is very easy to add back in again.
- I had to revamp how `HostInfo` works to clean up the logic and make it possible to deal with asynchronous scans. First of all, I noticed that `HostInfo` internally stored something which I believe was supposed to be some kind of percentage progress. However, this progress was never actually updated or treated in any way. I removed this percentage value for now. This has an impact on how the update of a `HostInfo` works when a new result comes in and `scanner.do_addition()` returns true. I don't know what the expected behavior is there and since all tests still pass, I will just leave this as it is for now. Secondly, the new `HostInfo` stores the number of leftover VTs for each target host. In this way, it can properly track which hosts still have running VTs and which ones are finished. This was necessary because we now start multiple VTs at the same time, whereas previously they were done one by one.- Loading branch information
