Okta Dep Update #28121
Merged
Okta Dep Update #28121
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
the
hashicorp-contributed-pr
|
CI Results: |
kpcraig
force-pushed
the
VAULT-28868/okta-upgrade
branch
from
3b6fa3b to
57bed20
Compare
kpcraig
force-pushed
the
VAULT-28868/okta-upgrade
branch
from
fada61e to
80a05ad
Compare
kpcraig
force-pushed
the
VAULT-28868/okta-upgrade
branch
from
80a05ad to
b96116b
Compare
|
Build Results: |
kpcraig
added
backport/ent/1.16.x+ent
kpcraig
commented
Nov 22, 2024
| @@ -2070,21 +2070,25 @@ func (c *Core) validateOkta(ctx context.Context, mConfig *mfa.Config, username s | |||
| return err | |||
| } | |||
|
|
|||
| // Okta doesn't return the transactionID as a parameter in the response, but it's encoded in the URL | |||
| // this approach comes from: https://github.com/okta/okta-sdk-golang/issues/300, but it's not ideal. | |||
| // It is, however, what the dotnet library by Okta themselves does. | |||
There was a problem hiding this comment.
It is also the inverse of what GetFactorTransactionStatus does.
kpcraig
commented
Nov 23, 2024
| } | ||
| } | ||
|
|
||
| if !factorFound { | ||
| return fmt.Errorf("no push-type MFA factor found for user") | ||
| } | ||
|
|
||
| result, _, err := client.UserFactor.VerifyFactor(ctx, user.Id, userFactor.Id, okta.VerifyFactorRequest{}, userFactor, nil) | ||
| result, _, err := client.UserFactorAPI.VerifyFactor(ctx, user.GetId(), userFactor.GetId()).Execute() |
There was a problem hiding this comment.
The difference between the approach here and the approach in path_login.go is for 'light touch' reasons - path_login.go uses the "shim", since the setting a token is optional there. The code here already used the okta API directly, and so the updated version can be similarly updated, without using the "write the request yourself" escape hatch.
thyton
approved these changes
Nov 25, 2024
tvoran
reviewed
Nov 26, 2024
Co-authored-by: Theron Voran <tvoran@users.noreply.github.com>
3 tasks
Monkeychip
pushed a commit
that referenced
this pull request
Nov 27, 2024
Update okta to use v5 sdk instead of v2 --------- Co-authored-by: Theron Voran <tvoran@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR updates our Okta SDK dependency to v4, so that we transitively remove a go-jose dependency CVE-2024-28180. There have been a series of other tasks to purge the vulnerable dependency from
vault, this is the only one that requires larger code changes.Unfortunately, to get rid of the <=v2.6.2 vulerability, we had to move the
okta-sdkto at least v3, which causes these breaking changes below. There didn't appear to be additional (relevant) changes between v3 and v5, so I moved to v5 directly.This code retains the "shim" that was written that integrates
github.com/chrismalek/oktasdk-go, only in the case of operating without an API token - a mode that was deprecated in Vault 1.4. I think this might be removable, and an alternative option used for the "no token" case, but that might only be viable for a version of this PR that isn't targeted at backporting.TODO only if you're a HashiCorp employee
to N, N-1, and N-2, using the
backport/ent/x.x.x+entlabels. If this PR is in the CE repo, you should only backport to N, using thebackport/x.x.xlabel, not the enterprise labels.in the PR description, commit message, or branch name.