Skip to content

Commit c0fb233

Browse files
joshistejoshiste
committed
fix(netfault): replace root qdisc instead of add to support pre-existing qdiscs
Network attacks using tc (delay, loss, corruption, bandwidth) failed on hosts where the kernel had already attached a root qdisc to the target interface (e.g. `mq` on GKE COS, EKS, AKS, RHCOS). `tc qdisc add ... root` returned `NLM_F_REPLACE needed to override` and the attack could not start. Switch the root qdisc command to `tc qdisc replace ... root` on apply. On revert we still `qdisc del root`; the kernel then re-attaches its default qdisc (`mq` on multi-queue devices, `noqueue` on veth, otherwise the configured `net.core.default_qdisc`), so common cloud node setups are restored to their pre-attack state automatically. Add a preflight that runs `tc qdisc show` once per Apply, parses all root qdiscs into a map, and emits a warning for each affected interface whose root qdisc is not in the kernel-auto-restored allowlist (mq, noqueue, pfifo_fast, fq_codel, fq). Callers surface the warnings via the new `Apply` return value. Refactor the Opts interface to opt in to subsystem behavior via three optional providers, mirroring the existing iptablesScriptProvider: - tcCommandProvider (tcCommands + tcRootQdiscInterfaces) - ipCommandProvider (ipCommands) - iptablesScriptProvider (iptablesScripts) Each opts type now only implements the providers for the subsystems it actually uses, removing six `return nil, nil` stubs across blackhole, delay, loss, corruption, bandwidth, and tcp_reset. generateAndRunCommands and Apply discover behavior via type assertions, the same pattern already used for iptables scripts. Breaking changes: - `netfault.Apply` now returns `([]string, error)`. The string slice contains preflight warnings to surface to the user. - The `Opts` interface no longer requires `ipCommands` or `tcCommands`. External Opts implementations that returned `nil, nil` from these methods can simply remove them; external callers that consumed those methods need a type assertion first.
1 parent 9ad8185 commit c0fb233

19 files changed

Lines changed: 474 additions & 85 deletions

‎go/action_kit_commons/CHANGELOG.md‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,24 @@
11
# Changelog
22

3+
## 1.8.0
4+
5+
- netfault: use `tc qdisc replace` (instead of `add`) for the root qdisc in
6+
delay/loss/corruption/bandwidth attacks so they no longer fail on hosts
7+
with a pre-existing root qdisc (e.g. `mq` on GKE COS / EKS / AKS).
8+
- netfault: add preflight check that warns when an interface has a
9+
user-installed root qdisc (anything other than `mq`, `noqueue`,
10+
`pfifo_fast`, `fq_codel`, `fq`); the kernel default will be restored
11+
after revert in that case.
12+
- **Breaking:** `netfault.Apply` now returns `([]string, error)` — the
13+
string slice contains preflight warnings to surface to the user.
14+
- **Breaking:** The `Opts` interface no longer requires `ipCommands` or
15+
`tcCommands`. Subsystem behavior is now opt-in via two new optional
16+
interfaces: `tcCommandProvider` (`tcCommands` + `tcRootQdiscInterfaces`)
17+
and `ipCommandProvider` (`ipCommands`), mirroring the existing
18+
`iptablesScriptProvider`. External `Opts` implementations that returned
19+
`nil, nil` from these methods can simply remove them; external callers
20+
that consumed those methods need a type assertion first.
21+
322
## 1.6.1
423

524
- Add UseMangleChain to TcpResetOpts to enable tcp reset on istio

‎go/action_kit_commons/network/netfault/bandwidth.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -46,8 +46,8 @@ func (o *LimitBandwidthOpts) doesConflictWith(opts Opts) bool {
4646
return false
4747
}
4848

