From e75969696752eabfd839e554dfc89c068273548e 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 | 2 + pkg/link/spoofcheck.go | 35 ++++++++++++----- pkg/link/spoofcheck_test.go | 8 ++-- .../networkplumbing/go-nft/nft/config.go | 7 ++++ .../networkplumbing/go-nft/nft/exec/exec.go | 38 +++++++++++++++++++ vendor/modules.txt | 2 +- 7 files changed, 78 insertions(+), 16 deletions(-) diff --git a/go.mod b/go.mod index 9ded6e9de..1237ff35d 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.3.1-0.20230602131639-734b9ba38567 github.com/onsi/ginkgo/v2 v2.9.2 github.com/onsi/gomega v1.27.6 github.com/opencontainers/selinux v1.11.0 diff --git a/go.sum b/go.sum index bbba7ba84..d9bab99c1 100644 --- a/go.sum +++ b/go.sum @@ -488,6 +488,8 @@ github.com/mxk/go-flowrate v0.0.0-20140419014527-cca7078d478f/go.mod h1:ZdcZmHo+ 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.3.1-0.20230602131639-734b9ba38567 h1:eh5vjjPoYcrhQpMFR98BA/1FG7oQpK+vBeVSdAWvTyQ= +github.com/networkplumbing/go-nft v0.3.1-0.20230602131639-734b9ba38567/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 f22a67534..9f207bddf 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,20 +106,31 @@ 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) StoredRules() (*nft.Config, error) { + if sc.rulestore != nil { + rulestore := sc.rulestore + sc.rulestore = nil + return rulestore, nil + } + return sc.configurer.Read(listChainBridgeNatPrerouting()...) +} + // 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 { + currentConfig, ifaceMatchRuleErr := sc.StoredRules() + if currentConfig != 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). @@ -127,7 +142,7 @@ func (sc *SpoofChecker) Teardown() error { for _, rule := range rules { c.DeleteRule(rule) } - if err := sc.configurer.Apply(c); err != nil { + if _, err := sc.configurer.Apply(c); err != nil { ifaceMatchRuleErr = fmt.Errorf("failed to delete iface match rule: %v", err) } } else { @@ -140,7 +155,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 f26aa6a62..a0eb5a270 100644 --- a/pkg/link/spoofcheck_test.go +++ b/pkg/link/spoofcheck_test.go @@ -276,16 +276,16 @@ type configurerStub struct { failReadConfig 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 + return c, nil } func (a *configurerStub) Read(_ ...string) (*nft.Config, error) { diff --git a/vendor/github.com/networkplumbing/go-nft/nft/config.go b/vendor/github.com/networkplumbing/go-nft/nft/config.go index 27ad5edb7..c16ffc127 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 9ce3d181f..72c6b396d 100644 --- a/vendor/github.com/networkplumbing/go-nft/nft/exec/exec.go +++ b/vendor/github.com/networkplumbing/go-nft/nft/exec/exec.go @@ -33,6 +33,8 @@ import ( const ( cmdBin = "nft" + cmdHandle = "-a" + cmdEcho = "-e" cmdFile = "-f" cmdJSON = "-j" cmdList = "list" @@ -90,6 +92,42 @@ func ApplyConfig(ctx context.Context, c *nftconfig.Config) error { return nil } +// 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 + } + + tmpFile, err := ioutil.TempFile(os.TempDir(), "spoofcheck-") + if err != nil { + return nil, fmt.Errorf("failed to create temporary file: %v", err) + } + defer os.Remove(tmpFile.Name()) + + if _, err = tmpFile.Write(data); err != nil { + return nil, fmt.Errorf("failed to write to temporary file: %v", err) + } + + if err := tmpFile.Close(); err != nil { + return nil, fmt.Errorf("failed to close temporary file: %v", err) + } + + stdout, err := execCommand(ctx, cmdHandle, cmdEcho, cmdJSON, cmdFile, tmpFile.Name()) + 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, args ...string) (*bytes.Buffer, error) { cmd := exec.CommandContext(ctx, cmdBin, args...) diff --git a/vendor/modules.txt b/vendor/modules.txt index d4127a753..770ac7d14 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.3.1-0.20230602131639-734b9ba38567 ## explicit; go 1.16 github.com/networkplumbing/go-nft/nft github.com/networkplumbing/go-nft/nft/config