fix(s7commplus): restore V1 challenge wire layout - #841
Conversation
|
|
|
@gijzelaerr we unfortunatelly don't have any PLC that supports the V1 protocol. I get: which means the protocol version is V2 (I assume). However, I tested the branch with the three S7-1500 plcs and they all worked for a browse + variable read. |
|
@xBiggs @bvanelli I pushed follow-up fixes for the captured V1 response handling: non-fatal V254 SystemEvents are now consumed while waiting for the matching sequence, and fragmented V3 responses have their per-fragment HMAC removed before reassembly. @xBiggs, could you please retest this branch on the original S7-1200 FW V4.2 reproduction ( Install: pip install --upgrade "python-snap7 @ git+https://github.com/gijzelaerr/python-snap7.git@fix-v1-sessionkey-challenge-layout"Local validation is green: 1,827 passed, 78 skipped, with the full pre-commit suite passing. |
|
|
|
|
|
The latest hardware results confirm that this PR fixes a real captured wire-layout regression and improves SystemEvent response handling, but it does not fully resolve #710. I have changed the closing keyword to #848 may change the request transport-flag mapping and #827 identifies additional reassembly/silent-skip cases. I am keeping this PR open until those interactions are reviewed and the resulting branch is retested against the available S7-1200/S7-1500 hardware. |
|
Merged current master without rebasing and addressed the remaining async parity cases: short InitSSL and CreateObject replies now raise S7ConnectionError, and async InitSSL uses the same 10-byte minimum as the sync path. Added regression coverage. Local build, full tests (1822 passed, 82 skipped), pre-commit, and the GitHub CI matrix pass. |
Summary
GET_VAR_SUBSTREAMEDchallenge-request layoutRoot cause
The final refactor in #775 reused the TLS/V2 substreamed request builder for the V1 SessionKey legitimation challenge. That changed the sequence field from a one-byte VLQ to a two-byte integer and changed the fill from three to four bytes. In the latest #710 capture, the S7-1200 accepts SessionKey setup and session activation, rejects this two-byte-longer challenge request with a V254 SystemEvent, and resets the TCP connection.
The corrected request matches the earlier capture that the same PLC accepted byte-for-byte. Follow-up testing also found that async framing and correlation failures used generic RuntimeError exceptions, allowing browse to misclassify them as valueless DBs. The sync and async paths now share the same typed error contract and bounded reconnect behavior.
This advances the handshake and fixes deterministic response-handling gaps, but does not resolve every S7-1200/S7-1500 session and data-access failure tracked in #710.
Validation
Hardware validation requested
@xBiggs @bvanelli could you please test this branch on your S7-1200 hardware, especially the original
s7commplus.Client().connect()plusbrowse()reproduction from #710? Adb_read()/db_write()check would also be useful if convenient.pip install --upgrade "python-snap7 @ git+https://github.com/gijzelaerr/python-snap7.git@fix-v1-sessionkey-challenge-layout"Progresses #710.