Add keybase integration #661

devsnek · 2019-04-15T15:42:18Z

This allows users to verify keybase with their account.

Refs: https://keybase.io/docs/proof_integration_guide

This is currently untested as I lack the infra needed to test lobster, and some SVG assets of the logo are needed for keybase.json. If anyone can help with these that would be nice. After this is deployed, an admin of the site will need to submit keybase.json to keybase.

This allows users to verify keybase with their account. Refs: https://keybase.io/docs/proof_integration_guide

pushcx · 2019-04-17T14:01:15Z

I've posted a story to discuss this as I'm not sure what the value is here and, more generally, when an auth integration is worth it.

malgorithms · 2019-04-18T14:23:23Z

hi, @pushcx - someone asked me to comment here; I posted on the story instead of GitHub, but here's a link to it: https://lobste.rs/s/24k19n/should_we_support_authing_keybase#c_8s4fex

I don't know if it's better to discuss the PR here or there, but people from Keybase can be around for technical discussion if here. We don't have a particular attachment to this PR but would of course be quite happy to see lobste.rs as one of our first non-mastodon and non-commercial integrations.

https://lobste.rs/u/cc

pushcx · 2019-04-24T13:01:41Z

That discussion has wound down and we got a bunch of folks who'd like to see it, so let's give a shot.(And, like the other integrations, someone besides me has done all the hard work.) Let me take a look at the spec and review the code here...

pushcx · 2019-04-24T13:03:16Z

app/controllers/settings_controller.rb

@@ -256,6 +256,25 @@ def twitter_disconnect
    return redirect_to "/settings"
  end

+  def keybase_auth
+    if !params[:kb_username].present? || !params[:kb_signature].present? || !params[:kb_ua].present? || (!params[:username].present? || !params[:username] == @user.username)


Can Rails's strong parameters clean this up? You may need to make like a non-db-backed model object named KeybaseAuth or something for the params to live on... and that would have the nice effect of pulling much of this complexity out of the controller.

I'll be honest I have no idea what any of this means, can you elaborate?

It's the Rails security feature for saying "I expect this form to give all these fields, and only these fields": https://guides.rubyonrails.org/action_controller_overview.html#strong-parameters

for any users with multiple lobsters accounts, it might be nice to have the !params[:username] == @user.username error case handled a little differently. i.e. tell them that they're logged in as the wrong user. otherwise it could be pretty confusing what's actually going wrong.

Users are not permitted to have more than one account, it's considered sockpuppeting and leads to banning all but one account, at the least.

I don't try to actively seek it out because I'm ok with (hypothetical scenario here) an Apple employee who wants to gripe about how they don't like Swift without getting in trouble at work, but it's a regular problem with self-promoters who vote up their own links and comments.

pushcx · 2019-04-24T13:04:28Z

app/controllers/settings_controller.rb

+      @user.keybase_signatures = [{ kb_username: kb_username, sig_hash: kb_signature }]
+      return redirect_to Keybase.success_url(kb_username, kb_signature, kb_ua, @user.username)
+    else
+      flash[:error] = "Failed to connect your account to Keybase, invalid proof signature was provided."


What's next step can we give the user here? Should they be contacting Keybase about their bug, or mailing the site admin about our own bug?

it's more of a 4xx than a 5xx, since invalid auth data was provided

Can the user fix it? If they can, let's tell them what to change. If it's something in our json config that's borked, tell them to email the admin.

it's likely that the only situation where the user can do anything is if they're logged in as a different user than the one they're trying to prove. other than that, it's (1) a problem in the request being made by lobsters (maybe catch this with logging and general observability), (2) an error in keybase (not much we can do about it here i'm sorry to say), (3) the user is accidentally trying to prove something that will never work (maybe a notification that they should try again from keybase), (4) the user is purposefully trying to prove something that will never work (fuck 'em).

pushcx · 2019-04-24T13:08:23Z

app/models/user.rb

@@ -67,6 +67,7 @@ class User < ApplicationRecord
    s.string :twitter_oauth_token
    s.string :twitter_oauth_token_secret
    s.string :twitter_username
+    s.array :keybase_signatures


Suggested change

s.array :keybase_signatures

s.string :keybase_signatures, array: true, default: [], null: false

I'm not super-familiar with typedstore so it needs a doublecheck, but I'd prefer to both prohibit nil and be explicit about the type of the elements in the array.

From closer reading above I see this isn't an array of strings, it's an array of hashes of symbols => strings... so, yeah, please fix. And please add a test (not roundtripping Keybase, just that we can store + retrieve credentials): some sister sites use postgresql (and I daydream about it...). There's a warning in the typedstore readme about its support for arrays, so at least we could give them a failing test to point directly to the problem.

I've never used ruby before, can you elaborate on what I need to do here?

I don't know typedstore well enough; I'd just be googling for an example. It's the gem (library) we're using to serialize these attributes to a single database column. We want to configure it with the shape of the data.

pushcx · 2019-04-24T13:14:54Z

app/controllers/settings_controller.rb

+    kb_ua = params[:kb_ua]
+
+    if Keybase.validate_initial(kb_username, kb_signature, @user.username)
+      @user.keybase_signatures = [{ kb_username: kb_username, sig_hash: kb_signature }]


Please filter these response values down to alphanumeric if possible - we print them later and I'd like to make sure there's no XSS injection potential. I'd be less paranoid on the way in if Rails would let us tag attributes as user-supplied and really freak out if they ever get to output, but that's not a feature sooo....

I've never used ruby before, how would I go about doing this?

I hadn't read the Keybase class before I wrote this, and it would be a good place to enforce the validity of this data. kb_username.gsub!(/[^0-9A-Za-z_\-]/, '') removed anything that's not an acceptable character in the kb_username variable.

pushcx · 2019-04-24T13:19:03Z

app/views/settings/index.html.erb

+          raw("<a href=\"https://keybase.io/\">Keybase</a>:"),
+          :class => "required" %>
+        <span>
+        <% if @edit_user.keybase_signatures.present? && Keybase.verify(@edit_user.keybase_signatures[0].kb_username, @edit_user.keybase_signatures[0].sig_hash, @edit_user.username) %>


Remove this. We don't want to hit an API from a view. If this is a check that has to happen before we're sure their Keybase identity is verified, put it before storing those credentials.

this check should be done every time we provide the signature externally, where should it go?

I don't see that in their doc and I don't want to block our page load on a call to their service. If they were down or degraded we'd break. User profiles are some of our highest-traffic pages.

I hope this isn't a dealbreaker requirement from Keybase.

tricky thing here. we can't actually check that the proof is live in keybase before saving it in lobsters. keybase needs to see the proof being served from lobsters so it can report back to you that it's live.

Keybase can't see /settings without authenticating as that user. Sounds like the PR will need to add an endpoint to serve back the proof.

keybase currently is configured to check /u/name.json

confirmed. keybase is hitting, e.g. lobste.rs/u/pushcx.json, and looking at the "keybase_signatures" field in there. that should all be unauthed.

pushcx · 2019-04-24T13:19:55Z

app/views/settings/index.html.erb

+        <span>
+        <% if @edit_user.keybase_signatures.present? && Keybase.verify(@edit_user.keybase_signatures[0].kb_username, @edit_user.keybase_signatures[0].sig_hash, @edit_user.username) %>
+          Linked to
+          <strong><a href="https://keybase.io/<%= h(@edit_user.keybase_signatures[0].keybase_username)


Please show all, not one.

I'm not sure how to iterate with this template language, can you provide some more information

<% @edit_user.keybase_signatures.each do |kb_signature| %> <strong><a href="https://keybase.io/<%= h(kb_signature.keybase_username)</a></strong> <% end %>

extras/keybase.rb

pushcx · 2019-04-24T13:21:09Z

config/routes.rb

@@ -146,6 +146,7 @@
  get "/settings/twitter_auth" => "settings#twitter_auth"
  get "/settings/twitter_callback" => "settings#twitter_callback"
  post "/settings/twitter_disconnect" => "settings#twitter_disconnect"
+  get "/settings/keybase_auth" => "settings#keybase_auth"


if Keybase.enabled?

where do I put that?

Ruby allows trailining conditionals, you can put it at the end of the line. (If there's some syntax error hassle, just put it above the get, indent the get, and add the end.)

pushcx · 2019-04-24T13:21:45Z

app/controllers/settings_controller.rb

@@ -256,6 +256,25 @@ def twitter_disconnect
    return redirect_to "/settings"
  end

+  def keybase_auth


raise SomethingAppropriate unless Keybase.enabled?

what is SomethingAppropriate here? I'm not familiar with errors in ruby

I totally guessed wrong about your familiarity, sorry about that. :) I was writing quickly and didn't want to stop to look it up. ActionController::RoutingError.new("Keybase not enabled")

(This is a belt-and-suspenders check that's redundant with disabling the routing, but I'm a little paranoid around security features like authorization.)

pushcx · 2019-04-24T13:47:36Z

keybase.json

+  "brand_color": "#AC130D",
+
+  "logo": {
+    "svg_black": "https://lobste.rs/small-black-logo.svg",


This is all we have. Font is Baskerville Semi-Bold if you want to create an svg.

pushcx · 2019-04-24T13:48:35Z

keybase.json

+  },
+
+  "username": {
+    "re": "^[A-Za-z0-9][A-Za-z0-9_-]{0,24}$",


Please add a comment that this needs to match the User model's validation, and a comment there pointing back here.

json doesn't have comments, but i will add some comments to the other locations

pushcx · 2019-04-24T13:51:19Z

Also needs to display on app/views/user/show.html.erb.

Thanks for diving in to a new environment to contribute! I'll respond as I can (coming up on the end of my pre-work coding time) so maybe @malgorithms or someone else who knows Ruby can also help.

xgess · 2019-04-24T14:54:45Z

the way this is implemented, lobsters is immediately validating and saving whatever keybase sends it in the initial GET request to settings/keybase_auth. per the keybase docs, the user should have a chance to review that the two accounts are both theirs before submitting (e.g. a second POST route on the same controller). it could be a little jarring as a user to push a button in keybase, wait a couple seconds, and then see another keybase thing without ever actually seeing that they've gone to lobste.rs and verified something.

xgess · 2019-04-24T19:09:05Z

i made a pr into your pr to show a few places where things weren't quite working. devsnek#1
if you hit me up on keybase @xgess i can set you up with a sockpuppet account so you can test this against a live keybase environment a little more easily.

and thanks for building this! we're excited about it :)

patch keybase integration for functionality

xgess · 2019-04-24T20:09:16Z

app/views/settings/index.html.erb

+          raw("<a href=\"https://keybase.io/\">Keybase</a>:"),
+          :class => "required" %>
+        <span>
+        <% if @edit_user.keybase_signatures.present? && Keybase.validate(@edit_user.keybase_signatures[0][:kb_username], @edit_user.keybase_signatures[0][:sig_hash], @edit_user.username) %>


the comment is gone, but i agree that these requests are better done in the controller than in the view. i'd suggest setting up an instance variable in the controller (like @edit_user) for @keybase_signatures that's got everything on it. that way the view can just iterate through and show whatever it wants from it.

xgess · 2019-05-03T13:46:12Z

just a little update. @devsnek and i are working on this, and we'll push a bigger update probably in the next several days.

xgess · 2019-05-15T13:49:18Z

@pushcx i haven't heard from @devsnek in a bit, so i kinda just did the last couple bits. devsnek#2 is a PR into this branch that i think makes it feature complete, but i need him to merge it. if we don't hear from him, i can just open my branch as a new PR into lobsters. that way you can see it all at once.

some keybase integration updates

devsnek · 2019-05-15T14:08:58Z

yeah sorry this fell off my radar a bit 😄

xgess · 2019-05-15T17:37:31Z

sweet. thanks @devsnek. @pushcx i think this is ready for a re-review.

app/models/user.rb

pushcx · 2019-05-29T12:41:31Z

app/controllers/keybase_proofs_controller.rb

+private
+
+  def check_user_matches
+    unless @user.username.casecmp(params[:username]).zero?


Would casecmp? match Keybase's desired behavior here? I like that it's a little clearer to use a boolean predicate than check for zero on a sorting operator, and it's nice that it supports Unicode for the non-english sister sites that have broadened the definition of User::VALID_USERNAME.

casecmp? isn't until ruby 2.4.6 and we're on 2.3.3 :/
but i can extract this into a predicate so at least it's easier to read.

pushcx · 2019-05-29T12:47:13Z

app/controllers/keybase_proofs_controller.rb

@@ -0,0 +1,45 @@
+class KeybaseProofsController < ApplicationController


I really like that you've split this out into its own controller rather than fit it into Users or Settings, this is great.

Could you bring the /.well-known endpoint into this controller as well? If we support more of those endpoints in the future it makes a lot more sense to group code by functionality than by URL. And if (knock on wood) anyone has to debug Keybase integration they'll be happy to have it all together.

pushcx · 2019-05-29T12:55:39Z

app/controllers/keybase_proofs_controller.rb

+  end
+
+  def check_new_params
+    redirect_to settings_path unless [:kb_username, :kb_signature, :kb_ua, :username].all? do |k|


Suggested change

redirect_to settings_path unless [:kb_username, :kb_signature, :kb_ua, :username].all? do |k|

redirect_to settings_path unless [:kb_username, :kb_signature, :kb_ua, :username].all?(&:present?)

Could you explain this logic? Why do we push someone back to /settings without an error message if all the params are present? Should this be sending them back if any are missing instead of if all of them are present?

this redirects to the main settings unless they're all there. also, this should never happen in the normal user flow, since these parameters will all be populated by keybase (there aren't links to the /new path anywhere because you need to start the proof flow from keybase). so you'd only hit this redirect if you were manually messing with the url trying to do something weird.

