Because of the new cert retrieval logic, upgrades from previous app versions will lead to ignored ca certs

Would it be difficult to make this backwards compatible?

I thought about some kind of migration logic, should be possible.
Then I wondered how you want it to execute?

1. Without interaction fully automatic when "finding" old settings?
2. With a dialog with the option to migrate the configured ca file?
3. ...

I think 1 is preferred but 2 is also fine.

Understood, I will try that

It's now implemented in a3dd80c.
The check is performed before client creation, but because there are several clients created at the same time, the first request fails direct after cert migration.
Clicking retry then fixed it as the client then found the certificate.

A solution could be interactive dialog to restart the app or something, but then the migration does not work in the background without opening the app. With the current implementation the user can just update the app and when the websocket starts, it can (hopefully) migrate it by itself.

This boolean is a little overkill I think, we can check directly if the list is empty like this. What do you think?

val trustManagers = mutableListOf<TrustManager>()
val keyManagers = mutableListOf<KeyManager>()
settings.caCertPath?.let { trustManagers.addAll(certToTrustManager(it)) }
settings.clientCertPath?.let {
    keyManagers.addAll(certToKeyManager(it, settings.clientCertPassword))
}

if (!settings.validateSSL) {
    trustManagers.add(trustAll)
    builder.hostnameVerifier { _, _ -> true }
}
if (trustManagers.isNotEmpty() || keyManagers.isNotEmpty() || !settings.validateSSL) {
    val context = SSLContext.getInstance("TLS")
    context.init(
        keyManagers.toTypedArray(), trustManagers.toTypedArray(), SecureRandom()
    )
    if (trustManagers.isEmpty()) {
        // Fall back to system trust managers
        trustManagers.addAll(defaultSystemTrustManager())
    }
    builder.sslSocketFactory(
        context.socketFactory,
        trustManagers[0] as X509TrustManager
    )
}

I like it. Especially the more Kotlin-like coding with suppression of nullable types and Kotlin helper functions. 👍

I think I would then go with MutableSets instead of MutableLists.
Additionally I would not add trustAll for the case where !settings.validateSSL but rather overwrite previous trustManagers with trustAll because we trust all certificates then anyway.

And the || !settings.validateSSL wouldn't be necessary anymore because the trustManagers is already non-empty in that case.

Something like this. But I still have to test it

val trustManagers = mutableSetOf<TrustManager>()
val keyManagers = mutableSetOf<KeyManager>()
if (settings.validateSSL) {
    // Custom SSL validation
    settings.caCertPath?.let { trustManagers.addAll(certToTrustManager(it)) }
} else {
    // Disable SSL validation
    trustManagers.add(trustAll)
    builder.hostnameVerifier { _, _ -> true }
}
settings.clientCertPath?.let {
    keyManagers.addAll(certToKeyManager(it, settings.clientCertPassword))
}
if (trustManagers.isNotEmpty() || keyManagers.isNotEmpty()) {
    val context = SSLContext.getInstance("TLS")
    context.init(
        keyManagers.toTypedArray(), trustManagers.toTypedArray(), SecureRandom()
    )
    if (trustManagers.isEmpty()) {
        // Fall back to system trust managers
        trustManagers.addAll(defaultSystemTrustManager())
    }
    builder.sslSocketFactory(
        context.socketFactory,
        trustManagers.elementAt(0) as X509TrustManager
    )
}

Why is it possible that the dialogDoneButton is not initialized yet?

1. Everytime the user opens the dialog it checks if the clientCertPath is null or not.
2. Based on the result it calls respective methods showSelectClientCertificate or showRemoveClientCertificate.
3. They include the password check (and try to toggle the dialog button).
4. BUT the button can only be accessed after the dialog has been created.
5. When on the initial check when opening the dialog the button does not exist

Not the most beautiful code I've ever written but it does it's thing. I've tried to implement it without rewriting the whole thing.
Feel free to suggest improvements. :)

This can be a local variable.

var dialog = ..

Close the input stream.

To prevent one NPE for a cert without a name.

You're right. Previously we've used (ca as X509Certificate).subjectDN.name which couldn't even return null.
This one was deprecated, that's why I've moved to (ca as X509Certificate).subjectX500Principal.name.
Never thought about it, but according to some people on StackOverflow it seems a CN isn't necessary for certificates.

If this occurs then all requests will likely fail because the cert isn't used right? I think then we can omit the fallback to reset legacyCert because it's unlikely that this recovers. We just let it in the invalid state and the user has to relogin.

Alright

In #344 (comment) you mentioned that some requests fail because multiple clients are created. Can this be fixed with synchronization? I'd expect this happens when two separate threads are trying to migrate the settings and then one sees the legacy cert, tries to migrate it while the other sees no legacy cert and no certPath because it's not completely migrated yet.

How about we add a new method to the Settings class which is executed on all entry points of the app: GotifyApplication and maybe WebSocketService but I'd expect the Application to be called before that.

fun migrate() {
    synchronized(this::class) {
        if (this.legacyCert != null) {
            Logger.info("Migrating legacy CA cert to new location")
            try {
                val legacyCert = this.legacyCert
                this.legacyCert = null
                val caCertFile = File(filesDir, CertUtils.CA_CERT_NAME)
                FileOutputStream(caCertFile).use {
                    it.write(legacyCert?.encodeToByteArray())
                }
                this.caCertPath = caCertFile.absolutePath
                Logger.info("Migration of legacy CA cert succeeded")
            } catch (e: IOException) {
                Logger.error(e, "Migration of legacy CA cert failed")
            }
        }
    }
}

Tried it out, unfortunately still gives an error, but clicking "Retry" now fixes it for me

Nice improvement (:.

Yeah, I had to add the settings object to all client factory methods.
That's was the motivation to simplify and unify the method signatures overall.

I've debugged nearly an hour why this didn't worked for me and guess what the PKCS12 key provider didn't exist on my emulator. On my real device this works fine but on my emulator this error was logged. I couldn't really find anything online regarding to this, so it's probably some edge-case that doesn't happen in real devices.

Failed to apply SSL settings: java.io.IOException: error constructing MAC: java.security.InvalidKeyException: No installed provider supports this key: com.android.org.bouncycastle.jcajce.PKCS12Key
    at com.android.org.bouncycastle.jcajce.provider.keystore.pkcs12.PKCS12KeyStoreSpi.engineLoad(PKCS12KeyStoreSpi.java:852)
    at java.security.KeyStore.load(KeyStore.java:1484)
    at com.github.gotify.api.CertUtils.certToKeyManager(CertUtils.kt:118)
    at com.github.gotify.api.CertUtils.applySslSettings(CertUtils.kt:63)
    at com.github.gotify.api.ClientFactory.defaultClient(ClientFactory.kt:80)
    at com.github.gotify.api.ClientFactory.unauthorized(ClientFactory.kt:21)
    at com.github.gotify.api.ClientFactory.versionApi(ClientFactory.kt:49)
    at com.github.gotify.login.LoginActivity.doCheckUrl(LoginActivity.kt:147)
    at com.github.gotify.login.LoginActivity.onPostCreate$lambda$6(LoginActivity.kt:118)
    at com.github.gotify.login.LoginActivity.$r8$lambda$YPO6IepZ2o2Scolja7nou-1QsOA(Unknown Source:0)
    at com.github.gotify.login.LoginActivity$$ExternalSyntheticLambda4.onClick(D8$$SyntheticClass:0)
    at android.view.View.performClick(View.java:7455)
    at com.google.android.material.button.MaterialButton.performClick(MaterialButton.java:1218)
    at android.view.View.performClickInternal(View.java:7432)
    at android.view.View.access$3700(View.java:835)
    at android.view.View$PerformClick.run(View.java:28810)
    at android.os.Handler.handleCallback(Handler.java:938)
    at android.os.Handler.dispatchMessage(Handler.java:99)
    at android.os.Looper.loopOnce(Looper.java:201)
    at android.os.Looper.loop(Looper.java:288)
    at android.app.ActivityThread.main(ActivityThread.java:7870)
    at java.lang.reflect.Method.invoke(Native Method)
    at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:548)
    at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:1003)

Oh no, I've actually never tried it with an emulator :(
I suppose the MAC generation (via bouncy castle) is supported by the hardware security chip which does not exist in an emulator.

We could disallow mTLS on emulators.