Skip to content

Conversation

sergeymatov
Copy link

No description provided.

@sergeymatov sergeymatov requested a review from Frostman as a code owner June 10, 2025 14:07
@sergeymatov sergeymatov marked this pull request as draft June 10, 2025 14:07
@sergeymatov sergeymatov force-pushed the pr/smatov/firewall-api-propoasl branch from 873a289 to f08bdd5 Compare June 10, 2025 14:30
@sergeymatov sergeymatov force-pushed the pr/smatov/firewall-api-propoasl branch from f08bdd5 to 8f8729f Compare June 11, 2025 11:52
@sergeymatov sergeymatov requested a review from Frostman June 11, 2025 14:26
@sergeymatov sergeymatov marked this pull request as ready for review June 12, 2025 09:26
@sergeymatov sergeymatov requested a review from mvachhar June 12, 2025 09:26
@sergeymatov sergeymatov changed the title WIP: feat: add simple Firewall design feat: add simple Firewall design Jun 12, 2025
@sergeymatov sergeymatov force-pushed the pr/smatov/firewall-api-propoasl branch from 8f8729f to ad10042 Compare June 13, 2025 09:25
@sergeymatov sergeymatov requested a review from Frostman June 13, 2025 09:28
@sergeymatov sergeymatov force-pushed the pr/smatov/firewall-api-propoasl branch from ad10042 to 1f6aae8 Compare June 17, 2025 18:16
Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I note that the concept is very close to K8s' Network Policies. Have we considered making our API closer to these? I do understand they're different objects, but users might find the syntax easier to remember if they're already familiar with it?

One point in particular is that we currently define CIDRs under tcp -> src, for example; but it seems to me like it's more frequent to define access to multiple services for a given IP or CIDR, so this may lead to some duplication of the CIDRs involved. Wouldn't it make more sense to specify the CIDR first, then for this CIDR, to specify the protocol?

## Simple firewall implementation
VPC Firewall implies zero trust if firewall pattern is specified.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do I understand this right?:

  • No firewall policy specified: all traffic allowed
  • allow policy specified: all traffic denied, save for allowed patterns
  • deny-only policy specified: all traffic denied
  • allow + deny policies: traffic not matching allowed patterns is denied; traffic matching allowed patterns is allowed, except if it's explicitly denied by the deny policy, which always takes precedence

Is this correct?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. If there is deny only policy specified, allow should be added for any other traffic.
Also, order is defined by order in the API message.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. If there is deny only policy specified, allow should be added for any other traffic.

Sounds good to me, thanks!

Also, order is defined by order in the API message.

This is more concerning to me. It sounds super-easy to enable by mistake the traffic you specifically wanted to deny, because a user didn't understand the significance of the order, or they got confused and mentally swapped the order, or did a copy-paste error and put the deny policy at the wrong place. Are we sure we need to nest multiple levels of allow and deny policies? Wouldn't a single level of allow + a preceding level of deny be safer (and easier) to use?

@Frostman Frostman marked this pull request as draft July 29, 2025 17:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants