Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Http2 header 5780 v3 #8621

Closed
wants to merge 2 commits into from
Closed

Conversation

catenacyber
Copy link
Contributor

Link to redmine ticket:
https://redmine.openinfosecfoundation.org/issues/5780

Describe changes:

  • http2: rename http2.header to http.request_header and http.response_header as it is confusing with http.header

Replaces #8615 with added commit to add the HTTP1 functionality

I have lots of questions...

Should this be one commit ?
Even if it is renaming + adding HTTP1 functionality

Did I get right the use of HttpHeaderBuffer ?

Is src/detect-http-header.c the right file do put it ?

I removed the validation callback as it is HTTP2-specific
Should we just remove the escaping being done in HTTP2, and not be able to match differently on a HTTP2 header name having comma in it like x: y versus a HTTP2 header having x as its name and y as its value ?

How do we merger with S-V having some test with rules with the keyword being deprecated ?

Do we keep the deprecated keyword in 6 ?

suricata-verify-pr: 1153
OISF/suricata-verify#1153

@catenacyber
Copy link
Contributor Author

Commit-check pointing me to do only one bug commit ...

@codecov
Copy link

codecov bot commented Mar 23, 2023

Codecov Report

Merging #8621 (dd001e2) into master (416a780) will increase coverage by 0.14%.
The diff coverage is 53.36%.

❗ Current head dd001e2 differs from pull request most recent head e538a98. Consider uploading reports for the commit e538a98 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8621      +/-   ##
==========================================
+ Coverage   81.76%   81.90%   +0.14%     
==========================================
  Files         968      968              
  Lines      278546   278621      +75     
==========================================
+ Hits       227751   228218     +467     
+ Misses      50795    50403     -392     
Flag Coverage Δ
fuzzcorpus 64.34% <18.26%> (+0.26%) ⬆️
suricata-verify 59.67% <53.36%> (-0.03%) ⬇️
unittests 63.26% <18.26%> (-0.02%) ⬇️

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

Or http.response_header based on the direction

http2.header had a different behavior than http.header and this was
confusing.

Ticket: OISF#5780
So that it is generic for HTTP1 and HTTP2

Ticket: OISF#5780
@suricata-qa
Copy link

Information: QA ran without warnings.

Pipeline 12816

@suricata-qa
Copy link

WARNING:

field baseline test %
SURI_TLPW1_stats_chk
.tcp.rst 113267 145129 128.13%
TREX_GENERIC_stats_chk
.capture.kernel_drops 0 56 0.00

Pipeline 12817

@catenacyber
Copy link
Contributor Author

Replaced by #8775

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants