internal/appsec: review patch

- Update exported capabilities
- Split ASM_FEATURES handling appart from rules produts handling
- Simplify rules-toggle test
- Update rulesManager with RC configs in a separate function for easier
  testing

internal/appsec/remoteconfig.go

@@ -50,21 +50,13 @@ func mergeMaps[K comparable, V any](m1 map[K]V, m2 map[K]V) map[K]V {
 	return m1
 }
 
-// onRCUpdate is called when one or several remote configuration updates are available
-func (a *appsec) onRCUpdate(updates map[string]remoteconfig.ProductUpdate) map[string]rc.ApplyStatus {
-	r := a.cfg.rulesManager.clone()
+// combineRCRulesUpdates updates the state of the given rulesManager with the combination of all the provided rules updates
+func combineRCRulesUpdates(r *rulesManager, updates map[string]remoteconfig.ProductUpdate) (map[string]rc.ApplyStatus, error) {
 	statuses := map[string]rc.ApplyStatus{}
 	// Set the default statuses for all updates to unacknowledged
 	for _, u := range updates {
 		statuses = mergeMaps(statuses, statusesFromUpdate(u, false, nil))
 	}
-	// Process remote activation update separately
-	// This makes it easier to short circuit the following loop that focuses on rules related updates
-	if u, ok := updates[rc.ProductASMFeatures]; ok {
-		statuses = mergeMaps(statuses, a.handleASMFeatures(u))
-		delete(updates, rc.ProductASMFeatures)
-	}
-
 	var err error
 updateLoop:
 	// Process rules related updates
@@ -112,36 +104,60 @@ updateLoop:
 				}
 				var f rulesFragment
 				if err = json.Unmarshal(data, &f); err != nil || !f.validate() {
-					if err == nil { //
+					if err == nil {
 						err = errors.New("invalid configuration payload")
 					}
 					log.Debug("appsec: Remote config: error processing ASM config %s: %v", path, err)
 					statuses[path] = genApplyStatus(true, err)
 					break updateLoop
-				} else {
-					r.addEdit(path, f)
 				}
+				r.addEdit(path, f)
 			}
 		default:
 			log.Debug("appsec: Remote config: ignoring unsubscribed product %s", p)
 		}
 	}
 
