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

Client certificate authentication (mTLS) #344

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

cyb3rko
Copy link
Contributor

@cyb3rko cyb3rko commented Apr 19, 2024

Continuation of #230
Closes #85
Related to gotify/server#416

I've adopted some of the code and translated it to Kotlin, but most parts are rewritten.
Here's a summary:

  • You can now configure a client certificate (PKCS#12) including password in the advanced settings (on the login screen)
  • The certificate contents are no longer stored in the shared preferences but the certificates are copied as a whole over to the app's file directory; the helper functions all just require input streams, so we cut the additional loading from shared preferences into another local bytestream beforehand, which then was forwared to the helper functions
  • Because of the new cert retrieval logic, upgrades from previous app versions will lead to ignored ca certs
  • We can not preview the CN of the client cert as with the CA cert as it's encrypted
  • I just know login and basic messages work when configuring no CA but a client cert

Testing the following cases is still to be done:

  • Fully functional when not configuring anything
  • Fully functional when only configuring CA cert
  • Fully functional when only configuring client cert (fixed in 60946e4)
  • Fully functional when configuring CA cert and client cert

Maybe taking a look at image loading is important as well.

And I will add some kind of hint that you have to give a password, it seems like (according to my trial and error) the Java client key implementation requires one.


For reference my reverse proxy settings (Caddy, docs for client_authentication):

(require_client_cert) {
    tls {
        client_auth {
            mode require
            trusted_leaf_cert_file client.crt
        }
    }
}

my.domain:1234 {
    import require_client_cert
    # gotify on port 8080
    reverse_proxy localhost:8080
    tls internal
}

@cyb3rko cyb3rko marked this pull request as draft April 19, 2024 00:16
@cyb3rko
Copy link
Contributor Author

cyb3rko commented Apr 21, 2024

@jmattheis I've just wondered what the real advantage of the import of a CA cert inside the app is over the system-wide native CA import.
Or do some older Android versions not have this feature?

@jmattheis
Copy link
Member

I don't know. This feature was added added more than 5 years ago. I could imagine that some users may not want to globally trust the CA cert and what to only configure this for certain apps.

@cyb3rko cyb3rko marked this pull request as ready for review April 23, 2024 20:46
@cyb3rko cyb3rko marked this pull request as draft April 23, 2024 21:02
@cyb3rko
Copy link
Contributor Author

cyb3rko commented Apr 23, 2024

Oh wait, I wanted to implement some kind of hint to add a password when selecting a client cert.
The crypto library apparently enforces a non-empty password.

And btw, while merging the master another problem occured:
The refactor to coil also updated the okhttp version.
And this code

@Suppress("DEPRECATION")
builder.sslSocketFactory(context.socketFactory)

can not be suppressed anymore, it's now an error. So we can't just leave the trust manager empty anymore.

PS: Fixed it by inserting default trust managers

@cyb3rko cyb3rko marked this pull request as ready for review April 23, 2024 22:23
Copy link
Member

@jmattheis jmattheis left a comment

Choose a reason for hiding this comment

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

Haven't tested the new feature yet.

@desbma
Copy link

desbma commented May 24, 2024

Any update on this?
I would love to get this feature merged, and since there has already been some significant discussion in the review, it would be a shame to see this stall.

@jmattheis
Copy link
Member

FYI: I'll review this pr on the weekend.

@cyb3rko
Copy link
Contributor Author

cyb3rko commented Jun 5, 2024

And FYI: The implementation of the changes mentioned in the two remaining PR comments is still pending.
Atm struggling again with creating working certificates. I should have documented the steps last time...

@cyb3rko
Copy link
Contributor Author

cyb3rko commented Jun 6, 2024

That should be it for now. The mentioned improvements are all implemented.

Comment on lines +36 to +38
var caCertCN: String?
get() = sharedPreferences.getString("caCertCN", null)
set(value) = sharedPreferences.edit().putString("caCertCN", value).apply()
Copy link
Member

Choose a reason for hiding this comment

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

This setting is unused.

Copy link
Contributor Author

@cyb3rko cyb3rko Jun 11, 2024

Choose a reason for hiding this comment

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

It wasn't in use previously, right?
Then we could safely remove it.

app/src/main/kotlin/com/github/gotify/api/CertUtils.kt Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

feature request: option to send certificate for NGINX ssl client verification
3 participants