Automatically manage firewall rules (iptables) for services #1

darkgallium · 2020-06-28T17:22:32Z

darkgallium commented

2020-06-28 17:22:32 +00:00

We chose to use the iptables crate rather than the nftables one (nftnl) because it is much higher level and seems to be more mature/popular.

This adds a new firewall (fw) actor that handles event and a fw module that provides high level functions (setup, add port, cleanup).

Still TODO / needs to be discussed :

Do we need to cleanup iptables rules when diplonat is stopped ? Do we tackle this with tokio / unix signal handlers / shutdown_hooks ?
More global question : are we handling "delete service/port" events ? it doesn't seem to be done in the other actors, or am I mistaken ? :)

We chose to use the iptables crate rather than the nftables one (nftnl) because it is much higher level and seems to be more mature/popular. This adds a new firewall (fw) actor that handles event and a fw module that provides high level functions (setup, add port, cleanup). Still TODO / needs to be discussed : * Do we need to cleanup iptables rules when diplonat is stopped ? Do we tackle this with tokio / unix signal handlers / shutdown_hooks ? * More global question : are we handling "delete service/port" events ? it doesn't seem to be done in the other actors, or am I mistaken ? :)

quentin commented

2020-07-02 13:48:43 +00:00

Should we cleanup? We should avoid state if possible

So in my review I propose a bit agressive solutions: flushing entries on each notification. Of course, we should be careful to not break the connection if we do that, it is maybe a bad idea. But we can cleanup at process start. Cleaning on stopping is a bit hard as process may crash, I think it will be lot of burden for close to no gain

IGD ports are removed by reaching a timeout due to not renewing them

Considering IGD, ports are registered with a timeout: if registration is not renewed, the entry is garbage collected automatically by the router.
On each notification, the full sets of ports to be opened is sent, so by not renewing ports that are not required, the NAT entry on the router will naturally expire.

## Should we cleanup? We should avoid state if possible So in my review I propose a bit agressive solutions: flushing entries on each notification. Of course, we should be careful to not break the connection if we do that, it is maybe a bad idea. But we can cleanup at process start. Cleaning on stopping is a bit hard as process may crash, I think it will be lot of burden for close to no gain ## IGD ports are removed by reaching a timeout due to not renewing them Considering IGD, ports are registered with a timeout: if registration is not renewed, the entry is garbage collected automatically by the router. On each notification, the full sets of ports to be opened is sent, so by not renewing ports that are not required, the NAT entry on the router will naturally expire.

quentin requested changes 2020-07-02 14:29:47 +00:00

Dismissed

quentin left a comment

Nice job, some minor comments before the merge to make your code more homogenous with our coding style ;)

README.md

					
				@ -20,0 +24,4 @@

				  [...]

				  options {

				    docker.privileged.enabled = "true"

quentin commented

2020-07-02 14:20:53 +00:00

Uuuuh nicely spot, I did not think to it but it makes sense :)
Thanks for testing the whole pipeline!

Uuuuh nicely spot, I did not think to it but it makes sense :) Thanks for testing the whole pipeline!

darkgallium marked this conversation as resolved

src/fw.rs Outdated

					
				@ -0,0 +2,4 @@

				use regex::Regex;

				use std::collections::HashSet;

				use crate::messages;

quentin commented

2020-07-02 13:55:05 +00:00

Could you add the anyhow crate to handle errors please:

use anyhow::{Result,Context}

Could you add the anyhow crate to handle errors please: ```rust use anyhow::{Result,Context} ```

src/fw.rs Outdated

					
				@ -0,0 +12,4 @@

				    }

				}

				pub fn setup(ipt: &iptables::IPTables) -> Result<(), FirewallError> {

quentin commented

2020-07-02 13:56:52 +00:00

anyhow will override your Result<> object, taking only a return value, error will be generic then.
eg:

pub fn setup(ipt: &iptables::IPTables) -> Result<()> {

This tip applies for the whole document, and in any case build will fail as soon as you will have added the anyhow use statement.

anyhow will override your Result<> object, taking only a return value, error will be generic then. eg: ```rust pub fn setup(ipt: &iptables::IPTables) -> Result<()> { ``` This tip applies for the whole document, and in any case build will fail as soon as you will have added the anyhow use statement.

src/fw.rs Outdated

					
				@ -0,0 +14,4 @@

				pub fn setup(ipt: &iptables::IPTables) -> Result<(), FirewallError> {

				    if !ipt.chain_exists("filter", "DIPLONAT")? {

quentin commented

2020-07-02 14:17:23 +00:00

It's very clever to put the rules in a separate chain, well done ;)

src/fw.rs Outdated

					
				@ -0,0 +23,4 @@

				    Ok(())

				}

				pub fn open_ports(ipt: &iptables::IPTables, ports: messages::PublicExposedPorts) -> Result<(), FirewallError> {

quentin commented

2020-07-02 14:03:44 +00:00

Same as before, you can just use Result<()>

Same as before, you can just use `Result<()>`

src/fw.rs Outdated

					
				@ -0,0 +35,4 @@

				    Ok(())

				}

				pub fn get_opened_ports(ipt: &iptables::IPTables) -> Result<messages::PublicExposedPorts, FirewallError> {

quentin commented

2020-07-02 14:04:47 +00:00

Same as before, you can just use Result<messages::PublicExposedPorts>

Same as before, you can just use `Result<messages::PublicExposedPorts>`

src/fw.rs Outdated

					
				@ -0,0 +42,4 @@

				    };

				    let list = ipt.list("filter", "DIPLONAT")?;

				    let re = Regex::new(r"-A.*? -p (w+).*--dport (d+).*?-j ACCEPT").unwrap();

quentin commented

2020-07-02 14:02:48 +00:00

A convention we chose with LX (we should document it, mea culpa) is to use the anyhow crate and to handle errors without crashing whenever possible but forwarding it "higher" in the call stack.

The idea is the caller can choose to crash or to bypass the error and then your code can be used as a handful library. So, as your return type is already a Result, you can return Ok() or, thanks to anyhow, any kind of error. You can simply rewrite your statement as follow (as soon as you have added anyhow):

let re = Regex::new(r"-A.*? -p (w+).*--dport (d+).*?-j ACCEPT")?

To help debugging, you can add a context to your error:

let re = Regex::new(r"-A.*? -p (w+).*--dport (d+).*?-j ACCEPT").context("Regex matching open ports encountered an unexpected rule")?

As a more general rule, do not use:

expect
unwrap

Instead use:

?
context
handle error in place if it's possible

A convention we chose with LX (we should document it, mea culpa) is to use the anyhow crate and to handle errors without crashing whenever possible but forwarding it "higher" in the call stack. The idea is the caller can choose to crash or to bypass the error and then your code can be used as a handful library. So, as your return type is already a `Result`, you can return `Ok()` or, thanks to anyhow, any kind of error. You can simply rewrite your statement as follow (as soon as you have added anyhow): ```rust let re = Regex::new(r"-A.*? -p (w+).*--dport (d+).*?-j ACCEPT")? ``` To help debugging, you can add a context to your error: ```rust let re = Regex::new(r"-A.*? -p (w+).*--dport (d+).*?-j ACCEPT").context("Regex matching open ports encountered an unexpected rule")? ``` As a more general rule, do not use: - `expect` - `unwrap` Instead use: - `?` - `context` - handle error in place if it's possible

src/fw.rs Outdated

					
				@ -0,0 +47,4 @@

				        let caps = re.captures(&i);

				        match caps {

				            Some(c) => {

				                let raw_proto = c.get(1).unwrap();

quentin commented

2020-07-02 14:08:08 +00:00

Lines 50, 51 and 54 you can replace unwrap by ?

Lines 50, 51 and 54 you can replace `unwrap` by `?`

src/fw.rs Outdated

					
				@ -0,0 +67,4 @@

				    Ok(ports)

				}

				pub fn cleanup(ipt: &iptables::IPTables) -> Result<(), FirewallError> {

quentin commented

2020-07-02 14:05:21 +00:00

Same as before, you can just use Result<()>

Moreover, I see that you never call this function.
What about calling it on the setup, so if the app reboots at least it will start with a clean state?
Even more agressive, could we not just simply regenerates the whole chain on every change?
So, no difference to compute or ports to close?

I believe that on each modification to iptables, you have to flush the whole set of rules,
so if we could do a single "commit" to the kernel with a brand new chain it could be great.

In practise:

If it's two lines to change, it's easy and takes 5 minutes, you can do the modification
Otherwise, let's keep ip for a next version

Same as before, you can just use `Result<()>` --- Moreover, I see that you never call this function. What about calling it on the setup, so if the app reboots at least it will start with a clean state? Even more agressive, could we not just simply regenerates the whole chain on every change? So, no difference to compute or ports to close? I believe that on each modification to iptables, you have to flush the whole set of rules, so if we could do a single "commit" to the kernel with a brand new chain it could be great. In practise: - If it's two lines to change, it's easy and takes 5 minutes, you can do the modification - Otherwise, let's keep ip for a next version

src/fw_actor.rs Outdated

					
				@ -0,0 +23,4 @@

				impl FirewallActor {

				  pub async fn new(_refresh: Duration, rxp: &watch::Receiver<messages::PublicExposedPorts>) -> Result<Self> {

				    let ctx = Self { 

				      ipt: iptables::new(false).unwrap(),

quentin commented

2020-07-02 14:08:48 +00:00

Replace with ?

Replace with `?`

src/fw_actor.rs Outdated

					
				@ -0,0 +29,4 @@

				      refresh: _refresh,

				    };

				    fw::setup(&ctx.ipt).expect("iptables setup failed");

quentin commented

2020-07-02 14:09:24 +00:00

Here also you can put a ? :)

Here also you can put a `?` :)

src/fw_actor.rs

					
				@ -0,0 +50,4 @@

				      // 3. Update firewall rules

				      match self.do_fw_update().await {

				        Ok(()) => debug!("Successfully updated firewall rules"),

				        Err(e) => error!("An error occured while updating firewall rules. {}", e),

quentin commented

2020-07-02 14:16:20 +00:00

By putting ? everywhere, we can handle all errors here.
Currently, this line can't be called as there is no way that do_fw_update returns an Err as inside the function, you never returns an Err as unwrap makes the app panic ;)

By putting `?` everywhere, we can handle all errors here. Currently, this line can't be called as there is no way that `do_fw_update` returns an `Err` as inside the function, you never returns an `Err` as `unwrap` makes the app panic ;)

darkgallium marked this conversation as resolved

src/fw_actor.rs Outdated

					
				@ -0,0 +56,4 @@

				  }

				  pub async fn do_fw_update(&self) -> Result<()> {

				    let curr_opened_ports = fw::get_opened_ports(&self.ipt).unwrap();

quentin commented

2020-07-02 14:12:53 +00:00

You should replace unwrap ;)

You should replace `unwrap` ;)

src/fw_actor.rs

					
				@ -0,0 +58,4 @@

				  pub async fn do_fw_update(&self) -> Result<()> {

				    let curr_opened_ports = fw::get_opened_ports(&self.ipt).unwrap();

				    let diff_tcp = self.last_ports.tcp_ports.difference(&curr_opened_ports.tcp_ports).copied().collect::<HashSet<u16>>();

quentin commented

2020-07-02 14:20:10 +00:00

Nice idea the set difference ;)

darkgallium commented

2020-07-04 15:07:11 +00:00

Thanks a lot :)

quentin marked this conversation as resolved

src/fw_actor.rs

					
				@ -0,0 +65,4 @@

				        tcp_ports: diff_tcp,

				        udp_ports: diff_udp

				    };

quentin commented

2020-07-02 14:19:25 +00:00

I see, maybe we could add later logic to close ports but I also like the idea to do the things incrementally.
So let's focus on stabilizing the current features and see how it runs in production :)

I see, maybe we could add later logic to close ports but I also like the idea to do the things incrementally. So let's focus on stabilizing the current features and see how it runs in production :)

quentin marked this conversation as resolved

src/fw_actor.rs Outdated

					
				@ -0,0 +66,4 @@

				        udp_ports: diff_udp

				    };

				    fw::open_ports(&self.ipt, ports_to_open).unwrap();

quentin commented

2020-07-02 14:13:05 +00:00

Here also ;)

darkgallium commented

2020-07-04 15:06:50 +00:00

So in my review I propose a bit agressive solutions: flushing entries on each notification. Of course, we should be careful to not break the connection if we do that, it is maybe a bad idea.

I'm a little concerned about flushing entries on notification for the exact reason you are mentioning. I'd rather test it in another version as you are suggesting :)

But we can cleanup at process start. Cleaning on stopping is a bit hard as process may crash, I think it will be lot of burden for close to no gain

Agreed. Cleaning up on start is much more reliable than cleaning up on stop. I added that to the code.

> So in my review I propose a bit agressive solutions: flushing entries on each notification. Of course, we should be careful to not break the connection if we do that, it is maybe a bad idea. I'm a little concerned about flushing entries on notification for the exact reason you are mentioning. I'd rather test it in another version as you are suggesting :) > But we can cleanup at process start. Cleaning on stopping is a bit hard as process may crash, I think it will be lot of burden for close to no gain Agreed. Cleaning up on start is much more reliable than cleaning up on stop. I added that to the code.

quentin referenced this pull request from a commit

2020-07-04 15:16:23 +00:00

Merge pull request 'Automatically manage firewall rules (iptables) for services' (#1) from add-firewall-rules into master Reviewed-on: https://git.deuxfleurs.fr/Deuxfleurs/diplonat/pulls/1

quentin merged commit 7ec74a21d4 into master

2020-07-04 15:16:23 +00:00

quentin deleted branch add-firewall-rules

2020-07-04 15:16:37 +00:00

Sign in to join this conversation.

No reviewers