Restore the `isolation_provider.base.kill_process_group()` function,
which was previously mocked, at the end of the
`test_linger_unkillable()` test. This function is initially mocked, in
order to simulate a hang process. After the mocking completes, the test
needs the original function once more, in order to actually kill the
spawned process.
Add a doc that contains an MP4 video in it, which has an audio and video
stream. This type of document could not be converted with the latest
Dangerzone releases, because PyMuPDF threw this error in the container's
stdout:
MuPDF error: unsupported error: cannot create appearance stream for
Screen annotations
This error message was treated literally by our client code, which
parsed the first few bytes in order to find out the page height/width.
This resulted to a misleading Dangerzone error, e.g.:
A page exceeded the maximum height
This issue started occurring since 0.6.0, which added streaming support,
and was fixed by commit 3f86e7b465. That
fix was not accompanied by a test document that would ensure we would
not have this regression from now on, so we add it in this
commit.
Refs #877Closes#917
Make the Dummy isolation provider follow the rest of the isolation
providers and perform the second part of the conversion on the host. The
first part of the conversion is just a dummy script that reads a file
from stdin and prints pixels to stdout.
Extend the base isolation provider to immediately convert each page to
a PDF, and optionally use OCR. In contract with the way we did things
previously, there are no more two separate stages (document to pixels,
pixels to PDF). We now handle each page individually, for two main
reasons:
1. We don't want to buffer pixel data, either on disk or in memory,
since they take a lot of space, and can potentially leave traces.
2. We can perform these operations in parallel, saving time. This is
more evident when OCR is not used, where the time to convert a page
to pixels, and then back to a PDF are comparable.
Add a new way to detect where the Tesseract data are stored in a user's
system. On Linux, the Tesseract data should be installed via the package
manager. On macOS and Windows, they should be bundled with the
Dangerzone application.
There is also the exception of running Dangerzone locally, where even
on Linux, we should get the Tesseract data from the Dangerzone share/
folder.
Do not use the `provider_wait` fixture in our termination logic tests,
and switch instead to the `provider` fixture, which instantiates a
typical isolation provider.
The `provider_wait` fixture's goal was to emulate how would the process
behave if it had fully spawned. In practice, this masked some
termination logic issues that became apparent in the WIP on-host
conversion PR. Now that we kill the spawned process via its process
group, we can just use the default isolation provider in our tests.
In practice, in this PR we just do `s/provider_wait/provider`, and
remove some stale code.
Add a fixture that returns our stock Dummy provider. Also, explicitly
use a blocking Dummy provider (`DummyWait`) for a specific test case.
This will prove useful when we stop using the `provider_wait` variant of
our isolation providers in the next commits.
Instead of killing just the invoked Podman/Docker/qrexec process, kill
the whole process group, to make sure that other components that have
been spawned die as well. In the case of Podman, conmon is one of the
processes that lingers, so that's one way to kill it.
Start the conversion process in a new session, so that we can later on
kill the process group, without killing the controlling script (i.e.,
the Dangezone UI). This should not affect the conversion process in any
other way.
As per Etienne Perot's comment on #908:
> Then it seems to me like it would be easy to simply apply this seccomp
profile under all container runtimes (since there's no reason why the
same image and the same command-line would call different syscalls under
different container runtimes).
Some of the files in our large test set can make LibreOffice hang. We
do not have a proper solution for this yet, but we can at least make
the tests timeout quickly, so that they can finish at some point.
Refs #878
We have encountered several conversions where the `docker kill` command
hangs. Handle this case by specifying a timeout to this command. If the
timeout expires, log a warning and proceed with the rest of the
termination logic (i.e., kill the conversion process).
Fixes#854
We have a container-specific test that deals with missing OCR files in
the container image. This test _can_ be run under Qubes, and it may
fail since it requires Podman.
Make the pytest guard more strict and don't allow running this test on
Qubes.
Also, fix a typo in the word "omission".
The platform where we run our tests directly affects the isolation
providers we can choose. For instance, we cannot run Qubes tests on a
Windows/macOS platform, nor can we spawn containers in a Qubes platform,
if the `QUBES_CONVERSION` envvar has been specified.
This platform incompatibility was never an issue before, because
Dangerzone is capable of selecting the proper isolation provider under
the hood. However, with the addition of tests that target specific
isolation providers, it's possible that we may run by mistake a test
that does not apply to our platform.
To counter this, we employed `pytest.skipif()` guards around classes,
but we may omit those by mistake. Case in point, the `TestContainer`
class does not have such a guard, which means that we attempt to run
this test case on Qubes and it fails.
Add module-level guards in our isolation provider tests using pytest's
`pytest.skip("...", allow_module_level=True)` function, so that we make
such restrictions more explicit, and less easy to forget when we add a
new class.
This wraps the existing container image inside a gVisor-based sandbox.
gVisor is an open-source OCI-compliant container runtime.
It is a userspace reimplementation of the Linux kernel in a
memory-safe language.
It works by creating a sandboxed environment in which regular Linux
applications run, but their system calls are intercepted by gVisor.
gVisor then redirects these system calls and reinterprets them in
its own kernel. This means the host Linux kernel is isolated
from the sandboxed application, thereby providing protection against
Linux container escape attacks.
It also uses `seccomp-bpf` to provide a secondary layer of defense
against container escapes. Even if its userspace kernel gets
compromised, attackers would have to additionally have a Linux
container escape vector, and that exploit would have to fit within
the restricted `seccomp-bpf` rules that gVisor adds on itself.
Fixes#126Fixes#224Fixes#225Fixes#228
I'm actually ensure how the previous version was working, but since we
are now loading the pytest fixtures automatically, it uncovered a misuse
in the tests.
The `updater` fixture sets `updater.dangerzone.app` to a magic mock
instance, whereas `qt_updater` returns the real QT app, which is what we
want in our tests.
They ideally should find their way by themselves.
> You don’t need to import the fixture you want to use in a test,
> it automatically gets discovered by pytest. The discovery of fixture
> functions starts at test classes, then test modules, then conftest.py
> files and finally builtin and third party plugins.>
>
> — [pytest docs](https://docs.pytest.org/en/4.6.x/fixture.html#conftest-py-sharing-fixture-functions)
A few minor changes about when to use `==` and when to use `is`.
Basically, this uses `is` for booleans, and `==` for other values.
With a few other changes about coding style which was enforced by
`ruff`.
This commit removes code that's not being used, it can be exceptions
with the `as e` where the exception itself is not used, the same with
`with` statements, and some other parts where there were duplicated
code.
> f-strings are a convenient way to format strings, but they are not
> necessary if there are no placeholder expressions to format. In this
> case, a regular string should be used instead, as an f-string without
> placeholders can be confusing for readers, who may expect such a
> placeholder to be present.
>
> — [ruff docs](https://docs.astral.sh/ruff/rules/f-string-missing-placeholders/)
In d632908a44 we improved our
`replace_control_chars()` function, by replacing every control or
invalid Unicode character with a placeholder one. This change, however,
made our debug logs harder to read, since newlines were not preserved.
There are indeed various cases in which replacing newlines is wise
(e.g., in filenames), so we should keep this behavior by default.
However, specifically for reading debug logs, we add an option to keep
newlines to improve readability, at no expense to security.
The `exit()` [1] function is not necessarily present in every Python
environment, as it's added by the `site` module. Also, this function is
"[...] useful for the interactive interpreter shell and should not be
used in programs"
For this reason, we replace all such occurrences with `sys.exit()` [2],
which is the canonical function to exit Python programs.
[1]: https://docs.python.org/3/library/constants.html#exit
[2]: https://docs.python.org/3/library/sys.html#sys.exit
On Windows, if we somehow attempt to archive the same document twice
(e.g, because it got archived once, and then we copy it back), we will
get an error, because Windows does not overwrite the target path, if it
already exists.
Fix this issue by always removing the previously archived version, when
performing the next archival action, and update our tests.
Quote from `pytest-mock` docs [1]:
> The purpose of this plugin is to make the use of context managers and
> function decorators for mocking unnecessary, so it will emit a warning
> when used as such.
Thus using it as a context manager currently produces a warning during
test runs in CI which is extra noise that could make new (possibly more
important) warnings harder to spot.
[1]: https://pytest-mock.readthedocs.io/en/latest/usage.html#usage-as-context-manager
On Unix systems a filename can be a sequence of bytes that is not valid
UTF-8. Python uses[1] surrogate escapes to allow to decode such
filenames to Unicode (bytes that cannot be decoded are replaced by a
surrogate; upon encoding the surrogate is converted to the original
byte).
From `click` docs[2]:
> Invalid bytes or surrogate escapes will raise an error when written
> to a stream with `errors="strict"`. This will typically happen with
> `stdout` when the locale is something like `en_GB.UTF-8`.
To fix that, we use `utils.replace_control_chars()` before printing the
filenames to `stdout` so that surrogate escapes are replaced by �.
Fixes#768
The `util.replace_control_chars()` function was overly strict, and
would replace every non-ASCII character with "_". This included both
control characters, as well as normal characters in a non-English
alphabet.
Relax these restrictions by checking each character and deciding if it's
a Unicode control character, using the `unicodedata` Python package.
With this change, emojis and non-English letters are now allowed.
Add a pytest fixture that crafts a filename with Unicode characters that
are not considered common for this use. By default, this fixture uses
an invalid Unicode character as well, but we strip it in case of macOS
(APFS) since filenames must be UTF-8 encoded.
[1]: https://en.wikipedia.org/wiki/Filename#Comparison_of_filename_limitations
Add termination tests for the Dummy provider, so that we can have
cross-platform coverage in our Windows/macOS CI runners, which can't use
the Container isolation providers.
Add termination-related tests for Qubes. To achieve this, we need
to make a change to the Qubes isolation provider. More specifically,
we need to make the isolation provider yield control to the caller only
when the disposable qube is up and running.
Qubes does not provide us a solid guarantee to do so, but we've found a
hacky workaround, whereby we wait until the `qrexec-client-vm` process
opens a `/dev/xen` character device. This should happen, in theory, once
the disposable qube is ready, and has sent a `MSG_SERVICE_CONNECT` RPC
message to the caller.
Add termination-related tests for containers. To achieve this, we need
to make a change to the container isolation provider. More specifically,
we need to make the isolation provider yield control to the caller only
when the container is up and running. Failure to do so may lead to
lingering processes.
Add some test cases in the isolation provider tests, that check how it
behaves when a process completes successfully, lingers, or cannot
terminate.
These tests cannot run yet, since they must be imported by a concrete
isolation provider test class. In subsequent commits, we will start
enabling them.
Pass the Document instance that will be converted to the
`IsolationProvider.start_doc_to_pixels_proc()` method. Concrete classes
can then associate this name with the started process, so that they can
later on kill it.
Previously settings was implicitly tested on tests/gui/test_updater.py.
However this was concerned with updater-related tests only, which
incidentally covered almost all of settings.py. However, a few tests
were missing. This commit increases the test coverage, but also tests
additional test conditions.
The goal is to help us increase the test coverage of the previous
scenario, which tested for the persistence of user data (settings). This
way we can drop the requirement to test this on linux hosts, which is
slightly harder (more cumbersome) to do.
When we get an early EOF from the converter process, we should
immediately get the exit code of that process, to find out the actual
underlying error. Currently, the exception we raise masks the underlying
error.
Raise a ConverterProcException, that in turns makes our error handling
code read the exit code of the spawned process, and converts it to a
helpful error message.
Fixes#714
Remove timeouts due to several reasons:
1. Lost purpose: after implementing the containers page streaming the
only subprocess we have left is LibreOffice. So don't have such a
big risk of commands hanging (the original reason for timeouts).
2. Little benefit: predicting execution time is generically unsolvable
computer science problem. Ultimately we were guessing an arbitrary
time based on the number of pages and the document size. As a guess
we made it pretty lax (30s per page or MB). A document hanging for
this long will probably lead to user frustration in any case and the
user may be compelled to abort the conversion.
3. Technical Challenges with non-blocking timeout: there have been
several technical challenges in keeping timeouts that we've made effort
to accommodate. A significant one was having to do non-blocking read to
ensure we could timeout when reading conversion stream (and then used
here)
Fixes#687
