Geoconfig and API settings models: AES CBC encryption read and write #35642
Conversation
…reading both ecb and cbc encrypted
…se cbc decryption if it is prefixed with "aes-cbc"
…e reencryption process
…g both ecb and cbc encrypted
Jtang-1
requested review from
jingcheng16,
a team,
AddisonDunn,
minhaminha,
Robert-Costello and
esoergel
and removed request for
a team and
AddisonDunn
| try: | ||
| plaintext = b64_aes_decrypt(ciphertext) | ||
| except UnicodeDecodeError: | ||
| raise AesEcbDecryptionError("Failed to decrypt the AES-ECB-encrypted text.") |
There was a problem hiding this comment.
Just curious, in what case we might fail to decrypt it?
There was a problem hiding this comment.
I'm not sure why. On staging (and only staging, not prod) ConnectionSettings contains encrypted passwords that after being decrypted, raises a UnicodeDecodeError when attempting to decode it (which happens within b4_aes_decrypt. But since it's already unusable (so probably hasn't been used), I'm raising this error so I can set the new value to '' during the migration.
There was a problem hiding this comment.
Did you print those encrypted passwords, are they start with the ECB prefix? I'm worried something goes wrong when we encrypt the ConnectionSettings pwd.
There was a problem hiding this comment.
the existing passwords that were failing do start with the aes prefix.
I'm worried something goes wrong when we encrypt the ConnectionSettings pwd.
I'm not following - what's the concern you have?
|
|
||
| new_ciphertext = b64_aes_cbc_encrypt(b64_aes_decrypt(ciphertext)) | ||
| try: | ||
| plaintext = b64_aes_decrypt(ciphertext) |
There was a problem hiding this comment.
If there is no existing_prefix, then it means the text is not encrypted, so we should not decrypt it right? Can this try except block moved to the first if block, when text starts with existing_prefix?
There was a problem hiding this comment.
If there is no existing_prefix, then it means the text is not encrypted, so we should not decrypt it right?
No that's not the case. The expectation is that the encrypted_text passed to reencrypt_ecb_to_cbc_mode is encrypted. The existing_prefix is only to handle situations where the encrypted text is prefixed such that the prefix can be stripped.
There was a problem hiding this comment.
Or maybe we don't need existing_prefix at all? Shouldn't this function be able to take in any text, determine if and how is encrypted then do whatever needs to return a cbc encrypted string?
There was a problem hiding this comment.
Is there a case that the text is encrypted without any prefix?
There was a problem hiding this comment.
Is there a case that the text is encrypted without any prefix?
Yes, it depends on the model.
If we remove the existing_prefix, then we would need to handle stripping prefix on the higher level function that calls reencrypt_ecb_to_cbc_mode and pass only the encrypted text. Is that what you're suggesting?
corehq/motech/models.py
Outdated
| return json.loads(plaintext) | ||
| if self.last_token_aes.startswith(f'${ALGO_AES_CBC}$'): | ||
| ciphertext = self.client_secret.split('$', 2)[2] | ||
| return b64_aes_cbc_decrypt(ciphertext) |
There was a problem hiding this comment.
Should the result of b64_aes_cbc_decrypt() be passed to json.loads() similar to what is done with the result of b64_aes_decrypt() below?
Is there a test that covers this code?
There was a problem hiding this comment.
Yes it does, thanks for catching that and also for reminding me to add tests.. it caught a couple other bugs as well. bc3d6a6
…etting and getting last token
Product Description
N/A
Technical Summary
Continuation of the work initiated by this PR
Jira Ticket
Introduces a solution for these code scan alerts:
https://github.com/dimagi/commcare-hq/security/code-scanning/295
https://github.com/dimagi/commcare-hq/security/code-scanning/296
This PR updates
GeoConfig'sapi_token, andConnectionSettings'spassword,client_secret, andlast_token_aes. The model continues supporting reads for both AES ECB and AES CBC encryption while writes only with CBC encryption.Feature Flag
no FF
Safety Assurance
Safety story
locally tested. This change will not affect any existing data and existing data will be read the same way.
Automated test coverage
There exists tests that the encryption and decryption with CBC results in the expected plaintext.
QA Plan
no QA
Migrations
There is no migration in this PR. But there is a follow up PR that does a migration related to the changes in this PR.
Related migration
Rollback instructions
This PR will start writing data with AES CBC encryption while also adding support to read AES CBC encrypted data. If this PR is reverted, the data encrypted with EBC will fail to be decrypted. To rollback this PR, data written with AES CBC encryption will need to be reencrypted with EBC.
Labels & Review