Skip to content

Fix "cryptography_symmetric_encrypt" function and make sure it handles unicode values correctly #4528

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.

Sign up for GitHub

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

Merged
Kami merged 2 commits into from
Feb 1, 2019
Merged

Fix "cryptography_symmetric_encrypt" function and make sure it handles unicode values correctly #4528

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
78bd99b
Fix "cryptography_symmetric_encrypt" function and make sure we correctly
Kami Jan 31, 2019
35de7b6
Add changelog entry for #4528.
Kami Jan 31, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion CHANGELOG.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,11 @@ Fixed
``st2 action-alias execute 'pack install xxx'``. #4511

Contributed by Nick Maludy (Encore Technologies)

* Fix datastore value encryption and make sure it also works correctly for unicode (non-ascii)
values.

Reported by @dswebbthg, @nickbaum. (bug fix) #4513 #4527 #4528

2.10.0 - December 13, 2018
--------------------------

Expand Down
14 changes: 8 additions & 6 deletions st2common/st2common/util/crypto.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,8 +216,14 @@ def cryptography_symmetric_encrypt(encrypt_key, plaintext):
assert isinstance(aes_key_bytes, six.binary_type)
assert isinstance(hmac_key_bytes, six.binary_type)

if isinstance(plaintext, (six.text_type, six.string_types)):
# Convert data to bytes
data = plaintext.encode('utf-8')
else:
data = plaintext

# Pad data
data = pkcs5_pad(plaintext)
data = pkcs5_pad(data)

# Generate IV
iv_bytes = os.urandom(KEYCZAR_AES_BLOCK_SIZE)
Expand All @@ -230,10 +236,6 @@ def cryptography_symmetric_encrypt(encrypt_key, plaintext):
# bytes) so we simply add 5 0's
header_bytes = b'00000'

if isinstance(data, (six.text_type, six.string_types)):
# Convert data to bytes
data = data.encode('utf-8')

ciphertext_bytes = encryptor.update(data) + encryptor.finalize()
msg_bytes = header_bytes + iv_bytes + ciphertext_bytes

Expand Down Expand Up @@ -368,7 +370,7 @@ def pkcs5_pad(data):
Pad data using PKCS5
"""
pad = KEYCZAR_AES_BLOCK_SIZE - len(data) % KEYCZAR_AES_BLOCK_SIZE
data = data + pad * chr(pad)
data = data + pad * chr(pad).encode('utf-8')
return data


Expand Down
26 changes: 26 additions & 0 deletions st2common/tests/unit/test_crypto_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
# -*- coding: utf-8 -*-
# Licensed to the StackStorm, Inc ('StackStorm') under one or more
# contributor license agreements. See the NOTICE file distributed with
# this work for additional information regarding copyright ownership.
Expand Down Expand Up @@ -53,6 +54,31 @@ def setUpClass(cls):
super(CryptoUtilsTestCase, cls).setUpClass()
CryptoUtilsTestCase.test_crypto_key = AESKey.generate()

def test_symmetric_encrypt_decrypt_short_string_needs_to_be_padded(self):
original = u'a'
crypto = symmetric_encrypt(CryptoUtilsTestCase.test_crypto_key, original)
plain = symmetric_decrypt(CryptoUtilsTestCase.test_crypto_key, crypto)
self.assertEqual(plain, original)

def test_symmetric_encrypt_decrypt_utf8_character(self):
values = [
u'£',
Copy link
Contributor

@blag blag Jan 31, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the u syntax is supported in Python 3. Better to use six.u() so we can run our tests with Python 3.

Copy link
Member Author

@Kami Kami Jan 31, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is - u as in prefix for strings which is also a default (e.g. u'foo).

Those tests also run under Python 2.x and 3.6 on Travis and I also ran them under both versions locally.

Copy link
Contributor

@blag blag Jan 31, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, good to know.

u'£££',
u'££££££',
u'č š hello đ č p ž Ž',
u'hello 💩',
u'💩💩💩💩💩'
u'💩💩💩',
u'💩😁'
]

for index, original in enumerate(values):
crypto = symmetric_encrypt(CryptoUtilsTestCase.test_crypto_key, original)
plain = symmetric_decrypt(CryptoUtilsTestCase.test_crypto_key, crypto)
self.assertEqual(plain, original)

self.assertEqual(index, (len(values) - 1))

def test_symmetric_encrypt_decrypt(self):
original = 'secret'
crypto = symmetric_encrypt(CryptoUtilsTestCase.test_crypto_key, original)
Expand Down