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.

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

Encrypt API Key #35471

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Encrypt API Key #35471

wants to merge 7 commits into from

Conversation

Jtang-1
Copy link
Contributor

@Jtang-1 Jtang-1 commented Dec 3, 2024

Product Description

N/A

Technical Summary

USH-3047

The original plan was to use PBKDF2 algorithm as it is Django's default for passwords. However, there exists a use where the API key needs to be viewable. This prevents us from using hashing since it is non-reversible.

The solution here uses AES in CBC mode to encrypt the API key. Other API keys and passwords in the system already use this AES. However, CBC mode is used instead of ECB because ECB is considered a weak algorithm (This PR beings addressing that issue)

The plaintext key will temporarily continue being stored in the database until we verify that this transition to an encrypted key is successful. I have added logging to log an error if the encrypted key fails. In cases of failure, it will fallback to using the stored plaintext key.

Sanity check was done for performance. From this Datadog Dashboard, api/v0.5/odata/cases is the highest frequency and peaks at 1500 request over 5mins. Summing all the peaks results in ~8500 requests over 5 minutes.

The statistics to decrypt 8500 keys on staging are shown below and indicate there is no performance concern.
Average time: 0.000041 seconds
Minimum time: 0.000033 seconds
Maximum time: 0.001398 seconds

Feature Flag

no FF

Safety Assurance

Safety story

Tested locally and will test on staging. The plaintext key will not yet be deleted and we will fall back to the using that value if there's failure with getting the decrypted value.

Automated test coverage

There exists tests that checks creating an HQApiKey and using its stored plaintext key for authentication.

QA Plan

no QA

Migrations

  • The migrations in this code can be safely applied first independently of the code

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

@dimagimon dimagimon added reindex/migration Reindex or migration will be required during or before deploy Risk: High Change affects files that have been flagged as high risk. labels Dec 3, 2024
@Jtang-1 Jtang-1 marked this pull request as ready for review December 4, 2024 00:21
@Jtang-1 Jtang-1 requested review from a team and minhaminha and removed request for a team December 4, 2024 00:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
reindex/migration Reindex or migration will be required during or before deploy Risk: High Change affects files that have been flagged as high risk.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants