[icloud] dynamic certificates

[hawkeye-bot]

As apple is frequently (increasingly) updating the certificate on the server-side, users are more often confronted with a non-functioning iCloud binding.
One of the issues here is that the certificate that is used to verify the server (SSL handshake) is stored statically in the codebase. This means that every time the certificate is changed, a new certificate needs to be included in the codebase (with all the work that comes with it).

This pull request changes the code so that it dynamically retrieves the certificate from the server when needed. The only loophole is that there is no check on the client-side that it's the correct certificate. On the other hand, from what I could tell from the last couple of manual updates, this wasn't being done anyway. If needed, this can be achieved in the future by using a specialised trustmanager to verify the certificate is issues by a trusted party.

With this PR, the binding now works as following with regards to certificates:

• When the binding is started, it checks to see if the file {OPENHAB_DIR}/userdata/tmp/fmipmobile.crt is present. If it exists, it will use this for connecting. This will always allow a user to manually override the certificate should it ever be necessary
• In case it doesn't find this file, it will automatically retrieve the latest certificate and place this in the aforementioned file
• When connecting to the server, it uses the certificate that is last retrieved. In case the request fails with an SSLHandshakeException, it will try to proactively update the certificate again, and retry the request. It will only try to update the certificate once during the course of the request, to prevent the code from getting in an infinite loop
• In case the connection still fails after updating the certificate, the items will go offline. On the next attempt, it will simply try to retrieve a new certificate again, thereby rendering any manual action unnecessary

Please note that I've also added some extra logging, and cleaned up some minor things.

Any suggestions on how the code needs to be improved would be good. I've asked users to try out the snapshot https://drive.google.com/open?id=1qoNP4PlFAcAwtu9w9ydYwtJSdMymkzV7 in thread https://community.openhab.org/t/icloud-ssl-issue-again/79182/157. So far reports seem to be positive.

[TravisBuddy]

Travis tests have failed

Hey @hoeckxer,
please read the following log in order to understand the failure reason. There might also be some helpful tips along the way.
It will be awesome if you fix what is wrong and commit the changes.

2nd Build

Expand here

