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

[hue] Check HTTPS connection (download of PEM certificate) #13617

Merged
merged 3 commits into from
Oct 30, 2022

Conversation

lolodomo
Copy link
Contributor

Fix #13586

Signed-off-by: Laurent Garnier lg.hc@free.fr

@lolodomo lolodomo added the bug An unexpected problem or unintended behavior of an add-on label Oct 29, 2022
@lolodomo lolodomo requested a review from cweitkamp as a code owner October 29, 2022 06:09
@cweitkamp
Copy link
Contributor

First of all thank you for taking care.

I see two issues with your approach. First one is that the certificate is requested/ loaded twice - which is the minor one. Second the constructor of the HueBridge already starts an authentication request which obviously will fail is HTTPS is activated because of the late tls provider registration.

@lolodomo
Copy link
Contributor Author

First one is that the certificate is requested/ loaded twice - which is the minor one.

Yes, that is true but that is only at the startup of OH (binding).

Second the constructor of the HueBridge already starts an authentication request which obviously will fail is HTTPS is activated because of the late tls provider registration.

I don't see any request started. Here is the code of the called HueBridge contructor:
https://github.com/openhab/openhab-addons/blob/main/bundles/org.openhab.binding.hue/src/main/java/org/openhab/binding/hue/internal/connection/HueBridge.java#L128
Only class members are initialized.

@lolodomo
Copy link
Contributor Author

lolodomo commented Oct 29, 2022

Maybe you thought the second constructor was called, the one which also calls authenticate ? But that is not the case.

@lolodomo
Copy link
Contributor Author

First one is that the certificate is requested/ loaded twice - which is the minor one.

Yes, that is true but that is only at the startup of OH (binding).

That being said, I can avoid it.

@lolodomo
Copy link
Contributor Author

First one is that the certificate is requested/ loaded twice - which is the minor one.

Yes, that is true but that is only at the startup of OH (binding).

That being said, I can avoid it.

It is now fixed.

@lolodomo
Copy link
Contributor Author

I cached the PEM trust manager to not download the certificate twice.
In case it should not be cached on a long period, I could replace the cache by an expiry cache. Just let me know.

Fix openhab#13586

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
Signed-off-by: Laurent Garnier <lg.hc@free.fr>
@lolodomo
Copy link
Contributor Author

@cweitkamp @jlaur : do not hesitate to test it in case you think it cannot work with a V2 bridge. I am confident it will work perfectly ;)

@jlaur
Copy link
Contributor

jlaur commented Oct 29, 2022

do not hesitate to test it in case you think it cannot work with a V2 bridge. I am confident it will work perfectly ;)

Already running it. :-)

  • HTTPS communication still works (with default parameters).
  • Setting protocol to http works.
  • Setting ipAddress to some other host responding on port 443 but not exposing Hue API no longer causes infinite errors.
  • Setting port to 80 and protocol to https causes same behavior as you probably see with Hue bridge V1. However, besides setting the bridge offline description, an exception is also logged with log level ERROR. Not sure if desired now? See below.

Exception:

2022-10-29 16:49:01.847 [ERROR] [onnection.HueTlsTrustManagerProvider] - An unexpected exception occurred: javax.net.ssl.SSLException: Unsupported or unrecognized SSL message
org.openhab.core.io.net.http.PEMTrustManager$CertificateInstantiationException: javax.net.ssl.SSLException: Unsupported or unrecognized SSL message
	at org.openhab.core.io.net.http.PEMTrustManager.getPEMCertificateFromServer(PEMTrustManager.java:220) ~[?:?]
	at org.openhab.core.io.net.http.PEMTrustManager.getInstanceFromServer(PEMTrustManager.java:131) ~[?:?]
	at org.openhab.core.io.net.http.PEMTrustManager.getInstanceFromServer(PEMTrustManager.java:116) ~[?:?]
	at org.openhab.binding.hue.internal.connection.HueTlsTrustManagerProvider.getPEMTrustManager(HueTlsTrustManagerProvider.java:77) ~[?:?]
	at org.openhab.binding.hue.internal.handler.HueBridgeHandler.lambda$10(HueBridgeHandler.java:721) ~[?:?]
	at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:515) [?:?]
	at java.util.concurrent.FutureTask.run(FutureTask.java:264) [?:?]
	at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:304) [?:?]
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128) [?:?]
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628) [?:?]
	at java.lang.Thread.run(Thread.java:829) [?:?]
Caused by: javax.net.ssl.SSLException: Unsupported or unrecognized SSL message
	at sun.security.ssl.SSLSocketInputRecord.handleUnknownRecord(SSLSocketInputRecord.java:451) ~[?:?]
	at sun.security.ssl.SSLSocketInputRecord.decode(SSLSocketInputRecord.java:175) ~[?:?]
	at sun.security.ssl.SSLTransport.decode(SSLTransport.java:111) ~[?:?]
	at sun.security.ssl.SSLSocketImpl.decode(SSLSocketImpl.java:1506) ~[?:?]
	at sun.security.ssl.SSLSocketImpl.readHandshakeRecord(SSLSocketImpl.java:1416) ~[?:?]
	at sun.security.ssl.SSLSocketImpl.startHandshake(SSLSocketImpl.java:456) ~[?:?]
	at sun.security.ssl.SSLSocketImpl.startHandshake(SSLSocketImpl.java:427) ~[?:?]
	at sun.net.www.protocol.https.HttpsClient.afterConnect(HttpsClient.java:572) ~[?:?]
	at sun.net.www.protocol.https.AbstractDelegateHttpsURLConnection.connect(AbstractDelegateHttpsURLConnection.java:201) ~[?:?]
	at sun.net.www.protocol.https.HttpsURLConnectionImpl.connect(HttpsURLConnectionImpl.java:168) ~[?:?]
	at org.openhab.core.io.net.http.PEMTrustManager.getPEMCertificateFromServer(PEMTrustManager.java:209) ~[?:?]
	... 10 more

Perhaps it could be reduced to DEBUG?

@lolodomo
Copy link
Contributor Author

Perhaps it could be reduced to DEBUG?

OK

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
@lolodomo
Copy link
Contributor Author

  • Setting ipAddress to some other host responding on port 443 but not exposing Hue API no longer causes infinite errors.

So I even fixed a bug I was not aware :)

@lolodomo
Copy link
Contributor Author

lolodomo commented Oct 29, 2022

  • Setting port to 80 and protocol to https causes same behavior as you probably see with Hue bridge V1. However, besides setting the bridge offline description, an exception is also logged with log level ERROR.

Fixed => DEBUG level now used.

@cweitkamp
Copy link
Contributor

I cached the PEM trust manager to not download the certificate twice.
In case it should not be cached on a long period, I could replace the cache by an expiry cache. Just let me know.

There is a small chance that it will not work. E.g. if the Hue Bridge doesn't have a fixed IP address. But for now I'm happy with it.

I already know that there's a but inside the PEMTrustManager logic. The whole setup will fail once the certificate expires. This must be considered in the future.

@lolodomo
Copy link
Contributor Author

So can we merge the PR ?

Copy link
Contributor

@cweitkamp cweitkamp left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks.

@cweitkamp cweitkamp modified the milestones: 4.0, 3.4 Oct 30, 2022
@cweitkamp cweitkamp removed the bug An unexpected problem or unintended behavior of an add-on label Oct 30, 2022
@cweitkamp cweitkamp merged commit b959122 into openhab:main Oct 30, 2022
@lolodomo lolodomo deleted the hue_https branch October 30, 2022 12:14
andan67 pushed a commit to andan67/openhab-addons that referenced this pull request Nov 6, 2022
…3617)

* [hue] Check HTTPS connection (download of PEM certificate)

Fix openhab#13586

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
ip + ":" + hueBridgeConfig.getPort(), hueBridgeConfig.useSelfSignedCertificate);

// Check before registering that the PEM certificate can be downloaded
if (tlsTrustManagerProvider.getPEMTrustManager() == null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a heads up, will analyze and create issue later: After upgrading to RC1 I kept an eye on my things while booting, and I ended up in this situation. So I will need to analyze exactly how it happened - but it did, and as a result, no further attempts were made to get online and I had to manually restart the binding - after which everything worked fine.

borazslo pushed a commit to borazslo/openhab-mideaac-addon that referenced this pull request Jan 8, 2023
…3617)

* [hue] Check HTTPS connection (download of PEM certificate)

Fix openhab#13586

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
psmedley pushed a commit to psmedley/openhab-addons that referenced this pull request Feb 23, 2023
…3617)

* [hue] Check HTTPS connection (download of PEM certificate)

Fix openhab#13586

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
nemerdaud pushed a commit to nemerdaud/openhab-addons that referenced this pull request Feb 28, 2023
…3617)

* [hue] Check HTTPS connection (download of PEM certificate)

Fix openhab#13586

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[hue] Binding broken until you update the new protocol setting to "http" (bridge V1)
3 participants