From 2ba7f1608fe53821fa407d3ef4ef1b646f9fa06a Mon Sep 17 00:00:00 2001 From: Phil Sutter Date: Thu, 1 Jun 2023 14:08:27 +0200 Subject: [PATCH] spoofcheck: Make use of go-nft's ApplyConfigEcho() Store the relevant applied config part for later to extract the rule to delete from there instead of having to list the ruleset. This is much faster especially with large rulesets. Signed-off-by: Phil Sutter --- go.mod | 2 +- go.sum | 4 +- pkg/link/spoofcheck.go | 68 ++++++++++++------- pkg/link/spoofcheck_test.go | 34 ++++++++-- .../networkplumbing/go-nft/nft/config.go | 7 ++ .../networkplumbing/go-nft/nft/exec/exec.go | 58 ++++++++++------ vendor/modules.txt | 2 +- 7 files changed, 120 insertions(+), 55 deletions(-) diff --git a/go.mod b/go.mod index 306a8095..07dd5f93 100644 --- a/go.mod +++ b/go.mod @@ -14,7 +14,7 @@ require ( github.com/d2g/dhcp4server v0.0.0-20181031114812-7d4a0a7f59a5 github.com/godbus/dbus/v5 v5.1.0 github.com/mattn/go-shellwords v1.0.12 - github.com/networkplumbing/go-nft v0.3.0 + github.com/networkplumbing/go-nft v0.4.0 github.com/onsi/ginkgo/v2 v2.11.0 github.com/onsi/gomega v1.27.8 github.com/opencontainers/selinux v1.11.0 diff --git a/go.sum b/go.sum index 984be415..089d3248 100644 --- a/go.sum +++ b/go.sum @@ -486,8 +486,8 @@ github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822/go.mod h1:+n7T8m github.com/mwitkow/go-conntrack v0.0.0-20161129095857-cc309e4a2223/go.mod h1:qRWi+5nqEBWmkhHvq77mSJWrCKwh8bxhgT7d/eI7P4U= github.com/mxk/go-flowrate v0.0.0-20140419014527-cca7078d478f/go.mod h1:ZdcZmHo+o7JKHSa8/e818NopupXU1YMK5fe1lsApnBw= github.com/ncw/swift v1.0.47/go.mod h1:23YIA4yWVnGwv2dQlN4bB7egfYX6YLn0Yo/S6zZO/ZM= -github.com/networkplumbing/go-nft v0.3.0 h1:IIc6yHjN85KyJx21p3ZEsO0iBMYHNXux22rc9Q8TfFw= -github.com/networkplumbing/go-nft v0.3.0/go.mod h1:HnnM+tYvlGAsMU7yoYwXEVLLiDW9gdMmb5HoGcwpuQs= +github.com/networkplumbing/go-nft v0.4.0 h1:kExVMwXW48DOAukkBwyI16h4uhE5lN9iMvQd52lpTyU= +github.com/networkplumbing/go-nft v0.4.0/go.mod h1:HnnM+tYvlGAsMU7yoYwXEVLLiDW9gdMmb5HoGcwpuQs= github.com/niemeyer/pretty v0.0.0-20200227124842-a10e7caefd8e h1:fD57ERR4JtEqsWbfPhv4DMiApHyliiK5xCTNVSPiaAs= github.com/niemeyer/pretty v0.0.0-20200227124842-a10e7caefd8e/go.mod h1:zD1mROLANZcx1PVRCS0qkT7pwLkGfwJo4zjcN/Tysno= github.com/nxadm/tail v1.4.4/go.mod h1:kenIhsEOeOJmVchQTgglprH7qJGnHDVpk1VPCcaMI8A= diff --git a/pkg/link/spoofcheck.go b/pkg/link/spoofcheck.go index f22a6753..de9d04c2 100644 --- a/pkg/link/spoofcheck.go +++ b/pkg/link/spoofcheck.go @@ -30,7 +30,7 @@ const ( ) type NftConfigurer interface { - Apply(*nft.Config) error + Apply(*nft.Config) (*nft.Config, error) Read(filterCommands ...string) (*nft.Config, error) } @@ -39,12 +39,16 @@ type SpoofChecker struct { macAddress string refID string configurer NftConfigurer + rulestore *nft.Config } type defaultNftConfigurer struct{} -func (dnc defaultNftConfigurer) Apply(cfg *nft.Config) error { - return nft.ApplyConfig(cfg) +func (dnc defaultNftConfigurer) Apply(cfg *nft.Config) (*nft.Config, error) { + const timeout = 55 * time.Second + ctxWithTimeout, cancelFunc := context.WithTimeout(context.Background(), timeout) + defer cancelFunc() + return nft.ApplyConfigEcho(ctxWithTimeout, cfg) } func (dnc defaultNftConfigurer) Read(filterCommands ...string) (*nft.Config, error) { @@ -59,7 +63,7 @@ func NewSpoofChecker(iface, macAddress, refID string) *SpoofChecker { } func NewSpoofCheckerWithConfigurer(iface, macAddress, refID string, configurer NftConfigurer) *SpoofChecker { - return &SpoofChecker{iface, macAddress, refID, configurer} + return &SpoofChecker{iface, macAddress, refID, configurer, nil} } // Setup applies nftables configuration to restrict traffic @@ -88,7 +92,7 @@ func (sc *SpoofChecker) Setup() error { macChain := sc.macChain(ifaceChain.Name) baseConfig.AddChain(macChain) - if err := sc.configurer.Apply(baseConfig); err != nil { + if _, err := sc.configurer.Apply(baseConfig); err != nil { return fmt.Errorf("failed to setup spoof-check: %v", err) } @@ -102,37 +106,51 @@ func (sc *SpoofChecker) Setup() error { rulesConfig.AddRule(sc.matchMacRule(macChain.Name)) rulesConfig.AddRule(sc.dropRule(macChain.Name)) - if err := sc.configurer.Apply(rulesConfig); err != nil { + rulestore, err := sc.configurer.Apply(rulesConfig) + if err != nil { return fmt.Errorf("failed to setup spoof-check: %v", err) } + sc.rulestore = rulestore return nil } +func (sc *SpoofChecker) findPreroutingRule(ruleToFind *schema.Rule) ([]*schema.Rule, error) { + ruleset := sc.rulestore + if ruleset == nil { + chain, err := sc.configurer.Read(listChainBridgeNatPrerouting()...) + if err != nil { + return nil, err + } + ruleset = chain + } + return ruleset.LookupRule(ruleToFind), nil +} + // Teardown removes the interface and mac-address specific chains and their rules. // The table and base-chain are expected to survive while the base-chain rule that matches the // interface is removed. func (sc *SpoofChecker) Teardown() error { ifaceChain := sc.ifaceChain() - currentConfig, ifaceMatchRuleErr := sc.configurer.Read(listChainBridgeNatPrerouting()...) - if ifaceMatchRuleErr == nil { - expectedRuleToFind := sc.matchIfaceJumpToChainRule(preRoutingBaseChainName, ifaceChain.Name) - // It is safer to exclude the statement matching, avoiding cases where a current statement includes - // additional default entries (e.g. counters). - ruleToFindExcludingStatements := *expectedRuleToFind - ruleToFindExcludingStatements.Expr = nil - rules := currentConfig.LookupRule(&ruleToFindExcludingStatements) - if len(rules) > 0 { - c := nft.NewConfig() - for _, rule := range rules { - c.DeleteRule(rule) - } - if err := sc.configurer.Apply(c); err != nil { - ifaceMatchRuleErr = fmt.Errorf("failed to delete iface match rule: %v", err) - } - } else { - fmt.Fprintf(os.Stderr, "spoofcheck/teardown: unable to detect iface match rule for deletion: %+v", expectedRuleToFind) + expectedRuleToFind := sc.matchIfaceJumpToChainRule(preRoutingBaseChainName, ifaceChain.Name) + // It is safer to exclude the statement matching, avoiding cases where a current statement includes + // additional default entries (e.g. counters). + ruleToFindExcludingStatements := *expectedRuleToFind + ruleToFindExcludingStatements.Expr = nil + + rules, ifaceMatchRuleErr := sc.findPreroutingRule(&ruleToFindExcludingStatements) + if ifaceMatchRuleErr == nil && len(rules) > 0 { + c := nft.NewConfig() + for _, rule := range rules { + c.DeleteRule(rule) } + if _, err := sc.configurer.Apply(c); err != nil { + ifaceMatchRuleErr = fmt.Errorf("failed to delete iface match rule: %v", err) + } + // Drop the cache, it should contain deleted rule(s) now + sc.rulestore = nil + } else { + fmt.Fprintf(os.Stderr, "spoofcheck/teardown: unable to detect iface match rule for deletion: %+v", expectedRuleToFind) } regularChainsConfig := nft.NewConfig() @@ -140,7 +158,7 @@ func (sc *SpoofChecker) Teardown() error { regularChainsConfig.DeleteChain(sc.macChain(ifaceChain.Name)) var regularChainsErr error - if err := sc.configurer.Apply(regularChainsConfig); err != nil { + if _, err := sc.configurer.Apply(regularChainsConfig); err != nil { regularChainsErr = fmt.Errorf("failed to delete regular chains: %v", err) } diff --git a/pkg/link/spoofcheck_test.go b/pkg/link/spoofcheck_test.go index f26aa6a6..50899b69 100644 --- a/pkg/link/spoofcheck_test.go +++ b/pkg/link/spoofcheck_test.go @@ -113,6 +113,25 @@ var _ = Describe("spoofcheck", func() { ))) }) }) + + Context("echo", func() { + It("succeeds, no read called", func() { + c := configurerStub{} + sc := link.NewSpoofCheckerWithConfigurer(iface, mac, id, &c) + Expect(sc.Setup()).To(Succeed()) + Expect(sc.Teardown()).To(Succeed()) + Expect(c.readCalled).To(BeFalse()) + }) + + It("succeeds, fall back to config read", func() { + c := configurerStub{applyReturnNil: true} + sc := link.NewSpoofCheckerWithConfigurer(iface, mac, id, &c) + Expect(sc.Setup()).To(Succeed()) + c.readConfig = c.applyConfig[0] + Expect(sc.Teardown()).To(Succeed()) + Expect(c.readCalled).To(BeTrue()) + }) + }) }) func assertExpectedRegularChainsDeletionInTeardownConfig(action configurerStub) { @@ -274,21 +293,28 @@ type configurerStub struct { failFirstApplyConfig bool failSecondApplyConfig bool failReadConfig bool + + applyReturnNil bool + readCalled bool } -func (a *configurerStub) Apply(c *nft.Config) error { +func (a *configurerStub) Apply(c *nft.Config) (*nft.Config, error) { a.applyCounter++ if a.failFirstApplyConfig && a.applyCounter == 1 { - return fmt.Errorf(errorFirstApplyText) + return nil, fmt.Errorf(errorFirstApplyText) } if a.failSecondApplyConfig && a.applyCounter == 2 { - return fmt.Errorf(errorSecondApplyText) + return nil, fmt.Errorf(errorSecondApplyText) } a.applyConfig = append(a.applyConfig, c) - return nil + if a.applyReturnNil { + return nil, nil + } + return c, nil } func (a *configurerStub) Read(_ ...string) (*nft.Config, error) { + a.readCalled = true if a.failReadConfig { return nil, fmt.Errorf(errorReadText) } diff --git a/vendor/github.com/networkplumbing/go-nft/nft/config.go b/vendor/github.com/networkplumbing/go-nft/nft/config.go index 27ad5edb..c16ffc12 100644 --- a/vendor/github.com/networkplumbing/go-nft/nft/config.go +++ b/vendor/github.com/networkplumbing/go-nft/nft/config.go @@ -67,3 +67,10 @@ func ApplyConfig(c *Config) error { func ApplyConfigContext(ctx context.Context, c *Config) error { return nftexec.ApplyConfig(ctx, c) } + +// ApplyConfigEcho applies the given nftables config on the system, echoing +// back the added elements with their assigned handles +// The system is expected to have the `nft` executable deployed and nftables enabled in the kernel. +func ApplyConfigEcho(ctx context.Context, c *Config) (*Config, error) { + return nftexec.ApplyConfigEcho(ctx, c) +} diff --git a/vendor/github.com/networkplumbing/go-nft/nft/exec/exec.go b/vendor/github.com/networkplumbing/go-nft/nft/exec/exec.go index 9ce3d181..ad0d1b71 100644 --- a/vendor/github.com/networkplumbing/go-nft/nft/exec/exec.go +++ b/vendor/github.com/networkplumbing/go-nft/nft/exec/exec.go @@ -23,8 +23,6 @@ import ( "bytes" "context" "fmt" - "io/ioutil" - "os" "os/exec" "strings" @@ -33,22 +31,24 @@ import ( const ( cmdBin = "nft" + cmdHandle = "-a" + cmdEcho = "-e" cmdFile = "-f" cmdJSON = "-j" cmdList = "list" cmdRuleset = "ruleset" + cmdStdin = "-" ) // ReadConfig loads the nftables configuration from the system and // returns it as a nftables config structure. // The system is expected to have the `nft` executable deployed and nftables enabled in the kernel. func ReadConfig(ctx context.Context, filterCommands ...string) (*nftconfig.Config, error) { - whatToList := cmdRuleset if len(filterCommands) > 0 { whatToList = strings.Join(filterCommands, " ") } - stdout, err := execCommand(ctx, cmdJSON, cmdList, whatToList) + stdout, err := execCommand(ctx, nil, cmdJSON, cmdList, whatToList) if err != nil { return nil, err } @@ -69,38 +69,52 @@ func ApplyConfig(ctx context.Context, c *nftconfig.Config) error { return err } - tmpFile, err := ioutil.TempFile(os.TempDir(), "spoofcheck-") - if err != nil { - return fmt.Errorf("failed to create temporary file: %v", err) - } - defer os.Remove(tmpFile.Name()) - - if _, err = tmpFile.Write(data); err != nil { - return fmt.Errorf("failed to write to temporary file: %v", err) - } - - if err := tmpFile.Close(); err != nil { - return fmt.Errorf("failed to close temporary file: %v", err) - } - - if _, err := execCommand(ctx, cmdJSON, cmdFile, tmpFile.Name()); err != nil { + if _, err := execCommand(ctx, data, cmdJSON, cmdFile, cmdStdin); err != nil { return err } return nil } -func execCommand(ctx context.Context, args ...string) (*bytes.Buffer, error) { +// ApplyConfigEcho applies the given nftables config on the system, echoing +// back the added elements with their assigned handles +// The system is expected to have the `nft` executable deployed and nftables enabled in the kernel. +func ApplyConfigEcho(ctx context.Context, c *nftconfig.Config) (*nftconfig.Config, error) { + data, err := c.ToJSON() + if err != nil { + return nil, err + } + + stdout, err := execCommand(ctx, data, cmdHandle, cmdEcho, cmdJSON, cmdFile, cmdStdin) + if err != nil { + return nil, err + } + + config := nftconfig.New() + if err := config.FromJSON(stdout.Bytes()); err != nil { + return nil, fmt.Errorf("failed to parse echo: %v", err) + } + + return config, nil +} + +func execCommand(ctx context.Context, input []byte, args ...string) (*bytes.Buffer, error) { cmd := exec.CommandContext(ctx, cmdBin, args...) var stdout, stderr bytes.Buffer cmd.Stderr = &stderr cmd.Stdout = &stdout + if input != nil { + var stdin bytes.Buffer + stdin.Write(input) + cmd.Stdin = &stdin + } + if err := cmd.Run(); err != nil { return nil, fmt.Errorf( - "failed to execute %s %s: %v stdout:'%s' stderr:'%s'", - cmd.Path, strings.Join(cmd.Args, " "), err, stdout.String(), stderr.String(), + "failed to execute %s %s: %v stdin:'%s' stdout:'%s' stderr:'%s'", + cmd.Path, strings.Join(cmd.Args, " "), err, string(input), stdout.String(), stderr.String(), ) } diff --git a/vendor/modules.txt b/vendor/modules.txt index 0e14e56e..5ce66b0a 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -103,7 +103,7 @@ github.com/google/pprof/profile # github.com/mattn/go-shellwords v1.0.12 ## explicit; go 1.13 github.com/mattn/go-shellwords -# github.com/networkplumbing/go-nft v0.3.0 +# github.com/networkplumbing/go-nft v0.4.0 ## explicit; go 1.16 github.com/networkplumbing/go-nft/nft github.com/networkplumbing/go-nft/nft/config