Add FirewallRule class #86

ndowmon · 2021-05-17T17:09:28Z

No description provided.

mknoedel · 2021-05-17T17:46:20Z

src/schemas/FirewallRule.json

@@ -0,0 +1,59 @@
+{
+  "$schema": "http://json-schema.org/draft-07/schema#",


Will we need to port these changes to the AWS integration? It looks like we just put these rules in an array on a key for that integration.
https://bitbucket.org/jupiterone/jupiter-integration-aws/src/b1b082a76600f85738911adbea9d4112869059e7/src/integration-aws/ec2/converters/entities.ts?at=master#entities.ts-583

I also assume you already took a look at Rule, but thought I'd bring that up as well.

Yeah, if this goes through I think it'd be good to get it into existing integrations like AWS, since the JSON.stringify(ipPermission) operation makes these properties useless from a query language perspective.

I have looked at Rule but IMO a firewall rule is a super-specific class of configuration (as indicated by the common properties shared across all firewall rules)

mknoedel

Looks good to me so long as you have determined we need this. I would like a little documentation on the PR for easier debugging in the future before you merge this though.

austinkelleher · 2021-05-17T18:29:41Z

src/schemas/FirewallRule.json

+          "type": "integer"
+        },
+        "source": {
+          "description": "Source of the firewall rule. Can be IP address, CIDR range, 'Any', or other firewall-defined options.",


Should we recommend only * instead of Any?

austinkelleher · 2021-05-17T18:30:33Z

src/schemas/FirewallRule.json

+          "format": "ip"
+        },
+        "sourcePort": {
+          "description": "Source port of the firewall rule. Typically an integer between 0 and 65535, but could also be 'Any', '*', or range of ports.",


Any, *, or "range of ports", will not be accepted when thetype is integer

austinkelleher · 2021-05-17T18:35:02Z

src/schemas/FirewallRule.json

+          "format": "ip"
+        },
+        "destinationPort": {
+          "description": "Destination port of the firewall rule. Typically an integer between 0 and 65535, but could also be 'Any', '*', or range of ports.",


Should we recommend only * instead of Any?

austinkelleher · 2021-05-17T18:35:22Z

src/schemas/FirewallRule.json

+        "protocol": {
+          "description": "The protocol ",
+          "type": "string",
+          "examples": ["TCP", "UDP", "Any"]


Same question about recommending * here?

erkangz · 2021-05-18T03:15:11Z

I did not follow the change here closely. Why is this class needed?

I think we have a Rule and RuleSet class already?

erkangz · 2021-05-18T03:18:03Z

With the AWS integration, details of each rule are captures as properties on the relationship between the entity and what is allowed to/from the entity.

I'm concerned that adding this will unnecessarily add more entities, which is undesired from both our perspective (data volume) and customer's (cost).

ndowmon · 2021-05-18T11:58:48Z

@erkangz yes, I see how this works in AWS. My concern with putting firewall rules on edges is that we cannot be as expressive on edges as we can on nodes (at least in J1QL). For example, there's not currently a way in J1QL to express direction of edges, representing ingress/egress, and edges do not allow array properties for IP addresses, ports, or protocols defined by these rules. Those are some of my main concerns when assigning firewall rules to edges.

As for creating a new class FirewallRule as opposed to using Rule, this is a pretty rare case where almost all firewall rules will share this collection of properties (sourcePort, sourceIp, destinationPort, destinationIp, protocol, action, direction), and none of these properties would be valid for almost any other type of Rule, so the distinction in my mind lends FirewallRule to its own classification.

erkangz · 2021-05-21T14:00:05Z

@erkangz yes, I see how this works in AWS. My concern with putting firewall rules on edges is that we cannot be as expressive on edges as we can on nodes (at least in J1QL). For example, there's not currently a way in J1QL to express direction of edges, representing ingress/egress, and edges do not allow array properties for IP addresses, ports, or protocols defined by these rules. Those are some of my main concerns when assigning firewall rules to edges.

As for creating a new class FirewallRule as opposed to using Rule, this is a pretty rare case where almost all firewall rules will share this collection of properties (sourcePort, sourceIp, destinationPort, destinationIp, protocol, action, direction), and none of these properties would be valid for almost any other type of Rule, so the distinction in my mind lends FirewallRule to its own classification.

Valid concern. However, that's a query lang issue/limitation and I don't think we should change the data-model because of that. For direction, specifically to firewall rules, we do have ingress and egress boolean properties on the edge that can be used to filter on direction.

aiwilliams · 2021-05-23T01:50:07Z

tldr; I would stick with the current approach until we can prove it won't work (perhaps examples from Azure or Google Cloud) and we should develop docs describing the firewall model and how to implement it.

A few thoughts:

We can make any entity class non-billable.
There would be number of rules X 2 more objects if we had another entity between a Firewall and the resources it relates to because of a rule (Firewall -> FirewallRule -> (Host|IpAddress|Network) vs Firewall -> (Host|IpAddress|Network)), but I think it very little data in the big scheme of things.
We want to relate the Firewall to each (Host|IpAddress|Network) represented in a rule, so it isn't necessary for these rule relationships to support string[] properties.
It is reasonable to anticipate we can make the query language support relationships better, though I believe we've added properties to entities in the past to get them into the index and duplicated property names to make queries look better, efforts that acknowledge it could be some time before the query language/service supports features to avoid those ingestion decisions.
It is understandable that early decisions can make it difficult to change things now, and there would need to be good reasons to go through pains to do so.

Here are some reasons I recall that relationships were chosen.

Firewall rules simply define relationships between the Firewall and (Host|IpAddress|Network) entities, similar to a policy document (rules could be added as raw data of a Firewall).
This query reads nicely: find Host that PROTECTS Firewall that ALLOWS Internet.
This query reads less nice: find Host that PROTECTS Firewall that HAS Rule that ALLOWS Internet.
This query allows listing specific rules just fine: find Host that PROTECTS Firewall that ALLOWS as rule Network where rule.inbound=true return rule.*.

@erkangz, do you have others?

aiwilliams · 2021-05-23T01:51:15Z

Note that there is a TypeScript type for rule relationships:

data-model/src/relationships.ts

Line 1 in de4d2ff

export interface FirewallRuleProperties {

Add FirewallRule class

27688b2

ndowmon requested review from aiwilliams, kenanwarren, austinkelleher, mknoedel, erkangz and a-u-h-g May 17, 2021 17:09

mknoedel reviewed May 17, 2021

View reviewed changes

mknoedel previously approved these changes May 17, 2021

View reviewed changes

ndowmon linked an issue May 17, 2021 that may be closed by this pull request

Create FirewallRule entity class #70

Open

austinkelleher suggested changes May 17, 2021

View reviewed changes

Allow string on FirewallRule.sourcePort/.destinationPort

dd738b8

ndowmon dismissed mknoedel’s stale review via dd738b8 May 18, 2021 11:32

ndowmon requested a review from austinkelleher May 18, 2021 11:42

ndowmon closed this Jul 21, 2021

zemberdotnet deleted the add-FirewallRule branch August 24, 2022 19:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add FirewallRule class #86

Add FirewallRule class #86

ndowmon commented May 17, 2021

mknoedel May 17, 2021

mknoedel May 17, 2021

ndowmon May 17, 2021

mknoedel left a comment

austinkelleher May 17, 2021

austinkelleher May 17, 2021

austinkelleher May 17, 2021

austinkelleher May 17, 2021

erkangz commented May 18, 2021

erkangz commented May 18, 2021

ndowmon commented May 18, 2021

erkangz commented May 21, 2021

aiwilliams commented May 23, 2021

aiwilliams commented May 23, 2021

		@@ -0,0 +1,59 @@
		{
		"$schema": "http://json-schema.org/draft-07/schema#",

Add FirewallRule class #86

Add FirewallRule class #86

Conversation

ndowmon commented May 17, 2021

mknoedel May 17, 2021

Choose a reason for hiding this comment

mknoedel May 17, 2021

Choose a reason for hiding this comment

ndowmon May 17, 2021

Choose a reason for hiding this comment

mknoedel left a comment

Choose a reason for hiding this comment

austinkelleher May 17, 2021

Choose a reason for hiding this comment

austinkelleher May 17, 2021

Choose a reason for hiding this comment

austinkelleher May 17, 2021

Choose a reason for hiding this comment

austinkelleher May 17, 2021

Choose a reason for hiding this comment

erkangz commented May 18, 2021

erkangz commented May 18, 2021

ndowmon commented May 18, 2021

erkangz commented May 21, 2021

aiwilliams commented May 23, 2021

aiwilliams commented May 23, 2021