patzly / grocy-android

ERP beyond your fridge, now on your phone – An awesome companion app for grocy
https://patrickzedler.com/grocy/
GNU General Public License v3.0
879 stars 85 forks source link

Add support for client certificates #771

Open stephanritscher opened 1 year ago

stephanritscher commented 1 year ago

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.

dominiczedler commented 1 year ago

Sorry for the delay, I will look into it as soon as I have time after my bachelor thesis. Thanks for your pull request.

patzly commented 1 year ago

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.

stephanritscher commented 1 year ago

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.

patzly commented 1 year ago

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 commented 1 year ago

I think I got it - only one place to change in the end.

stephanritscher commented 11 months ago

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.

koostamas commented 3 months ago

@patzly and @dominiczedler Can you review this PR? I would like to see the feature implemented.

patzly commented 2 weeks ago

@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: https://github.com/patzly/grocy-android/blob/a4c5fe1213a941bd3599774642cbf34b05b04cfb/app/src/main/java/xyz/zedler/patrick/grocy/ssl/mtm/MemorizingTrustManager.java#L207 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?

stephanritscher commented 1 day ago

Hi, thanks for your efforts! It seems you added the toasts in places which previously were logged silently. By current design, the keystore file is only written in keyStoreUpdated referenced by storeCert and deleteCertificate. So only if the feature is used, the file will be created. Anyway, a missing keystore file no error but will occur at least after first app start. I suggest to check for existence of the keyStoreFile and only load its content (lines 204-205) if it exists, if you want to keep the toast. I will update you later about my test with the master branch.

stephanritscher commented 1 day ago

My first try was to upgrade the instance I was running with the code from my PR. Connectivity is broken, with the stacktrace: 10-05 21:45:35.174 22117 22263 E Volley : [150] NetworkUtility.shouldRetryException: Unexpected response code 400 for https://xxx.xxx/grocy/api/system/db-changed-time 10-05 21:45:35.176 22117 22117 E ShoppingListViewModel: onError: VolleyError: com.android.volley.ClientError 10-05 21:45:35.177 22117 22117 W System.err: com.android.volley.ClientError 10-05 21:45:35.177 22117 22117 W System.err: at com.android.volley.toolbox.NetworkUtility.shouldRetryException(NetworkUtility.java:193) 10-05 21:45:35.177 22117 22117 W System.err: at com.android.volley.toolbox.BasicNetwork.performRequest(BasicNetwork.java:145) 10-05 21:45:35.177 22117 22117 W System.err: at com.android.volley.NetworkDispatcher.processRequest(NetworkDispatcher.java:132) 10-05 21:45:35.178 22117 22117 W System.err: at com.android.volley.NetworkDispatcher.processRequest(NetworkDispatcher.java:111) 10-05 21:45:35.178 22117 22117 W System.err: at com.android.volley.NetworkDispatcher.run(NetworkDispatcher.java:90) 10-05 21:45:35.178 22117 22117 W System.err: com.android.volley.ClientError 10-05 21:45:35.178 22117 22117 W System.err: at com.android.volley.toolbox.NetworkUtility.shouldRetryException(NetworkUtility.java:193) 10-05 21:45:35.178 22117 22117 W System.err: at com.android.volley.toolbox.BasicNetwork.performRequest(BasicNetwork.java:145) 10-05 21:45:35.178 22117 22117 W System.err: at com.android.volley.NetworkDispatcher.processRequest(NetworkDispatcher.java:132) 10-05 21:45:35.179 22117 22117 W System.err: at com.android.volley.NetworkDispatcher.processRequest(NetworkDispatcher.java:111) 10-05 21:45:35.179 22117 22117 W System.err: at com.android.volley.NetworkDispatcher.run(NetworkDispatcher.java:90)

Error code 400 indicates, that the request didn't contain the client certificate. Was there something you changed when you integrated the files that could break my current setup?

stephanritscher commented 1 day ago

I just found your commit disabling the client certificate code - next try with that commit reverted ;-)

stephanritscher commented 1 day ago

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 rocy.debug/app_KeyStore/KeyStore.bks 10-05 22:07:51.636 31080 31080 E xyz.zedler.patrick.grocy.ssl.mtm.MemorizingTrustManager: java.io.FileNotFoundException: /data/user/0/xyz.zedler.patrick.grocy.debug/app_KeySt ore/KeyStore.bks: open failed: ENOENT (No such file or directory) ... 10-05 22:07:51.638 1805 8332 W PackageConfigPersister: App-specific configuration not found for packageName: xyz.zedler.patrick.grocy.debug and userId: 0 ... --------- beginning of crash 10-05 22:07:51.746 31080 31137 F libc : Fatal signal 6 (SIGABRT), code -1 (SI_QUEUE) in tid 31137 (Thread-10), pid 31080 (ick.grocy.debug)

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.

patzly commented 1 day ago

I just found your commit disabling the client certificate code - next try with that commit reverted ;-)

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 :)

patzly commented 1 day ago

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.

stephanritscher commented 2 hours ago

There wasn't anything relevant in the log, I fear