ENH Increase rate limit

[emteknetnz]

Issue https://github.com/silverstripe/silverstripe-framework/issues/9474

Related PR for limiting rate limit to only POST requests https://github.com/silverstripe/silverstripe-framework/pull/9982

[madmatt]

This seems good to me. As you can see below, we double the number of necessary requests (assuming only POST is required) for a single good verification, so doubling the number of allowed failures before rate limiting kicks in feels okay too, though 20 also feels like maybe it's too many (and if we haven't had lots of complaints, maybe it's not really that big of a deal and we could just leave this alone - given if only POST requests count now then it's far less likely to cause issues already)?

Coupled with silverstripe/silverstripe-framework#9982, where now only POST requests would be counted, does that change how many requests MFA would make? As I see it, here's what happens (for TOTP at least, I don't know if other methods like WebAuthN require additional calls):

1. GET request to /Security/login (previously was counted, now won't be)
2. POST request to /Security/login/default/LoginForm (1 POST request towards rate limit)
3. GET request to /Security/login/default/mfa (previously was counted, now won't be)
4. GET request to /Security/login/default/mfa/verify/totp (previously was counted, now won't be)
5. POST request to /Security/login/default/mfa/verify/totp with TOTP code for example (1 POST request towards rate limit)
6. (More singular POST requests to the verify endpoints if the first verification fails)
7. GET request to /Security/login/default/mfa/complete once MFA is complete (previously was counted, now won't be)

So all in all, previously there were 6 requests minimum that counted, now with these two changes there would only be two requests that count (assuming the user correctly enters their request) - so I'd say this almost feels unnecessary.

[emteknetnz]

My thinking here is:

• The previous max limit of 10 pre-dates MFA, and my assumption of the intention was "10 login attempts from a single IP" rather than "10 requests"
• MFA doubles the required number of POST requests, for increase this to 20 requests still retains the "10 login attempts from a single IP" - though unfortunately this also allows "1 correct user/pass + 19 attempts at MFA"

so I'd say this almost feels unnecessary.

So what you're saying is a) do keep the PR that restricts rate-limiting to POST requests b) discard the PR that bumps the limit when MFA is installed from 10 to 20

?

[madmatt]

Basically I think that implementing both changes feels like a bit of overkill, so my opinion is to do one or the other, but not both. I've flip-flopped on this a bit, so I feel like maybe another opinion is needed.

I think the best option is to merge this change and not merge silverstripe/silverstripe-framework#9982. This means that both GET and POST requests will be counted toward the limit, and therefore we increase the number of allowed attempts when using MFA (because more requests are needed to successfully login).

Keen for your thoughts on this too!

[emteknetnz]

Only bumping it to 20 on MFA seems good. It does mean less code changes which is a good thing

20 is still probably on the low end specifically for scenario of shared IPs within an office

There is a risk that increasing this too high opens the door for brute force attacks, however there is an additional layer of limiting on MFA as mentioned on the parent issue:

On my local when I repeatedly enter the wrong TOTP code, eventually I get:
"Your account is temporarily locked. Please try again later." with a 403 status code
This codes from silverstripe-mfa/src/Authenticator/LoginHandler.php

This functionally is triggered when the number of login attempts exceeds the private static on Member $lock_out_after_incorrect_logins = 10;

This applies to both MFA and the regular user/pass login form.

This seems like it would negate the riskiest thing would be a site with MFA installed though not mandatory, meaning an attacker has more attempts at cracking the user/pass

I'm OK with 20, though 50 is also probably fine too.

@madmatt Let me know what you think the final number should be

[madmatt]

IMO let's do 20. I don't think we've had any actual complaints about the feature yet, so let's not go too overboard with this.

Thanks!

[emteknetnz]

Cool, thanks for you assistance on this one @madmatt

Someone else in the team will pick this up for review