Move from keyczar to cryptography library for symmetric encryption / decryption #4165

Kami · 2018-06-06T14:57:04Z

This pull request moves our code for handling symmetric AES encryption and decryption from keyczar to cryptography library.

The change is fully backward and forward compatible as it's an implementation detail so it should in no way affect end users (and they shouldn't notice it either).

Background

When python-keyczar library was released, it was one of the best and most user-friendly crypto library with sane and safe defaults in the Python world.

Over the years the crypto world moved on (one of the notable moves forward was development of the cryptography library which was meant to be a more portable and safer / saner higher level replacement for PyCrypto, M2Crypto and other crypto libraries in the Python world).

Python-keyczar is still a relatively safe and good choice in the Python world, but one of the biggest problem is that it's not actively maintained / developed anymore and it doesn't support Python 3.

There are also some minor weakness in keyczar library, but nothing really major.

For us, the main reason for moving away from keyczar to cryptography is Python 3 support and active development and maintenance of cryptography library.

Implementation

The main goal behind this change was to make our crypto code work under Python 3 and to make it fully backward compatible aka don't require any user interaction / migration.

To be able to do that, I needed to re-implement keyczar AES Encrypt() and Decrypt() methods, but use the primitives from the cryptography library.

I'm never a fan or proponent of "reimplementing" crypto stuff, but in this case it was mostly limited to simple stuff such as safely generating IV string (using safe and recommended os.urandom), generating HMAC signature for the message and serializing the encrypted message (and vice-versa for decryption).

Even though that code is relatively simple and actual hard part such as encryption + signing is outsourced to cryptography, please perform a very thorough review / audit of the code to make sure I'm not doing anything stupid / unsafe.

In addition to that, I also needed to implement the code which knows how to read and handle JSON key files generated by keyczar which contain AES key and HMAC key.

While working on that, I also made a small change / improvement - I changed the default size for the AES and HMAC keys we generate from 128 to 256 bits.

Future Improvements

As mentioned above, re-implementing those two methods is not ideal.

In the future when we have a plan for a seamless migration in place, we should move to Fernet recipe in the cryptography library for handling symmetric encryption and decryption (https://cryptography.io/en/latest/fernet/).

Underneath, that recipe actually implements quite a similar algorithm to one used by keyczar (AES in CBC mode with SHA1 HMAC in keyczar case and AES in CBC mode with SHA256 HMAC in cryptography case), but it's better audited and maintained than our code (and that's always important when dealing with crypto / security sensitive code).

Keep in mind that the "seamless" migration in the future will at the very least require generating new secret key which will be used by cryptography fernet recipe.

With that change, we now have 90% or so of our unit tests running and passing under Python 3.6.

Resolves #3974

functions which utilize crypto primitives from cryptography library. We are moving from keyczar to cryptography, because keyczar doesn't support Python 3, but cryptography does. NOTE: Those functions utilize cryptography library primitives, but they follow the same approach as keyczar Encrypt() and Decrypt() methods so they are fully compatible with keyczar key files and keyczar encrypted / decrypted data.

primitives to safely generate a new random symmetric AES and HMAC key in keyczar compatible format.

functionality and correctness.

files.

Kami · 2018-06-06T14:57:53Z

test-requirements.txt

@@ -25,3 +25,4 @@ psutil==5.4.5
 webtest==2.0.25
 rstcheck>=3.3.0,<3.4
 pyrabbit
+python-keyczar


StackStorm doesn't depend on python-keyczar anymore, but we still needs it for tests where we check our code and output is fully compatible with keyczar one.

Can you add that in as a comment in the file?

Yep, will do.

I thought I already added it, but probably to the other file.

Now that our crypto code is Python 3 compatible, we can also run those tests under Python 3.

names.

lakshmi-kannan · 2018-06-07T19:27:01Z

Whoever wrote the original code with python-keyczar had right abstractions that changing libraries was almost easy :D. I like the changes.

…czar_to_cryptography_py3

bigmstone

This looks good to me. Especially added assertions that key is large enough. I would almost prefer this be its own library w/ the work you put into building backwards compatibility for keyczar JSON compatibility and reading that into AESKey. 👍

