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

Qubes: Client-side timeouts #446

Closed
Tracked by #412
apyrgio opened this issue Jun 14, 2023 · 0 comments · Fixed by #547
Closed
Tracked by #412

Qubes: Client-side timeouts #446

apyrgio opened this issue Jun 14, 2023 · 0 comments · Fixed by #547
Assignees
@deeplow
Labels
P:Qubes QubesOS integration
Milestone
0.5.0

Comments

@apyrgio
Copy link
Contributor

apyrgio commented Jun 14, 2023 •
edited
Loading

Timeouts are currently implemented within the conversion sandbox, since this is where we know the number of pages in a document (see #327). For edge cases, we allow users to disable timeouts, by passing an environment variable to the container.

In Qubes, this is not as easy, for two reasons:

  1. We don't want the attacker to decide when to stop the execution. This may be tolerable for containers, but it's not for heavyweight VMs.
  2. To propagate to the disposable qube that we want to disable timeouts, we must update our RPC policy. This is something that users cannot easily do, and we don't want to mess with variadic arguments to Qubes RPC calls at the moment.

A better way forward is to implement client timeouts. Due to the streaming nature of conversion in Qubes (see https://github.com/freedomofpress/dangerzone/wiki/Qubes-OS-Integration#wip-binary-protocol), we can find out the number of pages early in the conversion process, and the size of the document and each page as well. So, we can use our calculate_timeout() method on the client-side, and stop the disposable qube when it exceeds a timeout.

Note:

  1. This will be used for the container isolation provider as well, once we adapt it to the streaming model of the Qubes isolation provider.
  2. Our timeouts must take into account the time the system needs to start a disposable VM.
@apyrgio apyrgio added the P:Qubes QubesOS integration label Jun 14, 2023
@apyrgio apyrgio added this to the 0.5.0 milestone Jun 14, 2023
This was referenced Jun 14, 2023
Merged
Closed
Closed
Closed
apyrgio added a commit that referenced this issue Sep 13, 2023
@apyrgio
qubes: Add client-side timeouts
c3d7505 
Extend the client-side capabilities of the Qubes isolation provider, by
adding client-side timeout logic.

This implementation brings the same logic that we used server-side to
the client, by taking into account the original file size and the number
of pages that the server returns.

Since the code does not have the exact same insight as the server has,
the calculated timeouts are in two places:

1. The timeout for getting the number of pages. This timeout takes into
   account:
   * the disposable qube startup time, and
   * the time it takes to convert a file type to PDF
2. The total timeout for converting the PDF into pixels, in the same way
   that we do it on the server-side.

Besides these changes, we also ensure that partial reads (e.g., due to
EOF) are detected (see exact=... argument)

Some things that are not resolved in this commit are:
* We have both client-side and server-side timeouts for the first phase
  of the conversion. Once containers can stream data back to the
  application (see #443), these server-side timeouts can be removed.
* We do not show a proper error message when a timeout occurs. This will
  be part of the error handling PR (see #430)

Fixes #446
Refs #443
Refs #430
@apyrgio apyrgio mentioned this issue Sep 13, 2023
Merged
apyrgio added a commit that referenced this issue Sep 20, 2023
@apyrgio
qubes: Add client-side timeouts
6e05e3b 
Extend the client-side capabilities of the Qubes isolation provider, by
adding client-side timeout logic.

This implementation brings the same logic that we used server-side to
the client, by taking into account the original file size and the number
of pages that the server returns.

Since the code does not have the exact same insight as the server has,
the calculated timeouts are in two places:

1. The timeout for getting the number of pages. This timeout takes into
   account:
   * the disposable qube startup time, and
   * the time it takes to convert a file type to PDF
2. The total timeout for converting the PDF into pixels, in the same way
   that we do it on the server-side.

Besides these changes, we also ensure that partial reads (e.g., due to
EOF) are detected (see exact=... argument)

Some things that are not resolved in this commit are:
* We have both client-side and server-side timeouts for the first phase
  of the conversion. Once containers can stream data back to the
  application (see #443), these server-side timeouts can be removed.
* We do not show a proper error message when a timeout occurs. This will
  be part of the error handling PR (see #430)

Fixes #446
Refs #443
Refs #430
@apyrgio apyrgio mentioned this issue Sep 26, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@deeplow deeplow

Labels
P:Qubes QubesOS integration
Projects
None yet
Milestone
0.5.0
Development

Successfully merging a pull request may close this issue.

qubes: Add client-side timeouts freedomofpress/dangerzone
2 participants
@apyrgio @deeplow