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

Perform on-host conversion for the pixels to PDF stage #748

Merged
merged 19 commits into from
Oct 17, 2024
Merged

Conversation

apyrgio
Copy link
Contributor

@apyrgio apyrgio commented Mar 14, 2024

This PR introduces a fundamental change in the way Dangerzone processes documents. Instead of first grabbing all of the pixel data from the first container, storing them on disk, and then reconstructing the PDF on a second container, Dangerzone now immediately reconstructs the PDF on the host, while the doc to pixels conversion is still running on the first container. The sanitzation is no less safe, since the boundaries between the sandbox and the host are still respected.

What we gain is that we no longer use mounts, and we have much faster conversions, especially on Windows and macOS.

Fixes #625

Note

This PR still has some rough edges. Off the top of my head, we need to:

  • Test the changes across all of our supported platforms, and fix all of our CI errors.
  • Remove tool.poetry.group.container.dependencies section from pyproject.toml, as it's duplicated info.
    • Actually, it still has its uses
  • Remove --userns keep-id option in Podman.
  • Make donwload-tessdata.py cacheable in our CI runs.
  • Turn OCR language deps into recommendations in Linux systems, and handle if some are not installed.
  • Improve our Dummy isolation provider, so that the steps that run in the host actually run in our Windows / macOS CI runners.
  • Update our packaging logic so that we don't include share/tessdata in our .debs / .rpms.
  • Update our wording in various places, so that we no longer refer to using two containers for the sanitization.
  • Draft an ARCHITECTURE.md, which will be the source of truth on how Dangerzone works now.

All these cannot be tackled in a single PR, but we at least need to have issues for the ones we won't tackle immediately, before merging this PR.

Copy link
Contributor

@deeplow deeplow left a comment

Choose a reason for hiding this comment

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

This is pretty incredible. Congrats! 🥳 A lot of work went before this and now this feels like the cherry on top. I have some minor code improvement suggestions.

What I still have to do:

  • test on windows and macOS

Other observations:

  • thanks for removing the dead code!
  • the GUI code is crashing on me. I think the latest PySide6 version on pypi is broken.
  • ubuntu focal - how do we solve lack of support? PyMuPDF in a virtualenv?
  • dummy can have pixels_to_pdf removed
  • Progress text improvements: because the conversion to PDF is now native and so fast, maybe we could replace the two log lines by one saying "converting page X". And when OCR is used we could say "making page X searchable"

dangerzone/isolation_provider/base.py Outdated Show resolved Hide resolved
dangerzone/conversion/common.py Show resolved Hide resolved
dangerzone/conversion/common.py Show resolved Hide resolved
dangerzone/isolation_provider/base.py Outdated Show resolved Hide resolved
dangerzone/conversion/pixels_to_pdf.py Show resolved Hide resolved
dangerzone/isolation_provider/container.py Show resolved Hide resolved
dangerzone/isolation_provider/container.py Show resolved Hide resolved
dangerzone/isolation_provider/base.py Outdated Show resolved Hide resolved
dangerzone/isolation_provider/base.py Outdated Show resolved Hide resolved
@apyrgio apyrgio force-pushed the 625-host-stream branch 2 times, most recently from ae9090d to 8884cb8 Compare March 27, 2024 12:24
@apyrgio
Copy link
Contributor Author

apyrgio commented Mar 27, 2024

I'll reply to some of your observations as well:

the GUI code is crashing on me. I think the latest PySide6 version on pypi is broken.

In my Fedora 39 dev environment, the GUI seems to work. Can you provide the error log?

ubuntu focal - how do we solve lack of support? PyMuPDF in a virtualenv?

I was thinking of either reusing PyMuPDF within the container, or using Tesseract just for Ubuntu Focal. I'll let you know.

dummy can have pixels_to_pdf removed

Yeap, you're right.

Progress text improvements: because the conversion to PDF is now native and so fast, maybe we could replace the two log lines by one saying "converting page X". And when OCR is used we could say "making page X searchable"

Yeap, you're right.

@apyrgio apyrgio force-pushed the 625-host-stream branch 5 times, most recently from da0dd54 to 10522c2 Compare March 28, 2024 16:29
@deeplow
Copy link
Contributor

deeplow commented Mar 28, 2024

Update our packaging logic so that we don't include share/tessdata in our .debs / .rpms.

I worked on this. The code is in the branch 625-host-stream-tessdata-packaging. A lot of stuff had to be moved and I didn't manage to finish testing this week. I tested on fedora and debian and it seems to be building fine. The only thing is that it includes the .gitkeep in share/container.

On macOS it seems to be failing but I haven't had time to investigate. If you have the chance before me, feel free to continue where I left @apyrgio.

stdeb.cfg Outdated Show resolved Hide resolved
@apyrgio apyrgio mentioned this pull request Apr 18, 2024
@apyrgio apyrgio added this to the 0.7.0 milestone Jun 3, 2024
dangerzone/util.py Outdated Show resolved Hide resolved
@almet almet removed this from the 0.7.0 milestone Jun 12, 2024
@apyrgio apyrgio force-pushed the 625-host-stream branch 2 times, most recently from 8f918c8 to 3125a59 Compare June 17, 2024 16:48
@apyrgio apyrgio mentioned this pull request Aug 8, 2024
12 tasks
Add a Python script that can run in all supported platforms, and can
download and extract the Tesseract language data from GitHub, while
also:

1. Checking that the expected hash matches.
2. Informing the user if the language data have already been downloaded.
3. Extracting only the subset of language data that Dangerzone needs
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.
The PyMuPDF package was previously mainly used within the Dangerzone
container, as well as on Qubes. With on-host conversion, PyMuPDF will be
used in all supported platforms by default. For this reason, we can
promote it to a main dependency.
Update .deb/.rpm specs to include PyMuPDF as a required package.
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.
Move the logic for grabbing debug logs to a new place, now that we have
merged the two conversion stages (doc to pixels, pixels to PDF).
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.
Copy link
Contributor

@almet almet left a comment

Choose a reason for hiding this comment

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

… and we're good to go on this one, congrats 🙌🏼

@almet almet dismissed deeplow’s stale review October 17, 2024 13:23

Comments have been adressed :-)

@apyrgio apyrgio merged commit 03b3c9e into main Oct 17, 2024
90 checks passed
@apyrgio apyrgio deleted the 625-host-stream branch October 17, 2024 13:26
apyrgio added a commit that referenced this pull request Dec 4, 2024
Now that #748 has been merged, we can move the `--userns nomap` argument
to the list with the rest of our security arguments.
apyrgio added a commit that referenced this pull request Dec 9, 2024
Now that #748 has been merged, we can move the `--userns nomap` argument
to the list with the rest of our security arguments.
apyrgio added a commit that referenced this pull request Dec 10, 2024
Now that #748 has been merged, we can move the `--userns nomap` argument
to the list with the rest of our security arguments.
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.

On-host pixels to PDF conversion
3 participants