Skip to content

Tx sub states/v1.11#15657

Draft
victorjulien wants to merge 37 commits into
OISF:mainfrom
victorjulien:tx-sub-states/v1.11
Draft

Tx sub states/v1.11#15657
victorjulien wants to merge 37 commits into
OISF:mainfrom
victorjulien:tx-sub-states/v1.11

Conversation

@victorjulien

@victorjulien victorjulien commented Jun 16, 2026

Copy link
Copy Markdown
Member

SV_BRANCH=OISF/suricata-verify#3168

Work in progress branch on https://redmine.openinfosecfoundation.org/issues/8386

Defines 2 sub-states for HTTP2:

  • "stream" for the regular HTTP-style request/responses
  • "global" for the stream id 0 settings
    Each has it's own state machine.

Issues/open questions:

  • stream has a closed (data end of stream reached) and complete (both sides closed). This is because after the end of stream is reached, we can still receive window updates. But since this doesn't always happen, we often get no traffic on "complete". So a rule like accept:tx http2:stream:request_complete ... is not reliable. Not sure yet how to handle this.
  • should http.uri; absent; or http.uri; content:!"/index" causes matches on the global txs? I think no, but this is a behavior changes vs main.
  • I wonder if the DOH2 TX should be it's own sub type, perhaps even instead of a alproto?

For review:

  • overhauls DNS over HTTP/2: forces all keywords to explicitly register for DOH2.

TODO

  • complete registration logic
  • docs
  • commit cleanups
  • CI (probably)

@suricata-qa

suricata-qa commented Jun 16, 2026

Copy link
Copy Markdown

ERROR:
ERROR: ASAN TEST FAIL in ASAN_TLPR1_cfg QA test

ERROR: QA failed on ASAN_TLPR1_cfg.

Pipeline = 32077

@victorjulien victorjulien force-pushed the tx-sub-states/v1.11 branch from 10b8507 to b7e0e94 Compare June 17, 2026 07:07
@suricata-qa

suricata-qa commented Jun 17, 2026

Copy link
Copy Markdown

ERROR:
ERROR: ASAN TEST FAIL in ASAN_TLPR1_cfg QA test

ERROR: QA failed on ASAN_TLPR1_cfg.

Pipeline = 32080

victorjulien
victorjulien commented Jun 17, 2026
View reviewed changes
Comment thread rust/src/http2/detect.rs
});
//we do not expect more data from client
tx.progress_ts = HTTP2TxProgress::HTTP2ProgClosed;
if let HTTP2Progress::STREAM(ref mut stream_tx) = tx.progress {

@victorjulien victorjulien Jun 17, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

not introduced by this PR, but it's really icky that we modify the progress here from the detection path

@catenacyber catenacyber Jun 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is not really the detection path. This function should be put in http2.rs I guess

victorjulien
victorjulien commented Jun 17, 2026
View reviewed changes
Comment thread rust/src/http2/http2.rs Outdated
victorjulien
victorjulien commented Jun 17, 2026
View reviewed changes
Comment thread rust/src/http2/http2.rs Outdated
victorjulien
victorjulien commented Jun 17, 2026
View reviewed changes
Comment thread rust/src/http2/http2.rs Outdated
stream_tx.progress_tc = HTTP2TxProgress::HTTP2ProgHeaders;
}
} else {
panic!("global");

@victorjulien victorjulien Jun 17, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

TODO push promise might get here as well?

@catenacyber catenacyber Jun 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

push promise is not so nice anyways now, there is https://redmine.openinfosecfoundation.org/issues/7317 about it

victorjulien
victorjulien commented Jun 17, 2026
View reviewed changes
Comment thread rust/src/http2/http2.rs
child_stream_id: 0,
progress_tc: HTTP2TxProgress::HTTP2ProgStart,
progress_ts: HTTP2TxProgress::HTTP2ProgStart,
progress: HTTP2Progress::STREAM(HTTP2StreamProgress::init()),

@victorjulien victorjulien Jun 17, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

maybe init should be started or something, to reflect its initialized to that state

victorjulien
victorjulien commented Jun 17, 2026
View reviewed changes
Comment thread src/app-layer-parser.c Outdated
victorjulien
victorjulien commented Jun 17, 2026
View reviewed changes
Comment thread src/app-layer-parser.c Outdated
victorjulien
victorjulien commented Jun 17, 2026
View reviewed changes
Comment thread src/app-layer-parser.c Outdated
victorjulien
victorjulien commented Jun 17, 2026
View reviewed changes
Comment thread src/detect-engine-analyzer.c
SCJbSetBool(ctx.js, "is_mpm", app->mpm);
SCJbSetString(ctx.js, "app_proto", AppProtoToString(app->alproto));
SCJbSetUint(ctx.js, "progress", app->progress);
SCJbSetUint(ctx.js, "sub_state", app->sub_state);

@victorjulien victorjulien Jun 17, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

should be a string

victorjulien
victorjulien commented Jun 17, 2026
View reviewed changes
Comment thread src/detect-engine-file.h
const struct DetectEngineAppInspectionEngine_ *engine, const Signature *s, Flow *f,
uint8_t flags, void *_alstate, void *tx, uint64_t tx_id);

