Add klap support for TAPO protocol by splitting out Transports and Protocols by sdb9696 · Pull Request #557 · python-kasa/python-kasa · GitHub

This PR adds KLAP support for the TAPO/SMART protocol. The way the encryption works is that the the original message that worked with #552 is instead encrypted with KLAP:

"method": method,
"params": params,
"requestID": self.request_id_generator.generate_id(),
"request_time_milis": round(time.time() * 1000),
"terminal_uuid": self.terminal_uuid,

As the current klapprotocol implementation assumes the old kasa style of request/response this PR proposes to separate the transport from the protocol. The changes are:

Introduce a base TPLinkTransport abstract class.
Rename TplinkKlap to TPlinkIotProtocol. It seems this is the distinction in the discovery result, device_type beginning with IOT. uses the old protocol, device_type beginning with SMART. uses the new.
Rename TPLinkAes to TPLinkSmartProtocol.
Break out klap logic from TPLinkKlap into a new TPLinkKlapTransport class.
Break out AES logic from TPLinkAes into a new TPLinkAesTransport class.
The klap handshake hashes are slightly different with the SMART. devices so there's a TPLinkKlapTransportV2 class derived from TPLinkKlapTransport that overrides the three hash functions.

A good starting point to understand the changes here should be the get_protocol_from_connection_name diff in device_factory.py

Codecov Report

Attention: 148 lines in your changes are missing coverage. Please review.

Comparison is base (347cbfe) 75.76% compared to head (63310d3) 79.50%.

Files	Patch %	Lines
kasa/aestransport.py	46.70%	97 Missing ⚠️
kasa/smartprotocol.py	78.57%	16 Missing and 8 partials ⚠️
kasa/klaptransport.py	85.26%	6 Missing and 8 partials ⚠️
kasa/iotprotocol.py	85.96%	4 Missing and 4 partials ⚠️
kasa/discover.py	75.00%	1 Missing and 2 partials ⚠️
kasa/device_factory.py	89.47%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #557      +/-   ##
==========================================
+ Coverage   75.76%   79.50%   +3.74%     
==========================================
  Files          34       36       +2     
  Lines        2876     2991     +115     
  Branches      751      777      +26     
==========================================
+ Hits         2179     2378     +199     
+ Misses        640      541      -99     
- Partials       57       72      +15

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Just a couple of very brief comments, mostly about things to make it easier to review.

Tests have now been added and the test framework is working with the new tapo devices. I will add more tests but wanted to get this in to ease review.

That's quite a bit of code to review and test, but here's the first round of comments. In general, separating the concerns of the encryption and the transport sounds good to me 👍

A couple of things:

Update the PR title to be more descriptive.
Class naming: let's try to avoid useless TPLink*, Smart etc. prefixes where feasible, that pattern came from the pyhs100 project to keep backwards compatibility, but we should not introduce any more of them.
Using a self-created str<->(protocol, transport) mapping feels wrong and potentially error-prone when new versions will be deployed. I think we should decide soon what's a good abstraction for the discovery-like information to device instances..

I'm wondering if it would be better to run these tests on all devices for now, just to keep the interfaces consistent.

In the future, it makes sense to separate common interfaces as discussed elsewhere to avoid mixing implementation & interfaces like we currently do by inheriting from Smart* for the new devices.

Thanks for the review @rytilahti. I know it was a lot and I thought hard about how to make it smaller but couldn’t come up with anything that made much sense.

Most of the comments are agreeable and doable and I will hopefully get sorted Monday. There is one question I have to clarify with you first. I restricted these tests with @device_iot as a short term measure to keep the PR from getting bigger (yes I know :-). Currently too many tests access sysinfo and break.

So I think we have two options:

Move all the SMART fixtures into a different directory so they don’t run on all tests.
Restrict them for now with @device_iot and then gradually re-enable test_file by test_file as we introduce the interfaces.

I prefer 2. because it will mean we have a single test suite and a good interface. What do you think?

Hmm, so those tests that test for specific payloads (like those schema checks in test_smartdevice) should only be tested against "device_iot"/"legacy" payloads, and indeed worth guarding for the time being.

Even then, we probably want to keep the "smart" fixtures separated from the legacy ones, so creating a new directory to host them from the get-go is a good idea if that's simple to achieve. If not, let's keep it simple for the time being and revisit it later.

But maybe the more generic ones (like testing for state_attributes structure, tests for cli outputs, ..) could still test everything if doing so doesn't require too many changes?

HI @rytilahti, I had a bit of time today to address this review. There's still work needed on the test framework but I think it will be better to perfect that in further PRs. After we get this in we can hopefully start to focus on separate streams, i.e. TAPO PRs, test framework PRs, test coverage PRs, fixture PRs and interface PRs. I hope we have the baseline now :)

This is looking good, I added some extra comments on top of the few already existing unresolveds. @sdb9696 take a look and see what makes sense to fix prior merging this so that we can start again having smaller PRs :-)

LGTM, let's get this merged. Thanks @sdb9696! 🥇

This is looking good, I added some extra comments on top of the few already existing unresolveds. @sdb9696 take a look and see what makes sense to fix prior merging this so that we can start again having smaller PRs :-)

Hi @rytilahti , I've addressed most of the comments from today and the open ones I missed from the other day (for some reason they keep becoming hidden for me in github).

If we're good to merge this now I'll go through afterwards and collate all of the "Subsequent PR" items we discussed and add them to the discussion so we don't lose sight of them.

I've addressed most of the comments from today and the open ones I missed from the other day (for some reason they keep becoming hidden for me in github).

I think github sometimes loses track of things when doing force pushes but it may be just a feeling. :-)

…otocols (python-kasa#557) * Add support for TAPO/SMART KLAP and seperate transports from protocols * Add tests and some review changes * Update following review * Updates following review

github-advanced-security AI found potential problems Nov 29, 2023

View reviewed changes

sdb9696 mentioned this pull request Nov 29, 2023

Add support for the protocol used by TAPO devices and some newer KASA devices. #552

Merged

sdb9696 force-pushed the add_tapo_klap branch 2 times, most recently from dd5c584 to f28ff9b Compare November 30, 2023 12:17

sdb9696 mentioned this pull request Nov 30, 2023

Add klap lv2 #550

Closed

sdb9696 marked this pull request as ready for review November 30, 2023 12:20

rytilahti reviewed Nov 30, 2023

View reviewed changes

Comment thread devtools/dump_devinfo.py

Comment thread kasa/aestransport.py Outdated

Comment thread kasa/tests/fixtures/HS100(UK)_1.0_1.2.6.json Outdated

sdb9696 requested a review from rytilahti December 1, 2023 15:19

rytilahti added the enhancement New feature or request label Dec 1, 2023

This was referenced Dec 1, 2023

Kasa KP125M basic emeter support #560

Merged

Update dump_devinfo to produce new TAPO/SMART fixtures #561

Merged

rytilahti mentioned this pull request Dec 2, 2023

Add P110 fixture #562

Merged

rytilahti requested changes Dec 2, 2023

View reviewed changes

rytilahti mentioned this pull request Dec 3, 2023

Add tapo L530 fixture #563

Closed

sdb9696 force-pushed the add_tapo_klap branch from c097f8f to 404e008 Compare December 3, 2023 18:21

github-advanced-security AI found potential problems Dec 3, 2023

View reviewed changes

sdb9696 changed the title Add tapo klap Add klap support for TAPO protocol by splitting out Transports and Protocols Dec 3, 2023

sdb9696 requested a review from rytilahti December 3, 2023 18:36

sdb9696 force-pushed the add_tapo_klap branch from 404e008 to 53ac5ca Compare December 3, 2023 18:56

rytilahti requested changes Dec 4, 2023

View reviewed changes

sdb9696 added 4 commits December 4, 2023 18:22

Add support for TAPO/SMART KLAP and seperate transports from protocols

e26b6a1

Add tests and some review changes

30effec

Update following review

90bf240

Updates following review

63310d3

sdb9696 force-pushed the add_tapo_klap branch from 53ac5ca to 63310d3 Compare December 4, 2023 18:22

github-advanced-security AI found potential problems Dec 4, 2023

View reviewed changes

rytilahti approved these changes Dec 4, 2023

View reviewed changes

sdb9696 requested a review from rytilahti December 4, 2023 18:45

rytilahti merged commit 4a00199 into python-kasa:master Dec 4, 2023

sdb9696 deleted the add_tapo_klap branch December 6, 2023 19:28

This was referenced Jan 4, 2024

Need to do error code checking for new protocols #612

Closed

Need to create common interfaces for legacy and new devices #613

Closed

sdb9696 mentioned this pull request Jan 20, 2024

Add protocol and transport documentation #663

Merged

+                  @staticmethod
+                  def create_key_pair(key_size: int = 1024):
+                      """Create a key pair."""
+                      private_key = rsa.generate_private_key(public_exponent=65537, key_size=key_size)

-              class TPLinkKlap(TPLinkProtocol):
+              def _sha1(payload: bytes) -> bytes:
+                  digest = hashes.Hash(hashes.SHA1())  # noqa: S303
+                  digest.update(payload)

+              def md5(payload: bytes) -> bytes:
+                  """Return an md5 hash of the payload."""
+                  digest = hashes.Hash(hashes.MD5())  # noqa: S303
+                  digest.update(payload)

+              def _sha1(payload: bytes) -> str:
+                  sha1_algo = hashlib.sha1()  # noqa: S324
+                  sha1_algo.update(payload)

Add klap support for TAPO protocol by splitting out Transports and Protocols by sdb9696 · Pull Request #557 · python-kasa/python-kasa · GitHub

Repository navigation

Add klap support for TAPO protocol by splitting out Transports and Protocols - #557

Add klap support for TAPO protocol by splitting out Transports and Protocols#557rytilahti merged 4 commits intopython-kasa:masterpython-kasa/python-kasa:masterfrom sdb9696:add_tapo_klapsdb9696/python-kasa:add_tapo_klapCopy head branch name to clipboard

Conversation

sdb9696 commented Nov 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Check failure

codecov Bot commented Nov 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

rytilahti left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sdb9696 commented Dec 1, 2023

Uh oh!

rytilahti left a comment

Choose a reason for hiding this comment

Uh oh!

rytilahti Dec 2, 2023

Choose a reason for hiding this comment

Uh oh!

sdb9696 Dec 2, 2023

Choose a reason for hiding this comment

Uh oh!

rytilahti Dec 2, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Check failure

Check failure

sdb9696 commented Dec 3, 2023

Uh oh!

rytilahti left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Check failure

rytilahti left a comment

Choose a reason for hiding this comment

Uh oh!

sdb9696 commented Dec 4, 2023

Uh oh!

rytilahti commented Dec 4, 2023

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Add klap support for TAPO protocol by splitting out Transports and Protocols#557
rytilahti merged 4 commits into
python-kasa:masterfrom
sdb9696:add_tapo_klap

sdb9696 commented Nov 29, 2023 •
edited
Loading

codecov Bot commented Nov 29, 2023 •
edited
Loading