Add client certificate support (https://github.com/gotify/android/issues/229) #230
Conversation
| @@ -100,6 +105,7 @@ private void invalidateUrl() { | |||
| checkUrlButton.setText(getString(R.string.check_url)); | |||
| } | |||
|
|
|||
| @RequiresApi(api = Build.VERSION_CODES.O) | |||
There was a problem hiding this comment.
This method must not have the RequiredApi annotation, wrap the client cert code into an if similar to
The client cert options inside the advanced settings dialog should be hidden / disabled if Build.VERSION.SDK_INT < Build.VERSION_CODES.O
Currently, if an android version less than Build.VERSION_CODES.O is used, then the app will crash when base64 stuff is accessed, because these classes don't exist in older android versions.
(only briefly looked at the PR, will look into your remarks later this week)
There was a problem hiding this comment.
Ok, those guards have been put into place. Tested on 4a API 30 and 3 API 24 and everything works against my server with and without client certificates respectively.
| settings.clientCert(clientCertContents); | ||
| settings.clientCertUri(path); |
There was a problem hiding this comment.
Why set the settings here? Now you have clientCertContents and settings.clientCert which have the same value. Values should only be set on the settings inside com.github.gotify.login.LoginActivity#onCreatedClient
Also the settings.clientCert won't be unset on remove client certificate, so the setting/variable become out of sync.
.onClickRemoveClientCertificate(() -> {
clientCertContents = null;
settings.clientCertPass("");
clientCertPassword = "";
settings.clientCertUri(null);
clientCertUri = null;
})
There was a problem hiding this comment.
Resetting the setting values in that handler is done because I thought it was similar to the existing onClickRemoveCaCertificate above reset the ca cert contents on removal.
I will add the missing clientCert reset.
| } catch (Exception e) { | ||
|
|
||
| if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) { | ||
| if (settings.clientCert != null) { |
There was a problem hiding this comment.
I couldn't get client certificates working together with a self-signed server certificate, could you test if this works correctly. Maybe the SSLContext gets overridden because
SSLContext context = SSLContext.getInstance("TLS");
context.init(...);
is called two times.
There was a problem hiding this comment.
Yes, the problem was I was not adding the client cert to the key store for branch related to self-signed certs at all. It now tries to add the client cert to that as well if possible. I've retested against a real cert and it's still working. I am having issues configuring my docker gotify for a self-signed cert (I currently have an installation that uses caddy and real certs). If there's anyway you can retest with your setup in the meantime, it would be appreciated.
| } | ||
|
|
||
| new AlertDialog.Builder(context) | ||
| .setView(dialogView) | ||
| .setTitle(R.string.advanced_settings) | ||
| .setPositiveButton(context.getString(R.string.done), (ignored, ignored2) -> {}) | ||
| .setPositiveButton(context.getString(R.string.done), (ignored, ignored2) -> { | ||
| settings.clientCertPass(holder.editClientCertPass.getText().toString()); |
There was a problem hiding this comment.
Settings should only be set inside com.github.gotify.login.LoginActivity#onCreatedClient
There was a problem hiding this comment.
This is my first android code, so help me here. The other buttons on the advanced screen start file selection activities that end up being handled by an onActivityResult in LoginActivity.java. I was not sure how to get the value of the client cert password except through a lambda that is run after the done button is clicked. Do you have another way? I do see settings being modified in other functions besides onCreatedClient.
There was a problem hiding this comment.
I'm running out of things to try in creating the self-signed certificate. I've tried a couple of different things and the LOG on the login page says Hostname .... not verified. I've tried selecting my CA root and the server cert and I can see that it is trying to validate the CA cert. This is all on the master branch by the way.
There was a problem hiding this comment.
I use caddy for testing the self-signed certificate thingy in gotify (it works for me on master and your branch)
Caddyfile:
{
http_port 8000
https_port 8443
}
192.168.178.2 {
tls internal
reverse_proxy localhost:8080
}
gotify/server listens on localhost:8080, my desktop PC is 192.168.178.2. When starting caddy run -config Caddyfile, then Caddy will create certificates at ~/.local/share/caddy/certificates/local/192.168.178.2 and the crt is located at 192.168.178.2.crt.
In gotify I'll use https://192.168.178.2:8443 as server url. This error should be logged, when the ca .crt file isn't configured inside gotify, but the connection works after the cert is configured.
Exception
2022-05-16T07:26:07.679Z ERROR: Error while api call
Code(0) Response:
at com.github.gotify.api.Callback$RetrofitCallback.onFailure(Callback.java:80)
at retrofit2.ExecutorCallAdapterFactory$ExecutorCallbackCall$1$2.run(ExecutorCallAdapterFactory.java:80)
at android.os.Handler.handleCallback(Handler.java:883)
at android.os.Handler.dispatchMessage(Handler.java:100)
at android.os.Looper.loop(Looper.java:214)
at android.app.ActivityThread.main(ActivityThread.java:7356)
at java.lang.reflect.Method.invoke(Native Method)
at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:492)
at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:930)
Caused by: javax.net.ssl.SSLHandshakeException: java.security.cert.CertPathValidatorException: Trust anchor for certification path not found.
at com.android.org.conscrypt.ConscryptFileDescriptorSocket.startHandshake(ConscryptFileDescriptorSocket.java:231)
at okhttp3.internal.connection.RealConnection.connectTls(RealConnection.java:319)
at okhttp3.internal.connection.RealConnection.establishProtocol(RealConnection.java:283)
at okhttp3.internal.connection.RealConnection.connect(RealConnection.java:168)
at okhttp3.internal.connection.StreamAllocation.findConnection(StreamAllocation.java:257)
at okhttp3.internal.connection.StreamAllocation.findHealthyConnection(StreamAllocation.java:135)
at okhttp3.internal.connection.StreamAllocation.newStream(StreamAllocation.java:114)
at okhttp3.internal.connection.ConnectInterceptor.intercept(ConnectInterceptor.java:42)
at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.java:147)
at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.java:121)
at okhttp3.internal.cache.CacheInterceptor.intercept(CacheInterceptor.java:93)
at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.java:147)
at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.java:121)
at okhttp3.internal.http.BridgeInterceptor.intercept(BridgeInterceptor.java:93)
at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.java:147)
at okhttp3.internal.http.RetryAndFollowUpInterceptor.intercept(RetryAndFollowUpInterceptor.java:126)
at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.java:147)
at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.java:121)
at okhttp3.RealCall.getResponseWithInterceptorChain(RealCall.java:254)
at okhttp3.RealCall$AsyncCall.execute(RealCall.java:200)
at okhttp3.internal.NamedRunnable.run(NamedRunnable.java:32)
at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1167)
at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:641)
at java.lang.Thread.run(Thread.java:919)
Caused by: java.security.cert.CertificateException: java.security.cert.CertPathValidatorException: Trust anchor for certification path not found.
at com.android.org.conscrypt.TrustManagerImpl.checkTrustedRecursive(TrustManagerImpl.java:658)
at com.android.org.conscrypt.TrustManagerImpl.checkTrustedRecursive(TrustManagerImpl.java:617)
at com.android.org.conscrypt.TrustManagerImpl.checkTrusted(TrustManagerImpl.java:507)
at com.android.org.conscrypt.TrustManagerImpl.checkTrusted(TrustManagerImpl.java:426)
at com.android.org.conscrypt.TrustManagerImpl.getTrustedChainForServer(TrustManagerImpl.java:354)
at android.security.net.config.NetworkSecurityTrustManager.checkServerTrusted(NetworkSecurityTrustManager.java:94)
at android.security.net.config.RootTrustManager.checkServerTrusted(RootTrustManager.java:89)
at com.android.org.conscrypt.Platform.checkServerTrusted(Platform.java:224)
at com.android.org.conscrypt.ConscryptFileDescriptorSocket.verifyCertificateChain(ConscryptFileDescriptorSocket.java:407)
at com.android.org.conscrypt.NativeCrypto.SSL_do_handshake(Native Method)
at com.android.org.conscrypt.NativeSsl.doHandshake(NativeSsl.java:387)
at com.android.org.conscrypt.ConscryptFileDescriptorSocket.startHandshake(ConscryptFileDescriptorSocket.java:226)
... 23 more
Caused by: java.security.cert.CertPathValidatorException: Trust anchor for certification path not found.
... 35 more
I still cannot get the client certificate working with my setup, I get a 421 misdirected redirect from caddy. But I don't really know what I'm doing there, never used client certificates.
There was a problem hiding this comment.
I will try Caddy's internal CA next. I was creating a local CA and a server cert and signing it manually and configuring Caddy to use that.
There was a problem hiding this comment.
Ok this is what worked for me:
I was getting all my cert files confused and was trying to use the wrong file for the client cert. Also, by using the Caddy auto-cert I am getting around some apparent issue with how I was trying to manually create the CA and server cert (a problem for another day). In my case, the client cert and client CA are both separately generated and totally unrelated to the server cert (Caddy autogen cert or otherwise).
Commands to create the client certificate:
openssl req -x509 -newkey rsa:2048 -keyout gotify_client_cert.key -out gotify_client_cert.crt -days 3000
openssl req -new -key gotify_client_cert.key -out gotify_client_cert.csr
openssl x509 -req -days 3000 -in gotify_client_cert.csr -signkey gotify_client_cert.key -out gotify_client_cert-CA.crt
cat gotify_client_cert.crt gotify_client_cert.key > gotify_client_cert.pem
openssl pkcs12 -export -out gotify_client_cert.p12 -inkey gotify_client_cert.key -in gotify_client_cert.pem
Caddyfile:
(require_client_cert) {
tls {
client_auth {
mode require_and_verify
trusted_ca_cert_file /client_certs/gotify_client_cert-CA.crt
trusted_leaf_cert_file /client_certs/gotify_client_cert.crt
}
}
}
* *.home.lan {
import require_client_cert
tls internal
reverse_proxy http://gotify:80
}
Docker compose file:
version: '3.3'
services:
caddy:
image: caddy:2.4.6-alpine
ports:
- "3080:80"
- "3443:443"
volumes:
- ./Caddyfile:/etc/caddy/Caddyfile
- ./.ssh:/client_certs
gotify:
image: gotify/server:2.1.4
volumes:
caddy_data1:
There was a problem hiding this comment.
I'll have a look at this on the weekend.
| }) | ||
| .onClickRemoveCaCertificate( | ||
| () -> { | ||
| invalidateUrl(); | ||
| caCertContents = null; | ||
| }) | ||
| .show(disableSSLValidation, selectedCertName); | ||
| .onClickSelectClientCertificate(() -> | ||
| doSelectCertificate(R.string.select_client_cert_file, CLI_CERT_FILE_SELECT_CODE)) |
There was a problem hiding this comment.
Call invalidateUrl before selecting a certificate.
| .onClickSelectClientCertificate(() -> | ||
| doSelectCertificate(R.string.select_client_cert_file, CLI_CERT_FILE_SELECT_CODE)) | ||
| .onClickRemoveClientCertificate(() -> { | ||
| clientCertContents = null; |
| } catch (IOException iex) { | ||
| String tx = iex.toString(); | ||
| if (iex.toString().contains("wrong password")) { | ||
| Log.e("Incorrect client certificate password.", iex); | ||
| return; | ||
| } | ||
|
|
||
| Log.e("Error opening client certificate.", iex); | ||
| } |
There was a problem hiding this comment.
This catch can be omitted, because the exception message is already sufficient and says that the password is wrong / the certificate is invalid.
| @@ -77,6 +78,20 @@ public void onPrepareLoad(Drawable placeHolderDrawable) {} | |||
| }; | |||
| } | |||
|
|
|||
| public static String binaryFileToBase64(@NonNull InputStream inputStream) { | |||
There was a problem hiding this comment.
This method needs a @RequiresApi annotation, see build error.
The usage of this method then must be wrapped into an if inside com.github.gotify.login.LoginActivity#onActivityResult.
| TrustManagerFactory trustManagerFactory = TrustManagerFactory.getInstance( | ||
| TrustManagerFactory.getDefaultAlgorithm()); | ||
| trustManagerFactory.init((KeyStore) null); | ||
| TrustManager[] trustManagers = trustManagerFactory.getTrustManagers(); | ||
| if (trustManagers.length != 1 || !(trustManagers[0] instanceof X509TrustManager)) { | ||
| throw new IllegalStateException("Unexpected default trust managers:"); | ||
| } | ||
| X509TrustManager trustManager = (X509TrustManager) trustManagers[0]; |
There was a problem hiding this comment.
Why is this check needed, couldn't we pass all the trust managers into context.init(keyManagers, trustManagers, null);?
There was a problem hiding this comment.
I believe that was in some of the code I used as reference. I will check those calls and if there's always a default trust manager, we could remove it.
|
Should we try to implement this again? |
|
Yeah, this feature is useful, it was also requested in a reddit thread somewhere. |
|
I've now worked a bit on this. My current setup responds with 308, according to curl with what seems a redirection loop: where gotify runs on port 80. I do not have any experience with reverse proxies, any ideas? |
|
Are you accessing the https port? The doubled server header seems suspicious, is the request proxied twice? You can try enable debug logging and look what caddy is saying. |
|
Nevermind, I got it working (incl. client side certificate authentication). |
So a few things to point out:
The
@RequiresApi(api = Build.VERSION_CODES.O)annotations were added because Android studio complained about using some of the base64 classes. Everything appeared to run without them, but I'm not sure if it's best practice to not have them.The 3 client cert settings added were to keep the advanced settings dialog in line with how it currently behaves - there's no way to guarantee being able to show a name based on the DN of a self-created cert, so I show the file name of the actual picked cert file.
CertUtils.java:104 will catch the correct exception when the password for the cert is incorrect although it is hacky having to check the exception text (not sure why it's such a generic exception type). With that said, the current way the exception handling is routing and the snackbar messages are constructed, this message will never be shown to the user. They will get another message about an error 0, which I believe the SSL connection failing to handshake.
Ideally, with a refactor of the advanced settings screen, the user could test the client cert password before committing the login information permanently to settings.
Let me know what you think and if you want anything tweaked.