Add poller to fetch WebAuthn OTP #6774

jenshenny · 2023-06-27T01:01:06Z

What was the end-user or developer problem that led to this PR?

Fixes: rubygems/rubygems.org#3826
Discussed in the issue, we decided to rely on a polling solution for Safari users

RubyGems.org PR: rubygems/rubygems.org#3873

What is your fix for the problem, implemented in this PR?

If the user is using Safari, instead of redirecting the user to localhost after successful webauthn verification in the browser, we'll show the success page (since the auth on the browser was successful).

On the CLI, a poller is added that would poll /api/v1/webauthn_verification/<token>/status every 5 seconds.

Since we can't be sure which browser the user will be using (finding the default browser won't be fool proof since a user can paste the link into any browser), so we'll have a socket and poller thread open.

Whichever thread terminates successfully with the otp will be used. This is done via a loop checking the status of the two threads.

Make sure the following tasks are checked

Describe the problem / feature
Write tests for features and bug fixes
Write code to solve the problem
Make sure you follow the current code style and write meaningful commit messages without tags

Testing

Use this RubyGems.org branch Add polling fallback to Safari browsers rubygems.org#3873
Have a Webauthn credential associated with your user.
Run RUBYGEMS_HOST=http://localhost:3000 ruby -Ilib exe/gem signin and webauthn verify using Safari

jenshenny · 2023-06-27T01:04:59Z

test/rubygems/test_gem_gemcutter_utilities.rb

@@ -345,4 +348,75 @@ def test_verify_missing_api_key
      @cmd.verify_api_key :missing
    end
  end
+
+  class SignInFetcher < Gem::FakeFetcher


util_gem_signin was getting too gnarly to add fetcher data so I extracted a helper class to setup the fetcher. After writing the rest of the tests for the other commands, I'm intending to follow up with another refactor to have some sort of a universal child fetcher for webauthn verifications since they all stub the same requests.

Done in f49fb73

jenshenny · 2023-06-27T01:06:22Z

lib/rubygems/gemcutter_utilities.rb

+  def wait_for_otp(*threads)
+    loop do
+      threads.each do |otp_thread|
+        next unless !otp_thread.alive?
+        if otp = otp_thread[:otp]
+          return otp
+        elsif error = otp_thread[:error]
+          alert_error error.message
+          terminate_interaction(1)
+        end
+      end
+      sleep 0.1
+    end
+  ensure
+    threads.each(&:exit)
+  end


Is there a better way to terminate once the poller or the listener succeeds?

jenshenny · 2023-06-29T16:23:51Z

lib/rubygems/gemcutter_utilities.rb

+      Timeout.timeout(300) do
+        loop do
+          response = webauthn_verification_poll_response(webauthn_url, credentials)
+          raise Gem::WebauthnVerificationError, response.message unless response.is_a?(Net::HTTPSuccess)
+
+          require "json"
+          parsed_response = JSON.parse(response.body)
+          case parsed_response["status"]
+          when "pending"
+            sleep 5
+          when "success"
+            Thread.current[:otp] = parsed_response["code"]
+            break
+          else
+            raise Gem::WebauthnVerificationError, parsed_response["message"]
+          end
+        end


Thinking of extracting this logic out into a class as a follow up

Why in followup?

Mostly to get the current functionality out as soon as possible and wanted to get people's thoughts on it. If you feel that extracting would be good to do in this PR, I can do that 👍

commit to extract the logic to a class 78a8b8a

I also moved the webauthn listener class to the gemcutter utilities namespace fd92d0d

ericherscovich · 2023-07-17T15:42:59Z

lib/rubygems/gemcutter_utilities.rb

+      url_with_port = "#{webauthn_url}?port=#{port}"
+      say "You have enabled multi-factor authentication. Please visit #{url_with_port} to authenticate via security device. If you can't verify using WebAuthn but have OTP enabled, you can re-run the gem signin command with the `--otp [your_code]` option."
+
+      threads = [WebauthnListener.listener_thread(host, server), WebauthnPoller.poll_thread(options, host, webauthn_url, credentials)]


If polling works for both Safari and any other browser, why not just use the polling alone instead?

Good question, I toiled with this a bit. The original reason sockets were chosen was that the connection between the browser and the CLI cannot be phished + responses would be immediate. With polling, while very limited, there's still a risk that someone else can retrieve an OTP code if they have access to credentials + your unique link while also having up to a 5 second delay. With those reasons, I still think it would be worth to have polling as just a fallback.

Yeah, I agree that the socket is preferred, and if Safari ever lets us remove polling, we should.

ericherscovich · 2023-07-17T15:44:48Z

lib/rubygems/gemcutter_utilities.rb

+  def wait_for_otp_thread(*threads)
+    loop do
+      threads.each do |otp_thread|
+        return otp_thread unless otp_thread.alive?
+      end
+      sleep 0.1


Is it perhaps worth adding a timeout on these threads where if they're left hanging for a couple minutes you kill the threads?

I have added a 5 minute timeout for the poller since we don't want to hit the API indefinitely https://github.com/Shopify/rubygems/blob/polling-safari-fallback/lib/rubygems/gemcutter_utilities/webauthn_poller.rb#L47

Once 5 min is up, that thread would terminate safely which would trigger the other threads to terminate as well. Let me know if you think it makes more sense to move the timeout to this loop instead.

ericherscovich · 2023-07-17T15:48:51Z

lib/rubygems/gemcutter_utilities/webauthn_listener/response.rb

+      end
+
+      def body
+        "success"


Is it worth adding a bit more context here? I.e., "Success. You can return back to your terminal."

The body isn't user facing but more of a status for the application to use to determine if verification was successful. Modifying the body would be a breaking change

https://github.com/rubygems/rubygems.org/blob/2f0f168e1a2bdd95cb7c548599fb498dfc48085c/app/assets/javascripts/webauthn.js#L138

jenshenny · 2023-07-18T19:31:18Z

Is there anything blocking this from moving forward? cc: @indirect @simi @segiddins

indirect · 2023-07-18T21:32:32Z

Nothing from me!

simi · 2023-07-18T21:47:52Z

lib/rubygems/gemcutter_utilities/webauthn_poller.rb

+          response = webauthn_verification_poll_response(webauthn_url, credentials)
+          raise Gem::WebauthnVerificationError, response.message unless response.is_a?(Net::HTTPSuccess)
+
+          require "json"


@deivid-rodriguez is it safe in here to require JSON? I did some experiment and we can also vendor pure Ruby JSON if needed. It is possible to also use YAML in API, but I'm not sure that will make any difference in here.

My understanding is that this is only used for gem push, gem yank, and gem owner. If that's the case I think it's fine, at least for now, since there's no user code involved in these commands, so there should not be any conflicts.

jenshenny · 2023-07-19T14:45:09Z

Thanks folks for taking a look at this!

Hmm after rebasing, the tests appear to be a bit flaky now with the changes 🤔
https://github.com/rubygems/rubygems/actions/runs/5600005188/jobs/10241758022
https://github.com/rubygems/rubygems/actions/runs/5600328937/jobs/10242557190

Going to dig into this before this gets merged in.

to prevent real TCPServers from being created and be leaked into other tests

jenshenny · 2023-07-20T19:03:22Z

So to deal with the possibly flaky nature of using real TCPServers to test the commands (signin, owner add/remove, yank), I created a mock server object to mock that logic. I say possibly flaky since running ruby-core on the same test seed passes + I couldn't find a way to reproduce the errors.

I think it's better to mock the server as we aren't testing the WebAuthn listener logic (which interacts the TCPServer instance) in those tests but more on the output (we are stubbing the result of the WebAuthn listener anyways). We are still using real TCPServer instances on the Webauthn Listener tests to test requests coming in and out of the socket as realistically as possible.

Let me know if you all think differently! Open to other suggestions.

indirect · 2023-07-20T22:20:28Z

That sounds like a totally reasonable approach to me. 👍🏻

deivid-rodriguez · 2023-07-21T18:09:24Z

Happy to merge and release this whenever needed 👍.

jenshenny · 2023-07-21T19:52:49Z

Thanks @deivid-rodriguez 🙌 I tested everything again and it all works well. If this can be a part of the next rubygems release, that would be perfect!

lib/rubygems/gemcutter_utilities/webauthn_listener.rb

lib/rubygems/gemcutter_utilities/webauthn_listener/response.rb

lib/rubygems/gemcutter_utilities/webauthn_poller.rb

Add poller to fetch WebAuthn OTP (cherry picked from commit 736f447)

jenshenny commented Jun 27, 2023

View reviewed changes

jenshenny mentioned this pull request Jun 27, 2023

Add polling fallback to Safari browsers rubygems/rubygems.org#3873

Merged

jenshenny force-pushed the polling-safari-fallback branch 6 times, most recently from 90fbc97 to 8047416 Compare June 28, 2023 20:21

jenshenny marked this pull request as ready for review June 28, 2023 21:23

jenshenny force-pushed the polling-safari-fallback branch from 18db2cd to dc9cdf7 Compare June 29, 2023 16:04

jenshenny commented Jun 29, 2023

View reviewed changes

jenshenny mentioned this pull request Jun 29, 2023

Add WebAuthn CLI fallback for Safari browsers rubygems/rubygems.org#3826

Closed

jenshenny force-pushed the polling-safari-fallback branch from 3856479 to 33ca40e Compare June 29, 2023 19:55

jenshenny marked this pull request as draft July 1, 2023 02:38

jenshenny force-pushed the polling-safari-fallback branch 3 times, most recently from 762e62e to 57ad484 Compare July 1, 2023 04:10

jenshenny force-pushed the polling-safari-fallback branch from aabb69f to f49fb73 Compare July 11, 2023 04:46

jenshenny marked this pull request as ready for review July 11, 2023 04:51

jenshenny force-pushed the polling-safari-fallback branch from f49fb73 to 2c4afbb Compare July 11, 2023 04:52

jenshenny requested a review from simi July 11, 2023 13:27

This was referenced Jul 11, 2023

Show webauthn warning if default browser is Safari #6605

Closed

Remove no Safari Webauthn CLI support warning rubygems/guides#338

Merged

ericherscovich reviewed Jul 17, 2023

View reviewed changes

indirect approved these changes Jul 18, 2023

View reviewed changes

simi approved these changes Jul 18, 2023

View reviewed changes

simi requested a review from deivid-rodriguez July 18, 2023 21:38

simi reviewed Jul 18, 2023

View reviewed changes

jenshenny added 7 commits July 19, 2023 09:48

Create SignInFetcher

38afc47

Remove fetcher login in util_sign_in

8e6bc44

Add Webauthn verification poller to fetch OTP

39c5e86

Extract polling logic into its own class

218b83a

Move WebauthnListener into the Gem::GemcutterUtilities namespace

3080394

Move Webauthn listener thread to WebauthnListener class

6ec4749

Use assert_raise in webauthn poller tests

0969ad3

jenshenny force-pushed the polling-safari-fallback branch from 2c4afbb to 4eab352 Compare July 19, 2023 13:50

Create MultifactorAuthFetcher to reduce duplication among tests

dead211

jenshenny force-pushed the polling-safari-fallback branch from 4eab352 to dead211 Compare July 19, 2023 14:19

Create MockServer object to test WebAuthn logic

96d6cb3

to prevent real TCPServers from being created and be leaked into other tests

jenshenny force-pushed the polling-safari-fallback branch from 548f3ca to 96d6cb3 Compare July 20, 2023 18:24

jenshenny mentioned this pull request Jul 20, 2023

Decoupling MFA level and Devices rubygems/rubygems.org#3800

Closed

segiddins approved these changes Jul 23, 2023

View reviewed changes

lib/rubygems/gemcutter_utilities/webauthn_listener.rb Outdated Show resolved Hide resolved

lib/rubygems/gemcutter_utilities/webauthn_listener/response.rb Outdated Show resolved Hide resolved

lib/rubygems/gemcutter_utilities/webauthn_poller.rb Outdated Show resolved Hide resolved

jenshenny added 2 commits July 23, 2023 19:18

Set thread configurations inside block

860b145

Add charset to Webauthn response content-type

442a3e8

deivid-rodriguez added the rubygems: enhancement label Jul 28, 2023

deivid-rodriguez merged commit 736f447 into rubygems:master Jul 28, 2023
83 checks passed

jenshenny deleted the polling-safari-fallback branch July 28, 2023 17:17

jenshenny mentioned this pull request Jul 29, 2023

Revert set thread configurations inside block in WebAuthn poller and listener #6841

Closed

4 tasks

deivid-rodriguez added a commit that referenced this pull request Jul 31, 2023

Merge pull request #6774 from Shopify/polling-safari-fallback

3bdee36

Add poller to fetch WebAuthn OTP (cherry picked from commit 736f447)

jenshenny mentioned this pull request Aug 3, 2023

Add webauthn Banner, Move webauthn above TOTP rubygems/rubygems.org#3945

Merged

deivid-rodriguez mentioned this pull request Aug 7, 2023

Add RubyGems Monthly Update for July 2023 rubygems/rubygems.github.io#165

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add poller to fetch WebAuthn OTP #6774

Add poller to fetch WebAuthn OTP #6774

jenshenny commented Jun 27, 2023

jenshenny Jun 27, 2023 •

edited

jenshenny Jul 11, 2023

jenshenny Jun 27, 2023

jenshenny Jun 29, 2023

simi Jun 29, 2023

jenshenny Jun 29, 2023

jenshenny Jul 11, 2023

ericherscovich Jul 17, 2023

jenshenny Jul 17, 2023

indirect Jul 17, 2023

ericherscovich Jul 17, 2023

jenshenny Jul 17, 2023 •

edited

ericherscovich Jul 17, 2023

jenshenny Jul 17, 2023

jenshenny commented Jul 18, 2023

indirect commented Jul 18, 2023

simi Jul 18, 2023

deivid-rodriguez Jul 19, 2023

jenshenny commented Jul 19, 2023

jenshenny commented Jul 20, 2023

indirect commented Jul 20, 2023

deivid-rodriguez commented Jul 21, 2023

jenshenny commented Jul 21, 2023

Add poller to fetch WebAuthn OTP #6774

Add poller to fetch WebAuthn OTP #6774

Conversation

jenshenny commented Jun 27, 2023

What was the end-user or developer problem that led to this PR?

What is your fix for the problem, implemented in this PR?

Make sure the following tasks are checked

Testing

jenshenny Jun 27, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jenshenny Jul 17, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jenshenny commented Jul 18, 2023

indirect commented Jul 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jenshenny commented Jul 19, 2023

jenshenny commented Jul 20, 2023

indirect commented Jul 20, 2023

deivid-rodriguez commented Jul 21, 2023

jenshenny commented Jul 21, 2023

jenshenny Jun 27, 2023 •

edited

jenshenny Jul 17, 2023 •

edited