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

[META] Experimental network features #15230

Open
mmomjian opened this issue Jan 10, 2025 · 27 comments
Open

[META] Experimental network features #15230

mmomjian opened this issue Jan 10, 2025 · 27 comments

Comments

@mmomjian
Copy link
Contributor

mmomjian commented Jan 10, 2025

Experimental network features

Features

There are some network features that have been added to the Immich mobile app through the years that interact with various libraries and background tasks in unexpected ways. These features include:

  • HTTP Basic Auth for server URL
  • Self-signed SSL certs
  • Mutual TLS
  • Custom HTTP proxy headers

Limitations

Typically, these network features work fine in the normal library view, but cause issues with:

  • video playback
  • foreground asset upload
  • background asset upload
  • asset download
  • App crashing

Solutions

Thanks to the recent implementation of auto-switching server URLs in #14437, many of these features are no longer needed for a smooth Immich experience. There are other options, including the use of a VPN or a real SSL cert.

Handling known issues

We have updated the FAQ (link will work once #15228 is merged). This issue can be used to track any known issues with these experimental network features. These issues will not be a priority for the dev team, but can be improved by the community if possible.

Please do not use this discussion for comments like "same" or +1". This is designed only to keep track of current known limitations to these experimental features, or discuss potential solutions and future patches.

@rovo89
Copy link
Contributor

rovo89 commented Jan 10, 2025

The final sentences sound quite intimidating, so I'm wondering: Is it allowed to discuss analysis of root causes and potential solutions here? Or where would that go?

@mmomjian
Copy link
Contributor Author

The final sentences sound quite intimidating, so I'm wondering: Is it allowed to discuss analysis of root causes and potential solutions here? Or where would that go?

If the discussion is relevant to potential future PRs or improvements, or the actual underlying issue, that's fine. Often these topics turn into people repetitively saying "I want this feature too", that is what we are trying to avoid

@ckuyehar
Copy link

I propose that we use the discussions area to discuss a specific feature. If you start a discussion, mention it in the discussion and ask @mmomjian to update the top comment so that it links to the discussion.

The moment, two or more features are discussed in this single thread, it's going to get chaotic.

@bo0tzz
Copy link
Member

bo0tzz commented Jan 10, 2025

It is already know that these features do not work well, so we should avoid rehashing "it's broken" over and over again.
I would propose instead to keep this thread focussed on very specific discussion of why things are not working (eg referencing specific code paths and such) that might result in a PR. That way things should not get chaotic, and there should be no need for more separate issues or discussions (which is chaotic in its own way).

@rovo89
Copy link
Contributor

rovo89 commented Jan 10, 2025

As per #14845 (comment), "app crashes" should be added to the list of limitations. Happens on Android whenever I navigate to a video, e.g. by clicking on it in the timeline or when swiping through it. I don't even need to click the "play" button to provoke the crash.

@rovo89
Copy link
Contributor

rovo89 commented Jan 10, 2025

As far as analysis is concerned, I'm copying my findings from #5553 (comment) which is closed meanwhile. My setup uses mTLS (client certificates) and a Let's Encrypt server certificate, I'm using Android.


A major problem seems to be the many levels of abstraction and different packages used. Flutter (using Dart), Jetpack, Android framework... that make it hard to follow the call chain and pass the required information down to the places where connections are actually used.

For most parts of the app, Immich uses HttpSSLCertOverride to set the client certificate and allow self-signed certificates (for the Immich server host only), which is set globally here. But I think that only applies to places where Dart's standard HttpClient class is used. Under the hood, I think it makes adjustments to OpenSSL's SSLCertContext which is wrapped by Dart's SecurityContext. My understanding is that the native HTTP client isn't involved at all here, but OpenSSL is used directly.

The video player seems to use the Android native HTTP client which is created here. DefaultHttpDataSource comes from here and uses HttpURLConnection. IIUC, Android uses a customized OpenJDK with their own handlers, in this case com.android.okhttp.HttpsHandler. This seems to be the place where they set SSL options, which uses HttpsURLConnection.getDefaultSSLSocketFactory(). I haven't tracked it down completely yet, but I think SSLContext.init() is where a client certificate would be specified.

I have just written this down to understand the current flow and maybe get corrections from someone. Next step would be finding a way to pass the client certificate to the appropriate place without breaking abstraction... Maybe it would be sufficient to set call HttpsURLConnection.setDefaultSSLSocketFactory() in Immich code?

For completeness, the download library has some code to accept untrusted certificates which calls exactly that method with an accept-all trust manager, indicating that it could profit from this approach as well.

@ckuyehar
Copy link

Custom HTTP proxy headers

  • I think this issue refers to using the "custom proxy headers" in the Immich mobile app.
  • I use this feature and I have no issue with this.

Can someone refer an issue related to this one?

@mmomjian
Copy link
Contributor Author

Custom HTTP proxy headers

  • I think this issue refers to using the "custom proxy headers" in the Immich mobile app.

  • I use this feature and I have no issue with this.

Can someone refer an issue related to this one?

It's possible there's no known issues. However we still consider it an experimental feature. Glad it's working well!

@rovo89
Copy link
Contributor

rovo89 commented Jan 10, 2025

As for the crash in the video player, here is the trace with a debug build:

E/ExoPlayerImplInternal(10356): Playback error
E/ExoPlayerImplInternal(10356):   androidx.media3.exoplayer.ExoPlaybackException: Source error
E/ExoPlayerImplInternal(10356):       at androidx.media3.exoplayer.ExoPlayerImplInternal.handleIoException(ExoPlayerImplInternal.java:737)
E/ExoPlayerImplInternal(10356):       at androidx.media3.exoplayer.ExoPlayerImplInternal.handleMessage(ExoPlayerImplInternal.java:709)
E/ExoPlayerImplInternal(10356):       at android.os.Handler.dispatchMessage(Handler.java:105)
E/ExoPlayerImplInternal(10356):       at android.os.Looper.loopOnce(Looper.java:232)
E/ExoPlayerImplInternal(10356):       at android.os.Looper.loop(Looper.java:317)
E/ExoPlayerImplInternal(10356):       at android.os.HandlerThread.run(HandlerThread.java:85)
E/ExoPlayerImplInternal(10356):   Caused by: androidx.media3.datasource.HttpDataSource$InvalidResponseCodeException: Response code: 400
E/ExoPlayerImplInternal(10356):       at androidx.media3.datasource.DefaultHttpDataSource.open(DefaultHttpDataSource.java:401)
E/ExoPlayerImplInternal(10356):       at androidx.media3.datasource.StatsDataSource.open(StatsDataSource.java:87)
E/ExoPlayerImplInternal(10356):       at androidx.media3.exoplayer.source.ProgressiveMediaPeriod$ExtractingLoadable.load(ProgressiveMediaPeriod.java:1085)
E/ExoPlayerImplInternal(10356):       at androidx.media3.exoplayer.upstream.Loader$LoadTask.run(Loader.java:450)
E/ExoPlayerImplInternal(10356):       at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1145)
E/ExoPlayerImplInternal(10356):       at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:644)
E/ExoPlayerImplInternal(10356):       at java.lang.Thread.run(Thread.java:1012)
D/AndroidRuntime(10356): Shutting down VM
E/AndroidRuntime(10356): FATAL EXCEPTION: main
E/AndroidRuntime(10356): Process: app.alextran.immich.debug, PID: 10356
E/AndroidRuntime(10356): java.lang.ClassCastException: androidx.media3.datasource.HttpDataSource$InvalidResponseCodeException cannot be cast to java.lang.Error
E/AndroidRuntime(10356): 	at me.albemala.native_video_player.NativeVideoPlayerViewController.onPlayerError(NativeVideoPlayerViewController.kt:137)
E/AndroidRuntime(10356): 	at androidx.media3.exoplayer.ExoPlayerImpl.lambda$updatePlaybackInfo$16(ExoPlayerImpl.java:2111)
E/AndroidRuntime(10356): 	at androidx.media3.exoplayer.ExoPlayerImpl$$ExternalSyntheticLambda27.invoke(D8$$SyntheticClass:0)
E/AndroidRuntime(10356): 	at androidx.media3.common.util.ListenerSet$ListenerHolder.invoke(ListenerSet.java:342)
E/AndroidRuntime(10356): 	at androidx.media3.common.util.ListenerSet.lambda$queueEvent$0(ListenerSet.java:226)
E/AndroidRuntime(10356): 	at androidx.media3.common.util.ListenerSet$$ExternalSyntheticLambda1.run(D8$$SyntheticClass:0)
E/AndroidRuntime(10356): 	at androidx.media3.common.util.ListenerSet.flushEvents(ListenerSet.java:248)
E/AndroidRuntime(10356): 	at androidx.media3.exoplayer.ExoPlayerImpl.updatePlaybackInfo(ExoPlayerImpl.java:2174)
E/AndroidRuntime(10356): 	at androidx.media3.exoplayer.ExoPlayerImpl.handlePlaybackInfo(ExoPlayerImpl.java:2008)
E/AndroidRuntime(10356): 	at androidx.media3.exoplayer.ExoPlayerImpl.lambda$new$1$androidx-media3-exoplayer-ExoPlayerImpl(ExoPlayerImpl.java:348)
E/AndroidRuntime(10356): 	at androidx.media3.exoplayer.ExoPlayerImpl$$ExternalSyntheticLambda10.run(D8$$SyntheticClass:0)
E/AndroidRuntime(10356): 	at android.os.Handler.handleCallback(Handler.java:991)
E/AndroidRuntime(10356): 	at android.os.Handler.dispatchMessage(Handler.java:102)
E/AndroidRuntime(10356): 	at android.os.Looper.loopOnce(Looper.java:232)
E/AndroidRuntime(10356): 	at android.os.Looper.loop(Looper.java:317)
E/AndroidRuntime(10356): 	at android.app.ActivityThread.main(ActivityThread.java:8787)
E/AndroidRuntime(10356): 	at java.lang.reflect.Method.invoke(Native Method)
E/AndroidRuntime(10356): 	at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:591)
E/AndroidRuntime(10356): 	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:871)
I/Process (10356): Sending signal. PID: 10356 SIG: 9

So it crashes here because HttpDataSource.InvalidResponseCodeException is a java.lang.Exception, not a java.lang.Error. The common super-class for both is java.lang.Throwable, and indeed changing the cast and the onError() parameter to Throwable solves the crash. The throbber (animated circle spinning while loading) is simply shown indefinitely and I can navigate to other images.

I'll send a PR for that to both upstream and the Immich fork.
EDIT: It's only in the code that was added to move to Exoplayer.

@ckuyehar
Copy link

ckuyehar commented Jan 10, 2025

For completeness, the download library has some code to accept untrusted certificates which calls exactly that method with an accept-all trust manager, indicating that it could profit from this approach as well.

The background_downloader does NOT allow self-signed certificates in release mode. I previously mentioned this in #15188.

However... even if you were to trust your CA and import your client certificate in your mobile device, the background_downloader will still fail. Why? The package background_downloader v8.9.0 doesn't support mutual TLS.

@rovo89
Copy link
Contributor

rovo89 commented Jan 10, 2025

The background_downloader does NOT allow self-signed certificates in release mode. I previously mentioned this in #15188.

I wasn't planning to use their debug feature. I mentioned it because it shows that their implementation seems to consider whatever is dictated by HttpsURLConnection.setDefaultSSLSocketFactory(). Calling the latter from Immich code should therefore have the same effect, and I have hopes that this approach would also allow to pass client certificates. See KeyManager in SSLContext.init().

rovo89 added a commit to rovo89/native_video_player that referenced this issue Jan 10, 2025
See immich-app/immich#15230 (comment).
So far, only java.lang.Error was caught, but
HttpDataSource.InvalidResponseCodeException is a java.lang.Exception.
mertalev pushed a commit to immich-app/native_video_player that referenced this issue Jan 10, 2025
…player) (#7)

fix: Don't crash on Android if data source throws Exception

See immich-app/immich#15230 (comment).
So far, only java.lang.Error was caught, but
HttpDataSource.InvalidResponseCodeException is a java.lang.Exception.
rovo89 added a commit to rovo89/native_video_player that referenced this issue Jan 10, 2025
See immich-app/immich#15230 (comment).
So far, only java.lang.Error was caught, but
HttpDataSource.InvalidResponseCodeException is a java.lang.Exception.
mertalev pushed a commit to immich-app/native_video_player that referenced this issue Jan 10, 2025
See immich-app/immich#15230 (comment).
So far, only java.lang.Error was caught, but
HttpDataSource.InvalidResponseCodeException is a java.lang.Exception.
@rovo89
Copy link
Contributor

rovo89 commented Jan 11, 2025

Yeah, my proof of concept for Android is working. 🥳 I added the following in ImmichApp.kt:

val keyStore = KeyStore.getInstance("PKCS12")
val clientCertificateContent = getAssets().open("mykey.pfx")
keyStore.load(clientCertificateContent, "mypassword".toCharArray())

val keyManagerFactory = KeyManagerFactory.getInstance(KeyManagerFactory.getDefaultAlgorithm())
keyManagerFactory.init(keyStore, null)

val sslContext = SSLContext.getInstance("TLS")
sslContext.init(keyManagerFactory.keyManagers, null, SecureRandom())
HttpsURLConnection.setDefaultSSLSocketFactory(sslContext.socketFactory)

I successfully tested videos and image downloads. That's for mTLS only. I see no problem accepting any server certificate, the question is whether that would need to be restricted to only the Immich server host like here. But maybe that could be done by checking the hostnames in the certificate... not sure.

Any comments on whether such an approach would be acceptable? Of course, the imported key would need to be used instead of assets (was just easier for testing) and it would have to be reloaded when the key is changed. Also no idea how I can create a dedicate native "component" for this stuff, I might need some assistance with that once the general idea has been confirmed. And sorry, no idea about iOS.

@ckuyehar
Copy link

I successfully tested videos and image downloads.

🥳 congrats! awesome job.

I see no problem accepting any server certificate, the question is whether that would need to be restricted to only the Immich server host like here.

Immich should get out of the business of making that decision and defer that to Android/iOS. Let the OS perform the necessary checks.

Any comments on whether such an approach would be acceptable?

I do have some thoughts... Instead of maintaining/creating another certificate store within the Immich mobile app (abbrev IMA)... IMA should select the certificate from the OS. This will ensure we're using the OS certificate stores (trusted CAs and user) instead of a custom store within IMA. This will also allow IMA (when the time is right) to depreciate "allow self-signed certificates" because the CA would be trusted on the mobile device and a "self-signed" error would never appear.

In other words you should be able to trust the CA, uncheck "allow self-signed certificates" in IMA, select your client certificate in the Android User Store and login to IMA, download images and play videos without issue.

@mertalev
Copy link
Contributor

cc @etnoy if you have any thoughts on this

@bo0tzz
Copy link
Member

bo0tzz commented Jan 11, 2025

Immich should get out of the business of making that decision and defer that to Android/iOS. Let the OS perform the necessary checks.

Flutter has its own certificate handling and doesn't use the system's. That's the whole reason we're here in the first place.

@rovo89
Copy link
Contributor

rovo89 commented Jan 11, 2025

Exactly. It's neither my idea nor my preferred approach to upload the certificate in the Immich app, I'm just following the path that was already taken.

@rovo89
Copy link
Contributor

rovo89 commented Jan 11, 2025

dart-lang/sdk#50435 sounds like no real solution is planned for the HTTP client in Dart, they suggest using package:cronet_http. No idea about that, I'm not familiar with Dart. Indeed it feels a bit clunky having to upload the certificates again that I already configured in the OS, but it's a one-time thing, so it's acceptable for me as a user with a special setup.

I also thought about adding the self-signed certificate to the trust store, which I would anyway do in the OS, but using it from there has the same limitations as above and I'm not sure if yet another upload button in Immich would be worth it. So the simple switch sounds like an acceptable compromise to me.

@LordArrin
Copy link

Right now, the system looks like so overloaded with all of it security stuff, it's no wonder something breaks. I'll join the commentators above, all apps on device MUST trust system CA, it just built for it. Some strange decisions like SSL Pinning can be acceptable in banking or payment apps, but not in userspace.

@LordArrin
Copy link

And yep, I use Immich behind nginx with my personal CA's, and literally all works amasing, but not a video playback and downloading images from Immich on Android. From desktop browsers, android browsers all of it works like charm.

@rovo89
Copy link
Contributor

rovo89 commented Jan 29, 2025

While I generally agree that apps should use the system SSL stack, the reason why this is not the case is in the platform abstraction of Dart, not in the immich app itself. As document above, Dart doesn't use the Android HTTPS client, but rolls its own based on OpenSSL directly.

I don't think switching away from Dart is an option. 😉 There are some ideas to add support for private keys from the Android keystore (which can't be accessed as bytes, the system will take care of encryption): dart-lang/http#1237. Or it has been mentioned in dart-lang/sdk#50435 to use package:cronet_http instead.

