Skip to content
This repository

Add ActiveSupport::KeyGenerator as a simple wrapper around PBKDF2 #6952

Merged
merged 2 commits into from over 1 year ago

7 participants

Michael Koziarski Aaron Patterson Guillermo Iguaran Hiroshi Nakamura Martin Bosslet Konstantin Haase Thai Duong
Michael Koziarski
Owner
NZKoz commented July 03, 2012

As part of the changes for Rails4 we want to sort out all the cryptography related things we have in place right now. The first part of this will be to add PBKDF2 for key derivation, then we can switch the session store and cookies.signed to use derived keys instead of using the bare secret.

Similarly future functionality like encrypted cookies / sessions can utilize derived keys.

So before merging I want to get some feedback from @meder and @thaidn on the particulars of our implementation. In particular, what are suitable default values for iterations and key_size.

This is a pre-requisite for #3955 #5034 and possibly others.

Note to other committers: Please leave this one for me to merge

Aaron Patterson
Owner
Michael Koziarski
Owner
NZKoz commented July 03, 2012

Once I have someone confirm we're on the right track, I'll expand this functionality to provide something like:

Your::Application.derive_key("asdf")

I may also rename the class because it's deriving not generating keys. At that stage I think we'll be able to work on #3955 and friends

Guillermo Iguaran
Owner
Hiroshi Nakamura
nahi commented July 03, 2012

Preferred default parameters:

  • iterations: should be > 10000. I use 100000 generally, but 16384 would be not bad at this moment. The key point is enough slowness and it depends on purposes.
  • salt: should have enough entropy. Length check would be good to have. 16 bytes or more?
  • derived key length: should be specified by caller b/c it depends on purpose. SHA1 length (160bit) would be good if you need a default.
  • Something#derive_key interface should have at least a parameter "salt". Just in case it's not a typo.

@emboss should confirm my thoughts. :)

Martin Bosslet
emboss commented July 04, 2012

iterations: should be > 10000. I use 100000 generally, but 16384 would be not bad at this moment. The key point is enough slowness and it depends on purposes.

I agree, it should be at least a 5 digits number. Although, when chosen too large, this can quickly become an attack vector for denial of service attacks. It largely depends on the infrastructure of how much it can handle - maybe chose a default like @nahi proposed, but make still have it configurable somehow?

salt: should have enough entropy. Length check would be good to have. 16 bytes or more?

PKCS#5 says that the salt should at least be 8 bytes, I typically also use the 16 bytes that nahi proposed. What is essential here is that these bytes must be chosen by a cryptographically secure random number generator, OpenSSL::Random or SecureRandom. Ideally a fresh, per-user salt should be used each time, to forbid any opportunity for precomputation right from the start. Normally an attacker would not know the salt, but it's better to design as if they did, which is plausible - think of bribed/rogue devs/admins etc.
This would however require the salt to be a part of the return value or the salt to be part of the interface, handed over by the user. The latter option has the disadvantage that users could still get things wrong with salt generation, though.

derived key length: should be specified by caller b/c it depends on purpose. SHA1 length (160bit) would be good if you need a default.

Yes, the security margin is tightly related to the underlying digest that was used. It can't exceed the output length of the digest - so in nahi's example of SHA-1, choosing more than 160 bit will for example not increase security. So it's probably best to choose the exact output length of the digest. Some doubt has been cast over SHA-1 recently, and the current recommendations are to use SHA-256 wherever possible. Either way, you can then conveniently choose the dk_len parameter as OpenSSL::Digest#digest_length.

One final note about timing attacks when comparing password hashes. It is argued from time to time on the web that one would have to use some "equal time" comparison methods to compare password hashes in order to not leak any information subject to timing attacks. This is a very delicate issue. I asked a cryptographer who I can absolutely trust on these matters and his advice was as follows: If the underlying hash function were ideal, this attack would be even harder than finding collisions or preimages. So in theory there's no need to worry. But, since current hash functions cannot be proven to be ideal, it doesn't hurt to do equal time comparison, better safe than sorry.

If you'd like to follow his advice, the best method that I see currently for doing equal time comparisons is to do something like this:

d = #some digest
h1 = #first password hash
h2 = #second password hash
if d.digest(h1) == d.digest(h2)
  #success
else
  #fail
end

Execution time of computing a digest is independent of its input. This method was also proposed in Dan Boneh's cryptography course on Coursera, so I believe we can trust it. The issue with other popular methods (keeping a "running sum" of XORs of the individual bits) besides being sufficiently complicated is often that optimizing compilers might one day "see what we did there" and would "optimize" our efforts away :)

Martin Bosslet
emboss commented July 04, 2012

Execution time of computing a digest is independent of its input.

To clarify: It is now possible to time the resulting digests of course. But this cannot leak any information about the original values if we use a cryptographically secure digest in the process.

Michael Koziarski
Owner
NZKoz commented July 04, 2012

To clarify the purpose here, we're not looking at using this for passwords. The bcrypt gem does that much more transparently and simply than we could here.

The goal is instead to let us derive multiple keys from a single application secret.

Currently we have:

config.secret_token = "af60a5492cabefa4f84707919d84354c5f7a7cfcd978f977ff16af4aaea4678b14b48f705f6cff7cd86706fd837dd9e2d00fb06e9bd74fa2e615f8a1787faac1"

In every generated app, we use that for signing cookies and the cookie store. However we want to make it possible to ship an encrypted cookie store, which requires an encryption key and a secret.

We also want to make it possible for users to generate their own MessageVerifiers and MessageEncryptors.

On the advice of the google security guys, we don't want to use the same secret as both the encryption key. As a result we need a deterministic way to generate additional keys based on the secret and another value. That's the intended use of the KeyGenerator here.

When we sign cookies, we won't use the raw secret_token, we'll instead use:

  cookie_secret = Application.key_generator.derive_key(some_salt)

The salts will either be generated when your app is generated, or perhaps even just defaulting to static values like "cookie hmac secret", "cookie encryption key".

So, is that a bit clearer? Additionally, does that plan make any sense? ;)

Hiroshi Nakamura
nahi commented July 04, 2012

@NZKoz My comments and confirmations by @emboss are for key derivation, not directly for password authentication.

@emboss also stated about password authentication. It's also valid because password authentication is actually a key derivation; deriving authentication key (stretched password hash) from master key (password.) That said, I have no idea how Rails should use bcrypt and PBKDF2.

Ah, you meant that Application::Something#derive_key gets only salt to derive a new key because you're going to hide master key inside of Application, right? That would be fine.

Michael Koziarski
Owner
NZKoz commented July 04, 2012

@nahi exactly, take it as given that the secret is a securely generated master key which is secret. derive_key will be called with a salt which is either static, or configurable, for the sole purpose of deterministically generating keys to be used for other purposes in the application.

We can't use a random salt obviously, otherwise those keys are non-deterministic and we can't then re-validate cookies signed or encrypted with them.

Questions of timing attacks etc are moot as this only generates the keys, existing code in MessageVerifier does the hmac validation and that won't be changing.

Martin Bosslet
emboss commented July 05, 2012

@NZKoz Yeah, sorry for the confusion - I was under the impression that you were trying to replace bcrypt by PBKDF2. I should've read the title, too, not only the conversation ;) But as @nahi said, the same advice for PBKDF2 still holds for key derivation except for the comparison part because you no longer need to compare things.

A couple of questions about the master key. How is it derived? Is it some form of external input (a password?) or would it be generated within the application itself?

So that I get it right this time: you want to

  1. Create a master key
  2. Derive "session keys" deterministically from that master key in order to encrypt cookies or other things when needed?

If so, how are you planning to do deterministic derivation? That is, based on what information would you derive the key for let's say cookie encryption?

Michael Koziarski
Owner
NZKoz commented July 05, 2012

The master key itself is generated by SecureRandom when the application is generated. Users can change this as they wish.

The session key would be derived by providing another value as the salt to PBKDF2 with the master key as the 'password. The particular values would be probably be:

  key_generator = KeyGenerator.new(config.secret_token)
  cookie_signing_key = key_generator.generate_key(config.cookie_signing_salt || "Cookie Signing Key")

We could remove the default values, but that makes upgrading a pain in the ass. Essentially the main goal I have is that we have a reliable way to generate keys by transforming the existing secret so that we don't end up using a single as hmac, encryption key, everywhere.

using pbkdf2 to derive keys seems like a sane way to avoid that while simultaneously keeping things pretty straightforward for users.

Martin Bosslet
emboss commented July 07, 2012

I also discussed the issue with Jean-Philippe Aumasson. It was my impression and he confirmed that PBKDF2 is not the optimal choice in your context. PBKDF2 was designed to mitigate the poor entropy of passwords, but as you confirmed you would be using a SecureRandom master secret right from the start, there is no need for a password-based derivation algorithm.

I thought of something simple at first, like KDF 1-4 on this page, but Jean-Philippe warned me that there are issues when deriving keys in that manner. To overcome these problems, he recommended HMAC as a good solution.

It turns out that there is a standard using HMAC for key derivation that provides all of the features you are looking for, HKDF. It also comes with a paper.

In your situation, where you have a SecureRandom master key, you could skip the "Extract" phase altogether, and immediately use the "Expand" phase. Determinism is supported by providing appropriate "info" strings (see also the comments in section 3.2) to

HKDF-Expand(PRK, info, L)

As the underlying hash function, I would recommend to use SHA-256. This solution has the advantage that there's no need for salt values at all since your initial key material is already secure pseudo-random.

On the downside, there's no implementation in stdlib available so far. But I wanted to add key derivation support to Ruby OpenSSL for quite some time now, because it's also important for key exchange protocols.

@nahi Would you think it's OK to add HKDF support to Ruby OpenSSL even if it's not directly supported by OpenSSL itself? My guess is we could implement it entirely in Ruby, probably no need for native code.

Alternatively, you could use this gem or we could discuss adding a reduced version ("Expand" only) to Rails directly. What do you think?

Michael Koziarski
Owner
NZKoz commented July 08, 2012

I'm incredibly hesitant to add a dependency on crypto code that's not part of OpenSSL, it's hard enough to use crypto correctly, implementing it's a touch too risky. If there's an implementation of HKDF in a later version of OpenSSL then we could make use of it there.

So you mention that PBKDF2 is not ideal due to our keys already having sufficient entropy, if the sole issues is that the derivation will take an unnecessarily long time, then I'm not worried about it. We'll be generating a handful of keys per application, and doing it once. Are there additional concerns with our using pbkdf2 in this way?

Martin Bosslet
emboss commented July 09, 2012

Valid points - and nahi confirmed that we would only add HKDF support to Ruby OpenSSL if OpenSSL itself supports it one day.

Regarding PBKDF2, it's always a bit tricky if an algorithm is used in slightly different ways than intended.

I also do believe that the iteration count should be no concern. My main concern is rather how to handle the salts and how to get the determinism without weakening anything.

A different question - will the master key be (re-)generated on startup (and only kept in memory) or is it read from (and stored to) an external location? If so, are there any measures to protect it there or is this left to the user?

Michael Koziarski
Owner
NZKoz commented July 13, 2012

Sorry, real life intervened.

The master key is generated when the application is generated and it's up to the user to protect it beyond that. We have a task rake secret, but all that does is print a new random secret out to stdout. There's no KeyCzar style functionality for rotating those keys. However users can simply change it and rails will silently discard the old cookies / sessions. Previously it has raised exceptions however you get lots of false positives with mis-behaving bots and proxies truncating the headers.

My plan for the salts is that they're config values to the application with hard coded defaults. f.ex to sign cookies we may have a config value

  config.something.cookie_signature_salt = "7da55e95cab4feae54f0a8af22a4e0a1280b63b5d5055948066"

But if the value's not set by the user, simply default it to some known value like "org.rubyonrails.cookies.signature"?

Konstantin Haase
rkh commented July 16, 2012

Using a known salt is about as useful as using no salt, no?

Martin Bosslet
emboss commented July 16, 2012

@rkh In this case it doesn't add to the security, I would agree. But using a salt would make it possible to derive "sub keys" from the master key in a reproducible way.

Sorry, real life intervened.

No problem, it keeps doing that sometimes ;)

The salt itself can be treated as if it were public once established, but to prevent the ability to precompute values it should be selected randomly at some point. Instead of hardcoding them once and for all, would it be possible to set the individual values for cookie signing etc. at application generation time, together with the master key?

This part from PKCS#5 seems also good advice:

2) Otherwise, the salt should contain data that explicitly
distinguishes between different operations and different key
lengths, in addition to a random part that is at least eight
octets long, and this data should be checked or regenerated by
the party receiving the salt. For instance, the salt could have
an additional non-random octet that specifies the purpose of
the derived key. Alternatively, it could be the encoding of a
structure that specifies detailed information about the derived
key, such as the encryption or authentication technique and a
sequence number among the different keys derived from the
password. The particular format of the additional data is left
to the application.

Taking this into account, how about prefixing the salt with the purpose and then adding a random part? Something like

"org.rubyonrails.cookies.signature7da55e95cab4feae54f0a8af22a4e0a1280b63b5d5055948066"

The raw, (non-hex) salt should still be at least 8 bytes long, to prevent generating the same key twice for different applications:

2) It is unlikely that the same key will be selected twice.
Again, if the salt is 64 bits long, the chance of "collision"
between keys does not become significant until about 2^32 keys
have been produced, according to the Birthday Paradox. This
addresses some of the concerns about interactions between
multiple uses of the same key, which may apply for some
encryption and authentication techniques.

Since the original master key is already secure random, I would assume it's fine to stay modest with the iterations...

@nahi Would you agree?

Thai Duong

What we want is to provide a simple API for the developers (including Rails' core ones) to derive different keys from one single master key. I'd like to see something as simple as Application.key_generator.derive_key(info), where info might contain some non-random data to identify the derived key, e.g., "encrypted_cookie_store_key".

One simple way you can do is to use http://www.ruby-doc.org/stdlib-2.0/libdoc/openssl/rdoc/OpenSSL/PKCS5.html#method-c-pbkdf2_hmac with SHA256 as the hash function and pass the arguments as follows:

  • pass: the master key

  • salt: info that passed by the caller

  • iter: 1000. We don't need anything bigger here because the master key is already random.

  • keylen: 256 (= hashlen). This is enough key bits for all crypto operations.

This is actually a misuse of the OpenSSL-PBKDF2 API, but we really know what we are doing here for the following reasons:

1) In this settings PBKDF2-HMAC is very similar to HKDF. When keylen is equal to hashlen, HKDF would output T_1 computed as follows:

T_1 = HMAC-Hash(PRK, info | 0x01)

and PBKDF2-HMAC would output this T_1

F (P, S, c, i) = U_1 \xor U_2 \xor ... \xor U_c
T_1 = F (P, S, c, 1) ,

where

U_1 = PRF (P, S || INT (i)) ,
U_2 = PRF (P, U_1) ,
...
U_c = PRF (P, U{c-1}) .

Here, S is salt (non-random info in our case), c is the iteration counter and INT (i) is a four-octet encoding of the integer i, most significant octet first. You probably notice that HKDF's T_1 is actually PBKDF2-HMAC's U_1. In other words, the 1000 iteration count actually makes PBKDF2 stronger than HKDF.

