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

Optimize storage and speed when making an encrypted backup #1825

Open
wants to merge 1 commit into
base: v8
Choose a base branch
from

Conversation

pellaras
Copy link

@pellaras pellaras commented Jul 24, 2024

This PR addresses two issues:

  • There is a chance of a backup to be stored as unencrypted, without noticing it.
  • The free disk space needed for an encrypted backup, is double of what an unencrypted backup needs.

In more detail:

Using a Listener to wait for a successful creation of backup had the side effect of first creating a zip archive, unencrypted, then create the encrypted zip archive using the unencrypted one, which demanded double the available disk space, as well as more time.

Further to the above, and most concerning, is when the Listener fails to encrypt the zip file, it shows a warning about a notification failed to be sent, but it continues to store the archive on the destination disks, allowing for the unencrypted version to make it to a remote destination with possible privacy implications. A very likely scenario for this to happen, is when there is not enough disk space available for the encrypted backup to be generated, but the unencrypted one was successfully created.

This PR tackles the issue by setting the zip archive to use encryption as soon as the files are added, without the need of first making an unencrypted version on the temp folder. This also has a nice side-effect of needing the same available disk space as if no encryption was configured.

Happy to create a PR also for v9

Note: Tide-up the tests related to the encryption, as the Listener is no longer used.

@flexchar
Copy link

LGTM! Would you be as kind to spare a few minutes on this one @freekmurze?:)

@freekmurze
Copy link
Member

Will do to, could you rebase against main to resolve the conflicts?

@pellaras
Copy link
Author

@freekmurze are you asking me? If yes, I can do a PR against main, after getting this merged under v8.

@Nielsvanpach
Copy link
Member

There are merge conflicts preventing this from being merged. You can rebase against the v8 branch.

@pellaras
Copy link
Author

pellaras commented Oct 7, 2024

@freekmurze rebased

@pellaras
Copy link
Author

Is there anything else that can be done to help merge this PR?

I am emphasizing the issue of having unencrypted archives make it to a remote storage (i.e. s3) under specific circumstances.

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.

4 participants