49-
func (o *LimitBandwidthOpts) ipCommands(_ family, _ mode) ([]string, error) {
50-
return nil, nil
49+
func (o *LimitBandwidthOpts) tcRootQdiscInterfaces() []string {
50+
return o.Interfaces
5151
}
5252

5353
func (o *LimitBandwidthOpts) tcCommands(mode mode) ([]string, error) {
@@ -63,7 +63,7 @@ func (o *LimitBandwidthOpts) tcCommands(mode mode) ([]string, error) {
6363

6464
filter := optimizeFilter(o.Filter)
6565
for _, ifc := range o.Interfaces {
66-
cmds = append(cmds, fmt.Sprintf("qdisc %s dev %s root handle 1: htb default 30", mode, ifc))
66+
cmds = append(cmds, fmt.Sprintf("qdisc %s dev %s root handle 1: htb default 30", rootQdiscVerb(mode), ifc))
6767
cmds = append(cmds, fmt.Sprintf("class %s dev %s parent 1: classid %s htb rate %s", mode, ifc, handleInclude, o.Bandwidth))
6868

6969
filterCmds, err := tcCommandsForFilter(mode, filter, ifc)

‎go/action_kit_commons/network/netfault/bandwidth_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ func TestLimitBandwidthOpts_TcCommands(t *testing.T) {
6161
Bandwidth: "100mbit",
6262
Interfaces: []string{"eth0"},
6363
},
64-
wantAdd: []byte(`qdisc add dev eth0 root handle 1: htb default 30
64+
wantAdd: []byte(`qdisc replace dev eth0 root handle 1: htb default 30
6565
class add dev eth0 parent 1: classid 1:3 htb rate 100mbit
6666
filter add dev eth0 protocol ip parent 1: prio 1 u32 match ip src 192.168.2.1/32 match ip sport 80 0xffff flowid 1:1
6767
filter add dev eth0 protocol ip parent 1: prio 2 u32 match ip dst 192.168.2.1/32 match ip dport 80 0xffff flowid 1:1

‎go/action_kit_commons/network/netfault/blackhole.go‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -83,10 +83,6 @@ func (o *BlackholeOpts) ipCommands(family family, mode mode) ([]string, error) {
8383
return cmds, nil
8484
}
8585

86-
func (o *BlackholeOpts) tcCommands(_ mode) ([]string, error) {
87-
return nil, nil
88-
}
89-
9086
func (o *BlackholeOpts) String() string {
9187
var sb strings.Builder
9288
sb.WriteString("blocking traffic ")

‎go/action_kit_commons/network/netfault/delay.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -59,8 +59,8 @@ func (o *DelayOpts) doesConflictWith(opts Opts) bool {
5959
return false
6060
}
6161

62-
func (o *DelayOpts) ipCommands(_ family, _ mode) ([]string, error) {
63-
return nil, nil
62+
func (o *DelayOpts) tcRootQdiscInterfaces() []string {
63+
return o.Interfaces
6464
}
6565

6666
const steadybitDelayFwMark uint32 = 0x1
@@ -174,7 +174,7 @@ func (o *DelayOpts) tcCommands(mode mode) ([]string, error) {
174174

175175
filter := optimizeFilter(o.Filter)
176176
for _, ifc := range o.Interfaces {
177-
cmds = append(cmds, fmt.Sprintf("qdisc %s dev %s root handle 1: prio priomap 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0", mode, ifc))
177+
cmds = append(cmds, fmt.Sprintf("qdisc %s dev %s root handle 1: prio priomap 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0", rootQdiscVerb(mode), ifc))
178178
cmds = append(cmds, fmt.Sprintf("qdisc %s dev %s parent %s handle 30: netem delay %dms %dms", mode, ifc, handleInclude, o.Delay.Milliseconds(), o.Jitter.Milliseconds()))
179179

180180
if o.TcpPshOnly {

‎go/action_kit_commons/network/netfault/delay_test.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ func TestDelayOpts_TcCommands(t *testing.T) {
5555
Jitter: 10 * time.Millisecond,
5656
Interfaces: []string{"eth0"},
5757
},
58-
wantAdd: []byte(`qdisc add dev eth0 root handle 1: prio priomap 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0
58+
wantAdd: []byte(`qdisc replace dev eth0 root handle 1: prio priomap 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0
5959
qdisc add dev eth0 parent 1:3 handle 30: netem delay 100ms 10ms
6060
filter add dev eth0 protocol ip parent 1: prio 1 u32 match ip src 192.168.2.1/32 match ip sport 80 0xffff flowid 1:1
6161
filter add dev eth0 protocol ip parent 1: prio 2 u32 match ip dst 192.168.2.1/32 match ip dport 80 0xffff flowid 1:1
@@ -112,7 +112,7 @@ qdisc del dev eth0 root handle 1: prio priomap 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0
112112
Interfaces: []string{"eth0"},
113113
TcpPshOnly: true,
114114
},
115-
wantAdd: []byte(`qdisc add dev eth0 root handle 1: prio priomap 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0
115+
wantAdd: []byte(`qdisc replace dev eth0 root handle 1: prio priomap 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0
116116
qdisc add dev eth0 parent 1:3 handle 30: netem delay 100ms 10ms
117117
filter add dev eth0 protocol ip parent 1: prio 1 handle 0x1 fw flowid 1:3
118118
filter add dev eth0 protocol ipv6 parent 1: prio 2 handle 0x1 fw flowid 1:3
@@ -141,7 +141,7 @@ qdisc del dev eth0 root handle 1: prio priomap 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0
141141
Jitter: 10 * time.Millisecond,
142142
Interfaces: []string{"eth0"},
143143
},
144-
wantAdd: []byte(`qdisc add dev eth0 root handle 1: prio priomap 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0
144+
wantAdd: []byte(`qdisc replace dev eth0 root handle 1: prio priomap 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0
145145
qdisc add dev eth0 parent 1:3 handle 30: netem delay 100ms 10ms
146146
filter add dev eth0 protocol ip parent 1: prio 1 u32 match ip src 192.168.2.1/32 match ip sport 80 0xffff flowid 1:1
147147
filter add dev eth0 protocol ip parent 1: prio 2 u32 match ip dst 192.168.2.1/32 match ip dport 80 0xffff flowid 1:1

‎go/action_kit_commons/network/netfault/netfault.go‎

Lines changed: 25 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -42,31 +42,43 @@ type CommandRunner interface {
4242
id() string
4343
}
4444

45-
func Apply(ctx context.Context, runner CommandRunner, opts Opts) error {
46-
return generateAndRunCommands(ctx, runner, opts, modeAdd)
45+
// Apply installs the attack. The returned warnings describe pre-existing
46+
// root qdiscs that the kernel will not auto-restore on Revert.
47+
func Apply(ctx context.Context, runner CommandRunner, opts Opts) ([]string, error) {
48+
var interfaces []string
49+
if p, ok := opts.(tcCommandProvider); ok {
50+
interfaces = p.tcRootQdiscInterfaces()
51+
}
52+
warnings := preflightWarnings(ctx, runner, interfaces)
53+
if err := generateAndRunCommands(ctx, runner, opts, modeAdd); err != nil {
54+
return warnings, err
55+
}
56+
return warnings, nil
4757
}
4858

4959
func Revert(ctx context.Context, runner CommandRunner, opts Opts) error {
5060
return generateAndRunCommands(ctx, runner, opts, modeDelete)
5161
}
5262

5363
func generateAndRunCommands(ctx context.Context, runner CommandRunner, opts Opts, mode mode) error {
54-
ipCommandsV4, err := opts.ipCommands(familyV4, mode)
55-
if err != nil {
56-
return err
57-
}
64+
var ipCommandsV4, ipCommandsV6, tcCommands []string
65+
var err error
5866

59-
var ipCommandsV6 []string
60-
if ipv6Supported() {
61-
ipCommandsV6, err = opts.ipCommands(familyV6, mode)
62-
if err != nil {
67+
if p, ok := opts.(ipCommandProvider); ok {
68+
if ipCommandsV4, err = p.ipCommands(familyV4, mode); err != nil {
6369
return err
6470
}
71+
if ipv6Supported() {
72+
if ipCommandsV6, err = p.ipCommands(familyV6, mode); err != nil {
73+
return err
74+
}
75+
}
6576
}
6677

67-
tcCommands, err := opts.tcCommands(mode)
68-
if err != nil {
69-
return err
78+
if p, ok := opts.(tcCommandProvider); ok {
79+
if tcCommands, err = p.tcCommands(mode); err != nil {
80+
return err
81+
}
7082
}
7183

7284
if log.Debug().Enabled() {

‎go/action_kit_commons/network/netfault/netfault_test.go‎

Lines changed: 88 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -14,23 +14,6 @@ import (
1414
"github.com/stretchr/testify/assert"
1515
)
1616

17-
type fakeRunner struct {
18-
calls []struct {
19-
args []string
20-
cmds []string
21-
}
22-
}
23-
24-
func (f *fakeRunner) run(_ context.Context, args []string, cmds []string) (string, error) {
25-
f.calls = append(f.calls, struct {
26-
args []string
27-
cmds []string
28-
}{args: args, cmds: cmds})
29-
return "", nil
30-
}
31-
32-
func (f *fakeRunner) id() string { return "testns" }
33-
3417
func TestApply_Order_IptablesBeforeTcWhenTcpPshOnly(t *testing.T) {
3518
// Disable ipv6 for the test to avoid ip6tables invocation
3619
ipv6Supported = func() bool { return false }
@@ -45,7 +28,7 @@ func TestApply_Order_IptablesBeforeTcWhenTcpPshOnly(t *testing.T) {
4528
}
4629

4730
r := &fakeRunner{}
48-
err := Apply(context.Background(), r, opts)
31+
_, err := Apply(context.Background(), r, opts)
4932
assert.NoError(t, err)
5033

5134
iptablesIdx := -1
@@ -54,7 +37,7 @@ func TestApply_Order_IptablesBeforeTcWhenTcpPshOnly(t *testing.T) {
5437
if len(c.args) > 0 && c.args[0] == "iptables-restore" {
5538
iptablesIdx = i
5639
}
57-
if len(c.args) > 0 && c.args[0] == "tc" && len(c.cmds) > 0 && strings.HasPrefix(c.cmds[0], "qdisc add") {
40+
if len(c.args) > 0 && c.args[0] == "tc" && len(c.cmds) > 0 && strings.HasPrefix(c.cmds[0], "qdisc replace") {
5841
tcBatchIdx = i
5942
}
6043
}
@@ -67,6 +50,92 @@ func TestApply_Order_IptablesBeforeTcWhenTcpPshOnly(t *testing.T) {
6750
}
6851
}
6952

53+
func TestApply_ReturnsPreflightWarnings(t *testing.T) {
54+
ipv6Supported = func() bool { return false }
55+
defer func() { ipv6Supported = defaultIpv6Supported }()
56+
57+
tests := []struct {
58+
name string
59+
interfaces []string
60+
tcOutput string
61+
wantWarnings int
62+
wantSubstr string
63+
}{
64+
{
65+
name: "kernel default (mq) — no warning",
66+
interfaces: []string{"eth0"},
67+
tcOutput: `qdisc mq 8002: dev eth0 root`,
68+
wantWarnings: 0,
69+
},
70+
{
71+
name: "user-installed (htb) — warning",
72+
interfaces: []string{"eth0"},
73+
tcOutput: `qdisc htb 1: dev eth0 root refcnt 2 r2q 10 default 0x30`,
74+
wantWarnings: 1,
75+
wantSubstr: `"htb"`,
76+
},
77+
{
78+
name: "two interfaces, one user-installed",
79+
interfaces: []string{"eth0", "eth1"},
80+
tcOutput: `qdisc mq 0: dev eth0 root
81+
qdisc cake 8001: dev eth1 root refcnt 2 bandwidth 1Gbit`,
82+
wantWarnings: 1,
83+
wantSubstr: `"cake"`,
84+
},
85+
}
86+
87+
for _, tt := range tests {
88+
t.Run(tt.name, func(t *testing.T) {
89+
opts := &DelayOpts{
90+
Filter: Filter{Include: []network.NetWithPortRange{mustParseNetWithPortRange("0.0.0.0/0", "*")}},
91+
Delay: 100 * time.Millisecond,
92+
Interfaces: tt.interfaces,
93+
}
94+
95+
// Unique netNsId per subtest so activeNetfault state does not leak.
96+
r := &fakeRunner{netNsId: tt.name, stdout: tt.tcOutput}
97+
warnings, err := Apply(context.Background(), r, opts)
98+
assert.NoError(t, err)
99+
assert.Len(t, warnings, tt.wantWarnings)
100+
if tt.wantWarnings > 0 && tt.wantSubstr != "" {
101+
assert.Contains(t, warnings[0], tt.wantSubstr)
102+
}
103+
})
104+
}
105+
}
106+
107+
// TestOptsProviderMatrix locks down which subsystem providers each Opts type
108+
// implements. Generated commands flow through type assertions in
109+
// generateAndRunCommands, so accidentally adding tcCommands to BlackholeOpts
110+
// (or any similar mistake) would silently start hitting a different code
111+
// path without any other test failing.
112+
func TestOptsProviderMatrix(t *testing.T) {
113+
cases := []struct {
114+
name string
115+
opts Opts
116+
hasIp bool
117+
hasTc bool
118+
hasIptables bool
119+
}{
120+
{"BlackholeOpts", &BlackholeOpts{}, true, false, false},
121+
{"DelayOpts", &DelayOpts{}, false, true, true},
122+
{"PackageLossOpts", &PackageLossOpts{}, false, true, false},
123+
{"CorruptPackagesOpts", &CorruptPackagesOpts{}, false, true, false},
124+
{"LimitBandwidthOpts", &LimitBandwidthOpts{}, false, true, false},
125+
{"TcpResetOpts", &TcpResetOpts{}, false, false, true},
126+
}
127+
for _, tt := range cases {
128+
t.Run(tt.name, func(t *testing.T) {
129+
_, isIp := tt.opts.(ipCommandProvider)
130+
_, isTc := tt.opts.(tcCommandProvider)
131+
_, isIptables := tt.opts.(iptablesScriptProvider)
132+
assert.Equal(t, tt.hasIp, isIp, "ipCommandProvider")
133+
assert.Equal(t, tt.hasTc, isTc, "tcCommandProvider")
134+
assert.Equal(t, tt.hasIptables, isIptables, "iptablesScriptProvider")
135+
})
136+
}
137+
}
138+
70139
func TestDelayOpts_IptablesScripts_FilterByFamily(t *testing.T) {
71140
opts := &DelayOpts{
72141
Filter: Filter{

‎go/action_kit_commons/network/netfault/packageCorruption.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -45,16 +45,16 @@ func (o *CorruptPackagesOpts) doesConflictWith(opts Opts) bool {
4545
return false
4646
}
4747

48-
func (o *CorruptPackagesOpts) ipCommands(_ family, _ mode) ([]string, error) {
49-
return nil, nil
48+
func (o *CorruptPackagesOpts) tcRootQdiscInterfaces() []string {
49+
return o.Interfaces
5050
}
5151

5252
func (o *CorruptPackagesOpts) tcCommands(mode mode) ([]string, error) {
5353
var cmds []string
5454

5555
filter := optimizeFilter(o.Filter)
5656
for _, ifc := range o.Interfaces {
57-
cmds = append(cmds, fmt.Sprintf("qdisc %s dev %s root handle 1: prio priomap 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0", mode, ifc))
57+
cmds = append(cmds, fmt.Sprintf("qdisc %s dev %s root handle 1: prio priomap 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0", rootQdiscVerb(mode), ifc))
5858
cmds = append(cmds, fmt.Sprintf("qdisc %s dev %s parent %s handle 30: netem corrupt %d%%", mode, ifc, handleInclude, o.Corruption))
5959

6060
filterCmds, err := tcCommandsForFilter(mode, filter, ifc)

‎go/action_kit_commons/network/netfault/packageCorruption_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ func TestCorruptPackagesOpts_TcCommands(t *testing.T) {
3838
Corruption: 90,
3939
Interfaces: []string{"eth0"},
4040
},
41-
wantAdd: []byte(`qdisc add dev eth0 root handle 1: prio priomap 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0
41+
wantAdd: []byte(`qdisc replace dev eth0 root handle 1: prio priomap 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0
4242
qdisc add dev eth0 parent 1:3 handle 30: netem corrupt 90%
4343
filter add dev eth0 protocol ip parent 1: prio 1 u32 match ip src 192.168.2.1/32 match ip sport 80 0xffff flowid 1:1
4444
filter add dev eth0 protocol ip parent 1: prio 2 u32 match ip dst 192.168.2.1/32 match ip dport 80 0xffff flowid 1:1

0 commit comments

Comments
 (0)