feat_: add pre login log #6285
feat_: add pre login log #6285
Conversation
Jenkins BuildsClick to see older builds (96)
|
qfrank
force-pushed
the
feat/pre_login_log
branch
3 times, most recently
from
ce0993d to
3d90b00
Compare
Codecov ReportAttention: Patch coverage is
❌ Your patch status has failed because the patch coverage (43.58%) is below the target coverage (50.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## develop #6285 +/- ##
============================================
+ Coverage 0.45% 59.65% +59.19%
============================================
Files 824 865 +41
Lines 109683 113135 +3452
============================================
+ Hits 497 67488 +66991
+ Misses 109129 37786 -71343
- Partials 57 7861 +7804
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
@qfrank, code LGTM. I'm just missing some evidence that the default ERROR level during pre-login is not leaking any private information. Could you please verify the log Error calls are not leaking private data? If you share with me the list of Error call sites during pre login I can also double-check (as a reviewer).
qfrank
force-pushed
the
feat/pre_login_log
branch
3 times, most recently
from
4d36326 to
34fb976
Compare
RPC test fixed |
I tried use regex I have retained the following error log because I think we can keep it, if you have a different opinion, feel free to express it. Next, I will check whether there is any personal information included when constructing the error object in our code. @ilmotta @osmaczko |
qfrank
force-pushed
the
feat/pre_login_log
branch
from
98e4472 to
57ef643
Compare
It's also done with this PR |
This might be overly cautious, as pre-login, by definition, should not be capable of leaking private data since the user database remains encrypted 🤔 |
The definition of pre-login is actually not clear enough. When I implemented it, I did it this way: only after a successful login will the log call |
I see. I am wondering if there is a way to immediately hook into database decryption to modify the logging settings, as post-login would be too late, as you mentioned. I believe this is still less work than obfuscating all logs. It is a never-ending battle—new logs will inevitably appear, sooner or later, breaking the assumption that no private or sensitive data is present in the logs. |
if we do it like this, the error log between successful decryption and fail
True, encrypting logs sounds good, but we can merge this one and PR as first step? @osmaczko |
@osmaczko I would say this is a good problem to have. We should be able to draw the line between what's considered private data in logs when PRs are approved. I still think we should be using the type system to protect certain data from ever being printed, as is supported by Go and zap (although that's a large effort in some cases).
@qfrank Encrypting logs seems like an unnecessary overhead in mobile devices because other apps cannot access the app's data folder (ignoring rooted phones). I'm assuming we can write all log files to the app's protected data directory, but for some reason we write the For desktop apps, the practice of encrypting logs is unusual. Usually, the user controls encryption at rest using other mechanisms, like telling the application to write logs to a special folder, but the application itself is oblivious to this. There are a myriad of tools to control access and protect directories/files. Maybe I'm completely wrong here, in which case, I'd be happy to learn from other desktop apps that do differently. Additionally, encrypting logs will only help with data at rest, but the original motivation for this PR and logging improvements in general is to allow logs to be more safely shared by CCs and users facing dreadful bugs, or at least parts of such logs. |
qfrank
force-pushed
the
feat/pre_login_log
branch
2 times, most recently
from
f4bf73f to
7ae58a4
Compare
Right, I didn't think of it. Still better to print post decryption login errors to
This PR looks fine to me - already approved! 👍 PS. I am not sure about encryption anymore, @ilmotta raised a good points. |
|
Thanks @ilmotta for input!
I agree, and I see two aspects to consider:
While the first category is obvious and should never be logged, even at the trace/debug level, the second is more nuanced. I believe it should only be logged in debug mode. However, this means that logs shared by users who did not have debug enabled will likely be useless.
Sounds reasonable, but following the same logic, why do we use encryption for the database on mobile?
That sounds logical indeed. The question should be: are there many desktop apps that require users to authenticate each time they want to use them? My point is that if the database is encrypted for a reason, and we cannot guarantee that no private data is leaked in logs (which we cannot), then the logs should probably be encrypted as well. |
|
Thanks @osmaczko. Interesting follow-up points.
Fully agree with you. The second category is a more difficult one.
This is a good good question. I actually don't know the original reasons that led to the encryption of the database in mobile. I would imagine it was done to be part of a defense in depth strategy, but I don't know what were the threat models. Mobile devices can be rooted and there's benefit in having data encrypted at rest. Or another reason may be that there are way too many incarnations of Android, and in some of them the OS may be less strict or is outdated (because the manufacturer doesn't provide OS upgrades anymore due to planned obsolescence) and could be more easily exploited by malicious apps. @Samyoul do you have context on this one?
There are layers to this and I don't have a strong opinion. We could encrypt logs and there are upsides in doing that, although seems somewhat unusual.
Status should never ask for full log files from users and we agreed on that with the Legal team. We offer a feature to share the logs, but this should be done only by those who understand what they are doing, usually CCs, and ideally, using test accounts for the single purpose of reproducibility. Further, we could improve the UX to explain to users the implications associated with different log levels or point out to an external documentation.
Yes, they may be useless at the error level. Besides, we are in control, we can better wrap errors in status-go and provide more meaningful logs, use unique error codes, and so on. I think we could further improve crash reports. For instance, in the mobile app, when the user shakes the device some data is captured from the runtime (e.g. a small and safe part of the UI state), apart from the logs and zip them altogether. git provides a bug report command, and many tools have something like that. In some cases, as I've seen in tickets online, the user may want to share parts of logs if the bug is severe and the UI provides no clues. In many occasions in the mobile app, I've noticed that the UI didn't show something meaningful to the user after an error, which forced them to resort to logs. Proper UI feedback is yet another way to help users not need to change log levels and share logs.
Not sure if I got the question right. I'm just one data point, but I think in desktop apps in GNU/Linux, we have such cases. For example, Spotify leaks some private data in |
qfrank
force-pushed
the
feat/pre_login_log
branch
from
7ae58a4 to
2689308
Compare
qfrank
force-pushed
the
feat/pre_login_log
branch
from
2689308 to
1ddaa93
Compare
Change Summary:
SetupLogSettingsto the end of the call chain (e.g.createAccountAndLogin/loginAccount/restoreAccountAndLogin) to ensure that logs before a successful login are written to pre_login.logrelated: status-im/status-mobile#21501