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

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

Merged
merged 1 commit into from Oct 4, 2017

Conversation

Projects
None yet
3 participants
@jmchilton
Copy link
Member

commented Sep 27, 2017

Generally more validation and more testing as well. Test cases layout the reasons for some of these changes.

Prevents bugs like #4710. Ensure CSRF tokens won't break existing Galaxies regardless of how long id_secret people have chosen (unless they chose one that would have broken normal generation of keys).

@@ -11,6 +11,11 @@

log = logging.getLogger(__name__)

MAXIMUM_ID_SECRET_BITS = 448
MAXIMUM_ID_SECRET_LENGTH = MAXIMUM_ID_SECRET_BITS / 8

This comment has been minimized.

Copy link
@martenson

martenson Sep 27, 2017

Member

Is this guaranteed? 1 byte per character? Always ascii? In the sample we say 'any string'

This comment has been minimized.

Copy link
@jmchilton

jmchilton Sep 27, 2017

Author Member

well it isn't going to be smaller than this right? I'll try an unpatriotic string and see what happens.

This comment has been minimized.

Copy link
@jmchilton

jmchilton Sep 27, 2017

Author Member

Well - I guess the code is correct the docs are wrong:

======================================================================
ERROR: test_security_helper.test_maximum_length_handling_nonascii
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/john/.pyenv/versions/2.7.12/lib/python2.7/site-packages/nose/case.py", line 197, in runTest
    self.test(*self.arg)
  File "/Users/john/workspace/galaxy/test/unit/test_security_helper.py", line 46, in test_maximum_length_handling_nonascii
    helper = security.SecurityHelper(id_secret=longest_id_secret)
  File "/Users/john/workspace/galaxy/lib/galaxy/web/security/__init__.py", line 49, in __init__
    self.id_cipher = Blowfish.new(self.id_secret)
  File "/Users/john/.pyenv/versions/2.7.12/lib/python2.7/site-packages/Crypto/Cipher/Blowfish.py", line 101, in new
    return BlowfishCipher(key, *args, **kwargs)
  File "/Users/john/.pyenv/versions/2.7.12/lib/python2.7/site-packages/Crypto/Cipher/Blowfish.py", line 65, in __init__
    blockalgo.BlockAlgo.__init__(self, _Blowfish, key, *args, **kwargs)
  File "/Users/john/.pyenv/versions/2.7.12/lib/python2.7/site-packages/Crypto/Cipher/blockalgo.py", line 141, in __init__
    self._cipher = factory.new(key, *args, **kwargs)
UnicodeEncodeError: 'ascii' codec can't encode characters in position 0-55: ordinal not in range(128)

This comment has been minimized.

Copy link
@jmchilton

jmchilton Sep 27, 2017

Author Member

Pushed a clarification of this into galaxy.ini.sample in this branch - thanks for catch @martenson.

This comment has been minimized.

Copy link
@martenson

martenson Sep 27, 2017

Member

I do not think that is correct though. I can use č as my secret_id and Galaxy runs fine.

This comment has been minimized.

Copy link
@jmchilton

jmchilton Sep 27, 2017

Author Member

Hmm... but my test...

This comment has been minimized.

Copy link
@jmchilton

jmchilton Sep 27, 2017

Author Member

Fine you win, it seems if you have a non-ascii character the maximum length is more like 448 / 24. I pulled out language describing that - since I'm not sure I understand it myself. But I do have test cases that clarify this is the boundary and that the _longest_bits is working properly even with long (though not as long) non-ASCII strings.

This comment has been minimized.

Copy link
@jmchilton

jmchilton Sep 27, 2017

Author Member

Or is this just because my test is wrong:

>>> "◎" * (24 / 3 + 1)
'\xe2\x97\x8e\xe2\x97\x8e\xe2\x97\x8e\xe2\x97\x8e\xe2\x97\x8e\xe2\x97\x8e\xe2\x97\x8e\xe2\x97\x8e\xe2\x97\x8e'

Ugh...

This comment has been minimized.

Copy link
@jmchilton

jmchilton Sep 27, 2017

Author Member

Updated the test case again. I think everything in here is right - it just unicode handling under Python 2 is a bit garbage. Having these test cases will hopefully help us when migrating though.

