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.

Sign up for GitHub

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

Jump to bottom

Move MIME handling to systemd services (boot provisioning) #2033

Merged
merged 1 commit into from
Jun 3, 2024
Merged

Move MIME handling to systemd services (boot provisioning) #2033

merged 1 commit into from
Jun 3, 2024

Conversation

deeplow
Copy link
Contributor

@deeplow deeplow commented May 24, 2024 •
edited by legoktm
Loading

Status

Ready to review.

Description

Fixes freedomofpress/securedrop-workstation#1042.
Fixes freedomofpress/securedrop-workstation#615.

Test Plan

  • CI passes
  • Setup
  • Confirm that sd-proxy is disposable
  • Open terminal in sd-proxy and:
    • confirm symlink: /home/user/.local/share/applications/mimeapps.list -> /opt/securedrop/mimeapps.list.default
    • confirm symlink: /home/user/.mailcap -> /opt/securedrop/.mailcap.default
  • Start sd-proxy-dvm and confirm the following files (or parent directory) do NOT exist:
    • /home/user/.local/share/applications/mimeapps.list
    • /home/user/.mailcap
  • Start a disposable based on sd-viewer and:
    • confirm symlink: /home/user/.local/share/applications/mimeapps.list -> /opt/securedrop/mimeapps.list.sd-viewer
    • confirm symlink: /home/user/.mailcap -> /opt/securedrop/.mailcap.default
  • Start sd-devices-dvm and confirm the following files (or parent directory) do NOT exist:
    • /home/user/.local/share/applications/mimeapps.list
    • /home/user/.local/.mailcap
  • Start sd-devices and:
    • confirm symlink: /home/user/.local/share/applications/mimeapps.list -> /opt/securedrop/mimeapps.list.sd-viewer
      • actually points to the sd-devices variant, which I believe was intended behavior
    • confirm symlink: /home/user/.mailcap -> /opt/securedrop/.mailcap.default
  • test in staging environment due to changes in "opening files in VMs".

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:

  • I have tested these changes in the appropriate Qubes environment
  • I do not have an appropriate Qubes OS workstation set up (the reviewer will need to test these changes)
  • These changes should not need testing in Qubes

If these changes add or remove files other than client code, the AppArmor profile may need to be updated. Please check as applicable:

  • I have updated the AppArmor profile
  • No update to the AppArmor profile is required for these changes
  • I don't know and would appreciate guidance

If these changes modify the database schema, you should include a database migration. Please check as applicable:

  • I have written a migration and upgraded a test database based on main and confirmed that the migration is self-contained and applies cleanly
  • I have written a migration but have not upgraded a test database based on main and would like the reviewer to do so
  • I need help writing a database migration
  • No database schema changes are needed

@deeplow deeplow requested a review from a team as a code owner May 24, 2024 17:05
@deeplow deeplow mentioned this pull request May 24, 2024
Merged
4 tasks
@deeplow deeplow requested a review from cfm May 24, 2024 17:17
legoktm
legoktm reviewed May 24, 2024
View reviewed changes
Copy link
Member

@legoktm legoktm left a comment

Choose a reason for hiding this comment

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

Yay! I really like where this is headed

workstation-config/securedrop-mime-handling Outdated Show resolved Hide resolved
workstation-config/securedrop-mime-handling Outdated Show resolved Hide resolved
debian/securedrop-workstation-config.install Outdated Show resolved Hide resolved
workstation-config/securedrop-mime-handling Outdated Show resolved Hide resolved
@deeplow
Copy link
Contributor Author

deeplow commented May 28, 2024

Added a caveat that may need some consideration: https://github.com/freedomofpress/securedrop-client/pull/2033/files#r1616905179

@legoktm
Copy link
Member

legoktm commented May 30, 2024

The test plan currently says:

  • confirm symlink: /home/user/.local/.mailcap -> /opt/securedrop/.mailcap.default

It's supposed to be /home/user/.mailcap - code was correct, test plan was wrong. Updating it now.

legoktm
legoktm requested changes May 30, 2024
View reviewed changes
Copy link
Member

@legoktm legoktm left a comment

Choose a reason for hiding this comment

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

Overall test plan checks out \o/ Pointed out a few small things inline and also ruff needs to be run to make CI happy

workstation-config/securedrop-mime-handling.py Outdated Show resolved Hide resolved
workstation-config/securedrop-mime-handling.py Outdated Show resolved Hide resolved
@deeplow
Copy link
Contributor Author

deeplow commented May 31, 2024 •
edited
Loading

