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

Fixes related key issue. #23496

Merged
merged 2 commits into from
Oct 27, 2020

Conversation

lynn-stephenson
Copy link
Contributor

This should fix it, I implemented tests, too. Everything seems to work fine when I ran the tests. I decided to leave additional changes for a later pull request. I don't really see any sense in overhauling the encryption scheme to where it breaks a bunch of unit tests if I'm not going to just pull in Halite, which would be far better than my implementation anyways.

Here you go. 😄

Signed-off-by: lynn-stephenson <lynn.stephenson@protonmail.com>
Signed-off-by: lynn-stephenson <lynn.stephenson@protonmail.com>
@lynn-stephenson
Copy link
Contributor Author

Well, nobody's perfect. Accidentally said version 1 and 2; not 2 and 3. Oops.

@nickvergessen

This comment has been minimized.

@nickvergessen nickvergessen added this to the Nextcloud 21 milestone Oct 21, 2020
@nickvergessen
Copy link
Member

Ah, it's from you too, ref #23427

@kesselb
Copy link
Contributor

kesselb commented Oct 21, 2020

Looks good to me 👍

I understand (roughly) why using key material derived from the password is better then using the password directly. But my knowledge about crypto is limited hence someone else needs to decide on this.

Please keep in mind that hash_hkdf may return false on failure. But that looks more like a theoretical issue because the password is never empty, sha512 shipped with php and length is set by the hash function. If you see any other issues from this method we may use v2 as fallback.

@lynn-stephenson
Copy link
Contributor Author

I understand (roughly) why using key material derived from the password is better then using the password directly.

@kesselb It's not just that. You don't want to use passwords directly, NOR use practically the same key for encryption and MAC.

I could have improved so much more, and I did on my last PR as a draft (even then I didn't include all cryprography improvements due to a lot of work). But then I realized it broke so much stuff I might as well have swapped out NextCloud's crypto with Halite.

@lynn-stephenson
Copy link
Contributor Author

But switching to Libsodium and Halite is definitely something NextCloud should look into.

Copy link
Member

@rullzer rullzer left a comment

Choose a reason for hiding this comment

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

Nice one.
This crypto file is so weird (not passing the cipher around but having it as a class member etc. But that is not related to this PR.

Also kudos for the tests!

@rullzer rullzer merged commit cf1af5b into nextcloud:master Oct 27, 2020
@welcome
Copy link

welcome bot commented Oct 27, 2020

Thanks for your first pull request and welcome to the community! Feel free to keep them coming! If you are looking for issues to tackle then have a look at this selection: https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22

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.

4 participants