Disabling client side rate limiting in Okta login MFA client #15369
Conversation
| @@ -0,0 +1,3 @@ | |||
| ```release-note:bug | |||
| mfa/okta: disable client side rate limiting causing delays in push notifications | |||
There was a problem hiding this comment.
Nit: Should we just use "auth" here as the top level category for the change?
There was a problem hiding this comment.
I'd think that auth/okta refers to the Okta auth method specifically.
There was a problem hiding this comment.
Agreed. Not sure how to categorize login mfa.
There was a problem hiding this comment.
I am good either way. I just mentioned that because we used auth as the category for login MFA before, and then message was more specifically talked about what has changed in Login MFA.
| @@ -1825,6 +1825,8 @@ func (c *Core) validateOkta(ctx context.Context, mConfig *mfa.Config, username s | |||
| } else { | |||
| client = okta.NewClient(cleanhttp.DefaultClient(), oktaConfig.OrgName, oktaConfig.APIToken, oktaConfig.Production) | |||
| } | |||
| // Disable client side rate limiting | |||
| client.RateRemainingFloor = 0 | |||
There was a problem hiding this comment.
According to Okta docs, the minimum rate limit is 100 for the requests that are generated from us. So, I am wondering if setting the floor value to 0 would cause any issue (performance or security)? Since 100 limit is imposed from Okta, should we consider increasing the floor value from 0 to something like 30, or even 50? This way, we would still have enforced some limit on the number of requests, and mitigate the delay in push notifications.
There was a problem hiding this comment.
The way that I understand how RateRemainingFloor works (docs) is that it behaves more like a padding, with the default value of 30 if unset. This means that for an Okta (dev) account that has a limit of 100 req/min, once the server returns "30 request remaining", the client will stop making requests. So what we're trying to do here is to remove that padding on the client side. Fortunately non-dev accounts have much higher rate limits (600).
There was a problem hiding this comment.
Oh, then that'd definitely do it! The client would never be able to poll for dev accounts past the first call. Thanks for cross-checking the docs against the actual default!
There was a problem hiding this comment.
Yeah, the comment says 30 while the actual value is 100. Since we are moving aways from this lib, I'm fine with 30 or 50. But really I don't really see to much of a need for client side rate limiting, even though it exists in this lib.
There was a problem hiding this comment.
Another questions to think about is, will be enable client side rate limiting when migrating to the official SDK? It looks like the official SDK only has a WithRateLimitMaxBackOff but I would guess that is when you run up to the limit instead of a threshold when you limit client side requests.
There was a problem hiding this comment.
I see your point about the client side rate limit. I am good with setting the floor value to 0. Thanks for the context.
WithRateLimitMaxBackOff seems to be setting maxBackoff to 30. Not sure how does in work though. In general, I would vote for being consistent between releases. But, that might be hard to verify.
|
That is a good point, I believe non-dev users should be fine. Hmm, based on this, would we need this change? |
|
@hghaf099 Considering that it is unusable right now for dev accounts, I think we still should make some change (whether disable completely or lower the threshold). Many individuals will be validating this behavior with dev accounts and we should provide them a similar experience as production use. |
The behavior of the login MFA is currently limited due to the client side rate limiting happening in the SDK. This SDK is older and there may be a change in the API behavior that is not fully accounted for in this client.