Use Privacy Access Tokens, CAPTCHA, and Rack to rate limit sign ins and sign ups #3519

mercedesb · 2023-03-02T21:55:35Z

Resolves #3518

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

Please see #3518

Rubygems.org wants to reduce (and prevent if possible) bot activity around signup and login. Captchas are a common way to reduce activity, but can also be annoying and repetitive. We’d like to use Privacy Pass to get the same security with a smoother user experience then Captcha, although it doesn’t completely replace Captchas.

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

Following the product design in the linked issue, I've implemented privacy pass token challenges and redemption, hcaptcha verification, and stricter rate limiting around sign in and sign up.

To test

Privacy pass is currently supported on iOS and MacOS Ventura. I tested this with Safari in MacOS Ventura 13.2.1 (it doesn't work in any non-Apple browser yet as far as I can tell).

Deployment next steps

Once we hear back from Fastly about getting added to their beta issuer we can take the following steps

Uncomment the PRIVACY_PASS_ISSUER_PUBLIC_KEY and PRIVACY_PASS_ISSUER_NAME vars from config/deploy/unicorn.yaml.erb
Add the var values to config/deploy/staging/secrets.ejson and config/deploy/production/secrets.ejson

Other helpful info

Apple has the best info currently about how to set up your web server to serve privacy pass challenges and redeem privacy pass tokens.

The various docs around the IETF RFC are also helpful

hcaptcha was straightforward to get set up and working. Their docs are pretty good.

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

codecov · 2023-03-02T22:07:27Z

Codecov Report

Merging #3519 (2619a61) into master (ebf32fe) will decrease coverage by 0.05%.
The diff coverage is 97.81%.

@@            Coverage Diff             @@
##           master    #3519      +/-   ##
==========================================
- Coverage   98.69%   98.65%   -0.05%     
==========================================
  Files         198      204       +6     
  Lines        4902     5120     +218     
==========================================
+ Hits         4838     5051     +213     
- Misses         64       69       +5

Impacted Files	Coverage Δ
lib/hcaptcha_verifier.rb	`90.00% <90.00%> (ø)`
app/controllers/users_controller.rb	`97.50% <96.87%> (-2.50%)`	⬇️
app/controllers/concerns/captcha_verifiable.rb	`100.00% <100.00%> (ø)`
...p/controllers/concerns/privacy_pass_supportable.rb	`100.00% <100.00%> (ø)`
app/controllers/sessions_controller.rb	`100.00% <100.00%> (ø)`
app/helpers/hcaptcha_helper.rb	`100.00% <100.00%> (ø)`
lib/privacy_pass_redeemer.rb	`100.00% <100.00%> (ø)`
lib/privacy_pass_tokenizer.rb	`100.00% <100.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

segiddins · 2023-03-02T22:43:48Z

config/routes.rb

@@ -209,7 +210,11 @@
      end
    end

-    get '/sign_in' => 'clearance/sessions#new', as: 'sign_in'
+    resource :user, only: [] do


is resource only: [] any different from using namespace ?

Good question, I was following the pattern that currently exists in routes for session and I think in this case it makes more sense. /user isn't a namespace but a singular resource. Namespacing it would mean that by convention we would expect the controller to exist within a user module.

From the Rails docs

2.5 Singular Resources
Sometimes, you have a resource that clients always look up without referencing an ID. ..
2.6 Controller Namespaces and Routing
You may wish to organize groups of controllers under a namespace. Most commonly, you might group a number of administrative controllers under an Admin:: namespace, and place these controllers under the app/controllers/admin directory. You can route to such a group by using a namespace block:

app/controllers/sessions_controller.rb

app/controllers/users_controller.rb

lib/hcaptcha_verifier.rb

segiddins · 2023-03-02T22:54:17Z

lib/privacy_pass_redeemer.rb

+  #   uint8_t token_key_id[32];
+  #   uint8_t authenticator[Nk];
+  # } Token;
+  TOKEN_TYPE_LENGTH = 1


should this be 2 since it's a uint16_t as opposed to a uint8?

No, because we're checking the length of unpacked data and we're unpacking this via the uint16 directive (n)

lib/privacy_pass_redeemer.rb

lib/privacy_pass_tokenizer.rb

indirect · 2023-03-07T01:13:16Z

Fastly says we are in their beta access group now, and they are targeting end of Q2 for the beta issuer. In the meantime, I would be okay to ship this with just captchas now, and add privacy pass support once there's a production issuer. (Fastly says the demo issuer that we built this to work with is not for production use, so we shouldn't ship anything that depends on that.)

mercedesb · 2023-03-16T22:26:09Z

In the meantime, I would be okay to ship this with just captchas now, and add privacy pass support once there's a production issuer.

Based on @indirect's most recent feedback, I went ahead and feature flagged the privacy pass implementation using LaunchDarkly (someone with access to that will need to configure the variation)

segiddins · 2023-03-16T22:32:13Z

app/controllers/concerns/privacy_pass_supportable.rb

+
+  def privacy_pass_enabled?
+    ld_context = LaunchDarkly::LDContext.with_key(self.class.name)
+    Rails.configuration.launch_darkly_client.variation("privacy_pass.enabled", ld_context, false)


nit: let's call the flag "gemcutter.privacy_pass.enabled". should we use the current controller & action as the key, so we can test by only turning it on for a single action, rather than every action?

I updated the name.

I think using the current controller would be good. My only concern about using the action is that for every place we want it (sign up or sign in) we actually need it enabled in 2 actions. The new action presents the challenge and the create action handles redeeming the token.

fair enough! I just think we'll need a kind on the LDContext of say controller so it doesn't look like a user

simi · 2023-03-17T07:41:20Z

app/views/sessions/captcha.html.erb

+<div class="mfa__container">
+  <div class="mfa__option">
+    <%= form_tag captcha_create_session_path, method: :post, class: "mfa-form" do %>
+      <div class="h-captcha" data-sitekey="<%= ENV.fetch("HCAPTCHA_SITE_KEY", "10000000-ffff-ffff-ffff-000000000001") %>"></div>


Can you move HCAPTCHA_SITE_KEY to be ingested in config/secrets.yml from ENV (and specify this default for dev/testing environment if useful), introduce helper like hcaptcha_site_key to read secrets and use it in templates?

simi · 2023-03-17T07:50:53Z

lib/hcaptcha_verifier.rb

+        sitekey: ENV.fetch("HCAPTCHA_SITE_KEY", "10000000-ffff-ffff-ffff-000000000001")
+      }
+
+      response = RestClient.post VERIFY_ENDPOINT,


RestClient was replaced by Faraday recently at #3602

simi · 2023-03-17T08:20:52Z

app/controllers/sessions_controller.rb

@@ -172,6 +194,16 @@ def setup_mfa_authentication
    session[:mfa_user] = @user.id
  end

+  def setup_captcha_verification
+    create_catpcha_user(id: @user.id)


Per my local testing, captcha is also created when trying to login with unknown email, that means @user is nil in here and it crashes.

The same seems applied when using wrong password for existing account. find_user returns nil -> @user is nil.

simi · 2023-03-17T08:26:02Z

I did some testing and left some initial comments. Regarding ENV values, please move them into config/secrets.yml (could be ingested from ENV still) or to config/rubygems.yml for fixed values.

Here's a screenshot from testing of captcha. The red text is just notice of using testing environment. It looks good to me.

mercedesb · 2023-03-17T15:46:05Z

put hcpatcha vals into secrets with testing creds as dev defaults
replaced RestClient with Faraday
fixed bug when user not found on login but captcha conditions met

mercedesb · 2023-03-17T15:52:16Z

I got an email from GitGuardian about an exposed generic password. But I think that's ok b/c Hcaptcha publicly publishes this site key and secret for development testing

simi · 2023-03-19T07:23:58Z

lib/privacy_pass_tokenizer.rb

+    # fastly demo fallback
+    # https://www.fastly.com/blog/private-access-tokens-stepping-into-the-privacy-respecting-captcha-less#demo-configuration
+    # rubocop:disable Layout/LineLength
+    ENV.fetch("PRIVACY_PASS_ISSUER_PUBLIC_KEY",


Would it make sense to move this also to rubygems.yml/secrets.yml?

I can. It's not really a secret but no harm in adding it there.

simi · 2023-03-19T08:38:45Z

app/controllers/concerns/privacy_pass_supportable.rb

+
+  def valid_privacy_pass_redemption?
+    return false unless privacy_pass_enabled?
+    return session[:redeemed_privacy_pass] unless session[:redeemed_privacy_pass].nil?


I found out this a little hard to read during testing. Would it be possible to keep the condition "positive"?

return session[:redeemed_privacy_pass] if session.key?(:redeemed_privacy_pass)

simi · 2023-03-19T08:48:48Z

@mercedesb any more hints for local Privacy pass testing? I tried with Privacy pass extension, but I couldn't get it to work 🤔.

mercedesb · 2023-03-19T14:34:25Z

any more hints for local Privacy pass testing? I tried with Privacy pass extension, but I couldn't get it to work

We're using the Fastly issuer and Fastly does not work with the Privacy Pass extension. That extension is a Cloudflare product.
I never had much luck testing with the extension anywhere on the web and neither did a colleague of mine. They have a lot of open issues in their client repo with people having similar trouble and I don't have a lot of confidence in how well-supported the extension is.

Privacy Pass is currently supported natively only in iOS 16+, iPad 16+, macOS Ventura (13+). Through my testing, I had to use the Safari browser. But if you're running a supported OS with Safari, that should be all you need for testing.

If things are working as expected, you won't see the 401 response with the token challenge in the browser's dev tools' Network tab. You'll see a 200 with an Authorization header from the client which means the challenge was handled successfully. But you can step through the code to see the entire flow.

simi · 2023-03-19T17:07:32Z

That extension supports also hCaptcha and seems active on hCaptcha rubygems.org page, but I couldn't get it to work as well. I don't have any Apple product around to test this.

mercedesb · 2023-03-19T17:17:34Z

That extension supports also hCaptcha and seems active on hCaptcha rubygems.org page

hcaptcha is a different issuer and won't work with Fastly tokens. We are using the Fastly issuer.

I don't have any Apple product around to test this.

That's a bummer. Given that Apple seems to be the only native support for PATs at the moment, I do think it important that it's tested there.

simi · 2023-03-19T17:20:51Z

app/controllers/users_controller.rb

+    elsif @user.save
+      handle_user_after_save
+    else
+      render template: "users/new"


If I understand it well, this is tricky to test. valid? is called few lines before, but this save can still fail. In theory codecov could be fixed by using save! and rescue_from, but I'm not sure if that's worth it. 🤔

simi · 2023-03-19T17:23:59Z

That's a bummer. Given that Apple seems to be the only native support for PATs at the moment, I do think it important that it's tested there.

We can ask someone for help with testing on staging.

Other than that part I can't test, this looks good to me. 💪

simi · 2023-03-19T17:26:31Z

lib/hcaptcha_verifier.rb

+      should_verify(keys, SIGN_UP_CAPTCHA_THRESHOLD)
+    end
+
+    def call(client_response, remote_ip)


nitpick: I'm personally not big fan of call methods if not used as a proc. Would it make sense to rename this to something like verified?. Similar change could be done in PrivacyPassRedeemer.

simi · 2023-03-19T17:28:49Z

lib/hcaptcha_verifier.rb

+        f.request :url_encoded
+        f.response :json
+        f.response :logger, logger, headers: false, errors: true
+        f.response :raise_error


Is this codecov complain related to stubs being used in tests instead of using test adapter? 🤔

https://lostisland.github.io/faraday/adapters/testing

segiddins reviewed Mar 2, 2023

View reviewed changes

mercedesb force-pushed the mb-rate-limit-sign-ups branch from a41be42 to 9527095 Compare March 3, 2023 01:59

mercedesb requested a review from segiddins March 3, 2023 14:46

mercedesb changed the title ~~Use Privacy Pass, captcha, and Rack to rate limit sign ins and sign ups~~ Use Privacy Access Tokens, CAPTCHA, and Rack to rate limit sign ins and sign ups Mar 3, 2023

mercedesb force-pushed the mb-rate-limit-sign-ups branch 2 times, most recently from eca0839 to 3f6cce8 Compare March 3, 2023 22:10

simi self-requested a review March 7, 2023 01:14

mercedesb force-pushed the mb-rate-limit-sign-ups branch from 3f6cce8 to 4f44b20 Compare March 16, 2023 22:25

segiddins reviewed Mar 16, 2023

View reviewed changes

mercedesb force-pushed the mb-rate-limit-sign-ups branch from 95e9733 to 3293a22 Compare March 16, 2023 22:45

simi reviewed Mar 17, 2023

View reviewed changes

mercedesb force-pushed the mb-rate-limit-sign-ups branch from 6b3c3b4 to e7b8f9f Compare March 17, 2023 15:45

simi reviewed Mar 19, 2023

View reviewed changes

mercedesb added 6 commits March 19, 2023 09:40

throttle login by user, no more than 15 per hour

c336047

throttle sign up by ip, no more than 50 per hour

5962b58

create const to expose rack login cache key

96ed264

add content security policy config for hcaptcha

c101b7b

add i18n for captcha

e520d8a

add service class to handle hcaptcha verification

46c123c

mercedesb added 22 commits March 19, 2023 09:40

tokenizer and redeemer unit tests

39f5277

integration test privacy pass sign in and sign up flows

7c2cadc

handle nil auth header

f215648

cleanup comments

9abaaef

lint fix

20cecb9

fix base64 string

54eb409

resolve rack test now that captcha is added

494769e

resolve PR feedback

508b0b2

add env vars to unicorn k8s container definition

6dd4474

add hcaptcha secrets

3cc8e6d

clean up concerns

7208230

feature flag privacy pass

72bc324

lint fix

71880ce

rename LD variation

1b89db2

lint fix

da8d566

add context kind

007936d

add hcaptcha vals to secrets

8b7384f

use Faraday instead of RestClient

d7c709b

fix bug when user not found on signing in and captcha conditions met

cffedcc

fix tests

50a120f

add privacy pass issuer public key to secrets

a984568

flip negative conditional

2619a61

mercedesb force-pushed the mb-rate-limit-sign-ups branch from 657c299 to 2619a61 Compare March 19, 2023 14:40

simi reviewed Mar 19, 2023

View reviewed changes

simi approved these changes Mar 19, 2023

View reviewed changes

simi reviewed Mar 19, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Privacy Access Tokens, CAPTCHA, and Rack to rate limit sign ins and sign ups #3519

Use Privacy Access Tokens, CAPTCHA, and Rack to rate limit sign ins and sign ups #3519

mercedesb commented Mar 2, 2023 •

edited

Loading

codecov bot commented Mar 2, 2023 •

edited

Loading

segiddins Mar 2, 2023

mercedesb Mar 3, 2023

segiddins Mar 2, 2023

mercedesb Mar 3, 2023 •

edited

Loading

indirect commented Mar 7, 2023

mercedesb commented Mar 16, 2023

segiddins Mar 16, 2023

mercedesb Mar 16, 2023

segiddins Mar 16, 2023

simi Mar 17, 2023

simi Mar 17, 2023

simi Mar 17, 2023

simi Mar 17, 2023

simi commented Mar 17, 2023

mercedesb commented Mar 17, 2023

mercedesb commented Mar 17, 2023

simi Mar 19, 2023

mercedesb Mar 19, 2023

simi Mar 19, 2023

simi commented Mar 19, 2023

mercedesb commented Mar 19, 2023

simi commented Mar 19, 2023

mercedesb commented Mar 19, 2023

simi Mar 19, 2023

simi commented Mar 19, 2023

simi Mar 19, 2023

simi Mar 19, 2023

Use Privacy Access Tokens, CAPTCHA, and Rack to rate limit sign ins and sign ups #3519

Are you sure you want to change the base?

Use Privacy Access Tokens, CAPTCHA, and Rack to rate limit sign ins and sign ups #3519

Conversation

mercedesb commented Mar 2, 2023 • edited Loading

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

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

To test

Deployment next steps

Other helpful info

Make sure the following tasks are checked

codecov bot commented Mar 2, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mercedesb Mar 3, 2023 • edited Loading

Choose a reason for hiding this comment

indirect commented Mar 7, 2023

mercedesb commented Mar 16, 2023

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

simi commented Mar 17, 2023

mercedesb commented Mar 17, 2023

mercedesb commented Mar 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simi commented Mar 19, 2023

mercedesb commented Mar 19, 2023

simi commented Mar 19, 2023

mercedesb commented Mar 19, 2023

Choose a reason for hiding this comment

simi commented Mar 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mercedesb commented Mar 2, 2023 •

edited

Loading

codecov bot commented Mar 2, 2023 •

edited

Loading

mercedesb Mar 3, 2023 •

edited

Loading