Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

polling /register to check if email verification is complete is CPU intensive #7956

Closed
ara4n opened this issue Jul 25, 2020 · 3 comments · Fixed by #8009
Closed

polling /register to check if email verification is complete is CPU intensive #7956

ara4n opened this issue Jul 25, 2020 · 3 comments · Fixed by #8009
Assignees
Labels
A-Performance Performance, both client-facing and admin-facing z-p2 (Deprecated Label)

Comments

@ara4n
Copy link
Member

ara4n commented Jul 25, 2020

Element polls /register to see whether your email verification link has been clicked or not. Apparently we submit the password every time we do so, and /register hashes that with bcrypt every time, meaning you can easily use >100% CPU with people just trying to sign up. Surely we should not re-hash every time, and/or not submit a password every time, given we're just polling to check if the link has been clicked...

@ara4n ara4n changed the title polling /register to see if your email verification link is clicked chews CPU due to overeager bcrypt polling /register to see if your email verification link is clicked can chew >100% CPU due to overeager bcrypt Jul 25, 2020
@anoadragon453 anoadragon453 added z-p2 (Deprecated Label) A-Performance Performance, both client-facing and admin-facing labels Jul 27, 2020
@anoadragon453
Copy link
Member

The benefit for the client sending the password here is that once the email is verified, the check call will result in complete account registration. There's no "ok, the email's finally verified. Let's make the final call to finish account registration". It all just happens in one call once the email is verified - so it makes sense for the client to be doing this.

Thus I think this should be on us to fix server-side, and from discussion surrounding it on matrix doesn't seem impossible for any reason.

@clokep
Copy link
Member

clokep commented Jul 28, 2020

I poked a little bit and it should be doable. The code there is already a bit confusing so not sure it'll make it worse. 😉

@clokep clokep changed the title polling /register to see if your email verification link is clicked can chew >100% CPU due to overeager bcrypt polling /register to check if email verification is complete is CPU intensive Jul 30, 2020
@clokep
Copy link
Member

clokep commented Jul 30, 2020

and/or not submit a password every time

I think this is the real solution, FWIW. If you're only polling to see if it is complete yet, passing the session ID should be enough.

Anyway, having looked at the code this is all a bit complicated since we seem to have a lot of backwards compatibility hacks:

  1. The non-auth parameters passed to register (or any UI auth operation) get stored in the database.
  2. Because we do not want to store the password in plaintext, we hash it first.
  3. In subsequent requests if no non-auth parameters are passed the stored parameters are used.

Note that it is a bit unclear whether the no-auth parameters need to be passed each time or if they should only be passed once. I think we've had past conversation about this, but I don't remember where they landed.

I'm not seeing a great way to know whether or not the password is "new" (and thus needs to be hashed vs. discarded), but I'll look again after clearing my head.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-Performance Performance, both client-facing and admin-facing z-p2 (Deprecated Label)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants