Skip to content

Commit

Permalink
Fix L7NP enable logging wrong packet
Browse files Browse the repository at this point in the history 
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 <yaoq@vmware.com>
  • Loading branch information
@qiyueyao
qiyueyao committed Sep 20, 2024
1 parent b9f67f4 commit 009dc8d
Show file tree
Hide file tree
Showing 5 changed files with 65 additions and 42 deletions.
39 changes: 20 additions & 19 deletions docs/antrea-l7-network-policy.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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
}
Expand Down
22 changes: 7 additions & 15 deletions pkg/agent/controller/networkpolicy/l7engine/reconciler.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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++
Expand All @@ -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)
Expand Down Expand Up @@ -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))
Expand Down Expand Up @@ -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)
}
Expand Down
4 changes: 2 additions & 2 deletions pkg/agent/controller/networkpolicy/l7engine/reconciler_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand All @@ -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)

Expand Down
6 changes: 3 additions & 3 deletions pkg/agent/controller/networkpolicy/networkpolicy_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down Expand Up @@ -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
}
}
Expand Down Expand Up @@ -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
}
}
Expand Down
36 changes: 33 additions & 3 deletions test/e2e/l7networkpolicy_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,12 @@ package e2e

import (
"context"
"encoding/base64"
"encoding/hex"
"encoding/json"
"fmt"
"net"
"reflect"
"regexp"
"slices"
"strings"
"testing"
Expand All @@ -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"
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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 {
Expand Down

0 comments on commit 009dc8d

Please sign in to comment.