Merge pull request #4713 from jmchilton/id_secret_fix

[17.09] Better handling of long id secrets when generating per-kind encryption keys.

config/galaxy.ini.sample

@@ -978,7 +978,7 @@ use_interactive = True
 # Galaxy encodes various internal values when these values will be output in
 # some format (for example, in a URL or cookie).  You should set a key to be
 # used by the algorithm that encodes and decodes these values.  It can be any
-# string.
+# string up to 448 bits long.
 # One simple way to generate a value for this is with the shell command:
 #   python -c 'import time; print time.time()' | md5sum | cut -f 1 -d ' '
 #id_secret = USING THE DEFAULT IS NOT SECURE!

lib/galaxy/web/security/__init__.py

@@ -3,14 +3,21 @@ import os
 import os.path
 import logging
 
-import galaxy.exceptions
-
 from Crypto.Cipher import Blowfish
 from Crypto.Util.randpool import RandomPool
 from Crypto.Util import number
 
+import galaxy.exceptions
+
+from galaxy.util import smart_str
+
 log = logging.getLogger(__name__)
 
+MAXIMUM_ID_SECRET_BITS = 448
+MAXIMUM_ID_SECRET_LENGTH = MAXIMUM_ID_SECRET_BITS / 8
+KIND_TOO_LONG_MESSAGE = "Galaxy coding error, keep encryption 'kinds' smaller to utilize more bites of randomness from id_secret values."
+
+
 if os.path.exists("/dev/urandom"):
     # We have urandom, use it as the source of random data
     random_fd = os.open("/dev/urandom", os.O_RDONLY)
@@ -37,7 +44,8 @@ else:
 class SecurityHelper(object):
 
     def __init__(self, **config):
-        self.id_secret = config['id_secret']
+        id_secret = config['id_secret']
+        self.id_secret = id_secret
         self.id_cipher = Blowfish.new(self.id_secret)
 
         per_kind_id_secret_base = config.get('per_kind_id_secret_base', self.id_secret)
@@ -127,4 +135,15 @@ class _cipher_cache(collections.defaultdict):
         self.secret_base = secret_base
 
     def __missing__(self, key):
-        return Blowfish.new(self.secret_base + "__" + key)
+        assert len(key) < 15, KIND_TOO_LONG_MESSAGE
+        secret = self.secret_base + "__" + key
+        return Blowfish.new(_last_bits(secret))
+
+
+def _last_bits(secret):
+    """We append the kind at the end, so just use the bits at the end.
+    """
+    last_bits = smart_str(secret)
+    if len(last_bits) > MAXIMUM_ID_SECRET_LENGTH:
+        last_bits = last_bits[-MAXIMUM_ID_SECRET_LENGTH:]
+    return last_bits

test/unit/test_security_helper.py

@@ -1,3 +1,5 @@
+# -*- coding: utf-8 -*-
+
 from galaxy.web import security
 
 
@@ -5,6 +7,64 @@ test_helper_1 = security.SecurityHelper(id_secret="sec1")
 test_helper_2 = security.SecurityHelper(id_secret="sec2")
 
 
+def test_maximum_length_handling_ascii():
+    # Test that id secrets can be up to 56 characters long.
+    longest_id_secret = "m" * security.MAXIMUM_ID_SECRET_LENGTH
+    helper = security.SecurityHelper(id_secret=longest_id_secret)
+    helper.encode_id(1)
+
+    # Test that security helper will catch if the id secret is too long.
+    threw_exception = False
+    longer_id_secret = "m" * (security.MAXIMUM_ID_SECRET_LENGTH + 1)
+    try:
+        security.SecurityHelper(id_secret=longer_id_secret)
+    except Exception:
+        threw_exception = True
+
+    assert threw_exception
+
+    # Test that different kinds produce different keys even when id secret
+    # is very long.
+    e11 = helper.encode_id(1, kind="moo")
+    e12 = helper.encode_id(1, kind="moo2")
+
+    assert e11 != e12
+
+    # Test that long kinds are rejected because it uses up "too much" randomness
+    # from id_secret values. This isn't a strict requirement up but lets just enforce
+    # the best practice.
+    assertion_error_raised = False
+    try:
+        helper.encode_id(1, kind="this is a really long kind")
+    except AssertionError:
+        assertion_error_raised = True
+
+    assert assertion_error_raised
+
+
+def test_maximum_length_handling_nonascii():
+    longest_id_secret = "◎◎◎◎◎◎◎◎◎◎◎◎◎◎◎◎◎◎"
+    helper = security.SecurityHelper(id_secret=longest_id_secret)
+    helper.encode_id(1)
+
+    # Test that security helper will catch if the id secret is too long.
+    threw_exception = False
+    longer_id_secret = "◎◎◎◎◎◎◎◎◎◎◎◎◎◎◎◎◎◎◎"
+    try:
+        security.SecurityHelper(id_secret=longer_id_secret)
+    except Exception:
+        threw_exception = True
+
+    assert threw_exception
+
+    # Test that different kinds produce different keys even when id secret
+    # is very long.
+    e11 = helper.encode_id(1, kind="moo")
+    e12 = helper.encode_id(1, kind="moo2")
+
+    assert e11 != e12
+
+
 def test_encode_decode():
     # Different ids are encoded differently
     assert test_helper_1.encode_id(1) != test_helper_1.encode_id(2)