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.

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

EmailSettings password encryption improvement #35403

Merged
merged 15 commits into from
Dec 9, 2024
Merged

EmailSettings password encryption improvement #35403

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
4a6e8b2
adds helper function to encrypt and decrypt with aes, cbc mode
Jtang-1 Nov 19, 2024
9afcca4
Add new fields in EmailSettings
Jtang-1 Nov 19, 2024
49ec705
write password also to new password_cbc field
jingcheng16 Nov 19, 2024
c7d40e8
copy and reencrypt ecb encrypted passwords to cbc password field
Jtang-1 Nov 19, 2024
75f7831
update test
Jtang-1 Nov 19, 2024
0820feb
add comment to note field will be replaced
Jtang-1 Nov 19, 2024
ac20dac
change strategy to migrating existing password field to add prefex af…
Jtang-1 Dec 5, 2024
0751ac4
bug: handle case where existing value is an empty string
Jtang-1 Dec 5, 2024
fb4c15a
make migration idempotent
Jtang-1 Dec 5, 2024
15ed1c1
add a revert migration
Jtang-1 Dec 6, 2024
a3d6bf2
bug: need to handle situations where there is no prefix
Jtang-1 Dec 6, 2024
1601646
refactor to use query filter
Jtang-1 Dec 6, 2024
3ef5ff8
don't do migration yet as the the migrated data is dependent on code …
Jtang-1 Dec 9, 2024
0a94aa1
correct function name typo
Jtang-1 Dec 9, 2024
b260ec9
update test to use email_esttings plaintext_password
Jtang-1 Dec 9, 2024
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
19 changes: 17 additions & 2 deletions corehq/apps/email/migrations/0003_emailsettings_password_cbc.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

from corehq.motech.const import ALGO_AES, ALGO_AES_CBC
from corehq.util.django_migrations import skip_on_fresh_install
from corehq.motech.utils import reencrypt_ecb_to_cbc_mode
from corehq.motech.utils import reencrypt_ecb_to_cbc_mode, reencrypt_cbc_to_ecb_mode


@skip_on_fresh_install
Expand All @@ -17,12 +17,27 @@ def copy_and_reencrypt_password_to_password_cbc(apps, schema_editor):
email_settings.save()


def revert_password_cbc_to_password(apps, schema_editor):
EmailSettings = apps.get_model('email', 'EmailSettings')

for email_settings in EmailSettings.objects.all():
if email_settings.password.startswith(f'${ALGO_AES}$'):
continue

if email_settings.password.startswith(f'${ALGO_AES_CBC}$'):
prefix = f'${ALGO_AES_CBC}$'
else:
prefix = None
email_settings.password = reencrypt_cbc_to_ecb_mode(email_settings.password, prefix)
email_settings.save()


class Migration(migrations.Migration):

dependencies = [
('email', '0002_emailsettings_return_path_email'),
]

operations = [
migrations.RunPython(copy_and_reencrypt_password_to_password_cbc, migrations.RunPython.noop),
migrations.RunPython(copy_and_reencrypt_password_to_password_cbc, revert_password_cbc_to_password),
]
19 changes: 18 additions & 1 deletion corehq/motech/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
from Crypto.Util.py3compat import bord
from Crypto.Random import get_random_bytes

from corehq.motech.const import AUTH_PRESETS, OAUTH2_PWD, ALGO_AES_CBC
from corehq.motech.const import AUTH_PRESETS, OAUTH2_PWD, ALGO_AES_CBC, ALGO_AES

AES_BLOCK_SIZE = 16
AES_KEY_MAX_LEN = 32 # AES key must be either 16, 24, or 32 bytes long
Expand Down Expand Up @@ -149,6 +149,23 @@ def reencrypt_ecb_to_cbc_mode(encrypted_text, existing_prefix=None):
return f'${ALGO_AES_CBC}${new_ciphertext}'


# Only needed for migration revert from CBC to ECB mode.
def reencrypt_cbc_to_ecb_mode(encrypted_text, existing_prefix=None):
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is only referenced in the migration, what do you think of moving it into the migration file? (And removing it from this PR since the migration has been removed.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm open to that, but this same function will be needed for migration of other models (in future PRs). Does it make sense to copy this same function into each migration file?

Copy link
Contributor

@millerdev millerdev Dec 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, I think so. Migration files should avoid importing things from other modules as much as possible because they should be frozen-in-time rather than depending on things that may be refactored and change in other ways over time. Granted, this function is probably unlikely to change over time, but I still think it's safer to include it in each migration file that needs it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense, how would you advise i unit test the function in that case? I don't think i can import functions defined within a migration file

"""
Re-encrypt a message that was encrypted using CBC mode to ECB mode.
"""
if not encrypted_text:
return encrypted_text

if existing_prefix and encrypted_text.startswith(existing_prefix):
ciphertext = encrypted_text[len(existing_prefix):]
else:
ciphertext = encrypted_text

new_ciphertext = b64_aes_encrypt(b64_aes_cbc_decrypt(ciphertext))
return f'${ALGO_AES}${new_ciphertext}'


def unpad(bytestring):
"""
Unpad will remove padding from the right of a string that has been
Expand Down