Show output during preflight updater

kennethrrosen commented 2 months ago

[x] I have searched for duplicates or related issues

Description

Similar to the Qubes OS Update GUI, show output of preflight updater.

How will this impact SecureDrop/SecureDrop Workstation users?

Providing better diagnostics/time-to-completion/debug and error messaging for users, rather than "ask administrator" for assistance.

How would this affect the SecureDrop Workstation threat model?

I do not believe so.

User Stories

Going in search of which log may contain the error of an unsuccessful preflight update can be tedious if experienced. As a journalist/SD operator, I want to be able to diagnose and correct or report any issues quickly, so that I may fix on my own or alert devs to an issue with adequate logging and information. I'd also be grateful to know roughly how much time the update may take so to apportion time better.

kennethrrosen commented 1 month ago

I've worked on this a little bit and think I've found a solution. The proposed solution includes an optional output box for displaying logs and improves the log messaging format while maintaining the original logging mechanism and log files. This approach avoids using eval and ensures security by using safer alternatives like isinstance for type checking.

I have made these changes to minimize the impact on the existing codebase and avoid meddling with the core functionality. Feedback/comments appreciated. Ideally the output from the salt states would also be included, but I have not gotten that far and believe that would require a bit more effort.

https://github.com/kennethrrosen/securedrop-workstation/commit/d0e7ab69c8b5fb08e32890ee06adc6635a8b0c9e

rocodes commented 1 month ago

Hey @kennethrrosen, thank you for looking at this and looking for ways to improve the updater story. We have some work in progress to move towards the Qubes native updater, which @deeplow is primarily working on, so I will defer to him in terms of how this fits in there.

Contributions are always welcome, and we wouldn't want you to put time towards something that was already being worked on elsewhere (or in the case of the updater, towards something we may end up deprecating)-- if you are looking to contribute, feel free to join us on Gitter (pretty quiet these days but available, edit: gitter link), or throw a comment in any ticket that interests you, and we can be a bit more legible about whether work is already in progress or not. (Mostly, stuff that's being worked on is tracked here: https://github.com/orgs/freedomofpress/projects/17/views/2 - but we also have a bit of milestone-based roadmapping in some of our repositories, particularly the client, which gives you an idea of what we want to get to next beyond the immediate picture on the board.)

kennethrrosen commented 1 month ago

@rocodes very happy to see that @deeplow is working to integrate with the native updater. I think that's a great idea. This would not fit in with that, other than perhaps a small patch in the interim.

deeplow commented 1 month ago

Thanks for your interest! It looks like something that we could make use of in the interim (like you suggested), although the goal will be to replace it eventually with the GUI updater as @rocodes mentioned.

I took a spin through the code and generally, to set a logging handler feels like a good approach, since we do in fact already use logging to output progress information. However, there are two special needs we have for this. They are:

avoiding parsing - from what I saw in the implementation, you add quite a bit of parsing. Since we're dealing with untrusted input, it's best if we don't try processing with it, other than (next point)
Disabling any text rendering properties. In this case, it would be using QTextEdit.insertPlainText instead of QTextEdit.append, for example. Without this, a malicious template could send HTML-formatted text and exploit the HTML parser.

But I think overall you're on a good direction. There are also minor things I'd suggest later (pharsing, etc.), but these are minor and can be addressed later.

Also, have you played around with trying to get the updater-detail logs? I think those would be pretty useful since I guess the main issue with the updater is to know what failed.

kennethrrosen commented 1 month ago

@deeplow glad to hear this us something worth pursuing.

Agreed on both points/needs, both of which I've fixed. Once this was working (there are some small things I still want to tweak, like the return of the updater to its original size if a user unchecks the checkbox after checking it) I had tried to get the more detailed output from Updater.py but struggled.

I'll play around with it some more and will update when I have that working, as the complete output, I agree, is what would be most useful.

deeplow commented 1 month ago

I had tried to get the more detailed output from Updater.py but struggled.

I was trying to find the source code for the 4.1 Qubes salt updater. It had something like a collapsible details, which we could borrow from. That patter could work here to help preserve the original dimensions.

kennethrrosen commented 1 month ago

4.1 Qubes salt updater.

@deeplow I've had no luck. Is there a 4.1 iso floating around? I could start there.

deeplow commented 1 month ago

In that case, it may be easier to ask in the qubes-public matrix channel for where the source code can be found or on qubes-devel. But in any case, you can find all the downloads historical downloads at https://mirrors.edge.kernel.org/qubes/iso/.

freedomofpress / securedrop-workstation