Skip to content
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

Temporary account lockout for repeated auth failure #5032

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

rnewson
Copy link
Member

@rnewson rnewson commented Apr 17, 2024

Overview

Enhance couchdb to efficiently reject requests for a given user if repeated requests have failed for authentication reasons, from the same IP address. This helps slow brute-force password attacks and is especially helpful if each authentication attempt is very expensive (pbkdf2 with a high iteration count, typically).

Testing recommendations

will be covered by automated tests

Related Issues or Pull Requests

Checklist

  • Code is written and works correctly
  • Changes are covered by tests
  • Any new configurable parameters are documented in rel/overlay/etc/default.ini
  • Documentation changes were made in the src/docs folder
  • Documentation changes were backported (separated PR) to affected branches

@@ -28,7 +28,7 @@
-export([cookie_auth_header/2]).
-export([handle_session_req/1, handle_session_req/2]).

-export([authenticate/4, verify_totp/2]).
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

apparently not used externally, so I removed the export rather than update the arity.

Copy link
Contributor

@big-r81 big-r81 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would couch_auth_lock[out].erl be a clearer name for the module instead of couch_lockout.erl?

@janl
Copy link
Member

janl commented Apr 18, 2024

Would couch_auth_lock[out].erl be a clearer name for the module instead of couch_lockout.erl?

lockout is its own thing, e.g. https://en.wikipedia.org/wiki/Lockout_%28industry%29

@rnewson rnewson force-pushed the lockout branch 4 times, most recently from a7697f2 to 8ab44e9 Compare April 18, 2024 18:49
-export([is_locked_out/3, lockout/3]).

is_locked_out(#httpd{} = Req, UserName, UserSalt) ->
case config:get_boolean("couch_lockout", "enable", true) of
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rnewson

Would it make sense now (after renaming to couch_auth_lockout.erl) to put it under the [chttpd_auth] section as lockout key?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah no, maybe not at the moment, because it isn't a cluster wide setting / functionality yet ...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm open to suggestions here. putting the controls under [chttpd_auth] seems reasonable to me. @janl ?

@rnewson rnewson force-pushed the lockout branch 4 times, most recently from e929e29 to 0bf5fbc Compare April 25, 2024 14:41
@rnewson rnewson marked this pull request as ready for review April 25, 2024 14:44
@rnewson rnewson force-pushed the lockout branch 3 times, most recently from 222a1ab to 424dda4 Compare April 26, 2024 15:36
Comment on lines +29 to +37
defp test_couch_auth_lockout_enforcement do
# exceed the lockout threshold
for _n <- 1..5 do
resp = Couch.get("/_all_dbs",
no_auth: true,
headers: [authorization: "Basic #{:base64.encode("couch_auth_lockout:baz")}"]
)
assert resp.status_code == 401
end
Copy link
Contributor

@nickva nickva Apr 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be worth having a test with a warn mode just validate that code path and that doesn't crash anything?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah for sure, I also meant to add a test to prove it forgives you (and goes back to 401)

; failures. The account is automatically unlocked at the end of this time, starting
; from the _first_ authentication failure.
; note: changing this setting requires a couchdb restart.
;max_lifetime = 300000
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a newline to avoid having a red warning marker, and mess with some users editors where it would auto-append it on save later when we update the file again.

Copy link
Contributor

@nickva nickva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 a very nice feature. I like the warn mode to allow users to test out the setting without impacting their customers.

See a few suggestions about formatting and added a test case for warn mode.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants