Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix default reject rule to correctly handle packets that should be assembled #6857

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix default reject rule to correctly handle packets that should be assembled #6857

wants to merge 1 commit into from

Conversation

hongliangl
Copy link
Contributor

When using an L7 NetworkPolicy that allows egress HTTP requests, the corresponding Suricata rules may look like the following example:

reject ip any any -> any any (msg: "Reject by AntreaNetworkPolicy:default/egress-allow-http"; flow: to_server, established; sid: 1;)
pass http any any -> any any (msg: "Allow http by AntreaNetworkPolicy:default/egress-allow-http"; http.method; content:"GET"; sid: 2;)`

If an HTTP request exceeds the MTU, it will be split into multiple packets. The packets should be reassembled and allowed by the corresponding Suricata rule for the L7 NetworkPolicy.

However, there is a default reject rule which is to reject packets which are not matched by the pass rule, which will take effect before packets are reassembled and matched by the pass rule, causing the connection to fail.

To address the issue, the keyword only_stream is added to the default reject rule. This ensures that only reassembled packets are matched, preventing premature rejection of packets that should be allowed after reassembly.

@hongliangl
Fix default reject rule to correctly handle packets that should be re…
c93a403 
…assembled

When using an L7 NetworkPolicy that allows egress HTTP requests, the corresponding
Suricata rules may look like the following example:

```
reject ip any any -> any any (msg: "Reject by AntreaNetworkPolicy:default/egress-allow-http"; flow: to_server, established; sid: 1;)
pass http any any -> any any (msg: "Allow http by AntreaNetworkPolicy:default/egress-allow-http"; http.method; content:"GET"; sid: 2;)`
```

If an HTTP request exceeds the MTU, it will be split into multiple packets. The packets
should be reassembled and allowed by the corresponding Suricata rule for the L7
NetworkPolicy.

However, there is a default reject rule which is to reject packets which are not matched
by the `pass` rule, which will take effect before packets are reassembled and matched by
the `pass` rule, causing the connection to fail.

To address the issue, the keyword `only_stream` is added to the default reject rule. This
ensures that only reassembled packets are matched, preventing premature rejection of
packets that should be allowed after reassembly.

Signed-off-by: Hongliang Liu <[email protected]>
@hongliangl hongliangl added area/network-policy Issues or PRs related to network policies. action/backport Indicates a PR that requires backports. labels Dec 12, 2024
@hongliangl hongliangl added this to the Antrea v2.3 release milestone Dec 12, 2024
@hongliangl hongliangl mentioned this pull request Dec 12, 2024
Open
tnqn
tnqn approved these changes Dec 12, 2024
View reviewed changes
Copy link
Member

@tnqn tnqn left a comment

Choose a reason for hiding this comment

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

Great finding.

But does it fix a new issue or #6806?

@antoninbas
Copy link
Contributor

I have read about the only_stream keyword previously, but had trouble understanding how it works. Other people also seem to find the behavior surprising: https://forum.suricata.io/t/rule-with-flow-option-only-stream-never-seems-to-match-anything/1655

We definitely need to test the case where request packets (to_server) exceed the MTU, with a new e2e test. The e2e tests are currently failing with this change:

--- FAIL: TestL7NetworkPolicy (143.42s)
    --- FAIL: TestL7NetworkPolicy/HTTP (50.38s)
        --- FAIL: TestL7NetworkPolicy/HTTP/Ingress (22.19s)
        --- FAIL: TestL7NetworkPolicy/HTTP/Egress (22.18s)
    --- FAIL: TestL7NetworkPolicy/TLS (18.16s)
    --- FAIL: TestL7NetworkPolicy/Logging (45.23s)

BTW, I have found that testing Suricata rules is much more convenient when using a pcap file and running Suricata in offline mode. While we still need e2e tests, I wonder if we could have some extra tests (maybe with input pcap and expected JSON logs) to quickly evaluate our Suricata rules. Maybe the best approach would be to have a separate repo where we can keep some reference pcaps that people can use when working on L7NP (bug fixing, support for new protocols, etc.). What do you think @tnqn @hongliangl?

@antoninbas
Copy link
Contributor

Great finding.

But does it fix a new issue or #6806?

I don't know if the current patch is correct because tests are failing.
I think the issue may be orthogonal - but related - to #6806. #6806 was for reply packets coming from the external packet. This fix is for packets in the other direction (which match the to_server keyword). Both fixes are going to be needed. This fix is about handling app requests that span multiple packets, but we still need to handle checksum issues for reply traffic (#6806, and its proposed fix #6843).

@hongliangl
Copy link
Contributor Author

This PR cannot fix #6876 since the keyword only_stream invalidates the default reject rule and close it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tnqn tnqn tnqn approved these changes

@antoninbas antoninbas Awaiting requested review from antoninbas

Assignees
No one assigned
Labels
action/backport Indicates a PR that requires backports. area/network-policy Issues or PRs related to network policies.
Projects
None yet
Milestone
Antrea v2.3 release
Development

Successfully merging this pull request may close these issues.
3 participants
@hongliangl @antoninbas @tnqn