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

Trust user-added CAs on Android #461

Open
gnprice opened this issue Dec 23, 2023 · 11 comments · Fixed by #474 or #671
Open

Trust user-added CAs on Android #461

gnprice opened this issue Dec 23, 2023 · 11 comments · Fixed by #474 or #671
Labels
a-Android Issues specific to Android, or requiring Android-specific work a-first-hour Issues specific to using the app for the first time beta feedback Things beta users have specifically asked for
Milestone

Comments

@gnprice
Copy link
Member

gnprice commented Dec 23, 2023

This is a configuration change that was requested a few times over the years for zulip-mobile, as zulip/zulip-mobile#3312, before we got a contribution implementing it as zulip/zulip-mobile#5493 . We should do the same thing here.

The implementation for zulip-mobile (see zulip/zulip-mobile@85c3a71) is entirely at the Android metadata layer, so we should be able to pretty much just copy-paste that.

@gnprice gnprice added a-Android Issues specific to Android, or requiring Android-specific work a-first-hour Issues specific to using the app for the first time labels Dec 23, 2023
@gnprice gnprice added this to the Beta 2 milestone Dec 23, 2023
@gnprice gnprice added the beta feedback Things beta users have specifically asked for label Dec 23, 2023
gnprice added a commit to gnprice/zulip-flutter that referenced this issue Jan 4, 2024
This is copied from the corresponding config in zulip-mobile,
which was added in the following commit:
  zulip/zulip-mobile@85c3a71

Unlike in zulip-mobile, though, we don't need any adjustments
in debug (or profile) builds, because we don't use the
`android:usesCleartextTraffic` attribute which this would clobber.

(React Native debug builds reach out to the development host over
cleartext HTTP in order to support hot updates; but the Flutter
development tools have the host contact the app instead, using the
Dart VM protocol.  It's also not clear that any Dart libraries
respect the `android:usesCleartextTraffic` flag anyway.)

Fixes: zulip#461
@gnprice gnprice self-assigned this Jan 4, 2024
@gnprice gnprice closed this as completed in cffb112 Jan 4, 2024
@gnprice
Copy link
Member Author

gnprice commented Jan 9, 2024

This feature is in the new beta release v0.0.9. @shrizza please give it a try — I'd be glad to have confirmation of whether it works for you.

@gnprice
Copy link
Member Author

gnprice commented Jan 10, 2024

Seems like that version doesn't yet work — but there is hope that a small further tweak to the manifest will make it work. Details in chat:
https://chat.zulip.org/#narrow/stream/48-mobile/topic/flutter.3A.20user-added.20certs/near/1716835

@gnprice gnprice reopened this Jan 10, 2024
@gnprice gnprice modified the milestones: Beta 2, B2: Summer 2024 May 9, 2024
@gnprice gnprice assigned gnprice and unassigned gnprice May 13, 2024
@gnprice
Copy link
Member Author

gnprice commented May 13, 2024

Rereading the thread, it looks like the next step is to make that small further tweak to our manifest, and then put that in the next beta release so @shrizza can try it out. I think we have another beta release (v0.0.15) coming up this week, so I'll aim to include this there.

(The last round of this was just before I went travelling for part of a week and then got covid; I guess by the time I was back at full speed, this had fallen off my radar.)

@gnprice
Copy link
Member Author

gnprice commented May 13, 2024

I guess by the time I was back at full speed, this had fallen off my radar

Oho, partly due to this GitHub bug/gap/misfeature:
image
image

So when we originally marked the issue fixed, that caused its "status" to become "Done" in the GitHub "project" we've been using to manage tasks for the Flutter beta app. But then when I reopened it a few days later, that didn't cause the "status" to change back — the project still saw it as "Done", until I happened to notice just last week.

As a result, the issue was absent from the project board we've been watching. So the "fallen off my radar" metaphor was unusually apt here — this issue literally did not appear on the display we were regularly checking to spot tasks we should handle next.

Anyway, glad we've now caught it.

@shrizza
Copy link

shrizza commented May 14, 2024

I appreciate the proactive follow-up, and look forward to the next release.

