Store hashed tokens #81

timwis · 2020-04-05T18:37:16Z

Stores hashed versions of the tokens in the database per #80. This PR is not quite done yet, but I wanted to share the draft to get feedback in the meantime. I'm not normally a ruby developer so I'm sure there's things we could improve in it.

I used OpenSSL::HMAC like devise does (and basically copied their token generator class) since BCrypt is not idempotent (there's no way to do session = Session.find_by(token_hash: BCrypt::Password.create(token)) as create generates a new salt every time).

I'll leave more comments inline in the code.

I know I still need to:

Get the assertion in navigation_test to pass
Figure out how to use the rails secret key for generating the key name for HMAC, or whether that's even necessary (devise seems to do it)

Looking forward to your feedback (feel free to edit directly). Hope this is a good start.

timwis · 2020-04-05T18:38:07Z

app/models/passwordless/session.rb

- break token unless Session.find_by(token: token)
- }
+
+ if not self.token or not self.token_digest


I couldn't get conditional assignment to work with destructuring 🤷‍♂️

timwis · 2020-04-05T18:38:37Z

db/migrate/20171104221735_create_passwordless_sessions.rb

@@ -13,7 +13,7 @@ def change
 t.datetime :claimed_at
 t.text :user_agent, null: false
 t.string :remote_addr, null: false
- t.string :token, null: false
+ t.string :token_digest, null: false


Does it make sense to modify the existing migration, or should we add a second migration?

I think we should modify the original but provide upgrade instructions for existing installations.

timwis · 2020-04-05T18:40:27Z

app/models/passwordless/session.rb

@@ -13,11 +13,13 @@ class Session < ApplicationRecord
 :expires_at,
 :user_agent,
 :remote_addr,
- :token,
+ :token_digest,


I thought calling the database column token_digest made it more clear that it's not the actual token. The tradeoff is this is a breaking change and assumedly would require a major version bump and running a migration. We can keep the column name the same to make upgrading easier, but you could still call it a breaking change if there are any valid non-hashed tokens in the database when you upgrade.

I agree we should hint in the column name that it's not the actual token 👍

As we're still pre 1.0, we can do breaking changes like this in a minor release. But I still like to provide all the necessary upgrade info. I think we should:

Rename the field so all new installations have it right

Provide upgrade instructions in the changelog including an example migration

timwis · 2020-04-05T18:41:35Z

lib/passwordless.rb

