Automatically manage firewall rules (iptables) for services #1
67
src/fw.rs
|
@ -2,61 +2,58 @@ use iptables;
|
|||
use regex::Regex;
|
||||
use std::collections::HashSet;
|
||||
use crate::messages;
|
||||
use anyhow::{Result,Context};
|
||||
|
||||
use log::*;
|
||||
|
||||
#[derive(Debug)]
|
||||
pub struct FirewallError(String);
|
||||
pub fn setup(ipt: &iptables::IPTables) -> Result<()> {
|
||||
|
||||
impl From<iptables::error::IPTError> for FirewallError {
|
||||
fn from(error: iptables::error::IPTError) -> Self {
|
||||
FirewallError(error.to_string())
|
||||
}
|
||||
}
|
||||
|
||||
pub fn setup(ipt: &iptables::IPTables) -> Result<(), FirewallError> {
|
||||
|
||||
if !ipt.chain_exists("filter", "DIPLONAT")? {
|
||||
ipt.new_chain("filter", "DIPLONAT")?;
|
||||
}
|
||||
|
||||
ipt.insert_unique("filter", "INPUT", "-j DIPLONAT", 1)?;
|
||||
// ensure we start from a clean state without any rule already set
|
||||
cleanup(ipt)?;
|
||||
|
||||
ipt.new_chain("filter", "DIPLONAT").context("Failed to create new chain")?;
|
||||
ipt.insert_unique("filter", "INPUT", "-j DIPLONAT", 1).context("Failed to insert jump rule")?;
|
||||
|
||||
quentin
commented
anyhow will override your Result<> object, taking only a return value, error will be generic then.
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.
|
||||
Ok(())
|
||||
}
|
||||
quentin
commented
It's very clever to put the rules in a separate chain, well done ;) It's very clever to put the rules in a separate chain, well done ;)
|
||||
|
||||
pub fn open_ports(ipt: &iptables::IPTables, ports: messages::PublicExposedPorts) -> Result<(), FirewallError> {
|
||||
pub fn open_ports(ipt: &iptables::IPTables, ports: messages::PublicExposedPorts) -> Result<()> {
|
||||
for p in ports.tcp_ports {
|
||||
ipt.append("filter", "DIPLONAT", &format!("-p tcp --dport {} -j ACCEPT", p))?;
|
||||
ipt.append("filter", "DIPLONAT", &format!("-p tcp --dport {} -j ACCEPT", p)).context("Failed to insert port rule")?;
|
||||
}
|
||||
|
||||
for p in ports.udp_ports {
|
||||
ipt.append("filter", "DIPLONAT", &format!("-p udp --dport {} -j ACCEPT", p))?;
|
||||
ipt.append("filter", "DIPLONAT", &format!("-p udp --dport {} -j ACCEPT", p)).context("Failed to insert port rule")?;
|
||||
}
|
||||
quentin
commented
Same as before, you can just use Same as before, you can just use `Result<()>`
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
pub fn get_opened_ports(ipt: &iptables::IPTables) -> Result<messages::PublicExposedPorts, FirewallError> {
|
||||
pub fn get_opened_ports(ipt: &iptables::IPTables) -> Result<messages::PublicExposedPorts> {
|
||||
let mut ports = messages::PublicExposedPorts {
|
||||
tcp_ports: HashSet::new(),
|
||||
udp_ports: HashSet::new()
|
||||
};
|
||||
|
||||
let list = ipt.list("filter", "DIPLONAT")?;
|
||||
let re = Regex::new(r"\-A.*? \-p (\w+).*\-\-dport (\d+).*?\-j ACCEPT").unwrap();
|
||||
let re = Regex::new(r"\-A.*? \-p (\w+).*\-\-dport (\d+).*?\-j ACCEPT").context("Regex matching open ports encountered an unexpected rule")?;
|
||||
quentin
commented
Same as before, you can just use Same as before, you can just use `Result<messages::PublicExposedPorts>`
|
||||
for i in list {
|
||||
let caps = re.captures(&i);
|
||||
match caps {
|
||||
Some(c) => {
|
||||
let raw_proto = c.get(1).unwrap();
|
||||
let raw_port = c.get(2).unwrap();
|
||||
|
||||
if let (Some(raw_proto), Some(raw_port)) = (c.get(1), c.get(2)) {
|
||||
|
||||
quentin
commented
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
To help debugging, you can add a context to your error:
As a more general rule, do not use:
Instead use:
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
|
||||
let proto = String::from(raw_proto.as_str());
|
||||
let number = String::from(raw_port.as_str()).parse::<u16>().unwrap();
|
||||
let proto = String::from(raw_proto.as_str());
|
||||
let number = String::from(raw_port.as_str()).parse::<u16>()?;
|
||||
|
||||
if proto == "tcp" {
|
||||
ports.tcp_ports.insert(number);
|
||||
quentin
commented
Lines 50, 51 and 54 you can replace Lines 50, 51 and 54 you can replace `unwrap` by `?`
|
||||
} else {
|
||||
ports.udp_ports.insert(number);
|
||||
}
|
||||
|
||||
if proto == "tcp" {
|
||||
ports.tcp_ports.insert(number);
|
||||
} else {
|
||||
ports.udp_ports.insert(number);
|
||||
error!("Unexpected rule found in DIPLONAT chain")
|
||||
}
|
||||
|
||||
},
|
||||
|
@ -67,10 +64,18 @@ pub fn get_opened_ports(ipt: &iptables::IPTables) -> Result<messages::PublicExpo
|
|||
Ok(ports)
|
||||
}
|
||||
|
||||
pub fn cleanup(ipt: &iptables::IPTables) -> Result<(), FirewallError> {
|
||||
ipt.flush_chain("filter", "DIPLONAT")?;
|
||||
ipt.delete("filter", "INPUT", "-j DIPLONAT")?;
|
||||
ipt.delete_chain("filter", "DIPLONAT")?;
|
||||
pub fn cleanup(ipt: &iptables::IPTables) -> Result<()> {
|
||||
|
||||
if ipt.chain_exists("filter", "DIPLONAT")? {
|
||||
ipt.flush_chain("filter", "DIPLONAT").context("Failed to flush the DIPLONAT chain")?;
|
||||
quentin
commented
Same as before, you can just use Moreover, I see that you never call this function. I believe that on each modification to iptables, you have to flush the whole set of rules, In practise:
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
|
||||
|
||||
if ipt.exists("filter", "INPUT", "-j DIPLONAT")? {
|
||||
ipt.delete("filter", "INPUT", "-j DIPLONAT").context("Failed to delete jump rule")?;
|
||||
}
|
||||
|
||||
ipt.delete_chain("filter", "DIPLONAT").context("Failed to delete chain")?;
|
||||
}
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
|
|
|
@ -23,13 +23,13 @@ pub struct FirewallActor {
|
|||
impl FirewallActor {
|
||||
pub async fn new(_refresh: Duration, rxp: &watch::Receiver<messages::PublicExposedPorts>) -> Result<Self> {
|
||||
let ctx = Self {
|
||||
ipt: iptables::new(false).unwrap(),
|
||||
ipt: iptables::new(false)?,
|
||||
quentin
commented
Replace with Replace with `?`
|
||||
rx_ports: rxp.clone(),
|
||||
last_ports: messages::PublicExposedPorts::new(),
|
||||
refresh: _refresh,
|
||||
};
|
||||
|
||||
fw::setup(&ctx.ipt).expect("iptables setup failed");
|
||||
fw::setup(&ctx.ipt)?;
|
||||
quentin
commented
Here also you can put a Here also you can put a `?` :)
|
||||
|
||||
return Ok(ctx);
|
||||
}
|
||||
|
@ -56,7 +56,7 @@ impl FirewallActor {
|
|||
}
|
||||
|
||||
pub async fn do_fw_update(&self) -> Result<()> {
|
||||
let curr_opened_ports = fw::get_opened_ports(&self.ipt).unwrap();
|
||||
let curr_opened_ports = fw::get_opened_ports(&self.ipt)?;
|
||||
quentin
commented
You should replace You should replace `unwrap` ;)
|
||||
|
||||
let diff_tcp = self.last_ports.tcp_ports.difference(&curr_opened_ports.tcp_ports).copied().collect::<HashSet<u16>>();
|
||||
quentin marked this conversation as resolved
quentin
commented
Nice idea the set difference ;) Nice idea the set difference ;)
darkgallium
commented
Thanks a lot :) Thanks a lot :)
|
||||
let diff_udp = self.last_ports.udp_ports.difference(&curr_opened_ports.udp_ports).copied().collect::<HashSet<u16>>();
|
||||
|
@ -66,7 +66,7 @@ impl FirewallActor {
|
|||
udp_ports: diff_udp
|
||||
};
|
||||
|
||||
quentin marked this conversation as resolved
quentin
commented
I see, maybe we could add later logic to close ports but I also like the idea to do the things incrementally. 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 :)
|
||||
fw::open_ports(&self.ipt, ports_to_open).unwrap();
|
||||
fw::open_ports(&self.ipt, ports_to_open)?;
|
||||
quentin
commented
Here also ;) Here also ;)
|
||||
|
||||
return Ok(());
|
||||
}
|
||||
|
|
Could you add the anyhow crate to handle errors please: