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. |
…pagation # Conflicts: # CHANGES.md # s7commplus/connection.py
|
@Poseidonas @xBiggs You both recently exercised the V1 and response-error paths behind this change. Could you review the current head, especially the V1 challenge layout and the merged notification/SystemEvent receive path? The conflict resolution is now CI-green. |
|
Ran the parity suite from #827 against The SystemEvent path looks solid to me. I fuzzed Two things from my side. The three async divergences I listed earlier are still there, now at
The reason I keep raising these is the symptom underneath: A V1 question on the legitimation challenge. The sync side now builds the V1 shape through Same length, different ObjectQualifier encoding and a four-byte fill instead of three. The async client takes its protocol version from CreateObject and only forces V2 when TLS is active, so on a non-TLS S7-1200 it stays on V1, which is where this PR says the old shape gets rejected. Is the async path expected to reach legitimation on such a PLC? If it is, it looks like it wants the same treatment; if legitimation there is TLS-only in practice then this is nothing. I have no S7-1200 or S7-1500 here, so none of the above is hardware-confirmed. The wire layout itself, the V1 challenge bytes and the notification ordering are yours to verify against the captures. |
Co-Authored-By: Codex <noreply@openai.com>
|
Addressed the async error-contract findings in fe5d7d9.
On the V1 challenge question: the async client deliberately supports TLS legitimation only today. authenticate() rejects non-TLS sessions before _get_legitimation_challenge(), so the V1 SessionKey layout is not reachable through the async API. Full legacy async authentication remains tracked in #836 rather than being partially added here. Validation: 1,851 passed, 82 skipped; full pre-commit suite and source/wheel build pass. |
Co-Authored-By: Codex <noreply@openai.com>
…ct-20260908 # Conflicts: # s7commplus/connection.py # tests/test_s7_unit.py
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.