Configurable token randomness #67

mtsmfm · 2017-06-13T16:35:27Z

To increase security, I want to change token randomness by configuration 🔑

Ch4s3 · 2017-06-13T20:35:44Z

Should we set a minimum?

mtsmfm · 2017-06-15T15:06:31Z

@Ch4s3 good to have but I'm not sure how long minimum should be 🤔

snkrheadz · 2017-06-15T16:36:47Z

lib/sorcery/model/temporary_token.rb

@@ -12,7 +12,7 @@ def self.included(base)

      # Random code, used for salt and temporary tokens.
      def self.generate_random_token
-        SecureRandom.urlsafe_base64(15).tr('lIO0', 'sxyz')
+        SecureRandom.urlsafe_base64(Controller::Config.token_randomness).tr('lIO0', 'sxyz')


i think this is not safety enough.
can you consider to do something like below?

SecureRandom.urlsafe_base64([15, Controller::Config.token_randomness].max).tr('lIO0', 'sxyz')

and there may be one more thing, you can calculate real length of the result value if you want.

real_length = (15 * 3) / 4 SecureRandom.urlsafe_base64(real_length).tr('lIO0', 'sxyz').size => 15

[15, Controller::Config.token_randomness].max

It is weird that it isn't set the specified value by user. So, I think the specified value should always be set.

and there may be one more thing, you can calculate real length of the result value if you want.

I think it is difficult to control real length:

[3] pry(main)> (15..100).select {|i| SecureRandom.urlsafe_base64((i * 3) / 4).size != i } => [17, 21, 25, 29, 33, 37, 41, 45, 49, 53, 57, 61, 65, 69, 73, 77, 81, 85, 89, 93, 97]

i think this is not safety enough.
can you consider to do something like below?

I agree with setting minimum randomness as 15 👍

We (@kyuden and I) talked in person and we think the specified value always should be respected so I won't implement minimum value.

kyuden · 2017-06-15T17:29:41Z

lib/sorcery/controller/config.rb

@@ -12,6 +12,8 @@ class << self
        attr_accessor :save_return_to_url
        # set domain option for cookies
        attr_accessor :cookie_domain
+        # Set token randomness
+        attr_accessor :token_randomness


Sorcery:Controler::Config is configuration used for Sorcery::Controller. But token_randomness is used for Sorcery::Model::TemporaryToken So, It is better to set it with Sorcery:Model::Config like encryption_algorithm

We can't access Sorcery::Model::Config instance directly because Sorcery::Model::TemporaryToken is a helper module and isn't included into user_class.

If you really want to do that, the code will be the following:

Sorcery::Controller::Config.user_class.to_s.constantize.sorcery_config.token_randomness

https://github.com/Sorcery/sorcery/blob/36698d7/lib/sorcery/adapters/active_record_adapter.rb#L22

Should I keep the setting in Sorcery:Controler::Config or do such trick?

💭 IMO, it seems that Sorcery::Model::Config is singleton so it is not bad to change implementation to access the instance as same as Sorcery::Controller::Config

Sorcery::Model::Config.token_randomness #=> 15

TemporaryToken is under Model module so I'll move this setting to Sorcery::Model::Config

mtsmfm · 2017-06-18T06:44:13Z

@kyuden @akinrt Could you review this PR again? 🙏

kyuden · 2017-06-18T08:33:22Z

lib/generators/sorcery/templates/initializer.rb

+    # The length of the result string is about 4/3 of `token_randomness`.
+    # Default: `15`
+    #
+    # config.token_randomness =


s/config.token_randomness =/user.token_randomness =/

@mtsmfm Could you actually check that not only the test code but also this setting works?

@kyuden

Fixed 🙇

I confirmed that a token in password reset email becomes longer:

kyuden · 2017-06-18T14:59:53Z

@athix I approved this PR. Could you take a look at this?

kyuden · 2017-06-24T18:50:22Z

@athix Are you busy in this week?

joshbuker · 2017-06-27T07:03:40Z

@kyuden Sorry, yeah I've been busy with some personal stuff and work. I'll try to get a review for this done as soon as I can.

kyuden · 2017-06-27T15:39:18Z

@athix thx!

joshbuker

Looks good, but I'd like to see if we can make the sorcery_config call consistent with other similar calls before rolling this out.

joshbuker · 2017-06-29T20:58:48Z

lib/sorcery/model/temporary_token.rb

@@ -12,7 +12,7 @@ def self.included(base)

      # Random code, used for salt and temporary tokens.
      def self.generate_random_token
-        SecureRandom.urlsafe_base64(15).tr('lIO0', 'sxyz')
+        SecureRandom.urlsafe_base64(Sorcery::Controller::Config.user_class.to_s.constantize.sorcery_config.token_randomness).tr('lIO0', 'sxyz')


It looks like this randomization method is used globally throughout Sorcery, so I'm a bit hesitant tying the config specifically to the user. Also, it seems a bit odd that we'd have to hook through the Controller config to get the value, instead of being able to call sorcery_config directly. Looks like getting that to work isn't exactly trivial though because the generate method is outside of ClassMethods/InstanceMethods.

I'll keep digging around and see if I can figure out what's going on in there.

joshbuker · 2017-06-29T21:01:27Z

spec/sorcery_temporary_token_spec.rb

@@ -0,0 +1,27 @@
+require 'spec_helper'


Yay! Thanks for writing tests for the new functionality. 👍

…g to general section

joshbuker · 2017-07-13T19:59:24Z

lib/sorcery/model/temporary_token.rb

@@ -7,12 +7,14 @@ module Model
    # such as reseting password and activating the user by email.
    module TemporaryToken
      def self.included(base)
+        # FIXME: This may not be the ideal way of passing sorcery_config to generate_random_token.
+        @sorcery_config = base.sorcery_config


Mildly worried that this might cause @sorcery_config to be unintentionally exposed, but not familiar enough with how ruby inheritance/privacy works to be sure. This is how it gets saved in lib/sorcery/model.rb however.

@sorcery_config is owned by TemporaryToken.
It will be overridden if we include TemporaryToken.

module Foo def self.included(base) @var = base.name end def self.foo puts @var end end class Bar include Foo end Foo.foo #=> Bar puts Foo.instance_variable_get(:@var) #=> Bar class Baz include Foo end Foo.foo #=> Baz puts Foo.instance_variable_get(:@var) #=> Baz

Is this intentional behaviour?

As far as I can tell, Sorcery::Model is the only place where TemporaryToken is/should be included. Based on your description, it sounds like it should be working exactly as intended. What I was worried about was more along the lines of @sorcery_config accidentally becoming available inside of a controller/model class.

For example:

class User < ApplicationRecord authenticates_with_sorcery! def some_method # @sorcery_config should not be available here! @sorcery_config = nil # This hopefully shouldn't break things either! end end

Sorry for late reply 💦

Don't worry, User instance can't override @sorcery_config

module Foo def self.included(base) @var = base.name end def self.foo @var end end class Bar include Foo def bar @var end def bar=(var) @var = var end end p Foo.foo #=> "Bar" p Foo.instance_variable_get(:@var) #=> "Bar" bar = Bar.new p bar.bar #=> nil bar.bar = :bar p bar.bar #=> :bar p Foo.foo #=> Bar p Foo.instance_variable_get(:@var) #=> Bar

joshbuker · 2017-07-13T20:05:42Z

@Ch4s3 I think this is probably ready to go now, mind giving this a once over?

joshbuker · 2017-07-25T20:45:15Z

I'm planning on cutting a new release sometime in the next couple weeks, if I don't hear any objections before then I'll merge this in and include it for that release.

snkrheadz reviewed Jun 15, 2017

View reviewed changes

kyuden reviewed Jun 15, 2017

View reviewed changes

mtsmfm force-pushed the config-for-token-randomness branch from 36698d7 to 19143fc Compare June 18, 2017 06:43

kyuden reviewed Jun 18, 2017

View reviewed changes

Configurable token randomness

d0e403a

mtsmfm force-pushed the config-for-token-randomness branch from 19143fc to d0e403a Compare June 18, 2017 11:36

kyuden approved these changes Jun 18, 2017

View reviewed changes

Add changelog entry for pull Sorcery#67

cfe32eb

joshbuker requested changes Jun 29, 2017

View reviewed changes

joshbuker added 2 commits July 13, 2017 12:52

Update config call in generate_random_token and move randomness confi…

87526f2

…g to general section

Merge branch 'master' into config-for-token-randomness

964a5cd

joshbuker approved these changes Jul 13, 2017

View reviewed changes

joshbuker reviewed Jul 13, 2017

View reviewed changes

joshbuker merged commit c9b8108 into Sorcery:master Aug 3, 2017

joshbuker mentioned this pull request Jun 5, 2021

token_randomness can't be changed #276

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Configurable token randomness #67

Configurable token randomness #67

mtsmfm commented Jun 13, 2017

Ch4s3 commented Jun 13, 2017

mtsmfm commented Jun 15, 2017

snkrheadz Jun 15, 2017

snkrheadz Jun 15, 2017

kyuden Jun 15, 2017

mtsmfm Jun 16, 2017

mtsmfm Jun 16, 2017

mtsmfm Jun 18, 2017

kyuden Jun 15, 2017

mtsmfm Jun 16, 2017

mtsmfm Jun 16, 2017 •

edited

Loading

mtsmfm Jun 18, 2017

mtsmfm commented Jun 18, 2017

kyuden Jun 18, 2017

mtsmfm Jun 18, 2017 •

edited

Loading

kyuden commented Jun 18, 2017

kyuden commented Jun 24, 2017

joshbuker commented Jun 27, 2017

kyuden commented Jun 27, 2017

joshbuker left a comment

joshbuker Jun 29, 2017

joshbuker Jun 29, 2017

joshbuker Jul 13, 2017

mtsmfm Jul 14, 2017

joshbuker Jul 14, 2017

mtsmfm Jul 26, 2017

joshbuker commented Jul 13, 2017

joshbuker commented Jul 25, 2017

Configurable token randomness #67

Configurable token randomness #67

Conversation

mtsmfm commented Jun 13, 2017

Ch4s3 commented Jun 13, 2017

mtsmfm commented Jun 15, 2017

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

mtsmfm Jun 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtsmfm commented Jun 18, 2017

Choose a reason for hiding this comment

mtsmfm Jun 18, 2017 • edited Loading

Choose a reason for hiding this comment

kyuden commented Jun 18, 2017

kyuden commented Jun 24, 2017

joshbuker commented Jun 27, 2017

kyuden commented Jun 27, 2017

joshbuker 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshbuker commented Jul 13, 2017

joshbuker commented Jul 25, 2017

mtsmfm Jun 16, 2017 •

edited

Loading

mtsmfm Jun 18, 2017 •

edited

Loading