You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@couchdb.apache.org by GitBox <gi...@apache.org> on 2020/07/23 15:58:36 UTC
[GitHub] [couchdb] iilyak opened a new pull request #3031: Clean up logs
iilyak opened a new pull request #3031:
URL: https://github.com/apache/couchdb/pull/3031
## Overview
When OTP processes crash OTP logs the current state of the process and last message it received in the log file.
However this feature can lead to leakage of sensitive date into log files. Preventing this from happening requires fixing both sources of leakage. The leakage via state term could be prevented via usage of `format_status/2` callback intended to reformat state before passing it to logger. The leakage via last message is harder to prevent because there is no good way to detect sensitive data in arbitrary term. The safest approach is to remove the last message term before it is included into log message. However in some cases last message can be a useful debugging tool that's why instead of removing it altogether I've added `[log]-> strip_last_msg = false` configuration option. The extra configuration option wouldn't cause an extra latency thanks to an optimization in couch_log application.
This PR removes sensitive data from state terms. It also tries to make state size predictable by replacing complex terms with size of the term (for example for lists we use `length(List)` and for dict we use `dict:size(Dict)`).
This PR is not sufficient to remove all sensitive data from logs, see other parts of the solution in the following independent PRs:
* https://github.com/apache/couchdb-config/pull/30 - this would require a follow up PR to pass sensitive flag
* https://github.com/apache/couchdb-ibrowse/pull/4 - similar PR for ibrowse
## Testing recommendations
Regression testing can be done using `make check`
## Related Issues or Pull Requests
* This is an alternative to https://github.com/apache/couchdb/pull/2627
## 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`
- [ ] A PR for documentation changes has been made in https://github.com/apache/couchdb-documentation
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] tonysun83 commented on a change in pull request #3031: Clean up logs
Posted by GitBox <gi...@apache.org>.
tonysun83 commented on a change in pull request #3031:
URL: https://github.com/apache/couchdb/pull/3031#discussion_r461326057
##########
File path: src/setup/src/setup_httpd.erl
##########
@@ -173,3 +173,8 @@ get_body(Req) ->
couch_log:notice("Body Fail: ~p~n", [Else]),
couch_httpd:send_error(Req, 400, <<"bad_request">>, <<"Missing JSON body'">>)
end.
+
+remove_sensitive(KVList0) ->
+ KVList1 = lists:keyreplace(<<"username">>, 1, KVList0, {<<"username">>, <<"****">>}),
+ KVList2 = lists:keyreplace(<<"password">>, 1, KVList1, {<<"password">>, <<"****">>}),
+ KVList2.
Review comment:
is this redundant or just a stylist preference?
##########
File path: src/couch_log/src/couch_log_config.erl
##########
@@ -97,4 +98,10 @@ transform(max_message_size, SizeStr) ->
Size -> Size
catch _:_ ->
16000
- end.
\ No newline at end of file
+ end;
+
+transform(strip_last_msg, "false") ->
+ false;
+
+transform(strip_last_msg, _) ->
+ true.
Review comment:
add an end of line here
##########
File path: src/couch_mrview/src/couch_mrview_index.erl
##########
@@ -324,3 +325,14 @@ update_local_purge_doc(Db, State, PSeq) ->
BaseDoc
end,
couch_db:update_doc(Db, Doc, []).
+
+format_status(_Opt, [_PDict, State]) ->
Review comment:
If we're doing this for traditional views as well, do we need to modify this section as well: https://github.com/apache/couchdb/blob/master/src/dreyfus/src/dreyfus_index.erl#L37?
##########
File path: src/couch/src/couch_work_queue.erl
##########
@@ -186,3 +186,18 @@ code_change(_OldVsn, State, _Extra) ->
handle_info(X, Q) ->
{stop, X, Q}.
+
+format_status(_Opt, [_PDict, Queue]) ->
+ #q{
+ queue = Q,
+ blocked = Blocked,
+ work_waiters = Waiters
+ } = Queue,
+ Scrubbed = Queue#q{
+ queue = queue:len(Q),
+ blocked = length(Blocked),
+ work_waiters = length(Waiters)
+ },
+ [{data, [{"State",
+ ?record_to_keyval(q, Scrubbed)
+ }]}].
Review comment:
whitespace
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] tonysun83 edited a comment on pull request #3031: Clean up logs
Posted by GitBox <gi...@apache.org>.
tonysun83 edited a comment on pull request #3031:
URL: https://github.com/apache/couchdb/pull/3031#issuecomment-665280532
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] tonysun83 commented on pull request #3031: Clean up logs
Posted by GitBox <gi...@apache.org>.
tonysun83 commented on pull request #3031:
URL: https://github.com/apache/couchdb/pull/3031#issuecomment-665280532
I think shortening the complex terms is the right approach. Similar to what I wrote in the other PR about more meaningful output, I was thinking if providing a length(Term) is necessary. It'll just be a bunch of integers in the output that don't have much meaning. Some other options
1) Leaving it nil or blank (like what you did for `mrviews`
2) Only show first X number or characters (a little bit more work)
3) Prepend it with some descriptor '{<name of field>_size, length(Term)}
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] iilyak merged pull request #3031: Clean up logs
Posted by GitBox <gi...@apache.org>.
iilyak merged pull request #3031:
URL: https://github.com/apache/couchdb/pull/3031
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] iilyak commented on a change in pull request #3031: Clean up logs
Posted by GitBox <gi...@apache.org>.
iilyak commented on a change in pull request #3031:
URL: https://github.com/apache/couchdb/pull/3031#discussion_r461756723
##########
File path: src/setup/src/setup_httpd.erl
##########
@@ -173,3 +173,8 @@ get_body(Req) ->
couch_log:notice("Body Fail: ~p~n", [Else]),
couch_httpd:send_error(Req, 400, <<"bad_request">>, <<"Missing JSON body'">>)
end.
+
+remove_sensitive(KVList0) ->
+ KVList1 = lists:keyreplace(<<"username">>, 1, KVList0, {<<"username">>, <<"****">>}),
+ KVList2 = lists:keyreplace(<<"password">>, 1, KVList1, {<<"password">>, <<"****">>}),
+ KVList2.
Review comment:
Just a style. Can change if needed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] iilyak commented on pull request #3031: Clean up logs
Posted by GitBox <gi...@apache.org>.
iilyak commented on pull request #3031:
URL: https://github.com/apache/couchdb/pull/3031#issuecomment-666337564
`couch_jobs_tests:210: type_timeouts_and_server...*failed*` - is known to be flaky.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] iilyak commented on a change in pull request #3031: Clean up logs
Posted by GitBox <gi...@apache.org>.
iilyak commented on a change in pull request #3031:
URL: https://github.com/apache/couchdb/pull/3031#discussion_r462317114
##########
File path: src/couch_mrview/src/couch_mrview_index.erl
##########
@@ -324,3 +325,14 @@ update_local_purge_doc(Db, State, PSeq) ->
BaseDoc
end,
couch_db:update_doc(Db, Doc, []).
+
+format_status(_Opt, [_PDict, State]) ->
Review comment:
done
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] tonysun83 commented on pull request #3031: Clean up logs
Posted by GitBox <gi...@apache.org>.
tonysun83 commented on pull request #3031:
URL: https://github.com/apache/couchdb/pull/3031#issuecomment-665980112
+1, please squash and merge, good work!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org