Add support for client certificates #771
Conversation
stephanritscher
force-pushed
the
master
branch
from
79453ce to
a4e1b29
Compare
|
Sorry for the delay, I will look into it as soon as I have time after my bachelor thesis. Thanks for your pull request. |
|
Is it possible to keep minSdk 21? Else this would be a rather drastic change, I think a few people are running Grocy Android on old tablets e. g. in the kitchen, these devices often have early Android versions. |
|
Hi, this is due to InteractiveKeyManager which is my own project and not too big, so I could give it a try if you are interested in integrating the PR. |
|
I see, this would be cool! Maybe you could just surround the methods that don't exist on API <23 with if/else in your library? So that the client certificats feature is maybe only working on devices with API 23+ and we would be able to keep minSdk 21? |
stephanritscher
force-pushed
the
master
branch
from
a4e1b29 to
1048fb5
Compare
|
I think I got it - only one place to change in the end. |
|
Any objections so far? It's working flawlessly on my side and I just noticed a minor conflict which came up in the PR - I hope to resolve it later today. |
stephanritscher
force-pushed
the
master
branch
from
1048fb5 to
e1aff33
Compare
stephanritscher
force-pushed
the
master
branch
from
e1aff33 to
4de3135
Compare
|
@patzly and @dominiczedler Can you review this PR? I would like to see the feature implemented. |
|
@stephanritscher Hi, I implemented the files directly into the project (was important for translation management and design choices) and cleaned them up a bit. You can find the current implementation in the features/client_certificates branch. However, for me (on the simple demo instance) there always appears the toast at startup that the keystore cannot be found, from here: or the finally body. Without any client keystore config at all (and without any knowledge) there shouldn't be any errors or warnings I think. How can this prevented? Or did I messed up something? Could you please check if the current master branch works for you regarding client certificates?
|
|
Hi, thanks for your efforts! |
|
My first try was to upgrade the instance I was running with the code from my PR. Connectivity is broken, with the stacktrace: Error code 400 indicates, that the request didn't contain the client certificate. |
|
I just found your commit disabling the client certificate code - next try with that commit reverted ;-) |
|
Now it crashes: 10-05 22:07:51.636 31080 31080 E xyz.zedler.patrick.grocy.ssl.mtm.MemorizingTrustManager: loadAppKeyStore: exception loading file key store: /data/user/0/xyz.zedler.patrick.g The key store is also not found running the code from my branch since my server uses a certificate recognized by my phone. However, on my branch it doesn't crash. And I don't see any clue what causes the crash. |
Ah sorry I moved the test to the feature branch I mentioned in my message, on the master branch I disabled it for the release :) |
|
Now I see that maybe Dominic added the toasts and that they weren't there in your code, thanks! The crash is really weird... Could you please post the whole crash log again? It seems that the lines where the real crash happens are missing, because the error prints are the ones from the 'Log.e' in catch which should catch the exception and the crash. |
|
There wasn't anything relevant in the log, I fear |
|
I retried with a fresh installation of feature/client_certificate. It crashed on startup. Output of adb logcat ':W': |
|
Hi, after quite some debugging and diffing your code with mine I found the problem is the logging statement you modified in the beginning of |
|
Please review my suggestion to fix the feature at #892. |
This adds support for providing client certificates (which can be picked from system certificate store or from file system) and for accepting self-signed single server certificates without storing them in system certificate store.
This is realized via two libraries which have minimal impact on the application code and thus are easy to maintain. MemorizingTrustManager only has been forked by me due to a build issue with current gradle version (we should switch back to the original project after my pull request has been merged), InteractiveKeyManager has been developed by me inspired by MemorizingTrustManager.