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

wip: test fix for multiple rule match #794

Closed
wants to merge 1 commit into from
Closed

Conversation

jptosso
Copy link
Member

@jptosso jptosso commented May 15, 2023

In the past there used to be a bug that printed logs multiple times. Now we are not properly printing multiple matches for a single rule, for example:

SecRule REQUEST_URI|ARGS "@rx .*" \
    "id:930110,\
    phase:2,\
    block,msg:'message',\
    multiMatch"

Previous statement is only generating one message but it should generate two if there is a match

@jptosso jptosso requested a review from a team as a code owner May 15, 2023 14:38
@jptosso jptosso marked this pull request as draft May 15, 2023 14:38
@codecov
Copy link

codecov bot commented May 15, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.01 ⚠️

Comparison is base (94b4968) 81.96% compared to head (e0dd346) 81.96%.

Additional details and impacted files
@@            Coverage Diff             @@
##           v3/dev     #794      +/-   ##
==========================================
- Coverage   81.96%   81.96%   -0.01%     
==========================================
  Files         153      153              
  Lines        8256     8255       -1     
==========================================
- Hits         6767     6766       -1     
  Misses       1272     1272              
  Partials      217      217              
Flag Coverage Δ
default 78.24% <100.00%> (-0.01%) ⬇️
examples 25.81% <100.00%> (-0.01%) ⬇️
ftw 49.18% <100.00%> (-0.01%) ⬇️
ftw-multiphase 49.29% <100.00%> (-0.01%) ⬇️
tinygo 77.40% <100.00%> (-0.01%) ⬇️

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

Impacted Files Coverage Δ
internal/corazawaf/transaction.go 77.77% <100.00%> (-0.02%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@M4tteoP
Copy link
Member

M4tteoP commented May 15, 2023

Hey @jptosso, could you please take a look at #792 (Specifically the Example 5)? As part of a more extensive PR to fix logs, I did some tweaks also about multiple matches that might have conflicts with this PR.

Recapping the outputs generated:

Current Coraza

2023/05/15 17:09:33 [DEBUG] Rule matched tx_id="lLMoxSGTsGjlutEhfWX" rule_id=930110
[logError][emergency] [client "127.0.0.1"] Coraza: Warning. message [file "default.conf"] [line "9"] [id "930110"] [rev ""] [msg "message"] [data ""] [severity "emergency"] [ver ""] [maturity "0"] [accuracy "0"] [hostname ""] [uri "/?id=0"] [unique_id "lLMoxSGTsGjlutEhfWX"]
[client "127.0.0.1"] Coraza: Warning. message [file "default.conf"] [line "9"] [id "930110"] [rev ""] [msg "message"] [data ""] [severity "emergency"] [ver ""] [maturity "0"] [accuracy "0"] [hostname ""] [uri "/?id=0"] [unique_id "lLMoxSGTsGjlutEhfWX"]
matchedRules 1
log lines 1
messages 2 (matchData are iterated inside ErrorLog()

This PR

[logError][emergency] [client "127.0.0.1"] Coraza: Warning. message [file "default.conf"] [line "9"] [id "930110"] [rev ""] [msg "message"] [data ""] [severity "emergency"] [ver ""] [maturity "0"] [accuracy "0"] [hostname ""] [uri "/?id=0"] [unique_id "MAJgdfgcrAUqPkhfXBL"]
[client "127.0.0.1"] Coraza: Warning. message [file "default.conf"] [line "9"] [id "930110"] [rev ""] [msg "message"] [data ""] [severity "emergency"] [ver ""] [maturity "0"] [accuracy "0"] [hostname ""] [uri "/?id=0"] [unique_id "MAJgdfgcrAUqPkhfXBL"]
[logError][emergency] [client "127.0.0.1"] Coraza: Warning. message [file "default.conf"] [line "9"] [id "930110"] [rev ""] [msg "message"] [data ""] [severity "emergency"] [ver ""] [maturity "0"] [accuracy "0"] [hostname ""] [uri "/?id=0"] [unique_id "MAJgdfgcrAUqPkhfXBL"]
[client "127.0.0.1"] Coraza: Warning. message [file "default.conf"] [line "9"] [id "930110"] [rev ""] [msg "message"] [data ""] [severity "emergency"] [ver ""] [maturity "0"] [accuracy "0"] [hostname ""] [uri "/?id=0"] [unique_id "MAJgdfgcrAUqPkhfXBL"]
matchedRules 2
log lines 2
messages 4 (matchData are still iterated inside ErrorLog(), so we double them

PR #792

[logError][emergency] [client "127.0.0.1"] Coraza: Warning. message [file "default.conf"] [line "9"] [id "930110"] [rev ""] [msg "message"] [data ""] [severity "emergency"] [ver ""] [maturity "0"] [accuracy "0"] [hostname ""] [uri "/?id=0"] [unique_id "hXbxJwtsSOCuNEBczMd"][msg_match_1 "message"] [data_match_1 ""]
matchedRules 1
log lines 1
messages 1 (With the extra matches added as trailing elements)

The approach is a bit different. The idea with #792 is about writing in a more concise way the logs, considering that the matchedRule is one (always the same) but with multiple matches, printed as trailing payloads. This PR, considers each matched value as a completely new match, adding multiple time the same rule inside matchedRules and printing multiple times like a completely different match.

@jptosso
Copy link
Member Author

jptosso commented May 15, 2023

Thanks @M4tteoP ! I will wait for your PR to be merged

@jptosso jptosso closed this May 15, 2023
@jptosso jptosso deleted the fix-multiplematches branch May 15, 2023 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants