Trust user-added CAs on Android #5493

shrizza · 2022-09-22T06:17:22Z

In response to issue #3312. In Android, allow trusting user-added CAs, while still deferring trust to public CAs for both zulipchat.com and zulip.chat.org.

Fixes: #3312

chrisbobbe · 2022-09-23T04:09:41Z

Hi @shrizza, thanks for the PR! gnprice will need to approve it before it's merged, and he's on vacation until next Tuesday Pacific time, 2022-09-27. 🙂

These lines look like they're meant to identify the domains Greg pointed to in his proposal:

<domain includeSubdomains="true">zulipchat.com</domain>

<domain includeSubdomains="true">zulip.org</domain>

Those domains were: "zulipchat.com and its subdomains, and chat.zulip.org." For the latter, it looks like your code actually points to zulip.org and all its subdomains. Is there a reason you chose to do that instead of just "chat.zulip.org"? Thanks!

Also, it would help if you squashed your branch's two commits into a single commit and gave that a commit message that follows our style guide. But for one small PR, we also don't mind doing that ourselves.

Oh, and a question for Greg when you get back: should we also think about zulip.com and its subdomains?

shrizza · 2022-09-23T12:35:07Z

Hi @chrisbobbe,

Thank you for the feedback. I've squashed the changes and modified the commit message. Should this PR's title be adjusted similarly?

I'm actually not familiar with how the zulipchat.com and zulip.org domains are organized, but if there's an elaborate subdomain delegation scheme I can adjust the deferred domains to strictly zulipchat.com and chat.zulip.org, and no further subdomains. I will wait on Greg's response on this point so that it can all be considered together with the verdict on zulip.com.

chrisbobbe · 2022-10-20T13:22:50Z

(Tagging @gnprice since I didn't do so above. 🙂)

gnprice

Thanks @shrizza for sending this! Sorry for the slow response -- I came down with covid the day after getting back from vacation, and then when I emerged, this was a bit buried in the pile of other things to catch up on.

Generally this looks good -- small comments below.

One question for you: have you been able to try this out in a build of the app, and confirmed that it succeeds in solving your use case? Based on the documentation I believe it should, but it'd be great to have empirical confirmation that we haven't missed something.

gnprice · 2022-10-29T00:05:33Z

android/app/src/main/res/xml/network_security_config.xml

+      <domain-config>
+           <domain includeSubdomains="true">zulipchat.com</domain>
+           <trust-anchors><certificates src="system" /></trust-anchors>
+      </domain-config>
+      <domain-config>
+           <domain includeSubdomains="true">zulip.org</domain>
+           <trust-anchors><certificates src="system" /></trust-anchors>


Let's use one domain-config element for all the domains we're giving the same config to, in order to dedupe the latter.

There's an example in the docs with multiple domain elements per domain-config:
https://developer.android.com/training/articles/security-config#LimitingCas
(and the spec at the bottom of that doc also specifies that that's accepted.)

I've adjusted the commit to fulfill this desire.

gnprice · 2022-10-29T00:14:16Z

android/app/src/main/res/xml/network_security_config.xml

+           <trust-anchors><certificates src="system" /></trust-anchors>
+      </domain-config>
+      <domain-config>
+           <domain includeSubdomains="true">zulip.org</domain>


Yeah, covering *.zulip.org this way sounds good to me.

Let's also add *.zulip.com too -- good idea.

In general, as I said in the comment on #3312, the goal here is to cover domains that we know should be identified by certs from global trusted CAs. That includes not only the domains where we as the publishers of the app run Zulip servers -- so *.zulipchat.com and chat.zulip.org -- but also more broadly the domains that we operate. So that includes *.zulip.org in general and also *.zulip.com.

Yes, applying this policy uniformly across all those domains should be fine as long as you do not find yourself delegating subdomains of those to third-parties that may wish to use private CAs.

Commit is updated to additionally include zulip.com.

gnprice · 2022-10-29T01:25:23Z

android/app/src/main/res/xml/network_security_config.xml

+      <base-config>
+           <trust-anchors>
+                <certificates src="system" />
+                <certificates src="user" />
+           </trust-anchors>


I'd like to have a comment here with a sentence or two explaining what this is doing and why, and also one for the domain-config element. Should link to the request #3312 , and also to the reference docs for understanding this file's semantics:
https://developer.android.com/training/articles/security-config#FileFormat

I'll probably write those comments up in the course of merging this. But if you can make a go at writing a draft, that would help me out and would be appreciated.

I've added a number of comments to address your concern. Please let me know if it needs any further adjustment.

gnprice · 2022-10-29T01:30:18Z

Oh, and one more small comment: please mark the commit and the PR as resolving the issue. See:
https://github.com/zulip/zulip-mobile/blob/main/docs/style.md#mention-fixed-issue
https://github.com/zulip/zulip-mobile/blob/main/docs/style.md#fixes-format

shrizza · 2022-10-30T16:52:33Z

Oh, and one more small comment: please mark the commit and the PR as resolving the issue. See: https://github.com/zulip/zulip-mobile/blob/main/docs/style.md#mention-fixed-issue https://github.com/zulip/zulip-mobile/blob/main/docs/style.md#fixes-format

I've adjusted the commit message accordingly.

I'm not sure about the PR since it seems I've referenced the issue in both the title and my first comment. I'm not seeing a description I can edit for this PR; please let me know if I'm missing something.

shrizza · 2022-10-30T17:08:49Z

One question for you: have you been able to try this out in a build of the app, and confirmed that it succeeds in solving your use case? Based on the documentation I believe it should, but it'd be great to have empirical confirmation that we haven't missed something.

To be honest, I have no experience in Android app development and have not actually tested my changes. I don't have an Android SDK set up either so I'd imagine that would be necessary for testing.

This was unclear in a couple of respects, as I learned here: #5493 (comment) Specifically, fill in some background that's well known to those of us who work in GitHub daily but isn't otherwise obvious. Also make better cross-references, and expand a bit on leaving issue numbers out of PR titles.

gnprice · 2022-11-01T04:21:55Z

Thanks for the revision!

To be honest, I have no experience in Android app development and have not actually tested my changes. I don't have an Android SDK set up either so I'd imagine that would be necessary for testing.

Perfectly reasonable. Yeah, the app should be pretty straightforward to build once one has an Android dev setup, but the latter can be a bit annoying and a few tens of GB of disk.

Before merge I'll test locally that it builds, but it would take a fair bit of setup to directly test that it solves the issue. We'll just rely, then, on the fact it's a short file and the docs seem pretty clear about its semantics.

I'm not sure about the PR since it seems I've referenced the issue in both the title and my first comment. I'm not seeing a description I can edit for this PR; please let me know if I'm missing something.

Ah, I see how the style guide isn't clear about this, thanks. I'll go try to clarify there… OK, please take a look now:
https://github.com/zulip/zulip-mobile/blob/main/docs/style.md#mention-fixed-issue
or to see just what changed: 4e09f23

Do let me know if that still doesn't do it. We aspire to have the docs clear enough to answer this sort of recurring question by themselves, so this feedback when they aren't is helpful.

gnprice · 2022-11-01T04:34:40Z

Before merge I'll test locally that it builds

(We test this in CI, but CI failed here -- following the link, it looks like an operational issue unrelated to your change:

* What went wrong:
A problem occurred configuring project ':react-native-photo-view'.
> Could not resolve all artifacts for configuration ':react-native-photo-view:classpath'.
   > Could not resolve com.android.tools.build:gradle-core:2.1.0.
     Required by:
         project :react-native-photo-view > com.android.tools.build:gradle:2.1.0
      > Could not resolve com.android.tools.build:gradle-core:2.1.0.
         > Could not get resource 'https://jcenter.bintray.com/com/android/tools/build/gradle-core/2.1.0/gradle-core-2.1.0.pom'.
            > Could not GET 'https://jcenter.bintray.com/com/android/tools/build/gradle-core/2.1.0/gradle-core-2.1.0.pom'.
               > Read timed out

but which caused it to fail before the actual Android build. I guess I'll also rerun CI to see if that makes it pass. Previous instance at #5193 (comment); chat thread from then.)

shrizza · 2022-11-01T07:34:13Z

Do let me know if that still doesn't do it. We aspire to have the docs clear enough to answer this sort of recurring question by themselves, so this feedback when they aren't is helpful.

Thank you for reviewing and for expanding on the meaning of the PR description; that does make it more clear to me. I've adjusted this PR's title and description to more closely fit the guideline.

gnprice · 2022-11-03T02:09:20Z

Hmm, well, I'm glad I tested this a bit manually before merging it -- because in its current form, it causes the app to stop working in the debug builds we use for development!

I suspect what's happening is that it's inadvertently clobbering the debug-only config that allows plaintext HTTP traffic, which the debug builds use for talking to the local web server that provides the app's JS code.

I'll do a bit more poking at it now to see if I can resolve the issue. In the meantime, I rebased and edited the comments to a version I'm happy with.

gnprice · 2022-11-03T02:10:54Z

I suspect what's happening is that it's inadvertently clobbering the debug-only config that allows plaintext HTTP traffic, which the debug builds use for talking to the local web server that provides the app's JS code.

Yup, that's it:
https://developer.android.com/guide/topics/manifest/application-element#usesCleartextTraffic

This flag is ignored on Android 7.0 (API level 24) and above if an Android Network Security Config is present.

I think the fix will be straightforward.

But disable the new config in debug builds, because otherwise it clobbers the `android:usesCleartextTraffic` attribute we rely on there: https://developer.android.com/guide/topics/manifest/application-element#usesCleartextTraffic [greg: expanded explanatory comments; added debug fix; tested that both debug and release builds still work.] Fixes: zulip#3312

gnprice · 2022-11-03T02:27:24Z

OK, fixed that -- see 85c3a71 .

And merged! Thanks again @shrizza for the contribution.

shrizza force-pushed the issue3312-userca branch from dde356c to 1a2b051 Compare September 23, 2022 10:45

gnprice reviewed Oct 29, 2022

View reviewed changes

shrizza force-pushed the issue3312-userca branch 2 times, most recently from def1499 to b9a3f83 Compare October 30, 2022 16:40

shrizza requested a review from gnprice October 30, 2022 17:10

shrizza changed the title ~~Trust user-added CAs on Android (Issue #3312)~~ Trust user-added CAs on Android Nov 1, 2022

gnprice force-pushed the issue3312-userca branch from b9a3f83 to dc4b8ed Compare November 3, 2022 02:07

gnprice force-pushed the issue3312-userca branch from dc4b8ed to 85c3a71 Compare November 3, 2022 02:26

gnprice merged commit 85c3a71 into zulip:main Nov 3, 2022

gnprice mentioned this pull request Dec 23, 2023

Trust user-added CAs on Android zulip/zulip-flutter#461

Open

gnprice mentioned this pull request Jan 4, 2024

android: Trust user-supplied CAs, except on known servers zulip/zulip-flutter#474

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Trust user-added CAs on Android #5493

Trust user-added CAs on Android #5493

shrizza commented Sep 22, 2022 •

edited

Loading

chrisbobbe commented Sep 23, 2022 •

edited

Loading

shrizza commented Sep 23, 2022

chrisbobbe commented Oct 20, 2022

gnprice left a comment

gnprice Oct 29, 2022

shrizza Oct 30, 2022 •

edited

Loading

gnprice Oct 29, 2022

shrizza Oct 29, 2022

shrizza Oct 30, 2022 •

edited

Loading

gnprice Oct 29, 2022

shrizza Oct 30, 2022

gnprice commented Oct 29, 2022

shrizza commented Oct 30, 2022

shrizza commented Oct 30, 2022

gnprice commented Nov 1, 2022

gnprice commented Nov 1, 2022

shrizza commented Nov 1, 2022 •

edited

Loading

gnprice commented Nov 3, 2022

gnprice commented Nov 3, 2022

gnprice commented Nov 3, 2022

Trust user-added CAs on Android #5493

Trust user-added CAs on Android #5493

Conversation

shrizza commented Sep 22, 2022 • edited Loading

chrisbobbe commented Sep 23, 2022 • edited Loading

shrizza commented Sep 23, 2022

chrisbobbe commented Oct 20, 2022

gnprice left a comment

Choose a reason for hiding this comment

gnprice Oct 29, 2022

Choose a reason for hiding this comment

shrizza Oct 30, 2022 • edited Loading

Choose a reason for hiding this comment

gnprice Oct 29, 2022

Choose a reason for hiding this comment

shrizza Oct 29, 2022

Choose a reason for hiding this comment

shrizza Oct 30, 2022 • edited Loading

Choose a reason for hiding this comment

gnprice Oct 29, 2022

Choose a reason for hiding this comment

shrizza Oct 30, 2022

Choose a reason for hiding this comment

gnprice commented Oct 29, 2022

shrizza commented Oct 30, 2022

shrizza commented Oct 30, 2022

gnprice commented Nov 1, 2022

gnprice commented Nov 1, 2022

shrizza commented Nov 1, 2022 • edited Loading

gnprice commented Nov 3, 2022

gnprice commented Nov 3, 2022

gnprice commented Nov 3, 2022

shrizza commented Sep 22, 2022 •

edited

Loading

chrisbobbe commented Sep 23, 2022 •

edited

Loading

shrizza Oct 30, 2022 •

edited

Loading

shrizza Oct 30, 2022 •

edited

Loading

shrizza commented Nov 1, 2022 •

edited

Loading