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

Chunker permutation #5070

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Chunker permutation #5070

wants to merge 5 commits into from

Conversation

ncleaton
Copy link
Contributor

@ncleaton ncleaton commented Apr 2, 2020

Make the buzhash chunker more resistant to fingerprinting by introducing a permutation of the buzhash table, as discussed at #3687

@ncleaton ncleaton mentioned this pull request Apr 2, 2020
@codecov-io
Copy link

codecov-io commented Apr 2, 2020

Codecov Report

Merging #5070 (6647331) into master (d2673c0) will decrease coverage by 0.00%.
The diff coverage is 92.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5070      +/-   ##
==========================================
- Coverage   83.15%   83.15%   -0.01%     
==========================================
  Files          38       38              
  Lines       10058    10078      +20     
  Branches     1668     1672       +4     
==========================================
+ Hits         8364     8380      +16     
- Misses       1202     1204       +2     
- Partials      492      494       +2     
Impacted Files Coverage Δ
src/borg/crypto/key.py 86.71% <90.47%> (+0.11%) ⬆️
src/borg/archive.py 81.44% <100.00%> (-0.14%) ⬇️
src/borg/selftest.py 73.80% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c2118f1...6647331. Read the comment docs.

@ThomasWaldmann
Copy link
Member

It would be useful, if someone else would also review this.

@textshell
Copy link
Member

On first look this seems to silently diverge on the chunking when this key is used in older borg versions? That sounds problematic.

Copy link
Member

@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.

found one cosmetic thing while doing another review.


if pool == 0:
# the pool value was less than 256!, we have an unbiased choice
return bytes(perm)
Copy link
Member

Choose a reason for hiding this comment

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

could be also just break.

@ThomasWaldmann
Copy link
Member

ThomasWaldmann commented Nov 7, 2020

@textshell looks to me like:

  • if repo was initialized with old borg (e.g. 1.1.14), the PR code will use the null permutation and behave same, compute same chunks, no dedup issue, even if borg is switched between old and new versions.

  • if repo was initialized with new PR code, the key material will be longer (132 bytes instead of 100 bytes), the new code would compute different chunks than the old code. So there would be a dedup issue, but we could just document that in changelog.

I have still to check how the old code deals with a key that is longer than expected. I guess it would just silently ignore that.

Do you think it is problematic if we just document "don't use borg 1.1.x on repos created with borg 1.2"?

@ThomasWaldmann
Copy link
Member

ThomasWaldmann commented Nov 7, 2020

We could also set item.Key.version to 2 (was: 1), then borg 1.1.x code will reject working with a key made by borg 1.2.

borg 1.2 would check the version and accept version 1 (null permutation, compatibility mode) or version 2 (random permutation).

@ThomasWaldmann
Copy link
Member

rebased on current master.

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