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

Update password check not working as expected #33757

Closed
juliusknorr opened this issue Aug 30, 2022 · 3 comments · Fixed by #33898
Closed

Update password check not working as expected #33757

juliusknorr opened this issue Aug 30, 2022 · 3 comments · Fixed by #33898
Labels
1. to develop Accepted and waiting to be taken care of bug performance 🚀

Comments

@juliusknorr
Copy link
Member

Has anyone verified if this has any effect?

I just installed 23.0.8 and still see all the user's oc_authtokens being updated on basic auth requests.

My theory: this patch compares the saved encrypted password with the newly generated encrypted password. What it does not take into account is that openssl_public_encrypt outputs a different ciphertext every time, even on identical input values.

see https://stackoverflow.com/a/2980121 for an explanation.

Originally posted by @Mask in #33485 (comment)

@juliusknorr
Copy link
Member Author

So I can reproduce that the fix is not working as expected. Now solving this is not as easy as initially thought, since we do not store a hashed password, only an encrypted one, where the private key can only be decrypted by the specific session id that created the authtoken.

For some potential solutions we could calculate the signature of the encrypted password or the hash but would need to store that additionally in oc_authtokens. Then we could use openssl_verify or compare the hashes to know if the password actually has changed.

Now maybe we can work on a backward compatible patch which just appends the signature to the current password column, but feels less clean than an additional column. However a column is probably nothing back portable as it requires a migration and may take some time to add depending on the size of oc_authtokens.

@juliusknorr juliusknorr added bug 1. to develop Accepted and waiting to be taken care of performance 🚀 labels Aug 30, 2022
@juliusknorr
Copy link
Member Author

juliusknorr commented Aug 30, 2022

@nickvergessen @PVince81 @ChristophWurst Any opinions on one or the other way for this?

@nickvergessen
Copy link
Member

I'd say revert on stable and then just fix 25/26+ with a new column?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1. to develop Accepted and waiting to be taken care of bug performance 🚀
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants