feat: add support for Android user-installed certificates#19830
feat: add support for Android user-installed certificates#19830jfly wants to merge 2 commits intoimmich-app:mainfrom
Conversation
| <base-config> | ||
| <trust-anchors> | ||
| <certificates src="system"/> | ||
| <certificates src="user"/> |
There was a problem hiding this comment.
There's no way to make this opt-in, right?
There was a problem hiding this comment.
I don't know, but everything I know about android development I learned in the last week while researching this issue.
8546274 to
01f0a57
Compare
|
@jfly Hey, Thanks a lot for working on this. However, if we are to merge this in, rather than deprecating the existing "Allow Self signed certificates" toggle, we could just remove all the custom verification logic all together. We don't need a lot of different codepath to address the same issue. However, this would mean that we would also need to test this exhaustively to make sure it does not break anything from the current setup. So things like:
For all cases, we'd need to test both the dart side of things and the native side of things. Most network calls are from the dart side. Video player, Asset download are made from native side and we should validate these as well. If we are to remove the self signed toggle, we've to make sure everything works on iOS as well. You could just inline all the changes. We are migrating to using pigeon for all message passing between flutter and the platform side, so you can add an API to fetch the certificates and load it in the dart's SecurityContext. We might also need some documentation change to inform users about installing certificates to the OS cert store, rather than loading it in the app like they used to. The core team currently has got it's hands full with working towards the stable release. I know this is a significant undertaking for your free time, but if you can get this properly tested after removing the existing toggle and adding in your changes, we can happily merge this in. If you need help setting up specific test scenarios or access to different certificate setups, let us know - we can coordinate with the community to help test various configurations. |
mobile/android/app/src/main/res/xml/network_security_config.xml
Outdated
Show resolved
Hide resolved
Sure, I'm happy to take this on. I've converted my understanding of your requirements into a checklist below. Please let me know if I'm missing anything:
|
This is perfect! Thanks a lot for taking this on. As far the documentation, we can discuss this during the release of this PR. But yes, a callout in the release note might just do. It is also probably worth testing the migration from the older version with the setting on with a certificate to the new one without the toggle. Please feel free to ping me here or over discord if you need any help with the implementation or testing |
5182425 to
c6419f1
Compare
I ran into this while testing out <immich-app#19830>. When I add, change, or remove a client certificate under Immich's advanced settings, the change wouldn't take effect until some mysterious point in the future. For example: 1. Add a client certificate. It doesn't get used. 2. Remove certificate. *Now* the client certificate from step 1) is used. 3. Restart application. Now no client certificate is used. This all boils down to some missing `await`s. The user would change the cert, and we'd start asynchronously saving it to the store, and while the save is still happening, [`HttpSSLOptions` pulls the "old" value out of `SSLClientCertStoreVal`](https://github.com/immich-app/immich/blob/v1.136.0/mobile/lib/utils/http_ssl_options.dart#L30). With the appropriate `await`s, this behaves much more sanely.
0925f1e to
187a465
Compare
I ran into this while testing out <immich-app#19830>. When I add, change, or remove a client certificate under Immich's advanced settings, the change wouldn't take effect until some mysterious point in the future. For example: 1. Add a client certificate. It doesn't get used. 2. Remove certificate. *Now* the client certificate from step 1) is used. 3. Restart application. Now no client certificate is used. This all boils down to some missing `await`s. The user would change the cert, and we'd start asynchronously saving it to the store, and while the save is still happening, [`HttpSSLOptions` pulls the "old" value out of `SSLClientCertStoreVal`](https://github.com/immich-app/immich/blob/v1.136.0/mobile/lib/utils/http_ssl_options.dart#L30). With the appropriate `await`s, this behaves much more sanely.
187a465 to
813adfc
Compare
I ran into this while testing out <immich-app#19830>. When I add, change, or remove a client certificate under Immich's advanced settings, the change wouldn't take effect until some mysterious point in the future. For example: 1. Add a client certificate. It doesn't get used. 2. Remove certificate. *Now* the client certificate from step 1) is used. 3. Restart application. Now no client certificate is used. This all boils down to some missing `await`s. The user would change the cert, and we'd start asynchronously saving it to the store, and while the save is still happening, [`HttpSSLOptions` pulls the "old" value out of `SSLClientCertStoreVal`](https://github.com/immich-app/immich/blob/v1.136.0/mobile/lib/utils/http_ssl_options.dart#L30). With the appropriate `await`s, this behaves much more sanely.
I ran into this while testing out <immich-app#19830>. When I add, change, or remove a client certificate under Immich's advanced settings, the change wouldn't take effect until some mysterious point in the future. For example: 1. Add a client certificate. It doesn't get used. 2. Remove certificate. *Now* the client certificate from step 1) is used. 3. Restart application. Now no client certificate is used. This all boils down to some missing `await`s. The user would change the cert, and we'd start asynchronously saving it to the store, and while the save is still happening, [`HttpSSLOptions` pulls the "old" value out of `SSLClientCertStoreVal`](https://github.com/immich-app/immich/blob/v1.136.0/mobile/lib/utils/http_ssl_options.dart#L30). With the appropriate `await`s, this behaves much more sanely.
I ran into this while testing out <#19830>. When I add, change, or remove a client certificate under Immich's advanced settings, the change wouldn't take effect until some mysterious point in the future. For example: 1. Add a client certificate. It doesn't get used. 2. Remove certificate. *Now* the client certificate from step 1) is used. 3. Restart application. Now no client certificate is used. This all boils down to some missing `await`s. The user would change the cert, and we'd start asynchronously saving it to the store, and while the save is still happening, [`HttpSSLOptions` pulls the "old" value out of `SSLClientCertStoreVal`](https://github.com/immich-app/immich/blob/v1.136.0/mobile/lib/utils/http_ssl_options.dart#L30). With the appropriate `await`s, this behaves much more sanely.
db8d15e to
b396b38
Compare
|
@shenlong-tanwen, I'm ready for you to take another look at this PR. I've updated my checklist with what I have done and not done. Notably, with these changes, self-signed certificates work, but Android only lets you install them if they have the X.509v3 So, we need to decide what to do here. A couple proposals:
|
|
Personally I agree with your second point, since this feature here will not strictly replace the existing flag it could be a breaking change for some users to remove it. I'm also not sure about the options for IOS users to install custom CA. |
This works fine. My wife is on iOS and has our custom CA installed on her phone, and immich works great.
Yeah, that would be a ssh-style TOFU scheme. I don't have the same safety concerns, but that's a whole bunch of code that I don't really see the value in anyone writing, nor having the core immich team maintain. IMO, it would be better for immich to take an opinionated stance and say that anyone running their immich server without "universally trusted" certs is required to be have that cert chained off of a root of trust with Of course I am happy to do whatever the core team needs to get this PR landed. IMO, the best solution would be to rebrand the existing option as dangerous and mark it deprecated, with the intent to remove it before the stable release. |
b396b38 to
5e45a35
Compare
|
Thanks, @shenlong-tanwen.
What if keep the existing option, but reword it a little bit (as I suggested here: #19830 (comment)).
Do you mean "after" the next major release? |
f24922b to
0cfc278
Compare
|
@denysvitali, agreed that using a single client with support for Android user-installed certificates sounds like a better long term solution. I did just rebase this PR and dropped the breaking change. Instead, I've just marked the existing "Allow self-signed SSL certificates" feature as deprecated. @shenlong-tanwen, I'd really love to get this PR merged. I've been daily driving it since July 2025 and have had no issues, other than my immich android client getting progressively more out of date. |
382e892 to
6fbe850
Compare
shenlong-tanwen
left a comment
There was a problem hiding this comment.
Sorry that this got shelved for so long. Can you add the pigeon generator command to the mise.toml file in the mobile app directory as well?
The other parts looks good and we can actually merge this sooner now that we have proper backward support for current installs. Apologies again and Thanks a lot for working on this
This is part of immich-app#15230. Frustratingly, Dart/Flutter ignores user-installed certificates. Working around this requires rooting your Android device to install certificates as "system" certs, which isn't an option for everyone. This is a known issue with Dart, see dart-lang/sdk#50435 and flutter/flutter#56607 for details. I have read through <immich-app#15230> and <immich-app#13555>, and I understand that switching to [`cronnet_http`](immich-app#14335 (comment)) would also resolve this. While that may be the correct long-term approach, it looks like there are [a lot of network codepaths](immich-app#15230 (comment)) in Immich, and as I know basically nothing about Dart, nor Flutter, nor Immich's codebase, I thought this would be a better short term approach. For the record: the important code here is copied from my fork of `johnstef99/flutter_user_certificates_android`. See this PR for more context: <johnstef99/flutter_user_certificates_android#2>.
This is unnecessary now that we support user-installed certificates on Android. Users should be adding certs to their system instead, which will work in more places (such as browsers), and won't blindly trust the cert.
6fbe850 to
d9c9ff5
Compare
@shenlong-tanwen, done. It's unclear to me how this relates to the |
|
I'd love to move away from the Dart HttpClient entirely after measuring just how much worse it is compared to the native clients. To that end, I'd prefer a solution that moves away from HttpClient rather than depending on it. |
|
@mertalev My proposed solution uses OkHttp on Android and solve the issues mentioned here |
I think OkHttp is the right direction here. I have a WIP branch that handles certs app-wide for both platforms. It creates a shared client on the platform side and constructs the cupertino_http/ok_http client from its pointer on the Dart side. This would mean even all Flutter isolates and engines use the same client. |
|
I don't care what networking stack we use. OkHttp sounds great if it can also do proper handling of user-installed certificates on Android. I just want to get back to installing Immich from f-droid, rather than infrequently rebasing this branch (and dealing with conflicts). My preference would be to land this PR as is so certificate handling on android can get un-broken. I'd be happy for @denysvitali to resurrect their work in #22768 and delete the hack I've added here. I don't think landing this PR increases the effort to switch to OkHttp. @shenlong-tanwen, WDYT? |
|
I totally get that you want to fix this sooner rather than later and I agree! I'm just already working on a more complete solution, so it seems unnecessary to merge this code if it's going to be superseded soon. |
|
Sorry, I'm getting little frustrated here. This wouldn't be the first time this PR has had to wait for a networking refactor you've done (#19830 (comment)). This PR exists, is reviewed and ready to land, and fixes a real issue in the wild. It also deprecates a very dangerous feature that IMO shouldn't exist. Why not merge as is? I'll still cheer the day you delete this hack! |
|
Sorry for the frustration. This wouldn't be merged until 2.6, by which point I expect the whole app to be switched over to native clients. Merging this PR implies testing it only to revert it right after, which in turn means a larger diff for others in the team to review. It's honestly not a good use of time. As a team, we have our own planning and this is a task I've already picked up, so please rest assured that I'll take care of it. |
|
Ok, I appreciate the timeline. |
|
What a surprise to see this PR closed after all the discussions and changes made to remove any breaking changes. |
|
@mertalev, could you share an issue/PR I can subscribe to so I know how your OkHttp work is going? Thanks, and good luck! |
It still needs some work before it's ready for a PR, but I'll ping you when it's up :) |
Description
This is part of #15230.
Frustratingly, Dart/Flutter ignores user-installed certificates. Working around this requires rooting your Android device to install certificates as "system" certs, which isn't an option for everyone.
This is a known issue with Dart, see
dart-lang/sdk#50435 and
flutter/flutter#56607 for details.
I have read through
#15230 and #13555, and I understand that switching to
cronnet_httpwould also resolve this. While that may be the correct long-term approach, it looks like there are a lot of network codepaths in Immich, and as I know basically nothing about Dart, nor Flutter, nor Immich's codebase, I thought this would be a better short term approach.This depends on my fork of
johnstef99/flutter_user_certificates_android, which I've sent a PR for herejohnstef99/flutter_user_certificates_android#2. If y'all don't like the supply chain implications of that, I'm happy to inline the implementation here instead.
How Has This Been Tested?
I have step-ca and DNS configured so that immich.mm resolves to an immich server with a certificate issued by my step-ca certificate authority. I've installed that certificate authority as a user-installed certificate on my Android phone.
Screenshots (if appropriate)
The help text on this option is no longer true. I'd suggest that we deprecate it in favor of encouraging people to add certificates to their OS:
Checklist:
src/services/uses repositories implementations for database calls, filesystem operations, etc.src/repositories/is pretty basic/simple and does not have any immich specific logic (that belongs insrc/services/)