Add support for WinDivert packet capture engine #2019
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #2019 +/- ##
========================================
Coverage 83.45% 83.45%
========================================
Files 311 312 +1
Lines 54556 54568 +12
Branches 11491 11820 +329
========================================
+ Hits 45529 45541 +12
- Misses 7798 7835 +37
+ Partials 1229 1192 -37
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- Add missing doxygen documentation
seladb
changed the title
| OverlappedResult getOverlappedResult(const IWinDivertHandle* handle) override | ||
| { | ||
| auto winDivertHandle = dynamic_cast<const WinDivertHandle*>(handle); | ||
| if (winDivertHandle == nullptr) | ||
| { | ||
| throw std::runtime_error("Failed to get WinDivertHandle"); | ||
| } | ||
|
|
||
| DWORD packetLen = 0; | ||
| if (GetOverlappedResult(winDivertHandle->get(), &m_Overlapped, &packetLen, FALSE)) | ||
| { | ||
| return { OverlappedResult::Status::Success, static_cast<uint32_t>(packetLen), 0 }; | ||
| } | ||
|
|
||
| return { OverlappedResult::Status::Failed, 0, static_cast<uint32_t>(GetLastError()) }; | ||
| } |
There was a problem hiding this comment.
As far as I know, Win32 GetOverlappedResult does not modify anything in the result structure, so I think the whole operation can be const.
Also, wouldn't a reference param be better instead of pointer? We don't want nullptr handles being passed?
There was a problem hiding this comment.
GetOverlappedResult updates m_Overlapped so I guess it shouldn't be made const, no? 🤔
This method gets a pointer because we want to be able to cast it to WinDivertHandle
There was a problem hiding this comment.
GetOverlappedResult updates m_Overlapped so I guess it shouldn't be made const, no?
Sure, we can leave it non-const then.
This method gets a pointer because we want to be able to cast it to WinDivertHandle
We can also cast to WinDivertHandle if we take a reference, as those are technically a pointer under the hood.
OverlappedResult getOverlappedResult(const IWinDivertHandle& handle) override
{
auto winDivertHandle = dynamic_cast<const WinDivertHandle*>(&handle);
if (winDivertHandle == nullptr)
{
throw std::runtime_error("Failed to get WinDivertHandle");
}
/* ... */
}Also if we are just throwing an exception on cast failure can't we just use:
auto& winDivertHandle = dynamic_cast<const WinDivertHandle&>(handle);This has a built-in std::bad_cast throw if the cast fails, which IMO is more descriptive than std::runtime_error?
There was a problem hiding this comment.
We can also cast to
WinDivertHandleif we take a reference, as those are technically a pointer under the hood.
I guess we can but it's not so nice to do dynamic_cast<const WinDivertHandle*>(&handle); 🤔
This has a built-in
std::bad_castthrow if the cast fails, which IMO is more descriptive thanstd::runtime_error?
I think throwing a custom exception with a custom message is nicer, no?
| PTF_ASSERT_TRUE(sendURLRequest("www.google.com")); | ||
| // let the capture work for couple of seconds | ||
| totalSleepTime = incSleep(capturedPackets, 2, 7); | ||
| totalSleepTime = incSleep(capturedPackets, 2, 20); |
There was a problem hiding this comment.
This test also failed in CI so I made it more robust
| parser.add_argument( | ||
| "--include-tests", | ||
| "-t", | ||
| type=str, | ||
| nargs="+", | ||
| default=[], | ||
| help="Pcap++ tests to include", | ||
| ) | ||
| parser.add_argument( |
There was a problem hiding this comment.
This change was needed to support running only WinDivert tests in the windivert job in CI
There was a problem hiding this comment.
Curious can't we write the entire script in powershell instead of having batch script that runs an embedded powershell script?
GH actions do support running a step directly in powershell.
There was a problem hiding this comment.
Our other scripts are batch scripts and not ps1 scripts... maybe it's better to convert the powershell part to batch, what do you think?
| virtual uint32_t close(const IWinDivertHandle* handle) = 0; | ||
| virtual uint32_t recvEx(const IWinDivertHandle* handle, uint8_t* buffer, uint32_t bufferLen, | ||
| size_t addressesSize, IOverlappedWrapper* overlapped) = 0; | ||
| virtual std::vector<WinDivertAddress> recvExComplete() = 0; | ||
| virtual uint32_t sendEx(const IWinDivertHandle* handle, uint8_t* buffer, uint32_t bufferLen, | ||
| size_t addressesSize) = 0; | ||
| virtual std::unique_ptr<IOverlappedWrapper> createOverlapped() = 0; | ||
| virtual bool getParam(const IWinDivertHandle* handle, WinDivertParam param, uint64_t& value) = 0; | ||
| virtual bool setParam(const IWinDivertHandle* handle, WinDivertParam param, uint64_t value) = 0; |
There was a problem hiding this comment.
ditto: Windivert handle pointer -> reference. The API is meaningless for nullptr handle.
There was a problem hiding this comment.
Also, can't those functions be member functions of the IWinDivertHandle wrapper instead? That way we don't have to dynamic cast inside this class's implementors.
There was a problem hiding this comment.
Expanding on this, I think restructuring IWinDivertImplementation and IWinDivertHandle would be beneficial for code performance and readability.
-
IWinDivertImplementationshould just be a stateless factory forIWinDivertHandleobjects.
MaybegetNetworkInterfacesshould be kept in there too, as that seems non-specific to the handles. The change would allow a single factory object to be shared between device instances. -
IWinDivertHandlechanges:- Operations directly applied to the handle should be transferred to the object (e.g.
recvEx,sendEx,setParam,getParam, etc...). This would simplify the interface and remove the need to downcast insideIWinDivertImplementation, which is slow. - The current member
IWinDivertImplementation::closeshould be removed. The handle would be closed by deleting theIWinDivertHandleobject in RAII pattern.
- Operations directly applied to the handle should be transferred to the object (e.g.
There was a problem hiding this comment.
The idea was to mimic the WinDivert interface as much as possible - IWinDivertHandle is an abstraction of WinDivert's HANDLE, and IWinDivertImplementation should be a very thin wrapper around WinDivert interface. I did it on purpose so mocking WinDivert will be straightforward and most of the business logic will be inside WinDivertDevice
There was a problem hiding this comment.
I see.
I think we can still keep mostly the same API structure even with the proposed redesign, no?
Won't be complete 1 to 1, but the main change would be the handle first param would be omitted from the signature.
My main issue is that the current design uses dynamic cast on every operation, which is unnecessary overhead and makes the thin wrapper heavier than it needs to be.
seladb
changed the title
| find_package(WinDivert REQUIRED) | ||
| if(NOT WinDivert_FOUND) | ||
| message(FATAL_ERROR "WinDivert not found!") | ||
| endif() |
There was a problem hiding this comment.
The required flag on find_package will make the process fail if WinDivert is missing. There is no need for the if not found branch.
This PR integrates WinDivert as another packet capture engine in PcapPlusPlus, enabling packet capture/injection on Windows via the WinDivert driver.
What is WinDivert
WinDivert is an open-source Windows library (kernel + user-mode) that allows applications to intercept, modify, drop or inject network packets traversing the Windows network stack. It is designed for use cases such as packet sniffing, firewalling, NAT-/VPN-tunneling, loopback traffic inspection, etc.
Key features include:
Project Links
Testing
This PR includes basic tests for the
WinDivertDevice. However, it also adds a lightweight abstraction over the WinDivert API using internal interfaces. It enables testingWinDivertDevicelogic without the real driver by providing mock implementations. These mock tests aren't implemented in this PR, but can be added later.