pushcx · 2019-05-29T13:00:38Z

app/controllers/well_known_controller.rb

+    @brand_color = "#AC130D"
+    @description = "Computing-focused community centered around link aggregation and discussion"
+    @contacts = ["admin@#{Keybase.DOMAIN}"]
+    # rubocop:disable Style/FormatStringToken


Huh, why did this cop fire? I've read its docs and I don't even get why it's firing in this case. I see you do %{} instead of #{} for some values, but I don't see a call to format or % in this code or view, so I'm puzzled why you do that?

sure. %{} is what keybase is expecting, so we need to do that. and here's the error rubocop gives...

C: Style/FormatStringToken: Prefer annotated tokens (like %<foo>s) over template tokens (like %{foo}).

pushcx · 2019-05-29T13:02:42Z

app/models/user.rb

@@ -433,6 +438,14 @@ def is_new?
    Time.current - self.created_at <= NEW_USER_DAYS.days
  end

+  def add_or_update_keybase_proof(kb_username, kb_signature)
+    self.keybase_signatures ||= []
+    self.keybase_signatures = self.keybase_signatures.select do |kbsig|


Suggested change

self.keybase_signatures = self.keybase_signatures.select do |kbsig|

self.keybase_signatures.reject! {|kbsig| kbsig['kb_username'] == kb_username }

One-liner, also makes it explicit that we're throwing away any existing sig with this username.

agreed. this is strictly better.

pushcx · 2019-05-29T13:07:19Z

app/views/users/show.html.erb

    </div>
  <% end %>

+  <% if Keybase.enabled? %>
+    <% for_self = (@showing_user == @user) %>


...oh, of course. :) Nice bit of abstraction to be explicit about what repeated checks are doing.

pushcx · 2019-05-29T13:09:30Z

extras/sponge.rb

@@ -141,7 +141,7 @@ def fetch(url, method = :get, fields = nil, raw_post_data = nil, headers = {}, l
    end

    if BAD_NETS.select {|n| IPAddr.new(n).include?(ip) }.any?
-      raise BadIPsError.new("refusing to talk to IP #{ip}")
+      raise BadIPsError.new("refusing to talk to IP #{ip}") if Rails.env.production?


What triggered this in dev/testing? I'm leery of relaxing this protection in such a general way.

this blocks requests to localhost, so i couldn't test an end-to-end proof locally (with a locally running keybase stack) without suppressing the check in dev. i understand your concern. i'll remove it and replace with a little comment somewhere else.

pushcx · 2019-05-29T13:14:17Z

This is looking great! I have a couple small tweaks and then a few more questions to make sure I understand what's at work. It's all small enough that I'll be hope to squeeze in a merge some lunch/evening rather than wait until my next Wednesday morning to re-review.

* casecmp.zero? => case_insensitive_match? * move keybase config to the keybase proofs controller * keybase_proofs select => reject * remove previous restriction of BadIPsError and replace with descriptive comment * clean up keybase link creation with a helper

* remove a test i just added that wasn't actually worth it * rubocop

xgess · 2019-06-05T17:23:16Z

hey @pushcx, i just wanted to ping you here and maybe keep the momentum going. thanks!

pushcx · 2019-06-12T13:04:08Z

extras/sponge.rb

@@ -141,7 +141,9 @@ def fetch(url, method = :get, fields = nil, raw_post_data = nil, headers = {}, l
    end

    if BAD_NETS.select {|n| IPAddr.new(n).include?(ip) }.any?
-      raise BadIPsError.new("refusing to talk to IP #{ip}") if Rails.env.production?
+      # This blocks all requests to localhost, so you might need to comment


Great comment, makes it instantly apparent what's going on.

pushcx · 2019-06-12T13:07:38Z

Hey, I was out of town last week for work and didn't have much outside dev time. I'm merging and deploying this now.

Thanks for all this work implementing and revising this PR, and also to @xgess for joining in.

pushcx · 2019-06-12T14:09:36Z

OK. Their validator reports our config is good, so this is waiting on Keybase listing the integration. I've emailed them according to the install guide.

arp242 · 2019-06-13T12:23:53Z

Seems active and working 👍 I linked my profile: https://lobste.rs/u/arp242

pushcx · 2021-11-02T14:34:54Z

Leaving a comment here because people are presumably still cc'd - is there value to keeping this integration? We have a couple open bugs (#807, #796) but I'm not sure they're worth dev time given how few users have mentioned them.

Keybase's value was supposed to be integrating with many sites to collect identity info, but since Zoom acquired them their public repos have had very little activity. There's not much benefit to Keybase if they don't add new sites and maintain integrations. We can wait until Zoom shuts it down entirely, but if that happens informally when something breaks in a way that exceeds their maintenance budget, we're suddenly given a high-priority bug report as our integration starts throwing 500s or timing out. Maybe it's better to bite the bullet and remove it now.

Looking at the database, 505 of our users have authenticated with Keybase, which is significantly higher than I expected. I'll send a note to the couple dozen that have been active on Lobsters recently to ask what value they get out of our integration and post back here, probably later this month.

devsnek force-pushed the feature/keybase branch 2 times, most recently from 0fb743f to 7b706b5 Compare April 15, 2019 16:01

Add keybase integration

d403d3a

This allows users to verify keybase with their account. Refs: https://keybase.io/docs/proof_integration_guide

devsnek force-pushed the feature/keybase branch from 7b706b5 to d403d3a Compare April 15, 2019 16:04

pushcx force-pushed the master branch 4 times, most recently from acf676b to 316d894 Compare April 24, 2019 03:37

pushcx reviewed Apr 24, 2019

View reviewed changes

extras/keybase.rb Show resolved Hide resolved

pushcx reviewed Apr 24, 2019

View reviewed changes

extras/keybase.rb Show resolved Hide resolved

pushcx reviewed Apr 24, 2019

View reviewed changes

patch keybase integration for functionality

e39047f

Merge pull request #1 from xgess/xgess/feature/keybase

ae0af99

patch keybase integration for functionality

xgess reviewed Apr 24, 2019

View reviewed changes

some keybase integration updates (#2)

0a56c0c

some keybase integration updates

devsnek marked this pull request as ready for review May 18, 2019 15:18

Merge branch 'master' into feature/keybase

d80dbc4

xgess reviewed May 18, 2019

View reviewed changes

app/models/user.rb Show resolved Hide resolved

xgess added 2 commits May 24, 2019 16:29

add default keybase base_url

7f159a8

fixes for rubocop

b5cf2ed

pushcx reviewed May 29, 2019

View reviewed changes

xgess mentioned this pull request May 31, 2019

a few small tweaks devsnek/lobsters#4

Merged

xgess added 2 commits May 31, 2019 12:02

a few small tweaks (#4)

20d71f5

* casecmp.zero? => case_insensitive_match? * move keybase config to the keybase proofs controller * keybase_proofs select => reject * remove previous restriction of BadIPsError and replace with descriptive comment * clean up keybase link creation with a helper

4th times the charm (#5)

6742765

* remove a test i just added that wasn't actually worth it * rubocop

pushcx reviewed Jun 12, 2019

View reviewed changes

pushcx merged commit 4fc391e into lobsters:master Jun 12, 2019

devsnek deleted the feature/keybase branch June 12, 2019 13:56

pushcx mentioned this pull request Jun 14, 2019

strange keybase information in public profile view for self #691

Closed

pushcx pushed a commit that referenced this pull request Jan 7, 2022

Add keybase integration (#661)

b7f5a85

	s.array :keybase_signatures
	s.string :keybase_signatures, array: true, default: [], null: false

		@@ -0,0 +1,45 @@
		class KeybaseProofsController < ApplicationController

	redirect_to settings_path unless [:kb_username, :kb_signature, :kb_ua, :username].all? do \|k\|
	redirect_to settings_path unless [:kb_username, :kb_signature, :kb_ua, :username].all?(&:present?)

	self.keybase_signatures = self.keybase_signatures.select do \|kbsig\|
	self.keybase_signatures.reject! {\|kbsig\| kbsig['kb_username'] == kb_username }

Add keybase integration #661

Add keybase integration #661

Conversation

devsnek commented Apr 15, 2019

pushcx commented Apr 17, 2019

malgorithms commented Apr 18, 2019

pushcx commented Apr 24, 2019

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pushcx commented Apr 24, 2019

xgess commented Apr 24, 2019

xgess commented Apr 24, 2019 • edited Loading

Choose a reason for hiding this comment

xgess commented May 3, 2019

xgess commented May 15, 2019

devsnek commented May 15, 2019

xgess commented May 15, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pushcx May 29, 2019 • edited Loading

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

Choose a reason for hiding this comment

pushcx commented May 29, 2019

xgess commented Jun 5, 2019

Choose a reason for hiding this comment

pushcx commented Jun 12, 2019

pushcx commented Jun 12, 2019

arp242 commented Jun 13, 2019

pushcx commented Nov 2, 2021 • edited Loading

xgess commented Apr 24, 2019 •

edited

Loading

pushcx May 29, 2019 •

edited

Loading

pushcx commented Nov 2, 2021 •

edited

Loading