"""
last_bits = secret
if len(secret) > MAXIMUM_ID_SECRET_LENGTH:
last_bits = secret[-MAXIMUM_ID_SECRET_LENGTH:]

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Oct 3, 2017

Member

This won't work on Python3, were the length of a string containing UTF-8 characters is different from the number of bytes, e.g.:

$ python2
Python 2.7.6 (default, Jun 22 2015, 17:58:13) 
[GCC 4.8.2] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> secret = "◎◎◎◎◎◎◎◎◎◎◎◎◎◎◎◎◎◎◎"
>>> print len(secret)
57
>>> 
$ python3
Python 3.4.3 (default, Nov 17 2016, 01:08:31) 
[GCC 4.8.4] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> secret = "◎◎◎◎◎◎◎◎◎◎◎◎◎◎◎◎◎◎◎"
>>> print(len(secret))
19
>>> 

I'd rewrite these lines as:

    last_bits = smart_str(secret)
    if len(last_bits) > MAXIMUM_ID_SECRET_LENGTH:
        last_bits = last_bits[-MAXIMUM_ID_SECRET_LENGTH:]

and add

from galaxy.util import smart_str

in the import section.

This comment has been minimized.

Copy link
@jmchilton

jmchilton Oct 3, 2017

Author Member

@nsoranzo Thanks - what do you think about 7f84f8e instead so it is more clear we are only modifying id_secret if it is too many bits?

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Oct 3, 2017

Member

7f84f8e would work only if we can guarantee that secret is always of type str in Python2:

$ python
Python 2.7.6 (default, Jun 22 2015, 17:58:13) 
[GCC 4.8.2] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> from Crypto.Cipher import Blowfish
>>> from galaxy.util import smart_str
>>> MAXIMUM_ID_SECRET_LENGTH = 56
>>> secret = u"◎◎◎◎◎◎◎◎◎◎◎◎◎◎◎◎◎◎"
>>> print type(secret)
<type 'unicode'>
>>> print len(secret)
18
>>> last_bits = secret
>>> as_str = smart_str(secret)
>>> len(as_str)
54
>>> len(as_str) > MAXIMUM_ID_SECRET_LENGTH
False
>>> Blowfish.new(last_bits)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/users/ga002/soranzon/software/nsoranzo_galaxy/.venv/local/lib/python2.7/site-packages/Crypto/Cipher/Blowfish.py", line 101, in new
    return BlowfishCipher(key, *args, **kwargs)
  File "/usr/users/ga002/soranzon/software/nsoranzo_galaxy/.venv/local/lib/python2.7/site-packages/Crypto/Cipher/Blowfish.py", line 65, in __init__
    blockalgo.BlockAlgo.__init__(self, _Blowfish, key, *args, **kwargs)
  File "/usr/users/ga002/soranzon/software/nsoranzo_galaxy/.venv/local/lib/python2.7/site-packages/Crypto/Cipher/blockalgo.py", line 141, in __init__
    self._cipher = factory.new(key, *args, **kwargs)
UnicodeEncodeError: 'ascii' codec can't encode characters in position 0-17: ordinal not in range(128)

This comment has been minimized.

Copy link
@jmchilton

jmchilton Oct 3, 2017

Author Member

Okay - I'll go with your variant as long you promise smart_str isn't going to break anyone's existing values. It looks like existing unicode secrets wouldn't work without this tweak anyway so that probably isn't an issue?

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Oct 3, 2017

Member
$ python
Python 2.7.6 (default, Jun 22 2015, 17:58:13) 
[GCC 4.8.2] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> from Crypto.Cipher import Blowfish
>>> from galaxy.util import smart_str
>>> secret = "◎◎◎◎◎◎◎◎◎◎◎◎◎◎◎◎◎◎"
>>> b_old = Blowfish.new(secret)
>>> s = 'foobar12'
>>> old_enc_s = b_old.encrypt(s)
>>> b_new = Blowfish.new(smart_str(secret))
>>> print b_new.encrypt(s) == old_enc_s
True

This comment has been minimized.

Copy link
@jmchilton

jmchilton Oct 4, 2017

Author Member

Fair enough - sorry for being dense. I've rebased it all into one commit including this change.

Better hanlding of long id secrets when generating per-kind encryptio…
…n keys.

Generally more validation and more testing as well. Test cases layout the reasons for some of these changes.

Thanks to @nsoranzo for the Python 3 fix.

@jmchilton jmchilton force-pushed the jmchilton:id_secret_fix branch from 7f84f8e to fb05863 Oct 4, 2017

@martenson martenson merged commit c3f7f39 into galaxyproject:release_17.09 Oct 4, 2017

6 checks passed

api test Build finished. 292 tests run, 4 skipped, 0 failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
framework test Build finished. 161 tests run, 0 skipped, 0 failed.
Details
integration test Build finished. 46 tests run, 0 skipped, 0 failed.
Details
lgtm analysis: JavaScript No alert changes
Details
toolshed test Build finished. 579 tests run, 0 skipped, 0 failed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.