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

Add Initial Certificate Override Support #518

Closed
wants to merge 2 commits into from
Closed

Add Initial Certificate Override Support #518

wants to merge 2 commits into from

Conversation

OdinVex
Copy link

@OdinVex OdinVex commented Oct 22, 2023

Back-end work to provide certificate override functionality added. User Interface to manage history and provide context should be added. Clarification when new certificates for a host:port combination (if invalid) are added versus never having been approved should be given to the user. Allow them to reject if the remote end has changed (or is behind a load-balancer with multiple certs/ports...)

Things still needed to complete. When users connect any time and there is a certificate error several things should happen:

  1. The back-end should be queried to determine whether any invalid certificate has ever been overridden for the host:port combination.

  2. If no certificate has been overridden present a dialog to allow the connection temporarily, permanently, or cancel. The dialog should warn the user that their connection may not be secure as the certificate supplied is invalid*.

  3. If a certificate has been overridden in the past but this certificate is a different invalid certificate warn the user that the previous certificate used to access the server has changed. Prompt the user to allow the connection temporarily, permanently, or cancel.

Provide a means of editing these overrides. The front-end just needs development now. I didn't want to implement any bridge between the front-end and back-end (such as a way to retrieve the actual overrides), I figured that would be best left to someone that can use Dart/Flutter much more effectively.

* Also note that you'll need to modify the "on connection error dialog" area of code to actually differentiate what kind of error occurred instead of just showing that small notification. Try to determine if it is invalid because the certificate is self-signed, expired, or could not be verified. I'm not sure of the specific error codes, but it looked like this uses code-words (unique and in latin, so ASCII). That will make it easy if you can't do it by error code. *

Helpful Code Specifics:

Files:
lib/main.dart
lib/models/finamp_models.dart
lib/models/finamp_models.g.dart
lib/services/finamp_settings_helper.dart

Classes:
MyHttpOverrides (lib/main.dart)

Functions:
FinampSettingsHelper.addCertificateOverride(host, port, thumbprint)
FinampSettingsHelper.deleteCertificateOverride(host, port, thumbprint)
FinampSettingsHelper.resetCertificateOverrides()
FinampSettingsHelper.hasCertificateOverride(host, port, thumbprint)

Variables:
Map<String, Set<String>> FinampSettings.overriddenCertificates \* Expects "host:port" key, "thumbprint" value.

* Invalid could also simply mean self-signed or not trusted for whatever reason. If it's expired, inform them. If it's self-signed, inform them. If it's an unrecognized authority, inform them.

Edit: Corrected three brain-fart typos, cert.sha1 to thumbprint.

Backend work to provide certificate override functionality added. User Interface to manage history and provide context should be added. Clarification of when new certs for a domain (if not valid) are added versus never having been approved should be noted to the user. Allow them to reject if the remote end has changed (or is behind a load-balancer with multiple certs/ports...)

Signed-off-by: Odin Vex <[email protected]>
@jmshrv
Copy link
Owner

jmshrv commented Oct 22, 2023

Thanks for working on this! This has been a popular request for ages. The code looks good, we just need a frontend. The most secure way of handling this would be checking thumbprints as you said in #106, as you may as well not use HTTPS at all if you're just going to ignore any certificate errors.

If I have time tomorrow, I'll take a look at the TODOs here and finally get this issue fixed.

@OdinVex
Copy link
Author

OdinVex commented Oct 22, 2023

