nimblehq / android-templates

Our optimized Android templates used in our projects
https://nimblehq.co/
MIT License
97 stars 23 forks source link

Avoid defining app credentials via BuildConfig #188

Closed luongvo closed 1 year ago

luongvo commented 2 years ago

Issue

Currently, we're having a sample to define BASE_API_URL via BuildConfig. BuildConfig will not be obfuscated by proguard so this way is not safe to go, especially when we have more secrets like API_KEY or API_SECRET. We should provide them through a separate class which will be obfuscated in each flavor folder instead.

Solution

By having 2 environments as default: production and staging, we will need to define 2 new ApiEndpointUrl classes in each flavor.

image

Who Benefits?

Developers

I think this is essential because app credentials management is very important.

What's Next?

luongvo commented 2 years ago

@Tuubz added reason to be essential ticket

sleepylee commented 2 years ago

@luongvo you might want to have a quick check again, simply read a field from BuildConfig, package an APK and then run jadx to reverse it. It seems like if we are not defining

-keep class com.example.BuildConfig { *; } in proguard file, it will be obfuscated like this:

image

Seems like it works nicely these days with proguard/R8. If so, we should continue to keep this practice.

sleepylee commented 2 years ago

or maybe it worked with the conversion from gradle.properties -> gradle.app -> BuildConfig -> app

https://medium.com/@abhishekdubey_70837/using-gradle-to-secure-app-keys-in-android-9d2796ca4007

the expected outcome is that the plain string must not be visible, either using the class way or this way 🆗

sleepylee commented 2 years ago

with the class obfuscation, only the var/function name is obfuscated FYI Screen Shot 2022-04-26 at 13 00 07

manh-t commented 2 years ago

@luongvo I have checked by decompiling the APK and the result is the same with @sleepylee . 🙏

The classes/ method is obfuscated only, not the value 😅

sleepylee commented 2 years ago

@manh-t it's public repository so we better not leaking client's detail.

manh-t commented 2 years ago

@sleepylee oops, I will censor the image, thanks for reminding 🙇

toby-thanathip commented 2 years ago

I can confirm that both BuildConfig & ApiEndpointUrl only obfuscate the function name and not the value itself 🙏

Screen Shot 2565-04-29 at 11 22 27

However, I do have a preference for ApiEndpointUrl (Ex: Secrets.kt):

@luongvo Any thoughts? In case you agree, would you mind rewriting the description of this issue? 🚀

sleepylee commented 2 years ago

@Tuubz did you try the solution I provided above? It seems that I and @manh-t receive a new result here with BuildConfig: both the name + value are obfuscated ⭐

toby-thanathip commented 2 years ago

@sleepylee Oh, seems like I misunderstood then

Anyways, I tried the following for CoroutineTemplate:

  1. Enable isMinifyEnabled & isShrinkResources to true for debug build
  2. Move BASE_API_URL in to gradle.properties
  3. Adapt build.gradle.kts so BASE_API_URL is used: buildConfigField("String", "BASE_API_URL", baseApiUrl)
  4. Generate an APK
  5. Run jadx on the APK
  6. Open up the result in a text-editor
  7. Search references for BASE_API_URL

Did I miss something here? 🙇

sleepylee commented 2 years ago

@Tuubz that sounds correct and the value URL is supposed to be obfuscated as well with this approach 🤔 Not sure what could be missing from your side, so could you please zip the codebase (remove your build/ part to minimize the size) and post it here for us to investigate?

toby-thanathip commented 2 years ago

@sleepylee @manh-t I tried this on CoroutineTemplate could you guys do the same? 🙏

If the value is still obfuscated on your side, then something is definitely missing in the code.

Or perhaps something is wrong with my jadx, when I run it it says: finished with errors, count: 27 🙈

manh-t commented 2 years ago

@sleepylee unfortunately, Toby and I got the same result 💸 Although I didn't keep BuildConfig in proguard, it doesn't obfuscate the BuildConfig's value as the image I provided above. I'm using Worpt project for demonstration 🙏 Not sure if there is any extra config from your side, we'd love to know that 🙏

  • And this is from BuildConfig Screen Shot 2022-04-27 at 15 31 53
toby-thanathip commented 2 years ago

Continue discussion here 👋

Short summary:

toby-thanathip commented 2 years ago

@luongvo Our plan is to go with the BuildConfig + properties approach instead, what do you think?

If you agree, I can create a new issue and reject this one

luongvo commented 1 year ago

This issue is now invalid and should be closed