Code: Firewall filter #416

markmandel · 2021-10-07T21:21:58Z

What type of PR is this?

Uncomment only one /kind <> line, press enter to put that in a new line, and remove leading whitespace from that line:

/kind breaking
/kind bug
/kind cleanup
/kind documentation

/kind feature

/kind hotfix

What this PR does / Why we need it:

Code implementation of a Firewall filter that will allow/deny packets based on their from address on both read and write.

Which issue(s) this PR fixes:

Work on #158
Work on #343

Special notes for your reviewer:

Documentation to come next to finish off the above two tickets.

Also have some questions on a couple of places that I think would be good to get confirmation on.

markmandel · 2021-10-07T21:23:02Z

/cc @davidkornel who might be interested.

markmandel · 2021-10-07T21:25:05Z

src/filters/firewall.rs

+            }
+        }
+
+        debug!(self.log, "default: Deny"; "event" => "read", "from" => ctx.from.to_string());


So if no rules match, I default to not allowing a packet through. Is this acceptable?

I see a few options:

This is fine

If someone wants to allow all through by default, they can use an "everything" cidr range to do so.

We add some sort of "default" allow/deny option to the config for on_read and on_write.

I'm happy to leave things as is for the time being, and see if anyone has a preference, but figured I would highlight this and ask the question.

What's the behaviour of other firewall projects here, what happens in Envoy, firewalld, iptables, etc? I think that will probably give us an answer.

thinking denying by default could be fine depending on what guarantees we're providing which we should mention in the docs - if we guarantee that we'll validate according to the rules list order then yeah it should be fine and they can have a catch all range at the end of the list.
If we don't guarantee the order rules are checked against (which would enable us to optimizations to e.g support when we have lots of rules and want to avoid scanning) then having an explicit 'default allow/deny' option might be preferrable.

Coming to think of it (probably not for this PR), we should also document the behavior when a user specifies conflicting port ranges in different rules e.g allow 10-20 in one rule and deny 20-30 in another - e.g we could treat the config as invalid

I'm struggling to find an example with Envoy where it will do firewall filtering (I'm not sure it can? but it's entirely possible I'm lost in it's docs), but i did find it in Istio:
https://istio.io/latest/docs/reference/config/security/authorization-policy/ which has an interesting formula of decision making, but if nothing is applied, it does deny in the end (I personally think that checking and matching in the order configured is easier to understand, but maybe that's just me).

Authorization policy supports CUSTOM, DENY and ALLOW actions for access control. When CUSTOM, DENY and ALLOW actions are used for a workload at the same time, the CUSTOM action is evaluated first, then the DENY action, and finally the ALLOW action. The evaluation is determined by the following rules:

If there are any CUSTOM policies that match the request, evaluate and deny the request if the evaluation result is deny.
If there are any DENY policies that match the request, deny the request.
If there are no ALLOW policies for the workload, allow the request.
If any of the ALLOW policies match the request, allow the request.
Deny the request.

For iptables, the default is ACCEPT, but you can configure it to be DENY by default, there's a setting. I thought about having there be a top level default option, but wasn't sure if it was worth it? (or could be added later).

https://superuser.com/questions/437013/what-does-an-empty-iptables-mean
https://servercomputing.blogspot.com/2012/03/change-iptables-default-policy-to-drop.html

But definitely taking note that this needs to be spelling out very clearly in the docs, which will come once this PR is merged).

markmandel · 2021-10-07T21:26:10Z

src/filters/firewall/metrics.rs

