feat(network scan): A working scanner with ping, port discovery, broadcast, and netbios #683

irvingoujAtDevolution · 2024-02-05T04:22:22Z

This PR contains code necessary for new functionalities, changes,improvement and refactoring of the network scan crates. Please see network-scan/examples for usages.

@CBenoit Let me know if smaller PR would be preferable, The main changes include

uses DashMap to imporve the preformance of event loop
add netbios,ping,port discovery and broadcast
integrate everything to make a working scanner

CBenoit

Nice work!

Let me know if smaller PR would be preferable

It would indeed be very much welcomed as smaller PRs. Probably it’s too much work for you to change that now so we can try to handle this the way it is now. I’ll not go through every change though.

1. uses DashMap to imporve the preformance of event loop

Did you perform any form of benchmarking before making this move?
I would be interested in seeing some numbers before we add this dependency.
(Following on the above: this alone could be a dedicated PR.)

I’ll continue reviewing tomorrow.

crates/network-scanner-net/src/runtime.rs

crates/network-scanner/src/broadcast/block.rs

crates/network-scanner/src/task_utils.rs

rm bench marks remove clone functions remove unnessary Ok!() macro

irvingoujAtDevolution · 2024-02-05T18:38:07Z

Nice work!

Let me know if smaller PR would be preferable

It would indeed be very much welcomed as smaller PRs. Probably it’s too much work for you to change that now so we can try to handle this the way it is now. I’ll not go through every change though.
1. uses DashMap to imporve the preformance of event loop
Did you perform any form of benchmarking before making this move? I would be interested in seeing some numbers before we add this dependency. (Following on the above: this alone could be a dedicated PR.)

I’ll continue reviewing tomorrow.

I'll try give a benchmark, but notably,even if the performance is slightly worse, the logic is much more strightforward and now lock-free.

Plus, it gives our the potential ability to implement method like async fn writeable(&self) or async fn readable(&self) in the future, by checking if cetarin event exist in the event cache.

irvingoujAtDevolution · 2024-02-05T22:14:56Z

crates/network-scanner-net/Cargo.toml

 futures = "0.3.30"
 parking_lot = "0.12.1"
-polling = { git = "https://github.com/smol-rs/polling", rev = "62430fd56e668559d08ca7071ab13a0e116ba515" }
+polling = { git = "https://github.com/irvingoujAtDevolution/polling.git", branch = "fix-linux-tcp-fail" }


Wasn't aware the problem of Linux side until CI reported the error. :( , this is actually a hard one to fix.

This is currently necessary for linux TCP to work, see line 237 for reason in socket.rs.

I will try convince the repo owner of Polling to merge it, otherwise let's use a branch on our own repo for now.

Can you link to the issue / pull request you created on their side?

the problem on polling is that they are trying to have a platform indepent interface for all polling mechanisms, so I guess they would particularly against have something like this. I have a better idea in mind to how to implement this so it is more acceptable for both us and them, I'll put on a update shortly.

A PR on polling smol-rs/polling#189

irvingoujAtDevolution · 2024-02-05T22:24:32Z

crates/network-scanner-net/src/runtime.rs

@@ -40,7 +41,7 @@ impl Drop for Socket2Runtime {
    }
 }

-const QUEUE_CAPACITY: usize = 1024;
+const QUEUE_CAPACITY: usize = 8024;


Maybe too big?

Any reason why you changed? It wasn’t big enough?

yes, it was causing errors on registering the events.

irvingoujAtDevolution · 2024-02-05T22:27:24Z

crates/network-scanner-net/src/socket.rs

-            return std::task::Poll::Ready(Ok(()));
+            // This is a special case, this happens when using epoll to wait for a unconnected TCP socket.
+            // We clearly needs to call connect function, so we break the loop and call connect.
+            #[cfg(target_os = "linux")]


Linux and Windows have different behavior, Linux shots event to you even before you call connect, we need to identify if the event is "Connect failed" or "Connect successful" or something else (out and hup). Polling does not expose plateform dependent args, so we have to make a banch specific to our needs here. Will disguss with Polling's owner later.

irvingoujAtDevolution · 2024-02-05T22:29:27Z

crates/network-scanner-net/src/test.rs

