-
Notifications
You must be signed in to change notification settings - Fork 3
feat(remediation): support custom remediations #139
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
116ef5b
093fb80
4bca4dc
727b7b5
c9640a5
fffda45
e3737d1
5277f9e
c3ba1d8
04de61b
8346aa6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,37 +1,140 @@ | ||||||||||||||||||||||||||||||
| package remediation | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| // The order matters since we use slices.Max to get the max value | ||||||||||||||||||||||||||||||
| import ( | ||||||||||||||||||||||||||||||
| "sync" | ||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| // Default weights for built-in remediations | ||||||||||||||||||||||||||||||
| // Allow=0, Unknown=1, then expand others to allow custom remediations to slot in between | ||||||||||||||||||||||||||||||
| const ( | ||||||||||||||||||||||||||||||
| WeightAllow = 0 | ||||||||||||||||||||||||||||||
| WeightUnknown = 1 | ||||||||||||||||||||||||||||||
| WeightCaptcha = 10 | ||||||||||||||||||||||||||||||
| WeightBan = 20 | ||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| // Remediation represents a remediation type as a string | ||||||||||||||||||||||||||||||
| type Remediation string | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| // Built-in remediation constants | ||||||||||||||||||||||||||||||
| const ( | ||||||||||||||||||||||||||||||
| Allow Remediation = iota // Allow remediation | ||||||||||||||||||||||||||||||
| Unknown // Unknown remediation (Unknown is used to have a value for remediation we don't support EG "MFA") | ||||||||||||||||||||||||||||||
| Captcha // Captcha remediation | ||||||||||||||||||||||||||||||
| Ban // Ban remediation | ||||||||||||||||||||||||||||||
| Allow Remediation = "allow" | ||||||||||||||||||||||||||||||
| Unknown Remediation = "unknown" | ||||||||||||||||||||||||||||||
| Captcha Remediation = "captcha" | ||||||||||||||||||||||||||||||
| Ban Remediation = "ban" | ||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| type Remediation uint8 // Remediation type is smallest uint to save space | ||||||||||||||||||||||||||||||
| // registry manages remediation weights | ||||||||||||||||||||||||||||||
| type registry struct { | ||||||||||||||||||||||||||||||
| mu sync.RWMutex | ||||||||||||||||||||||||||||||
| weights map[string]int // Maps remediation name to its weight | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| var globalRegistry = ®istry{ | ||||||||||||||||||||||||||||||
|
Comment on lines
+27
to
+33
|
||||||||||||||||||||||||||||||
| // registry manages remediation weights | |
| type registry struct { | |
| mu sync.RWMutex | |
| weights map[string]int // Maps remediation name to its weight | |
| } | |
| var globalRegistry = ®istry{ | |
| // weightRegistry manages remediation weights | |
| type weightRegistry struct { | |
| mu sync.RWMutex | |
| weights map[string]int // Maps remediation name to its weight | |
| } | |
| var globalRegistry = &weightRegistry{ |
Copilot
AI
Dec 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SetWeight function does not normalize the remediation name to lowercase. This creates inconsistency with the built-in remediations (initialized as lowercase in init) and can cause weight lookups to fail if the case doesn't match. The name parameter should be normalized using strings.ToLower.
Copilot
AI
Dec 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The GetWeight function uses RLock/RUnlock on every weight lookup, which can be a significant performance overhead for hot path operations. The PR description claims "Performance: Pointer-based lookups faster than string comparisons", but this implementation actually introduces mutex overhead that wasn't present in the integer-based comparison system. Consider using sync.Map or caching weights to reduce lock contention.
Copilot
AI
Dec 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The LoadWeights function does not normalize remediation names to lowercase. This creates inconsistency with the built-in remediations (initialized as lowercase in init) and can cause weight lookups to fail if the case doesn't match. The name keys should be normalized using strings.ToLower.
Copilot
AI
Dec 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The LoadWeights function does not validate inputs. Missing validation includes: (1) No check for negative weights, which violates the documented requirement that weights must be >= 0; (2) No normalization of remediation names to lowercase, creating case-sensitivity issues; (3) No protection against extremely large weights that could cause integer overflow in Compare. These issues could lead to unexpected behavior or security problems from malicious configuration.
Copilot
AI
Dec 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The New function does not normalize the input string (e.g., to lowercase). This creates a critical inconsistency where "Ban", "ban", and "BAN" are treated as different remediations with potentially different weights. Since weight lookups are case-sensitive, this could lead to incorrect priority comparisons and unexpected behavior. The function should normalize the input, similar to how scope is normalized with strings.ToLower in pkg/dataset/root.go line 65.
Copilot
AI
Dec 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Compare function documentation is misleading. It states "zero if a == b" which could be interpreted as comparing remediation equality, but the function actually compares weights. Two different remediations with the same weight will return zero. The documentation should clarify this, for example: "zero if a and b have the same weight".
| // - negative if a < b | |
| // - zero if a == b | |
| // - positive if a > b | |
| // - negative if a has a lower weight than b | |
| // - zero if a and b have the same weight | |
| // - positive if a has a higher weight than b |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,6 +7,7 @@ import ( | |
| "gopkg.in/yaml.v2" | ||
|
|
||
| "github.com/crowdsecurity/crowdsec-spoa/internal/geo" | ||
| "github.com/crowdsecurity/crowdsec-spoa/internal/remediation" | ||
| "github.com/crowdsecurity/crowdsec-spoa/pkg/host" | ||
| cslogging "github.com/crowdsecurity/crowdsec-spoa/pkg/logging" | ||
| "github.com/crowdsecurity/go-cs-lib/csyaml" | ||
|
|
@@ -36,6 +37,32 @@ type BouncerConfig struct { | |
| ListenUnix string `yaml:"listen_unix"` | ||
| PrometheusConfig PrometheusConfig `yaml:"prometheus"` | ||
| PprofConfig PprofConfig `yaml:"pprof"` | ||
| // RemediationWeights allows users to configure custom weights for remediations. | ||
| // | ||
| // Format: | ||
| // remediation_weights: | ||
| // <remediation_name>: <weight> | ||
| // | ||
| // Example: | ||
| // remediation_weights: | ||
| // mfa: 15 # slots between captcha (10) and ban (20) | ||
| // | ||
| // Valid weight range: integer values >= 0. Lower values are less severe; higher values are more severe. | ||
| // Recommended: Use values between 0 and 100. | ||
| // | ||
| // Built-in defaults: | ||
| // allow=0, unknown=1, captcha=10, ban=20 | ||
| // | ||
| // Custom weights override or supplement built-in remediations. If a custom remediation is defined, | ||
| // its weight will be used for ordering and severity. Custom remediations can slot between built-in | ||
| // ones by choosing an appropriate weight value. | ||
| // | ||
| // Tie-breaking: If two remediations have the same weight, alphabetical order of the remediation | ||
| // name is used as a deterministic tie-breaker when determining priority. | ||
| // | ||
| // Note: Custom weights for built-in remediations (allow, unknown, captcha, ban) must be set | ||
| // before package initialization. After init(), package-level constants already have cached weights. | ||
LaurenceJJones marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| RemediationWeights map[string]int `yaml:"remediation_weights,omitempty"` | ||
| } | ||
|
|
||
| // MergedConfig() returns the byte content of the patched configuration file (with .yaml.local). | ||
|
|
@@ -67,6 +94,11 @@ func NewConfig(reader io.Reader) (*BouncerConfig, error) { | |
| return nil, fmt.Errorf("failed to setup logging: %w", err) | ||
| } | ||
|
|
||
| // Load custom remediation weights if configured (loads all weights at once on startup) | ||
| if config.RemediationWeights != nil { | ||
| remediation.LoadWeights(config.RemediationWeights) | ||
| } | ||
|
Comment on lines
+97
to
+100
|
||
|
|
||
| if err := config.Validate(); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,15 +15,15 @@ import ( | |
| type BartAddOp struct { | ||
| Prefix netip.Prefix | ||
| Origin string | ||
| R remediation.Remediation | ||
| R string // Remediation name as string | ||
|
||
| IPType string | ||
| Scope string | ||
| } | ||
|
|
||
| // BartRemoveOp represents a single prefix removal operation for batch processing | ||
| type BartRemoveOp struct { | ||
| Prefix netip.Prefix | ||
| R remediation.Remediation | ||
| R string // Remediation name as string | ||
| Origin string | ||
| IPType string | ||
| Scope string | ||
|
|
@@ -91,7 +91,7 @@ func (s *BartRangeSet) initializeBatch(operations []BartAddOp) { | |
| // Only build logging fields if trace level is enabled | ||
| var valueLog *log.Entry | ||
| if s.logger.Logger.IsLevelEnabled(log.TraceLevel) { | ||
| valueLog = s.logger.WithField("prefix", prefix.String()).WithField("remediation", op.R.String()) | ||
| valueLog = s.logger.WithField("prefix", prefix.String()).WithField("remediation", op.R) | ||
| valueLog.Trace("initial load: collecting prefix operations") | ||
| } | ||
|
|
||
|
|
@@ -101,7 +101,7 @@ func (s *BartRangeSet) initializeBatch(operations []BartAddOp) { | |
| data = RemediationMap{} | ||
| } | ||
| // Add the remediation (this handles merging if prefix already seen) | ||
| data.Add(valueLog, op.R, op.Origin) | ||
| data.Add(valueLog, remediation.FromString(op.R), op.Origin) | ||
| prefixMap[prefix] = data | ||
| } | ||
|
|
||
|
|
@@ -134,7 +134,7 @@ func (s *BartRangeSet) updateBatch(cur *bart.Table[RemediationMap], operations [ | |
| // Only build logging fields if trace level is enabled | ||
| var valueLog *log.Entry | ||
| if s.logger.Logger.IsLevelEnabled(log.TraceLevel) { | ||
| valueLog = s.logger.WithField("prefix", prefix.String()).WithField("remediation", op.R.String()) | ||
| valueLog = s.logger.WithField("prefix", prefix.String()).WithField("remediation", op.R) | ||
| valueLog.Trace("adding to bart trie") | ||
| } | ||
|
|
||
|
|
@@ -146,15 +146,15 @@ func (s *BartRangeSet) updateBatch(cur *bart.Table[RemediationMap], operations [ | |
| valueLog.Trace("exact prefix exists, merging remediations") | ||
| } | ||
| // bart already cloned via our Cloner interface, modify directly | ||
| existingData.Add(valueLog, op.R, op.Origin) | ||
| existingData.Add(valueLog, remediation.FromString(op.R), op.Origin) | ||
| return existingData, false // false = don't delete | ||
| } | ||
| if valueLog != nil { | ||
| valueLog.Trace("creating new entry") | ||
| } | ||
| // Create new data | ||
| newData := make(RemediationMap) | ||
| newData.Add(valueLog, op.R, op.Origin) | ||
| newData.Add(valueLog, remediation.FromString(op.R), op.Origin) | ||
| return newData, false // false = don't delete | ||
| }) | ||
| } | ||
|
|
@@ -193,7 +193,7 @@ func (s *BartRangeSet) RemoveBatch(operations []BartRemoveOp) []*BartRemoveOp { | |
| // Only build logging fields if trace level is enabled | ||
| var valueLog *log.Entry | ||
| if s.logger.Logger.IsLevelEnabled(log.TraceLevel) { | ||
| valueLog = s.logger.WithField("prefix", prefix.String()).WithField("remediation", op.R.String()) | ||
| valueLog = s.logger.WithField("prefix", prefix.String()).WithField("remediation", op.R) | ||
| valueLog.Trace("removing from bart trie") | ||
| } | ||
|
|
||
|
|
@@ -210,11 +210,12 @@ func (s *BartRangeSet) RemoveBatch(operations []BartRemoveOp) []*BartRemoveOp { | |
|
|
||
| // Check if the remediation exists with the matching origin before removing | ||
| // This prevents removing decisions when the origin has been overwritten (e.g., by CAPI) | ||
| if !existingData.HasRemediationWithOrigin(op.R, op.Origin) { | ||
| if !existingData.HasRemediationWithOrigin(remediation.FromString(op.R), op.Origin) { | ||
| // Origin doesn't match - this decision was likely overwritten by another origin | ||
| // Don't remove it, as it's not the decision we're trying to delete | ||
| if valueLog != nil { | ||
| storedOrigin, exists := existingData[op.R] | ||
| r := remediation.FromString(op.R) | ||
| storedOrigin, exists := existingData[r] | ||
| if exists { | ||
| valueLog.Tracef("remediation exists but origin mismatch (stored: %s, requested: %s), skipping removal", storedOrigin, op.Origin) | ||
| } else { | ||
|
|
@@ -228,7 +229,7 @@ func (s *BartRangeSet) RemoveBatch(operations []BartRemoveOp) []*BartRemoveOp { | |
| // bart already cloned via our Cloner interface, modify directly | ||
| // Remove returns an error if remediation doesn't exist (duplicate delete) | ||
| // We already checked origin above, so this should succeed | ||
| err := existingData.Remove(valueLog, op.R) | ||
| err := existingData.Remove(valueLog, remediation.FromString(op.R)) | ||
| if errors.Is(err, ErrRemediationNotFound) { | ||
| // This shouldn't happen since we checked above, but handle it gracefully | ||
| if valueLog != nil { | ||
|
|
@@ -288,11 +289,11 @@ func (s *BartRangeSet) Contains(ip netip.Addr) (remediation.Remediation, string) | |
| return remediation.Allow, "" | ||
| } | ||
|
|
||
| remediationResult, origin := data.GetRemediationAndOrigin() | ||
| r, origin := data.GetRemediationAndOrigin() | ||
| if valueLog != nil { | ||
| valueLog.Tracef("bart result: %s (data: %+v)", remediationResult.String(), data) | ||
| valueLog.Tracef("bart result: %s (data: %+v)", r.String(), data) | ||
| } | ||
| return remediationResult, origin | ||
| return r, origin | ||
| } | ||
|
|
||
| // HasRemediation checks if an exact prefix has a specific remediation with a specific origin. | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR description claims "Automatic deduplication: Shared *string pointers deduplicate map keys" and "Reduced allocations: Eliminates duplicate string headers in maps", but this implementation uses string values (not pointers) as the Remediation type. The claimed benefits about pointer-based deduplication are not realized by this design, making the documentation misleading.