Deuxfleurs/diplonat
23
4
Fork 1
Code Issues 8 Pull requests Releases Wiki Activity

Automatically manage firewall rules (iptables) for services #1

Merged
quentin merged 7 commits from add-firewall-rules into master 2020-07-04 15:16:23 +00:00
Conversation 2 Commits 7 Files changed 10 +228 -2
3 changed files with 228 additions and 2 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files
Showing only changes of commit 1a9199011d - Show all commits

126
Cargo.lock generated
View file

@ -127,7 +127,10 @@ dependencies = [
"anyhow",
"futures",
"igd",
"libc",
"log",
"mnl",
"nftnl",
"pretty_env_logger",
"reqwest",
"serde",
@ -164,6 +167,20 @@ dependencies = [
"termcolor",
]
[[package]]
name = "err-derive"
version = "0.2.4"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "22deed3a8124cff5fa835713fa105621e43bbdc46690c3a6b68328a012d350d4"
dependencies = [
"proc-macro-error",
"proc-macro2",
"quote",
"rustversion",
"syn",
"synstructure",
]
[[package]]
name = "fnv"
version = "1.0.6"
@ -548,9 +565,9 @@ dependencies = [
[[package]]
name = "libc"
version = "0.2.66"
version = "0.2.70"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "d515b1f41455adea1313a4a2ac8a8a477634fbae63cc6100e3aebb207ce61558"
checksum = "3baa92041a6fec78c687fa0cc2b3fae8884f743d672cf551bed1d6dac6988d0f"
[[package]]
name = "log"
@ -620,6 +637,27 @@ dependencies = [
"ws2_32-sys",
]
[[package]]
name = "mnl"
version = "0.2.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "e6efb50a48dbacd112e7e847b9847fa530ec4a473ba6322a2f82c42ef333e226"
dependencies = [
"libc",
"log",
"mnl-sys",
]
[[package]]
name = "mnl-sys"
version = "0.2.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "5302035599c722b3a5b92a6502ff73c501dc6d100c53b89f0fae0cb932a37122"
dependencies = [
"libc",
"pkg-config",
]
[[package]]
name = "native-tls"
version = "0.2.3"
@ -649,6 +687,30 @@ dependencies = [
"winapi 0.3.8",
]
[[package]]
name = "nftnl"
version = "0.3.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "f3727d1e8c1c9af88857f46539c3030693158a2a7586056b8cab6ded523bf7aa"
dependencies = [
"bitflags",
"err-derive",
"libc",
"log",
"nftnl-sys",
]
[[package]]
name = "nftnl-sys"
version = "0.3.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "dba134c9b125b7d7c13d813388aaeb2aeeba60fb1eb702799163fb845086ca33"
dependencies = [
"cfg-if",
"libc",
"pkg-config",
]
[[package]]
name = "nom"
version = "4.2.3"
@ -764,6 +826,32 @@ dependencies = [
"log",
]
[[package]]
name = "proc-macro-error"
version = "1.0.2"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "98e9e4b82e0ef281812565ea4751049f1bdcdfccda7d3f459f2e138a40c08678"
dependencies = [
"proc-macro-error-attr",
"proc-macro2",
"quote",
"syn",
"version_check 0.9.1",
]
[[package]]
name = "proc-macro-error-attr"
version = "1.0.2"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "4f5444ead4e9935abd7f27dc51f7e852a0569ac888096d5ec2499470794e2e53"
dependencies = [
"proc-macro2",
"quote",
"syn",
"syn-mid",
"version_check 0.9.1",
]
[[package]]
name = "proc-macro-hack"
version = "0.5.11"
@ -952,6 +1040,17 @@ dependencies = [
"winreg",
]
[[package]]
name = "rustversion"
version = "1.0.2"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "b3bba175698996010c4f6dce5e7f173b6eb781fce25d2cfc45e27091ce0b79f6"
dependencies = [
"proc-macro2",
"quote",
"syn",
]
[[package]]
name = "ryu"
version = "1.0.2"
@ -1071,6 +1170,29 @@ dependencies = [
"unicode-xid",
]
[[package]]
name = "syn-mid"
version = "0.5.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "7be3539f6c128a931cf19dcee741c1af532c7fd387baa739c03dd2e96479338a"
dependencies = [
"proc-macro2",
"quote",
"syn",
]
[[package]]
name = "synstructure"
version = "0.12.3"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "67656ea1dc1b41b1451851562ea232ec2e5a80242139f7e679ceccfb5d61f545"
dependencies = [
"proc-macro2",
"quote",
"syn",
"unicode-xid",
]
[[package]]
name = "tempfile"
version = "3.1.0"

3
Cargo.toml
View file

@ -17,3 +17,6 @@ serde = { version = "1.0.107", features = ["derive"] }
serde_json = "1.0.53"
serde-lexpr = "0.1.1"
anyhow = "1.0.28"
nftnl = "0.3.0"
mnl = "0.2.0"
libc = "0.2.70"

101
src/fw.rs Normal file
View file

@ -0,0 +1,101 @@
use nftnl::{nft_expr, Batch, Chain, FinalizedBatch, ProtoFamily, Rule, Table};
use std::{
ffi::{self, CString},
io,
};
quentin commented 2020-07-02 13:55:05 +00:00
Outdated
Review
Copy link

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} ```
use mnl;
use libc;
const TABLE_NAME: &str = "diplonat";
const OUT_CHAIN_NAME: &str = "out";
const IN_CHAIN_NAME: &str = "in";
#[derive(Debug)]
struct Error(String);
quentin commented 2020-07-02 13:56:52 +00:00
Outdated
Review
Copy link

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.
impl From<io::Error> for Error {
fn from(error: io::Error) -> Self {
quentin commented 2020-07-02 14:17:23 +00:00
Outdated
Review
Copy link

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 ;)
Error(error.to_string())
}
}
impl From<ffi::NulError> for Error {
fn from(error: ffi::NulError) -> Self {
Error(error.to_string())
}
}
quentin commented 2020-07-02 14:03:44 +00:00
Outdated
Review
Copy link

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

Same as before, you can just use `Result<()>`
fn send_and_process(batch: &FinalizedBatch) -> Result<(), Error> {
let socket = mnl::Socket::new(mnl::Bus::Netfilter)?;
socket.send_all(batch)?;
let portid = socket.portid();
let mut buffer = vec![0; nftnl::nft_nlmsg_maxsize() as usize];
while let Some(message) = socket_recv(&socket, &mut buffer[..])? {
match mnl::cb_run(message, 2, portid)? {
mnl::CbResult::Stop => {
break;
quentin commented 2020-07-02 14:04:47 +00:00
Outdated
Review
Copy link

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

Same as before, you can just use `Result<messages::PublicExposedPorts>`
}
mnl::CbResult::Ok => (),
}
}
Ok(())
}
quentin commented 2020-07-02 14:02:48 +00:00
Outdated
Review
Copy link

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
fn socket_recv<'a>(socket: &mnl::Socket, buf: &'a mut [u8]) -> Result<Option<&'a [u8]>, Error> {
let ret = socket.recv(buf)?;
if ret > 0 {
Ok(Some(&buf[..ret]))
} else {
quentin commented 2020-07-02 14:08:08 +00:00
Outdated
Review
Copy link

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

Lines 50, 51 and 54 you can replace `unwrap` by `?`
Ok(None)
}
}
fn add_port_allowed(port: u16) -> Result<(), Error> {
let mut batch = Batch::new();
// TODO: at the moment, I haven't found a way to properly separate setup part (create table and
// chains) and the add rule part because the add rule part needs a reference on the chains.
// apparently creating a table and chains that already exist seems to do nothing so it works
// doing the following. To be done properly though
let table = Table::new(&CString::new(TABLE_NAME).unwrap(), ProtoFamily::Inet);
batch.add(&table, nftnl::MsgType::Add);
let mut out_chain = Chain::new(&CString::new(OUT_CHAIN_NAME).unwrap(), &table);
let mut in_chain = Chain::new(&CString::new(IN_CHAIN_NAME).unwrap(), &table);
out_chain.set_hook(nftnl::Hook::Out, 0);
quentin commented 2020-07-02 14:05:21 +00:00
Outdated
Review
Copy link

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
in_chain.set_hook(nftnl::Hook::In, 0);
out_chain.set_policy(nftnl::Policy::Accept);
in_chain.set_policy(nftnl::Policy::Drop);
batch.add(&out_chain, nftnl::MsgType::Add);
batch.add(&in_chain, nftnl::MsgType::Add);
let mut _rule = Rule::new(&in_chain);
_rule.add_expr(&nft_expr!(meta nfproto));
_rule.add_expr(&nft_expr!(cmp == libc::NFPROTO_IPV4 as u8));
_rule.add_expr(&nft_expr!(meta l4proto));
_rule.add_expr(&nft_expr!(cmp == libc::IPPROTO_TCP as u8));
_rule.add_expr(&nftnl::expr::Payload::Transport(
nftnl::expr::TransportHeaderField::Tcp(nftnl::expr::TcpHeaderField::Dport),
));
_rule.add_expr(&nft_expr!(cmp == port.to_be()));
_rule.add_expr(&nft_expr!(verdict accept));
batch.add(&_rule, nftnl::MsgType::Add);
let finalized_batch = batch.finalize();
send_and_process(&finalized_batch)?;
Ok(())
}