Is it worth squashing some of the commits for a cleaner history? I went back and forth on services names, etc. So the history is a bit messy. I'm happy to reorganize the commit history to it makes more sense. Anyways, I'm fine with following standard practice. Just flagging because this PR had lots of changes like that.

@deeplow deeplow force-pushed the mime-systemd-provisioning branch from ecabf7c to 76daa81 Compare June 3, 2024 16:37
@deeplow
Copy link
Contributor Author

deeplow commented Jun 3, 2024 •
edited
Loading

Rebased to resolve the merge issue. I played a bit around with the git history to make it a bit more linear and without so much back and forth. Unfortunately it's rather convoluted and the most pragmatic option is to just all commits. However, we may loose some context on decisions. @legoktm what's the general development practice on securedrop regarding changing the git history in situations like this?

@zenmonkeykstop
Copy link
Contributor

@deeplow feel free to squash it down to a single commit before merge if you like - it's not a massive change and is well-contained.

@legoktm
Copy link
Member

legoktm commented Jun 3, 2024 •
edited
Loading

Unfortunately it's rather convoluted and the most pragmatic option is to just all commits. However, we may loose some context on decisions. @legoktm what's the general development practice on securedrop regarding changing the git history in situations like this?

Merging it like this is historically what the SD team has done, and that's fine. Personally I like to spend no more than 20 minutes rebasing it into a few logical commits (and sometimes that ends up being just one commit!). Any decisions that you think are important to document should be explained in the commit message or leave a comment in the file itself.

@deeplow
Move MIME handling to systemd services (boot provisioning)
21a1ec3 
Fixes freedomofpress/securedrop-workstation#1042, complemented
by freedomofpress/securedrop-workstation#1043.

Implemention reasoning:
- Failure to setup leads shutdown to make this security-critical
  component loud on failures
- Running in disp. templates fails (e.g. sd-viewer) to prevent
  populating user's home directory, thus contaminating all
  disposables based on them.
- MIME-handling behavior set via qubesdb - pass vm-specific data
  via the vm-config qubes feature (accessible through QubesDB) [1].
- Started after rsyslog.service to allow failure logging

[1]: https://dev.qubes-os.org/projects/core-admin-client/en/latest/manpages/qvm-features.html#vm-config
@deeplow deeplow force-pushed the mime-systemd-provisioning branch from 76daa81 to 21a1ec3 Compare June 3, 2024 16:55
@deeplow
Copy link
Contributor Author

deeplow commented Jun 3, 2024

Thanks for the input. I have decided to squash the commits.

@legoktm
Copy link
Member

legoktm commented Jun 3, 2024

Thanks, I think we're all set here. 👀 over to the workstation PR now :)

legoktm
legoktm reviewed Jun 3, 2024
View reviewed changes
workstation-config/securedrop-mime-handling.py
# sd-app as the only valid non-disposable.
persistent_home = QubesDB().read("/qubes-vm-persistence").decode() != "none"
vm_name = QubesDB().read("/name").decode()
if persistent_home and vm_name != "sd-app":
Copy link
Member

Choose a reason for hiding this comment

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

This triggers an interesting side effect - it's now impossible to start the sd-viewer VM because this unit will exit 1 and trigger systemd-halt.

legoktm
legoktm approved these changes Jun 3, 2024
View reviewed changes
Copy link
Member

@legoktm legoktm left a comment

Choose a reason for hiding this comment

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

Let's do this!

@legoktm legoktm merged commit 3663649 into main Jun 3, 2024
58 checks passed
@legoktm legoktm deleted the mime-systemd-provisioning branch June 3, 2024 21:10
@cfm
Copy link
Member

cfm commented Jun 4, 2024 via email

@deeplow
Copy link
Contributor Author

deeplow commented Jun 4, 2024 •
edited
Loading

Should there be a wrapper that makes them actual env vars and then only access those from within our programs? As a side-effect, tests could work outside of Qubes systems.

@zenmonkeykstop
Copy link
Contributor

Should there be a wrapper that makes them actual env vars and then only access those from within our programs? As a side-effect, tests could work outside of Qubes systems.

See proposal and decision in freedomofpress/securedrop-workstation#1013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rustybird rustybird rustybird left review comments

@legoktm legoktm legoktm approved these changes

@cfm cfm Awaiting requested review from cfm

Assignees

@deeplow deeplow

Labels
sprint goal
Projects
SecureDrop dev cycle
Archived in project
Milestone
No milestone
5 participants
@deeplow @legoktm @zenmonkeykstop @cfm @rustybird