FEAT: Add port close command feature and improve error handling #21
Conversation
This commit implements the port close command feature that allows users to close previously opened ports. Changes include: - Add close command implementation in client side - Implement close_port method in firewall implementation - Update protocol handling to support Close operation - Add robust error handling for edge cases - Implement detailed debug logging for better diagnostics - Update test scripts to verify close command functionality with both IPv4 and IPv6 - Fix assertions to accept both Knock and Close operations The implementation follows the same security patterns as the knock command with proper authentication.
| .windsurfrules | ||
| /.cursorrules |
| @@ -676,7 +684,7 @@ mod tests { | |||
| fn test_msg_raw_invalid_operation() { | |||
There was a problem hiding this comment.
Please also add a new test case for Close similar to test_msg_knock.
| @@ -0,0 +1,225 @@ | |||
| // -*- coding: utf-8 -*- | |||
There was a problem hiding this comment.
I am wondering, if we can unify the close and knock implementations on the client side and create one common "Protocol sequence" implementation for both.
They are basically identical. The only difference should only be the Operation::Close.
| @@ -70,6 +71,89 @@ fn parse_user(s: &str) -> ah::Result<UserId> { | |||
|
|
|||
| #[derive(Subcommand, Debug)] | |||
| enum Command { | |||
| /// Close a previously opened port on a server. | |||
| Close { | |||
There was a problem hiding this comment.
Can we avoid the duplication of the struct content?
| // Receive the initial knock message. | ||
| let knock = self.recv_msg(Operation::Knock).await?; | ||
| // Receive the initial message (Knock or Close). | ||
| let initial_msg = match timeout(self.conf.control_timeout(), self.conn.recv_msg()) |
There was a problem hiding this comment.
Please change self.recv_msg instead of open-coding the timeout and expected operation check here.
We could use a reference to a slice of expected operations as a parameter to self.recv_msg.
| println!("firewall: Looking for rule with comment: '{}'", comment); | ||
| println!("firewall: Rules found in kernel:"); | ||
| let mut found_any = false; | ||
|
|
||
| for obj in &*self.objs { |
There was a problem hiding this comment.
These changes look like debug-only, right? If so, please remove these changes.
| Ok(ruleset) => ruleset, | ||
| Err(e) => { | ||
| // In debug mode, handle the error gracefully | ||
| if conf.debug() { |
There was a problem hiding this comment.
This breaks backward compatibility. Debug mode is not supposed to change behavior.
If there is a good reason to ignore some errors, these cases should be explicitly coded and documented. It shall be preferred to not be dependent on the configuration whether errors are ignored or not at this level.
| if let Some(lease) = self.leases.remove(&id) { | ||
| println!("firewall: Found lease in memory for {remote_addr} port {port}, attempting to remove from kernel"); | ||
| // Lease exists, remove it from the kernel | ||
| let leases = vec![lease]; |
There was a problem hiding this comment.
This doesn't need to be a heap allocating vec.
| } else { | ||
| println!("firewall: No lease found in memory for {remote_addr} port {port}"); | ||
|
|
||
| // Try to remove from kernel anyway |
| @@ -136,6 +136,60 @@ impl FirewallConnection { | |||
| self.send_msg(&FirewallMessage::new_nack()).await?; | |||
| } | |||
| } | |||
| FirewallOperation::Close => { | |||
There was a problem hiding this comment.
This looks like Open and Close could share a lot of code.
|
Hi :) |
This commit implements the port close command feature that allows users to close previously opened ports. Changes include:
The implementation follows the same security patterns as the knock command with proper authentication.