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

crypt: support timestamped filenames from --b2-versions (attempt 2) #5205

Merged
merged 2 commits into from
Apr 12, 2021
Merged

crypt: support timestamped filenames from --b2-versions (attempt 2) #5205

merged 2 commits into from
Apr 12, 2021

Conversation

domyd
Copy link
Contributor

@domyd domyd commented Apr 3, 2021

What is the purpose of this change?

Continues #4024.

Fixes #1627

I refactored the versioning code into its own package lib/version, including the filename versioning tests in backend/b2.

While rclone --b2-versions ls TestCryptB2: would now correctly list all file versions in plaintext, rclone --b2-versions copy TestCryptB2:testfile-v(...).txt tmp/ wouldn't work. That turned out to be due to encryptFileName not dealing with the versioned filenames, yet decryptFileName does, so I've also fixed that.

I'd also like to add some integration tests to check the interaction between crypt and b2 - particularly for the copy issue I ran across - but I'm not sure how to go about it.

Was the change discussed in an issue or in the forum before?

Yes, in the previous PR.

Checklist

  • As noted above, an integration test between crypt and b2 with --b2-versions is probably something we want.
  • I have added documentation for the changes if appropriate.
  • All commit messages are in house style.
  • I'm done, this Pull Request is ready for review :-)

@domyd
Copy link
Contributor Author

domyd commented Apr 3, 2021

I'm not sure what that Race test failure is about, seems like a flaky test? It passed the first time around.

@ivandeex
Copy link
Member

ivandeex commented Apr 3, 2021

Most prominent flaky tests: cmd/mount cmd/cmount TestMaxTransfer/Cautious
Rerunning test suite: click "Rerun all jobs" from the "Check" tab. If you don't have permissions for that, the git commit --amend --no-edit; git push --force

@ivandeex
Copy link
Member

ivandeex commented Apr 3, 2021

@ncw can give good suggestions about adding integration tests

Copy link
Member

@ncw ncw left a comment

Choose a reason for hiding this comment

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

I'm finding this rather difficult to review...

Can you swap the order of the commits please? So the first commit is b2: factor version handling into lib/version and the second is crypt: support timestamped filenames from --b2-versions

Let me know if you need help with this. (I would git reset HEAD~2 then commit the b2 + lib/versions changes then the crypt changes in two separate commits).

Thanks

@domyd
Copy link
Contributor Author

domyd commented Apr 4, 2021

You're right, that makes much more sense. I was mostly concerned with preserving @wlritchi's commit, before I realized that there's a co-authoring feature. This should now be easier to review and still preserve authorship.

Copy link
Member

@ncw ncw left a comment

Choose a reason for hiding this comment

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

I think that is looking very nice.

Thanks for swizzling the commits and well done for figuring out the co-author feature.

I put a very small number of comments inline for you to look at.

I don't think this feature can ever clash with standard name encryption (as - isn't valid base32) or no name encryption. However it could clash with obfuscation...

Not sure what to do about that. Nothing probably as I think it is very unlikely.

@@ -172,11 +172,6 @@ the file instead of hiding it.
Old versions of files, where available, are visible using the
`--b2-versions` flag.

**NB** Note that `--b2-versions` does not work with crypt at the
Copy link
Member

Choose a reason for hiding this comment

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

This feature should probably get a mention in the docs but not here!

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'm not quite sure what you mean. Since this is merely a bugfix for B2 users, the removal of this notice seems appropriate? Where else might I change the docs?

backend/crypt/cipher_test.go Outdated Show resolved Hide resolved
lib/version/version.go Show resolved Hide resolved
backend/crypt/cipher_test.go Outdated Show resolved Hide resolved
domyd and others added 2 commits April 6, 2021 21:45
Standardizes the filename version tagging so that it can be used by any
backend.
With the file version format standardized in lib/version, `crypt` can
now treat the version strings separately from the encrypted/decrypted
file names. This allows --b2-versions to work with `crypt`.

Fixes #1627

Co-authored-by: Luc Ritchie <luc.ritchie@gmail.com>
@domyd
Copy link
Contributor Author

domyd commented Apr 7, 2021

I've made the requested changes to the tests, and also added a few more test cases.

Something I noticed is that the version adding is really awkward on obfuscated file names (e.g. hello-v2021-04-06-183434-000 turns into 20-v2021-04-06-183434-000.AxEEH), due to how it tries to be clever with file extensions. I tried having version.Add put the version string at the end of the filename when using obfuscation, but that ended up not working since the b2 backend doesn't know when obfuscation is being used. Fixing that would require b2 - and any other backend using the version strings - to know about obfuscated file names somehow, not sure if it's worth? You'd only notice if you were to access the raw crypt backend with --b2-versions and obfuscation enabled, so it's probably really niche.

@ncw
Copy link
Member

ncw commented Apr 12, 2021

I've made the requested changes to the tests, and also added a few more test cases.

Thank you - this looks great now, I will merge it in a moment.

Something I noticed is that the version adding is really awkward on obfuscated file names (e.g. hello-v2021-04-06-183434-000 turns into 20-v2021-04-06-183434-000.AxEEH), due to how it tries to be clever with file extensions. I tried having version.Add put the version string at the end of the filename when using obfuscation, but that ended up not working since the b2 backend doesn't know when obfuscation is being used. Fixing that would require b2 - and any other backend using the version strings - to know about obfuscated file names somehow, not sure if it's worth? You'd only notice if you were to access the raw crypt backend with --b2-versions and obfuscation enabled, so it's probably really niche.

Yes you are right - the namespace for obfuscated names and versions overlaps which is annoying but I don't think we can do anything sensible about it.

Thank you for taking this up and finishing it.

@ncw ncw merged commit 3fe2aaf into rclone:master Apr 12, 2021
@domyd domyd deleted the crypt-support-b2-versions branch April 18, 2021 10:59
@enginefeeder101
Copy link

@domyd Thank you for making the effort and finishing the work started by @wlritchi
Hopefully this will hit stable soon, I am not fond of using the master branch on my main backup remote to retrieve old versions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

--b2-versions doesn't work for crypt backed by B2
4 participants