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 2023/05/26 15:07:11 UTC
[GitHub] [couchdb] rnewson opened a new pull request, #4625: Add optional logging of security issues when replicating
rnewson opened a new pull request, #4625:
URL: https://github.com/apache/couchdb/pull/4625
## Overview
Log insecure replication requests to guide administrators before they activate additional security measures (like disabling http protocol).
## Testing recommendations
Will be covered by tests
## Related Issues or Pull Requests
N/A
## 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
[GitHub] [couchdb] nickva commented on a diff in pull request #4625: Add optional logging of security issues when replicating
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #4625:
URL: https://github.com/apache/couchdb/pull/4625#discussion_r1207016244
##########
src/couch_replicator/src/couch_replicator_utils.erl:
##########
@@ -276,6 +278,88 @@ seq_encode(Seq) ->
% object. We are being maximally compatible here.
?JSON_ENCODE(Seq).
+%% Log uses of http protocol and uses of https protocol where verify_peer
+%% would fail.
+log_security_warnings(#rep{} = Rep) ->
+ case config:get_boolean("couch_replicator", "log_security_warnings", false) of
+ false ->
+ ok;
+ true ->
+ ok = check_security(Rep, source),
+ ok = check_security(Rep, target)
+ end.
+
+check_security(#rep{} = Rep, Type) ->
+ Url =
+ case Type of
+ source -> Rep#rep.source#httpdb.url;
+ target -> Rep#rep.target#httpdb.url
+ end,
+ #url{protocol = Protocol} = ibrowse_lib:parse_url(Url),
+ case Protocol of
+ http ->
+ couch_log:warning("**security warning** replication ~s has insecure ~s at ~s", [
+ rep_principal(Rep), Type, Url
+ ]),
+ ok;
+ https ->
+ CACertFile = config:get("couch_replicator", "cacertfile"),
+ case CACertFile of
+ undefined ->
+ couch_log:warning("security warnings enabled but no cacertfile configured", []);
+ CACertFile ->
+ try
+ ibrowse:send_req(Url, [], head, [], [
Review Comment:
Here we'd be blocking the scheduler when a node boots as each job tries to make ibrowse calls. Also, in the replicator we typically use our own connection pool and don't rely on ibrowse for that.
It's good that we do a try...catch but some requests start timing out at a minute or so, it would still block for too long and cause the scheduler to misbehave.
--
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
[GitHub] [couchdb] rnewson commented on a diff in pull request #4625: Add optional logging of security issues when replicating
Posted by "rnewson (via GitHub)" <gi...@apache.org>.
rnewson commented on code in PR #4625:
URL: https://github.com/apache/couchdb/pull/4625#discussion_r1214626340
##########
src/couch_replicator/src/couch_replicator_utils.erl:
##########
@@ -276,6 +278,98 @@ seq_encode(Seq) ->
% object. We are being maximally compatible here.
?JSON_ENCODE(Seq).
+%% Log uses of http protocol and uses of https protocol where verify_peer
+%% would fail.
+log_security_warnings(#rep{} = Rep) ->
+ case config:get_boolean("replicator", "log_security_warnings", false) of
+ false ->
+ ok;
+ true ->
+ ok = check_security(Rep, source),
+ ok = check_security(Rep, target)
+ end.
+
+check_security(#rep{} = Rep, Type) ->
+ Url =
+ case Type of
+ source -> Rep#rep.source#httpdb.url;
+ target -> Rep#rep.target#httpdb.url
+ end,
+ #url{protocol = Protocol} = ibrowse_lib:parse_url(Url),
+ case Protocol of
+ http ->
+ couch_log:warning("**security warning** replication ~s has insecure ~s at ~s", [
+ rep_principal(Rep), Type, Url
+ ]),
+ ok;
+ https ->
+ VerifyEnabled = config:get_boolean("replicator", "verify_ssl_certificates", false),
+ CACertFile = config:get("replicator", "ssl_trusted_certificates_file"),
Review Comment:
I think you mean I should look at `ssl_options` from #rep.options which is constructed as
```
VerifyCerts = cfg_boolean("verify_ssl_certificates", false),
ssl_verify_options(VerifyCerts)
```
```
-spec ssl_verify_options(true | false) -> [_].
ssl_verify_options(true) -> Used 1 times
CAFile = cfg("ssl_trusted_certificates_file"),
[{verify, verify_peer}, {cacertfile, CAFile}];
ssl_verify_options(false) ->
[{verify, verify_none}].
```
But I need to detect when cacertfile _is_ set and verify_peer is _not_ set. Which is not possible from this.
--
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
[GitHub] [couchdb] rnewson commented on a diff in pull request #4625: Add optional logging of security issues when replicating
Posted by "rnewson (via GitHub)" <gi...@apache.org>.
rnewson commented on code in PR #4625:
URL: https://github.com/apache/couchdb/pull/4625#discussion_r1207095343
##########
src/couch_replicator/src/couch_replicator_utils.erl:
##########
@@ -276,6 +278,88 @@ seq_encode(Seq) ->
% object. We are being maximally compatible here.
?JSON_ENCODE(Seq).
+%% Log uses of http protocol and uses of https protocol where verify_peer
+%% would fail.
+log_security_warnings(#rep{} = Rep) ->
+ case config:get_boolean("couch_replicator", "log_security_warnings", false) of
+ false ->
+ ok;
+ true ->
+ ok = check_security(Rep, source),
+ ok = check_security(Rep, target)
+ end.
+
+check_security(#rep{} = Rep, Type) ->
+ Url =
+ case Type of
+ source -> Rep#rep.source#httpdb.url;
+ target -> Rep#rep.target#httpdb.url
+ end,
+ #url{protocol = Protocol} = ibrowse_lib:parse_url(Url),
+ case Protocol of
+ http ->
+ couch_log:warning("**security warning** replication ~s has insecure ~s at ~s", [
+ rep_principal(Rep), Type, Url
+ ]),
+ ok;
+ https ->
+ CACertFile = config:get("couch_replicator", "cacertfile"),
Review Comment:
ah yes, forgot we had a setting for the replicator already
--
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
[GitHub] [couchdb] nickva commented on a diff in pull request #4625: Add optional logging of security issues when replicating
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #4625:
URL: https://github.com/apache/couchdb/pull/4625#discussion_r1209673705
##########
src/couch_replicator/src/couch_replicator_utils.erl:
##########
@@ -276,6 +278,88 @@ seq_encode(Seq) ->
% object. We are being maximally compatible here.
?JSON_ENCODE(Seq).
+%% Log uses of http protocol and uses of https protocol where verify_peer
+%% would fail.
+log_security_warnings(#rep{} = Rep) ->
Review Comment:
Perhaps that's closer the valid_endpoint_protocols/valid_proxy_protocols https://docs.couchdb.org/en/stable/config/replicator.html#replicator/valid_endpoint_protocols
Same prefix with a `_log` suffix like `valid_endpoint_protocols_log` it would take the same values as (list of allowed schemas)? When the option with `_log` suffix is used we'd only log a warning when it's used without the log the jobs will be rejected or become `failed`.
--
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
[GitHub] [couchdb] jaydoane commented on a diff in pull request #4625: Add optional logging of security issues when replicating
Posted by "jaydoane (via GitHub)" <gi...@apache.org>.
jaydoane commented on code in PR #4625:
URL: https://github.com/apache/couchdb/pull/4625#discussion_r1220415603
##########
rel/overlay/etc/default.ini:
##########
@@ -589,6 +589,12 @@ partitioned||* = true
; in this list will fail to run.
;valid_endpoint_protocols = http,https
+; When enabled CouchDB will log any replication that uses the insecure http protocol.
+;valid_endpoint_protocols_log = false
+
+; When enabled CouchDB will check the validity of the TLS certificates of source and target.
+;verify_ssl_certificates_log = fa;se
Review Comment:
/fa;se/false/
##########
rel/overlay/etc/default.ini:
##########
@@ -589,6 +589,12 @@ partitioned||* = true
; in this list will fail to run.
;valid_endpoint_protocols = http,https
+; When enabled CouchDB will log any replication that uses the insecure http protocol.
+;valid_endpoint_protocols_log = false
+
+; When enabled CouchDB will check the validity of the TLS certificates of source and target.
+;verify_ssl_certificates_log = fa;se
Review Comment:
s/fa;se/false/
--
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
[GitHub] [couchdb] nickva commented on a diff in pull request #4625: Add optional logging of security issues when replicating
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #4625:
URL: https://github.com/apache/couchdb/pull/4625#discussion_r1209673705
##########
src/couch_replicator/src/couch_replicator_utils.erl:
##########
@@ -276,6 +278,88 @@ seq_encode(Seq) ->
% object. We are being maximally compatible here.
?JSON_ENCODE(Seq).
+%% Log uses of http protocol and uses of https protocol where verify_peer
+%% would fail.
+log_security_warnings(#rep{} = Rep) ->
Review Comment:
Perhaps something that's closer to `valid_endpoint_protocols/valid_proxy_protocols` https://docs.couchdb.org/en/stable/config/replicator.html#replicator/valid_endpoint_protocols
First part would be the same then a `_log` suffix, like valid_endpoint_protocols_log`. It could take the same values as (list of comma separated schemas). When the option with `_log` suffix is used we'd only log a warning, and when it's used without the log, the jobs will be rejected or become `failed`.
--
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
[GitHub] [couchdb] rnewson commented on a diff in pull request #4625: Add optional logging of security issues when replicating
Posted by "rnewson (via GitHub)" <gi...@apache.org>.
rnewson commented on code in PR #4625:
URL: https://github.com/apache/couchdb/pull/4625#discussion_r1207096896
##########
src/couch_replicator/src/couch_replicator_utils.erl:
##########
@@ -276,6 +278,88 @@ seq_encode(Seq) ->
% object. We are being maximally compatible here.
?JSON_ENCODE(Seq).
+%% Log uses of http protocol and uses of https protocol where verify_peer
+%% would fail.
+log_security_warnings(#rep{} = Rep) ->
+ case config:get_boolean("couch_replicator", "log_security_warnings", false) of
+ false ->
+ ok;
+ true ->
+ ok = check_security(Rep, source),
+ ok = check_security(Rep, target)
+ end.
+
+check_security(#rep{} = Rep, Type) ->
+ Url =
+ case Type of
+ source -> Rep#rep.source#httpdb.url;
+ target -> Rep#rep.target#httpdb.url
+ end,
+ #url{protocol = Protocol} = ibrowse_lib:parse_url(Url),
+ case Protocol of
+ http ->
+ couch_log:warning("**security warning** replication ~s has insecure ~s at ~s", [
+ rep_principal(Rep), Type, Url
+ ]),
+ ok;
+ https ->
+ CACertFile = config:get("couch_replicator", "cacertfile"),
Review Comment:
I initially tried putting it in the parser but it has no context. the log messages would not be very useful if they didn't help you find which replicator doc had the problem.
--
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
[GitHub] [couchdb] rnewson commented on pull request #4625: Add optional logging of security issues when replicating
Posted by "rnewson (via GitHub)" <gi...@apache.org>.
rnewson commented on PR #4625:
URL: https://github.com/apache/couchdb/pull/4625#issuecomment-1564538007
I make additional requests to source and target (once each at job start time) with a custom verify_fun. I do this as it's hard to get the db/docid/user context from the real calls but also to avoid disturbing existing code for something informational.
--
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
[GitHub] [couchdb] nickva commented on a diff in pull request #4625: Add optional logging of security issues when replicating
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #4625:
URL: https://github.com/apache/couchdb/pull/4625#discussion_r1214676492
##########
src/couch_replicator/src/couch_replicator_utils.erl:
##########
@@ -276,6 +278,98 @@ seq_encode(Seq) ->
% object. We are being maximally compatible here.
?JSON_ENCODE(Seq).
+%% Log uses of http protocol and uses of https protocol where verify_peer
+%% would fail.
+log_security_warnings(#rep{} = Rep) ->
+ case config:get_boolean("replicator", "log_security_warnings", false) of
+ false ->
+ ok;
+ true ->
+ ok = check_security(Rep, source),
+ ok = check_security(Rep, target)
+ end.
+
+check_security(#rep{} = Rep, Type) ->
+ Url =
+ case Type of
+ source -> Rep#rep.source#httpdb.url;
+ target -> Rep#rep.target#httpdb.url
+ end,
+ #url{protocol = Protocol} = ibrowse_lib:parse_url(Url),
+ case Protocol of
+ http ->
+ couch_log:warning("**security warning** replication ~s has insecure ~s at ~s", [
+ rep_principal(Rep), Type, Url
+ ]),
+ ok;
+ https ->
+ VerifyEnabled = config:get_boolean("replicator", "verify_ssl_certificates", false),
+ CACertFile = config:get("replicator", "ssl_trusted_certificates_file"),
Review Comment:
fair enough
--
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
[GitHub] [couchdb] rnewson commented on pull request #4625: Add optional logging of security issues when replicating
Posted by "rnewson (via GitHub)" <gi...@apache.org>.
rnewson commented on PR #4625:
URL: https://github.com/apache/couchdb/pull/4625#issuecomment-1564543147
needs a tweak, I was so focused on the bad cases I forgot the good ones, need to add cacertfile :)
--
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
[GitHub] [couchdb] rnewson merged pull request #4625: Add optional logging of security issues when replicating
Posted by "rnewson (via GitHub)" <gi...@apache.org>.
rnewson merged PR #4625:
URL: https://github.com/apache/couchdb/pull/4625
--
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
[GitHub] [couchdb] rnewson commented on a diff in pull request #4625: Add optional logging of security issues when replicating
Posted by "rnewson (via GitHub)" <gi...@apache.org>.
rnewson commented on code in PR #4625:
URL: https://github.com/apache/couchdb/pull/4625#discussion_r1207143306
##########
src/couch_replicator/src/couch_replicator_utils.erl:
##########
@@ -276,6 +278,88 @@ seq_encode(Seq) ->
% object. We are being maximally compatible here.
?JSON_ENCODE(Seq).
+%% Log uses of http protocol and uses of https protocol where verify_peer
+%% would fail.
+log_security_warnings(#rep{} = Rep) ->
+ case config:get_boolean("couch_replicator", "log_security_warnings", false) of
+ false ->
+ ok;
+ true ->
+ ok = check_security(Rep, source),
+ ok = check_security(Rep, target)
+ end.
+
+check_security(#rep{} = Rep, Type) ->
+ Url =
+ case Type of
+ source -> Rep#rep.source#httpdb.url;
+ target -> Rep#rep.target#httpdb.url
+ end,
+ #url{protocol = Protocol} = ibrowse_lib:parse_url(Url),
+ case Protocol of
+ http ->
+ couch_log:warning("**security warning** replication ~s has insecure ~s at ~s", [
+ rep_principal(Rep), Type, Url
+ ]),
+ ok;
+ https ->
+ CACertFile = config:get("couch_replicator", "cacertfile"),
+ case CACertFile of
+ undefined ->
+ couch_log:warning("security warnings enabled but no cacertfile configured", []);
+ CACertFile ->
+ try
+ ibrowse:send_req(Url, [], head, [], [
Review Comment:
moved invocation to the parser side as suggested.
--
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
[GitHub] [couchdb] nickva commented on a diff in pull request #4625: Add optional logging of security issues when replicating
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #4625:
URL: https://github.com/apache/couchdb/pull/4625#discussion_r1209723736
##########
src/couch_replicator/src/couch_replicator_utils.erl:
##########
@@ -276,6 +278,98 @@ seq_encode(Seq) ->
% object. We are being maximally compatible here.
?JSON_ENCODE(Seq).
+%% Log uses of http protocol and uses of https protocol where verify_peer
+%% would fail.
+log_security_warnings(#rep{} = Rep) ->
+ case config:get_boolean("replicator", "log_security_warnings", false) of
+ false ->
+ ok;
+ true ->
+ ok = check_security(Rep, source),
+ ok = check_security(Rep, target)
+ end.
+
+check_security(#rep{} = Rep, Type) ->
+ Url =
+ case Type of
+ source -> Rep#rep.source#httpdb.url;
+ target -> Rep#rep.target#httpdb.url
+ end,
+ #url{protocol = Protocol} = ibrowse_lib:parse_url(Url),
+ case Protocol of
+ http ->
+ couch_log:warning("**security warning** replication ~s has insecure ~s at ~s", [
+ rep_principal(Rep), Type, Url
+ ]),
+ ok;
+ https ->
+ VerifyEnabled = config:get_boolean("replicator", "verify_ssl_certificates", false),
+ CACertFile = config:get("replicator", "ssl_trusted_certificates_file"),
Review Comment:
couch_replicator_parse parses and validates some of these options already, it might be possible read those options from there instead of re-parsing everything from scratch.
--
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
[GitHub] [couchdb] nickva commented on a diff in pull request #4625: Add optional logging of security issues when replicating
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #4625:
URL: https://github.com/apache/couchdb/pull/4625#discussion_r1209723736
##########
src/couch_replicator/src/couch_replicator_utils.erl:
##########
@@ -276,6 +278,98 @@ seq_encode(Seq) ->
% object. We are being maximally compatible here.
?JSON_ENCODE(Seq).
+%% Log uses of http protocol and uses of https protocol where verify_peer
+%% would fail.
+log_security_warnings(#rep{} = Rep) ->
+ case config:get_boolean("replicator", "log_security_warnings", false) of
+ false ->
+ ok;
+ true ->
+ ok = check_security(Rep, source),
+ ok = check_security(Rep, target)
+ end.
+
+check_security(#rep{} = Rep, Type) ->
+ Url =
+ case Type of
+ source -> Rep#rep.source#httpdb.url;
+ target -> Rep#rep.target#httpdb.url
+ end,
+ #url{protocol = Protocol} = ibrowse_lib:parse_url(Url),
+ case Protocol of
+ http ->
+ couch_log:warning("**security warning** replication ~s has insecure ~s at ~s", [
+ rep_principal(Rep), Type, Url
+ ]),
+ ok;
+ https ->
+ VerifyEnabled = config:get_boolean("replicator", "verify_ssl_certificates", false),
+ CACertFile = config:get("replicator", "ssl_trusted_certificates_file"),
Review Comment:
couch_replicator_parse parses, validates these options already, it might be possible read those options instead of re-parsing everything from scratch.
--
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
[GitHub] [couchdb] nickva commented on a diff in pull request #4625: Add optional logging of security issues when replicating
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #4625:
URL: https://github.com/apache/couchdb/pull/4625#discussion_r1209673705
##########
src/couch_replicator/src/couch_replicator_utils.erl:
##########
@@ -276,6 +278,88 @@ seq_encode(Seq) ->
% object. We are being maximally compatible here.
?JSON_ENCODE(Seq).
+%% Log uses of http protocol and uses of https protocol where verify_peer
+%% would fail.
+log_security_warnings(#rep{} = Rep) ->
Review Comment:
Perhaps something that's closer to `valid_endpoint_protocols/valid_proxy_protocols` https://docs.couchdb.org/en/stable/config/replicator.html#replicator/valid_endpoint_protocols
First part would be the same then have `_log` suffix, like `valid_endpoint_protocols_log`? When the option with `_log` suffix is used, we'd only log a warning, and when it's used without the `_log` ending, the jobs will be rejected or become `failed`. The settings then look the same and take the same type of values (a list of protocol schemas).
--
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
[GitHub] [couchdb] rnewson commented on a diff in pull request #4625: Add optional logging of security issues when replicating
Posted by "rnewson (via GitHub)" <gi...@apache.org>.
rnewson commented on code in PR #4625:
URL: https://github.com/apache/couchdb/pull/4625#discussion_r1207115377
##########
src/couch_replicator/src/couch_replicator_utils.erl:
##########
@@ -276,6 +278,88 @@ seq_encode(Seq) ->
% object. We are being maximally compatible here.
?JSON_ENCODE(Seq).
+%% Log uses of http protocol and uses of https protocol where verify_peer
+%% would fail.
+log_security_warnings(#rep{} = Rep) ->
Review Comment:
suggestions welcome, I don't find it too general but I don't feel strongly 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
[GitHub] [couchdb] nickva commented on a diff in pull request #4625: Add optional logging of security issues when replicating
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #4625:
URL: https://github.com/apache/couchdb/pull/4625#discussion_r1209718878
##########
src/couch_replicator/src/couch_replicator_doc_processor.erl:
##########
@@ -160,14 +160,15 @@ maybe_remove_state_fields(DbName, DocId) ->
couch_replicator_docs:remove_state_fields(DbName, DocId)
end.
-process_updated({DbName, _DocId} = Id, JsonRepDoc) ->
+process_updated({DbName, DocId} = Id, JsonRepDoc) ->
% Parsing replication doc (but not calculating the id) could throw an
% exception which would indicate this document is malformed. This exception
% should propagate to db_change function and will be recorded as permanent
% failure in the document. User will have to update the documet to fix the
% problem.
Rep0 = couch_replicator_parse:parse_rep_doc_without_id(JsonRepDoc),
Rep = Rep0#rep{db_name = DbName, start_time = os:timestamp()},
+ ok = couch_replicator_utils:log_security_warnings(Rep#rep{doc_id = DocId}),
Review Comment:
`doc_id` should already be set by the `parse_rep_doc_without_id`
--
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
[GitHub] [couchdb] nickva commented on a diff in pull request #4625: Add optional logging of security issues when replicating
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #4625:
URL: https://github.com/apache/couchdb/pull/4625#discussion_r1209718878
##########
src/couch_replicator/src/couch_replicator_doc_processor.erl:
##########
@@ -160,14 +160,15 @@ maybe_remove_state_fields(DbName, DocId) ->
couch_replicator_docs:remove_state_fields(DbName, DocId)
end.
-process_updated({DbName, _DocId} = Id, JsonRepDoc) ->
+process_updated({DbName, DocId} = Id, JsonRepDoc) ->
% Parsing replication doc (but not calculating the id) could throw an
% exception which would indicate this document is malformed. This exception
% should propagate to db_change function and will be recorded as permanent
% failure in the document. User will have to update the documet to fix the
% problem.
Rep0 = couch_replicator_parse:parse_rep_doc_without_id(JsonRepDoc),
Rep = Rep0#rep{db_name = DbName, start_time = os:timestamp()},
+ ok = couch_replicator_utils:log_security_warnings(Rep#rep{doc_id = DocId}),
Review Comment:
`doc_id` should already be set by the `parse_rep_doc_without_id`.
What's missing in the parser is the `dbname`, it would be a nice cleanup to pass `DbName` as a parameter, so parser could set it. Then update the replicator so there would be two parse APIs one which creates `#rep{}` records for transient replications, and one which creates them for replications started from `_replicator` db docs. But that would be a larger cleanup that might be better done separately. Doing the protocol schema warning right after parsing works well 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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva commented on a diff in pull request #4625: Add optional logging of security issues when replicating
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #4625:
URL: https://github.com/apache/couchdb/pull/4625#discussion_r1209673705
##########
src/couch_replicator/src/couch_replicator_utils.erl:
##########
@@ -276,6 +278,88 @@ seq_encode(Seq) ->
% object. We are being maximally compatible here.
?JSON_ENCODE(Seq).
+%% Log uses of http protocol and uses of https protocol where verify_peer
+%% would fail.
+log_security_warnings(#rep{} = Rep) ->
Review Comment:
Perhaps something that's closer to `valid_endpoint_protocols/valid_proxy_protocols` https://docs.couchdb.org/en/stable/config/replicator.html#replicator/valid_endpoint_protocols
First part would be the same then have `_log` suffix, like `valid_endpoint_protocols_log`? When the option with `_log` suffix is used, we'd only log a warning, and when it's used without the `_log` ending, the jobs will be rejected or become `failed`. The settings then look the same and take the same type of value.
--
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
[GitHub] [couchdb] nickva commented on a diff in pull request #4625: Add optional logging of security issues when replicating
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #4625:
URL: https://github.com/apache/couchdb/pull/4625#discussion_r1214639377
##########
src/couch_replicator/src/couch_replicator_utils.erl:
##########
@@ -276,6 +278,98 @@ seq_encode(Seq) ->
% object. We are being maximally compatible here.
?JSON_ENCODE(Seq).
+%% Log uses of http protocol and uses of https protocol where verify_peer
+%% would fail.
+log_security_warnings(#rep{} = Rep) ->
+ case config:get_boolean("replicator", "log_security_warnings", false) of
+ false ->
+ ok;
+ true ->
+ ok = check_security(Rep, source),
+ ok = check_security(Rep, target)
+ end.
+
+check_security(#rep{} = Rep, Type) ->
+ Url =
+ case Type of
+ source -> Rep#rep.source#httpdb.url;
+ target -> Rep#rep.target#httpdb.url
+ end,
+ #url{protocol = Protocol} = ibrowse_lib:parse_url(Url),
+ case Protocol of
+ http ->
+ couch_log:warning("**security warning** replication ~s has insecure ~s at ~s", [
+ rep_principal(Rep), Type, Url
+ ]),
+ ok;
+ https ->
+ VerifyEnabled = config:get_boolean("replicator", "verify_ssl_certificates", false),
+ CACertFile = config:get("replicator", "ssl_trusted_certificates_file"),
Review Comment:
We can check if it's a TSL configuration with `{is_ssl, true}` so we don't have to re-parse the URL and if `{verify, verify_none}` is set. But you're right, we'd still have to fetch `CACertFile = config:get("replicator", "ssl_trusted_certificates_file")` separately as we don't keep that bit around. (Keeping the cacertfile result in the ssl options even with `verify_none` could also be an option but there is a chance we might break the fragile ssl app options if we pass a {cacertfile, undefined} into 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
[GitHub] [couchdb] nickva commented on a diff in pull request #4625: Add optional logging of security issues when replicating
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #4625:
URL: https://github.com/apache/couchdb/pull/4625#discussion_r1209718878
##########
src/couch_replicator/src/couch_replicator_doc_processor.erl:
##########
@@ -160,14 +160,15 @@ maybe_remove_state_fields(DbName, DocId) ->
couch_replicator_docs:remove_state_fields(DbName, DocId)
end.
-process_updated({DbName, _DocId} = Id, JsonRepDoc) ->
+process_updated({DbName, DocId} = Id, JsonRepDoc) ->
% Parsing replication doc (but not calculating the id) could throw an
% exception which would indicate this document is malformed. This exception
% should propagate to db_change function and will be recorded as permanent
% failure in the document. User will have to update the documet to fix the
% problem.
Rep0 = couch_replicator_parse:parse_rep_doc_without_id(JsonRepDoc),
Rep = Rep0#rep{db_name = DbName, start_time = os:timestamp()},
+ ok = couch_replicator_utils:log_security_warnings(Rep#rep{doc_id = DocId}),
Review Comment:
`doc_id` should already be set by the `parse_rep_doc_without_id`.
What's missing in the parser is the `dbname`, it would be a nice cleanup to pass `DbName` as a parameter, so parse could set it. Then update the replicator so there would be two parse APIs one which creates `#rep{}` records for transient replications, and one which creates them for replications started from `_replicator` db docs. But that would be a larger cleanup that might be better done separately. Doing the protocol schema warning right after parsing works well 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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva commented on a diff in pull request #4625: Add optional logging of security issues when replicating
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #4625:
URL: https://github.com/apache/couchdb/pull/4625#discussion_r1209718079
##########
src/couch_replicator/src/couch_replicator_utils.erl:
##########
@@ -276,6 +278,88 @@ seq_encode(Seq) ->
% object. We are being maximally compatible here.
?JSON_ENCODE(Seq).
+%% Log uses of http protocol and uses of https protocol where verify_peer
+%% would fail.
+log_security_warnings(#rep{} = Rep) ->
+ case config:get_boolean("couch_replicator", "log_security_warnings", false) of
+ false ->
+ ok;
+ true ->
+ ok = check_security(Rep, source),
+ ok = check_security(Rep, target)
+ end.
+
+check_security(#rep{} = Rep, Type) ->
+ Url =
+ case Type of
+ source -> Rep#rep.source#httpdb.url;
+ target -> Rep#rep.target#httpdb.url
+ end,
+ #url{protocol = Protocol} = ibrowse_lib:parse_url(Url),
+ case Protocol of
+ http ->
+ couch_log:warning("**security warning** replication ~s has insecure ~s at ~s", [
+ rep_principal(Rep), Type, Url
+ ]),
+ ok;
+ https ->
+ CACertFile = config:get("couch_replicator", "cacertfile"),
+ case CACertFile of
+ undefined ->
+ couch_log:warning("security warnings enabled but no cacertfile configured", []);
+ CACertFile ->
+ try
+ ibrowse:send_req(Url, [], head, [], [
Review Comment:
Ideally we would pass in `dbname` to parser so it can both update the `#rep` record with it, and log http warnings. But that's a bit of a larger change. So, checking the protocols schema right after the parsing step, like done here, will work well.
However, since the TLS certificate check makes an external http connection we don't want to do that from any singleton replicator process. That includes the db event handler, the design doc processor, or the scheduler. A better place would seem to be the job process itself after it started and initialized. Somewhere in https://github.com/apache/couchdb/blob/main/src/couch_replicator/src/couch_replicator_scheduler_job.erl#L177 perhaps.
In the replication job, to improve isolation and not block the process if the verify function misbehaves, we could spawn separate TLS cert checker process for source and target endpoints. The would be spawn_link-ed from https://github.com/apache/couchdb/blob/main/src/couch_replicator/src/couch_replicator_scheduler_job.erl#L182 somewhere, then added to `#rep_state{}` record. After performing their `head` requests, emitting warning logs, they would exit. When they die we'd note their exit and clean them up in the `handle_info({'EXIT', ...})` handlers similar to https://github.com/apache/couchdb/blob/main/src/couch_replicator/src/couch_replicator_scheduler_job.erl#L270-L272. Because they are linked to the main process if the main replication job crashes, they would be automatically cleaned up 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
[GitHub] [couchdb] nickva commented on a diff in pull request #4625: Add optional logging of security issues when replicating
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #4625:
URL: https://github.com/apache/couchdb/pull/4625#discussion_r1209718878
##########
src/couch_replicator/src/couch_replicator_doc_processor.erl:
##########
@@ -160,14 +160,15 @@ maybe_remove_state_fields(DbName, DocId) ->
couch_replicator_docs:remove_state_fields(DbName, DocId)
end.
-process_updated({DbName, _DocId} = Id, JsonRepDoc) ->
+process_updated({DbName, DocId} = Id, JsonRepDoc) ->
% Parsing replication doc (but not calculating the id) could throw an
% exception which would indicate this document is malformed. This exception
% should propagate to db_change function and will be recorded as permanent
% failure in the document. User will have to update the documet to fix the
% problem.
Rep0 = couch_replicator_parse:parse_rep_doc_without_id(JsonRepDoc),
Rep = Rep0#rep{db_name = DbName, start_time = os:timestamp()},
+ ok = couch_replicator_utils:log_security_warnings(Rep#rep{doc_id = DocId}),
Review Comment:
`doc_id` should already be set by the `parse_rep_doc_without_id`.
What's missing in the parser is the `dbname`, it would be a nice cleanup to pass `DbName` as a parameter, so parse could set it. Then update the replicator so there would be two parse APIs one which create `#rep{}` records for transient replication, and one which creates them for replications started from `_replicator` db docs. But that would be a larger cleanup that might be better done separately. Doing the protocol schema warning right after parsing works well 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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva commented on a diff in pull request #4625: Add optional logging of security issues when replicating
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #4625:
URL: https://github.com/apache/couchdb/pull/4625#discussion_r1207021498
##########
src/couch_replicator/src/couch_replicator_utils.erl:
##########
@@ -276,6 +278,88 @@ seq_encode(Seq) ->
% object. We are being maximally compatible here.
?JSON_ENCODE(Seq).
+%% Log uses of http protocol and uses of https protocol where verify_peer
+%% would fail.
+log_security_warnings(#rep{} = Rep) ->
Review Comment:
A minor nit: `security` seems a bit too general, we're not checking all possible security issues, just the endpoint protocols.
##########
src/couch_replicator/src/couch_replicator_utils.erl:
##########
@@ -276,6 +278,88 @@ seq_encode(Seq) ->
% object. We are being maximally compatible here.
?JSON_ENCODE(Seq).
+%% Log uses of http protocol and uses of https protocol where verify_peer
+%% would fail.
+log_security_warnings(#rep{} = Rep) ->
+ case config:get_boolean("couch_replicator", "log_security_warnings", false) of
+ false ->
+ ok;
+ true ->
+ ok = check_security(Rep, source),
+ ok = check_security(Rep, target)
+ end.
+
+check_security(#rep{} = Rep, Type) ->
+ Url =
+ case Type of
+ source -> Rep#rep.source#httpdb.url;
+ target -> Rep#rep.target#httpdb.url
+ end,
+ #url{protocol = Protocol} = ibrowse_lib:parse_url(Url),
+ case Protocol of
+ http ->
+ couch_log:warning("**security warning** replication ~s has insecure ~s at ~s", [
+ rep_principal(Rep), Type, Url
+ ]),
+ ok;
+ https ->
+ CACertFile = config:get("couch_replicator", "cacertfile"),
+ case CACertFile of
+ undefined ->
+ couch_log:warning("security warnings enabled but no cacertfile configured", []);
+ CACertFile ->
+ try
+ ibrowse:send_req(Url, [], head, [], [
Review Comment:
Here we'd be blocking the scheduler when a node boot as each job tries to make ibrowse calls. Also, in the replicator we typically use our own connection pool and don't rely on ibrowse for that.
It's good that we do a try...catch but some requests start timing out at a minute or so, it would still block for too long and cause the scheduler to misbehave.
##########
src/couch_replicator/src/couch_replicator_utils.erl:
##########
@@ -276,6 +278,88 @@ seq_encode(Seq) ->
% object. We are being maximally compatible here.
?JSON_ENCODE(Seq).
+%% Log uses of http protocol and uses of https protocol where verify_peer
+%% would fail.
+log_security_warnings(#rep{} = Rep) ->
+ case config:get_boolean("couch_replicator", "log_security_warnings", false) of
+ false ->
+ ok;
+ true ->
+ ok = check_security(Rep, source),
+ ok = check_security(Rep, target)
+ end.
+
+check_security(#rep{} = Rep, Type) ->
+ Url =
+ case Type of
+ source -> Rep#rep.source#httpdb.url;
+ target -> Rep#rep.target#httpdb.url
+ end,
+ #url{protocol = Protocol} = ibrowse_lib:parse_url(Url),
+ case Protocol of
+ http ->
+ couch_log:warning("**security warning** replication ~s has insecure ~s at ~s", [
+ rep_principal(Rep), Type, Url
+ ]),
+ ok;
+ https ->
+ CACertFile = config:get("couch_replicator", "cacertfile"),
Review Comment:
This should be `ssl_trusted_certificates_file`? https://docs.couchdb.org/en/stable/config/replicator.html#replicator/ssl_trusted_certificates_file
--
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
[GitHub] [couchdb] rnewson commented on a diff in pull request #4625: Add optional logging of security issues when replicating
Posted by "rnewson (via GitHub)" <gi...@apache.org>.
rnewson commented on code in PR #4625:
URL: https://github.com/apache/couchdb/pull/4625#discussion_r1214674583
##########
src/couch_replicator/src/couch_replicator_utils.erl:
##########
@@ -276,6 +278,98 @@ seq_encode(Seq) ->
% object. We are being maximally compatible here.
?JSON_ENCODE(Seq).
+%% Log uses of http protocol and uses of https protocol where verify_peer
+%% would fail.
+log_security_warnings(#rep{} = Rep) ->
+ case config:get_boolean("replicator", "log_security_warnings", false) of
+ false ->
+ ok;
+ true ->
+ ok = check_security(Rep, source),
+ ok = check_security(Rep, target)
+ end.
+
+check_security(#rep{} = Rep, Type) ->
+ Url =
+ case Type of
+ source -> Rep#rep.source#httpdb.url;
+ target -> Rep#rep.target#httpdb.url
+ end,
+ #url{protocol = Protocol} = ibrowse_lib:parse_url(Url),
+ case Protocol of
+ http ->
+ couch_log:warning("**security warning** replication ~s has insecure ~s at ~s", [
+ rep_principal(Rep), Type, Url
+ ]),
+ ok;
+ https ->
+ VerifyEnabled = config:get_boolean("replicator", "verify_ssl_certificates", false),
+ CACertFile = config:get("replicator", "ssl_trusted_certificates_file"),
Review Comment:
doesn't seem a worthwhile saving to me.
--
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
[GitHub] [couchdb] nickva commented on a diff in pull request #4625: Add optional logging of security issues when replicating
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #4625:
URL: https://github.com/apache/couchdb/pull/4625#discussion_r1220297288
##########
src/couch_replicator/src/couch_replicator_doc_processor.erl:
##########
@@ -168,6 +168,7 @@ process_updated({DbName, _DocId} = Id, JsonRepDoc) ->
% problem.
Rep0 = couch_replicator_parse:parse_rep_doc_without_id(JsonRepDoc),
Rep = Rep0#rep{db_name = DbName, start_time = os:timestamp()},
+ couch_replicator_utils:valid_endpoint_protocols_log(Rep),
Review Comment:
tiny stile nit: use `ok =` just to match the `couch_replicator.erl` line.
##########
src/docs/src/config/replicator.rst:
##########
@@ -166,17 +166,39 @@ Replicator Database Configuration
.. _inet: http://www.erlang.org/doc/man/inet.html#setopts-2
- .. config:option:: valid_endpoint_protocols :: Replicator endpoint protocols
+ .. config:option:: valid_endpoint_protocols :: Replicator endpoint protocols
- .. versionadded:: 3.3
+ .. versionadded:: 3.3
- Valid replication endpoint protocols. Replication jobs with endpoint
- urls not in this list will fail to run::
+ Valid replication endpoint protocols. Replication jobs with endpoint
+ urls not in this list will fail to run::
- [replicator]
- valid_endpoint_protocols = http,https
+ [replicator]
+ valid_endpoint_protocols = http,https
+
+ .. config:option:: valid_endpoint_protocols_log :: Log security issues with endpoints
+
+ .. versionadded:: 3.4
+
+ When enabled, CouchDB will log any replication that uses the insecure http
+ protocol::
- .. config:option:: valid_proxy_protocols :: Replicator proxy protocols
+ [replicator]
+ valid_endpoint_protocols_log = true
+
+ .. config:option:: verify_ssl_certificates_log :: Log security issues with endpoints
+
+ .. versionadded:: 3.4
+
+ When enabled, and if ``ssl_trusted_certificates_file`` is configured
+ but ``verify_ssl_certificates`` is not, CouchDB will check the
+ validity of the TLS certificates of all sources and targets (
+ without causing the replication to fail) and log any issues::
+
+ [replicator]
+ verify_ssl_certificates_log = true
+
+ .. config:option:: valid_proxy_protocols :: Replicator proxy protocols
Review Comment:
We should probably add these to the default.ini commented with their respective defaults;
```
[replicator]
; Description....
;verify_ssl_certificates_log = true
```
--
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
[GitHub] [couchdb] nickva commented on a diff in pull request #4625: Add optional logging of security issues when replicating
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #4625:
URL: https://github.com/apache/couchdb/pull/4625#discussion_r1220297288
##########
src/couch_replicator/src/couch_replicator_doc_processor.erl:
##########
@@ -168,6 +168,7 @@ process_updated({DbName, _DocId} = Id, JsonRepDoc) ->
% problem.
Rep0 = couch_replicator_parse:parse_rep_doc_without_id(JsonRepDoc),
Rep = Rep0#rep{db_name = DbName, start_time = os:timestamp()},
+ couch_replicator_utils:valid_endpoint_protocols_log(Rep),
Review Comment:
tiny style nit: use `ok =` just to match the `couch_replicator.erl` line.
--
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