Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Code: Firewall filter #416
Code: Firewall filter #416
Changes from all commits
fef299f
4301ad5
8bf9b25
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So if no rules match, I default to not allowing a packet through. Is this acceptable?
I see a few options:
on_read
andon_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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 anddeny 20-30
in another - e.g we could treat the config as invalidThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
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).