I'd recommend presenting certificate information (at least something) besides thumbprint if possible, but yes, thumbprints are usually compared. Technically you could have hash collisions, but the odds are so very far against it (computationally unfeasible unless the particular thumbprint algorithm used in the back-end library gets entirely broken). The functionality can be altered to compare the entire certificate if desired. As noted in the documentation, the certificate der would be the end solution in that desire of trust, but you'd be storing the certificate itself per host:port combination and as a listed object. (I added functionality to store every thumbprint per `host:port combination as a possibility, so you could run proxy servers/load-balancers/P2P-style setups if desired and get notified for any/all certificates but also allow to remember each one individually). Granted those are insane scenarios you'd probably only end up a hundred or so MB over a lifetime at most, assuming you rotated invalid certificates constantly such as every connection (quite paranoid to do that).

@jmshrv
Copy link
Owner

jmshrv commented Oct 23, 2023

If I have time tomorrow

I did not... Tomorrow hopefully :)

@OdinVex
Copy link
Author

OdinVex commented Oct 24, 2023

If I have time tomorrow

I did not... Tomorrow hopefully :)

@Chaphasilor
Copy link
Collaborator

Just some thoughts regarding the integration into the login flow (because that's the part where this feature will be used most of the time):

  1. The user starts by entering the server URL. We could try to fetch the server information after nothing has been entered for ~750ms, with certificate checking fully disabled (just to see if the URL is correct and pointing to a Jellyfin server)
  2. After the user confirms the URL, we again fetch the server and user info, but this time with certificate validation enabled. So if the server uses a self-signed cert, we would prompt the user about it here, presenting the a short description saying what the issue is, showing some info about the cert, and two buttons. This way we also don't need a checkbox or advanced settings.
  3. After the user logged into the app, they can manage the known certs in a new section within the settings. The section would contain all certs/thumbprints for the currently used domain only, since we would need to logout anyway to use a different domain. Next to each thumbprint, there should be a delete button, and tapping on a thumbprint should open a modal or bottom sheet showing any information that we have about the cert, with a close and a delete button.

Also, I'm not sure if the current backend code solves the issue of native code using its own certificate validation, e.g. when streaming music with Exoplayer or downloading songs. Hopefully there are ways to re-use the existing logic for these, e.g. by using some just_audio/flutter_downloader constructor argument...

@OdinVex
Copy link
Author

OdinVex commented Oct 24, 2023

1. The user starts by entering the server URL. We _could_ try to fetch the server information after nothing has been entered for ~750ms, with certificate checking fully disabled (just to see if the URL is correct and pointing to a Jellyfin server)

I'm not at all a fan of keystroke monitoring even for benign stuff like this.

2. After the user confirms the URL, we again fetch the server and user info, but this time with certificate validation enabled. So if the server uses a self-signed cert, we would prompt the user about it here, presenting the a short description saying what the issue is, showing some info about the cert, and two buttons. This way we also don't need a checkbox or advanced settings.

Redundant, you'd still need to do this anyway. No checkbox is needed. The front-end just needs to pull the reason for failure from the back-end and then present a dialog as appropriate.

3. After the user logged into the app, they can manage the known certs in a new section within the settings. The section would contain all certs/thumbprints for the currently used domain only, since we would need to logout anyway to use a different domain. Next to each thumbprint, there should be a delete button, and tapping on a thumbprint should open a modal or bottom sheet showing any information that we have about the cert, with a close and a delete button.

I don't think it is a good idea to limit it to the currently-used domain. Granted, you could make it first on the list but give the user the ability to manage all. Agreed with the rest, though. I thought about storing the certificate instead of the thumbprint and doing a cast to check just the thumbprint. This would allow 'offline/unconnected' viewing of certificate data for other certificates.

Also, I'm not sure if the current backend code solves the issue of native code using its own certificate validation, e.g. when streaming music with Exoplayer or downloading songs. Hopefully there are ways to re-use the existing logic for these, e.g. by using some just_audio/flutter_downloader constructor argument...

This is purely for the Finamp-side of things. If Exoplayer respects system-store certificates then it isn't a problem.

Edit: I've just confirmed that ExoPlayer is just as stupid as Flutter regarding the system-store of certificates. So even though you can browse the Jellyfin server with certificate overrides, the ExoPlayer needs a work-around. Anyone able to TLDR the ExoPlayer's instantiation for me? Maybe I can tie the ExoPlayer into this functionality, too.

Edit: ExoPlayer: This project is deprecated. All users should migrate to AndroidX Media3. Please refer to our [migration guide and script](https://developer.android.com/guide/topics/media/media3/getting-started/migration-guide) to move your codebase to the Media3 package names. ?

Edit: I figured I'd try to find the player's code to add an override. Yeah, no. Figured I'd try to patch just_audio's http client. Nope. Tried sticking to downloads to play them locally (a poor work-around, but a work-around no less). Nope, the downloader uses Flutter's backend. I can't stand this hostile-to-user (no overrides ground-up, not allowed to trust your own certificates as you please) Flutter environment, so I'mma stick to Jellyfin's official software for now. Edit: On a side-note, you could provide a local proxy and feed the players with some URL rewriting, but eh...

@PlexSheep
Copy link

Another idea that might cover some usecases while staying true to security would be to let the user add their own CA. That would help distinguish between malicious or bad certificates and real certificates, that are not recognized by the app.

@OdinVex
Copy link
Author

OdinVex commented Oct 25, 2023

Another idea that might cover some usecases while staying true to security would be to let the user add their own CA. That would help distinguish between malicious or bad certificates and real certificates, that are not recognized by the app.

Flutter/Dart is stupid and doesn't actually use the system store nor does the back-end allow adding certificates for ExoPlayer. All of the involved components are...stupid about certificates, mostly because Google forces almost all software to never allow users their own freedom and control to decide for themselves which certificate to trust. I blame Mozilla mostly for starting this trend of bundling certificates. They age. They need replacement, updating...and management. This is why I was trying to provide a short-hand way of ignoring certificates you approve of, but they don't use a single-point of validation.

One of two other means right now would be to do a local proxy by using the HTTP client to grab (it can be overridden) and then mirror-host it locally, then rewrite the links for ExoPlayer to those local links, but this is a last-resort untrusted method as it allows any software on the phone to connect to that port. Perhaps handing single-use tokens back and forth 'per request' could be used to prevent a bad actor from initial connection... I'm not going through all that trouble to add playback, I'll stick to Jellyfin. With CSS customization...and maybe some JavaScript injection you should be able to have the same functionality and layout even.

The other means of getting it to kinda work would be to use HTTP client to DOWNLOAD the files first, then play them locally as files. ExoPlayer would work, then. I don't have a grasp of Finamp from such a small interaction with it to figure out where it gives the URL to simply add a MITM rewriter if the cert is overridden and use locally. It would circumvent the entire issue but you wouldn't be streaming in memory.

@PlexSheep
Copy link

PlexSheep commented Oct 25, 2023

@OdinVex I know the library does not allow it (as mentioned in #106). My idea was rather to be able to add the CA in the app, not the system.

But I don't know a lot about android development, maybe it's not doable.

@OdinVex
Copy link
Author

OdinVex commented Oct 25, 2023

@OdinVex I know the library does not allow it (as mentioned in #106). My idea was rather to be able to add the CA in the app, not the system.

But I don't know a lot about android development, maybe it's not doable.

Yeah, you missed what I said. It's all good, though. I think the only workable solution until Flutter stops sucking ass is to either store the files temporarily (on-click, download the file and then tell the player to use the local filesystem) or to do a local proxy with HTTPS->HTTP rewriting (more stream-like, no need to download, full functionality, but this is frowned upon).

@jmshrv
Copy link
Owner

jmshrv commented Oct 25, 2023

We may need to provide a network security configuration for Android - https://developer.android.com/privacy-and-security/security-config

Also I may have to move my deadline up to the weekend, I've had no time to work on Finamp this week 😔

@OdinVex
Copy link
Author

OdinVex commented Oct 25, 2023

We may need to provide a network security configuration for Android - https://developer.android.com/privacy-and-security/security-config

Also I may have to move my deadline up to the weekend, I've had no time to work on Finamp this week 😔

That would only help if you add final certificates to the APK bundle itself. You'd need everyone to add their certificates.

@jmshrv
Copy link
Owner

jmshrv commented Oct 26, 2023

Android started ignoring the user certificate store a while ago for security:tm:. See https://developer.android.com/privacy-and-security/security-config#base-config

@jmshrv
Copy link
Owner

jmshrv commented Oct 26, 2023

Looks like ExoPlayer does something different, will investigate: ryanheise/just_audio#442

As for ExoPlayer being deprecated, that's an issue for just_audio, not us luckily

@jmshrv
Copy link
Owner

jmshrv commented Oct 26, 2023

ugh, iOS also needs manual intervention...

Almost as if self-signed certs are a bad idea ;)

@jmshrv
Copy link
Owner

jmshrv commented Oct 26, 2023

hmm maybe not? I may have messed up my certificate configuration

@jmshrv
Copy link
Owner

jmshrv commented Oct 26, 2023

Yep, needed to specify the IPs that I was using. Luckily this is only a slight pain to fix now 🙃

@OdinVex
Copy link
Author

OdinVex commented Oct 26, 2023

Android started

Android does not at all ignore the system (by default, and also trusts user < Android 6.0) certificate store, that's how organizations manage certificates for devices. 6.0 onward needs user opt-in, which quite frankly should be default-on. By default, secure connections (using protocols like TLS and HTTPS) from all apps trust the pre-installed system CAs, and apps targeting Android 6.0 (API level 23) and lower also trust the user-added CA store by default. System is included by default and my phone is rooted with a copy-to-system certificate, so it should work (it does, with all other software). This would imply Flutter is stupidly bundling its own and only using that (eg manual TCP/UDP, connection handling, eg not using Android APIs). On a separate note: If you don't want your app to trust all CAs trusted by the system, you can instead specify a reduced set of CAs. (System is trusted by default. User, not so on Android 6.0+. Again, mine is system, so it should be working unless Flutter is not using the system store.)

Looks like ExoPlayer does something different, will investigate: ryanheise/just_audio#442

As for ExoPlayer being deprecated, that's an issue for just_audio, not us luckily

just_audio uses ExoPlayer. You use just_audio. So it is an issue for Finamp.

ugh, iOS also needs manual intervention...

Almost as if self-signed certs are a bad idea ;)

Self-signed certificates aren't a bad idea, every certificate authority is a self-signed certificate, they're just pre-installed and pre-trusted. That's how certificate authorities work. The problem is that they don't want you using certificates the world won't trust as a certificate that hasn't been signed by pre-installed certificates. Traditionally, certificates have been rather expensive to maintain and only in recent years have they come down in price due to a single competitor (Let's Encrypt, free, but not everyone wants to bother maintaining libraries for software to use Let's Encrypt).

Edit: I think Flutter may behave differently based on whether you add system store/user store. It's working with my certificate (system-wide installed which means trusted by default, but it wasn't being used by Flutter/Dart/ExoPlayer). This leads me to think Flutter behaves differently purposefully.

Copy link
Author

@OdinVex OdinVex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should help anyone that added User-level certificates. System is trusted by default, but maybe the back-end doesn't use System but allows User if it is allowed.

@jmshrv
Copy link
Owner

jmshrv commented Oct 27, 2023

Did some more work on this today (not ready to commit yet), mostly plumbing for showing the certificate error screen

@OdinVex OdinVex closed this by deleting the head repository Dec 6, 2023
@Chaphasilor
Copy link
Collaborator

Why did you close the PR? :)

@OdinVex
Copy link
Author

OdinVex commented Dec 6, 2023

I forgot that deleting my GH repository that hosted the PR would do that. X_x; I've migrated most of my code to my privately-hosted Gitea instance and I'm trying to get away from Microsoft.

Edit: Fortunately you can see the files changed and see the direction I was headed to provide easy management of certificates once the backend had certificate-store management added. Easy enough to implement.

@Chaphasilor
Copy link
Collaborator

Haha okay, that makes sense! I'll create a new branch here on this repository and re-implement your changes there. Should I commit it under your name/email?

@OdinVex
Copy link
Author

OdinVex commented Dec 6, 2023

Haha okay, that makes sense! I'll create a new branch here on this repository and re-implement your changes there. Should I commit it under your name/email?

I'm easy going, your choice. :)

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

Successfully merging this pull request may close these issues.

4 participants