- mattr_accessor(:token_generator) { UrlSafeBase64Generator.new }
+ mattr_accessor(:token_generator) {
+ Passwordless::TokenGenerator.new(
+ ActiveSupport::CachingKeyGenerator.new(ActiveSupport::KeyGenerator.new('secret')),


I copied this from devise. I'm not sure whether it's necessary for OpenSSL::HMAC to get a secret "key". If so, should we use rails' SECRET_KEY_BASE or whatever?

I think I'd like to keep CachingKeyGenerator out of this block – maybe move it to a Passwordless.digest(token) method? Then use it in a setter on Session like:

def token=(value) self.token_digest = Passwordless.digest(value) end

I think using Rails' secret key is a good idea 👍

timwis · 2020-04-05T18:42:42Z

lib/passwordless/token_generator.rb

+module Passwordless
+ class TokenGenerator
+ def initialize(key_generator, friendly_token_generator, digest)
+ @key = key_generator.generate_key('passwordless')


Devise uses the column name but I've just called it 'passwordless'

test/integration/navigation_test.rb

mikker · 2020-04-06T05:28:37Z

Wonderful work so far ❤️

timwis · 2020-04-06T06:48:32Z

Thanks! I’d love your thoughts on the field rename and migration bit when you have a sec (see my inline comments). And the secret key thing if any ideas come to mind, otherwise I’ll do some more research on it.

mikker

Let me know what you think 😊

mikker · 2020-04-19T20:53:01Z

app/models/passwordless/session.rb

@@ -13,11 +13,13 @@ class Session < ApplicationRecord
 :expires_at,
 :user_agent,
 :remote_addr,
- :token,
+ :token_digest,


I agree we should hint in the column name that it's not the actual token 👍

As we're still pre 1.0, we can do breaking changes like this in a minor release. But I still like to provide all the necessary upgrade info. I think we should:

Rename the field so all new installations have it right

Provide upgrade instructions in the changelog including an example migration

mikker · 2020-04-19T21:09:51Z

lib/passwordless.rb

- mattr_accessor(:token_generator) { UrlSafeBase64Generator.new }
+ mattr_accessor(:token_generator) {
+ Passwordless::TokenGenerator.new(
+ ActiveSupport::CachingKeyGenerator.new(ActiveSupport::KeyGenerator.new('secret')),


I think I'd like to keep CachingKeyGenerator out of this block – maybe move it to a Passwordless.digest(token) method? Then use it in a setter on Session like:

def token=(value) self.token_digest = Passwordless.digest(value) end

I think using Rails' secret key is a good idea 👍

mikker · 2020-04-19T21:11:58Z

lib/passwordless/token_generator.rb

+require 'openssl'
+
+module Passwordless
+ class TokenGenerator


I think we should split out the hashing from this. The token generators are supposed to be easy to provide yourself and if you do, I think we should still provide the hashing.

Let's let the token generator still just return a simple, unhashed string.

timwis · 2020-04-21T16:33:44Z

Thanks for the feedback! All great notes. I'll work on that over the next week or so as I get time.

Didn't realise there could be breaking changes in a minor version release for this library though! I'll have to update my project's Gemfile to prevent breaking changes coming in 😬

test/models/passwordless/session_test.rb

timwis · 2020-05-07T14:00:08Z

lib/passwordless.rb

@@ -9,6 +10,8 @@
 module Passwordless
 mattr_accessor(:default_from_address) { "[email protected]" }
 mattr_accessor(:token_generator) { UrlSafeBase64Generator.new }
+ mattr_accessor(:digest_algorithm) { "SHA256" }
+ mattr_accessor(:digest_secret) { lambda { Rails.application.secret_key_base } }


According to the docs this should get it in all 3 environments. Note that deivse uses a helper class for this to support multiple versions of rails out of the box. We could copy that, or just point them here if they're using an older rails version.

No need to support anything below 5.1.4 as per https://github.com/mikker/passwordless/blob/master/passwordless.gemspec#L19

timwis · 2020-05-07T14:03:29Z

Okay, I think I've incorporated all of your feedback. I dropped the separate TokenGenerator module in order to keep token generation and hashing functionality separate. I moved hashing to a static Passwordless.digest method as you suggested, though the only way I could see to do that was on the Passwordless module - let me know if there's a better way. I also added a couple tests for the configurable bits.

Beyond this, I think it's just a matter of updating the readme to reflect (1) how to upgrade/migrate, and (2) how to customise hashing.

Looking forward to your feedback.

mikker

Sorry this took a bit. Left a few comments again 😊

mikker · 2020-05-27T09:43:06Z

db/migrate/20171104221735_create_passwordless_sessions.rb

@@ -13,7 +13,7 @@ def change
 t.datetime :claimed_at
 t.text :user_agent, null: false
 t.string :remote_addr, null: false
- t.string :token, null: false
+ t.string :token_digest, null: false


I think we should modify the original but provide upgrade instructions for existing installations.

mikker · 2020-05-27T09:45:26Z

app/models/passwordless/session.rb

+ if !token || !token_digest
+ self.token, self.token_digest = loop {
+ token = Passwordless.token_generator.call(self)
+ token_digest = Passwordless.digest(token)
+ break [token, token_digest] unless Session.find_by(token_digest: token_digest)
+ }
+ end


This looks a bit suspicious to me. I don't think anything's wrong but it's hard to make sense of. Do we need/want to check for both !token || !token_digest. What if we have one but not the other? Will that ever happen? Could we prevent it? Just dumping my brain, sorry 😊

mikker · 2020-05-27T09:46:26Z

lib/passwordless.rb

@@ -9,6 +10,8 @@
 module Passwordless
 mattr_accessor(:default_from_address) { "[email protected]" }
 mattr_accessor(:token_generator) { UrlSafeBase64Generator.new }
+ mattr_accessor(:digest_algorithm) { "SHA256" }
+ mattr_accessor(:digest_secret) { lambda { Rails.application.secret_key_base } }


No need to support anything below 5.1.4 as per https://github.com/mikker/passwordless/blob/master/passwordless.gemspec#L19

mikker · 2020-05-27T09:47:20Z

lib/passwordless.rb

+ def self.digest(token)
+ key_generator = ActiveSupport::CachingKeyGenerator.new(
+ ActiveSupport::KeyGenerator.new(digest_secret.call)
+ )
+ key = key_generator.generate_key("passwordless")
+ OpenSSL::HMAC.hexdigest(digest_algorithm, key, token)
+ end


Perhaps it would be nice to move this to its own class. Maybe only create the generator once and save it as an ivar.

xdmx · 2020-08-16T16:56:57Z

Any update on this? I'd love to start having hashed values in the db, instead of the real ones.

@timwis were you able to see @mikker's feedback?

timwis · 2020-08-18T08:19:20Z

Hi @xdmx, I haven't been able to integrate @mikker's feedback — unfortunately I've just started a new job that's had me quite full-on :/ I'd love to, but I'd certainly welcome the help if you or @mikker can pick it up! Only a few things left to do, I think.

mikker · 2020-08-18T08:37:00Z

I'm quite busy as well. Any help from anyone on bringing this home is very appreciated 😊

rickychilcott · 2021-12-21T02:44:08Z

It's been a while @timwis or @xdmx - interested in seeing this across the finish line?

mikker · 2021-12-25T22:45:58Z

AFAIR the work is already done in the big 1.0 PR. It was short sighted of me to have everything be in one PR. If anyone's willing to split it up into separate PRs it would be greatly appreciated.

Store hashed tokens (WIP)

211b684

timwis commented Apr 5, 2020

View reviewed changes

test/integration/navigation_test.rb Outdated Show resolved Hide resolved

Fix broken assertion

44adbd4

mikker reviewed Apr 19, 2020

View reviewed changes

Use static digest method in Passwordless module

b1e9ac3

timwis commented May 7, 2020

View reviewed changes

test/models/passwordless/session_test.rb Show resolved Hide resolved

timwis commented May 7, 2020

View reviewed changes

timwis changed the title ~~Store hashed tokens (WIP)~~ Store hashed tokens May 7, 2020

timwis requested a review from mikker May 7, 2020 14:00

timwis marked this pull request as ready for review May 7, 2020 14:01

mikker reviewed May 27, 2020

View reviewed changes

mikker mentioned this pull request Oct 7, 2020

Getting to 1.0 #89

Closed

7 tasks

mikker mentioned this pull request Jun 16, 2023

Store encrypted tokens #145

Merged

mikker closed this in #145 Jun 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Store hashed tokens #81

Store hashed tokens #81

timwis commented Apr 5, 2020 •

edited

Loading

timwis Apr 5, 2020

timwis Apr 5, 2020

mikker May 27, 2020

timwis Apr 5, 2020

mikker Apr 19, 2020

timwis Apr 5, 2020

mikker Apr 19, 2020

timwis Apr 5, 2020

mikker commented Apr 6, 2020

timwis commented Apr 6, 2020

mikker left a comment

mikker Apr 19, 2020

mikker Apr 19, 2020

mikker Apr 19, 2020

timwis commented Apr 21, 2020

timwis May 7, 2020

mikker May 27, 2020

timwis commented May 7, 2020

mikker left a comment

mikker May 27, 2020

mikker May 27, 2020

mikker May 27, 2020

mikker May 27, 2020

xdmx commented Aug 16, 2020

timwis commented Aug 18, 2020

mikker commented Aug 18, 2020

rickychilcott commented Dec 21, 2021

mikker commented Dec 25, 2021

Store hashed tokens #81

Store hashed tokens #81

Conversation

timwis commented Apr 5, 2020 • 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

mikker commented Apr 6, 2020

timwis commented Apr 6, 2020

mikker left a 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

timwis commented Apr 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timwis commented May 7, 2020

mikker left a 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

xdmx commented Aug 16, 2020

timwis commented Aug 18, 2020

mikker commented Aug 18, 2020

rickychilcott commented Dec 21, 2021

mikker commented Dec 25, 2021

timwis commented Apr 5, 2020 •

edited

Loading