@@ -15,7 +15,6 @@ use tokio::{
 use crate::socket::AsyncRawSocket;

 #[tokio::test(flavor = "multi_thread", worker_threads = 4)]
-#[ignore = "TODO"]
 async fn multiple_udp() -> anyhow::Result<()> {


Looks like all tests have passed. 😆

crates/network-scanner-proto/Cargo.toml

crates/network-scanner/src/ping.rs

irvingoujAtDevolution · 2024-02-05T22:33:07Z

crates/network-scanner/src/task_utils.rs

+    pub(crate) handles_sender: HandlesSender,
+}
+
+impl TaskExecutionContext {


This is created purely for the sake of readability, it was super hard of keep track of things without it.

crates/network-scanner/src/task_utils.rs

crates/network-scanner/Cargo.toml

crates/network-scanner/src/task_utils.rs

Co-authored-by: Benoît Cortier <[email protected]>

crates/network-scanner/src/task_utils.rs

…ions-gateway into Network-Scan-Scanner

CBenoit

We need to clean up the dependencies before we merge. Incidentally, the dependency on futures is unrequired not only for network-scanner but also for network-scanner-net

CBenoit

There is also a conflict to fix with master, but otherwise I’m thinking about merging today

crates/network-scanner/src/task_utils.rs

irvingoujAtDevolution · 2024-02-09T03:58:30Z

We need to clean up the dependencies before we merge. Incidentally, the dependency on futures is unrequired not only for network-scanner but also for network-scanner-net

updated, removed all unsued dependencies

irvingoujAtDevolution · 2024-02-09T03:58:50Z

There is also a conflict to fix with master, but otherwise I’m thinking about merging today

updated, thank you!

CBenoit · 2024-02-09T16:48:56Z

crates/network-scanner-net/Cargo.toml

-polling = { git = "https://github.com/devolutions/polling.git", branch = "fix-detect-connection-fail" }
+polling = { git = "https://github.com/devolutions/polling.git", rev = "c04e8ee40415cad551fe044457270f4a2d7c491d" }


Let’s hardcode the commit rather than the branch

CBenoit

Let’s merge. Thank you for this work!
In the future, try to send single-purpose PRs, please.
It’s okay even if the PR is very small, it’ll be quick to review.

irvingoujAtDevolution added 6 commits February 4, 2024 23:18

Network Scanner finished

bc31f68

Cargo.lock

4c60074

update cargo

cda38e5

remove drop runtime

178c6c0

checkout master for examples

7c6f256

checkout master for tests

c9d7af4

irvingoujAtDevolution requested a review from CBenoit February 5, 2024 04:29

fix ci test

2d2f054

CBenoit reviewed Feb 5, 2024

View reviewed changes

crates/network-scanner-net/src/runtime.rs Outdated Show resolved Hide resolved

crates/network-scanner/src/broadcast/block.rs Outdated Show resolved Hide resolved

crates/network-scanner/src/task_utils.rs Outdated Show resolved Hide resolved

irvingoujAtDevolution added 7 commits February 5, 2024 09:09

clippy and fmt

b145e36

Fix Runtime Logic

20df4fd

fix connect logic

e68b37d

review fix

00e2865

Update test.rs

a9c41a9

Debuggin test

6325c7f

CI and Review fix

a227487

rm bench marks remove clone functions remove unnessary Ok!() macro

irvingoujAtDevolution force-pushed the Network-Scan-Scanner branch from 4e6f34d to a227487 Compare February 5, 2024 18:32

irvingoujAtDevolution added 2 commits February 5, 2024 13:51

Fix tests

66249d6

Fix linux test

d05cf0b

irvingoujAtDevolution commented Feb 5, 2024

View reviewed changes

crates/network-scanner-proto/Cargo.toml Outdated Show resolved Hide resolved

irvingoujAtDevolution commented Feb 5, 2024

View reviewed changes

crates/network-scanner/src/ping.rs Show resolved Hide resolved

irvingoujAtDevolution commented Feb 5, 2024

View reviewed changes

remove code not related to PR

d92ec84

irvingoujAtDevolution commented Feb 5, 2024

View reviewed changes

crates/network-scanner/src/task_utils.rs Outdated Show resolved Hide resolved

small fixes, change log level

1b37f06

CBenoit reviewed Feb 9, 2024

View reviewed changes

crates/network-scanner/Cargo.toml Outdated Show resolved Hide resolved

fix examples

28634b9

CBenoit reviewed Feb 9, 2024

View reviewed changes

crates/network-scanner/src/task_utils.rs Outdated Show resolved Hide resolved

Update crates/network-scanner/src/task_utils.rs

2efeb0c

Co-authored-by: Benoît Cortier <[email protected]>

CBenoit reviewed Feb 9, 2024

View reviewed changes

crates/network-scanner/src/task_utils.rs Outdated Show resolved Hide resolved

irvingoujAtDevolution added 2 commits February 8, 2024 22:14

review fix

f298fa2

Merge branch 'Network-Scan-Scanner' of github.com:Devolutions/devolut…

5104f51

…ions-gateway into Network-Scan-Scanner

CBenoit reviewed Feb 9, 2024

View reviewed changes

irvingoujAtDevolution added 2 commits February 8, 2024 22:17

review fix

b607462

review fix

e646b99

CBenoit reviewed Feb 9, 2024

View reviewed changes

irvingoujAtDevolution added 3 commits February 8, 2024 22:30

review fix and better tracing

ffe8982

merge master

06a10fd

review fix

bdc9edf

CBenoit reviewed Feb 9, 2024

View reviewed changes

crates/network-scanner/src/task_utils.rs Outdated Show resolved Hide resolved

review fix

586f64d

irvingoujAtDevolution added 2 commits February 8, 2024 23:20

review fix

657d46f

adjust queue size

a541598

irvingoujAtDevolution requested a review from CBenoit February 9, 2024 04:40

irvingoujAtDevolution and others added 3 commits February 9, 2024 10:40

add netbios interval

58cf8d7

add dns resolver

ef29417

Fix dependencies

877ebe3

CBenoit reviewed Feb 9, 2024

View reviewed changes

CBenoit approved these changes Feb 9, 2024

View reviewed changes

CBenoit disabled auto-merge February 9, 2024 16:50

CBenoit enabled auto-merge (squash) February 9, 2024 16:51

CBenoit merged commit 0f4894d into master Feb 9, 2024
16 checks passed

CBenoit deleted the Network-Scan-Scanner branch February 9, 2024 17:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(network scan): A working scanner with ping, port discovery, broadcast, and netbios #683

feat(network scan): A working scanner with ping, port discovery, broadcast, and netbios #683

irvingoujAtDevolution commented Feb 5, 2024 •

edited

Loading

CBenoit left a comment

irvingoujAtDevolution commented Feb 5, 2024 •

edited

Loading

irvingoujAtDevolution Feb 5, 2024 •

edited

Loading

CBenoit Feb 6, 2024

irvingoujAtDevolution Feb 6, 2024

irvingoujAtDevolution Feb 7, 2024 •

edited

Loading

irvingoujAtDevolution Feb 5, 2024

CBenoit Feb 6, 2024 •

edited

Loading

irvingoujAtDevolution Feb 6, 2024

irvingoujAtDevolution Feb 5, 2024

irvingoujAtDevolution Feb 5, 2024

CBenoit Feb 6, 2024

irvingoujAtDevolution Feb 5, 2024

CBenoit left a comment

CBenoit left a comment

irvingoujAtDevolution commented Feb 9, 2024

irvingoujAtDevolution commented Feb 9, 2024

CBenoit Feb 9, 2024

CBenoit left a comment •

edited

Loading

		polling = { git = "https://github.com/devolutions/polling.git", branch = "fix-detect-connection-fail" }
		polling = { git = "https://github.com/devolutions/polling.git", rev = "c04e8ee40415cad551fe044457270f4a2d7c491d" }

feat(network scan): A working scanner with ping, port discovery, broadcast, and netbios #683

feat(network scan): A working scanner with ping, port discovery, broadcast, and netbios #683

Conversation

irvingoujAtDevolution commented Feb 5, 2024 • edited Loading

CBenoit left a comment

Choose a reason for hiding this comment

irvingoujAtDevolution commented Feb 5, 2024 • edited Loading

irvingoujAtDevolution Feb 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

irvingoujAtDevolution Feb 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CBenoit Feb 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CBenoit left a comment

Choose a reason for hiding this comment

CBenoit left a comment

Choose a reason for hiding this comment

irvingoujAtDevolution commented Feb 9, 2024

irvingoujAtDevolution commented Feb 9, 2024

Choose a reason for hiding this comment

CBenoit left a comment • edited Loading

Choose a reason for hiding this comment

irvingoujAtDevolution commented Feb 5, 2024 •

edited

Loading

irvingoujAtDevolution commented Feb 5, 2024 •

edited

Loading

irvingoujAtDevolution Feb 5, 2024 •

edited

Loading

irvingoujAtDevolution Feb 7, 2024 •

edited

Loading

CBenoit Feb 6, 2024 •

edited

Loading

CBenoit left a comment •

edited

Loading