Skip to content

Conversation

@claire-lex
Copy link
Contributor

This PR introduces a few changes to the KNXnet/IP layer, last updated in 2021:

  • Fix validation and support issues on KNX individual addresses and KNX group addresses (see below)
  • Add a few recognized KNX codes and supported types for MultipleTypeFields in complex packets
  • Add basic and KNX address-related unit tests

I made the PR mainly to fix issues when building packets containing individual address (format 1.1.1) or group address (format 1/1/1) fields. The layer only supported either individual address or group address in a field, but some fields (for instance, in cEMI blocks) can take both formats. For instance, the code below used to raise a ValueError and is now valid.

test_addr = CEMI(message_code=0x11) # L_Data.req
test_addr.cemi_data.address_type = 0 # Individual address
test_addr.cemi_data.destination_address = "1.1.1"
@guedou
Copy link
Member

guedou commented Feb 16, 2025

Thanks for the PR. Could you fix the flake8 issues at https://github.com/secdev/scapy/actions/runs/13262979518/job/37023594039?pr=4663 ?

@codecov
Copy link

codecov bot commented Mar 17, 2025

Codecov Report

❌ Patch coverage is 72.72727% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.87%. Comparing base (c15a670) to head (ddab133).
⚠️ Report is 140 commits behind head on master.

Files with missing lines Patch % Lines
scapy/contrib/knx.py 72.72% 9 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4663      +/-   ##
==========================================
- Coverage   81.55%   80.87%   -0.68%     
==========================================
  Files         359      368       +9     
  Lines       86557    90262    +3705     
==========================================
+ Hits        70592    73002    +2410     
- Misses      15965    17260    +1295     
Files with missing lines Coverage Δ
scapy/contrib/knx.py 90.57% <72.72%> (+8.07%) ⬆️

... and 111 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
@claire-lex
Copy link
Contributor Author

Done, it should be ok now! Sorry for the delay.
Thank you :)

Comment on lines 200 to 207
if self.structure_length is None:
p = struct.pack("!B", len(p)) + p[1:]
p = (len(p)).to_bytes(1, byteorder='big') + p[1:]
Copy link
Member

Choose a reason for hiding this comment

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

Why those changes?
I think I disagree.

Comment on lines 253 to 256
if self.structure_length is None:
p = struct.pack("!B", len(p)) + p[1:]
p = (len(p)).to_bytes(1, byteorder='big') + p[1:]
Copy link
Member

Choose a reason for hiding this comment

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

Same.

Comment on lines 287 to 288
if self.structure_length is None:
p = struct.pack("!B", len(p)) + p[1:]
p = (len(p)).to_bytes(1, byteorder='big') + p[1:]
Copy link
Member

Choose a reason for hiding this comment

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

Same (and in all following post_build)

@gpotter2
Copy link
Member

gpotter2 commented Sep 3, 2025

Hi @claire-lex, (btw great SSTIC presentation, if I guessed the username right).

Just a small bump regarding this PR. There are still a bunch of unaddressed changes (see my previous comments) that prevent us from going forward with this.
After a while, we might have to mark it as "draft" or close it, unless someone else picks it up.

Thanks

@claire-lex
Copy link
Contributor Author

Hi, yes it was me, thank you! Your presentation was really nice too :)

I will fix it but I still did not have the time to do it (properly) yet. I'm sorry for keeping this PR stuck, it was not supposed to stay like this so long...
As I will still not be able to do it for the next month or so, if you prefer to move it to drafts (or even close it) you can, and I will eventually ressucitate (or reopen) it.
Sorry again for the delay.
Claire

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

4 participants