2) The KDF used in SSL is super simple:

 key_block =
   MD5(master_secret + SHA(`A' + master_secret +
                           ServerHello.random +
                           ClientHello.random)) +
   MD5(master_secret + SHA(`BB' + master_secret +
                           ServerHello.random +
                           ClientHello.random)) +
   MD5(master_secret + SHA(`CCC' + master_secret +
                           ServerHello.random +
                           ClientHello.random)) + [...];

but it's Still Secure After All These Years (TM)!

3) Actually in practice people also use something as simple as HMAC(master_key, "0") and HMAC(master_key, "1") to derive different keys. It's okay because HMAC is a secure PRF, so as long as the master_key is random this would generate complete random keys.

So I guess we'll be fine with the approach I propose above. Anything more complex than it is probably overkill and might confuse developers.

Michael Koziarski Add ActiveSupport::KeyGenerator as a simple wrapper around PBKDF2
This will be used to derive keys from the secret and a salt, in order to allow us to
do things like encrypted cookie stores without using the secret for multiple
purposes directly.
def2ccb
Michael Koziarski
Owner

OK, I've updated this pull request as per @thaidn's helpful feedback, barring any objections in the next 24 hours I'll merge this in for 4.0 and begin the work of changing cookie / session store to derive the keys rather than using the bare secret.

Michael Koziarski Provide access to the application's KeyGenerator
Available both as an env entry for rack and an instance method on Rails::Application for other uses
0479bff
Michael Koziarski NZKoz merged commit 0a50792 into from October 02, 2012
Michael Koziarski NZKoz closed this October 02, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Showing 2 unique commits by 1 author.

Oct 01, 2012
Michael Koziarski Add ActiveSupport::KeyGenerator as a simple wrapper around PBKDF2
This will be used to derive keys from the secret and a salt, in order to allow us to
do things like encrypted cookie stores without using the secret for multiple
purposes directly.
def2ccb
Michael Koziarski Provide access to the application's KeyGenerator
Available both as an env entry for rack and an instance method on Rails::Application for other uses
0479bff
This page is out of date. Refresh to see the latest.
1  activesupport/lib/active_support.rb
@@ -48,6 +48,7 @@ module ActiveSupport
48 48
     autoload :Gzip
49 49
     autoload :Inflector
50 50
     autoload :JSON
  51
+    autoload :KeyGenerator
51 52
     autoload :MessageEncryptor
52 53
     autoload :MessageVerifier
53 54
     autoload :Multibyte
23  activesupport/lib/active_support/key_generator.rb
... ...
@@ -0,0 +1,23 @@
  1
+require 'openssl'
  2
+
  3
+module ActiveSupport
  4
+  # KeyGenerator is a simple wrapper around OpenSSL's implementation of PBKDF2
  5
+  # It can be used to derive a number of keys for various purposes from a given secret.
  6
+  # This lets rails applications have a single secure secret, but avoid reusing that
  7
+  # key in multiple incompatible contexts.
  8
+  class KeyGenerator
  9
+    def initialize(secret, options = {})
  10
+      @secret = secret
  11
+      # The default iterations are higher than required for our key derivation uses
  12
+      # on the off chance someone uses this for password storage
  13
+      @iterations = options[:iterations] || 2**16
  14
+    end
  15
+
  16
+    # Returns a derived key suitable for use.  The default key_size is chosen
  17
+    # to be compatible with the default settings of ActiveSupport::MessageVerifier.
  18
+    # i.e. OpenSSL::Digest::SHA1#block_length
  19
+    def generate_key(salt, key_size=64)
  20
+      OpenSSL::PKCS5.pbkdf2_hmac_sha1(@secret, salt, @iterations, key_size)
  21
+    end
  22
+  end
  23
+end
32  activesupport/test/key_generator_test.rb
... ...
@@ -0,0 +1,32 @@
  1
+require 'abstract_unit'
  2
+
  3
+begin
  4
+  require 'openssl'
  5
+  OpenSSL::PKCS5
  6
+rescue LoadError, NameError
  7
+  $stderr.puts "Skipping KeyGenerator test: broken OpenSSL install"
  8
+else
  9
+
  10
+require 'active_support/time'
  11
+require 'active_support/json'
  12
+
  13
+class KeyGeneratorTest < ActiveSupport::TestCase
  14
+  def setup
  15
+    @secret    = SecureRandom.hex(64)
  16
+    @generator = ActiveSupport::KeyGenerator.new(@secret, :iterations=>2)
  17
+  end
  18
+
  19
+  test "Generating a key of the default length" do
  20
+    derived_key = @generator.generate_key("some_salt")
  21
+    assert_kind_of String, derived_key
  22
+    assert_equal OpenSSL::Digest::SHA1.new.block_length, derived_key.length, "Should have generated a key of the default size"
  23
+  end
  24
+
  25
+  test "Generating a key of an alternative length" do
  26
+    derived_key = @generator.generate_key("some_salt", 32)
  27
+    assert_kind_of String, derived_key
  28
+    assert_equal 32, derived_key.length, "Should have generated a key of the right size"
  29
+  end
  30
+end
  31
+
  32
+end
11  railties/lib/rails/application.rb
@@ -101,6 +101,14 @@ def reload_routes!
101 101
       routes_reloader.reload!
102 102
     end
103 103
 
  104
+
  105
+    # Return the application's KeyGenerator
  106
+    def key_generator
  107
+      # number of iterations selected based on consultation with the google security
  108
+      # team. Details at https://github.com/rails/rails/pull/6952#issuecomment-7661220
  109
+      @key_generator ||= ActiveSupport::KeyGenerator.new(config.secret_token, :iterations=>1000)
  110
+    end
  111
+
104 112
     # Stores some of the Rails initial environment parameters which
105 113
     # will be used by middlewares and engines to configure themselves.
106 114
     # Currently stores:
@@ -121,7 +129,8 @@ def env_config
121 129
         "action_dispatch.show_exceptions" => config.action_dispatch.show_exceptions,
122 130
         "action_dispatch.show_detailed_exceptions" => config.consider_all_requests_local,
123 131
         "action_dispatch.logger" => Rails.logger,
124  
-        "action_dispatch.backtrace_cleaner" => Rails.backtrace_cleaner
  132
+        "action_dispatch.backtrace_cleaner" => Rails.backtrace_cleaner,
  133
+        "action_dispatch.key_generator" => key_generator
125 134
       })
126 135
     end
127 136
 
1  railties/test/application/configuration_test.rb
@@ -634,6 +634,7 @@ def index
634 634
       assert_equal      app.env_config['action_dispatch.show_exceptions'],   app.config.action_dispatch.show_exceptions
635 635
       assert_equal      app.env_config['action_dispatch.logger'],            Rails.logger
636 636
       assert_equal      app.env_config['action_dispatch.backtrace_cleaner'], Rails.backtrace_cleaner
  637
+      assert_equal      app.env_config['action_dispatch.key_generator'],     Rails.application.key_generator
637 638
     end
638 639
 
639 640
     test "config.colorize_logging default is true" do
Commit_comment_tip

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.