+
+        Ok(Metrics {
+            packets_denied_on_read: deny_metric
+                .get_metric_with_label_values(vec!["on_read"].as_slice())?,


Rather than "on_read" and "on_write" this could be "read" and "write" ?

I figured it would be good to set a precedent here, as we might do this in a few different places.

That sounds reasonable to me. (I'm thinking we should just call the actual filter methods on_read and on_write as well to match)

I figured it would be good to set a precedent here, as we might do this in a few different places.

I think switching it is fine, but I kinda wish we had a better, more typed API for getting and creating the metric labels so that it's easier to have it always be the right thing.

but I kinda wish we had a better, more typed API for getting and creating the metric labels so that it's easier to have it always be the right thing.

🤔 Yeah me too. I moved this into some consts since they get reused, and it'll be easier to put somewhere common if we need to later.

iffyio

Left some comments, overall I think this looks good!
We should include documentation for the added feature though (preferably in this PR since it'll be easier to review the docs that way)

iffyio · 2021-10-08T06:43:48Z

src/filters/firewall/config.rs

+#[derive(Clone, Debug, PartialEq)]
+pub struct PortRange {
+    pub min: u16,
+    pub max: u16,
+}
+
+impl PortRange {
+    /// Does this range contain a specific port value?
+    ///
+    /// # Arguments
+    ///
+    /// * `port`:
+    ///
+    /// returns: bool
+    pub fn contains(&self, port: u16) -> bool {
+        port >= self.min && port <= self.max
+    }
+}
+
+impl Serialize for PortRange {
+    /// Serialise the [PortRange] into a single digit if min and max are the same
+    /// otherwise, serialise it to "min-max".
+    fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
+    where
+        S: Serializer,
+    {
+        if self.max == self.min {
+            return serializer.serialize_str(self.min.to_string().as_str());
+        }
+
+        let range = format!("{}-{}", self.min, self.max);
+        serializer.serialize_str(range.as_str())
+    }
+}
+


We can serialize directly into rust's range type instead of introducing ours? https://doc.rust-lang.org/std/ops/struct.Range.html

Oh and @XAMPPRocky 's idea of using rust's range syntax wherever we need these types could be nice to use as well wdyt? #404 (comment)

@XAMPPRocky made the same comment about using a Range - that's an excellent point, I will make that change.

So the format for the port range I directly copied from how we do port ranges in the GCP Firewalls:
https://cloud.google.com/vpc/docs/firewalls#:~:text=protocol%20and%20port%20range

AWS does similar:
https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/security-group-rules.html#:~:text=for%20example%2C%207000-8000

I figured it would be good to stick with something that people know from configuring firewall rules in existing systems.

I figured it would be good to stick with something that people know from configuring firewall rules in existing systems.

That sounds very reasonable to me 👍

I ended up using the newtype pattern to wrap the Range, for two reasons:

If I used deserialize_with then I would have had to deserialize the entire Vector, which seems like too much work.

The other side effect was it forced me to write a PortRange::new(min: u16, max: u16) -> Result<Self, PortRangeError> which moved all the validation logic for the range into the constructor, rather than handling it in the parsing rules - which removes the repetition, and makes a better API surface if anyone else wants to use it directly.

Lemme know what you think 👍🏻

iffyio · 2021-10-08T06:49:27Z

src/filters/firewall/config.rs

+                ConvertProtoConfigError::new(
+                    format!("min too large: {}", err),
+                    Some("port.min".into()),
+                )


we likely need to do the validation after we have deserialized into the PortRange struct, so that we can avoid duplicating the validation logic?

This should be better now since we have PortRange::new(...) -> Result, and the validation is in there.

iffyio · 2021-10-08T06:59:07Z

src/filters/firewall/config.rs

+
+#[derive(Clone, Deserialize, Debug, PartialEq, Serialize)]
+pub enum Action {
+    /// Matching details will allow packets through.


Suggested change

/// Matching details will allow packets through.

/// Matching rules will allow packets through.

iffyio · 2021-10-08T06:59:17Z

src/filters/firewall/config.rs

+    /// Matching details will allow packets through.
+    #[serde(rename = "ALLOW")]
+    Allow,
+    /// Matching details will block packets.


Suggested change

/// Matching details will block packets.

/// Matching rules will block packets.

iffyio · 2021-10-08T07:07:32Z

src/filters/firewall/metrics.rs

+
+impl Metrics {
+    pub(super) fn new(registry: &Registry) -> MetricsResult<Self> {
+        let event_labels = vec!["events"];


Suggested change

let event_labels = vec!["events"];

let event_labels = vec!["event"];

iffyio · 2021-10-08T07:08:52Z

src/filters/firewall/metrics.rs

+
+        Ok(Metrics {
+            packets_denied_on_read: deny_metric
+                .get_metric_with_label_values(vec!["on_read"].as_slice())?,


That sounds reasonable to me. (I'm thinking we should just call the actual filter methods on_read and on_write as well to match)

iffyio · 2021-10-08T07:11:09Z

src/filters/firewall.rs

+        for rule in &self.on_read {
+            if rule.contains(ctx.from) {
+                return match rule.action {
+                    Action::Allow => {
+                        debug!(self.log, "Allow"; "event" => "read", "from" =>  ctx.from.to_string());
+                        self.metrics.packets_allowed_on_read.inc();
+                        Some(ctx.into())
+                    }
+                    Action::Deny => {
+                        debug!(self.log, "Deny"; "event" => "read", "from" => ctx.from );
+                        self.metrics.packets_denied_on_read.inc();
+                        None
+                    }
+                };
+            }
+        }


The loop and rules checking seem identical to both read and write, we can move it to its own function and pass in arguments that differ like the vec, eventtype, metrics?

iffyio · 2021-10-08T07:21:40Z

src/filters/firewall.rs

+            }
+        }
+
+        debug!(self.log, "default: Deny"; "event" => "read", "from" => ctx.from.to_string());


thinking denying by default could be fine depending on what guarantees we're providing which we should mention in the docs - if we guarantee that we'll validate according to the rules list order then yeah it should be fine and they can have a catch all range at the end of the list.
If we don't guarantee the order rules are checked against (which would enable us to optimizations to e.g support when we have lots of rules and want to avoid scanning) then having an explicit 'default allow/deny' option might be preferrable.

Coming to think of it (probably not for this PR), we should also document the behavior when a user specifies conflicting port ranges in different rules e.g allow 10-20 in one rule and deny 20-30 in another - e.g we could treat the config as invalid

XAMPPRocky

LGTM! Mostly style and tidy comments.

XAMPPRocky · 2021-10-08T06:22:21Z

src/filters/firewall.rs

+            }
+        }
+
+        debug!(self.log, "default: Deny"; "event" => "read", "from" => ctx.from.to_string());


What's the behaviour of other firewall projects here, what happens in Envoy, firewalld, iptables, etc? I think that will probably give us an answer.

XAMPPRocky · 2021-10-08T06:29:03Z

src/filters/firewall.rs

+        let firewall = Firewall {
+            log: logger(),
+            metrics: Metrics::new(&Registry::default()).unwrap(),
+            on_read: vec![Rule {
+                action: Action::Allow,
+                source: "192.168.75.0/24".parse().unwrap(),
+                ports: vec![PortRange { min: 10, max: 100 }],
+            }],
+            on_write: vec![],
+        };


Can you call Firewall::new here instead? Then if you later update Firewall with a new field from Config you don't have to also update these tests.

XAMPPRocky · 2021-10-08T06:33:36Z

src/filters/firewall.rs

+
+        let ctx = ReadContext::new(
+            UpstreamEndpoints::from(
+                Endpoints::new(vec![Endpoint::new("127.0.0.1:8080".parse().unwrap())]).unwrap(),


Using parse here is a bit of anti-pattern as we're doing string parsing when we construct valid sockets with ([u8; 4], u16)/(Ipv4Addr, u16).

Suggested change

Endpoints::new(vec![Endpoint::new("127.0.0.1:8080".parse().unwrap())]).unwrap(),

Endpoints::new(vec![Endpoint::new((Ipv4Addr::LOCALHOST, 8080).into())]).unwrap(),

XAMPPRocky · 2021-10-08T06:34:15Z

src/filters/firewall.rs

+            UpstreamEndpoints::from(
+                Endpoints::new(vec![Endpoint::new("127.0.0.1:8080".parse().unwrap())]).unwrap(),
+            ),
+            "192.168.75.20:80".parse().unwrap(),


Suggested change

"192.168.75.20:80".parse().unwrap(),

([192, 168, 75, 20], 80).into(),

I did not know you could do that! Nice!

XAMPPRocky · 2021-10-08T06:35:45Z

src/filters/firewall.rs

+                Endpoints::new(vec![Endpoint::new("127.0.0.1:8080".parse().unwrap())]).unwrap(),
+            ),
+            "192.168.75.20:2000".parse().unwrap(),


I'd add the addresses from above as consts and reference them here.

I moved them into variables. Was a bit easier for ownership.

XAMPPRocky · 2021-10-08T07:39:00Z

src/filters/firewall/config.rs

+use super::quilkin::extensions::filters::firewall::v1alpha1::firewall::{
+    Action as ProtoAction, PortRange as ProtoPortRange, Rule as ProtoRule,
+};
+use super::quilkin::extensions::filters::firewall::v1alpha1::Firewall as ProtoConfig;


Suggested change

use super::quilkin::extensions::filters::firewall::v1alpha1::firewall::{

Action as ProtoAction, PortRange as ProtoPortRange, Rule as ProtoRule,

};

use super::quilkin::extensions::filters::firewall::v1alpha1::Firewall as ProtoConfig;

use super::quilkin::extensions::filters::firewall::v1alpha1::{

Firewall as ProtoConfig,

firewall::{Action as ProtoAction, PortRange as ProtoPortRange, Rule as ProtoRule},

};

XAMPPRocky · 2021-10-08T07:40:37Z

src/filters/firewall/config.rs

+use std::convert::TryFrom;
+use std::fmt;
+use std::fmt::Formatter;
+use std::net::SocketAddr;


I'd reduce these to a single import. People still write code like this because nested imports were a later addition.

Suggested change

use std::convert::TryFrom;

use std::fmt;

use std::fmt::Formatter;

use std::net::SocketAddr;

use std::{convert::TryFrom, fmt::{self, Formatter}, net::SocketAddr};

Oooh! I like it! Today I also learned the IDEA Rust can do this for you 👍

XAMPPRocky · 2021-10-08T07:44:54Z

src/filters/firewall.rs

+use crate::filters::firewall::config::{Action, Config, Rule};
+use crate::filters::firewall::metrics::Metrics;
+use crate::filters::{
+    CreateFilterArgs, DynFilterFactory, Error, Filter, FilterFactory, FilterInstance, ReadContext,
+    ReadResponse, WriteContext, WriteResponse,
+};


Suggested change

use crate::filters::firewall::config::{Action, Config, Rule};

use crate::filters::firewall::metrics::Metrics;

use crate::filters::{

CreateFilterArgs, DynFilterFactory, Error, Filter, FilterFactory, FilterInstance, ReadContext,

ReadResponse, WriteContext, WriteResponse,

};

use crate::filters::prelude::*;

use self::{config::{Action, Config, Rule}, metrics::Metrics};

src/filters/firewall.rs

XAMPPRocky · 2021-10-08T07:49:02Z

src/filters/firewall/config.rs

+                        let min = split.0.parse::<u16>().map_err(de::Error::custom)?;
+                        let max = split.1.parse::<u16>().map_err(de::Error::custom)?;
+
+                        if min > max {


Pointing out here that this does allow effective ranges of length zero where min == max, not sure if that's supposed to be intentional.

Yep, that's intentional - it's the equivalent of having a single port value 👍🏻

The new changes fix this, so it can't be this way anymore.

markmandel · 2021-10-12T17:56:29Z

Thanks for the detailed feedback, I'll take some time to go through it all and make adjustments 👍🏻

markmandel

Still working through all the comments, but I think this is looking better! Thanks for all the input.

markmandel · 2021-10-19T01:07:17Z

src/filters/firewall/config.rs

+#[derive(Clone, Debug, PartialEq)]
+pub struct PortRange {
+    pub min: u16,
+    pub max: u16,
+}
+
+impl PortRange {
+    /// Does this range contain a specific port value?
+    ///
+    /// # Arguments
+    ///
+    /// * `port`:
+    ///
+    /// returns: bool
+    pub fn contains(&self, port: u16) -> bool {
+        port >= self.min && port <= self.max
+    }
+}
+
+impl Serialize for PortRange {
+    /// Serialise the [PortRange] into a single digit if min and max are the same
+    /// otherwise, serialise it to "min-max".
+    fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
+    where
+        S: Serializer,
+    {
+        if self.max == self.min {
+            return serializer.serialize_str(self.min.to_string().as_str());
+        }
+
+        let range = format!("{}-{}", self.min, self.max);
+        serializer.serialize_str(range.as_str())
+    }
+}
+


I ended up using the newtype pattern to wrap the Range, for two reasons:

If I used deserialize_with then I would have had to deserialize the entire Vector, which seems like too much work.

The other side effect was it forced me to write a PortRange::new(min: u16, max: u16) -> Result<Self, PortRangeError> which moved all the validation logic for the range into the constructor, rather than handling it in the parsing rules - which removes the repetition, and makes a better API surface if anyone else wants to use it directly.

Lemme know what you think 👍🏻

markmandel · 2021-10-19T01:08:05Z

src/filters/firewall/config.rs

+                ConvertProtoConfigError::new(
+                    format!("min too large: {}", err),
+                    Some("port.min".into()),
+                )


This should be better now since we have PortRange::new(...) -> Result, and the validation is in there.

markmandel · 2021-10-19T03:02:28Z

src/filters/firewall/config.rs

+use std::convert::TryFrom;
+use std::fmt;
+use std::fmt::Formatter;
+use std::net::SocketAddr;


Oooh! I like it! Today I also learned the IDEA Rust can do this for you 👍

markmandel · 2021-10-19T03:28:27Z

src/filters/firewall/config.rs

+                        let min = split.0.parse::<u16>().map_err(de::Error::custom)?;
+                        let max = split.1.parse::<u16>().map_err(de::Error::custom)?;
+
+                        if min > max {


The new changes fix this, so it can't be this way anymore.

markmandel

Finished my updates - should be good for a second review.

markmandel · 2021-10-22T04:16:06Z

src/filters/firewall.rs

+                Endpoints::new(vec![Endpoint::new("127.0.0.1:8080".parse().unwrap())]).unwrap(),
+            ),
+            "192.168.75.20:2000".parse().unwrap(),


I moved them into variables. Was a bit easier for ownership.

iffyio · 2021-10-22T06:53:44Z

src/filters/firewall/config.rs

+/// InitializeError is returned with an error message if the
+/// [`ClusterManager`] fails to initialize properly.


Suggested change

/// InitializeError is returned with an error message if the

/// [`ClusterManager`] fails to initialize properly.

Whoops! Nice catch. Copy paste error! Removed!

markmandel · 2021-10-25T22:12:56Z

🤞🏻 this should be good to go now.

XAMPPRocky

LGTM! Left some documentation comments but feel free to address in a future PR.

tests/firewall.rs

src/filters/firewall.rs

src/filters/firewall/config.rs

markmandel · 2021-10-26T17:59:55Z

@XAMPPRocky that's all excellent feedback (and I totally have a habit of writing Go style comments) -- lemme incorporate that all in before it gets merged 👍🏻

Code implementation of a Firewall filter that will allow/deny packets based on their from address on both read and write. Documentation to come next to finish off the below two tickets. Work on googleforgames#158 Work on googleforgames#343

markmandel · 2021-10-27T15:47:21Z

Just wanted to check before merging if there was anything else? Otherwise I will merge and start writing the docs. 👍🏻

quilkin-bot · 2021-10-27T21:25:26Z

Build Succeeded 🥳

Build Id: 75592aae-05e5-4de7-8f40-60c4e2dccbe8

To build this version:

git fetch [email protected]:googleforgames/quilkin.git pull/416/head:pr_416 && git checkout pr_416
cargo build

markmandel added kind/feature New feature or request area/user-experience Pertaining to developers trying to use Quilkin, e.g. cli interface, configuration, etc labels Oct 7, 2021

markmandel requested review from XAMPPRocky and iffyio October 7, 2021 21:21

google-cla bot added the cla: yes label Oct 7, 2021

github-actions bot added the size/l label Oct 7, 2021

markmandel commented Oct 7, 2021

View reviewed changes

iffyio reviewed Oct 8, 2021

View reviewed changes

XAMPPRocky approved these changes Oct 8, 2021

View reviewed changes

markmandel force-pushed the feature/firewall-filter branch from 5c9f500 to 9bfd5e2 Compare October 19, 2021 03:39

markmandel commented Oct 19, 2021

View reviewed changes

markmandel force-pushed the feature/firewall-filter branch from 97a5752 to cbc680e Compare October 22, 2021 04:29

markmandel commented Oct 22, 2021

View reviewed changes

iffyio approved these changes Oct 22, 2021

View reviewed changes

markmandel force-pushed the feature/firewall-filter branch from cbc680e to c8ab12d Compare October 25, 2021 22:05

XAMPPRocky approved these changes Oct 26, 2021

View reviewed changes

markmandel added 2 commits October 26, 2021 14:05

Code: Firewall filter

fef299f

Code implementation of a Firewall filter that will allow/deny packets based on their from address on both read and write. Documentation to come next to finish off the below two tickets. Work on googleforgames#158 Work on googleforgames#343

Review updates, including refactoring with PortRange::new()

4301ad5

markmandel force-pushed the feature/firewall-filter branch from c8ab12d to 4301ad5 Compare October 26, 2021 21:49

Merge branch 'main' into feature/firewall-filter

8bf9b25

XAMPPRocky merged commit 5d713e5 into googleforgames:main Oct 28, 2021

markmandel deleted the feature/firewall-filter branch November 2, 2021 22:28

	/// Matching details will allow packets through.
	/// Matching rules will allow packets through.

	/// Matching details will block packets.
	/// Matching rules will block packets.

	let event_labels = vec!["events"];
	let event_labels = vec!["event"];

	Endpoints::new(vec![Endpoint::new("127.0.0.1:8080".parse().unwrap())]).unwrap(),
	Endpoints::new(vec![Endpoint::new((Ipv4Addr::LOCALHOST, 8080).into())]).unwrap(),

	"192.168.75.20:80".parse().unwrap(),
	([192, 168, 75, 20], 80).into(),

		/// InitializeError is returned with an error message if the
		/// [`ClusterManager`] fails to initialize properly.

Code: Firewall filter #416

Code: Firewall filter #416

Conversation

markmandel commented Oct 7, 2021

markmandel commented Oct 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iffyio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XAMPPRocky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markmandel commented Oct 12, 2021

markmandel left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markmandel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markmandel commented Oct 25, 2021

XAMPPRocky left a comment

Choose a reason for hiding this comment

markmandel commented Oct 26, 2021

markmandel commented Oct 27, 2021

quilkin-bot commented Oct 27, 2021

markmandel left a comment •

edited

Loading