-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
auth/okta: Add support for Okta number challenge #15361
Conversation
# Conflicts: # builtin/credential/okta/backend.go # builtin/credential/okta/path_login.go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple of comments. Looks good to me otherwise 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@@ -246,7 +268,7 @@ func (b *backend) Login(ctx context.Context, req *logical.Request, username, pas | |||
} | |||
|
|||
select { | |||
case <-time.After(500 * time.Millisecond): | |||
case <-time.After(1 * time.Second): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Taking the opportunity in this PR to change the polling frequency from 500ms to 1s. The rationale behind this change is that Okta dev accounts has a rate limit of 100req/min on authn/
endpoints (non-dev accounts do have higher limits), and at this frequency calls made by a vault login will trigger a warning and eventually lead to rate limit exceeded errors. By lowering this to a max of 60req/min we avoid hitting this rate limit and getting into flaky testing scenarios. The bump should not be that noticeable since this is a purely human-based flow.
Login MFA uses a 1s polling frequency, so it should be acceptable for us here as well:
Line 1928 in a2fc885
case <-time.After(time.Second): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@calvn how can I know if you fixed that was merged to main was in deed included in a vault release version? thanks in advance. |
Hi @adrian-mi-romero, if you click on the commit sha from the "calvn merged commit" message above, and then expand the tags, it'll show which releases included this patch. Example screenshot: |
Thanks a lot @tvoran so it should be in v1.12.X ... wondering about it coz if you check the doc for v1.12.X it says your fix it is not supported/included. |
@adrian-mi-romero Best to open another issue about that. |
@calvn By chance did a recent update in 1.15.0 result in this feature breaking for Okta Verify Number Challenge? We're no longer presented with the corresponding matching number in the UI (previously working) as part of this PR. |
This PR adds the ability for the Okta Auth Method to perform the 3-number verification challenge if that's enabled on an Okta Organization.
The login flow is as follows, which happens within a single Vault login CLI command: