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 a session wrapper to encrypt the data before storing it on disk #18482

Merged
merged 2 commits into from
Aug 24, 2015

Conversation

LukasReschke
Copy link
Member

Reanimation of #17744 until we can have #17866

Reviewers please:

@nickvergessen @DeepDiver1975 @MorrisJobke

@LukasReschke
Copy link
Member Author

12:37:22 PHP Fatal error:  Class 'OC\Files\Storage\SMB' not found in /var/lib/jenkins/jobs/server-master-linux-externals-smb-windows-ext-ci/workspace/database/sqlite/external/smb-windows/label/master/apps/files_external/tests/backends/smb.php on line 41

This looks fishy @Xenopathic

@karlitschek
Copy link
Contributor

thanks @LukasReschke

ICrypto $crypto,
ISecureRandom $random,
IRequest $request,
ITimeFactory $timeFactory) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Probably not required anymore since this is now as well a session cookie

@scrutinizer-notifier
Copy link

A new inspection was created.

@ghost
Copy link

ghost commented Aug 21, 2015

🚀 Test PASSed.🚀
chuck

@PVince81
Copy link
Contributor

Works fine in web UI + sync client 1.8.4, tested with encryption which uses the session for the keys. 👍

@MorrisJobke
Copy link
Contributor

Tested and works 👍

MorrisJobke added a commit that referenced this pull request Aug 24, 2015
Add a session wrapper to encrypt the data before storing it on disk
@MorrisJobke MorrisJobke merged commit b3495a1 into master Aug 24, 2015
@MorrisJobke MorrisJobke deleted the encrypt-session-data branch August 24, 2015 10:10
@PVince81
Copy link
Contributor

Mysterious regression: #18557

@icewind1991
Copy link
Contributor

Did anyone bother checking the performance of this approach?

https://blackfire.io/profiles/compare/fc575a75-07e5-4b84-b266-d012db68745b/graph

33ms (on my machine) for every single request doesn't seem worth it to me since it's just obfuscation.
If we do want "encrypted" sessions we should make sure it's optimized and only enc/decrypt the data once instead of 43 times

@LukasReschke
Copy link
Member Author

If we do want "encrypted" sessions we should make sure it's optimized and only enc/decrypt the data once instead of 43 times

#17866 would have done this. - With this approach I'm not quite sure how we can achieve this if we want a somewhat useful encryption. Well, or we remove any HMAC verification which would make this even more snake-oily 🙊

@LukasReschke
Copy link
Member Author

Mhm… I think we could emulate this by putting everything into a single session field … Let me see…

@karlitschek
Copy link
Contributor

Hmm. Why does it happen 43 times?

@LukasReschke
Copy link
Member Author

Hmm. Why does it happen 43 times?

Because with this approach every item in the session is encrypted itself instead of the whole session. And thus decrypting each item is required on it's own which involves HMACing ;-)

@karlitschek
Copy link
Contributor

Hmmm. Can we have a config.php option so that this can be switch off on heavy load situations?

@LukasReschke
Copy link
Member Author

Hmmm. Can we have a config.php option so that this can be switch off on heavy load situations?

No. But we can fix this. Please no config switches for stuff that we can fix properly. THX.

@LukasReschke
Copy link
Member Author

Config switch = Nobody tests one of the conditions = It's broken.

@karlitschek
Copy link
Contributor

Agreed. IF you can fix it :-)

@LukasReschke
Copy link
Member Author

@LukasReschke
Copy link
Member Author

#18913

@karlitschek
Copy link
Contributor

LOL

@lock lock bot locked as resolved and limited conversation to collaborators Aug 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants