search

tink-crypto / tink

Tink is a multi-language, cross-platform, open source library that provides cryptographic APIs that are secure, easy to use correctly, and hard(er) to misuse.
https://developers.google.com/tink
Apache License 2.0
13.47k stars 1.18k forks source link

Missing classes dectected while running R8 #702

Closed GuilhE closed 1 year ago

GuilhE commented 1 year ago

Describe the bug:

I'm having the following problem when isMinifyEnabled = true and running :app:minifyDevDebugWithR8

Missing classes detected while running R8. Please add the missing classes or apply additional keep rules that are generated in .../missing_rules.txt. Missing class com.google.api.client.http.GenericUrl (referenced from: java.lang.String com.google.crypto.tink.util.KeysDownloader.fetchAndCacheData()) Missing class com.google.api.client.http.HttpHeaders (referenced from: java.lang.String com.google.crypto.tink.util.KeysDownloader.fetchAndCacheData() and 1 other context) Missing class com.google.api.client.http.HttpRequest (referenced from: java.lang.String com.google.crypto.tink.util.KeysDownloader.fetchAndCacheData()) Missing class com.google.api.client.http.HttpRequestFactory (referenced from: java.lang.String com.google.crypto.tink.util.KeysDownloader.fetchAndCacheData()) Missing class com.google.api.client.http.HttpResponse (referenced from: java.lang.String com.google.crypto.tink.util.KeysDownloader.fetchAndCacheData()) Missing class com.google.api.client.http.HttpTransport (referenced from: com.google.api.client.http.HttpTransport com.google.crypto.tink.util.KeysDownloader$Builder.httpTransport and 6 other contexts) Missing class com.google.api.client.http.javanet.NetHttpTransport$Builder (referenced from: void com.google.crypto.tink.util.KeysDownloader.()) Missing class com.google.api.client.http.javanet.NetHttpTransport (referenced from: com.google.api.client.http.javanet.NetHttpTransport com.google.crypto.tink.util.KeysDownloader.DEFAULT_HTTP_TRANSPORT and 3 other contexts) Missing class org.joda.time.Instant (referenced from: long com.google.crypto.tink.util.KeysDownloader.getCurrentTimeInMillis())

What was the expected behavior?

Success R8 when isMinifyEnabled = true

How can we reproduce the bug?

Honestly I don't know, I've upgraded to JDK17, gradle-8.0-bin.zip, kotlin 1.8.21 and android-tools 33/34. Didn't had this problem earlier. Could be a false positive or a gradle problem, I don't know 😞 , the thing is, all errors point to: referenced from: java.lang.String com.google.crypto.tink...

Do you have any debugging information?

Provide your version information:

tholenst commented 1 year ago

Hey, thanks for the report.

What version of Tink do you use?

In general, the dependencies referenced (http-client and joda-time) are needed for the class com.google.crypto.tink.util.KeysDownloader -- and we didn't list them in early versions of Tink. We later added them, but they really shouldn't be there: unless you use exactly KeysDownloader directly, Tink doesn't need them.

Are you using tink-android? They shouldn't be there.

GuilhE commented 1 year ago

I'm not using Tink directly, only via androidx.security:security-crypto:1.1.0-alpha06

tholenst commented 1 year ago

I see, thank you.

I will have to contact this team then. Did you already file an issue for androidx.security? If so, I will reuse that.

Also, did you try different versions of androidx.security? If so, what was the result?

morambro commented 1 year ago

androidx.security:security-crypto:1.1.0-alpha06 depends on com.google.crypto.tink:tink-android:1.8.0 (source) which doesn't explicitly depend on http-client (source). I am not very familiar with R8, but if IIUC adding -dontwarn com.google.api.client.http.* and -dontwarn org.joda.time.* to theproguard-rules.pro file (https://developer.android.com/build/shrink-code#configuration-files) should solve the issue.

tholenst commented 1 year ago

Oh I see. And somehow in 1.8 we included the keys downloader by mistake :(

https://github.com/tink-crypto/tink-java/blob/1.8/BUILD.bazel#L696-L699

This should be fixed with 1.9 though: https://github.com/tink-crypto/tink-java/blob/1.9/BUILD.bazel#L780-L782

So what remains to do is for someone to try to see if androidx.security:security-crypto can update to 1.9

GuilhE commented 1 year ago

androidx.security:security-crypto:1.1.0-alpha06 depends on com.google.crypto.tink:tink-android:1.8.0 (source) which doesn't explicitly depend on http-client (source). I am not very familiar with R8, but if IIUC adding -dontwarn com.google.api.client.http.* and -dontwarn org.joda.time.* to theproguard-rules.pro file (https://developer.android.com/build/shrink-code#configuration-files) should solve the issue.

This doesn't work, by adding:

-dontwarn com.google.api.client.http.**
-dontwarn org.joda.time.**

The build will succeed (because it mutes the warnings/errors) but then it will fail in runtime:

java.lang.ClassCastException: java.lang.Class cannot be cast to java.lang.reflect.ParameterizedType at retrofit2.HttpServiceMethod.parseAnnotations(HttpServiceMethod.java:46) at retrofit2.ServiceMethod.parseAnnotations(ServiceMethod.java:39)
at retrofit2.Retrofit.loadServiceMethod(Retrofit.java:202)
at retrofit2.Retrofit$1.invoke(Retrofit.java:160)
at java.lang.reflect.Proxy.invoke(Proxy.java:1006)
at $Proxy3.login(Unknown Source)
...

I'm using EncryptedSharedPreferences to store some credentials.

Oh I see. And somehow in 1.8 we included the keys downloader by mistake :(

https://github.com/tink-crypto/tink-java/blob/1.8/BUILD.bazel#L696-L699

This should be fixed with 1.9 though: https://github.com/tink-crypto/tink-java/blob/1.9/BUILD.bazel#L780-L782

So what remains to do is for someone to try to see if androidx.security:security-crypto can update to 1.9

Is it something I can test on my end?

I will have to contact this team then. Did you already file an issue for androidx.security? If so, I will reuse that.

I have not, I'll so you can reuse 😉

tholenst commented 1 year ago

You may be able to test it if you tell gradle explicitly that you need at least Tink 1.9. I don't know Gradle at all, so I cannot help you with how to do that.

But basically, if you can say explicitly that you need com.google.crypto.tink:tink-android:1.9.0 the issue should go away.

I filed an issue in the Google internal bug tracker, I hope that they can soon upgrade androidx.security:security-crypto:1.1.0-alpha06. I didn't find a public component for this.

Please wait a week and then Ping this issue if there is no new release.

GuilhE commented 1 year ago

I filed an issue in the Google internal bug tracker, I hope that they can soon upgrade androidx.security:security-crypto:1.1.0-alpha06. I didn't find a public component for this.

Thanks, I was indeed trying to find on Google Issue Tracker but I couldn't also, only for generic AndroidX.

You may be able to test it if you tell gradle explicitly that you need at least Tink 1.9. I don't know Gradle at all, so I cannot help you with how to do that.

But basically, if you can say explicitly that you need com.google.crypto.tink:tink-android:1.9.0 the issue should go away.

This makes a lot of sense, and the way to do that is by doing the following:

implementation("androidx.security:security-crypto:1.1.0-alpha06") {
     exclude(group = "com.google.crypto.tink", module = "tink-android")
}
implementation("com.google.crypto.tink:tink-android:1.9.0")

And it works 🥳

But now I end up again with the runtime error which made me think it was a false positive. A bit of research and I found that either way I need to add the following rules:

# Keep generic signature of Call, Response (R8 full mode strips signatures from non-kept items).
-keep,allowobfuscation,allowshrinking interface retrofit2.Call
-keep,allowobfuscation,allowshrinking class retrofit2.Response

# With R8 full mode generic signatures are stripped for classes that are not
# kept. Suspend functions are wrapped in continuations where the type argument
# is used.
-keep,allowobfuscation,allowshrinking class kotlin.coroutines.Continuation

This leaves me with 2 solutions:

Which do you think is the cleanest/safest? I prefer the second because as soon as the security-crypto uses officially the 1.9.0 I just need to remove the gradle configuration. Having the 2 new rules added sounds like code smell 😅

tholenst commented 1 year ago

Can you expand on the runtime error? A stacktrace would be helpful.

Please do try to use 1.9.0 -- in the end, we do fix a problem with 1.9.0 over 1.8.0, and if there is another problem we need to debug that one too. We will upgrade to 1.9.0 anyhow, and if it doesn't work then you cannot avoid it in the medium term.

GuilhE commented 1 year ago

The runtime error was a false positive towards Tink, the stacktrace was relative to coroutines and retrofit, thus the 3 proguard rules that also saved the day 🙏🏼

tholenst commented 1 year ago

I will mark this as finished as you said it was a false positive. I will also do another release as in Java Tink 1.9.0 still has some problems with regard to this class. However, if anyone has still issues with this, please comment here or open a new issue in https://github.com/tink-crypto/tink-java