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

Add helper class to manage Borg version and supported features. #250

Merged
merged 5 commits into from
Apr 14, 2019

Conversation

m3nu
Copy link
Contributor

@m3nu m3nu commented Apr 8, 2019

Fixes #205

@m3nu m3nu requested a review from ThomasWaldmann April 8, 2019 02:39
Copy link
Collaborator

@ThomasWaldmann ThomasWaldmann left a comment

Choose a reason for hiding this comment

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

some stuff i found.

@@ -0,0 +1,24 @@
from pkg_resources import parse_version

MIN_BORG_FOR_FEATURE = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

zstd compression comes to mind. present since 1.1.4.

from pkg_resources import parse_version

MIN_BORG_FOR_FEATURE = {
'REPOKEY_BLAKE2': parse_version('1.1.4'),
Copy link
Collaborator

Choose a reason for hiding this comment

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

blake2 since 1.1.0.

Copy link
Collaborator

Choose a reason for hiding this comment

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

guess the key should be rather named 'BLAKE2' (it applies to repokey and also to keyfile modes).


MIN_BORG_FOR_FEATURE = {
'REPOKEY_BLAKE2': parse_version('1.1.4'),
# add new version-checks here.
Copy link
Collaborator

Choose a reason for hiding this comment

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

from our docs/changes.rst:

  • pre-1.1.4 potential data corruption issue
  • pre-1.0.9 manifest spoofing vulnerability (CVE-2016-10099)
  • pre-1.0.9 potential data loss

vorta could warn for < 1.1.4 or < 1.0.9 if you like.

'keyfile-blake2')

if app.borg_compat.check('REPOKEY_BLAKE2'):
self.encryptionComboBox.addItem(self.tr('Keyfile-Blake2 (Key stored in home directory)'),
Copy link
Collaborator

Choose a reason for hiding this comment

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

with the key naming fix, (see above) this will read less strange.

@m3nu m3nu force-pushed the issue/205/limit-repokey-blake2-support branch from a86ac4c to be5e5f2 Compare April 9, 2019 03:47
@m3nu
Copy link
Contributor Author

m3nu commented Apr 9, 2019

Thanks for the input. Made some more changes:

  • Check for ZStd feature: This was tricky, as I didn't want to change existing user settings. So I ended up disabling them in the list and giving an error when a backup is created with an old version.
  • Renamed the BLAKE2 feature and went to just disabling the options instead of removing them altogether. And refactored into a list to have less repetitive code.
  • Moved the borg_compat class instance to utils. It's easier accessible there and we already have the keyring instance there, which is similar in its usage.

@@ -1,7 +1,8 @@
from pkg_resources import parse_version

MIN_BORG_FOR_FEATURE = {
'REPOKEY_BLAKE2': parse_version('1.1.4'),
'BLAKE2': parse_version('1.1.4'),
Copy link
Collaborator

Choose a reason for hiding this comment

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

1.1.0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't you say there were some issues with blake2 before 1.1.4? So I figured I use the version where it was rock-solid.

def toggle_available_compression(self):
use_zstd = borg_compat.check('ZSTD')
self.repoCompression.model().item(1).setEnabled(use_zstd)
self.repoCompression.model().item(2).setEnabled(use_zstd)
Copy link
Collaborator

Choose a reason for hiding this comment

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

hardcoded 1 and 2 here is a potential future bug.

maybe determine offset somehow or add a comment above the place where the compression menu items are added to update this place in case of changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually considered this, but finding the offset needs an extra statement.

Since you noticed this as well, I added selecting the offset by value, rather than index.

self.repoCompression.model().item(2).setEnabled(use_zstd)
for algo in ['zstd,3', 'zstd,8']:
ix = self.repoCompression.findData(algo)
self.repoCompression.model().item(ix).setEnabled(use_zstd)
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice and solid! :)

@m3nu m3nu merged commit b9486d0 into borgbase:master Apr 14, 2019
@m3nu m3nu deleted the issue/205/limit-repokey-blake2-support branch January 20, 2021 05:24
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.

Only offer repokey-blake2 support to >=1.1.4 borg versions
2 participants