Single addon pull request: Building org.openhab.binding.icloud
[WARNING] The POM for com.sun.xml.bind:jaxb-core:jar:2.2.11 is invalid, transitive dependencies (if any) will not be available, enable debug logging for more details
[WARNING] The POM for com.sun.xml.bind:jaxb-impl:jar:2.2.11 is invalid, transitive dependencies (if any) will not be available, enable debug logging for more details
[WARNING] Could not find file feature file features/karaf/openhab-addons/src/main/feature/feature.xml
WARNING: An illegal reflective access operation has occurred
WARNING: Illegal reflective access by com.sun.xml.bind.v2.runtime.reflect.opt.Injector (file:/home/travis/.m2/repository/com/sun/xml/bind/jaxb-impl/2.2.11/jaxb-impl-2.2.11.jar) to method java.lang.ClassLoader.defineClass(java.lang.String,byte[],int,int)
WARNING: Please consider reporting this to the maintainers of com.sun.xml.bind.v2.runtime.reflect.opt.Injector
WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations
WARNING: All illegal access operations will be denied in a future release
[WARNING] /home/travis/build/openhab/openhab-addons/bundles/org.openhab.binding.icloud/src/main/java/org/openhab/binding/icloud/internal/ICloudHandlerFactory.java:[34,1344] The import org.eclipse.smarthome.io.net.http.internal.ExtensibleTrustManagerImpl is never used
[WARNING] /home/travis/build/openhab/openhab-addons/bundles/org.openhab.binding.icloud/src/main/java/org/openhab/binding/icloud/internal/ICloudConnection.java:[24,680] The import org.eclipse.jdt.annotation.Nullable is never used
[WARNING] /home/travis/build/openhab/openhab-addons/bundles/org.openhab.binding.icloud/src/main/java/org/openhab/binding/icloud/internal/handler/ICloudAccountBridgeHandler.java:[111,4460] Potential null pointer access: this expression has a '@Nullable' type
[WARNING] /home/travis/build/openhab/openhab-addons/bundles/org.openhab.binding.icloud/src/main/java/org/openhab/binding/icloud/internal/handler/ICloudAccountBridgeHandler.java:[137,5540] Potential null pointer access: this expression has a '@Nullable' type
[WARNING] /home/travis/build/openhab/openhab-addons/bundles/org.openhab.binding.icloud/src/main/java/org/openhab/binding/icloud/internal/handler/ICloudAccountBridgeHandler.java:[147,5837] Potential null pointer access: this expression has a '@Nullable' type
[WARNING] /home/travis/build/openhab/openhab-addons/bundles/org.openhab.binding.icloud/src/main/java/org/openhab/binding/icloud/internal/handler/ICloudAccountBridgeHandler.java:[165,6577] Null type mismatch: required '@NonNull String' but the provided value is inferred as @Nullable
[WARNING] /home/travis/build/openhab/openhab-addons/bundles/org.openhab.binding.icloud/src/main/java/org/openhab/binding/icloud/internal/handler/ICloudAccountBridgeHandler.java:[165,6586] Null type mismatch: required '@NonNull String' but the provided value is inferred as @Nullable
[WARNING] /home/travis/build/openhab/openhab-addons/bundles/org.openhab.binding.icloud/src/main/java/org/openhab/binding/icloud/internal/handler/ICloudAccountBridgeHandler.java:[185,7447] Potential null pointer access: this expression has a '@Nullable' type
[WARNING] /home/travis/build/openhab/openhab-addons/bundles/org.openhab.binding.icloud/src/main/java/org/openhab/binding/icloud/internal/handler/ICloudDeviceHandler.java:[209,8449] Potential null pointer access: this expression has a '@Nullable' type
[WARNING] /home/travis/build/openhab/openhab-addons/bundles/org.openhab.binding.icloud/pom.xml [0:0]: Unused Export-Package instructions: [org.openhab.*] 
[WARNING] /home/travis/build/openhab/openhab-addons/bundles/org.openhab.binding.icloud/pom.xml [0:0]: Unused Import-Package instructions: [io.swagger.annotations.*, javax.annotation.security.*, org.eclipse.jdt.annotation.*, org.openhab.core.automation.annotation.*, org.openhab.*, com.google.common.*] 
[WARNING] Could not find file feature file features/karaf/openhab-addons/src/main/feature/feature.xml
     [java] Warnings generated: 2
[ERROR] Code Analysis Tool has found: 
 2 error(s)! 
 11 warning(s) 
 25 info(s)
[WARNING] org.openhab.binding.icloud.internal.ICloudBindingConstants.java:[29]
Javadoc author should not have empty contribution description.
[WARNING] org.openhab.binding.icloud.internal.ICloudHandlerFactory.java:[51]
Classes/Interfaces should be annotated with @NonNullByDefault
[WARNING] org.openhab.binding.icloud.internal.utilities.ICloudTextTranslator.java:[27]
Classes/Interfaces should be annotated with @NonNullByDefault
[WARNING] org.openhab.binding.icloud.internal.json.response.ICloudServerContext.java:[23]
Classes/Interfaces should be annotated with @NonNullByDefault
[WARNING] org.openhab.binding.icloud.internal.json.response.ICloudServerContextTimezone.java:[21]
Classes/Interfaces should be annotated with @NonNullByDefault
[WARNING] org.openhab.binding.icloud.internal.json.response.ICloudDeviceLocation.java:[22]
Classes/Interfaces should be annotated with @NonNullByDefault
[WARNING] org.openhab.binding.icloud.internal.json.response.ICloudAccountDataResponse.java:[25]
Classes/Interfaces should be annotated with @NonNullByDefault
[WARNING] org.openhab.binding.icloud.internal.json.response.ICloudAccountUserInfo.java:[21]
Classes/Interfaces should be annotated with @NonNullByDefault
[WARNING] org.openhab.binding.icloud.internal.json.response.ICloudDeviceFeatures.java:[21]
Classes/Interfaces should be annotated with @NonNullByDefault
[WARNING] org.openhab.binding.icloud.internal.json.response.ICloudDeviceInformation.java:[24]
Classes/Interfaces should be annotated with @NonNullByDefault
[WARNING] org.openhab.binding.icloud.internal.json.request.ICloudAccountDataRequest.java:[36]
Classes/Interfaces should be annotated with @NonNullByDefault
[ERROR] org.openhab.binding.icloud.internal.ICloudTlsCertificateProvider.java:[118]
Count of placeholder(0) is not equal to count of parameter(1)
[ERROR] org.openhab.binding.icloud.internal.ICloudTlsCertificateProvider.java:[123]
Count of placeholder(0) is not equal to count of parameter(1)
[ERROR] Failed to execute goal org.openhab.tools.sat:sat-plugin:0.9.0:report (sat-all) on project org.openhab.binding.icloud: 
[ERROR] Code Analysis Tool has found 2 error(s)! 
[ERROR] Please fix the errors and rerun the build.
[ERROR] -> [Help 1]
[ERROR] 
[ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
[ERROR] Re-run Maven using the -X switch to enable full debug logging.
[ERROR] 
[ERROR] For more information about the errors and possible solutions, please read the following articles:
[ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoFailureException
[ERROR] 
[ERROR] After correcting the problems, you can resume the build with the command
[ERROR]   mvn <args> -rf :org.openhab.binding.icloud

[paulianttila]

First of all, thanks for PR and I know this issue is huge problem for the users (I'm using it as well).

I think we should consider automatic certification renewal twice as it's a security flaw. Binding will automatically trust all server certificates, so it doesn't prevent "man in the middle" attacks anymore (user iCloud credentials could be sent to 3rd party).

At least feature should be configurable option and user could enable this feature by knowing consequences.

[TravisBuddy]

Travis tests were successful

Hey @hoeckxer,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

[hawkeye-bot]

First of all, thanks for PR and I know this issue is huge problem for the users (I'm using it as well).

I think we should consider automatic certification renewal twice as it's a security flaw. Binding will automatically trust all server certificates, so it doesn't prevent "man in the middle" attacks anymore (user iCloud credentials could be sent to 3rd party).

At least feature should be configurable option and user could enable this feature by knowing consequences.

That is my concern as well (as mentioned), but on the other hand nobody is checking the certificates that were installed over the past year as far as I've heard. I could make the dynamic retrieval configurable; in that case people can still actively update the certificate themselves by updating the used .crt file, or they can decide if they want to allow the dynamic retrieval. They could also simple flip the switch when it stops working, and disable it again after the certificate has been renewed. However, I somehow feel that that's a bit of quite specific configuration for the average user?
Is there any way that we can check the source of the certificate; eg is the certificate signed & verifiable by a static key?

[TravisBuddy]

Travis tests were successful

Hey @hoeckxer,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

[TravisBuddy]

Travis tests were successful

Hey @hoeckxer,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

[martinvw]

Doesn’t it use the same root certificate? I thought the root certificate has a very long validity. And the time I included i of course validated the correctness before creating a PR.

I would be really happy to have resolved but agree that the current implementation imposes a security issue.

Did you look into using the root certificate or can you look into it.

Thanks for your efforts!

[paulianttila]

Certificate in binding bundle seems to be the server certificate itself, not Root CA or Apple sub CA what it should be. iCloud binding should trust all certificates issued by Apple Root CA or at least Apple Server authentication CA, right?

I understand initially that Apple CA certificate has been changed, but is the reason just that server certificate has been changed?

[J-N-K]

Maybe it has been replaced with a wrong certificate? I think we should not use a trust-all for external connections where important credentials are used.

[martinvw]

The first time I worked on it it failed when I tried to use the the root certificate. I did not investigate that much further later on.

[martinvw]

The first time it broke down was because they switched from a normal certificate signed by a known CA to the self signed. After that I was not able to get it working using the Root / apple ca certificate so ended up using the server certificate planning to refresh it once per year however it feels that it is now more than once per year.

And of course a dynamic solution is preferable! So thanks again for bringing this up and sketching out a possible solution

[hawkeye-bot]

Thanks for the feedback. I'm happy to investigate if we can check the root certificate, I agree that we shouldn't allow all certificates. I need some time to dive into it, I'll report back when I've got an answer.

[martinvw]

I just checked and the Apple Root CA is still valid till 2035 so if we could validate against that it would be awesome.

If you want I can also try to take a further look, thanks again for your work to resolve this!

[hawkeye-bot]

@martinvw I can work it out no problem, but thanks :)

[maihacke]

Please see here: https://community.openhab.org/t/icloud-ssl-issue-again/79182/187?u=maihacke
I created a jar with apples CA cert, which seems to work: https://drive.google.com/open?id=1fUTq9kqfAqYhLuW7qJypom2HiSRBsOK-

[TravisBuddy]

Travis tests were successful

Hey @hoeckxer,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

[hawkeye-bot]

I updated the code so that it verifies the presented certificate up the certificate chain up to the root CA certificate, which is included in the bundle.

I'm intrigued by @maihacke 's solution though; can you expand on if you included the server CA certificate, or the root certificate?

@martinvw do you know what protocol was used when you were trying to implement the root certificate check? I've been digging a bit, and I think TLS1.2 mandates the certification path to be sent (the intermediary certificates) so that it should be able to check with just the root certificate on the client side.

Food for thought on how to move forward; we can see if the existing 2.5 code base works with just replacing the certificate with the root certificate, or we can go with the dynamic route, which now also automatically verifies the certification path. If it works with just updating the certificate to the root certificate, I think we should do that as the code is simpler and drop this pull request?

[maihacke]

@hoeckxer The certificate chain for the fmipmobile service contains at least three certificates. The first is issued for fmipmobileX.apple.com. This certificate was included in the bundle before my change.
The certificate is issued by „ Apple Server Authentication CA“. This is the CA which certificate is now in the bundle.
The certificate is signed by Apple Root CA. This cert is not part of the bundle.

[maihacke]

I would prefer including the required certificates in the bundle and not go for a solution which downloads certificates by itself.
By the way Apples Root CA certificate is available here https://www.apple.com/certificateauthority/
Also interessting may be this list: https://support.apple.com/de-de/HT209144
This shows which CA certificates are delivered with iOS. So if we come to the conclusion that just including the "Apple Server Authentication CA" is not "future-proof" against upcoming changes in the services certificate chain, another approach could be to mimic iOS and include exactly these CA certs.

[hawkeye-bot]

@maihacke using the CA certificate Apple Server Authentication CA is actually just as unsafe, as this certificate is included in the response sent by the server. This means that this certificate can be mimicked by hackers just as easily. The correct approach I think now is to provide the only certificate in the chain that is not sent by the server (these can be hacked after all), which is Apple's Root CA certificate. This is the only certificate that cannot be hacked remotely, and thereby provides a correct verification. I've made this change on a new branch, which seems to work just fine. I've created pull request #6948 for this new change. As far as I'm concerned this is the right way to go, and we should abandon this pull request (unless we run into serious issues with the other change, but I don't think so).

[maihacke]

No that is not correct. An attacker could not create a certificate matching the hosts DNS name. The JVMs trustmanager checks that the certificate is issued for the correct DNS and is signed by a trusted CA. For that the attacker would need the private key of Apples CA cert.

[J-N-K]

See #6948

[hawkeye-bot]

@maihacke using the server certificate in the binding would indeed work to check against, I misinterpreted my expectations. This would provide a longer valid certificate and a working binding. While we're at it I think it would be better though to use the root certificate to check against, as this is valid for an even longer period.