But all of these options are either just ideas/wishes without any commitment and timeline, or they would have a bigger impact on the app itself. As it has been made clear that use-cases such as mTLS have low priority, I can't imagine that we'll see such changes anytime soon.

On the other hand, the Immich app does already have options to upload the client certificate. Yes, it's redundant, but for me as a user/admin, that's much better than other workarounds I'd have to implement in the current state (e.g. add another domain without mTLS), or not being able to watch remote videos at all.

The approach I mentioned in #15230 (comment) is fairly low impact, not much code (= less to maintain) and doesn't add any further user-facing workarounds. It also matches the semantics of the existing codes to globally enable the client certificate - just for a different HTTP client.

That's why I'd still like to work on implementing this properly, and I hope to get some agreement from the core devs that such a PR would be merged once it's ready. Can I get that?

@LordArrin
Copy link

Find a temporary solution. I enabled LSPosed module SSLUnpinning for Immich, relaunch app and all videos plays like it may do.

@rovo89
Copy link
Contributor

rovo89 commented Jan 29, 2025

Interesting to see that variants of Xposed are still around, I'm the original author 😉 But nowadays I don't even root my phone anymore, which is the prerequisite.

Besides that, this method might help to disable SSL validation globally (which is a very big hammer), but I don't see how it would make mTLS possible. The private key of the client certificate should be stored in separate hardware, unaccessible even for the Android core, and encryption takes place via IPC as far as I understood.