Kami · 2018-06-11T08:47:37Z

@bigmstone

I would almost prefer this be its own library w/ the work you put into building backwards compatibility for keyczar JSON compatibility and reading that into AESKey. +1

Yeah, I was actually thinking of making a new package, cryptography-keyczar-adapter or something along those lines.

It's worth nothing that keyczar offers a lot more functionality and APIs which we don't use and as such, it's not implemented here (asymmetric / public crypto, key rotation, etc). So the adapter is / would be specifically for symmetric AES crypto and reading keyczar compatible JSON key files :)

blag · 2018-06-12T01:09:13Z

st2common/st2common/util/crypto.py

-    :param key_type: Type of crypto key.
-    :type key_type: :class:`keyczar.keys.KeyType`
+    def __init__(self, aes_key_string, hmac_key_string, hmac_key_size, mode='CBC', size=128):
+        if mode not in 'CBC':


I think this should be:

if mode not in ['CBC']:

True, good catch 👍

blag · 2018-06-12T04:00:27Z

st2common/st2common/util/crypto.py

+        return AESKey(aes_key_string=aes_key_string, hmac_key_string=hmac_key_string,
+                      hmac_key_size=key_size, mode='CBC', size=key_size)
+
+    def __json__(self):


This is probably not a good idea at this point, since special methods starting and ending with double underscores are reserved for the Python interpreter (read: their names get mangled by CPython).

Is this for backwards compatibility with keyczar? That's really the only good reason I can think to use a dunder for this method.

I would rename this to something like to_json or _to_json if it needs to be private.

Yeah, I agree - I will change it to to_json (__json__ is kinda a left over from the simplejson library and for consistency since we use it in some other places).

blag · 2018-06-12T04:04:55Z

st2common/st2common/util/crypto.py

+    'cryptography_symmetric_encrypt',
+    'cryptography_symmetric_decrypt',
+
+    # NOTE: Keyczar functions are here for testing reasons - they are only used by tests


You don't need to export these explicitly if you tweak everything that imports them to dig them out of the module:

from st2common.util.crypto import keyczar_symmetric_encrypt keyczar_symmetric_encrypt(...)

can be rewritten as:

import st2common.util.crypto as crypto crypto.keyczar_symmetric_encrypt(...)

Importing each function separately is just a personal preference. I prefer it over module imports where module names are generic as this one.

As far as exporting all the public functions and using __all__ goes - that's just a "safety guard" and good practice I tend to follow. It makes it easier to see which public methods / classes a module exports and in case someone does "from foo import *", it will only include "public" methods. I know asterik imports are a bad idea and we should have a lint check for that, but still.

Yep, I like to import each function separately too. Although I think you can still import individual functions even if they aren't "exported" with __all__, so you may not even need to export those functions at all. Whatever you think is best. 😃

Yep, __all__ just affects asterisk import (*) import behavior :)

blag · 2018-06-12T04:06:49Z

st2common/tests/unit/test_crypto_utils.py

+from st2common.util.crypto import symmetric_encrypt
+from st2common.util.crypto import symmetric_decrypt
+from st2common.util.crypto import keyczar_symmetric_decrypt
+from st2common.util.crypto import keyczar_symmetric_encrypt


Can be rewritten to:

import st2common.util.crypto as crypto keyczar_symmetric_encrypt = crypto.keyczar_symmetric_encrypt

so you don't have to export keyczar_symmetric_encrypt.

Same story with keyczar_symmetric_decrypt. See my companion comment in st2common/utils/crypto.py.

See my comment above - personal preference and I think it's easier to glance at the top of the module to see what functions / classes a particular module imports and uses.

blag · 2018-06-12T04:09:45Z

st2common/st2common/util/crypto.py


-    :rtype: ``str``


I would raise a warning here instead of an error - UNLESS you are maintaining backwards compatibility.

Some users may want to import old (<128 bit) keys.

