nickva commented on code in PR #6063:
URL: https://github.com/apache/couchdb/pull/6063#discussion_r3611890209
##########
src/couch/src/couch_auth_lockout.erl:
##########
@@ -27,7 +27,7 @@ is_locked_out(#httpd{} = Req, UserName, UserSalt) ->
is_locked_out_int(#httpd{} = Req, Mode, UserName, UserSalt) ->
LockoutThreshold = lockout_threshold(),
- case peer(Req) of
+ case Req#httpd.peer of
Review Comment:
Slight behavior change? Before on {remote, _, _} socket we always keyed by
the `nopeer` key. With the new behavior it would be something like `node(Pid)`?
Not sure when it would matter but maybe we could keep the old behavior and is
is_locked_out case exclude cases a peer is an `atom` (a header will never be).
##########
src/chttpd/src/chttpd.erl:
##########
@@ -1694,4 +1718,28 @@ handle_req_after_auth_test() ->
ok = meck:unload(chttpd_handlers),
ok = meck:unload(chttpd_auth).
+custom_peer_test() ->
+ Headers = mochiweb_headers:make([
+ {"HOST", "127.0.0.1:15984"}, {"X-Couch-Client-IP", "right_peer"}
+ ]),
+ MochiReq = mochiweb_request:new(
+ socket,
+ [],
+ 'PUT',
+ "/newdb",
+ version,
+ Headers
+ ),
+ Req = #httpd{
+ mochi_req = MochiReq,
+ begin_ts = {1458, 588713, 124003},
+ original_method = 'GET',
+ peer = "wrong_peer",
+ nonce = "nonce"
+ },
+ ok = meck:new(config, [passthrough]),
+ ok = meck:expect(config, get, fun("chttpd", "peer_header") ->
"x-couch-client-ip" end),
+ ?assertEqual("right_peer", peer(Req)),
+ ok = meck:unload(config).
Review Comment:
This test continues the pattern in this module, which is good, but in
general calling unload in the body of the test is not great. If one test fails
then it leave the mocked module hanging around. Something to be fixed in
another PR perhaps.
##########
rel/overlay/etc/default.ini:
##########
@@ -310,6 +310,11 @@ bind_address = 127.0.0.1
; Set to false to avoid scrubbing and revert to the previous behavior.
;scrub_json_request = true
+; Set to a string to override how couchdb determines the peer of a request
+; Default is undefined, so couchdb delegates to mochiweb which uses
+; X-Forwarded-For logic.
+;peer_header
Review Comment:
For `undefined` default values we usually use `;key =` pattern.
##########
src/chttpd/src/chttpd.erl:
##########
@@ -840,6 +841,29 @@ body(#httpd{mochi_req = MochiReq, req_body = ReqBody}) ->
validate_ctype(Req, Ctype) ->
couch_httpd:validate_ctype(Req, Ctype).
+peer(#httpd{} = Req) ->
+ peer(Req#httpd.mochi_req);
+peer(MochiReq) ->
+ case config:get("chttpd", "peer_header") of
+ undefined ->
+ peer_from_socket(MochiReq);
+ PeerHeader ->
+ case MochiReq:get_header_value(PeerHeader) of
Review Comment:
If the user explicitly set up a peer_header and it's missing, wonder if we
should fall back for `X-Forwarded-For` or got straight to getting from the
socket. In other words they tried to get away from XFF but misconfigured
something kind of a case?
##########
src/chttpd/src/chttpd.erl:
##########
@@ -840,6 +841,29 @@ body(#httpd{mochi_req = MochiReq, req_body = ReqBody}) ->
validate_ctype(Req, Ctype) ->
couch_httpd:validate_ctype(Req, Ctype).
+peer(#httpd{} = Req) ->
+ peer(Req#httpd.mochi_req);
+peer(MochiReq) ->
+ case config:get("chttpd", "peer_header") of
+ undefined ->
+ peer_from_socket(MochiReq);
+ PeerHeader ->
+ case MochiReq:get_header_value(PeerHeader) of
+ undefined ->
+ peer_from_socket(MochiReq);
+ Peer ->
Review Comment:
If an empty header is a frequent enough misconfiguration would it make sense
to handle that also as a fallback to peer_from_socket? Hmm, maybe not, just
trying to think of possible configuration footguns...
##########
src/chttpd/src/chttpd.erl:
##########
@@ -840,6 +841,29 @@ body(#httpd{mochi_req = MochiReq, req_body = ReqBody}) ->
validate_ctype(Req, Ctype) ->
couch_httpd:validate_ctype(Req, Ctype).
+peer(#httpd{} = Req) ->
+ peer(Req#httpd.mochi_req);
+peer(MochiReq) ->
+ case config:get("chttpd", "peer_header") of
+ undefined ->
+ peer_from_socket(MochiReq);
+ PeerHeader ->
+ case MochiReq:get_header_value(PeerHeader) of
+ undefined ->
+ peer_from_socket(MochiReq);
+ Peer ->
+ Peer
+ end
+ end.
+
+peer_from_socket(MochiReq) ->
+ case MochiReq:get(socket) of
+ {remote, _Pid, _} ->
+ nopeer;
+ _ ->
+ MochiReq:get(peer)
+ end.
Review Comment:
Wonder if we can remove the duplicate peer/peer_from_socket and just export
use the one copy from couch_httpd and instead of having any `nopeer` just treat
all `atom` keys as `nopeer` and don't lockout of them?
##########
src/docs/src/config/http.rst:
##########
@@ -288,6 +288,16 @@ HTTP Server Options
[chttpd]
disconnect_check_jitter_msec = 15000
+ .. config:option:: peer_header :: Header to use for peer
+
+ .. versionadded:: 3.5.3
+
+ If defined, CouchDB will use the value of this request header to
determine peer.
+ This affects logging as well as the auth lockout feature. ::
Review Comment:
The indent looks a bit off here
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