@etnoy
Copy link
Contributor

etnoy commented Jan 29, 2025

Interesting to see that variants of Xposed are still around, I'm the original author 😉 But nowadays I don't even root my phone anymore, which is the prerequisite.

Besides that, this method might help to disable SSL validation globally (which is a very big hammer), but I don't see how it would make mTLS possible. The private key of the client certificate should be stored in separate hardware, unaccessible even for the Android core, and encryption takes place via IPC as far as I understood.

I have nothing to contribute to this issue, but I just want to thank you for Xposed. It's been a few years but I remember those wild days fondly :)

@polomani
Copy link

polomani commented Feb 5, 2025

@rovo89

I'd have to implement in the current state (e.g. add another domain without mTLS)

Having multiple domains for the app might not work as in that case you'd need to work around the authentication (at least for the web version, it uses cookies, which aren't easily shared between domains).

After some trial and error, I solved it on my setup using HAProxy (it is most likely possible with other reverse proxies, but I needed something with low CPU/RAM consumption).

Here I first define verify optional which will validate the client certificate only if it is available.
Later in the code it checks the URL path, and lets the request through without the client cert only for the video playback endpoints.
Basically, mTLS is required on everything else, except the problematic video playback endpoint.

haproxy.cfg:

    global
      log stdout format raw local0

    defaults
      mode http

    frontend https-in
      # tls-certs should contain two files tls.crt, tls.crt.key (issued by trusted authority)
      # ca.crt is a self-signed CA for mTLS
      bind *:8443 ssl crt /etc/haproxy/tls-certs/ ca-file /etc/haproxy/mtls-ca/ca.crt verify optional

      acl public_video_playback path_reg ^/api/assets/[a-f0-9\-]+/video/playback
      acl client_has_cert ssl_c_used

      # Require mTLS for everything EXCEPT video playback
      http-request deny if !client_has_cert !public_video_playback

      use_backend backend_servers

    backend backend_servers
      server immich-server 127.0.0.1:2283 check # this might need to be adjusted based on setup

This is slightly not ideal, but if there is no proper fix, allows us to keep using mTLS setup and all is behind one domain / ip.
IMHO mutual TLS is a robust, secure, and reliable way for people to expose their services to the internet and should be a recommended approach. Much better then compared to relying on VPNs like Tailscale (as then it can't even be considered as pure self-hosting anymore :) ).

@rovo89
Copy link
Contributor

rovo89 commented Feb 27, 2025

I drafted PR #16403 to support remote video playback and asset downloads with mTLS. Android-only, not sure if this problem even exists on iOS. It shouldn't be too hard to add support for self-signed certificates, but I want to get some feedback first.

@pedropombeiro
Copy link

I drafted PR #16403 to support remote video playback and asset downloads with mTLS. Android-only, not sure if this problem even exists on iOS. It shouldn't be too hard to add support for self-signed certificates, but I want to get some feedback first.

@rovo89 I can confirm that the problem also affects iOS.

@shenlong-tanwen
Copy link
Member

I drafted PR #16403 to support remote video playback and asset downloads with mTLS. Android-only, not sure if this problem even exists on iOS. It shouldn't be too hard to add support for self-signed certificates, but I want to get some feedback first.

The changes required for cronet_http should be rather simple. We should test it a lot to make sure it is stable and that nothing breaks. However, AFAIK, using it will solve the issue only with the network calls made from dart but not the one made from the native side like the video player or the background downloads.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

10 participants