You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@couchdb.apache.org by "rnewson (via GitHub)" <gi...@apache.org> on 2024/04/17 16:29:08 UTC
[PR] Temporary account lockout for repeated auth failure [couchdb]
rnewson opened a new pull request, #5032:
URL: https://github.com/apache/couchdb/pull/5032
## 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
- [x] 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Temporary account lockout for repeated auth failure [couchdb]
Posted by "rnewson (via GitHub)" <gi...@apache.org>.
rnewson commented on code in PR #5032:
URL: https://github.com/apache/couchdb/pull/5032#discussion_r1570224803
##########
src/couch/src/couch_lockout.erl:
##########
@@ -0,0 +1,50 @@
+% Licensed under the Apache License, Version 2.0 (the "License"); you may not
+% use this file except in compliance with the License. You may obtain a copy of
+% the License at
+%
+% http://www.apache.org/licenses/LICENSE-2.0
+%
+% Unless required by applicable law or agreed to in writing, software
+% distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+% WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+% License for the specific language governing permissions and limitations under
+% the License.
+
+-module(couch_lockout).
+-compile(tuple_calls).
Review Comment:
It is needed for the MochiReq:get(peer) call at the end of the module.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Temporary account lockout for repeated auth failure [couchdb]
Posted by "big-r81 (via GitHub)" <gi...@apache.org>.
big-r81 commented on code in PR #5032:
URL: https://github.com/apache/couchdb/pull/5032#discussion_r1570187409
##########
src/couch/src/couch_lockout.erl:
##########
@@ -0,0 +1,50 @@
+% Licensed under the Apache License, Version 2.0 (the "License"); you may not
+% use this file except in compliance with the License. You may obtain a copy of
+% the License at
+%
+% http://www.apache.org/licenses/LICENSE-2.0
+%
+% Unless required by applicable law or agreed to in writing, software
+% distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+% WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+% License for the specific language governing permissions and limitations under
+% the License.
+
+-module(couch_lockout).
+-compile(tuple_calls).
Review Comment:
Is this necessary? Wasn't it removed in OTP 21?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Temporary account lockout for repeated auth failure [couchdb]
Posted by "rnewson (via GitHub)" <gi...@apache.org>.
rnewson commented on code in PR #5032:
URL: https://github.com/apache/couchdb/pull/5032#discussion_r1581363290
##########
src/couch/src/couch_auth_lockout.erl:
##########
@@ -0,0 +1,85 @@
+% Licensed under the Apache License, Version 2.0 (the "License"); you may not
+% use this file except in compliance with the License. You may obtain a copy of
+% the License at
+%
+% http://www.apache.org/licenses/LICENSE-2.0
+%
+% Unless required by applicable law or agreed to in writing, software
+% distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+% WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+% License for the specific language governing permissions and limitations under
+% the License.
+
+-module(couch_auth_lockout).
+-include_lib("couch/include/couch_db.hrl").
+
+-define(LRU, couch_lockout_lru).
+
+-export([is_locked_out/3, lockout/3]).
+
+is_locked_out(#httpd{} = Req, UserName, UserSalt) ->
+ case lockout_mode() of
+ off ->
+ false;
+ Mode ->
+ is_locked_out_int(Req, Mode, UserName, UserSalt)
Review Comment:
I'm not a fan of that style tbh, a shame our formatter isn't opinionated about it.
##########
src/couch/src/couch_auth_lockout.erl:
##########
@@ -0,0 +1,85 @@
+% Licensed under the Apache License, Version 2.0 (the "License"); you may not
+% use this file except in compliance with the License. You may obtain a copy of
+% the License at
+%
+% http://www.apache.org/licenses/LICENSE-2.0
+%
+% Unless required by applicable law or agreed to in writing, software
+% distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+% WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+% License for the specific language governing permissions and limitations under
+% the License.
+
+-module(couch_auth_lockout).
+-include_lib("couch/include/couch_db.hrl").
+
+-define(LRU, couch_lockout_lru).
+
+-export([is_locked_out/3, lockout/3]).
+
+is_locked_out(#httpd{} = Req, UserName, UserSalt) ->
+ case lockout_mode() of
+ off ->
+ false;
+ Mode ->
+ is_locked_out_int(Req, Mode, UserName, UserSalt)
+ end.
+
+is_locked_out_int(#httpd{} = Req, Mode, UserName, UserSalt) ->
+ LockoutThreshold = lockout_threshold(),
+ case peer(Req) of
+ nopeer ->
+ false;
+ Peer ->
+ case ets_lru:lookup_d(?LRU, {Peer, UserName, UserSalt}) of
+ {ok, FailCount} when FailCount >= LockoutThreshold, Mode == enforce ->
+ true;
+ {ok, FailCount} when FailCount >= LockoutThreshold, Mode == warn ->
+ couch_log:warning(
+ "~p: authentication failure threshold reached for ~s from ~s",
+ [?MODULE, UserName, Peer]
+ ),
+ false;
+ {ok, _} ->
+ false;
+ not_found ->
+ false
+ end
+ end.
+
+lockout(#httpd{} = Req, UserName, UserSalt) ->
+ case peer(Req) of
+ nopeer ->
+ ok;
+ Peer ->
+ case lockout_mode() of
+ off ->
+ ok;
+ _ ->
+ ets_lru:update_counter(?LRU, {Peer, UserName, UserSalt}, 1)
+ end
+ end.
+
+lockout_mode() ->
+ case config:get("couch_auth_lockout", "mode", "off") of
+ "off" ->
Review Comment:
I'm not a fan of that style tbh, a shame our formatter isn't opinionated about it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Temporary account lockout for repeated auth failure [couchdb]
Posted by "rnewson (via GitHub)" <gi...@apache.org>.
rnewson commented on code in PR #5032:
URL: https://github.com/apache/couchdb/pull/5032#discussion_r1581364013
##########
src/couch/src/couch_auth_lockout.erl:
##########
@@ -0,0 +1,85 @@
+% Licensed under the Apache License, Version 2.0 (the "License"); you may not
+% use this file except in compliance with the License. You may obtain a copy of
+% the License at
+%
+% http://www.apache.org/licenses/LICENSE-2.0
+%
+% Unless required by applicable law or agreed to in writing, software
+% distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+% WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+% License for the specific language governing permissions and limitations under
+% the License.
+
+-module(couch_auth_lockout).
+-include_lib("couch/include/couch_db.hrl").
+
+-define(LRU, couch_lockout_lru).
+
+-export([is_locked_out/3, lockout/3]).
+
+is_locked_out(#httpd{} = Req, UserName, UserSalt) ->
+ case lockout_mode() of
+ off ->
+ false;
+ Mode ->
+ is_locked_out_int(Req, Mode, UserName, UserSalt)
+ end.
+
+is_locked_out_int(#httpd{} = Req, Mode, UserName, UserSalt) ->
+ LockoutThreshold = lockout_threshold(),
+ case peer(Req) of
+ nopeer ->
+ false;
+ Peer ->
+ case ets_lru:lookup_d(?LRU, {Peer, UserName, UserSalt}) of
+ {ok, FailCount} when FailCount >= LockoutThreshold, Mode == enforce ->
+ true;
+ {ok, FailCount} when FailCount >= LockoutThreshold, Mode == warn ->
+ couch_log:warning(
+ "~p: authentication failure threshold reached for ~s from ~s",
+ [?MODULE, UserName, Peer]
Review Comment:
yes, because we never come here if Peer was `{remote, Ref, Pid}`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Temporary account lockout for repeated auth failure [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #5032:
URL: https://github.com/apache/couchdb/pull/5032#discussion_r1581378669
##########
test/elixir/test/auth_lockout_test.exs:
##########
@@ -0,0 +1,48 @@
+defmodule AuthLockoutTest do
+ use CouchTestCase
+
+ @moduletag :authentication
+
+ @moduletag config: [
+ {
+ "admins",
+ "couch_auth_lockout",
+ "bar"
+ }
+ ]
+
+ test "lockout after multiple failed authentications", _context do
+ server_config = [
+ %{
+ :section => "couch_auth_lockout",
+ :key => "mode",
+ :value => "enforce"
+ }
+ ]
+
+ run_on_modified_server(
+ server_config,
+ fn -> test_couch_auth_lockout_enforcement() end
+ )
+ end
+
+ 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
Review Comment:
Might be worth having a few with a `warn` mode just validate that code path and that doesn't crash anything?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Temporary account lockout for repeated auth failure [couchdb]
Posted by "big-r81 (via GitHub)" <gi...@apache.org>.
big-r81 commented on code in PR #5032:
URL: https://github.com/apache/couchdb/pull/5032#discussion_r1574531907
##########
src/couch/src/couch_auth_lockout.erl:
##########
@@ -0,0 +1,65 @@
+% Licensed under the Apache License, Version 2.0 (the "License"); you may not
+% use this file except in compliance with the License. You may obtain a copy of
+% the License at
+%
+% http://www.apache.org/licenses/LICENSE-2.0
+%
+% Unless required by applicable law or agreed to in writing, software
+% distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+% WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+% License for the specific language governing permissions and limitations under
+% the License.
+
+-module(couch_auth_lockout).
+-include_lib("couch/include/couch_db.hrl").
+
+-define(LRU, couch_lockout_lru).
+
+-export([is_locked_out/3, lockout/3]).
+
+is_locked_out(#httpd{} = Req, UserName, UserSalt) ->
+ case config:get_boolean("couch_lockout", "enable", true) of
Review Comment:
Ah no, maybe not at the moment, because it isn't a cluster wide setting / functionality yet ...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Temporary account lockout for repeated auth failure [couchdb]
Posted by "big-r81 (via GitHub)" <gi...@apache.org>.
big-r81 commented on PR #5032:
URL: https://github.com/apache/couchdb/pull/5032#issuecomment-2408542962
@DarrenCook see https://docs.couchdb.org/en/stable/config/auth.html#chttpd_auth_lockout
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Temporary account lockout for repeated auth failure [couchdb]
Posted by "DarrenCook (via GitHub)" <gi...@apache.org>.
DarrenCook commented on PR #5032:
URL: https://github.com/apache/couchdb/pull/5032#issuecomment-2408538390
What is the config option to disable it? The 3.4 changelog only says:
> [#5032](https://github.com/apache/couchdb/issues/5032): Temporarily block access by client IP for repeated authentication failures. Can be disabled in config.
And I can't spot it in https://docs.couchdb.org/en/stable/config/http.html (or any other docs pages so far).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Temporary account lockout for repeated auth failure [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #5032:
URL: https://github.com/apache/couchdb/pull/5032#discussion_r1581362105
##########
src/couch/src/couch_auth_lockout.erl:
##########
@@ -0,0 +1,85 @@
+% Licensed under the Apache License, Version 2.0 (the "License"); you may not
+% use this file except in compliance with the License. You may obtain a copy of
+% the License at
+%
+% http://www.apache.org/licenses/LICENSE-2.0
+%
+% Unless required by applicable law or agreed to in writing, software
+% distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+% WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+% License for the specific language governing permissions and limitations under
+% the License.
+
+-module(couch_auth_lockout).
+-include_lib("couch/include/couch_db.hrl").
+
+-define(LRU, couch_lockout_lru).
+
+-export([is_locked_out/3, lockout/3]).
+
+is_locked_out(#httpd{} = Req, UserName, UserSalt) ->
+ case lockout_mode() of
+ off ->
+ false;
+ Mode ->
+ is_locked_out_int(Req, Mode, UserName, UserSalt)
+ end.
+
+is_locked_out_int(#httpd{} = Req, Mode, UserName, UserSalt) ->
+ LockoutThreshold = lockout_threshold(),
+ case peer(Req) of
+ nopeer ->
+ false;
+ Peer ->
+ case ets_lru:lookup_d(?LRU, {Peer, UserName, UserSalt}) of
+ {ok, FailCount} when FailCount >= LockoutThreshold, Mode == enforce ->
+ true;
+ {ok, FailCount} when FailCount >= LockoutThreshold, Mode == warn ->
+ couch_log:warning(
+ "~p: authentication failure threshold reached for ~s from ~s",
+ [?MODULE, UserName, Peer]
Review Comment:
Will `Peer` always be a string format-able value? Might be safer to make it `~p`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Temporary account lockout for repeated auth failure [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #5032:
URL: https://github.com/apache/couchdb/pull/5032#discussion_r1581381931
##########
rel/overlay/etc/default.ini:
##########
@@ -1026,3 +1026,26 @@ url = {{nouveau_url}}
; are too many design documents, that may generate a lot of logs. The default
; is to aggregate reports per database.
;ddoc_report = false
+
+[couch_auth_lockout]
+; CouchDB can temporarily lock out IP addresses that repeatedly fail authentication
+; mode can be set to one of three recognised values;
+; off - CouchDB does not track repeated authentication failures
+; warn - CouchDB will log a warning if repeated authentication failures occur
+; enforce - CouchDB will reject requests with a 403 status code if repeated
+; authentication failures occur
+;mode = off
+
+; The number of authentication failures above which lockout occurs.
+;threshold = 5
+
+; to control memory usage CouchDB will only track authentication failure count
+; for this many username and IP address pairs.
+; note: changing this setting requires a couchdb restart.
+;max_objects = 10000
+
+; The maximum time, in milliseconds, that CouchDB will track repeated authentication
+; 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
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Temporary account lockout for repeated auth failure [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #5032:
URL: https://github.com/apache/couchdb/pull/5032#discussion_r1581360442
##########
src/couch/src/couch_auth_lockout.erl:
##########
@@ -0,0 +1,85 @@
+% Licensed under the Apache License, Version 2.0 (the "License"); you may not
+% use this file except in compliance with the License. You may obtain a copy of
+% the License at
+%
+% http://www.apache.org/licenses/LICENSE-2.0
+%
+% Unless required by applicable law or agreed to in writing, software
+% distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+% WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+% License for the specific language governing permissions and limitations under
+% the License.
+
+-module(couch_auth_lockout).
+-include_lib("couch/include/couch_db.hrl").
+
+-define(LRU, couch_lockout_lru).
+
+-export([is_locked_out/3, lockout/3]).
+
+is_locked_out(#httpd{} = Req, UserName, UserSalt) ->
+ case lockout_mode() of
+ off ->
+ false;
+ Mode ->
+ is_locked_out_int(Req, Mode, UserName, UserSalt)
+ end.
+
+is_locked_out_int(#httpd{} = Req, Mode, UserName, UserSalt) ->
+ LockoutThreshold = lockout_threshold(),
+ case peer(Req) of
+ nopeer ->
+ false;
+ Peer ->
+ case ets_lru:lookup_d(?LRU, {Peer, UserName, UserSalt}) of
+ {ok, FailCount} when FailCount >= LockoutThreshold, Mode == enforce ->
+ true;
+ {ok, FailCount} when FailCount >= LockoutThreshold, Mode == warn ->
+ couch_log:warning(
+ "~p: authentication failure threshold reached for ~s from ~s",
+ [?MODULE, UserName, Peer]
+ ),
+ false;
+ {ok, _} ->
+ false;
+ not_found ->
+ false
+ end
+ end.
+
+lockout(#httpd{} = Req, UserName, UserSalt) ->
+ case peer(Req) of
+ nopeer ->
+ ok;
+ Peer ->
+ case lockout_mode() of
+ off ->
+ ok;
+ _ ->
+ ets_lru:update_counter(?LRU, {Peer, UserName, UserSalt}, 1)
+ end
+ end.
+
+lockout_mode() ->
+ case config:get("couch_auth_lockout", "mode", "off") of
+ "off" ->
Review Comment:
Minor suggestion: like above, erlfmt might allow these to be one line clauses, to save three lines.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Temporary account lockout for repeated auth failure [couchdb]
Posted by "rnewson (via GitHub)" <gi...@apache.org>.
rnewson commented on code in PR #5032:
URL: https://github.com/apache/couchdb/pull/5032#discussion_r1575050079
##########
src/couch/src/couch_primary_sup.erl:
##########
@@ -33,6 +33,15 @@ init([]) ->
{max_idle, config:get_integer("couch_passwords_cache", "max_idle", 600_000)}
]
]},
+ permanent, 5000, worker, [ets_lru]},
+ {couch_lockout_lru,
+ {ets_lru, start_link, [
+ couch_lockout_lru,
+ [
+ {max_objects, config:get_integer("couch_lockout", "max_objects", 10_000)},
+ {max_idle, config:get_integer("couch_lockout", "max_lifetime", 300_000)}
Review Comment:
ah poops, yes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Temporary account lockout for repeated auth failure [couchdb]
Posted by "big-r81 (via GitHub)" <gi...@apache.org>.
big-r81 commented on code in PR #5032:
URL: https://github.com/apache/couchdb/pull/5032#discussion_r1570238250
##########
src/couch/src/couch_lockout.erl:
##########
@@ -0,0 +1,50 @@
+% Licensed under the Apache License, Version 2.0 (the "License"); you may not
+% use this file except in compliance with the License. You may obtain a copy of
+% the License at
+%
+% http://www.apache.org/licenses/LICENSE-2.0
+%
+% Unless required by applicable law or agreed to in writing, software
+% distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+% WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+% License for the specific language governing permissions and limitations under
+% the License.
+
+-module(couch_lockout).
+-compile(tuple_calls).
Review Comment:
But maybe we can get completely rid of it? https://github.com/mochi/mochiweb/pull/215
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Temporary account lockout for repeated auth failure [couchdb]
Posted by "janl (via GitHub)" <gi...@apache.org>.
janl commented on PR #5032:
URL: https://github.com/apache/couchdb/pull/5032#issuecomment-2063799701
> 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Temporary account lockout for repeated auth failure [couchdb]
Posted by "rnewson (via GitHub)" <gi...@apache.org>.
rnewson commented on code in PR #5032:
URL: https://github.com/apache/couchdb/pull/5032#discussion_r1574735524
##########
src/couch/src/couch_auth_lockout.erl:
##########
@@ -0,0 +1,65 @@
+% Licensed under the Apache License, Version 2.0 (the "License"); you may not
+% use this file except in compliance with the License. You may obtain a copy of
+% the License at
+%
+% http://www.apache.org/licenses/LICENSE-2.0
+%
+% Unless required by applicable law or agreed to in writing, software
+% distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+% WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+% License for the specific language governing permissions and limitations under
+% the License.
+
+-module(couch_auth_lockout).
+-include_lib("couch/include/couch_db.hrl").
+
+-define(LRU, couch_lockout_lru).
+
+-export([is_locked_out/3, lockout/3]).
+
+is_locked_out(#httpd{} = Req, UserName, UserSalt) ->
+ case config:get_boolean("couch_lockout", "enable", true) of
Review Comment:
I'm open to suggestions here. putting the controls under `[chttpd_auth]` seems reasonable to me. @janl ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Temporary account lockout for repeated auth failure [couchdb]
Posted by "rnewson (via GitHub)" <gi...@apache.org>.
rnewson commented on code in PR #5032:
URL: https://github.com/apache/couchdb/pull/5032#discussion_r1575051008
##########
src/couch/src/couch_auth_lockout.erl:
##########
@@ -0,0 +1,65 @@
+% Licensed under the Apache License, Version 2.0 (the "License"); you may not
+% use this file except in compliance with the License. You may obtain a copy of
+% the License at
+%
+% http://www.apache.org/licenses/LICENSE-2.0
+%
+% Unless required by applicable law or agreed to in writing, software
+% distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+% WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+% License for the specific language governing permissions and limitations under
+% the License.
+
+-module(couch_auth_lockout).
+-include_lib("couch/include/couch_db.hrl").
+
+-define(LRU, couch_lockout_lru).
+
+-export([is_locked_out/3, lockout/3]).
+
+is_locked_out(#httpd{} = Req, UserName, UserSalt) ->
+ case config:get_boolean("couch_lockout", "enable", true) of
+ true ->
+ is_locked_out_int(Req, UserName, UserSalt);
+ false ->
+ false
+ end.
+
+is_locked_out_int(#httpd{} = Req, UserName, UserSalt) ->
+ LockoutThreshold = config:get_integer("couch_lockout", "threshold", 5),
+ case peer(Req) of
+ nopeer ->
+ false;
+ Peer ->
+ case ets_lru:lookup_d(?LRU, {Peer, UserName, UserSalt}) of
+ {ok, FailCount} when FailCount >= LockoutThreshold ->
+ true;
+ {ok, _} ->
+ false;
+ not_found ->
+ false
+ end
+ end.
+
+lockout(#httpd{} = Req, UserName, UserSalt) ->
+ case peer(Req) of
+ nopeer ->
+ ok;
+ Peer ->
+ case config:get_boolean("couch_lockout", "enable", true) of
Review Comment:
the helper for peer is because we need to handle the special case where the peer is that weird `remote` thing I added to make `_node` internal proxying work. but sure.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Temporary account lockout for repeated auth failure [couchdb]
Posted by "big-r81 (via GitHub)" <gi...@apache.org>.
big-r81 commented on code in PR #5032:
URL: https://github.com/apache/couchdb/pull/5032#discussion_r1574530667
##########
src/couch/src/couch_auth_lockout.erl:
##########
@@ -0,0 +1,65 @@
+% Licensed under the Apache License, Version 2.0 (the "License"); you may not
+% use this file except in compliance with the License. You may obtain a copy of
+% the License at
+%
+% http://www.apache.org/licenses/LICENSE-2.0
+%
+% Unless required by applicable law or agreed to in writing, software
+% distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+% WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+% License for the specific language governing permissions and limitations under
+% the License.
+
+-module(couch_auth_lockout).
+-include_lib("couch/include/couch_db.hrl").
+
+-define(LRU, couch_lockout_lru).
+
+-export([is_locked_out/3, lockout/3]).
+
+is_locked_out(#httpd{} = Req, UserName, UserSalt) ->
+ case config:get_boolean("couch_lockout", "enable", true) of
Review Comment:
@rnewson
Would it make sense now (after renaming to `couch_auth_lockout.erl`) to put it under the `[chttpd_auth]` section as `lockout` key?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Temporary account lockout for repeated auth failure [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #5032:
URL: https://github.com/apache/couchdb/pull/5032#discussion_r1575059544
##########
src/couch/src/couch_auth_lockout.erl:
##########
@@ -0,0 +1,65 @@
+% Licensed under the Apache License, Version 2.0 (the "License"); you may not
+% use this file except in compliance with the License. You may obtain a copy of
+% the License at
+%
+% http://www.apache.org/licenses/LICENSE-2.0
+%
+% Unless required by applicable law or agreed to in writing, software
+% distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+% WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+% License for the specific language governing permissions and limitations under
+% the License.
+
+-module(couch_auth_lockout).
+-include_lib("couch/include/couch_db.hrl").
+
+-define(LRU, couch_lockout_lru).
+
+-export([is_locked_out/3, lockout/3]).
+
+is_locked_out(#httpd{} = Req, UserName, UserSalt) ->
+ case config:get_boolean("couch_lockout", "enable", true) of
+ true ->
+ is_locked_out_int(Req, UserName, UserSalt);
+ false ->
+ false
+ end.
+
+is_locked_out_int(#httpd{} = Req, UserName, UserSalt) ->
+ LockoutThreshold = config:get_integer("couch_lockout", "threshold", 5),
+ case peer(Req) of
+ nopeer ->
+ false;
+ Peer ->
+ case ets_lru:lookup_d(?LRU, {Peer, UserName, UserSalt}) of
+ {ok, FailCount} when FailCount >= LockoutThreshold ->
+ true;
+ {ok, _} ->
+ false;
+ not_found ->
+ false
+ end
+ end.
+
+lockout(#httpd{} = Req, UserName, UserSalt) ->
+ case peer(Req) of
+ nopeer ->
+ ok;
+ Peer ->
+ case config:get_boolean("couch_lockout", "enable", true) of
Review Comment:
it's just a suggestion for consistency, if you prefer to leave it as is, that's fine as well
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Temporary account lockout for repeated auth failure [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #5032:
URL: https://github.com/apache/couchdb/pull/5032#discussion_r1581395210
##########
src/couch/src/couch_auth_lockout.erl:
##########
@@ -0,0 +1,85 @@
+% Licensed under the Apache License, Version 2.0 (the "License"); you may not
+% use this file except in compliance with the License. You may obtain a copy of
+% the License at
+%
+% http://www.apache.org/licenses/LICENSE-2.0
+%
+% Unless required by applicable law or agreed to in writing, software
+% distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+% WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+% License for the specific language governing permissions and limitations under
+% the License.
+
+-module(couch_auth_lockout).
+-include_lib("couch/include/couch_db.hrl").
+
+-define(LRU, couch_lockout_lru).
+
+-export([is_locked_out/3, lockout/3]).
+
+is_locked_out(#httpd{} = Req, UserName, UserSalt) ->
+ case lockout_mode() of
+ off ->
+ false;
+ Mode ->
+ is_locked_out_int(Req, Mode, UserName, UserSalt)
Review Comment:
That's fine, if you think it looks good. It's just style suggestion and erlfmt likes it either way.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Temporary account lockout for repeated auth failure [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #5032:
URL: https://github.com/apache/couchdb/pull/5032#discussion_r1581378669
##########
test/elixir/test/auth_lockout_test.exs:
##########
@@ -0,0 +1,48 @@
+defmodule AuthLockoutTest do
+ use CouchTestCase
+
+ @moduletag :authentication
+
+ @moduletag config: [
+ {
+ "admins",
+ "couch_auth_lockout",
+ "bar"
+ }
+ ]
+
+ test "lockout after multiple failed authentications", _context do
+ server_config = [
+ %{
+ :section => "couch_auth_lockout",
+ :key => "mode",
+ :value => "enforce"
+ }
+ ]
+
+ run_on_modified_server(
+ server_config,
+ fn -> test_couch_auth_lockout_enforcement() end
+ )
+ end
+
+ 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
Review Comment:
Might be worth having a test with a `warn` mode just validate that code path and that doesn't crash anything?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Temporary account lockout for repeated auth failure [couchdb]
Posted by "DarrenCook (via GitHub)" <gi...@apache.org>.
DarrenCook commented on PR #5032:
URL: https://github.com/apache/couchdb/pull/5032#issuecomment-2408549958
Thanks. The changelog made it sound like it is on by default, but the docs say `off` is the default, which makes more sense.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Temporary account lockout for repeated auth failure [couchdb]
Posted by "big-r81 (via GitHub)" <gi...@apache.org>.
big-r81 commented on code in PR #5032:
URL: https://github.com/apache/couchdb/pull/5032#discussion_r1750898837
##########
src/couch/src/couch_auth_lockout.erl:
##########
@@ -0,0 +1,65 @@
+% Licensed under the Apache License, Version 2.0 (the "License"); you may not
+% use this file except in compliance with the License. You may obtain a copy of
+% the License at
+%
+% http://www.apache.org/licenses/LICENSE-2.0
+%
+% Unless required by applicable law or agreed to in writing, software
+% distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+% WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+% License for the specific language governing permissions and limitations under
+% the License.
+
+-module(couch_auth_lockout).
+-include_lib("couch/include/couch_db.hrl").
+
+-define(LRU, couch_lockout_lru).
+
+-export([is_locked_out/3, lockout/3]).
+
+is_locked_out(#httpd{} = Req, UserName, UserSalt) ->
+ case config:get_boolean("couch_lockout", "enable", true) of
Review Comment:
@rnewson Since Jan gives a +1 for this, it should be moved to `[chttpd_auth]` with key `lockout`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Temporary account lockout for repeated auth failure [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #5032:
URL: https://github.com/apache/couchdb/pull/5032#discussion_r1575034941
##########
src/couch/src/couch_auth_lockout.erl:
##########
@@ -0,0 +1,65 @@
+% Licensed under the Apache License, Version 2.0 (the "License"); you may not
+% use this file except in compliance with the License. You may obtain a copy of
+% the License at
+%
+% http://www.apache.org/licenses/LICENSE-2.0
+%
+% Unless required by applicable law or agreed to in writing, software
+% distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+% WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+% License for the specific language governing permissions and limitations under
+% the License.
+
+-module(couch_auth_lockout).
+-include_lib("couch/include/couch_db.hrl").
+
+-define(LRU, couch_lockout_lru).
+
+-export([is_locked_out/3, lockout/3]).
+
+is_locked_out(#httpd{} = Req, UserName, UserSalt) ->
+ case config:get_boolean("couch_lockout", "enable", true) of
+ true ->
+ is_locked_out_int(Req, UserName, UserSalt);
+ false ->
+ false
+ end.
+
+is_locked_out_int(#httpd{} = Req, UserName, UserSalt) ->
+ LockoutThreshold = config:get_integer("couch_lockout", "threshold", 5),
+ case peer(Req) of
+ nopeer ->
+ false;
+ Peer ->
+ case ets_lru:lookup_d(?LRU, {Peer, UserName, UserSalt}) of
+ {ok, FailCount} when FailCount >= LockoutThreshold ->
+ true;
+ {ok, _} ->
+ false;
+ not_found ->
+ false
+ end
+ end.
+
+lockout(#httpd{} = Req, UserName, UserSalt) ->
+ case peer(Req) of
+ nopeer ->
+ ok;
+ Peer ->
+ case config:get_boolean("couch_lockout", "enable", true) of
Review Comment:
Since we have a helper function for `peer` would it make sense to have another for `enable`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Temporary account lockout for repeated auth failure [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #5032:
URL: https://github.com/apache/couchdb/pull/5032#discussion_r1575035578
##########
src/couch/src/couch_primary_sup.erl:
##########
@@ -33,6 +33,15 @@ init([]) ->
{max_idle, config:get_integer("couch_passwords_cache", "max_idle", 600_000)}
]
]},
+ permanent, 5000, worker, [ets_lru]},
+ {couch_lockout_lru,
+ {ets_lru, start_link, [
+ couch_lockout_lru,
+ [
+ {max_objects, config:get_integer("couch_lockout", "max_objects", 10_000)},
+ {max_idle, config:get_integer("couch_lockout", "max_lifetime", 300_000)}
Review Comment:
`{max_idle, ...}` should be `{max_lifetime, ...}`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Temporary account lockout for repeated auth failure [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #5032:
URL: https://github.com/apache/couchdb/pull/5032#discussion_r1581359131
##########
src/couch/src/couch_auth_lockout.erl:
##########
@@ -0,0 +1,85 @@
+% Licensed under the Apache License, Version 2.0 (the "License"); you may not
+% use this file except in compliance with the License. You may obtain a copy of
+% the License at
+%
+% http://www.apache.org/licenses/LICENSE-2.0
+%
+% Unless required by applicable law or agreed to in writing, software
+% distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+% WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+% License for the specific language governing permissions and limitations under
+% the License.
+
+-module(couch_auth_lockout).
+-include_lib("couch/include/couch_db.hrl").
+
+-define(LRU, couch_lockout_lru).
+
+-export([is_locked_out/3, lockout/3]).
+
+is_locked_out(#httpd{} = Req, UserName, UserSalt) ->
+ case lockout_mode() of
+ off ->
+ false;
+ Mode ->
+ is_locked_out_int(Req, Mode, UserName, UserSalt)
Review Comment:
Minor suggestion: short clauses like these can be formatted inline `off -> false;` and `Mode -> ...`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Temporary account lockout for repeated auth failure [couchdb]
Posted by "rnewson (via GitHub)" <gi...@apache.org>.
rnewson commented on code in PR #5032:
URL: https://github.com/apache/couchdb/pull/5032#discussion_r1581450262
##########
test/elixir/test/auth_lockout_test.exs:
##########
@@ -0,0 +1,48 @@
+defmodule AuthLockoutTest do
+ use CouchTestCase
+
+ @moduletag :authentication
+
+ @moduletag config: [
+ {
+ "admins",
+ "couch_auth_lockout",
+ "bar"
+ }
+ ]
+
+ test "lockout after multiple failed authentications", _context do
+ server_config = [
+ %{
+ :section => "couch_auth_lockout",
+ :key => "mode",
+ :value => "enforce"
+ }
+ ]
+
+ run_on_modified_server(
+ server_config,
+ fn -> test_couch_auth_lockout_enforcement() end
+ )
+ end
+
+ 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
Review Comment:
ah for sure, I also meant to add a test to prove it forgives you (and goes back to 401)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Temporary account lockout for repeated auth failure [couchdb]
Posted by "rnewson (via GitHub)" <gi...@apache.org>.
rnewson commented on code in PR #5032:
URL: https://github.com/apache/couchdb/pull/5032#discussion_r1569133699
##########
src/couch/src/couch_httpd_auth.erl:
##########
@@ -28,7 +28,7 @@
-export([cookie_auth_header/2]).
-export([handle_session_req/1, handle_session_req/2]).
--export([authenticate/4, verify_totp/2]).
Review Comment:
apparently not used externally, so I removed the export rather than update the arity.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Temporary account lockout for repeated auth failure [couchdb]
Posted by "rnewson (via GitHub)" <gi...@apache.org>.
rnewson merged PR #5032:
URL: https://github.com/apache/couchdb/pull/5032
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org