-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Conversation
This looks fishy @Xenopathic |
thanks @LukasReschke |
cf68d90
to
e69045b
Compare
ICrypto $crypto, | ||
ISecureRandom $random, | ||
IRequest $request, | ||
ITimeFactory $timeFactory) { |
There was a problem hiding this comment.
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
e69045b
to
f14aee7
Compare
f14aee7
to
6a3fb0d
Compare
A new inspection was created. |
Works fine in web UI + sync client 1.8.4, tested with encryption which uses the session for the keys. 👍 |
Tested and works 👍 |
Add a session wrapper to encrypt the data before storing it on disk
Mysterious regression: #18557 |
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. |
#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 🙊 |
Mhm… I think we could emulate this by putting everything into a single session field … Let me see… |
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 ;-) |
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. |
Config switch = Nobody tests one of the conditions = It's broken. |
Agreed. IF you can fix it :-) |
LOL |
Reanimation of #17744 until we can have #17866
Reviewers please:
@nickvergessen @DeepDiver1975 @MorrisJobke