Skip to content
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

Update mimetype handling and --view-only for open-in-dvm #501

Merged
merged 3 commits into from
Mar 23, 2020

Conversation

emkll
Copy link
Contributor

@emkll emkll commented Mar 20, 2020

Status

Ready for review

❗ Temporary commit will need to be removed prior to merge ❗
This is due to the use of https://github.com/freedomofpress/securedrop-workstation/wiki/Evaluating-new-deb-package-behavior for reviewer convenience

The reviewer of this PR should also review and merge the following PRs before merging this one:

Description of Changes

Fixes freedomofpress/securedrop-client#960
Fixes freedomofpress/securedrop-client#961

Testing

In a dev VM:

  • Visually review diffs in the 3 PRs above
  • Build securedrop-client, securedrop-export and securedrop-workstation-svs-disp on the branches onf the PRs in the section above
  • copy built debs in sd-workstation folder

In dom0:

  • make clone and move config.json and sd-journalist.sec in place, if necessary
  • make all completes without issue
  • make test all tests pass (except update tests for sd-app, due to 0.1.3 < 0.1.3 nightly series)

In Tor browser

  • submit a desktop file to source interface (debian-xterm.desktop works well since it's installed in most places)

In sd-app

  • Client opens desktop file in dvm (and in gedit)
  • xdg-open that file on disk manually (from /home/user/.securedrop-client/data/...) opens in a dvm
  • Modify rules to permit Qubes.Filecopy from sd-app to sd-devices, and copy desktop file to sd-devices
  • xdg-open that file on disk manually (from ~/QubesIncoming/sd-app/...) opens in a dvm (I recommend you use a plaintext file for convenience for next step)
  • In the DispVM, the file is opened in read-only mode
  • manually editing the file in /tmp/sd-app... and then closing the application does not modify the file in sd-app

Checklist

If you have made code changes

If you have made changes to the provisioning logic

  • All tests (make test) pass in dom0 of a Qubes install

Copy link
Contributor

@conorsch conorsch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After building custom debs as described, then rebuilding locally, the changes here work well. I ran into a snag with the "all tests pass," related to v3 onions and --verify behavior. Pushed up a fix to run the check as the debian-tor user; if you've got v2 services enabled, would appreciate confirming it works well there too.

While I was able to confirm that gedit was used after submitting a .desktop file, and I wasn't able to save changes in the DVM (one of the core resolutions this PR aims for), testing with xdg-open inside sd-app did allow me to clobber the originally submitted file. Therefore we need updates to the corresponding package PRs in order to update the open-in-dvm behavior. For the context of the Client, we always want the open-in-dvm behavior to be read-only, so updating the corresponding desktop file will ensure that even xdg-open makes it read-only. Such a change is still in the spirit of defense-in-depth, given that the client explicitly calls qvm-open-in-dvm itself.

emkll and others added 3 commits March 23, 2020 09:07
Had only tested the `tor --verify-config` command with v2 onions. Under
v3, the test fails due to permissions. Running the same command as
"debian-tor", rather than as "user", resolves.
@emkll emkll force-pushed the 960-961-view-only-and-desktop branch from 1d3a93b to e3b0b5c Compare March 23, 2020 14:31
@emkll
Copy link
Contributor Author

emkll commented Mar 23, 2020

Thanks for the review and fixing the tor test case @conorsch . Your comments should now be addressed in respective repositories, also added tests here for the open-in-dvm desktop file. I have also updated the test plan accordingly.

@conorsch
Copy link
Contributor

Rebuilt local packages, ran through test plan, everything working beautifully. Test plan report:


In dom0:

  • make clone and move config.json and sd-journalist.sec in place, if necessary
  • make all completes without issue
  • make test all tests pass (except update tests for sd-app, due to 0.1.3 < 0.1.3 nightly series)

In Tor browser

  • submit a desktop file to source interface (debian-xterm.desktop works well since it's installed in most places)

In sd-app

  • Client opens desktop file in dvm (and in gedit)
  • xdg-open that file on disk manually (from /home/user/.securedrop-client/data/...) opens in a dvm
  • Modify rules to permit Qubes.Filecopy from sd-app to sd-devices, and copy desktop file to sd-devices
  • xdg-open that file on disk manually (from ~/QubesIncoming/sd-app/...) opens in a dvm (I recommend you use a plaintext file for convenience for next step)
    • FWIW, I used the same .desktop file here, since it was opening gedit, and easy to test read-only mode
  • In the DispVM, the file is opened in read-only mode
  • manually editing the file in /tmp/sd-app... and then closing the application does not modify the file in sd-app
    • technically, i used gedit to modify the text, then tried to save, and observed failure. on the previous round of testing, before the --view-only flag was added to the open-in-dvm desktop file, the save operation was successful, and changes were propagated back to the calling VM. that's no longer the case, which is good!

Not formally approving just yet, since we still need to snip out the TEMPORARY commit (will do so shortly), as well as approve the other related PRs. All PRs should be merged together in order to avoid problems.

@conorsch conorsch force-pushed the 960-961-view-only-and-desktop branch from e3b0b5c to 4b87327 Compare March 23, 2020 19:45
@conorsch
Copy link
Contributor

Rebased on top of latest master (505a616), and removed temporary commit as requested.

@conorsch conorsch self-requested a review March 23, 2020 19:47
Copy link
Contributor

@conorsch conorsch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test plan is passing. All related PRs have been reviewed and merged.

@conorsch conorsch merged commit e4e489c into master Mar 23, 2020
@legoktm legoktm deleted the 960-961-view-only-and-desktop branch May 28, 2024 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide explicit handing of application/x-desktop in mimeapps.list Use --view-only when invoking open-in-vm
2 participants