Route the git basic auth ban through labkit behind a flag

Nidhey Indurkarrequested to merge
nindurkar/labkit-git-basic-auth-ban into master

What does this MR do and why?

Routes the Git and container registry authentication IP ban through Labkit::RateLimit instead of Rack::Attack::Allow2Ban, behind the use_labkit_git_basic_auth_ban flag. Step 4 of gitlab-com/gl-infra/production-engineering#29557.

Allow2Ban is the last reason the rack-attack gem stays in the Gemfile, and the only thing keeping the Rack::Attack.cache.store assignment alive.

The three operations map onto labkit as:

today labkit
banned? peek, which reads the ban without counting
register_fail! check, which counts and bans on crossing the limit
reset! clear, which drops the counter and the ban

Settings keep their existing names and meanings. Only the mechanism changes.

Behaviour differences, both accepted deliberately

The ban lands one attempt later. Allow2Ban banned when the count reached maxretry; labkit blocks only once the count is above the limit. Agreed with @reprazent rather than special-casing labkit's comparison, so the new behaviour is one attempt more lenient.

The counting window is anchored differently. Allow2Ban counts inside wall-clock buckets, so attempts spread across a bucket boundary do not accumulate. Labkit anchors its window on the first write. This is the difference the counter added in !250836 (merged) exists to quantify, and it is why that MR shipped first.

The return value changes, and a caller reads it. Allow2Ban returns false on the attempt that writes the ban and true only afterwards. The labkit path returns true on the crossing attempt, because labkit cannot separate "already banned" from "banned by this attempt": a ban suppresses counting, so both look the same.

Gitlab::Auth#rate_limit! branches on that return value to write the "threshold exceeded" auth log, so the log now fires when the ban is created rather than one attempt later. That is the improvement @reprazent asked for: on the Allow2Ban path it only fires when a concurrent request banned the IP first, which is why production records almost none of them.

A bantime under one second means bans are off

bantime defaults to 1 hour, but the default is applied with ||= and 0 is truthy in Ruby, so an operator can configure 0. Labkit needs whole seconds and rejects anything under one, which raises before it reaches Redis, fails the whole rule open, and stops it counting at all.

The rule is therefore built without ban_for in that case, leaving an ordinary windowed limit that still blocks on the count. bantime is resolved once at build time rather than per check, which costs nothing since Settings is read at boot.

Notes for review

  • The allowlist stays a Ruby check in IpRateLimiter rather than becoming a :skip rule. Entries are netmasks, and labkit matchers do equality, regex or set membership, none of which express CIDR containment.
  • gitlab-ci-token stays exempt. The exemption lives in Gitlab::Auth, above this code, and short-circuits before banned? is called at all.
  • Both paths emit the same counter, gitlab_rate_limiter_git_basic_auth_ban_events_total, but two labels shift meaning. already_banned stops being emitted, and ban counts attempts blocked by a ban rather than bans created, because labkit cannot tell the crossing attempt from a later one. In practice both are bounded by the same race: Gitlab::Auth raises IpBlocked before authenticating, so a banned caller does not normally reach register_fail!, which is why already_banned reads near zero in production today. failure, blocked and reset are unchanged and are the labels to compare on.
  • In-flight bans are abandoned when the flag flips, since the two implementations use different Redis keys. Up to bantime, which is 15 minutes on GitLab.com.

Related

Edited by Nidhey Indurkar

Merge request reports