gnprice added a commit to gnprice/zulip-flutter that referenced this issue May 14, 2024
The previous attempt at this (cffb112, zulip#474) set the configuration
used by the HTTP implementation from the Android SDK, and by some
third-party HTTP implementations.  But in the bulk of the app's code,
we use the HTTP implementation from the Dart standard library, and
by default that does not consult the Android configuration.

Happily, Flutter has an opt-in feature to apply the same configuration
to the Dart standard library:
  https://chat.zulip.org/#narrow/stream/48-mobile/topic/flutter.3A.20user-added.20certs/near/1716845

So use that.

Fixes: zulip#461
chrisbobbe pushed a commit that referenced this issue May 14, 2024
The previous attempt at this (cffb112, #474) set the configuration
used by the HTTP implementation from the Android SDK, and by some
third-party HTTP implementations.  But in the bulk of the app's code,
we use the HTTP implementation from the Dart standard library, and
by default that does not consult the Android configuration.

Happily, Flutter has an opt-in feature to apply the same configuration
to the Dart standard library:
  https://chat.zulip.org/#narrow/stream/48-mobile/topic/flutter.3A.20user-added.20certs/near/1716845

So use that.

Fixes: #461
@gnprice
Copy link
Member Author

gnprice commented May 16, 2024

Well, unfortunately it looks like I'm going to have to revert the take-2 fix I made as #671.

After that PR, in a release build on Android, the app crashes at startup with SIGSEGV and a stack trace like:

#00 pc 00000000006b39e8  /apex/com.android.art/lib64/libart.so (art::JNI<false>::GetStringChars(_JNIEnv*, _jstring*, unsigned char*)+232) (BuildId: 7ece79c15d80914c83e60c9e93ac1684)
#01 pc 00000000000ddbd8  /system/lib64/libandroid_runtime.so (android::android_content_XmlBlock_nativeGetAttributeIndex(_JNIEnv*, _jobject*, long, _jstring*, _jstring*)+72) (BuildId: 2c0c526c96b2f0f37ce26c968792b5fe)
#02 pc 0000000000331474  /data/misc/apexdata/com.android.art/dalvik-cache/arm64/boot.oat (art_jni_trampoline+116)
#03 pc 00000000008bc6d4  /data/misc/apexdata/com.android.art/dalvik-cache/arm64/boot.oat (android.content.res.XmlBlock$Parser.getAttributeBooleanValue+84)
#04 pc 00000000005ba754  /apex/com.android.art/lib64/libart.so (nterp_helper+7636) (BuildId: 7ece79c15d80914c83e60c9e93ac1684)
#05 pc 000000000021525e  /data/app/~~OdBMxcLjRbZre3jUFwqhtA==/com.zulip.flutter-Ivlz_Plsw-9nCNlt4y7GhA==/base.apk (y4.a.c+58)
#06 pc 000000000033b680  /apex/com.android.art/lib64/libart.so (art_quick_invoke_static_stub+640) (BuildId: 7ece79c15d80914c83e60c9e93ac1684)
#07 pc 0000000000511714  /apex/com.android.art/lib64/libart.so (bool art::interpreter::DoCall<false>(art::ArtMethod*, art::Thread*, art::ShadowFrame&, art::Instruction const*, unsigned short, bool, art::JValue*)+2364) (BuildId: 7ece79c15d80914c83e60c9e93ac1684)
#08 pc 000000000049136c  /apex/com.android.art/lib64/libart.so (void art::interpreter::ExecuteSwitchImplCpp<false>(art::interpreter::SwitchImplContext*)+1892) (BuildId: 7ece79c15d80914c83e60c9e93ac1684)
#09 pc 00000000003545d8  /apex/com.android.art/lib64/libart.so (ExecuteSwitchImplAsm+8) (BuildId: 7ece79c15d80914c83e60c9e93ac1684)
#10 pc 0000000000215534  /data/app/~~OdBMxcLjRbZre3jUFwqhtA==/com.zulip.flutter-Ivlz_Plsw-9nCNlt4y7GhA==/base.apk (y4.a.e+0)
#11 pc 000000000036e6ec  /apex/com.android.art/lib64/libart.so (art::interpreter::Execute(art::Thread*, art::CodeItemDataAccessor const&, art::ShadowFrame&, art::JValue, bool, bool) (.__uniq.112435418011751916792819755956732575238.llvm.9545667076320299271)+232) (BuildId: 7ece79c15d80914c83e60c9e93ac1684)
#12 pc 000000000036dfe4  /apex/com.android.art/lib64/libart.so (artQuickToInterpreterBridge+964) (BuildId: 7ece79c15d80914c83e60c9e93ac1684)
#13 pc 0000000000351f68  /apex/com.android.art/lib64/libart.so (art_quick_to_interpreter_bridge+88) (BuildId: 7ece79c15d80914c83e60c9e93ac1684)
#14 pc 00000000005b8a18  /apex/com.android.art/lib64/libart.so (nterp_helper+152) (BuildId: 7ece79c15d80914c83e60c9e93ac1684)
#15 pc 00000000002161a6  /data/app/~~OdBMxcLjRbZre3jUFwqhtA==/com.zulip.flutter-Ivlz_Plsw-9nCNlt4y7GhA==/base.apk (y4.f.t+66)
#16 pc 00000000005b98d4  /apex/com.android.art/lib64/libart.so (nterp_helper+3924) (BuildId: 7ece79c15d80914c83e60c9e93ac1684)
#17 pc 000000000021614a  /data/app/~~OdBMxcLjRbZre3jUFwqhtA==/com.zulip.flutter-Ivlz_Plsw-9nCNlt4y7GhA==/base.apk (y4.f.s+10)
#18 pc 00000000005b98d4  /apex/com.android.art/lib64/libart.so (nterp_helper+3924) (BuildId: 7ece79c15d80914c83e60c9e93ac1684)
#19 pc 000000000017f720  /data/app/~~OdBMxcLjRbZre3jUFwqhtA==/com.zulip.flutter-Ivlz_Plsw-9nCNlt4y7GhA==/base.apk (io.flutter.embedding.engine.d.<init>+56)
#20 pc 00000000005b98d4  /apex/com.android.art/lib64/libart.so (nterp_helper+3924) (BuildId: 7ece79c15d80914c83e60c9e93ac1684)
#21 pc 0000000000174fa4  /data/app/~~OdBMxcLjRbZre3jUFwqhtA==/com.zulip.flutter-Ivlz_Plsw-9nCNlt4y7GhA==/base.apk (io.flutter.embedding.android.h.K+344)
#22 pc 00000000005b98d4  /apex/com.android.art/lib64/libart.so (nterp_helper+3924) (BuildId: 7ece79c15d80914c83e60c9e93ac1684)
#23 pc 00000000001746a2  /data/app/~~OdBMxcLjRbZre3jUFwqhtA==/com.zulip.flutter-Ivlz_Plsw-9nCNlt4y7GhA==/base.apk (io.flutter.embedding.android.h.s+14)
#24 pc 00000000005b98d4  /apex/com.android.art/lib64/libart.so (nterp_helper+3924) (BuildId: 7ece79c15d80914c83e60c9e93ac1684)
#25 pc 00000000001759ee  /data/app/~~OdBMxcLjRbZre3jUFwqhtA==/com.zulip.flutter-Ivlz_Plsw-9nCNlt4y7GhA==/base.apk (io.flutter.embedding.android.g.onCreate+26)
#26 pc 00000000008e83ec  /data/misc/apexdata/com.android.art/dalvik-cache/arm64/boot.oat (android.app.Activity.performCreate+924)
#27 pc 000000000064b5a0  /data/misc/apexdata/com.android.art/dalvik-cache/arm64/boot.oat (android.app.Instrumentation.callActivityOnCreate+80)
#28 pc 000000000072cf94  /data/misc/apexdata/com.android.art/dalvik-cache/arm64/boot.oat (android.app.ActivityThread.performLaunchActivity+2708)
[… more frames …]

I didn't catch this earlier because it doesn't happen in debug mode.

Not sure what the issue is. Those top few frames sound like it's parsing some XML. Seems likely to be a bug in Flutter (or in Android) one way or another — in an APK, these XML files have been converted by the Android build system into its own binary format, so there really shouldn't be any parse errors after that stage.

In any case, we'll need to debug that before we can reland that change.

@gnprice gnprice reopened this May 16, 2024
@gnprice gnprice removed their assignment May 16, 2024
gnprice added a commit that referenced this issue May 16, 2024
This reverts commit 9b2a5f2.

That change caused a mysterious crash at startup, in
release mode only:
  #461 (comment)

Reverting while we debug.
@gnprice
Copy link
Member Author

gnprice commented May 16, 2024

@rajveermalviya, would you be up for investigating this after the small notification tweaks that are currently on your list?

@rajveermalviya
Copy link
Collaborator

would you be up for investigating this after the small notification tweaks that are currently on your list?

@gnprice Sure, I'll self-assign it and take a at it look later.

@rajveermalviya rajveermalviya self-assigned this May 16, 2024
@gnprice
Copy link
Member Author

gnprice commented Jun 19, 2024

Here's an update on the state of this issue. Thanks to @rajveermalviya's investigation, we've learned:

  • The strategy we'd been trying above unfortunately isn't going to work — the Flutter feature that we thought was there, isn't actually there.

    That strategy was to use an Android "network security config" XML file, just like the one we'd used on zulip-mobile; it looked like there was a Flutter feature to read that config and apply it to Dart's HTTP library. But that feature doesn't actually exist. The chat thread where we figured that out is here.

  • There's an alternative strategy that should work: instead of the pure-Dart HTTP implementation that's in the Dart standard library, we could switch to package:cronet_http on Android. That uses Cronet, an HTTP client from Chromium. See chat thread.

  • Using Cronet (and perhaps package:cupertino_http for an analogous choice on iOS?) may be a good idea independent of this issue. But it's going to be nontrivial to do:

    • By default package:cronet_http depends on Google Play Services for the Cronet library it uses.
    • Some people use the Zulip app without Google Play Services, and we'd like it to keep working for them. There's a build-time flag we can use in order to bundle Cronet into the app instead.
    • But that bundled Cronet adds around 10 MB to the app's download size. That's a bigger size increase than I would feel great doing for all our users who don't need it (because they do have Play Services).
    • So we'd need to do something like build two different variants, with and without a bundled Cronet.

Given the greater complexity that it turns out will be involved in order to implement this, this isn't an issue we'll be able to get to right now. I still hope we'll be able to do this, but there are a number of other features we'll prioritize building first.

@gnprice gnprice modified the milestones: Beta 3: Summer 2024, Launch Jun 19, 2024
gnprice added a commit to gnprice/zulip-flutter that referenced this issue Jul 8, 2024
This was introduced in cffb112 / zulip#474 because we thought it
would be a fix for zulip#461.  As discussed on that issue thread,
though, it turns out this file doesn't have an effect on the
Dart HTTP implementation, which we use.

Since it looks like it's doing something but in fact it isn't,
cut it out to avoid confusion.  It'll still be there in the
Git history if we later want to use it as part of a future
effort to fix zulip#461.
gnprice added a commit to gnprice/zulip-flutter that referenced this issue Jul 8, 2024
This was introduced in cffb112 / zulip#474 because we thought it
would be a fix for zulip#461.  As discussed on that issue thread,
though, it turns out this file doesn't have an effect on the
Dart HTTP implementation, which we use.

Since it looks like it's doing something but in fact it isn't,
cut it out to avoid confusion.  It'll still be there in the
Git history if we later want to use it as part of a future
effort to fix zulip#461.
gnprice added a commit to gnprice/zulip-flutter that referenced this issue Jul 8, 2024
This was introduced in cffb112 / zulip#474 because we thought it
would be a fix for zulip#461.  As discussed on that issue thread,
though, it turns out this file doesn't have an effect on the
Dart HTTP implementation, which we use.

Since it looks like it's doing something but in fact it isn't,
cut it out to avoid confusion.  It'll still be there in the
Git history if we later want to use it as part of a future
effort to fix zulip#461.
@rajveermalviya

This comment was marked as off-topic.

@gnprice

This comment was marked as off-topic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a-Android Issues specific to Android, or requiring Android-specific work a-first-hour Issues specific to using the app for the first time beta feedback Things beta users have specifically asked for
Projects
Status: In Progress
3 participants