Tls states/v1a - #3370
Draft
victorjulien wants to merge 7 commits into
Draft
Tls states/v1a#3370victorjulien wants to merge 7 commits into
victorjulien wants to merge 7 commits into
Conversation
The TLS client states became active phases: the names changed (client_in_progress -> client_started, client_hello_done -> client_hello, client_cert_done -> client_cert, client_handshake_done -> client_data) and client_hello now starts at the first hello byte instead of hello completion. Update the state names in the affected rule files. The per-state default app policies under firewall.policies.app.tls resolve against the current state name table, so the policy config keys must follow the rename or they stop applying (states without a configured policy fall to the implicit default policy, drop). Update the client-track policy keys in the affected tests. With the policy keys applying again, the tests exercise their intended semantics: the SNI allow-list tests accept the flow when the SNI rule matches within the client_hello window, so the server-flight cert-chain drop remains the terminal event; the app-reject test rejects the flow in that window; and the all-accept tests accept every packet. Update the expectations accordingly.
The server TLS states become active phases: server_in_progress -> server_started, server_cert_done -> server_cert, server_handshake_done -> server_data, and the flight-completion state server_hello_done is dropped; rules that referenced it move to server_data, which now spans that window. Update the affected rule files. The firewall.policies.app.tls policy config keys resolve against the current state name table, so the tests that configure per-state policies for the server track must follow the renamed states or those policies stop applying (unconfigured states fall to the implicit default policy). Where the old flight-done and handshake-done states configured the same policy, they now map to a single server-data entry.
Two new tests for the active-phase TLS states, reusing existing tshark-validated pcaps: tls-sni-fragmented-hello: a client hello spanning three TCP records still yields the SNI exactly once, at the client_hello phase, and the tls log carries the SNI. Guards the fragment (record-buffer) path against the per-record phase writes. tls-sni-absent-terminality: a client hello without a SNI extension must close the client_hello window terminal - a rule requiring SNI counts zero while a same-state rule that can match counts one (the window was opened and evaluated), and the tls log has no sni key. The firewall decision-timing behaviour (state windows starting at the first hello byte, default policy sweep) is pinned by the updated firewall tests; a dedicated netns hook test was deemed redundant.
Two new firewall tests covering what a deployment sees after upgrading to the phase-based TLS states while rules/config still use the old names. 103: a firewall rule with a pre-phase state name fails to load with a "does not support hook" error (fatal under --init-errors-fatal, the framework's mode), and the two legacy firewall.policies keys are flagged with a rename hint; the hints pin the intended replacement names. 104: legacy firewall.policies keys are ignored with the hints, and the states fall back to the implicit default policy - the flow is dropped in the client_hello window (flow.action drop, 3 accepted / 59 blocked, drop_reason.default_app_policy 1) with no rules involved.
The client_hello window's no-match is terminal once the track advances past it, but nothing exercised what happens when a flow replays a ClientHello after the window closed (renegotiation): does the state machine regress, does the keyword engine get re-evaluated, and what does the parser do with a second SNI. New test with a crafted pcap (craft-pcap.py, tshark-validated): the tls-client-hello-frag-01 handshake plus a TLS 1.2 renegotiation exchange (ServerHello with renegotiation_info, ChangeCipherSpec, ClientHello with SNI reneg.example.com). Pins: the control rule matching the original SNI fires exactly once at client_hello - no re-alert when the renegotiation hello arrives, so the monotonic state machine did not reopen the window; a rule registered at client_hello and matching only the renegotiation SNI never fires - the signature is CANT_MATCH from the moment the track moved on; and the tls log keeps the first SNI (www.google.com), documenting that the parser does not replace the SNI buffer on a second hello (multiple-SNI extensions are skipped), so the suppression is the engine's terminality composed with the parser's keep-first SNI.
The rules files of these tests claim the flows get dropped ('no app
rules, so drop') while every check in all three tests pins that
nothing is dropped; the comments describe the opposite of the pinned
behavior.
Test 95's policy map listed http1 'request-started' twice and never
defined 'response-started', silently defeating its key-set
enumeration intent (duplicate YAML key collapses to one effective
key); the second entry becomes 'response-started'.
The analysis output pins the effective policy per state table, so its tls key references the state name table. On 9.x with the TLS phase state rename the key is client_hello; keep the pre-rename key under a lt-version 9 gate so the test still pins the older releases.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Ticket
If your pull request is related to a Suricata ticket, please provide
the full URL to the ticket here so this pull request can monitor
changes to the ticket status:
Redmine ticket: https://redmine.openinfosecfoundation.org/issues/