-
Notifications
You must be signed in to change notification settings - Fork 6
Add flow-filter stage to assert packets belong to a valid peering connection #1158
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
Open
qmonnet
wants to merge
5
commits into
main
Choose a base branch
from
pr/qmonnet/flow-filter
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The new flow-filter crate introduced in this commit contains a new pipeline stage for validating that a packet matches an existing peering connection, as defined in the configuration provided by the user. All packets that do not have a source IP, port and destination IP, port corresponding to existing, valid connections between the prefixes in exposed lists of peerings, get dropped. This allows us to enforce that traffic matches the peering rule: - Generically, whether or not the appropriate rules are set - For stateless NAT, which would just let the packet go un-NATed if there was no NAT rule to be found, so far - For stateful NAT, allowing us to remove the dirty implementation of similar checks in the stateful NAT code (in a future commit). Unit tests also come in a follow-up commit. Signed-off-by: Quentin Monnet <[email protected]>
bdc798d to
16de783
Compare
When looking up for the destination VPC for a packet, we need a special
treatment when IP/port ranges are shared across peerings. We already
have the checks in place to make sure that the lookup stage does not
return an answer when there are multiple possible matching destination
VPCs (based on source VPC and destination address).
However, the restriction is too strong: when we have overlapping, but
distinct IP ranges shared across peerings, we can tell the destination
VPC for some portion of the ranges.
Consider the following case, for example:
VPC A <-> VPC B <-> VPC C
1.0.0.0/24 2.0.0.0/24 2.0.0.0/24 1.0.0.0/24
In this case, we just can't tell, for packets coming from VPC B, what
the destination VPC is (unless there's some stateful NAT session in
place, but that's beyond the destination VPC lookup stage's scope).
But if, instead, we have:
VPC A <-> VPC B <-> VPC C
1.0.0.0/23 2.0.0.0/24 2.0.0.0/24 1.0.0.0/24
Then for packets going to 1.0.1.0/24, we know the destination is VPC A.
Right now the destination VPC lookup does not account for the
unambiguous portion of prefixes.
This commit does not change that, but it improves the flow-filter stage
instead, which is used to determine whether packets belong to allowed
connections (as defined by the user's peering configuration). This is
not necessary to check that the packet is allowed (if multiple valid
connections are present, the packet is definitely allowed), but due to
the context present in the flow-filter table, we consider replacing the
destination VPC lookup stage by the flow-filter stage only. This would
make one less stage in the pipeline, and would address the shared IPs
issues described above.
Signed-off-by: Quentin Monnet <[email protected]>
In preparation for replacing the destination VPC lookup stage, make the flow-filter set the destination VPC in the packet's metadata. The expectation is that the flow-filter stage should always find the same destination VPC as the dst_vpcd_lookup stage, with additional precision in the case of overlapping but distinct prefixes shared across peerings. The dst_vpcd_lookup stage is not remove yet. Signed-off-by: Quentin Monnet <[email protected]>
Rather than implementing longest-prefix-match lookups for IP prefixes with associated port ranges, reuse the generic struct that we recently introduced in the flow-filter crate. There should be no functional change. Signed-off-by: Quentin Monnet <[email protected]>
Add unit tests to test the various modules in the recent flow-filter crate. Most of these were generated by Claude, with manual control and adjustments. Co-Authored-By: Claude <[email protected]> Signed-off-by: Quentin Monnet <[email protected]>
16de783 to
0e29c5a
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
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.
The new flow-filter crate introduced in this commit contains a new pipeline stage for validating that a packet matches an existing peering connection, as defined in the configuration provided by the user. All packets that do not have a source IP, port and destination IP, port corresponding to existing, valid connections between the prefixes in exposed lists of peerings, get dropped.
This allows us to enforce that traffic matches the peering rule:
Because the new stage gets the whole context about the authorized connections (as per the peering configuration), it can also replace the destination VPC lookup, to avoid redundant structures and lookups.
In the future, we could also easily adjust and use the stage to mark whether packets should be processed for stateless or stateful NAT, which would allow us to drop the "NAT exempt list" hack used for stateful NAT.
PR is not ready to be merged because some points need discussion, but it's ready for review and comments.