Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow configuring crypto_providers with Strings #668

Closed

Conversation

mihael
Copy link

@mihael mihael commented May 5, 2019

In order to prevent autoloading SCrypt before config is applied, I want to use Strings to configure the default crypto_provider.

Expected Behavior

When I use authlogic 3.4.0+ on linux with a grsec kernel, it should not RuntimeError.

Actual Behavior

For example, running with authlogic on an Ubuntu 4.14.93-grsec-grsec+ results in a RuntimeError.

The method acts_as_authentic takes a block to allow configuration. When this is done the method crypto_provider will be called. The method signature contains a default argument with value of CryptoProviders::SCrypt.

See: https://github.com/mihael/authlogic/blob/master/lib/authlogic/acts_as_authentic/password.rb#L116

When this happens SCrypt lib will load and the following lines are executed:

attach_function :sc_calibrate, [:size_t, :double, :double, :pointer], :int, :blocking => true

See: https://github.com/pbhogan/scrypt/blob/v3.0.6/lib/scrypt.rb#L14

Which then fails with a RuntimeError in the attach function in the ffi lib.

The ffi lib states:

On Linux systems running with PaX (Gentoo, Alpine, etc.), FFI may trigger mprotect errors. You may need to disable mprotect for ruby (paxctl -m [/path/to/ruby]) for the time being until a solution is found.

So, if we simply try to configure BCrypt inside acts_as_authentic block, we will still load and run the SCrypt lib, and thus fail and the server will not work.

acts_as_authentic do |c|
    c.crypto_provider = Authlogic::CryptoProviders::BCrypt # <= failing with RuntimeError
end

Solution

Initially I worked around this by using BCrypt as the default value passed in the method. But then created this PR to have a better way to configure authlogic, so it would not load SCrypt prior to configuring. This PR does a bit of refactoring to allow passing String instead of Class for the default value of crypto_provider, thus avoiding the autoload.

With this code I can run authlogic on grsec kernels, since SCrypt will not autoload during runtime (assuming another provider was configured, like BCrypt, since SCrypt obviously can't work on grsec due to ffi limitation).

Extra context:
technion/ruby-argon2#15
ffi/ffi#578

@jaredbeck
Copy link
Collaborator

Hi Miha, Thanks for the contribution. constantize seems like a reasonable option.

Just to throw another idea out there, how about putting the default value in a Proc?

# lib/authlogic/acts_as_authentic/password.rb
         def crypto_provider(value = nil)
           CryptoProviders::Guidance.new(value).impart_wisdom
-          rw_config(:crypto_provider, value, CryptoProviders::SCrypt)
+          rw_config(:crypto_provider, value, ->() { CryptoProviders::SCrypt })
         end

# a/lib/authlogic/config.rb
-    def rw_config(key, value, default_value = nil)
+    def rw_config(key, value, default_value_proc = nil)
       if value.nil?
-        acts_as_authentic_config.include?(key) ? acts_as_authentic_config[key] : default_value
+        acts_as_authentic_config.include?(key) ? acts_as_authentic_config[key] : default_value_proc.call
       else
         self.acts_as_authentic_config = acts_as_authentic_config.merge(key => value)

@mihael
Copy link
Author

mihael commented May 5, 2019

Hi @jaredbeck , 👍

I created an alternative solution based on your suggestion. Touched therw_config method a bit to make it work with Proc.

👁 #669

@jaredbeck
Copy link
Collaborator

@tiegz what do you think?

@tiegz
Copy link
Collaborator

tiegz commented May 16, 2019

@jaredbeck @mihael +1 to the proc idea! Looks like I'd considered that in the past too. My only question is: would it be valuable to have any value be a proc rather than just the default?

@mihael
Copy link
Author

mihael commented May 16, 2019

Hey @tiegz

My only question is: would it be valuable to have any value be a proc rather than just the default?

I think there might be a case were someone would like to dynamically configure the crypto provider, say on startup of a system. Passing a proc would make this an easy task, right?

I like the proc solution, it's simpler and shorter and easier to understand. And it works nicely on my production box.

Am closing this in favour of #669 👍

@mihael mihael closed this May 16, 2019
jaredbeck added a commit that referenced this pull request Sep 8, 2019
[Fixes #668]

See changelog for description, rationale.
jaredbeck added a commit that referenced this pull request Sep 11, 2019
[Fixes #668]

See changelog for description, rationale.
@jaredbeck
Copy link
Collaborator

Fixed by #679

@jaredbeck
Copy link
Collaborator

Released as 6.0.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants