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.

Sign up for GitHub

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

Disabling client side rate limiting in Okta login MFA client #15369

Merged
merged 2 commits into from
May 12, 2022
Merged

Disabling client side rate limiting in Okta login MFA client #15369

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6f0a4a4
disabling client side rate limiting for MFA client
chrishoffman May 11, 2022
b01e169
add changelog
chrishoffman May 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions changelog/15369.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:bug
mfa/okta: disable client side rate limiting causing delays in push notifications
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Should we just use "auth" here as the top level category for the change?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd think that auth/okta refers to the Okta auth method specifically.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. Not sure how to categorize login mfa.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

```
2 changes: 2 additions & 0 deletions vault/login_mfa.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

@hghaf099 hghaf099 May 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@calvn calvn May 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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).

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure if 30 is the correct default though based on what I see in the code here.
Seems the default is 100

Copy link
Contributor

Choose a reason for hiding this comment

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

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.


var filterOpts *okta.UserListFilterOptions
if oktaConfig.PrimaryEmail {
Expand Down