+	// Set all statuses to ack if no error occured
+	if err == nil {
+		for _, u := range updates {
+			statuses = mergeMaps(statuses, statusesFromUpdate(u, true, nil))
+		}
+	}
+
+	return statuses, err
+
+}
+
+// onRemoteActivation is the RC callback called when an update is received for ASM_FEATURES
+func (a *appsec) onRemoteActivation(updates map[string]remoteconfig.ProductUpdate) map[string]rc.ApplyStatus {
+	statuses := map[string]rc.ApplyStatus{}
+	if u, ok := updates[rc.ProductASMFeatures]; ok {
+		statuses = a.handleASMFeatures(u)
+	}
+	return statuses
+
+}
+
+// onRCRulesUpdate is the RC callback called when security rules related RC updates are available
+func (a *appsec) onRCRulesUpdate(updates map[string]remoteconfig.ProductUpdate) map[string]rc.ApplyStatus {
+	// If appsec was deactivated through RC, stop here
+	if !a.started {
+		return map[string]rc.ApplyStatus{}
+	}
+
+	r := a.cfg.rulesManager.clone()
+	statuses, err := combineRCRulesUpdates(r, updates)
 	if err != nil {
 		log.Debug("appsec: Remote config: not applying any updates because of error: %v", err)
 		return statuses
 	}
 
-	// Set all statuses to ack
-	for _, u := range updates {
-		statuses = mergeMaps(statuses, statusesFromUpdate(u, true, nil))
-	}
-
-	// Compile the final rules once all updates have been processed
+	// Compile the final rules once all updates have been processed and no error occurred
 	r.compile()
 	data := r.raw()
 	log.Debug("appsec: Remote config: final compiled rules: %s", data)
-	// If an error occurs updating the WAF handle, don't swap the rulesManager and propagate the error
+	// If an error occurs while updating the WAF handle, don't swap the rulesManager and propagate the error
 	// to all config statuses since we can't know which config is the faulty one
 	if err = a.swapWAF(data); err != nil {
 		log.Error("appsec: Remote config: could not apply the new security rules: %v", err)
@@ -272,7 +288,8 @@ func mergeRulesDataEntries(entries1, entries2 []rc.ASMDataRuleDataEntry) []rc.AS
 
 func (a *appsec) startRC() {
 	if a.rc != nil {
-		a.rc.RegisterCallback(a.onRCUpdate)
+		a.rc.RegisterCallback(a.onRCRulesUpdate)
+		a.rc.RegisterCallback(a.onRemoteActivation)
 		a.rc.Start()
 	}
 }
@@ -344,10 +361,10 @@ func (a *appsec) enableRCBlocking() {
 	a.registerRCProduct(rc.ProductASM)
 	a.registerRCProduct(rc.ProductASMDD)
 	a.registerRCProduct(rc.ProductASMData)
-	a.registerRCCapability(remoteconfig.ASMUserBlocking)
-	a.registerRCCapability(remoteconfig.ASMRequestBlocking)
 
 	if _, isSet := os.LookupEnv(rulesEnvVar); !isSet {
+		a.registerRCCapability(remoteconfig.ASMUserBlocking)
+		a.registerRCCapability(remoteconfig.ASMRequestBlocking)
 		a.registerRCCapability(remoteconfig.ASMIPBlocking)
 		a.registerRCCapability(remoteconfig.ASMDDRules)
 		a.registerRCCapability(remoteconfig.ASMExclusions)

internal/appsec/remoteconfig_test.go

@@ -381,7 +381,7 @@ func TestCapabilities(t *testing.T) {
 		{
 			name:     "appsec-enabled/rulesManager-from-env",
 			env:      map[string]string{enabledEnvVar: "1", rulesEnvVar: "testdata/blocking.json"},
-			expected: []remoteconfig.Capability{remoteconfig.ASMRequestBlocking, remoteconfig.ASMUserBlocking},
+			expected: []remoteconfig.Capability{},
 		},
 	} {
 
@@ -531,7 +531,7 @@ func TestOnRCUpdate(t *testing.T) {
 			tc.ruleset.compile()
 			// Craft and process the RC updates
 			updates := craftRCUpdates(tc.ruleset.edits)
-			activeAppSec.onRCUpdate(updates)
+			activeAppSec.onRCRulesUpdate(updates)
 			// Compare rulesets
 			require.ElementsMatch(t, tc.ruleset.latest.Rules, activeAppSec.cfg.rulesManager.latest.Rules)
 			require.ElementsMatch(t, tc.ruleset.latest.Overrides, activeAppSec.cfg.rulesManager.latest.Overrides)
@@ -624,7 +624,7 @@ func TestOnRCUpdateStatuses(t *testing.T) {
 				t.Skip("AppSec needs to be enabled for this test")
 			}
 
-			statuses := activeAppSec.onRCUpdate(tc.updates)
+			statuses := activeAppSec.onRCRulesUpdate(tc.updates)
 			if tc.updateError {
 				for _, status := range statuses {
 					require.NotEmpty(t, status.Error)
@@ -651,15 +651,13 @@ func TestWafRCUpdate(t *testing.T) {
 		},
 	}
 
-	Start()
-	defer Stop()
-
-	if !Enabled() {
-		t.Skip("AppSec needs to be enabled for this test")
+	if waf.Health() != nil {
+		t.Skip("WAF needs to be available for this test")
 	}
 
 	t.Run("toggle-blocking", func(t *testing.T) {
-		cfg := activeAppSec.cfg
+		cfg, err := newConfig()
+		require.NoError(t, err)
 		wafHandle, err := waf.NewHandle(cfg.rulesManager.raw(), cfg.obfuscator.KeyRegex, cfg.obfuscator.ValueRegex)
 		require.NoError(t, err)
 		defer wafHandle.Close()
@@ -673,12 +671,11 @@ func TestWafRCUpdate(t *testing.T) {
 		require.Contains(t, string(matches), "crs-913-120")
 		require.Empty(t, actions)
 		// Simulate an RC update that disables the rule
-		statuses := activeAppSec.onRCUpdate(craftRCUpdates(map[string]rulesFragment{"override": override}))
+		statuses, err := combineRCRulesUpdates(cfg.rulesManager, craftRCUpdates(map[string]rulesFragment{"override": override}))
 		for _, status := range statuses {
 			require.Equal(t, status.State, rc.ApplyStateAcknowledged)
 		}
-		// Retrieve the new appsec cfg and instantiate a new waf handle form it to prepare the next run
-		cfg = activeAppSec.cfg
+		cfg.rulesManager.compile()
 		newWafHandle, err := waf.NewHandle(cfg.rulesManager.raw(), cfg.obfuscator.KeyRegex, cfg.obfuscator.ValueRegex)
 		require.NoError(t, err)
 		defer newWafHandle.Close()

internal/appsec/ruleset_builder.go

@@ -116,10 +116,11 @@ func (r_ *rulesFragment) clone() rulesFragment {
 // newRulesManager initializes and returns a new rulesManager using the provided rules.
 // If no rules are provided (nil), or the provided rules are invalid, the default rules are used instead
 func newRulesManager(rules []byte) *rulesManager {
-	f := defaultRulesFragment()
+	var f rulesFragment
 	buf := new(bytes.Buffer)
 	json.Compact(buf, rules)
 	if err := json.Unmarshal(buf.Bytes(), &f); err != nil {
+		f = defaultRulesFragment()
 		log.Debug("appsec: cannot create rulesManager from specified rules. Using default rules instead")
 	}
 	return &rulesManager{