-
Notifications
You must be signed in to change notification settings - Fork 46
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
Fixes #205 installs libreoffice in svs disp #263
Conversation
@emkll Yup, I will submit a proper PR in the |
The error was mentioned at freedomofpress/securedrop-workstation#263 (comment)
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.
I've proceeded with a full review using the svs-disp config package provided by https://github.com/freedomofpress/securedrop-debian-packaging/pull/47/files.
I submitted a .docx file to the source interface of a test SecureDrop instance and attempted to open it in the SecureDrop workstation. I've experienced 2 issues:
- The package ca-certificates-java is not fully installed (likely due to paxctld, see investigation in 1) below)
- MIME handling does not work and does not open LibreOffice, likely because the extension is missing (see investigation in 2) below)
1. In sd-svs-disp, apt reports that a package is not fully installed:
1 not fully installed or removed.
It's an issue with ca-certificates-java:
sudo dpkg --status ca-certificates-java
Package: ca-certificates-java
Status: install ok half-configured
It seems like the paxflag was not applied to java?
Jun 4 15:40:38 localhost kernel: [ 1309.201730] grsec: denied RWX mmap of <anonymous mapping> by /usr/lib/jvm/java-8-openjdk-amd64/jre/bin/java[java:2147] uid/euid:0/0 gid/egid:0/0, parent /var/lib/dpkg/info/ca-certificates-java.postinst[ca-certificates:2133] uid/euid:0/0 gid/egid:0/0
Jun 4 15:40:39 localhost kernel: [ 1310.117476] grsec: denied RWX mmap of <anonymous mapping> by /usr/lib/jvm/java-8-openjdk-amd64/jre/bin/java[java:2945] uid/euid:0/0 gid/egid:0/0, parent /etc/ca-certificates/update.d/jks-keystore[jks-keystore:2926] uid/euid:0/0 gid/egid:0/0
Jun 4 17:12:40 localhost kernel: [ 6830.956819] grsec: denied RWX mmap of <anonymous mapping> by /usr/lib/jvm/java-8-openjdk-amd64/jre/bin/java[java:3114] uid/euid:0/0 gid/egid:0/0, parent /var/lib/dpkg/info/ca-certificates-java.postinst[ca-certificates:3100] uid/euid:0/0 gid/egid:0/0
Jun 4 17:12:41 localhost kernel: [ 6832.077619] grsec: denied RWX mmap of <anonymous mapping> by /usr/lib/jvm/java-8-openjdk-amd64/jre/bin/java[java:3910] uid/euid:0/0 gid/egid:0/0, parent /etc/ca-certificates/update.d/jks-keystore[jks-keystore:3891] uid/euid:0/0 gid/egid:0/0
This might be because paxctld is dead, and the paxflag wasn't applied:
user@sd-svs-disp:~$ sudo service paxctld status
● paxctld.service - PaX flags maintenance daemon
Loaded: loaded (/lib/systemd/system/paxctld.service; disabled; vendor preset:
Active: inactive (dead)
2. Unfortunately a doc submission does not get opened by libreoffice, but rather by the archive manager as shown in #263 (comment) .
This is because xdg-open
relies on the file extension and not magic numbers (file
appears to rely on a file's magic numbers)
user@sd-svs-disp:$ xdg-mime query filetype 1-single-celled_authorship-doc
application/zip
user@sd-svs-disp:$ file --mime-type 1-single-celled_authorship-doc
1-single-celled_authorship-doc: application/vnd.openxmlformats-officedocument.wordprocessingml.document
Changing the extension does change the association:
user@sd-svs-disp:$ mv 1-single-celled_authorship-doc 1-single-celled_authorship-doc.doc
user@sd-svs-disp:$ xdg-mime query filetype 1-single-celled_authorship-doc.doc
application/msword
We could either:
a) Write a script that receives the file from sd-svs, looks at magic numbers and opens it with the correct application, or
b) See if xdg-mime supports using only magic numbers (however preliminary research came back empty so far)
c) Add extensions to submissions on the SecureDrop server code
Happy to hear what other's thoughts on this are, it sound like a) would be the most reliable approach and would allow us to control what files are and aren't supported
can you confirm that after the following steps:
the output of
It still shows as half-configured for me on the latest commit (9e2e5e3). |
I have the following:
|
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.
Thanks @kushaldas for your patience!
I have pushed 934b48c to sidestep/solve the first issue brought up in the previous review (In sd-svs-disp, apt reports that a package is not fully installed: ca-certificates-java) . I've also added a bit ensuring order of operation between paxctld service running and installing LibreOffice, just in case.
I have also opened #266 to track the second item of the previous review (doc submission does not get opened by libreoffice, but rather by the archive manager)
Please review my changes and merge/comment, otherwise LGTM!
Also noticed the sd-svs-disp tests weren't all running, pushed d08eb8d to resolve, and added a test for paxctld running |
Also restarts and enables paxctl service
…ffice Setting the paxctld in the requires block of the libreoffice install will ensure the service is always started prior to installing the package. ca-certificates-java was posing some issues at install time. As it is listed in "recommends" and not "requires", we can sidestep the issue and also reduce total amount of packages installed.
Some sd-svs-disp tests were not running, moving sd-svs-disp tests to their own file resolves.
As far as I can tell, extensions and filenames are supported just fine, the client just doesn't extract them right now (freedomofpress/securedrop-client#163). Test this yourself by decompressing a submission with Given this, will this issue be implicitly resolved once we resolve freedomofpress/securedrop-client#163 ? |
Used to determine whether paxctld is running inside the VMs. The most important is sd-svs-disp, since that's where we'll be installing libreoffice, and libreoffice requires pax flags to be set on java dependencies in order for the package to install successfully. Removes the previous implementation of a service check in favor of a more generalized approach. Overall, the config test suite should probably be reorganized.
Using Salt to force the service to run, so it's active immediately. More important for the child VMs is using the qvm-service calls to enable the service on boot of other, non-Template but Template-based VMs. Since we're now properly enforcing the paxctld service state in all VMs, removes the previous implementation specific to sd-svs-disp.
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.
Solid improvements here. Deferring discussion on the mimetype behavior to #266. With a few more test tweaks, this is ready to merge. As it stands, I can confirm that libreoffice is installed without issue. Will follow up with another test or two, then pass to @kushaldas for final sign-off before merge.
dom0/sd-svs-disp-files.sls
Outdated
paxctld: | ||
service.running: | ||
- enable: True | ||
- reload: True |
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.
The task should have a more explicit name, as all tasks exist in the same namespace once the various states have been merged. Compare with the task below: sd-svs-disp-install-libreoffice
, rather than libreoffice
.
More relevant for predictable service state is that we need to inform Qubes (via dom0) that a given VM should start a service. See description in #231, specifically the mention of qvm-service
. Following that approach will not only resolve concerns about the libreoffice install in this PR, but also allow us to resolve #231, since we'll be managed the service on all the requisite VMs, not just one.
|
||
def test_sd_client_package_installed(self): | ||
pkg = "securedrop-workstation-svs-disp" | ||
self.assertTrue(self._package_is_installed(pkg)) |
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.
Great catch on re-enabling these tests! Some of the test names still refer to sd-svs
(specifically test_sd_client_package_installed
), will clean up for clarity.
dom0/sd-svs-disp-files.sls
Outdated
interval: 60 | ||
- install_recommends: False | ||
- require: | ||
- service: paxctld |
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.
Given requested changes to task name, this requirement name should be updated. Better yet, if we ensure paxctld is running in a parent state, then we needn't redeclare it here.
tests/test_svs_disp.py
Outdated
cmd = ["qvm-run", "-p", "sd-svs-disp", | ||
"/usr/sbin/service paxctld status"] | ||
p = subprocess.check_output(cmd) | ||
self.assertTrue("active (running)".encode() in p) |
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.
👍 on checking the service in the config states. Although we've only been bitten by PaX flags on sd-svs-disp
so far, we in fact what the paxctld service running in other VMs, as well (see #231). Will adjust the test to run against other machines, as well.
- retry: | ||
attempts: 3 | ||
interval: 60 | ||
- install_recommends: False |
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.
Given that the install_recommends: False
sidesteps the PaX flags problem, do we still need the retry logic? Doesn't hurt to keep it in, but it seems like we resolve the problem without resorting to retries.
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.
Able to run libreoffice just fine now. With the new paxctld service logic, merge of this PR will close #231, as well as close #205, as original intended.
@kushaldas Please have a look, since @emkll and I both pushed changes here. Fine to merge if you agree!
Might take time due to high amount of download for libreoffice.
How to test?
qvm-remove sd-svs-disp
qvm-remove sd-svs-disp-template
make sd-svs-disp