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

Support base64 and base32768 file name encoding for crypt. #5802

Merged
merged 2 commits into from Nov 15, 2021

Conversation

tinytangent
Copy link
Contributor

@tinytangent tinytangent commented Nov 10, 2021

What is the purpose of this change?

This PR add support for base64 and base32768 file name encoding for crypt. This resolves #5801 and relieve the problem of file name length being too long when encrypted.

It is based on the commit 78cab2b by @Max-Sum . For other changes in https://github.com/Max-Sum/rclone/commits/compress_crypted_filename, I' m worried that some of them might break backward compatibility or involving security issue, and I' m not very familiar in cryptography.

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

https://forum.rclone.org/t/whats-going-on-with-the-proposal-about-base32768-file-name-encoding/27341
https://forum.rclone.org/t/base32768-to-compress-filename-length/13202

Checklist

  • I have read the contribution guidelines.
  • I have added tests for all changes in this PR if appropriate.
  • 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 :-)

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 put some initial comments inline.

This should probably also have some tests in backend/crypt/crypt_test.go to run the backend tests against the new file name encoding.

I'm going to allow the CI to run now - expect some linting problems you'll need to fix up!

backend/crypt/cipher.go Show resolved Hide resolved
DecodeString(s string) ([]byte, error)
}

type CaseInsensitiveBase32Encoding struct {
Copy link
Member

Choose a reason for hiding this comment

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

This seems over complicated to me....

I'd suggest you define this (note lower case so we don't make a public symbol)

type caseInsensitiveBase32Encoding struct {}

Then define the two methods

func (_ caseInsensitiveBase32Encoding) EncodeToString(src []byte) string {
// contents the same as encodeFileName in original source
}
func (_ caseInsensitiveBase32Encoding) DecodeString(s string) ([]byte, error) {
// contents the same as decodeFileName in original source
}

And finally check it satisfies the interface

var _ StrEncoding = caseInsensitiveBase32Encoding{}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed this change is modifying codes that already work. I reverted the base32 implementation in 79817be.

@@ -114,22 +117,65 @@ func (mode NameEncryptionMode) String() (out string) {
return out
}

// Cipher defines an encoding and decoding cipher for the crypt backend
// StrEncoding is encoding methods dealing with strings
type StrEncoding interface {
Copy link
Member

Choose a reason for hiding this comment

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

This should be called filenameEncoding - initial lower so it isn't exported and more specific name

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 sorry about this. Fixed in 2767ee3.

s = strings.ToLower(s)
switch s {
case "base32":
base32hex := base32.NewEncoding(encodeHexLower)
Copy link
Member

Choose a reason for hiding this comment

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

...this then becomes

enc = caseInsensitiveBase32Encoding{}

@tinytangent
Copy link
Contributor Author

I also added some new fstests in backend/crypt/crypt_test.go for the newly added base64 and base32768 encoding in 046656d.

@tinytangent
Copy link
Contributor Author

Linting problems should now be fixed in fb0fb43.

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.

This looks great now - thank you.

Can you rebase it on master, fix the conflicts and squash it down to two commits - one for Max-Sum's work and one for your work.

(can you make sure Max-Sum commit compiles on its own - if it doesn't fix it!)

I'll merge it then - thank you.

PS If any of this git stuff is too much for you then I can do it easily enough.

@tinytangent
Copy link
Contributor Author

tinytangent commented Nov 13, 2021

Thank you~

I have reorganized the commit history to make it cleaner. So now d2e180f contains all the code changes including go.sum and go.mod, and it compiles on it own (but will fail most of test cases due to internal interfaces changes).

1bd9007 updates test cases and docs.

@tinytangent
Copy link
Contributor Author

tinytangent commented Nov 13, 2021

There is another thing to note: Currently there is a failing test case in backend/crypt, but it is unlikely introduced by my changes because it is also happening on the master branch, it looks like this:

--- FAIL: TestNoDataObfuscate (0.53s)
    fstests.go:418: Using remote "TestCrypt4:"
    --- FAIL: TestNoDataObfuscate/FsMkdir (0.49s)
        --- FAIL: TestNoDataObfuscate/FsMkdir/FsPutFiles (0.39s)
            --- FAIL: TestNoDataObfuscate/FsMkdir/FsPutFiles/Internal (0.00s)
                --- FAIL: TestNoDataObfuscate/FsMkdir/FsPutFiles/Internal/ObjectInfo (0.00s)
                    crypt_internal_test.go:94: 
                                Error Trace:    crypt_internal_test.go:94
                                                                        crypt_internal_test.go:140
                                Error:          Not equal: 
                                                expected: 148
                                                actual  : 100
                                Test:           TestNoDataObfuscate/FsMkdir/FsPutFiles/Internal/ObjectInfo
                --- FAIL: TestNoDataObfuscate/FsMkdir/FsPutFiles/Internal/ObjectInfoWrap (0.00s)
                    crypt_internal_test.go:94: 
                                Error Trace:    crypt_internal_test.go:94
                                                                        crypt_internal_test.go:141
                                Error:          Not equal: 
                                                expected: 148
                                                actual  : 100
                                Test:           TestNoDataObfuscate/FsMkdir/FsPutFiles/Internal/ObjectInfoWrap

I' m not very certain if such a failure is expected or something went wrong. Maybe I should open another issue about this?

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.

That looks great now - thank you - I will merge now.

@ncw
Copy link
Member

ncw commented Nov 15, 2021

...actually I will wait for the CI to complete before merging!

@ncw
Copy link
Member

ncw commented Nov 15, 2021

There is another thing to note: Currently there is a failing test case in backend/crypt, but it is unlikely introduced by my changes because it is also happening on the master branch, it looks like this:
[snip]
I' m not very certain if such a failure is expected or something went wrong. Maybe I should open another issue about this?

That is very interesting - thank you.

What that shows up is that test isn't being run on the CI, nor in the integration tests which is a bit of an oversight, so thank you for bringing it to my attention.

I'll fix the test not being run and the test itself shortly :-)

@ncw ncw merged commit 8c61a09 into rclone:master Nov 15, 2021
@Max-Sum
Copy link
Contributor

Max-Sum commented Oct 17, 2022

Onedrive used to have a bug on processing special characters. Microsft ignores this issue and therefor I didn't continue on developing this. Sorry for that.
You could test if it works on Onedrive now.

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.

Base32768 file name encoding for crypt backend
4 participants