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

fix(all): Add fallback logic for corrupt keys to EncryptedKeyValueRepository #2686

Merged
merged 7 commits into from
Feb 7, 2024

Conversation

mattcreaser
Copy link
Member

  • PR title and description conform to Pull Request guidelines.

Issue #, if available:
#2681
#2684
#2548
#2616
#2510

Description of changes:
This PR aims to implement workarounds for corrupted master keys causing crashes when attempting to open the encrypted shared preferences.

Certain OEM devices have unreliable KeyStore implementations that can occasionally corrupt their key material. When this happens the keys become unusable, and the encrypted preferences are therefore unretrievable. The workaround is to catch this error, create a new key to use, and create a new set of encrypted preferences. Please see the Google tracker bug and tink issue for more details.

NB: This is destructive, as a corrupt key means we can never recover any previously stored data.

At a high level, the logic in this change can be described as follows:

  • For existing users that have repositories encrypted using the default master key there will be no change, such repositories will continue to use this key as normal.
  • Any new repositories created will instead use an amplify-specific master key to derive their key material.
  • If the default master key becomes corrupted then we delete the preferences instance and create a new one using the amplify master key.
  • If the amplify master key becomes corrupted then we delete and recreate the key, then delete and recreate the preferences instance using the new key.

How did you test these changes?
I simulated various keystore errors by manually throwing exceptions using a debugger. I tried various scenarios like an existing user upgrading to a new version without errors, an existing user encountering a corrupt default master key, and corrupt amplify master keys. I tried having an open repository and deleting the master key in a different instance.

Documentation update required?

  • No
  • Yes (Please include a PR link for the documentation update)

General Checklist

  • Added Unit Tests
  • Added Integration Tests
  • Security oriented best practices and standards are followed (e.g. using input sanitization, principle of least privilege, etc)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@mattcreaser mattcreaser requested a review from a team as a code owner January 23, 2024 16:44
Copy link
Member

@tylerjroach tylerjroach left a comment

Choose a reason for hiding this comment

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

A few general comments and before approval I'd like to do some manual test checks but this looks great overall.

tylerjroach
tylerjroach previously approved these changes Jan 31, 2024
@codecov-commenter
Copy link

Codecov Report

Attention: 51 lines in your changes are missing coverage. Please review.

Comparison is base (d229be4) 42.65% compared to head (38e5cff) 42.81%.
Report is 2 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2686      +/-   ##
==========================================
+ Coverage   42.65%   42.81%   +0.16%     
==========================================
  Files         905      905              
  Lines       29026    29088      +62     
  Branches     4131     4140       +9     
==========================================
+ Hits        12382    12455      +73     
+ Misses      15294    15277      -17     
- Partials     1350     1356       +6     

@mattcreaser mattcreaser enabled auto-merge (squash) February 7, 2024 17:57
@mattcreaser mattcreaser merged commit 2ee5eb2 into main Feb 7, 2024
9 checks passed
@mattcreaser mattcreaser deleted the mattcreaser/encrypted-prefs-crash branch February 7, 2024 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants