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

[gree] Use GCM encryption when bind fails #17398

Merged
merged 8 commits into from
Oct 7, 2024

Conversation

lovery
Copy link
Contributor

@lovery lovery commented Sep 10, 2024

Change the encryption mode to GCM if different when binding to AC device fails with SocketTimeoutException and try again. Detect if device version is bigger than or equal to 2 and set encryption to GCM

Fixes #16911

Additional info:

There are gree devices that on scan request responds with package encrypted with ECB encryption using the old key, but expects GCM encryption bind request in response. Such behavior is described in #16911
Also in openhab community forum a few people are saying that they have the same problem.

Not sure which approach is better to change the encryption mode to GCM when SocketTimeoutException was catched and try again or try to determine is the AC expects GCM encryption bind request base on the value of ver property of the scan response, so I am leaving them both and I am open to suggestion.

Testing

Builded jar file can be found here.

Change the encryption   mode to GCM if different when binding to AC
device fails with SocketTimeoutException and try again.
Detect if device version is bigger than or equal to 2 and set encryption
to GCM

Fixes openhab#16911

Signed-off-by: Zhivka Dimova <[email protected]>
Copy link
Contributor

@lsiepel lsiepel left a comment

Choose a reason for hiding this comment

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

Some minor typo's.
Would be nice if @markus7017 can comment on the functionality.

@lsiepel lsiepel added the bug An unexpected problem or unintended behavior of an add-on label Sep 10, 2024
@lsiepel lsiepel changed the title [gree]: use GCM encrption when binding fails - Bugfix [gree] Use GCM encryption when bind fails Sep 10, 2024
Signed-off-by: Zhivka Dimova <[email protected]>
@lovery
Copy link
Contributor Author

lovery commented Sep 11, 2024

@lsiepel Thank you for noticing my typo's I didn't pay so much attention to the debug messages, because I was thinking that may be there are a bit too much, but I wanted to keep them for testing purposes until some of the people reporting the problem test the change

@lsiepel
Copy link
Contributor

lsiepel commented Sep 12, 2024

Would be nice if @markus7017 can comment

Remove the use of version property of scan response for specifying the
encryption method

Signed-off-by: Zhivka Dimova <[email protected]>
@lsiepel lsiepel added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Sep 20, 2024
@gliter
Copy link

gliter commented Sep 23, 2024

For old Gree device there is an exception thrown, which is strange since it fails before your code change.
It is working correctly on addon from version 4.2.0

RuntimeException
java.lang.NumberFormatException: For input string: "V1."
        at java.lang.NumberFormatException.forInputString(NumberFormatException.java:67) ~[?:?]
        at java.lang.Integer.parseInt(Integer.java:668) ~[?:?]
        at java.lang.Integer.parseInt(Integer.java:786) ~[?:?]
        at org.openhab.binding.gree.internal.GreeCryptoUtil.getEncryptionType(GreeCryptoUtil.java:97) ~[?:?]
        at org.openhab.binding.gree.internal.handler.GreeAirDevice.<init>(GreeAirDevice.java:81) ~[?:?]
        at org.openhab.binding.gree.internal.discovery.GreeDeviceFinder.scan(GreeDeviceFinder.java:117) ~[?:?]
        at org.openhab.binding.gree.internal.handler.GreeHandler.initializeThing(GreeHandler.java:111) ~[?:?]
        at org.openhab.binding.gree.internal.handler.GreeHandler.lambda$1(GreeHandler.java:364) ~[?:?]
        at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:539) [?:?]
        at java.util.concurrent.FutureTask.runAndReset(FutureTask.java:305) [?:?]
        at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:305) [?:?]
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136) [?:?]
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635) [?:?]
        at java.lang.Thread.run(Thread.java:840) [?:?]

@gliter
Copy link

gliter commented Sep 23, 2024

I see you are also an author that implemented GreeCryptoUtil. Seems there have to be something strange with DTO mapping.

The response for scan package I had received from Gree device was:

{"t":"dev","cid":"<Redacted MAC>","bc":"000000000000000000000000000000","brand":"gree","catalog":"gree","mac":"<Redacted MAC>","mid":"10001","model":"gree","name":"<partial MAC>","series":"gree","vender":"1","ver":"V1.2.1","lock":0}

The only V1 occurence here is in ver field which judging from exception it looks like it is trying to put it to some integer field. No idea why.

@gliter
Copy link

gliter commented Sep 23, 2024

I see same problem is for new gree device:

2024-09-23 19:44:34.326 [DEBUG] [.internal.discovery.GreeDeviceFinder] - Response received from address 10.0.20.111: {"t":"dev","cid":"","bc":"00000000000000000000000000000000","brand":"gree","catalog":"gree","mac":"<redacted>","mid":"10001","model":"gree","name":"","lock":0,"series":"gree","vender":"1","ver":"V3.2.M","hid":"362001068254+U-4MWB65VRV2.08.bin","ModelType":"16752730"}
2024-09-23 19:44:34.356 [WARN ] [ernal.discovery.GreeDiscoveryService] - Discovery: Device Discovery failed: RuntimeException
java.lang.NumberFormatException: For input string: "V3."
        at java.lang.NumberFormatException.forInputString(NumberFormatException.java:67) ~[?:?]
        at java.lang.Integer.parseInt(Integer.java:668) ~[?:?]
        at java.lang.Integer.parseInt(Integer.java:786) ~[?:?]
        at org.openhab.binding.gree.internal.GreeCryptoUtil.getEncryptionType(GreeCryptoUtil.java:97) ~[?:?]
        at org.openhab.binding.gree.internal.handler.GreeAirDevice.<init>(GreeAirDevice.java:81) ~[?:?]
        at org.openhab.binding.gree.internal.discovery.GreeDeviceFinder.scan(GreeDeviceFinder.java:117) ~[?:?]
        at org.openhab.binding.gree.internal.discovery.GreeDiscoveryService.startScan(GreeDiscoveryService.java:90) ~[?:?]
        at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:539) [?:?]
        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:1136) [?:?]
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635) [?:?]
        at java.lang.Thread.run(Thread.java:840) [?:?]

@lsiepel
Copy link
Contributor

lsiepel commented Sep 23, 2024

@gliter Please confirm this issue is introduced by this PR by using a 4.3.0M1 (or snapshot) version of the binding and see if that has the same issue or not. Otherwise it would be a regression or atleast a new unrelated bug, that should be recorded in a seperate issue (and PR)

Edit: I can't related this PR with the version exception.

@lovery
Copy link
Contributor Author

lovery commented Sep 23, 2024

@gliter @lsiepel
It looks like I had a bug in the previous version of the PR, where I was checking the version in the response of the device. I already remove this code, because I came across a log from a device with version later than 2 and using the old encryption.
And it looks like I forgot to update the upload jar file for test. I will upload a new one tomorrow, or if @gliter can build one from the changed code it will be great 😃

@lovery
Copy link
Contributor Author

lovery commented Sep 24, 2024

@gliter Thank you for testing! Can you try this jar and let me know how it goes?

@grzegorz-liter-getindata

@lovery thank you for quick explanation. Yes I will test it in the evening (UTC+2 timezones).

@gliter
Copy link

gliter commented Sep 24, 2024

@lovery unfortunetly it doesn't seem to work. It does discover the device and there are no more logs (on trace or higher level)
I don't see this log:

 logger.debug("Unable to bind to device - changing the encryption mode to GCM and trying again", e);

so I guess it still hangs on clientSocket.send(sendPacket);

Maybe instead of fallback to GCM the encryption type could be an option to be set in .things file?

Move code that changes the encType for binding with device to a broader
catch statement

Signed-off-by: Zhivka Dimova <[email protected]>
@lovery
Copy link
Contributor Author

lovery commented Sep 25, 2024

@gliter Thank you for your help, I really appreciate it! Can you try again, I've uploaded a new version of the jar file?
It turn out that the clientSocket.send function doesn't throw an exception for timeout, so I moved the code for changing the encryption type to the already existing catch.
I will also check how I can implement your suggestion, it is a good proposition.

@gliter
Copy link

gliter commented Sep 25, 2024

@lovery initially had some issues but it turned out the device was disabled in my OpenHab UI, even thou I configure everythng in files.
Anyway becasue of that I thought there is still problem with changing encryption by connetion timeout so I wrote some implementation to add it to config.

Feel free to incorporate it in your PR:
Patch: https://gist.github.com/gliter/22216484effe2fed53cc73829010d450
or PR: lovery#1

Probably some documentation have to be updated as well.

With those changes I was able to connect to new Gree device.

Add option to change the enctription type used for communicating with
the AC device by the user

Signed-off-by: Zhivka Dimova <[email protected]>
Copy link
Contributor

@lsiepel lsiepel left a comment

Choose a reason for hiding this comment

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

Left some textual suggestions.

Signed-off-by: Zhivka Dimova <[email protected]>
Signed-off-by: Zhivka Dimova <[email protected]>
Signed-off-by: Zhivka Dimova <[email protected]>
@lsiepel
Copy link
Contributor

lsiepel commented Oct 7, 2024

@lovery did you consider the PR / diff from @gliter ?

@gliter
Copy link

gliter commented Oct 7, 2024

@lsiepel yeah, my suggestions were incorporates so we can config encryption directly in config. I have approved PR.

Copy link
Contributor

@lsiepel lsiepel left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM

@lsiepel lsiepel merged commit 90442a3 into openhab:main Oct 7, 2024
5 checks passed
@lsiepel lsiepel added this to the 4.3 milestone Oct 7, 2024
@lovery lovery deleted the gree-support-v1.23 branch October 8, 2024 10:31
joni1993 pushed a commit to joni1993/openhab-addons that referenced this pull request Oct 15, 2024
* [gree]: use GCM encryption when binding fails

Signed-off-by: Zhivka Dimova <[email protected]>
matchews pushed a commit to matchews/openhab-addons that referenced this pull request Oct 18, 2024
* [gree]: use GCM encryption when binding fails

Signed-off-by: Zhivka Dimova <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An unexpected problem or unintended behavior of an add-on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[gree] Bind operation does not work with Gree Clivia
4 participants