From 009dc8dd572cf795231404b7a4f78a1aad99ffcf Mon Sep 17 00:00:00 2001 From: Qiyue Yao Date: Wed, 4 Sep 2024 14:19:02 -0700 Subject: [PATCH] Fix L7NP enable logging wrong packet 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 #6636. Signed-off-by: Qiyue Yao --- docs/antrea-l7-network-policy.md | 39 ++++++++++--------- .../networkpolicy/l7engine/reconciler.go | 22 ++++------- .../networkpolicy/l7engine/reconciler_test.go | 4 +- .../networkpolicy/networkpolicy_controller.go | 6 +-- test/e2e/l7networkpolicy_test.go | 36 +++++++++++++++-- 5 files changed, 65 insertions(+), 42 deletions(-) diff --git a/docs/antrea-l7-network-policy.md b/docs/antrea-l7-network-policy.md index 1f917de0152..62fecd80d20 100644 --- a/docs/antrea-l7-network-policy.md +++ b/docs/antrea-l7-network-policy.md @@ -329,20 +329,20 @@ Allow ingress from client (10.10.1.9) to web (10.10.1.10/public/*). } ``` -Deny ingress from client (10.10.1.9) to web (10.10.1.10/admin/*) +Deny ingress from client (10.10.1.4) to web (10.10.1.3/admin/*). ```json { - "timestamp": "2024-08-26T22:38:26.019956+0000", - "flow_id": 642636870504569, + "timestamp": "2024-09-05T22:49:24.788756+0000", + "flow_id": 1131530446896560, "in_iface": "antrea-l7-tap0", "event_type": "alert", "vlan": [ 2 ], - "src_ip": "10.10.1.9", - "src_port": 37892, - "dest_ip": "10.10.1.10", + "src_ip": "10.10.1.4", + "src_port": 45034, + "dest_ip": "10.10.1.3", "dest_port": 80, "proto": "TCP", "pkt_src": "wire/pcap", @@ -362,36 +362,37 @@ Deny ingress from client (10.10.1.9) to web (10.10.1.10/admin/*) "flow": { "pkts_toserver": 3, "pkts_toclient": 1, - "bytes_toserver": 308, + "bytes_toserver": 307, "bytes_toclient": 78, - "start": "2024-08-26T22:38:26.018553+0000", - "src_ip": "10.10.1.9", - "dest_ip": "10.10.1.10", - "src_port": 37892, + "start": "2024-09-05T22:49:24.787742+0000", + "src_ip": "10.10.1.4", + "dest_ip": "10.10.1.3", + "src_port": 45034, "dest_port": 80 } } ``` -Additional packet log when `enableLogging` is set +Additional packet logs are available when `enableLogging` is set, which tracks all +packets in Suricata matching the dst IP address of the packet generating the alert. ```json { - "timestamp": "2024-08-26T22:38:26.025742+0000", - "flow_id": 642636870504569, + "timestamp": "2024-09-05T22:49:24.788756+0000", + "flow_id": 1131530446896560, "in_iface": "antrea-l7-tap0", "event_type": "packet", "vlan": [ 2 ], - "src_ip": "10.10.1.10", - "src_port": 80, - "dest_ip": "10.10.1.9", - "dest_port": 37892, + "src_ip": "10.10.1.4", + "src_port": 45034, + "dest_ip": "10.10.1.3", + "dest_port": 80, "proto": "TCP", "pkt_src": "wire/pcap", "tenant_id": 2, - "packet": "/hYGSsKknh8fnhcggQAAAggARQAAKN7MAABABoXdCgoBCgoKAQkAUJQE0EfjHLfFVXZQFAH7QroAAA==", + "packet": "dtwWezuaHlOhfWpNgQAAAggARQAAjU/0QABABtRcCgoBBAoKAQOv6gBQgOZTvPTauPuAGAH7TZcAAAEBCAouFZzsR8fBM0dFVCAvYWRtaW4vaW5kZXguaHRtbCBIVFRQLzEuMQ0KSG9zdDogMTAuMTAuMS4zDQpVc2VyLUFnZW50OiBjdXJsLzcuNzQuMA0KQWNjZXB0OiAqLyoNCg0K", "packet_info": { "linktype": 1 } diff --git a/pkg/agent/controller/networkpolicy/l7engine/reconciler.go b/pkg/agent/controller/networkpolicy/l7engine/reconciler.go index 56523773662..079a7c42844 100644 --- a/pkg/agent/controller/networkpolicy/l7engine/reconciler.go +++ b/pkg/agent/controller/networkpolicy/l7engine/reconciler.go @@ -78,10 +78,9 @@ outputs: enabled: no types: - alert: - tagged-packets: yes + packet: yes - http: extended: yes - tagged-packets: yes - tls: extended: yes - eve-log: @@ -170,19 +169,12 @@ func NewReconciler() *Reconciler { } } -func generateTenantRulesData(policyName string, protoKeywords map[string]sets.Set[string], enableLogging bool) *bytes.Buffer { +func generateTenantRulesData(policyName string, protoKeywords map[string]sets.Set[string]) *bytes.Buffer { rulesData := bytes.NewBuffer(nil) sid := 1 - // Enable logging of packets in the session that set off the rule, the session is tagged for 30 seconds. - // Refer to Suricata detect engine in codebase for detailed tag keyword configuration. - var tagKeyword string - if enableLogging { - tagKeyword = " tag: session, 30, seconds;" - } - // Generate default reject rule. - allKeywords := fmt.Sprintf(`msg: "Reject by %s"; flow: to_server, established;%s sid: %d;`, policyName, tagKeyword, sid) + allKeywords := fmt.Sprintf(`msg: "Reject by %s"; flow: to_server, established; sid: %d;`, policyName, sid) rule := fmt.Sprintf("reject ip any any -> any any (%s)\n", allKeywords) rulesData.WriteString(rule) sid++ @@ -193,9 +185,9 @@ func generateTenantRulesData(policyName string, protoKeywords map[string]sets.Se // It is a convention that the sid is provided as the last keyword (or second-to-last if there is a rev) // of a rule. if keywords != "" { - allKeywords = fmt.Sprintf(`msg: "Allow %s by %s"; %s%s sid: %d;`, proto, policyName, keywords, tagKeyword, sid) + allKeywords = fmt.Sprintf(`msg: "Allow %s by %s"; %s sid: %d;`, proto, policyName, keywords, sid) } else { - allKeywords = fmt.Sprintf(`msg: "Allow %s by %s";%s sid: %d;`, proto, policyName, tagKeyword, sid) + allKeywords = fmt.Sprintf(`msg: "Allow %s by %s"; sid: %d;`, proto, policyName, sid) } rule = fmt.Sprintf("pass %s any any -> any any (%s)\n", proto, allKeywords) rulesData.WriteString(rule) @@ -274,7 +266,7 @@ func (r *Reconciler) StartSuricataOnce() { }) } -func (r *Reconciler) AddRule(ruleID, policyName string, vlanID uint32, l7Protocols []v1beta.L7Protocol, enableLogging bool) error { +func (r *Reconciler) AddRule(ruleID, policyName string, vlanID uint32, l7Protocols []v1beta.L7Protocol) error { start := time.Now() defer func() { klog.V(5).Infof("AddRule took %v", time.Since(start)) @@ -304,7 +296,7 @@ func (r *Reconciler) AddRule(ruleID, policyName string, vlanID uint32, l7Protoco klog.InfoS("Reconciling L7 rule", "RuleID", ruleID, "PolicyName", policyName) // Write the Suricata rules to file. rulesPath := generateTenantRulesPath(vlanID) - rulesData := generateTenantRulesData(policyName, protoKeywords, enableLogging) + rulesData := generateTenantRulesData(policyName, protoKeywords) if err := writeConfigFile(rulesPath, rulesData); err != nil { return fmt.Errorf("failed to write Suricata rules data to file %s for L7 rule %s of %s, err: %w", rulesPath, ruleID, policyName, err) } diff --git a/pkg/agent/controller/networkpolicy/l7engine/reconciler_test.go b/pkg/agent/controller/networkpolicy/l7engine/reconciler_test.go index 340547cfba1..39a1745fd80 100644 --- a/pkg/agent/controller/networkpolicy/l7engine/reconciler_test.go +++ b/pkg/agent/controller/networkpolicy/l7engine/reconciler_test.go @@ -189,7 +189,7 @@ func TestRuleLifecycle(t *testing.T) { fe.startSuricataFn = fs.startSuricataFn // Test add a L7 NetworkPolicy. - assert.NoError(t, fe.AddRule(ruleID, policyName, vlanID, tc.l7Protocols, false)) + assert.NoError(t, fe.AddRule(ruleID, policyName, vlanID, tc.l7Protocols)) rulesPath := generateTenantRulesPath(vlanID) ok, err := afero.FileContainsBytes(defaultFS, rulesPath, []byte(tc.expectedRules)) @@ -206,7 +206,7 @@ func TestRuleLifecycle(t *testing.T) { assert.Equal(t, expectedScCommands, fs.calledScCommands) // Update the added L7 NetworkPolicy. - assert.NoError(t, fe.AddRule(ruleID, policyName, vlanID, tc.updatedL7Protocols, false)) + assert.NoError(t, fe.AddRule(ruleID, policyName, vlanID, tc.updatedL7Protocols)) expectedScCommands.Insert("reload-tenant 1 /etc/suricata/antrea-tenant-1.yaml") assert.Equal(t, expectedScCommands, fs.calledScCommands) diff --git a/pkg/agent/controller/networkpolicy/networkpolicy_controller.go b/pkg/agent/controller/networkpolicy/networkpolicy_controller.go index b95a8e1ec0f..c07707f8d8a 100644 --- a/pkg/agent/controller/networkpolicy/networkpolicy_controller.go +++ b/pkg/agent/controller/networkpolicy/networkpolicy_controller.go @@ -72,7 +72,7 @@ const ( ) type L7RuleReconciler interface { - AddRule(ruleID, policyName string, vlanID uint32, l7Protocols []v1beta2.L7Protocol, enableLogging bool) error + AddRule(ruleID, policyName string, vlanID uint32, l7Protocols []v1beta2.L7Protocol) error DeleteRule(ruleID string, vlanID uint32) error } @@ -797,7 +797,7 @@ func (c *Controller) syncRule(key string) error { vlanID := c.l7VlanIDAllocator.allocate(key) rule.L7RuleVlanID = &vlanID - if err := c.l7RuleReconciler.AddRule(key, rule.SourceRef.ToString(), vlanID, rule.L7Protocols, rule.EnableLogging); err != nil { + if err := c.l7RuleReconciler.AddRule(key, rule.SourceRef.ToString(), vlanID, rule.L7Protocols); err != nil { return err } } @@ -852,7 +852,7 @@ func (c *Controller) syncRules(keys []string) error { vlanID := c.l7VlanIDAllocator.allocate(key) rule.L7RuleVlanID = &vlanID - if err := c.l7RuleReconciler.AddRule(key, rule.SourceRef.ToString(), vlanID, rule.L7Protocols, rule.EnableLogging); err != nil { + if err := c.l7RuleReconciler.AddRule(key, rule.SourceRef.ToString(), vlanID, rule.L7Protocols); err != nil { return err } } diff --git a/test/e2e/l7networkpolicy_test.go b/test/e2e/l7networkpolicy_test.go index a2b6090c56f..b9f2bbbfce4 100644 --- a/test/e2e/l7networkpolicy_test.go +++ b/test/e2e/l7networkpolicy_test.go @@ -16,10 +16,12 @@ package e2e import ( "context" + "encoding/base64" + "encoding/hex" "encoding/json" "fmt" "net" - "reflect" + "regexp" "slices" "strings" "testing" @@ -28,6 +30,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/conversion" "k8s.io/apimachinery/pkg/util/wait" crdv1beta1 "antrea.io/antrea/pkg/apis/crd/v1beta1" @@ -401,7 +404,11 @@ func testL7NetworkPolicyLogging(t *testing.T, data *TestData) { DestPort: 8080, Protocol: "TCP", AppProtocol: "http", - Alert: &L7LogAlertEntry{Action: "blocked", Signature: fmt.Sprintf("Reject by AntreaNetworkPolicy:%s/%s", data.testNamespace, policyAllowPathHostname)}, + Packet: fmt.Sprintf("%s|HTTP|GET|%s", ip.String(), "/clientip"), + Alert: &L7LogAlertEntry{ + Action: "blocked", + Signature: fmt.Sprintf("Reject by AntreaNetworkPolicy:%s/%s", data.testNamespace, policyAllowPathHostname), + }, } hostMatcher := &L7LogEntry{ EventType: "http", @@ -434,12 +441,35 @@ type L7LogEntry struct { DestPort int32 `json:"dest_port"` Protocol string `json:"proto"` AppProtocol string `json:"app_proto,omitempty"` + Packet string `json:"packet"` Http *L7LogHttpEntry `json:"http,omitempty"` Alert *L7LogAlertEntry `json:"alert,omitempty"` } +// Compares if two L7LogEntry are equal. They must be exactly equal if Packet is +// in the same base. Otherwise, if either of the Packet is in base64, they equal +// if Packet in base64 contains essential message in other Packet. func (e *L7LogEntry) Equal(x *L7LogEntry) bool { - return reflect.DeepEqual(e, x) + packetLooseEqual := conversion.EqualitiesOrDie( + func(a, b L7LogEntry) bool { + if a != b { + originalText := b.Packet + decodedBytes, err := base64.StdEncoding.DecodeString(a.Packet) + if err != nil { + decodedBytes, err = base64.StdEncoding.DecodeString(b.Packet) + if err != nil { + return false + } + originalText = a.Packet + } + decoded, err := hex.DecodeString(hex.EncodeToString(decodedBytes)) + matcher := regexp.MustCompile(originalText) + return err == nil && matcher.MatchString(string(decoded)) + } + return true + }, + ) + return packetLooseEqual.DeepEqual(e, x) } func (e *L7LogEntry) String() string {