chore_: remove private information for error log #6334
Conversation
Jenkins BuildsClick to see older builds (49)
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #6334 +/- ##
===========================================
- Coverage 59.67% 59.63% -0.04%
===========================================
Files 865 865
Lines 112981 112995 +14
===========================================
- Hits 67418 67387 -31
- Misses 37727 37773 +46
+ Partials 7836 7835 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
qfrank
force-pushed
the
chore/remove_private_info_for_error_log
branch
2 times, most recently
from
f5be723 to
24cd545
Compare
Agree. WDYT? @osmaczko |
My 2 cents @seanstrom @qfrank: the logging model relies on levels, if we break this model we will be adding unnecessary complexity. Could be risky as well because we would need to be 100% sure the release builds will have error logs redacted correctly, whatever mechanism we come up with.
If there's an error returned from Go code and there's a need to log private data, I think surrounding code should provide the necessary context via other log levels (like |
|
this is NO GO to me. it removes information such as full message id. Which is then going to make impossible to trace message reliability issues. I think it's fine to ensure that message content is not shared. We could also consider that full message id only appear in "trace" and not "debug". But such a change may lead to create issue in both bug investigation and benchmarking cc @AlbertoSoutullo Also, transfer of chat protocol ownership has been done to the waku chat team. While we are working on setting the right boundary, we need to ensure that the chat team is in the loop of this kind of changes cc @plopezlpz |
I tried to avoid truncate full message/peer id etc as I knew the message tracing requirement, if there exist in this PR that must be my mistake, I'll recheck @fryorcraken |
qfrank
force-pushed
the
chore/remove_private_info_for_error_log
branch
from
9a4053b to
200e9cb
Compare
qfrank
force-pushed
the
chore/remove_private_info_for_error_log
branch
from
200e9cb to
dcde21c
Compare
Logs in trace make the experiments extremely more complicated because of the overhead in logging. |
|
Copying my message from Discord to retain context:
Thanks for that. I'll let @pabl0___ review as he is closer to the matter.
There is nuance to that. I think it's important to better define "privacy" in this context. Privacy can usually be distilled down to "linkability" between a user (aka PII aka IP or wallet address) and an action/data (aka a message) In this context, why would one censor logs? @osmaczko has already highlighted that such leakage should be out-of-scope for the Status app, as long as standard practices are followed:
Now, we know very well from our domain that there is also the risk of users sharing logs for bug investigation purposes (which is what I called "debugging" earlier, I did not strictly mean running gdb). I think this is the core of the question: do we want to ensure unlinkability between a user's github login, and them being the author of a specific message, or participation to a community? Well, the "good" (it's not good) news for now, is that Waku does not provide strong sender privacy (just yet). so the logs are not the weakest link (for now). What we do want, is full msg id, especially for logs shared by CCs, QA, and test runs, so that we can trace messages in case of reliability issues, across system. So it seems fair to do:
Now, I realise I may have jumped the gun with my comment here: #6334 (comment) My apologies. And you have pushed back here 👍 In general, let's ensure waku chat team is asked for review of changes in chat code (ie @plopezlpz ) please. |
Makes sense @fryorcraken. We should indeed use the log levels, and the
Right 👍🏼 And we already discussed some nuances regarding what privacy can or cannot be in the recent past, for example, that we shouldn't log addresses. I don't recall us ever agreeing exactly on the mapping between what data can be logged at which level and how much the data should be redacted in case we need some traceability. This seems to be a good exercise for us. Status already aligned some time ago with the Legal team that we will not ask users to share logs, no matter the level. We also cannot guarantee logs at the debug level won't leak some data that could be considered private or exploited because this level is supposed to be used for development pretty much. If the user shares logs they are always going to do so at their own risk. Additionally, we are trying to make pre-login logs safer to share at the
In mobile devices, we are starting the work to store logs only within the protected app directory. Nowadays in Android, logs like
Thanks for the reminder 👍🏼
@AlbertoSoutullo would logs at the |
qfrank
force-pushed
the
chore/remove_private_info_for_error_log
branch
from
dcde21c to
f172f29
Compare
|
@ilmotta From my experience, when we have a heavy logging we need to be careful with not overloading the monitoring system. |
The Lines 1806 to 1808 in f494d09
Interesting :) I think I understand what you mean, but could you elaborate on how you imagine using these "specific" logs? When would you use them instead of the normal logging system based on levels? |
relate comment
I've also tried search with keyword:
fmt.Error:fmt.Errorf-search-result.txt.zip
truncated some sensitive information in error messages.