-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Clean up logs 3.x.v2 #3380
Clean up logs 3.x.v2 #3380
Conversation
src/chttpd/src/chttpd_node.erl
Outdated
@@ -71,7 +71,9 @@ handle_node_req(#httpd{method='PUT', path_parts=[_, Node, <<"_config">>, Section | |||
Value = couch_util:trim(chttpd:json_body(Req)), | |||
Persist = chttpd:header_value(Req, "X-Couch-Persist") /= "false", | |||
OldValue = call_node(Node, config, get, [Section, Key, ""]), | |||
case call_node(Node, config, set, [Section, Key, ?b2l(Value), Persist]) of | |||
IsSensitive = Section == <<"admins">>, | |||
Opts = #{persisit => Persist, sensitive => IsSensitive}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit
Opts = #{persisit => Persist, sensitive => IsSensitive}, | |
Opts = #{persist => Persist, sensitive => IsSensitive}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I closely reviewed the first few cases and skimmed the rest as they seemed the same idea but adapted for the gen_server in question.
I like the structure of the code but I am concerned about the choices made. This is a great deal of redaction and most of it appears not to be related to credentials or sensitive information (reducing the btree record to just the size of the btree, for example).
our logs are an important source of debugging information, particularly for users who aren't comfortable with digging around in a remsh (or are in an environment where they are prohibited from doing so).
I don't think redaction beyond obvious things like passwords can be the default behaviour of CouchDB. I'm less sure if there should be an option for the kind of deep "cleanup" here. With my couchdb support hat on, would I be happy to diagnose a problem from a CouchDB log this heavily modified?
src/chttpd/src/chttpd_node.erl
Outdated
@@ -71,7 +71,9 @@ handle_node_req(#httpd{method='PUT', path_parts=[_, Node, <<"_config">>, Section | |||
Value = couch_util:trim(chttpd:json_body(Req)), | |||
Persist = chttpd:header_value(Req, "X-Couch-Persist") /= "false", | |||
OldValue = call_node(Node, config, get, [Section, Key, ""]), | |||
case call_node(Node, config, set, [Section, Key, ?b2l(Value), Persist]) of | |||
IsSensitive = Section == <<"admins">>, | |||
Opts = #{persisit => Persist, sensitive => IsSensitive}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo
src/setup/src/setup_httpd.erl
Outdated
@@ -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">>, <<"****">>}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
username isn't sensitive (or shouldn't be!)
src/setup/src/setup_httpd.erl
Outdated
remove_sensitive(KVList0) -> | ||
KVList1 = lists:keyreplace(<<"username">>, 1, KVList0, {<<"username">>, <<"****">>}), | ||
KVList2 = lists:keyreplace(<<"password">>, 1, KVList1, {<<"password">>, <<"****">>}), | ||
KVList2. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a style point (and I guess you're anticipating other fields in future) but I would just return the lists:keyreplace result
src/couch/src/couch_bt_engine.erl
Outdated
@@ -192,6 +193,44 @@ handle_db_updater_info({'DOWN', Ref, _, _, _}, #st{fd_monitor=Ref} = St) -> | |||
{stop, normal, St#st{fd=undefined, fd_monitor=closed}}. | |||
|
|||
|
|||
format_status(normal, [_PDict, #st{} = St]) -> | |||
[{data, [{"State", St}]}]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do like that we don't interfere with a sys:get_status call from a remsh (etc).
src/couch/src/couch_bt_engine.erl
Outdated
[{data, [{"State", reduce_record(St)}]}]. | ||
|
||
|
||
reduce_record(#st{} = St) -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think redact
is a better word as we have a quite different meaning for reduce
in couchdb.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The format_status/2
was added to prevent huge btree structures hitting logs. Since the reduction of terms is not longer a goal I am thinking about removing this function.
src/couch/src/couch_bt_engine.erl
Outdated
purge_seq_tree = reduce_record(PurgeSeqTree, undefined) | ||
}; | ||
|
||
reduce_record(Header, _) when element(1, Header) =:= db_header -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be when is_record(Header, db_header)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The is_record/2
works only for records in scope of a module (defined locally or included). The db_header
is a private record defined in couch_bt_engine_header.erl
.
couchdb/src/couch/src/couch_bt_engine.erl:224: record db_header undefined
Compiling couchdb/src/couch/src/couch_bt_engine.erl failed:
ERROR: compile failed while processing couchdb/src/couch: rebar_abort
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm, true.
src/couch/src/couch_bt_engine.erl
Outdated
reduce_record(Header, _) when element(1, Header) =:= db_header -> | ||
couch_bt_engine_header:reduce_record(Header); | ||
|
||
reduce_record(Tree, _) when element(1, Tree) =:= btree -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be when is_record(Tree, btree)
.
%% We just need a correctly shaped btree record, | ||
%% so we can pass Fd = nil | ||
{ok, Tree} = couch_btree:open(TreeState, nil), | ||
couch_btree:size(Tree). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment as above
pids=Pids | ||
} = State, | ||
Scrubbed = State#state{ | ||
pids={length, length(Pids)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pids aren't sensitive.
funs = Funs | ||
} = State, | ||
Scrubbed = State#evstate{ | ||
ddocs = {dict_size, dict:size(DDocs)}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
arguably not sensitive either
Personally I hit the same problem over and over. I have a bug and go to check logs. The log contains huge term which we truncate and all important information about other fields of a record omitted. |
Ah, sure, I get that point, thank you. |
Hey hey, just casually following this, but the diff is a bit complex. I think this might have a better chance if we focus on redacting sensitive data first, and tackle garbage cleanup later. |
Agree. I'll update the PR to decouple garbage cleanup out of this PR. |
I don't understand these;
Why so many conversions to map? |
There is no conversion to map for the state of the |
I am very uncomfortable to see that pattern cut and paste so many times, it strongly implies a bad design. I think I see how you got here though. |
we also can't use the ableist slur "sanitize" but that's a mechanical change ("scrub" perhaps?). will ponder if the boilerplating can be reduced/removed (my concern is not just the duplication but also the deviation from OTP and the need to remember to do this for every new gen_server). |
The alternative to
compared to
|
now you've explained I'm fine with the "context" item being a map. What's really awkward is having to alter every gen_server in an identical way in order to pass it. |
The alternative is a common module somewhere.
However the gen_servers we update are in different applications. We don't have an application which is included in all the others. I could create a new app which would hold the module. This app would be only for utility functions and couldn't depend on other apps to avoid circular dependencies. |
could this be done with a parse transform? When we used lager it added a parse transform (https://github.com/apache/couchdb-lager/blob/master/src/lager_transform.erl) which captures module name among other things. |
In fact my first take on this problem was a parse transform. The problem with this approach is use of |
changing all the rebar.configs seems ok, and would have the advantage that any new gen_servers are automatically covered. Did you show the parse_transform version? I've lost track. |
I didn't |
I think I would need to create a new repo for the parse_transform code. Because it need to be used as dependency for other applications which are not part of the monolithic repository. I am trying to choose between
|
Good cleanup @iilyak! This is a lot easer to comprehend. The one place where this could do with a comment about what happens is https://github.com/apache/couchdb/pull/3380/files#diff-33000329e1e5eb2554d72f4675fb32c2bf3e0599c981f044683467cb25efc02aR378 which seems very meta and it is not clear to me what is happening there, or if we still need it, now that ddocs are out of scope for redacting. I agree with @rnewson on the duplication and agree with methods to mitigate. A separate repo sounds good. I would go with option 1. until we have more transforms, at which point we can move things around. |
I rolled back the controversial changes and included only the changes needed to prevent leaking of passwords. I think we can have separate PR if we would want to prevent leaking of users' data or minimize state terms. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@iilyak this is very tidy, thank you for persevering. +1 on merging though do please squash beforehand.
9d2e7af
to
52bb729
Compare
52bb729
to
19625a5
Compare
Overview
This PR started as a port of #3031 (Clean up logs) into 3.x branch. The main goal of the PR is to remove sensitive data from the logs.
However as it was pointed out by reviewers it might be not the right approach to convert shape of gen_server state for a CouchDB version which is already widely deployed. Another problem with the scope of the PR is lack of definition of sensitive data. There are multiple points of view on the sensitive data:
This means that there is no generic solution which would fit all. In order to solve this problem for the purposes of this PR the sensitive data are defined as "user credentials".
Before going further down with description of the solution presented in the PR. I wanted to mention the requirements as I understood them:
sys:get_status/1
should return state as is (wrapped in[{data, [{"State", Term}]}]
) see https://erlang.org/doc/man/gen_server.html#Module:format_status-2couch_log_formatter
should be able to call a configurable sanitizer module which would deal with changing the shape of log entries and removing user's data from termssys:get_status/1
use case and termination of a process.In order to achieve 1) we need to handle
:format_status(normal, ...)
and:format_status(terminate, ...)
differently.In order to achieve 2) we need to pass the callback module name so it is available in
couch_log_formatter
. We also pass the process dictionary so sanitizer module could get some meta information about the process (for example tracing span_id). The process dictionary is not logged by default. However sanitizer module can include elements from process dictionary in a term it returns.The approach to
format_status/2
is to return term as is when first argument isnormal
in order forsys:get_status
to return original term. In both cases we remove "sensitive data" before returning the term fromformat_status/2
. Theterminate
clause of aformat_status/2
returns{Term, Ctx :: map()}
. TheCtx
includesmodule
anddictionary
keys for now. We need this information so sanitizer could distinguish messages coming from different gen_servers.Testing recommendations
make eunit
Related Issues or Pull Requests
Checklist
rel/overlay/etc/default.ini