Geoconfig and API settings: AES CBC encryption Migration

[Jtang-1]

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 migrates GeoConfig's api_token, and ConnectionSettings's password, client_secret, and last_token_aes. For values that are AES ECB encrypted, this migration reencrypts it with AES CBC. For values that are not encrypted, this migration encrypts it with AES CBC. In both cases, the prefix "$aes-cbc$" is prepended to the encrypted value and saved.

Feature Flag

no FF

Safety Assurance

Safety story

locally tested and will test on staging.

Automated test coverage

There exists tests that reencrypt_ecb_to_cbc_mode decrypts to the same plaintext value.

QA Plan

no QA

Migrations

• The migrations in this code can be safely applied first independently of the code
  The migration in this PR is related to this past PR and should not be merged until that PR is deployed.

Rollback instructions

• This PR can be reverted after deploy with no further considerations

Labels & Review

• Risk label is set correctly
• The set of people pinged as reviewers is appropriate for the level of risk of the change

[Jtang-1]

This PR is dependent on #35642 and should not be merged until after that PR is deployed.

[jingcheng16]

This three functions can be one function, the logic is the same, and you can pass connection.password, connection.client_secret, connection.last_token_aes to it.

[jingcheng16]

This can be further simplified after you bring some logic to reencrypt_ecb_to_cbc_mode in #35642

[Jtang-1]

I combined the function for password and client_secret here 903d12f. But last_token_aes needs to be handled differently. The difference being that the stored value is always encrypted but may or may not have the aes-cbc prefix. For password and client_secret, if the value does not contain the prefix, then it is not encrypted. To keep these two lines of logic cleanly separated, I would prefer to keep the functions separate.

[jingcheng16]

Thanks for the change. What do you think of combining the logic, and have a new parameter called reencrypt_only, reencrypt_only is True for last_token_aes.

[Jtang-1]

hmm, like this? I have mixed feelings about it, but don't have a strong preference. I can make this change

def _reencrypt_or_encrypt_value_with_cbc(value, reencrypt_only=False):
    if value == '':
        return ''
    if value.startswith(f'${ALGO_AES_CBC}$'):
        return value

    try:
        if value.startswith(f'${ALGO_AES}$'):
            return reencrypt_ecb_to_cbc_mode(value, f'${ALGO_AES}$')
        else:
            if reencrypt_only:
                return reencrypt_ecb_to_cbc_mode(value)
            else:
                ciphertext = b64_aes_cbc_encrypt(value)
                return f'${ALGO_AES_CBC}${ciphertext}'
    except AesEcbDecryptionError:
        return ''

[Jtang-1]

c322f58

[millerdev]

Should these _reencrypted_... functions also check for .startswith(f'${ALGO_AES_CBC}$') and skip re-encrypting the value if that returns true?

[Jtang-1]

Yes it should. Added this commit to check if the value already contains a aes-cbc prefix and just returns the value if it does. ee61d0f

[jingcheng16]

Looks good to me