Fix L7NP enable logging wrong packet #6651
Conversation
docs/antrea-l7-network-policy.md
Outdated
| "dest_port": 80, | ||
| "proto": "TCP", | ||
| "pkt_src": "wire/pcap", | ||
| "tenant_id": 2, | ||
| "pkt_src": "stream (flow timeout)", |
There was a problem hiding this comment.
If the pkt src is stream (flow timeout), you are probably looking at the wrong message in the logs. There should be a message with src wire/pcap that shows up immediately, instead of after a flow timeout (which takes 30s to a minute IIRC)
| @@ -178,7 +178,7 @@ func generateTenantRulesData(policyName string, protoKeywords map[string]sets.Se | |||
| // Refer to Suricata detect engine in codebase for detailed tag keyword configuration. | |||
| var tagKeyword string | |||
| if enableLogging { | |||
| tagKeyword = " tag: session, 30, seconds;" | |||
| tagKeyword = " tag:host;" | |||
There was a problem hiding this comment.
I wonder why the previous version was not working, it feels like session should also have included the request packet.
BTW, should we use something like tag:host,1,packets as we only need the first packet?
There was a problem hiding this comment.
I did some further investigation and found some problems:
Logging the wrong packet is a known bug/expected behavior documented https://redmine.openinfosecfoundation.org/issues/3480 and https://redmine.openinfosecfoundation.org/issues/2069. Basically the idea is that the log advanced past the first packet that triggered the event, so this will be the case for each session. If changed to host, the first alert stays the same, but the second alert would capture the initial packet as required.
Below is the complete log:
-----(trigger 1st time)
alert
10.10.1.3 → 10.10.1.4 TCP 80 → 47588 [RST, ACK]
-----(trigger 2nd time)
10.10.1.4 → 10.10.1.3 TCP 55344 → 80 [SYN] SACK_PERM
10.10.1.3 → 10.10.1.4 TCP 80 → 55344 [SYN, ACK] SACK_PERM
10.10.1.4 → 10.10.1.3 TCP 55344 → 80 [ACK]
alert
10.10.1.4 → 10.10.1.3 HTTP GET /admin/index.html HTTP/1.1
10.10.1.3 → 10.10.1.4 TCP 80 → 55344 [RST, ACK]
alert: timeout
http: timeout
Given this fact, for the e2e it is possible to test it by triggering twice and look for the original packet. But I'm not sure of the necessity of enableLogging actually if this issue continues in Suricata.
There was a problem hiding this comment.
If the first packet is dropped and we have to wait for the next request, then I would argue that this is not a correct implementation of the feature. It's a different request in a different TCP session (source port is different).
So it looks like it is not possible to capture the correct packet at all?
There was a problem hiding this comment.
AFAIK if we use the existing Suricata log for this feature, it's not possible to capture that initial packet =(
I can remove the tagged-packet if capturing the initial packet is the only purpose 😂
There was a problem hiding this comment.
Do you mean removing the feature completely?
There was a problem hiding this comment.
Actually I modified the alert configuration, it was able to log the initial packet
{"timestamp":"2024-09-06T22:56:25.262709+0000","flow_id":561336788716570,"in_iface":"antrea-l7-tap0","event_type":"alert","vlan":[2],"src_ip":"10.10.1.4","src_port":46094,"dest_ip":"10.10.1.3","dest_port":80,"proto":"TCP","pkt_src":"wire/pcap","tenant_id":2,"alert":{"action":"blocked","gid":1,"signature_id":1,"rev":0,"signature":"Reject by AntreaNetworkPolicy:default/allow-privileged-url-to-admin-role","category":"","severity":3,"tenant_id":2},"app_proto":"http","direction":"to_server","flow":{"pkts_toserver":3,"pkts_toclient":1,"bytes_toserver":307,"bytes_toclient":78,"start":"2024-09-06T22:56:25.261768+0000","src_ip":"10.10.1.4","dest_ip":"10.10.1.3","src_port":46094,"dest_port":80},"packet":"dtwWezuaHlOhfWpNgQAAAggARQAAjd+4QABABkSYCgoBBAoKAQO0DgBQH6jA1rH7p7qAGAH7zPsAAAEBCAou2X6GSIuizUdFVCAvYWRtaW4vaW5kZXguaHRtbCBIVFRQLzEuMQ0KSG9zdDogMTAuMTAuMS4zDQpVc2VyLUFnZW50OiBjdXJsLzcuNzQuMA0KQWNjZXB0OiAqLyoNCg0K","packet_info":{"linktype":1}}
But it applies to all rules because it's in the Suricata configuration, so cannot be controlled by enableLogging. Does it sound good to remove enableLogging which relates to tagged-packet, but add the packet support in alert that applies to all rules?
Or we can configure whether we want packet to be logged additionally in alert upon start of Suricata.
There was a problem hiding this comment.
Maybe it would be a good idea to discuss this at the community meeting.
There was a problem hiding this comment.
BTW, if this is for the alert event type, it only applies to rules that reject traffic, right?
We don't need the packet dump for allowed traffic, as we already have the app-layer metadata.
There was a problem hiding this comment.
Yes, tested that it only applies to rules rejecting traffic ( packet will only be added to alert logged for reject, only http will be logged for allow )
There was a problem hiding this comment.
We should do some perf testing using ab as @tnqn suggested, just to make sure that the number of rejected requests that we can handle stays roughly the same after enabling packet logging.
|
AB testing done: With With |
|
Thanks @qiyueyao. It seems that there is a noticeable impact, but nothing too significant (around 5%). Based on these results, I would lean towards enabling this by default, without introducing a new configuration parameter. But I'll wait until @tnqn chimes in, as he requested the benchmarking.
I am having trouble understanding these numbers. Shouldn't all requests fail (be rejected by Suricata)? |
I only sent 500 requests for both cases, and kept the socket open on receive errors. Based on online sources, Without specifying Without the source code of Apache ab tool, my guess is that for each rejected packet, |
qiyueyao
force-pushed
the
fix-l7-log
branch
from
c44fdd4
to
009dc8d
Compare
5% difference seems fine, enabling it without a new configuration parameter sounds good to me. We can add a configuration when there is a real need. |
antoninbas
added
the
action/release-note
test/e2e/l7networkpolicy_test.go
Outdated
| packetMatch := func(e, x *L7LogEntry) bool { | ||
| var expectedPacket *regexp.Regexp | ||
| var packetBytes string | ||
| if e.expectedPacketRegex != nil { | ||
| expectedPacket = e.expectedPacketRegex | ||
| packetBytes = string(x.PacketBytes) | ||
| } else if x.expectedPacketRegex != nil { | ||
| expectedPacket = x.expectedPacketRegex | ||
| packetBytes = string(e.PacketBytes) | ||
| } else { | ||
| return bytes.Equal(e.PacketBytes, x.PacketBytes) | ||
| } |
There was a problem hiding this comment.
I think we should make it more symmetric:
if e.expectedPacketRegex != nil {
// return false if x.PacketBytes don't match the regex
}
if x.expectedPacketRegex != nil {
// return false if e.PacketBytes don't match the regex
}
if e.expectedPacketRegex == nil && x.expectedPacketRegex == nil {
// return false if the byte slices don't match
}There was a problem hiding this comment.
Makes sense, updated.
test/e2e/l7networkpolicy_test.go
Outdated
| if e.expectedPacketRegex != nil { | ||
| return e.expectedPacketRegex.MatchString(string(x.PacketBytes)) | ||
| } | ||
| if x.expectedPacketRegex != nil { | ||
| return x.expectedPacketRegex.MatchString(string(e.PacketBytes)) | ||
| } | ||
| // Compare packet bytes if neither e nor x are expected log entry. | ||
| return bytes.Equal(e.PacketBytes, x.PacketBytes) |
There was a problem hiding this comment.
I don't think that's the logic I described in my last review.
There was a problem hiding this comment.
Oh I misunderstood, didn't consider there to be two expected log entries, but yeah it would be more comprehensive.
There was a problem hiding this comment.
Oh wait, is the only difference between the logics "check if both L7LogEntry contain expectedPacketRegex"? Then in that case, we should not compare any PacketBytes? (Am I missing something)
qiyueyao
force-pushed
the
fix-l7-log
branch
from
55a8b0a
to
e60d0c8
Compare
test/e2e/l7networkpolicy_test.go
Outdated
| if e.expectedPacketRegex != nil && !e.expectedPacketRegex.MatchString(string(x.PacketBytes)) || | ||
| x.expectedPacketRegex != nil && !x.expectedPacketRegex.MatchString(string(e.PacketBytes)) || | ||
| x.expectedPacketRegex == nil && e.expectedPacketRegex == nil && !bytes.Equal(e.PacketBytes, x.PacketBytes) { | ||
| return false | ||
| } | ||
| return true |
There was a problem hiding this comment.
I think you can use Match instead of MatchString and avoid the string conversion?
Yes this is the code I had in mind, but I find it more readable with multiple if statements.
There was a problem hiding this comment.
Understood, I'm still a bit curious on whether we need to compare PacketBytes at all, if both L7LogEntry contain expectedPacketRegex. Because in current case it would fail, as "" PacketBytes would not match the regex.
There was a problem hiding this comment.
If both entries contain expectedPacketRegex, then I think we only need to match PacketBytes against the expectedPacketRegex of the other entry.
Also in our current test code, I don't think this is an actual case (both entries containing expectedPacketRegex)? But regardless, the idea is to have a meaningful Match implementation here, and have Match(a,b) always be equivalent to Match(b,a).
There was a problem hiding this comment.
Thanks for explaining "have Match(a,b) always be equivalent to Match(b,a)"!
Current logs by Suricata when enableLogging is set, logs the wrong packet of RST instead of the original TCP packet, and remains an existing bug in Suricata. This solution modifies the design to remove enableLogging in L7, keep it in L4, and instead configure Suricata to log Packet info for all alert events. Fixes antrea-io#6636. Signed-off-by: Qiyue Yao <[email protected]>
qiyueyao
force-pushed
the
fix-l7-log
branch
from
e60d0c8
to
32ab682
Compare
|
Let's not backport this |
|
/test-all |
Currently when enableLogging is enabled for an L7NP rule, we enable "tagged" packet logging in the appropriate Suricata reject rule. Unfortunately, this leads to the wrong packet being logged: the RST packet generated by Suricata instead of the original TCP packet containing the HTTP request from the client. This solution modifies the design to remove enableLogging in L7, keep it in L4, and instead configure Suricata to log packet info for all alert events. This is a global configuration, and no longer a per-rule configuration. Experiments revealed that enabling packet logging for all reject rules only had a minor impact (5%) in performance. Therefore, logging is always enabled, unconditionally. Fixes antrea-io#6636. Signed-off-by: Qiyue Yao <[email protected]>
Current logs by Suricata when enableLogging is set, logs the wrong packet of RST instead of the original TCP packet, and remains an existing bug in Suricata.
This solution modifies the design to remove enableLogging in L7, keep it in L4, and instead configure Suricata to log Packet info for all alert events.
Also add Packet test to E2E tests.
Decode the packet
dtwWezuaHlOhfWpNgQAAAQgARQAAjbT0QABABm9cCgoBBAoKAQOJUgBQa2w1WZlax6yAGAH7nAIAAAEBCAorcsv8RSTwQkdFVCAvYWRtaW4vaW5kZXguaHRtbCBIVFRQLzEuMQ0KSG9zdDogMTAuMTAuMS4zDQpVc2VyLUFnZW50OiBjdXJsLzcuNzQuMA0KQWNjZXB0OiAqLyoNCg0K->
10.10.1.4 → 10.10.1.3 HTTP GET /admin/index.html HTTP/1.1Fixes #6636.