Merge pull request #902 from SirPhuttel/applyconfigecho

spoofcheck: Make use of go-nft's ApplyConfigEcho()

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

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=

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,45 +106,59 @@ 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()
 	regularChainsConfig.DeleteChain(ifaceChain)
 	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)
 	}
 

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)
 	}

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