Add Wear OS TLS client certificate authentication (TLS CCA) support #3924
Conversation
kleest
force-pushed
the
feat-wearos-tls
branch
2 times, most recently
from
26f2532 to
90c152e
Compare
|
Fixed a bug where further onboarding fails if the user previously logged out from the WearOS app. |
...n/java/io/homeassistant/companion/android/onboarding/integration/MobileAppIntegrationView.kt
Outdated
Show resolved
Hide resolved
wear/src/main/java/io/homeassistant/companion/android/phone/PhoneSettingsListener.kt
Outdated
Show resolved
Hide resolved
Good point, I did not think about it. I changed the logic. I added changes in a new commit instead of amending for easier reviewing. Before merging this PR into the master branch, I would like to squash the commits to have a clean history. |
|
Thanks! I'll have a look at the changes and test it soon.
All pull requests are squashed when merged, you don't need to do this yourself (in fact, that is preferred, as otherwise it makes it harder to review). |
|
But the squashing defaults do not retain the commit descriptions but only titles. The descriptions adds quite a bit of context to this PR. That's why I explicitly mentioned it 🙂 |
|
We use the squash & merge option on pull requests to merge which keeps titles + descriptions for each commit, see b0ad11d for example. Your pull request with all discussion and the branch at the time of merging will also remain available on GitHub. |
|
|
||
| @HiltAndroidApp | ||
| open class HomeAssistantApplication : Application() { | ||
| private val ioScope: CoroutineScope = CoroutineScope(Dispatchers.IO + Job()) |
There was a problem hiding this comment.
When creating a long-lived CoroutineScope (in this case, tied to the lifetime of HomeAssistantApplication), it's better to use SupervisorJob, as the scope won't get canceled forever if one coroutine fails. Now ioScope is only used at a single place, but in the future, others can also start using it, so it would be better to prepare.
| private val ioScope: CoroutineScope = CoroutineScope(Dispatchers.IO + Job()) | |
| private val ioScope: CoroutineScope = CoroutineScope(Dispatchers.IO + SupervisorJob()) |
There was a problem hiding this comment.
Not sure if I understood this correctly, CoroutineScope(Dispatchers.IO + Job()) is already used 7x in "common" and "app" modules in the source code. The new addition is this line in the "wear" module. Should all other occurrences be changed to SupervisorJob() as well?
There was a problem hiding this comment.
@marazmarci While you're not wrong let's not change a common pattern in this PR, that's more (unrelated) changes. Feel free to make that change in another PR. Using a 'normal' job works if you're aware of it and handle exceptions.
There was a problem hiding this comment.
While trying to test this, I encountered the following exception (on a Galaxy Watch 4 with Wear OS 4):
2023-10-22 15:09:55.263 15373-15373 PhoneSettingsListener io....stant.companion.android.debug E Cannot load TLS client certificate
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:1505)
at io.homeassistant.companion.android.phone.PhoneSettingsListener$login$1.invokeSuspend(PhoneSettingsListener.kt:160)
at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33)
at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:108)
at android.os.Handler.handleCallback(Handler.java:942)
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:7962)
at java.lang.reflect.Method.invoke(Native Method)
at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:550)
at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:952)
Could it be that not all watches support this? If so we should detect + send that info to the phone in advance to make the user understand why it doesn't work.
Also a few comments to clean up the code and improve the user experience.
common/src/main/java/io/homeassistant/companion/android/common/data/keychain/KeyStoreImpl.kt
Outdated
Show resolved
Hide resolved
common/src/main/java/io/homeassistant/companion/android/common/data/keychain/KeyStoreImpl.kt
Outdated
Show resolved
Hide resolved
common/src/main/java/io/homeassistant/companion/android/common/data/keychain/KeyStoreImpl.kt
Outdated
Show resolved
Hide resolved
common/src/main/java/io/homeassistant/companion/android/common/data/keychain/KeyStoreImpl.kt
Outdated
Show resolved
Hide resolved
...n/java/io/homeassistant/companion/android/onboarding/integration/MobileAppIntegrationView.kt
Outdated
Show resolved
Hide resolved
|
@jpelgrom Thanks for your comments!
Quick answer regarding the KeyStore exception: This is strange. The PKCS12 store type should be supported on Android since API Level 1 (see https://developer.android.com/reference/java/security/KeyStore) and it works perfectly fine on the emulator. I am also quite confused because there seems to exist a PKCS12 implementation, note the class I will have a look at your other comments in the next days. |
|
@jpelgrom The PKCS12 problem is due to different PKCS12 encryption/signature algorithms (see SO thread for details). In my tests, I used a legacy PKCS12 container (pbeWithSHA1And3-KeyTripleDES-CBC) since the modern format (AES-256-CBC) already failed during password check on the phone (API level 33). It seems like your phone works with the new container, but then Wear OS (API level 33) fails opening the container. I did not consider the case that it works on the phone but does not work on Wear OS. Are you using API level 34 on the phone and a modern keystore format (check with I am also unsure how to handle this problem. I see two options:
I prefer option 2. Apart from these options, I am not sure if we can detect if opening the container fails due to an unsupported format or the wrong key, but I'm afraid we can't. The user certificate import system dialog on the phone already has the issue that it fails with a wrong password if you try to import a modern format PKCS12 container on API level <= 33, even though the user provided the correct password. |
Yes and I think so? Not very familiar with all the details. Command output mentions what you describe as modern format:
I agree. However, could we maybe extend this to the phone as that is most likely where the user will see that the login failed? It already sends over the stacktrace to the phone (as text) as that is what causes the 'Failed to login' snackbar to show. This might be worth mentioning in the documentation, too much information and too technical to put in the app strings. |
|
Sorry for the late reply, but I totally missed the notification on the other thread #2771. I downloaded the artifact from here and installed the Android APK on the phone and the Wear APK on my Galaxy Watch 4 with Wear OS 4. I made two tests, one with a wrong certificate (actuallly revoked) and another with the correct one; both are PFX files without any password. Then I captured the logs on the Wear OS device from the PC with: First test: revoked certificate ✔I first tried using the expired certificate to check if an error message was printed.
Second test: correct certificate ✔Then the correct certificate, which is the one already working on the phone with the HA app.
Test tile ✔Once connected, even when out of the Wi-Fi range, the watch can finally read data from the Home Assistant instance!
I hope this PR will be merged quickly to the HA official branch! |
kleest
force-pushed
the
feat-wearos-tls
branch
from
12324a0 to
45624b8
Compare
|
@jpelgrom Sorry for the late response!
I am not sure how to go forward with this. Wear OS sends back one status indicator ( |
No problem.
Thinking about it more and considering what you wrote, maybe this is too much to explain in a single line snackbar and to add to the PR (scope creep), and docs is the better place for it. If this becomes more common, more detailed error handling can always be added later. |
Personally, I would prefer having the snackbar message on the phone at least to suggest whether it was a network error or a TLS error. In fact, when I tried with the revoked certificate above and saw the Could not register watch error I initially though that the watch lost the connection. But it was "working" instead, only with the wrong certificate. |
|
I am very excited for this feature! I tested the latest artifacts on my Galaxy S23 and Galaxy Watch 4, and everything worked flawlessly. (Similar to the test above, except with a password on the certificate file.) 🚀 Other than just testing, is there anything I can do to help push this over the finish line? |
|
@vexofp Thanks for testing! I will need to change the exception handling and want to add a simple certificate expiry check on the phone before sending the certificate to the watch. I plan to finish this on the weekend, I am currently a little busy. |
Wear OS does not currently allow the user to install certificates to the system-wide KeyChain for TLS CCA support. This commit adds support for using certificates from the app-specific Android KeyStore with UI for setting up a certificate during the Wear OS onboarding process. The manual step in the onboarding process is required since we cannot transmit certificates of the Android KeyChain because they are not extractable. In particular, this commit adds the following changes: * KeyStoreImpl as an additional KeyChainRepository interface implementation for loading and storing keys to the application's KeyStore. TLSHelper uses KeyStoreImpl as a fallback key manager. * UI for selecting a certificate file with GET_CONTENT intent during Wear OS onboarding in OnboardingActivity if it is detected that the Home Assistant may require TLS CCA. The UI includes a password check for the PKCS12 container. * During onboarding the app sends the raw PKCS12 data to Wear OS together with the container password. The connection is assumed to be encrypted and trusted so that no additional encryption is necessary.
kleest
force-pushed
the
feat-wearos-tls
branch
from
24c612b to
6477241
Compare
|
Rebased on current master branch and removed the redundant try-catch block. Unfortunately I will not have time to add the expiry check within the next days. If @jpelgrom agrees, I will add the certificate expiry check in another PR later so that this PR is now complete. |
That's fine, no need to keep expanding the scope of the PR :) |
...n/java/io/homeassistant/companion/android/onboarding/integration/MobileAppIntegrationView.kt
Outdated
Show resolved
Hide resolved
|
Has anyone successfully used this on a Xiaomi Watch ? With an ECC certificate I'm getting: |
|
@dynek As another one replied to you on your linked thread, it seems that you're using an unsupported certificate on the watch (which connects on its own, so even if the same certificate works on the smartphone, it must work on the watch). This type works on a Galaxy Watch 4: # openssl x509 -in mycert.crt -text -noout | grep -i algo
Signature Algorithm: sha256WithRSAEncryption
Public Key Algorithm: rsaEncryption
Signature Algorithm: sha256WithRSAEncryption
# openssl pkcs12 -in mycert.pfx -info -noout
Enter Import Password:
MAC: sha1, Iteration 2048
MAC length: 20, salt length: 8
PKCS7 Encrypted data: pbeWithSHA1And40BitRC2-CBC, Iteration 2048
Certificate bag
Certificate bag
PKCS7 Data
Shrouded Keybag: pbeWithSHA1And3-KeyTripleDES-CBC, Iteration 2048Of course you'll have to use the |
|
OK so that certificate has been generated with the |
|
@dynek Probably Wear OS had to strip something and chose the certificate algorithms, who knows? It's not very well documented. |
@jeankhawand It's already merged in the version avaliable on the Play Store, too. I'm using it successfully! |
@virtualdj |
|
@jeankhawand It's not on the watch, but on the smartphone app (complementary app section, see the screenshots above). |
@virtualdj |
|
@jeankhawand Did you set the external URL in the smartphone app? It should point to a DNS of your proxy that provides the TLS Certificate Authentication. Sorry I cannot be more precise because I configured it months ago (when it was released actually) and it worked so perfectly I didn't need to ever touch it 😁 EDIT: And you'll need the certificate installed on your smartphone, too, I think. |
|
Going to lock this as a merged PR is not the place for troubleshooting. You'll need to have configured the server with mTLS on the phone, then it'll prompt you to select a certificate (again) when setting up the watch app. If you cannot get it to work, please create an issue. |
Summary
Wear OS does not currently allow the user to install certificates to the system-wide KeyChain for TLS CCA support. This commit adds support for using certificates from the app-specific Android KeyStore with UI for setting up a certificate during the Wear OS onboarding process. The manual step in the onboarding process is required since we cannot transmit certificates of the Android KeyChain because they are not extractable.
In particular, this commit adds the following changes:
Screenshots
Onboarding with certificate selection (light mode):
Onboarding with certificate selection (dark mode):
Onboarding without certificate selection (no changes):
Link to pull request in Documentation repository
Documentation: home-assistant/companion.home-assistant#995
Any other notes
This is a fix #2771. The linked issues also contains the idea to generate a certificate signing request on the Wear OS device, however, this PR does not add support for this feature.