void DetectFileRegisterProto(

@victorjulien victorjulien Jun 17, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

should not be removed

victorjulien
victorjulien commented Jun 17, 2026
View reviewed changes
Comment thread src/detect-file-data.c
{ .alproto = ALPROTO_SMTP, .direction = SIG_FLAG_TOSERVER }, { .alproto = ALPROTO_UNKNOWN }
};

void DetectFileRegisterProto(

@victorjulien victorjulien Jun 17, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

undo removal

victorjulien
victorjulien commented Jun 17, 2026
View reviewed changes
Comment thread src/detect.h Outdated
@victorjulien victorjulien force-pushed the tx-sub-states/v1.11 branch from b7e0e94 to d97f799 Compare June 17, 2026 08:20
@suricata-qa

suricata-qa commented Jun 17, 2026

Copy link
Copy Markdown

ERROR:
ERROR: ASAN TEST FAIL in ASAN_TLPR1_cfg QA test

ERROR: QA failed on ASAN_TLPR1_cfg.

Pipeline = 32083

@victorjulien victorjulien force-pushed the tx-sub-states/v1.11 branch from d97f799 to 34909ea Compare June 17, 2026 14:04
@suricata-qa

suricata-qa commented Jun 17, 2026

Copy link
Copy Markdown

Information: QA ran without warnings.

Pipeline = 32086

@victorjulien victorjulien force-pushed the tx-sub-states/v1.11 branch from 34909ea to 7f5b89e Compare June 17, 2026 16:46
@suricata-qa

suricata-qa commented Jun 17, 2026

Copy link
Copy Markdown

Information: QA ran without warnings.

Pipeline = 32089

@victorjulien victorjulien force-pushed the tx-sub-states/v1.11 branch 2 times, most recently from 6faaedb to ce0d7af Compare June 17, 2026 20:34
@suricata-qa

suricata-qa commented Jun 17, 2026

Copy link
Copy Markdown

WARNING:

field baseline test %
SURI_TLPR1_stats_chk
.app_layer.flow.ftp_data 760 730 96.05%

Pipeline = 32091

@suricata-qa

suricata-qa commented Jun 18, 2026

Copy link
Copy Markdown

Information: QA ran without warnings.

Pipeline = 32101

@victorjulien victorjulien force-pushed the tx-sub-states/v1.11 branch from ce0d7af to 7fe8f33 Compare June 18, 2026 05:36
@suricata-qa

suricata-qa commented Jun 18, 2026

Copy link
Copy Markdown

Information: QA ran without warnings.

Pipeline = 32105

@victorjulien victorjulien force-pushed the tx-sub-states/v1.11 branch from 7fe8f33 to 615d387 Compare June 18, 2026 08:46
@codecov

codecov Bot commented Jun 18, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 91.93690% with 92 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.95%. Comparing base (09f0851) to head (cc83f7f).

Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #15657      +/-   ##
==========================================
- Coverage   82.96%   82.95%   -0.02%     
==========================================
  Files        1003     1003              
  Lines      275031   275666     +635     
==========================================
+ Hits       228192   228669     +477     
- Misses      46839    46997     +158
Flag Coverage Δ
fuzzcorpus 61.51% <73.53%> (+0.03%) ⬆️
livemode 18.43% <40.84%> (+0.05%) ⬆️
netns 22.85% <66.08%> (+0.10%) ⬆️
pcap 45.41% <63.10%> (+0.02%) ⬆️
suricata-verify 66.96% <90.44%> (+0.01%) ⬆️
unittests 58.45% <57.05%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

jasonish
jasonish reviewed Jun 18, 2026
View reviewed changes
Comment thread rust/src/http2/http2.rs Outdated
&& head.ftype != parser::HTTP2FrameType::GoAway as u8
{
self.set_event(HTTP2Event::InvalidFrameHeader);
input = rem;

@jasonish jasonish Jun 18, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In all other cases we use &rem[hlsafe..]. Is hlsafe always 0 in these cases?

@victorjulien victorjulien Jun 19, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Missed this, I've adopted the existing pattern here as well.

@victorjulien
detect/app-layer-event: support sub-state progress handling
02c5568
@victorjulien
output/tx: support substate completion flags
357cc23
@victorjulien
http2: split transaction state machines
dced315 
Split into 2 sub-states:
- stream, which has the "HTTP" requests and responses, including DOH2
- global, which has the settings and other global or control handling

Introduce a simpler progress tracking for the global sub state:
- HTTP2ProgGlobalStart and HTTP2ProgGlobalComplete.

The stream sub state uses the same state machine as before.

Ticket: OISF#8386.
@victorjulien
detect/parse: initial http2 substate support
99c05d3 
Hard coded for now.
@victorjulien
detect/file: register http/2 with sub-state
2edfe00
@victorjulien
detect/http2: don't double register engines
23d049b 
Each keyword supporting DOH2 must register explicitly
@victorjulien
detect/dns: register keywords for DOH2 as well
d6554cd 
Now that DNS keywords are no longer registered for DOH2, the keywords
need to manually registered for DOH2.
@victorjulien
detect/http: register keywords for HTTP/2
78df5f6 
Now that HTTP keywords are no longer automatically registered for
HTTP/2, register them manually.
@victorjulien
detect: change how DOH2 inspection works
5ebb455 
DNS/HTTP2 no longer automatically registers all keywords also for DOH2.

Instead, the DNS keywords and HTTP/2 stream keywords are registered for
DOH2 explicitly as well.

- flow alproto DOH2 + engine DOH2  -> inspect inner DNS
- flow alproto DOH2 + engine HTTP2 -> inspect outer HTTP/2
- flow alproto DOH2 + engine UNKNOWN -> inspect outer HTTP/2
@victorjulien
output/dns: update for DOH2 change
92306c5
@victorjulien
http2: mark HTTP2TxProgress and HTTP2TxGlobalProgress as AppLayerState
2eb8d73
@victorjulien
http2: register names for states per sub-state
4edccf8
@victorjulien
detect/firewall: policy for substates
55fab38 
For the most part hard coded for HTTP/2 for now.
@victorjulien victorjulien force-pushed the tx-sub-states/v1.11 branch from 1c85b5f to e89fafd Compare June 25, 2026 15:53
@victorjulien
detect/parse: parse sub state hooks
670dc2a 
Register a generic list for each sub state / progress combo.
@victorjulien
http2: set event on frame types not allowed on stream id
25eaf31
@victorjulien
http2: update push promise to account for stream id 0 filtering
63eb90c
@victorjulien
detect/parse: don't setup sub states if protocol doesn't support it
1f01828
@victorjulien
app-layer: for sub state API treat DOH2 as HTTP/2
47b236e 
Otherwise we'd have to re-register the relevant callbacks.
@victorjulien
detect/parse: allow bigger protocol/hook specifications
9f70b2a
@victorjulien
detect/firewall: implement app policy as hash table
7e7ac06 
With substate support the fixed table approach for policies was no
longer a good fit, so convert to a hash table. The policies are stored
per alproto, sub_state, progress and direction.
@victorjulien
detect/firewall: add default alert signature to policy object
ac22346 
Now that there is a policy hash, make the alert signature object a
member of that to avoid another hash table lookup.
@victorjulien
detect/mpm: don't register engines for disabled protocols
cc3ff6c
@victorjulien
detect: add debug validation checks
9dc8b68 
Check that sub state handling is correct.
@victorjulien
app-layer: use macro for progress ceiling
4cd96f6
@victorjulien victorjulien force-pushed the tx-sub-states/v1.11 branch from e89fafd to 4cd96f6 Compare June 25, 2026 19:36
@suricata-qa

suricata-qa commented Jun 26, 2026

Copy link
Copy Markdown

WARNING:

field baseline test %
SURI_TLPR1_stats_chk
.app_layer.flow.ftp_data 760 898 118.16%
.app_layer.error.ftp.parser 17 0 -

Pipeline = 32263

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

Reviewers

@jasonish jasonish jasonish left review comments
@catenacyber catenacyber catenacyber left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@victorjulien @suricata-qa @jasonish @catenacyber