Reduce Rack Attack false positives causing 403 errors during HTTP authentication
What does this MR do?
This MR reduces false positives causing 403 Forbidden messages after HTTP authentication.
A Git client may attempt to access a repository without a password. If it receives a 401 error, the client often will try again, this time supplying a password. The problem is that grack_auth.rb considers a blank password an authentication failure and increases a Redis counter each time this happens. With enough requests, an IP can be banned temporarily even though previous attempts may have been successful. This leads users to see 403 Forbidden errors until the ban times out (default: 1 hour).
To reduce the chance of a false positive, this MR resets the counter upon a successful authentication from an IP.
In addition, this MR logs when a user has been banned and introduces the ability to disable Rack Attack via a config variable.
Are there points in the code the reviewer needs to double check?
rack-attack v4.2.0 doesn't support the ability to clear counters out of the box, so rack_attack_helpers.rb includes a number of monkey patches to make it work. It looks like this functionality may be added in v4.3.0. I've also sent pull requests to rack-attack to add the functionality necessary to delete a key.
Each time an authentication is successful, the Redis counter for that IP is cleared. I deemed it better to clear the counter than to allow for blank passwords, since the latter seems like a security risk.
Why was this MR needed?
It was quite difficult to figure out why users were seeing 403 Forbidden, which is why the log message was added. Users were getting a lot of false positives when accessing repositories with HTTPS. Including the username in the HTTPS URL (e.g. https://username@mydomain.com/account/repo.git) caused authentication failures because while the git client provided the username, it left the password blank, leading to an authentication failure.
What are the relevant issue numbers / Feature requests?
See Issue #1171 (closed)