Implement KlapTransportV2 for new_klap #1580
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1580 +/- ##
==========================================
+ Coverage 92.82% 93.06% +0.23%
==========================================
Files 157 158 +1
Lines 9649 9712 +63
Branches 976 985 +9
==========================================
+ Hits 8957 9038 +81
+ Misses 492 480 -12
+ Partials 200 194 -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. |
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. |
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 How would you like to handle the CLI issues with this? Looking at the code for the CLI, it seems like the devices that use new_klap are older legacy iot devices but it makes them use the newer connection methods used by smart devices. It seems like the way the legacy devices are handled in the CLI and this new way are conflicting. |
|
Ok, I will also have to look at updating some other pieces and tests to handle the new fixture files that are created with this new_klap feature as I did not initially and now see some problems with those as well. I will look over everything that came back in testing today and try to update everything here accordingly. |
|
@Hashcode I think I am close to getting this resolved. I will try to get everything pushed out tomorrow for you to test if you don't mind. @rytilahti I added the test fixture here in this PR instead of a separate one because there were changes with the discover process and everything that were done with the new_klap that were required for the tests to work correctly with the fixture so I figured it would be best to have everything in here. I am making changes to the discover process as well, I want you to take a look and let me know. It may require some changes for contributing guidelines. |
Sounds great! ping me whenever you're ready and I'll give it a go. |
|
Ok, looks good on the tests and coverage. @Hashcode Give this new stuff a shot and let me know. @rytilahti If the testing by Hascode works out, then I believe we are finally good to go on this one. |
Hi @ZeliardM , vs. w/o the strip type: I did clean out the .venv files and ran uv cache clean to try and make sure I'm running the current branch from feature/new-klap. The tip SHA is: e9fc5dc ("Update discover and tests for new fixtures") Let me know what kind of debug information you'd like. |
|
@Hashcode If you can pull the debug for the failed command, may help in finding where it's failing for me. The other commands that were failing are working now? |
Discovery works great (and was working in the previous version of the PR). The connection 111 error happens when |
|
@ZeliardM When I use the NOTE: I see |
|
@ZeliardM Ah, I think we are missing a way of setting If we have that then I can run a command like this to avoid discovery: I tested this w/ a small diff to see if it worked: |
|
@Hashcode Ok, now I know where I need to look. Let me poke around a little more. Thanks! |
|
@Hashcode @rytilahti Ok, so after looking through everything, the issue is new_klap for sure. The --type flag does not any discover parts to query the device to get the connection parameters, it builds them based on the type sent and then uses them to get the protocol, the problem is that new_klap could be on different types of devices and has up until now been determined after querying the device. I will look and see if there is a way to do so inside of all of this to get new_klap before we send the connect piece and let you know. |
|
@Hashcode @rytilahti Ok, so, the only way I can figure out how to do this without initially discovering the device via the IP and then pulling if it has new_klap is to have the user specify the encrypt_type as well. The user would use "KLAPV2" and that would be flagged as new_klap for the DeviceConnectionParameters and that would allow the device to connect like it's supposed to. I am open to any other thought, questions, or concerns here. |
That's pretty much what I expected. Sounds good from my end. |
|
I also think that's fair, if I were to guess, this naming was not even supposed to be mainlined, so it might only concern very few devices and users. We can also always revise later, if necessary :-) |
|
Ok, sounds good. I will see if there is a way to add something about failing to connect to try KLAPV2 for encrypt_type, will see what I can put together and push out. |
|
@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. |
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.