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: encrypt stored sdk config #275

Closed
wants to merge 6 commits into from
Closed

Conversation

mrehan27
Copy link
Contributor

Background

Following the Slack discussion, we realized that we should encrypt the SDK config to prevent possible reverse engineering and unauthorized extraction of keys.

Changes

  • Utilized EncryptedSharedPreferences for secure storage of the SDK config
  • Renamed the existing preferences file for the SDK config to avoid corrupt values
  • Disabled backup for encrypted preferences, as recommended in the Android docs.

Testing

  • Native SDK users will not experience any impact, as they initialize the SDK in the Application class, eliminating the need for auto-initialization
  • For existing wrapper customers, the SDK won't be auto-initialized from the killed state until the app is launched after this update. As soon as the app is launched, stored config values will be updated, and everything will proceed as expected.
  • To test auto backup changes, enable allowBackup in the app and run the following command:
adb shell bmgr backupnow <app-package-name>

@mrehan27 mrehan27 requested a review from a team October 31, 2023 13:30
@mrehan27 mrehan27 self-assigned this Oct 31, 2023
Copy link

github-actions bot commented Oct 31, 2023

Sample app builds 📱

Below you will find the list of the latest versions of the sample apps. It's recommended to always download the latest builds of the sample apps to accurately test the pull request.


  • java_layout: rehan/encrypt-store-prefs (1698759129)
  • kotlin_compose: rehan/encrypt-store-prefs (1698759138)

@mrehan27 mrehan27 changed the title fix: encrypt sdk config fix: encrypt stored sdk config Oct 31, 2023
Copy link

codecov bot commented Oct 31, 2023

Codecov Report

Merging #275 (9a32ba8) into main (c494bb0) will decrease coverage by 29.56%.
Report is 20 commits behind head on main.
The diff coverage is n/a.

@@              Coverage Diff              @@
##               main     #275       +/-   ##
=============================================
- Coverage     49.76%   20.20%   -29.56%     
+ Complexity      237       27      -210     
=============================================
  Files           108       19       -89     
  Lines          2781      480     -2301     
  Branches        364       99      -265     
=============================================
- Hits           1384       97     -1287     
+ Misses         1282      371      -911     
+ Partials        115       12      -103     

see 89 files with indirect coverage changes

Copy link

Build available to test
Version: rehan-encrypt-store-prefs-SNAPSHOT
Repository: https://s01.oss.sonatype.org/content/repositories/snapshots/

Copy link
Contributor

@levibostian levibostian left a comment

Choose a reason for hiding this comment

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

I have a couple questions about this PR before merging. I'll reach out with my questions shortly.

Copy link
Contributor

@Shahroz16 Shahroz16 left a comment

Choose a reason for hiding this comment

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

Since it passes the manual test I am good with the change.

Comment on lines +6 to +7
// Using alpha version as current stable version (1.0.0) has minSdkVersion 23
internal const val ANDROIDX_SECURITY_CRYPTO = "1.1.0-alpha06"
Copy link
Contributor

@levibostian levibostian Oct 31, 2023

Choose a reason for hiding this comment

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

Because 1.1-alpha releases support minSdk 21 while 1.0 supports minSdk 23, do we anticipate that 1.1 stable will move to minSdk 21?

I would like to avoid us having to bump our minSdk because of this dependency.

longer be present when backup is restored -->
<exclude
domain="sharedpref"
path="io.customer.sdk.EncryptedConfigCache.xml" />
Copy link
Contributor

@levibostian levibostian Oct 31, 2023

Choose a reason for hiding this comment

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

Note: No need to act on this yet, until my original questions are discussed.

Does this hard-coded string need to match this hard-coded string?

If so, I suggest using a string resource value to store this string in 1 place.

@mrehan27
Copy link
Contributor Author

mrehan27 commented Nov 2, 2023

Closing this PR based on our discussion on Slack. We'll continue exploring improvements for this and revisit later.

@mrehan27 mrehan27 closed this Nov 2, 2023
@mrehan27 mrehan27 deleted the rehan/encrypt-store-prefs branch November 3, 2023 09:38
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.

3 participants