pkg/cache had tests for Get, Set and Delete but none for their CIDR
counterparts, so the range keyspace was only exercised end to end by the
e2e scenario.
Test_GetCIDR covers hits, the boundaries of a range, IPv6, IPv4 mapped
clients and invalid input. Test_GetCIDR_MostSpecific pins the precedence
between overlapping decisions, which is deliberate behaviour that nothing
was holding in place: a captcha on a /24 is not overruled by a ban on its
/8. Test_DeleteCIDR checks the wider decision survives a narrower one being
removed, and Test_SetCIDR_InvalidIsNotStored that a rejected value stores
nothing at all.
Test_GetCIDR_Reads counts cache reads through an isolated cacheInterface,
so the cost of a lookup is part of the contract: probing every prefix
length again turns it into 34 reads for IPv4 and 130 for IPv6 and fails.
The helpers were tested in isolation but nothing tied them together, and
what actually has to hold is that the key SetCIDR writes for a decision is
one of the keys GetCIDR looks up for an IP that decision covers. A
formatting change on either side would have silently stopped every range
decision from matching with all tests green.
TestCIDRKeys_MatchNormalizeCIDR pins that both ways, including the cases
worth being explicit about: a decision that is not on a network address,
IPv4 mapped clients against an IPv4 range, and that the two families do not
mix. Dropping the mask in cidrKey fails 9 of its cases.
Also covers CIDRLookupKeys against CIDRKeys length by length, the IPv6 side
of the network address test, and CIDRPrefixLen.
SetCIDR and DeleteCIDR returned silently when NormalizeCIDR rejected the
value, so a range decision the plugin does not understand is not enforced
and nothing says why. Every other operation of the package logs, and this
one fails open, which is the direction worth shouting about.
Log at Error with the raw value and what the consequence is, so an
unexpected decision format shows up in the logs instead of looking like a
decision that was applied.
In stream mode nothing caches a negative result per IP, so the exact IP
lookup misses on every legitimate request and each one fell through to
GetCIDR, which probed every possible prefix length: 33 cache reads for an
IPv4 client, 129 for an IPv6 one, even when no range decision existed at
all. On the local cache that is wasted work on the request path; with
redis it is 33 to 129 sequential round trips per request.
Keep the set of prefix lengths that have at least one decision under a
single key, written before the decision itself, and probe only those.
Measured cache reads per request: 1 with no range decision (was 33 / 129),
2 with a single /24 in use, 4 with four prefix lengths in use.
The set only grows, so a deleted or expired decision leaves a length
behind that costs one extra read rather than risking an unmatched
decision, and it is written with an effectively infinite duration since it
has to outlive every decision it describes. If it is ever missing while
decisions live (a redis eviction under maxmemory), range decisions stop
matching until the next one arrives; it is the hottest key of the
namespace, so an LRU policy evicts it last.
CIDRKeys masked the address byte by byte and formatted the result with
string concatenation, while SetCIDR/DeleteCIDR format their keys with
net.IPNet.String() via NormalizeCIDR. The two agreed only by coincidence:
any divergence in formatting silently stops every range decision from
matching, with no test covering the invariant.
Mask with net.IP.Mask and format through net.IPNet.String() so both sides
go through the same formatter. Output is byte for byte identical to the
previous implementation (checked against a golden dump of both IPv4 and
IPv6 keys, including ::ffff: forms).
Dropping the inner byte loops also removes the only intrange violation in
the tree, so the linter exclusion added for them is no longer needed, and
the redundant import alias on pkg/ip goes away with it.
* feat: Allow cache reading from replicas
* 🍱 fix logic
* ✨ add testing for redis with mock
* 🍱 fix permission
* 📝 test(e2e/redis): fix swapped IP→verdict comments
The mock returns "f" (not banned) for 1.2.3.4 and "t" (banned) for
1.2.3.5, and the run.sh assertions match that. Both doc comments
described the opposite mapping; correct them to match the code.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* ✅ test(e2e/redis): exercise read-from-replica path
The redis scenario only set redisCacheHost, so it validated the writer
but never the round-robin reader path this feature adds. Split the mock
into two roles: the primary (--redis-addr) now answers every GET with a
miss, while the replica (--redis-read-addr) serves the hardcoded
verdicts. The scenario points redisCacheReadHosts at the replica (twice,
to drive round-robin), so the banned-IP-blocked assertion only passes if
the plugin actually reads decisions from the replica rather than the
primary.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* 📝 docs: note replicas don't fall back to primary on outage
When RedisCacheReadHosts is set, reads are not retried against the
primary if the replicas are unreachable. Document that this, combined
with the default RedisCacheUnreachableBlock=true, means a replica outage
can block traffic while the primary is healthy.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* 🐛 fix(cache): avoid nil-pointer panic on empty redis read
redisCache.get fell through to `switch err.Error()` when Get returned a
nil error with an empty value, panicking on the nil error. simpleredis
never returns that combination today (a miss yields RedisMiss), so it
was unreachable in practice — but the read path is safer treating an
empty, error-free read as a cache miss, which also guarantees err is
non-nil before err.Error() is called.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* 🍱 add test for rotation
* 🐛 readd redis/run.sh
* 🐛 fix redis/run.sh
* 🐛 fix test label; remove log
* 🍱 add tests for roundRobin
* 🐛 fix test
---------
Co-authored-by: maxlerebourg <maxlerebourg@gmail.com>
Co-authored-by: mhx <mathieu@hanotaux.fr>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* 🐛 fix appsec silently 403-ing gRPC streams with unreadable body
A bidirectional gRPC stream is an HTTP/2 request with no Content-Length
whose body never reaches EOF. Since #321 removed the ContentLength guard,
appsecQuery buffered it with io.ReadAll, which blocked until the request
timed out and was turned into a 403 (issue #323). The backend was never
reached (OriginStatus:0).
Mirror the reference lua-cs-bouncer behaviour: detect an unreadable body
(ProtoMajor >= 2 && ContentLength < 0) and, instead of buffering it,
forward the request to Appsec with headers only. Add a new
CrowdsecAppsecDropUnreadableBody option (default false) that mirrors the
reference APPSEC_DROP_UNREADABLE_BODY: when true, such requests are
blocked outright instead of forwarded without their body.
Readable HTTP/1.1 bodies are still buffered and inspected, so the bypass
closed by #321 stays closed.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* 🚨 appsec: satisfy linters (gocritic ifElseChain, misspell)
Rewrite the body-handling if/else chain in appsecQuery as a switch
(gocritic) and use US spelling "behavior" (misspell).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* 🔇 appsec: drop redundant unreadable-body debug log
Address review on #332: the caller (handleNextServeHTTP) already logs the
returned error with the request IP, so the inner Debug line duplicated it.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* 🍱 increase gocyclo
* 🍱 fix lint
---------
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Co-authored-by: maxlerebourg <maxlerebourg@gmail.com>
* Add parameter to configure Ban Response Content-Type
* Add testing for new BanResponseContentType parameter
* Ensure there is a fallback to default Content-Type is user provided empty value
* Set Content-Type even if banTemplate is nil
* Add more edge cases for testing ban response Content-Type
* Add CR/LF validation for BanResponseContentType
* Add CaptchaResponseContentType to allow separate Content-Type configuration for captcha responses
* Add testing for new CaptchaResponseContentType
* Update README
* Split nil and CR/LF response Content-Type value validation into separate function
* Throw error instead of setting the default in case of empty parameter declaration
* Update testing accordingly
* ✨ remove HTML from var name, add tests and infer content type from filePath
* 🍱 fix lint ?
* 🍱 fix lint
* 🍱 fix lint
* 🍱 fix lint + naming
* 🍱 fix lint
* 🍱 fuck lint
---------
Co-authored-by: maxlerebourg <maxlerebourg@gmail.com>
* 🐛 fall back to system trust store when no custom TLS CA is set
Closes#327.
Until now, configuring `crowdsecLapiScheme=https` forced the operator
to either provide `crowdsecLapiTLSCertificateAuthority` (a custom CA)
or set `crowdsecLapiTLSInsecureVerify=true` — there was no way to rely
on the host's system trust store, which is the expected setup when the
LAPI sits behind a reverse proxy with a publicly trusted (e.g. Let's
Encrypt) certificate.
Two contributing bugs:
- `validateParamsTLS` rejected an empty CA up-front.
- `getTLSConfig` always allocated an empty `tls.Config.RootCAs`,
which silently disabled the standard library's fall-back to
`x509.SystemCertPool()`.
Fix: drop the validation error for the empty-CA case and only allocate
`RootCAs` when a custom CA is actually provided. Same change applies
symmetrically to the AppSec path since the helper is shared.
Add unit tests covering the four meaningful states (HTTP, HTTPS with
system CA, HTTPS with custom CA, HTTPS with insecure verify) plus the
malformed-PEM rejection. README updated to document the system trust
store as an explicit option for both LAPI and AppSec HTTPS.
* 📝 fix gofmt alignment in TLS test struct
* ✅ update existing test: https without CA is now accepted
* ♻️ tests: hoist shared validPEM to package level, rename cfgGarbage
Address review on #331:
- the self-signed validPEM block was duplicated in two test funcs; declare it
once at package level and drop both local copies.
- rename cfgGarbage -> cfgInvalidCA (and its test case) for a descriptive name.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* ✅ e2e mock: add tls-system-ca scenario (HTTPS LAPI via system trust store)
CI regression coverage for this PR: with no custom CA configured, the bouncer
must fall back to the OS/system trust store for an HTTPS LAPI.
In the binary suite the "system trust store" is whatever Go's
x509.SystemCertPool() reads, which honours SSL_CERT_FILE on the Traefik process.
The scenario mints a throwaway CA, serves the mock LAPI over HTTPS with a cert
signed by it, and runs the stack twice:
- positive: SSL_CERT_FILE = our CA -> LAPI trusted -> 200
- negative: SSL_CERT_FILE = empty bundle -> not trusted -> 403 (fail-closed)
The negative run proves the patch still VERIFIES (not an insecure skip).
- mocklapi: optional --lapi-tls-cert/--lapi-tls-key to serve the LAPI over TLS.
- common.sh: opt-in LAPI_TLS_CERT/KEY (HTTPS mock) and TRAEFIK_SSL_CERT_FILE
(inject SSL_CERT_FILE into Traefik); both default-empty, other scenarios
unaffected.
- adds openssl as a scenario-only dependency.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
* 🍱 not block on stream startup
* 🍱 fix
* 🍱 fix lint
* 🍱 fix lint
* 🍱 fix lint
* 🍱 update readme
* 📚 clarify StreamStartupBlock warning in README
Make the fail-open implication of StreamStartupBlock=false explicit:
banned IPs are allowed through until the first stream sync completes.
* 🐛 clear isCrowdsecStreamStartup on alreadyUpdated path
When cacheTimeoutKey is already set (another instance/process has
populated the stream cache within the update window), the early return
in handleStreamCache used to leave isCrowdsecStreamStartup at true,
causing this instance to keep sending startup=true on subsequent ticks.
Flip the flag in the early-return path so startup is correctly tracked
across multi-instance deployments sharing a cache.
---------
Co-authored-by: mhx <mathieu@hanotaux.fr>
* Do not validate Crowdsec LAPI key and TLS authentication if bouncer is in AppSec mode
* Add extra validation checks for lapiKey or appsecKey definition
* Fix linting on changed error message
* Update configuration.go
- Show paramName in validateParamsApiKey
- Remove check for empty appsecKey or lapiKey (LAPI can have TLS auth)
- Remove check for emptry appsecKey if CrowdsecMode is Appsec
* Update configuration_test.go after updated validateParamsAPIKey
* 🐛 fix start up config error for appsec
* :doc: add documentation on appsec variables and missing conf parameter
* 🍱 fix lint
* 🍱 fix lint
* 🍱 fix lint
* 🍱 fix after lot of tests
* update exemple tls with new variables tested
* fix exemple appsec with release and not localplugin
---------
Co-authored-by: mhx <mathieu@hanotaux.fr>
* Add parameter to specify Request header value to inject in HTML template
* Add documentation for specifying request header value
* Fix example in docs for CustomHeader
* Update parameter name to traceCustomHeader
* Ensure naming is consitent with project
mhx