-
Notifications
You must be signed in to change notification settings - Fork 42
Adds SysLogHandler to log everything on rsyslog #692
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
Conversation
a6ff478
to
9748eb2
Compare
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.
can we put the black change in another PR? (since it will need similar changes as https://github.com/freedomofpress/securedrop-proxy/issues/57)
Yes, I will remove my autoformat tool from |
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.
Can we also log to a separate client.log
file like we currently do? This makes it difficult to develop or repro an issue and provide logs if there are bunch of kernel and other logs mixed in with client logs.
As far as the black formatter changes, I think we should agree on what the standard should be for the client that black will enforce before committing changes. Perhaps we could have a separate PR that introduces black formatting changes across all files?
With rsyslog it's possible to route log messages to an application-specific destination. We could put
If we further change
then our logging would only go to that dedicated file. This could be done locally, in the centralized logging VM, or both. If having the file under /var/log/ is sufficient, no more changes need to be made to the Personally, I think I'd find a single log file in the central VM containing all workstation-related logging -- from client, proxy, everything -- the most useful. |
Found the discussion on |
9748eb2
to
aad2642
Compare
I think that was the idea to have rsyslog enabled. This way we can monitor the status of the VM and also our application in the same place. To see only the client related logs, one can do |
aad2642
to
75905e1
Compare
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.
if we no longer allow logs to be written to {sdc-home}/logs
then docs will need to be updated with this change here to explain where to look for logs and how to find specific logs, e.g. tail -f /var/log/syslog | grep client
(we might want to change this to tail -f /var/log/syslog | grep securedrop-client
to make it less likely to capture extra logs we don't want and hopefully other applications don't mention "securedrop-client" anywhere in their logs but if they do i guess we can mentally sort it out later... :/)
personally i think it would be very nice to be able to continue to use sdc-home
for capturing application-level logs in the specified directory as well as using SysLogHandler so that we can collect all the logs in the central vm, but since you have a solution for getting client logs (tail -f /var/log/syslog | grep client
) i don't think this needs to hold up your PR
so the request for changes is just for the docs and maybe adding back logging to {sdc-home}/logs
if you can do it in an straight-forward simple way
securedrop_client/app.py
Outdated
backupCount=5, delay=False, | ||
encoding=ENCODING) | ||
# define log handlers using system's rsyslog | ||
handler = SysLogHandler(address="/dev/log") |
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.
we'll also want to add a check for the platform here as we do in prevent_second_instance
, since this won't work on non-Linux platforms (recall that right now this project can be developed by contributors on other platforms, this is something we want to preserve)
c735859
to
ba99840
Compare
The existing logging system stays the same, this just adds extra logging ot rsyslog too.
ba99840
to
592f5da
Compare
I moved this from "Ready to Review" to "In Development" because there are requested changes. |
It looks like now you are no longer logging to rsyslog and switched back to |
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.
Ah, when I switched to my Qubes workstation and ran the client, I could see that this does log to /var/log/syslog AND {sdc-home}/logs so lgtm!
Now all log lines will go to
/var/log/syslog
file.Also
black
reformatted the file.Test Plan
You should be able to see the log entries in
/var/log/syslog
file.Checklist
If these changes modify code paths involving cryptography, the opening of files in VMs or network (via the RPC service) traffic, Qubes testing in the staging environment is required. For fine tuning of the graphical user interface, testing in any environment in Qubes is required. Please check as applicable: