Implement KlapTransportV2 for new_klap#1580
Implement KlapTransportV2 for new_klap#1580ZeliardM wants to merge 35 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1580 +/- ##
==========================================
+ Coverage 93.22% 93.43% +0.21%
==========================================
Files 157 157
Lines 9815 9860 +45
Branches 1003 1010 +7
==========================================
+ Hits 9150 9213 +63
+ Misses 472 460 -12
+ Partials 193 187 -6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@rytilahti I haven't seen this before: 1 high Annotations Code scanning Use of a broken or weak cryptographic hashing algorithm on sensitive data is used in a hashing algorithm (SHA1) that is insecure. Is this because I modified this file and now it's looking at it? You had the code they flag already in your code, but I'm not sure why it's being flagged as a problem now. |
|
Ok, had my user test and the discovery and CLI tests were successful, but we're getting 400 error responses at first with handshake1 one or two times and then it works. Not sure what that's about yet, will keep poking around. |
|
@rytilahti Ok, here's where it gets weird. I found that it actually isn't a new KLAP, it still uses XOR. The issue isn't the actual transport, it's how it's selecting the transport. I will modify things so that in the device factory, it's still forcing XOR if the device has new_klap. It's a much easier fix and has been working consistently on my end. |
There was a problem hiding this comment.
Pull Request Overview
This PR implements KlapTransportV3 for the new_klap protocol by adding support for a new "new_klap" parameter throughout the discovery and device configuration system. The implementation allows devices that support this new KLAP variant to be properly detected and configured to use XorTransport instead of the standard KlapTransport.
Key changes:
- Added
new_klapparameter to device configuration and discovery classes - Implemented special handling in discovery protocol to finalize devices with new_klap capability
- Updated device factory to route new_klap devices to XorTransport instead of KlapTransport
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| kasa/deviceconfig.py | Added new_klap field to DeviceConnectionParameters with custom serialization |
| kasa/discover.py | Added new_klap support to discovery protocol with special finalization logic |
| kasa/device_factory.py | Added routing logic to use XorTransport for new_klap devices |
| tests/test_discovery.py | Added comprehensive test for new_klap discovery finalization behavior |
| tests/test_deviceconfig.py | Added test configuration and serialization test for new_klap |
| tests/test_device_factory.py | Added test case for new_klap protocol selection |
| devtools/dump_devinfo.py | Updated device info capture to include new_klap parameter |
Comments suppressed due to low confidence (1)
tests/test_cli.py:1
- [nitpick] The test logic has changed from using explicit assert_not_called() and assert_called() to comparing call counts. This approach is less clear about the expected behavior and makes the test harder to understand. Consider using more descriptive assertions or adding comments explaining why call counts are being compared.
import json
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
@rytilahti Ok, so you can understand what I did here. I managed to make it where during discovery, it adds a new_klap portion to the management schema if it exists. Then it checks if that exists and if so, it runs the XOR transport on the IOT protocol. Otherwise, it was trying to actually use the KLAP transport which even though it says KLAP it's XOR. This allows the device to initially be discovered, but the problem comes up where the KLAP discovery results don't allow for the segmented device types for IOT. So, I update the device one time, pull the sysinfo, then mutate that back to what a XOR discovery result would be and re-initialize the device from the sysinfo. This seems to be working quite well and reliably for me and the one user who has been testing this with me, @allenrabinovich. Old IOT and new SMART devices are working correctly for me as well. |
rytilahti
left a comment
There was a problem hiding this comment.
A couple of quick comments, also, please do not forget to change the title of this PR when this is ready for a review :-)
|
@rytilahti Ok, I think I got everything figured out. It is actually KlapTransportV2. I have modified everything to handle this and updated all of the test coverage as well. Everything looks good on my end as well. |
rytilahti
left a comment
There was a problem hiding this comment.
I cannot test this mysef, but it looks good on surface. I would suggest tidying it up a bit to keep the code more maintainable if and when new authentication methods pop up, but your call!
|
I tested this locally and it's working well so far. Thanks! |
|
@rytilahti @Hashcode Ok, I have added in the CLI --encrypt-type KLAPV2 for this and that should resolve the remaining CLI issues. Let me know what you think and if we're good, this should be good to merge now. |
|
Found a few errors while testing, this last commit should fix them. @Hashcode Again, give it a test and let me know. @rytilahti Let me know when you want to proceed with this one, I am still working on the TPAP implementation, I think it should be in a new version with this one. |
|
@ZeliardM Sorry for the delay. I was out all last week with the flu. From my tests, this solves the issues I was seeing: Thanks for the hard work on this! |
|
@Hashcode Excellent, thanks! @rytilahti This looks like it's good to go if you are. |
|
Need anymore testing on this? I use this module within MaaSPower https://github.com/gilesknap/maaspower (I'll sync with them once this is merged to a new version to update their project), but I can run the kasa commands on my utility Linux server as a webservice as I use the power commands within Canonical's MaaS (Metal as a Service) solution which manages my k8s master nodes in my home environment. Constantly heartbeats for power states - could be a good test? |
|
I pulled the PR and it's working for me in a test venv: |
| if issubclass(device_class, IotDevice): | ||
| info = await protocol.query(GET_SYSINFO_QUERY) | ||
| _perf_log(True, "get_sysinfo") | ||
| device_class = get_device_class_from_sys_info(info) |
There was a problem hiding this comment.
Having checks like this indicates usually a code smell. Looking at the discovery response in the fixture, should it not be enough for us to decide on the device class?
There was a problem hiding this comment.
So, the biggest problem here is that with new_klap devices, they give a different discovery_result than the other IOT devices. They give a discovery_result that looks like the SMART devices, so there is no sysinfo. You need to give a base device type to get the sysinfo, then do a full discovery based on that because of the way that kasa identifies the devices with SMARTPLUGSWITCH or SMARTBULB and we break them all out into individual device classes which from the new discovery_result, you base the class off the family which does not work for all of the possible device types for IOT.
There was a problem hiding this comment.
Maybe we need to update the dev_tool as well to handle the new discovery result more like the SMART devices for new_klap devices that are IOT?
There was a problem hiding this comment.
It's odd too, sometimes the device responds with new_klap, sometimes i doesn't...I need to see if there is a way I can force it to always use new_klap and if I can get a new fixture that looks correct.
There was a problem hiding this comment.
Ok, I got a good fixture for the HS300 device, it has the following discovery result:
"discovery_result": {
"error_code": 0,
"result": {
"device_id": "00000000000000000000000000000000",
"device_model": "HS300(US)",
"device_type": "IOT.SMARTPLUGSWITCH",
"factory_default": false,
"hw_ver": "2.0",
"ip": "127.0.0.123",
"mac": "F0-A7-31-00-00-00",
"mgt_encrypt_schm": {
"ANS": false,
"encrypt_type": "KLAP",
"http_port": 80,
"is_support_https": false,
"lv": 2,
"new_klap": 1
},
"obd_src": "tplink",
"owner": "00000000000000000000000000000000",
"protocol_version": 1
}
},
This causes problems when it's classified as an IOT device but uses the new discovery process, but I've gotten everything I think squared away here on this one as well. Give it a once over if you want to include it in the next release as well.
|
Any idea when this might make it to production? I had been using the patch in this post after each HomeAssistant update but after the latest 2026.2.3 the patch doesn't work anymore. |
test logI tested the PR and it's working with HS103 firmware 1.1.3. Reminder: after upgrading the plug to firmware 1.1.3, even correct credentials can trigger an authentication error. The fix was to unregister the plug from the app and reinitialize it. |
|
@genzj Thanks for the test and information. There are still some transient issues with Discover and how devices respond that can cause some issues but overall this fix works. I have several other PRs that I'm trying to get all worked on and merged before another release but I'm not sure when that will happen. |
Implementing KlapTranportV2 to handle IoT devices with newer firmware and a new_klap flag and all associated tests hopefully.
With this PR, I will get the user to pull the specific branch for testing on their end, and I will test on my end to make sure that the old stuff still functions as well.