We never supported smaller keys so I think there is no need for that (our tool always generated 128 bit keys and AES doesn't support smaller keys anyway).

blag · 2018-06-12T04:10:58Z

st2common/st2common/util/crypto.py

+
+        :rtype: :class:`AESKey`
+        """
+        if key_size < 128:


You may also want to consider putting the minimum AES key size into its own constant to give the reader an idea of where the number 128 came from. Right now it's just a magic number to anybody who doesn't keep up on crypto recommendations.

Agreed, I will make a constant.

blag · 2018-06-12T04:24:49Z

st2common/st2common/util/crypto.py

+    NOTE: Header itself is unused, but it's added so the format is compatible with keyczar format.
+
+    """
+    assert isinstance(encrypt_key, AESKey), 'encrypt_key needs to be AESKey class instance'


These asserts look okay, just be aware that running Python with optimizations turned on bypasses all assertions made with the assert keyword. This has caused security issues in the past (example: PySAML).

Assertions are meant to be used as debugging aids, not as a mechanism for error handling.

It sucks, but you may want to use plain if statements for all of your assertions:

if not isinstance(plaintext, (six.text_type, six.string_types, six.binary_types)): raise CustomErrorType(...) # Not an AssertionError

More help: https://dbader.org/blog/python-assert-tutorial

Yeah, I used them for what they are intended - as debugging / developer aid aka to make sure programmer passes in a correct type (stuff which should get caught during tests and not during run time for which I would use exceptions).

For dynamic run time stuff I always use exceptions and not asserts.

My reasoning is that this code never gets a dynamic user data passed in and data is always passed in by a programmer so asserts make more sense since we are guarding against invalid use of the method by the programmer and not guarding a user potentially passing in invalid data.

blag · 2018-06-12T04:26:38Z

st2common/st2common/util/crypto.py

@@ -57,28 +308,128 @@ def symmetric_encrypt(encrypt_key, plaintext):
    'Initialization Vector' per run and the IV is part of the output.

    :param encrypt_key: Symmetric AES key to use for encryption.
-    :type encrypt_key: :class:`keyczar.keys.AesKey`
+    :type encrypt_key: :class:`keyczar.keys.AESKey`


This is close but not quite accurate. The class should be: st2common.utils.crypto.AESKey.

Good catch. Fixed.

blag · 2018-06-12T04:40:00Z

Definitely consider releasing it as its own package even if it only implements what we need from it. Other open source devs might be interested in it and implement what they need, to the point it becomes fairly complete. Heck, the cryptography developers themselves might be interested in absorbing it as an adapter.

I've appreciated keyczar but it's not really maintained anymore, so I welcome creating a backwards-compatible replacement/adapter for it.

Add changelog entry for #4165

Kami added 16 commits June 5, 2018 15:11

Fix shebang, always resolve path to full absolute path.

43d4c08

Add cryptography dependency.

3df2e6d

Update st2-generate-symmetric-crypto-key utilitu to use cryptography

dcd778e

primitives to safely generate a new random symmetric AES and HMAC key in keyczar compatible format.

Add back symmetric_encrypt and symmetric_decrypt alias functions.

9c58713

Update affected tests.

060e997

Add tests which verify symmetric_encrypt and symmetric_decrypt functions

554725b

functionality and correctness.

Add tests for keyczar compatible key file generation.

1526530

Add more tests, skip keyczar tests under Python 3.

f4caf64

Add keyczar test key fixtures.

d56f467

Throw on invalid mode and unsafe key size.

4a75060

Add tests for loading keys from keyczar compatible key files.

6e7934d

Also add test case for loading 128 bit aes keys.

e5645a3

Keyczar is now only a test dependency, re-generate requirements.txt

9bf6041

files.

Update help string.

bf02406

Various Python 3 compatibility fixes.

3f44c1a

Kami added the python3 label Jun 6, 2018

Kami added this to the 2.8.0 milestone Jun 6, 2018

Kami commented Jun 6, 2018

View reviewed changes

Run more tests under Python 3.

bae269a

Now that our crypto code is Python 3 compatible, we can also run those tests under Python 3.

Kami requested review from lakshmi-kannan, blag and bigmstone June 7, 2018 07:34

Kami added 3 commits June 7, 2018 09:36

Rephrase docstring.

d894bee

Improve docstrings, add additional asserts, use consistent variable

70f63c4

names.

Add more tests.

9be8ed8

Kami changed the title ~~[WIP] Move from keyczar to cryptography library for symmetric encryption / decryption~~ Move from keyczar to cryptography library for symmetric encryption / decryption Jun 7, 2018

Fix lint.

d1ba3b8

lakshmi-kannan approved these changes Jun 7, 2018

View reviewed changes

Kami added 2 commits June 8, 2018 09:39

Merge branch 'master' of github.com:StackStorm/st2 into move_from_key…

60426cf

…czar_to_cryptography_py3

Add a comment on why that requirement is there.

8f20eed

bigmstone approved these changes Jun 8, 2018

View reviewed changes

Merge branch 'master' into move_from_keyczar_to_cryptography_py3

5e0c29d

blag suggested changes Jun 12, 2018

View reviewed changes

Kami added 5 commits June 12, 2018 10:02

Fix mode check.

3c5c381

Rename __json__() method to to_json().

16a080b

Update invalid docstring.

fe48f38

Use a constant for minimum AES key size.

9e9ce8b

Add tests for AESKey class constructor.

6259ef4

blag approved these changes Jun 12, 2018

View reviewed changes

Merge branch 'master' into move_from_keyczar_to_cryptography_py3

5955e5c

Kami merged commit e3eef09 into master Jun 13, 2018

Kami deleted the move_from_keyczar_to_cryptography_py3 branch June 13, 2018 08:11

Kami added a commit that referenced this pull request Jun 13, 2018

Add changelog entry for #4165.

ebbb868

Kami mentioned this pull request Jun 13, 2018

Add changelog entry for #4165 #4178

Merged

Kami added a commit that referenced this pull request Jun 18, 2018

Merge pull request #4178 from StackStorm/add_changelog_entry_4165

ad49b74

Add changelog entry for #4165

arm4b mentioned this pull request Dec 1, 2023

Delete python-keyczar==0.716 #6078

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move from keyczar to cryptography library for symmetric encryption / decryption #4165

Move from keyczar to cryptography library for symmetric encryption / decryption #4165

Kami commented Jun 6, 2018 •

edited

Loading

Kami Jun 6, 2018

lakshmi-kannan Jun 7, 2018

Kami Jun 8, 2018

lakshmi-kannan commented Jun 7, 2018

bigmstone left a comment

Kami commented Jun 11, 2018

blag Jun 12, 2018

Kami Jun 12, 2018

blag Jun 12, 2018

Kami Jun 12, 2018 •

edited

Loading

blag Jun 12, 2018

Kami Jun 12, 2018

blag Jun 12, 2018

Kami Jun 12, 2018 •

edited

Loading

blag Jun 12, 2018

Kami Jun 12, 2018

blag Jun 12, 2018

Kami Jun 12, 2018 •

edited

Loading

blag Jun 12, 2018

Kami Jun 12, 2018

blag Jun 12, 2018

Kami Jun 12, 2018 •

edited

Loading

blag Jun 12, 2018

Kami Jun 12, 2018 •

edited

Loading

blag commented Jun 12, 2018

Move from keyczar to cryptography library for symmetric encryption / decryption #4165

Move from keyczar to cryptography library for symmetric encryption / decryption #4165

Conversation

Kami commented Jun 6, 2018 • edited Loading

Background

Implementation

Future Improvements

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lakshmi-kannan commented Jun 7, 2018

bigmstone left a comment

Choose a reason for hiding this comment

Kami commented Jun 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kami Jun 12, 2018 • edited Loading

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

Kami Jun 12, 2018 • edited Loading

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

Kami Jun 12, 2018 • edited Loading

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

Kami Jun 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kami Jun 12, 2018 • edited Loading

Choose a reason for hiding this comment

blag commented Jun 12, 2018

Kami commented Jun 6, 2018 •

edited

Loading

Kami Jun 12, 2018 •

edited

Loading

Kami Jun 12, 2018 •

edited

Loading

Kami Jun 12, 2018 •

edited

Loading

Kami Jun 12, 2018 •

edited

Loading

Kami Jun 12, 2018 •

edited

Loading