Skip to content

Conversation

@Icegrave0391
Copy link
Contributor

  • Use wintun to implement IPInterfaceProvider
  • Added testcases

@Icegrave0391
Copy link
Contributor Author

It seems that cargo clippy fails due to the source code of the used package wintun.

@Icegrave0391
Copy link
Contributor Author

@jaybosamiya-ms I fixed the CI by only including the dependency into target=Windows

Copy link
Member

@jaybosamiya-ms jaybosamiya-ms left a comment

Choose a reason for hiding this comment

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

Thanks for working on this, I've added a few comments.

Comment on lines +121 to +122
/// WinTun session for networking
wintun_session: std::sync::RwLock<Option<Arc<wintun::Session>>>,
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure I understand why this is an RwLock<Option<Arc<. Is this solely because of the exit handling?

Ok(adapter) => adapter,
Err(_) => {
// If opening failed (adapter doesn't exist), create a new one
wintun::Adapter::create(&wintun, adapter_name, "LiteBox", None)
Copy link
Member

Choose a reason for hiding this comment

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

Not sure why the tunner type is LiteBox here, please include a comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Comment on lines 604 to 606
if packet_bytes.len() != packet.len() {
unimplemented!("unexpected size {}", packet_bytes.len())
}
Copy link
Member

Choose a reason for hiding this comment

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

I think this should just be an assert_eq!(..., ...); unless you have good reason to believe that the two might differ

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

// Try to receive a packet from WinTun (non-blocking)
session
.try_receive()
.map_err(|_| litebox::platform::ReceiveError::WouldBlock)
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is correct; the errors that can happen here are not a "this would block" but instead these: https://docs.rs/wintun/latest/wintun/enum.Error.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think currently we only have one WouldBlock error in litebox::platform::ReceiveError. We may want to add more typt to the platform's error

Copy link
Member

Choose a reason for hiding this comment

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

For now, you can just throw in that case

Comment on lines 1402 to 1429

#[test]
fn test_ip_interface_without_wintun() {
// Test IP interface functionality when WinTun is not enabled
let platform = WindowsUserland::new(None);

// Create a sample IP packet (minimal IPv4 header)
let mut test_packet = [0u8; 64];
test_packet[0] = 0x45; // Version 4, Header length 20 bytes
test_packet[9] = 0x06; // Protocol: TCP

// Test send_ip_packet without WinTun - should panic with unimplemented
let result = std::panic::catch_unwind(|| platform.send_ip_packet(&test_packet));
assert!(
result.is_err(),
"send_ip_packet should panic when WinTun is not available"
);

// Test receive_ip_packet without WinTun - should panic with unimplemented
let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| {
let mut receive_buffer = [0u8; 1500];
platform.receive_ip_packet(&mut receive_buffer)
}));
assert!(
result.is_err(),
"receive_ip_packet should panic when WinTun is not available"
);
}
Copy link
Member

Choose a reason for hiding this comment

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

I don't see how this negative test is particularly helpful. In the future, if we ever did stuff without wintun, then this test would fail, but that would not be a bad thing, no? Probably can be deleted

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

}

#[test]
#[ignore] // This test requires Administrator privileges and WinTun driver to be installed
Copy link
Member

Choose a reason for hiding this comment

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

Does this actually fail on CI? I'm not fully sure how it helps to add a whole bunch of ignored tests. You can see the Linux one we actually do extra things on the CI to make sure we can run the CI tests. Is that possible here?

@github-actions
Copy link

🤖 SemverChecks 🤖 No breaking API changes detected

Note: this does not mean API is unchanged, or even that there are no breaking changes; simply, none of the detections triggered.

@jaybosamiya-ms jaybosamiya-ms marked this pull request as draft October 8, 2025 00:58
@CvvT
Copy link
Contributor

CvvT commented Oct 30, 2025

It looks like wintun requires admin privileges to create an adapter and it's per-process. I don't think we'd like to let untrusted code run with admin privileges. We could create a service as a proxy with admin privilege and then LiteBox can communicate with it via IPC. Alternatively, we could use tap on Windows, which is cleaner IMO. Also, we have discussed switching to macvlan + tap for Linux. I will try to see how much work we need for using tap.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants