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.

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

Fix Okta WebAuthn challenge #1059

Merged
merged 2 commits into from
Jun 16, 2023
Merged

Fix Okta WebAuthn challenge #1059

merged 2 commits into from
Jun 16, 2023

Conversation

anothertobi
Copy link
Contributor

PR #1039 introduced a change to okta_webauthn.go that most probably
is a relic from the development phase of the Duo integration (ref:
82b3ba3).

Since the Duo integration implements its own ChallengeU2F, this should
be safe to revert.

@anothertobi
Copy link
Contributor Author

Verified that this fixes Okta + WebAuthn using a YubiKey. Would be great if someone with Okta + Duo could double-check that this indeed doesn't break their setup (maybe @lizduty / @6f6d6172 ?).

@smlx
Copy link
Contributor

smlx commented May 23, 2023

I can confirm this fixes Okta U2F authentication for me.

@satyamab
Copy link

Thank you for fixing this! Also confirming that this fixes Okta U2F authentication for me. The impact is quite widespread for our eng team. Any idea when a release may be available for this fix? 🙏

@dasrecht
Copy link

Can also confirm this fix works as expected - Thanks @anothertobi!

@lizduty
Copy link
Contributor

lizduty commented May 25, 2023 via email

@smlx
Copy link
Contributor

smlx commented Jun 1, 2023

@anothertobi any chance you could take a look at the test failure? 🙂

@codecov-commenter
Copy link

codecov-commenter commented Jun 2, 2023

Codecov Report

Merging #1059 (f4ecfc5) into master (83489ec) will not change coverage.
The diff coverage is 100.00%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@           Coverage Diff           @@
##           master    #1059   +/-   ##
=======================================
  Coverage   36.29%   36.29%           
=======================================
  Files          53       53           
  Lines        7941     7941           
=======================================
  Hits         2882     2882           
  Misses       4686     4686           
  Partials      373      373           
Flag Coverage Δ
unittests 36.29% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/provider/okta/okta_webauthn.go 54.02% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@anothertobi
Copy link
Contributor Author

@anothertobi any chance you could take a look at the test failure? 🙂

68806fe seems to have fixed the build, but the linter is timing out. Locally this runs through in < 1s 🤔

@anothertobi anothertobi mentioned this pull request Jun 6, 2023
PR #1039 introduced a change to okta_webauthn.go that most probably
is a relic from the development phase of the Duo integration (ref:
82b3ba3).

Since the Duo integration implements its own ChallengeU2F, this should
be safe to revert.
@anothertobi
Copy link
Contributor Author

Rebased the branch and #1074 did fix the golangci-lint timeout we had earlier.

@roptoor
Copy link

roptoor commented Jun 15, 2023

Curious what more is needed to get this merged, as it seems to be affecting quite a few users?

@mapkon mapkon merged commit f8af25e into Versent:master Jun 16, 2023
@anothertobi anothertobi deleted the fix/okta-webauthn branch June 16, 2023 07:02
@jmandel1027
Copy link

Heyyo, would it be possible to cut a new release with this patch? It looks like this is still affecting a large number of folks in our eng org too

@mapkon
Copy link
Contributor

mapkon commented Jun 30, 2023

Yeah, we would need to test the current code for release readiness. If you volunteer to coordinate that, I would very much appreciate it

@szechyjs
Copy link
Contributor

Any ETA on getting this released. We are dead in the water without this.

@anothertobi
Copy link
Contributor Author

@szechyjs this PR is included in the v2.36.10 release.

@szechyjs
Copy link
Contributor

szechyjs commented Jul 20, 2023

@szechyjs this PR is included in the v2.36.10 release.

Good to hear. I didn't see it in the Changelog/Release Notes which is why I asked.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants